Dual funding and interactive tx construction wire messages #1794

dunxen · 2022-10-21T20:17:57Z

The first of a set of PRs to enable dual-funded channels using the interactive transaction construction workflow. This PR adds the messages and events required by the protocol to get that out of the way for the review of upcoming logic PRs.

ariard

Primarly just the dual-funding messages

lightning/src/ln/msgs.rs

TheBlueMatt

We should get some encoding/decoding tests. Are you looking to land this as-is prior to implementing the dual-funding stuff, or do you want to hold this PR until you have an implementation sketch?

lightning/src/ln/msgs.rs

dunxen · 2022-10-25T06:16:21Z

We should get some encoding/decoding tests. Are you looking to land this as-is prior to implementing the dual-funding stuff, or do you want to hold this PR until you have an implementation sketch?

Working on encoding tests for this PR. Yeah, my idea was to get this up and then just base the sketch on this PR. Although, I don't see any reason we can't land just the message stuff first as long as we don't actually handle them.

codecov-commenter · 2022-10-26T10:49:37Z

Codecov Report

Patch coverage: 42.22% and project coverage change: -0.58 ⚠️

Comparison is base (56b0c96) 91.63% compared to head (40a7a56) 91.05%.

❗ Current head 40a7a56 differs from pull request most recent head 2ace882. Consider uploading reports for the commit 2ace882 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1794      +/-   ##
==========================================
- Coverage   91.63%   91.05%   -0.58%     
==========================================
  Files         104      104              
  Lines       51985    52305     +320     
  Branches    51985    52305     +320     
==========================================
- Hits        47638    47628      -10     
- Misses       4347     4677     +330

Impacted Files	Coverage Δ
lightning-net-tokio/src/lib.rs	`75.76% <0.00%> (-2.85%)`	⬇️
lightning/src/events/mod.rs	`41.64% <0.00%> (-2.35%)`	⬇️
lightning/src/ln/channel.rs	`90.02% <ø> (+0.05%)`	⬆️
lightning/src/ln/channelmanager.rs	`88.80% <ø> (ø)`
lightning/src/ln/functional_test_utils.rs	`92.65% <0.00%> (-0.14%)`	⬇️
lightning/src/ln/peer_handler.rs	`57.43% <0.00%> (-5.15%)`	⬇️
lightning/src/ln/wire.rs	`56.84% <0.00%> (-7.05%)`	⬇️
lightning/src/util/test_utils.rs	`72.38% <0.00%> (-4.68%)`	⬇️
lightning/src/util/ser.rs	`88.23% <42.85%> (-0.21%)`	⬇️
lightning/src/ln/msgs.rs	`83.39% <67.73%> (-5.32%)`	⬇️

... and 23 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

TheBlueMatt · 2022-10-27T16:26:22Z

Working on encoding tests for this PR. Yeah, my idea was to get this up and then just base the sketch on this PR. Although, I don't see any reason we can't land just the message stuff first as long as we don't actually handle them.

We could also pipe them through the ChannelMessageHandler stuff, and just not expose the feature bit and ignore/error on the messages in ChannelManager itself. That would make the PR a bit more robust without having to add any specific logic.

lightning/src/ln/peer_handler.rs

TheBlueMatt · 2022-11-03T16:53:04Z

Oops, bad timing, looks like this needs rebase now.

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2022-11-03T16:55:18Z

General question - how likely is the spec to change at this point? Is it pretty fixed at this point or is it likely to change out from under us?

dunxen · 2022-11-03T17:27:52Z

General question - how likely is the spec to change at this point? Is it pretty fixed at this point or is it likely to change out from under us?

As far as I know, the messages seem quite fixed now after some small tweaks. There's just some discussion on ordering of inputs/outputs, although that may have be en resolved already. No rush for this to be reviewed and merged, but it is in a better state at least. I'd give it a little longer, maybe a topic for the next spec meeting.

TheBlueMatt · 2022-11-03T17:37:10Z

Alright, sounds good. Let's not merge this until we're confident things are fixed, but happy to land it when you think we're good.

wpaulino · 2023-01-12T22:14:39Z

Any updates @dunxen?

dunxen · 2023-01-13T05:53:32Z

Any updates @dunxen?

Just waiting on feedback on the est_witness_weight field in the spec which will probably be removed anyway. This is ready for review though.

TheBlueMatt

Didn't bother verifying the messages match the BOLT changes, but probably wont.

lightning/src/ln/functional_test_utils.rs

lightning/src/ln/msgs.rs

alecchendev

Mainly just looked through the new message structs and how they're serialized and compared them to the spec PR, noted anything that stood out to me :)

lightning/src/ln/msgs.rs

TheBlueMatt · 2023-02-14T21:20:40Z

Looks like the BOLT PR is ~ready to land? So we should be able to move forward here, right?

lightning/src/util/ser.rs

lightning/src/ln/channel.rs

lightning/src/ln/msgs.rs

fuzz/targets.h

wpaulino · 2023-05-02T22:03:56Z

Needs a rebase now that the dependent PR has landed.

wpaulino

LGTM, just two minor comments regarding witness serialization.

lightning/src/util/ser.rs

lightning/src/ln/msgs.rs

TheBlueMatt · 2023-05-04T20:35:14Z

lightning/src/ln/msgs.rs

+	/// The channel type that this channel will represent. If none is set, we derive the channel
+	/// type from the intersection of our feature bits with our counterparty's feature bits from
+	/// the Init message.
+	pub channel_type: Option<ChannelTypeFeatures>,


🤮 lets make this required in the spec.

I'll bring it up. So required for V2 channels?

Ok so I think with next release for each implementation, we want to target making this required, outside of the dual-fund spec: lightning/bolts#851 (comment)

Mmm, I haven't tried to look at what % of nodes support channel types, we're probably getting to the point where we can require it but maybe not quite yet. I'm not really I sure I understand tbast's point there but we can take it to that issue and leave it optional here for now.

lightning/src/ln/msgs.rs

This is the first of a set of PRs to enable the experimental dual-funded channels feature using interactive transaction construction. This allows both the channel initiator and channel acceptor to contribute funds towards the channel.

TheBlueMatt

All the comments can be addressed in a followup or left (or now), up to you.

TheBlueMatt · 2023-05-05T16:27:25Z

lightning/src/ln/msgs.rs

+///
+/// Use [`TransactionU16LenLimited::into_transaction`] to convert into the contained `Transaction`.
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub struct TransactionU16LenLimited(Transaction);


nit: I think this maybe belongs in ser.rs.

TheBlueMatt · 2023-05-05T16:27:49Z

lightning/src/ln/msgs.rs

+impl Readable for TransactionU16LenLimited {
+	fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
+		let len = <u16 as Readable>::read(r)?;
+		let mut tx_reader = FixedLengthReader::new(r, len as u64);


I think we should check that the reader has no more bytes after reading a transaction. Its not a huge deal, but nice to enforce.

TheBlueMatt · 2023-05-05T16:34:16Z

lightning/src/ln/msgs.rs

+	pub serial_id: u64,
+}
+
+/// A tx_complete message signalling the conclusion of a peer's transaction contributions during


TIL signaling has two ls in british english.

Oh true. My mind reads both as valid, but my linguistics write impl does British lol

TheBlueMatt · 2023-05-05T16:36:16Z

lightning/src/ln/msgs.rs

@@ -567,7 +850,7 @@ impl NetAddress {
 }

 impl Writeable for NetAddress {
-	fn write<W: Writer>(&self, writer: &mut W) -> Result<(), io::Error> {
+fn write<W: Writer>(&self, writer: &mut W) -> Result<(), io::Error> {


sigh, yeah :/
Yeah copied signature over.

dunxen · 2023-05-05T16:57:36Z

All the comments can be addressed in a followup or left (or now), up to you.

Will address as followup if that's okay. Out at the moment. Probably good to have the fix in for the fuzz sooner than later.

optout21 · 2023-08-31T21:28:03Z

lightning/src/events/mod.rs

+		/// The node_id of the node which should receive this message
+		node_id: PublicKey,
+		/// The message which should be sent.
+		msg: msgs::TxAddInput,


Looks like a typo -- TxAddInput instead of TxAbort (will create a PR, as this is merged)

ariard added the Review club label Oct 21, 2022

ariard reviewed Oct 23, 2022

View reviewed changes

lightning/src/ln/msgs.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed Oct 25, 2022

View reviewed changes

lightning/src/ln/msgs.rs Outdated Show resolved Hide resolved

lightning/src/ln/msgs.rs Outdated Show resolved Hide resolved

lightning/src/ln/msgs.rs Outdated Show resolved Hide resolved

dunxen force-pushed the 2022-10-dualfunding-act-1 branch 2 times, most recently from 5ff4b1f to e8b095f Compare October 27, 2022 13:01

TheBlueMatt removed the Review club label Oct 27, 2022

dunxen force-pushed the 2022-10-dualfunding-act-1 branch from e8b095f to ff23ca9 Compare November 1, 2022 14:12

TheBlueMatt reviewed Nov 2, 2022

View reviewed changes

lightning/src/ln/peer_handler.rs Outdated Show resolved Hide resolved

dunxen force-pushed the 2022-10-dualfunding-act-1 branch 2 times, most recently from 756064a to a788fcd Compare November 3, 2022 13:52

dunxen changed the title ~~DRAFT: Dual funding messages~~ Dual funding messages Nov 3, 2022

dunxen marked this pull request as ready for review November 3, 2022 13:54

TheBlueMatt reviewed Nov 3, 2022

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

dunxen force-pushed the 2022-10-dualfunding-act-1 branch from a788fcd to fb9706f Compare November 9, 2022 12:51

dunxen force-pushed the 2022-10-dualfunding-act-1 branch from fb9706f to 43f2e77 Compare December 23, 2022 13:58

dunxen force-pushed the 2022-10-dualfunding-act-1 branch from 43f2e77 to 78c8fe7 Compare January 11, 2023 09:31

TheBlueMatt reviewed Jan 13, 2023

View reviewed changes

alecchendev reviewed Jan 24, 2023

View reviewed changes

dunxen mentioned this pull request Feb 3, 2023

interactive-tx: Add dual-funding flow, using the interactive tx protocol (feature 28/29) lightning/bolts#851

Merged

dunxen force-pushed the 2022-10-dualfunding-act-1 branch from 78c8fe7 to dc9b02a Compare February 6, 2023 12:41

dunxen force-pushed the 2022-10-dualfunding-act-1 branch from 66f5adc to 645b337 Compare April 25, 2023 09:55

wpaulino reviewed Apr 25, 2023

View reviewed changes

lightning/src/util/ser.rs Show resolved Hide resolved

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

lightning/src/ln/msgs.rs Outdated Show resolved Hide resolved

lightning/src/ln/msgs.rs Show resolved Hide resolved

fuzz/targets.h Outdated Show resolved Hide resolved

dunxen force-pushed the 2022-10-dualfunding-act-1 branch from 645b337 to 40a7a56 Compare April 26, 2023 10:57

dunxen added the blocked on dependent pr label May 1, 2023

dunxen mentioned this pull request May 1, 2023

Remove OptionalField and make DataLossProtect fields mandatory #2253

Merged

wpaulino removed the blocked on dependent pr label May 2, 2023

dunxen force-pushed the 2022-10-dualfunding-act-1 branch 2 times, most recently from 223b799 to bce24b6 Compare May 3, 2023 08:55

wpaulino reviewed May 3, 2023

View reviewed changes

lightning/src/util/ser.rs Outdated Show resolved Hide resolved

lightning/src/util/ser.rs Outdated Show resolved Hide resolved

dunxen force-pushed the 2022-10-dualfunding-act-1 branch from bce24b6 to 5cd41ed Compare May 4, 2023 11:18

dunxen changed the title ~~Dual funding messages~~ Dual funding and interactive tx construction wire messages May 4, 2023

wpaulino previously approved these changes May 4, 2023

View reviewed changes

TheBlueMatt reviewed May 4, 2023

View reviewed changes

wpaulino mentioned this pull request May 4, 2023

Move channel_reestablish messages to the simple message fuzzer #2266

Closed

dunxen dismissed wpaulino’s stale review via ddfddcf May 5, 2023 09:27

dunxen force-pushed the 2022-10-dualfunding-act-1 branch from 5cd41ed to ddfddcf Compare May 5, 2023 09:27

dunxen added 3 commits May 5, 2023 11:40

Add new wire messaging and events but don't handle them

4e9a358

Add & generate dual-funding message fuzzing

2ace882

dunxen force-pushed the 2022-10-dualfunding-act-1 branch from ddfddcf to 2ace882 Compare May 5, 2023 09:41

TheBlueMatt approved these changes May 5, 2023

View reviewed changes

wpaulino approved these changes May 8, 2023

View reviewed changes

wpaulino merged commit 4062695 into lightningdevkit:main May 8, 2023

dunxen mentioned this pull request May 9, 2023

Dual funding message follow-ups #2281

Merged

dunxen deleted the 2022-10-dualfunding-act-1 branch May 9, 2023 17:38

This was referenced Aug 26, 2023

Add Splicing message definitions optout21/rust-lightning#1

Closed

Add Splicing messages #2542

Closed

optout21 reviewed Aug 31, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dual funding and interactive tx construction wire messages #1794

Dual funding and interactive tx construction wire messages #1794

dunxen commented Oct 21, 2022 •

edited

Loading

ariard left a comment

TheBlueMatt left a comment

dunxen commented Oct 25, 2022

codecov-commenter commented Oct 26, 2022 •

edited

Loading

TheBlueMatt commented Oct 27, 2022

TheBlueMatt commented Nov 3, 2022

TheBlueMatt commented Nov 3, 2022

dunxen commented Nov 3, 2022

TheBlueMatt commented Nov 3, 2022

wpaulino commented Jan 12, 2023

dunxen commented Jan 13, 2023

TheBlueMatt left a comment

alecchendev left a comment

TheBlueMatt commented Feb 14, 2023

wpaulino commented May 2, 2023

wpaulino left a comment

TheBlueMatt May 4, 2023

dunxen May 5, 2023

dunxen May 5, 2023

TheBlueMatt May 5, 2023

TheBlueMatt left a comment

TheBlueMatt May 5, 2023

TheBlueMatt May 5, 2023

TheBlueMatt May 5, 2023

dunxen May 5, 2023

TheBlueMatt May 5, 2023

dunxen May 5, 2023

dunxen commented May 5, 2023

optout21 Aug 31, 2023

Dual funding and interactive tx construction wire messages #1794

Dual funding and interactive tx construction wire messages #1794

Conversation

dunxen commented Oct 21, 2022 • edited Loading

ariard left a comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

dunxen commented Oct 25, 2022

codecov-commenter commented Oct 26, 2022 • edited Loading

Codecov Report

TheBlueMatt commented Oct 27, 2022

TheBlueMatt commented Nov 3, 2022

TheBlueMatt commented Nov 3, 2022

dunxen commented Nov 3, 2022

TheBlueMatt commented Nov 3, 2022

wpaulino commented Jan 12, 2023

dunxen commented Jan 13, 2023

TheBlueMatt left a comment

Choose a reason for hiding this comment

alecchendev left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Feb 14, 2023

wpaulino commented May 2, 2023

wpaulino left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunxen commented May 5, 2023

Choose a reason for hiding this comment

dunxen commented Oct 21, 2022 •

edited

Loading

codecov-commenter commented Oct 26, 2022 •

edited

Loading