Dual funding and interactive tx construction wire messages by dunxen · Pull Request #1794 · lightningdevkit/rust-lightning

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

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?

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.

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

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

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.

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 :)

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?

dunxen · 2023-02-14T21:56:03Z

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

Right! I'll check if I missed anything and squash for an easier final review in the morning. This should be followed by the OutboundChannel/InboundChannel refactoring I've been working on ASAP (lots of changes so expecting lots of conflicts before getting that through :/ - but yay type safety/making dual funding easier).

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.

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

+	/// 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.

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

+///
+/// 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

+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

+	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


 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

+ /// 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 reviewed Oct 23, 2022
View reviewed changes

Comment threadlightning/src/ln/msgs.rs Outdated

TheBlueMatt reviewed Oct 25, 2022
View reviewed changes

Comment threadlightning/src/ln/msgs.rs Outdated
Comment threadlightning/src/ln/msgs.rs Outdated
Comment threadlightning/src/ln/msgs.rs Outdated

dunxen force-pushed the 2022-10-dualfunding-act-1 branch 2 times, most recently from 5ff4b1f to e8b095fCompare 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 ff23ca9Compare November 1, 2022 14:12

TheBlueMatt reviewed Nov 2, 2022
View reviewed changes

Comment threadlightning/src/ln/peer_handler.rs Outdated

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

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

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

TheBlueMatt reviewed Nov 3, 2022
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated

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

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

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

TheBlueMatt reviewed Jan 13, 2023
View reviewed changes

alecchendev reviewed Jan 24, 2023
View reviewed changes

Comment threadlightning/src/ln/msgs.rs
Comment threadlightning/src/ln/msgs.rs
Comment threadlightning/src/ln/msgs.rs Outdated
Comment threadlightning/src/ln/msgs.rs Outdated
Comment threadlightning/src/ln/msgs.rs
Comment threadlightning/src/ln/msgs.rs

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 dc9b02aCompare February 6, 2023 12:41

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

wpaulino reviewed Apr 25, 2023
View reviewed changes

Comment threadlightning/src/util/ser.rs
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/msgs.rs Outdated
Comment threadlightning/src/ln/msgs.rs
Comment threadfuzz/targets.h Outdated

dunxen force-pushed the 2022-10-dualfunding-act-1 branch from 645b337 to 40a7a56Compare 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 bce24b6Compare May 3, 2023 08:55

wpaulino reviewed May 3, 2023
View reviewed changes

Comment threadlightning/src/util/ser.rs Outdated
Comment threadlightning/src/util/ser.rs Outdated

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

dunxen changed the title ~~Dual funding messages~~Dual funding and interactive tx construction wire messagesMay 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 ddfddcfMay 5, 2023 09:27

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

dunxen added 3 commits May 5, 2023 11:40

Add message structs required for dual-funded channels
adc1b55
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.

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 2ace882Compare 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:mainMay 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

Uh oh!

Conversation

dunxen commented Oct 21, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dunxen commented Oct 25, 2022

Uh oh!

codecov-commenter commented Oct 26, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

TheBlueMatt commented Oct 27, 2022

Uh oh!

Uh oh!

TheBlueMatt commented Nov 3, 2022

Uh oh!

Uh oh!

TheBlueMatt commented Nov 3, 2022

Uh oh!

dunxen commented Nov 3, 2022

Uh oh!

TheBlueMatt commented Nov 3, 2022

Uh oh!

wpaulino commented Jan 12, 2023

Uh oh!

dunxen commented Jan 13, 2023

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alecchendev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented Feb 14, 2023

Uh oh!

dunxen commented Feb 14, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wpaulino commented May 2, 2023

Uh oh!

wpaulino left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

dunxen commented Oct 21, 2022•
edited
Loading

codecov-commenter commented Oct 26, 2022•
edited
Loading