Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dual-funding: final final final final?? #6391

Merged
merged 4 commits into from
Jul 13, 2023

Conversation

niftynei
Copy link
Contributor

@niftynei niftynei commented Jul 7, 2023

What if we finished off the dual-funding spec??

This PR adds/updates the following. Note that it's missing/still needs the check for the next_funding_txid, which I'm hoping to get up soon ™️ .

  • converts the funding_contribution from an unsigned to a signed int. This is future-proofing for a world where splicing exists.
  • Changes the way we're serializing witnesses from a per-element structure to "take the entire blob and put it in the wire message". This is particularly annoying for us as libwally wants each element separately, so we end up doing some extra serialization/deserialization to keep libwally happy.
  • Sends the next_funding_txid if we remember the peer on reconnect + haven't received their tx-sigs yet. Needs a little bit of work.

@niftynei niftynei added this to the v23.08 milestone Jul 7, 2023
@niftynei niftynei requested a review from cdecker as a code owner July 7, 2023 06:16
@niftynei niftynei requested a review from ddustin July 7, 2023 06:16
@niftynei niftynei force-pushed the nifty/df-final-final branch 4 times, most recently from 0d6d5d6 to 92e8df4 Compare July 7, 2023 22:11
ddustin and others added 4 commits July 7, 2023 19:00
We're adding signed types to the spec! This adds the support mechanisms
for them.
the witnesses are maddeningly weird now (you concat everything together)

we also changed some things to be s64's (it's a teeny tiny change)
As per lightning/bolts@cd3c99e
we should send the next_funding_txid if we've sent our commitment sigs,
but we haven't received the peer's tx_signatures.

Note that we send here, but don't verify that it's arrived.
@niftynei niftynei force-pushed the nifty/df-final-final branch from 92e8df4 to c235f60 Compare July 8, 2023 00:05
*/
tlvs = tlv_channel_reestablish_tlvs_new(tmpctx);
if (!tx_state->remote_funding_sigs_rcvd)
tlvs->next_funding = &tx_state->funding.txid;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to tal_steal or tal a copy onto tlvs? Just so we don't accidentally free the pointee.

Copy link
Member

@cdecker cdecker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely about time other implementations start looking at DF!

ACK c235f60

@rustyrussell rustyrussell merged commit e366c19 into ElementsProject:master Jul 13, 2023
const u8 *data = witness->witness_data;
size_t size, max = tal_count(data);
bool ok;

wally_tx_witness_stack_free(in->final_witness);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@niftynei @rustyrussell You need to set this to NULL after freeing it, otherwise if line 72 returns you have a dangling pointer and potential later use-after-free here. Alternately use wally_psbt_set_input_final_witness(NULL) which will free any existing witness and set the value to NULL.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the note! Just added a PR to fix this.

@jgriffiths
Copy link
Contributor

@niftynei wally now exposes witness se/serialization using the psbt-spec format as used here: #6882 specifically fedfe6f

If I understand your change now uses that same serialization format? If so then your manual serialization can be removed once that PR is rebased and merged (has tal issues I need to look at in the last 2 commits).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants