Wait to create a channel until after accepting. by waterson · Pull Request #2428 · lightningdevkit/rust-lightning

waterson · 2023-07-18T14:48:32Z

Create a new table in 'peer_state' to maintain unaccepted inbound channels; i.e., a channel for which we've received an 'open_channel' message but that user code has not yet confirmed for acceptance. When user code accepts the channel (e.g. via 'accept_inbound_channel'), create the channel object and as before.

Currently, the 'open_channel' message eagerly creates an InboundV1Channel object before determining if the channel should be accepted. Because this happens /before/ the channel has been assigned a user identity (which happens in the handler for OpenChannelRequest), the channel is assigned a random user identity. As part of the creation process, the channel's cryptographic material is initialized, which then uses this randomly generated value for the user's channel identity e.g. in SignerProvider::generate_channel_keys_id.

By delaying the creation of the InboundV1Channel until /after/ the channel has been accepted, we ensure that we defer cryptographic initialization until we have given the user the opportunity to assign an identity to the channel.

dunxen · 2023-07-18T15:01:54Z

We're looking at actually reducing the maps we create and going with an enum approach and single map for channels #2422, so this might make sense as an AwaitingAccept variant.

Thanks for bringing this up. We could discuss on this PR, but I think an issue would be better.

waterson · 2023-07-20T16:05:44Z

Sounds good. I made some changes to get the tests to pass; would be interested in discussing those. Let me update the PR.

codecov-commenter · 2023-07-20T17:53:08Z

Codecov Report

Patch coverage: 96.26% and project coverage change: -0.04%⚠️

Comparison is base (131560e) 90.35% compared to head (2888f76) 90.32%.

❗ Current head 2888f76 differs from pull request most recent head 0184727. Consider uploading reports for the commit 0184727 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@ Coverage Diff @@## main #2428 +/- ##
==========================================
- Coverage 90.35% 90.32% -0.04% 
==========================================
Files 106 106 Lines 56242 56232 -10 Branches 56242 56232 -10 ==========================================
- Hits 50816 50789 -27 - Misses 5426 5443 +17

Files Changed	Coverage Δ
lightning/src/events/mod.rs	`42.51% <ø> (ø)`
lightning/src/ln/functional_tests.rs	`98.17% <75.00%> (-0.06%)`	⬇️
lightning/src/ln/channelmanager.rs	`85.61% <97.50%> (-0.07%)`	⬇️
lightning/src/ln/channel.rs	`89.48% <100.00%> (+0.02%)`	⬆️
lightning/src/ln/shutdown_tests.rs	`98.45% <100.00%> (ø)`

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dunxen

Thanks this is looking like the right direction. I need to do some more review of what’s moved around but this looks good.

TheBlueMatt · 2023-07-30T23:24:26Z

Oof, indeed, nice catch. Looks like this needs a small rebase, sorry about that.

alecchendev

Looks like rebase caught some extra commits :). Otherwise, LGTM besides one comment and a nit

waterson · 2023-07-31T19:35:31Z

Thank you for the reviews! PTAL when you get a chance!

dunxen

Looking good. I'm wondering if we can now do away with ChannelContext's inbound_awaiting_accept and InboundV1Channel::is_awaiting_accept()

dunxen · 2023-08-01T17:52:50Z

I'm wondering if we can now do away with ChannelContext's inbound_awaiting_accept and InboundV1Channel::is_awaiting_accept()

Actually I think we can punt this to a follow-up, but it would be nice to remove it if this PR makes it obsolete.

waterson · 2023-08-01T19:17:21Z

@dunxen fwiw here is a patch that removes inbound_awaiting_accept. It compiles and passes tests. I can add it to the commit chain here if you think that makes sense,.

dunxen · 2023-08-02T09:22:14Z

@dunxen fwiw here is a patch that removes inbound_awaiting_accept. It compiles and passes tests. I can add it to the commit chain here if you think that makes sense,.

Looks good! As for this comment, we can actually get rid of the InboundV1Channel::set_0conf() method entirely as we'll now know if we must set 0conf when invoking InboundV1Channel::new(), so I think we just need a field there like set_0conf: bool and then inside InboundV1Channel::new() we can then override the minimum_depth based on that. Then we must still keep the if !accept_0conf && channel.context.get_channel_type().requires_zero_conf() check.

We can do this since having an InboundV1Channel will always mean that it's not awaiting accept anymore, unless I'm mistaken?

EDIT: This also means we could probably get rid of the ChannelContext::inbound_awaiting_accept field entirely too. We don't use or serialize it anywhere else.

waterson · 2023-08-02T17:06:56Z

@dunxen fwiw here is a patch that removes inbound_awaiting_accept. It compiles and passes tests. I can add it to the commit chain here if you think that makes sense,.
Looks good! As for this comment, we can actually get rid of the InboundV1Channel::set_0conf() method entirely as we'll now know if we must set 0conf when invoking InboundV1Channel::new(), so I think we just need a field there like set_0conf: bool and then inside InboundV1Channel::new() we can then override the minimum_depth based on that. Then we must still keep the if !accept_0conf && channel.context.get_channel_type().requires_zero_conf() check.
We can do this since having an InboundV1Channel will always mean that it's not awaiting accept anymore, unless I'm mistaken?
EDIT: This also means we could probably get rid of the ChannelContext::inbound_awaiting_accept field entirely too. We don't use or serialize it anywhere else.

So... I'm a bit worried about this exploding into a mega PR that will become increasingly difficult to review. Would you be comfortable to pursuing this cleanup in a follow-on?

dunxen · 2023-08-02T17:08:56Z

So... I'm a bit worried about this exploding into a mega PR that will become increasingly difficult to review. Would you be comfortable to pursuing this cleanup in a follow-on?

Haha sure. I thought the PR blowing up might be the case 😄

dunxen · 2023-08-02T18:48:21Z

Ok so I believe this looks good, and I think we can squash any fixups. We'll need another reviewer though

wpaulino

Feel free to squash before addressing my feedback.

waterson · 2023-08-02T23:19:15Z

Thank you for the review @wpaulino! PTAL when you get a chance. Also a few unresolved that might need some advice... :)

dunxen · 2023-08-04T18:36:15Z

Oh hmm. I might have missed it but we'll need to make sure these requests get purged after some time like we do with inbound channels that are taking too long to finish handshake.

And probably on disconnecting too although maybe we can keep them and try when reconnected. Probably best to just drop them on disconnect for now.

EDIT: I don't see anything wrong with keeping them on disconnect so long as we still expire them after some time.

dunxen · 2023-08-09T05:59:52Z

The CI failures look like we're just missing some use of the new parameter for InboundV1Channel::new in some tests.

Otherwise looks good for squash.

wpaulino

LGTM but still needs a squash on commits 8d00338, 30101b3, and 3a3ba4b. Also, please keep commit messages to 72 characters max per line.

TheBlueMatt · 2023-08-11T00:16:07Z

Grr, this needs a trivial rebase, sorry about that. Conceptually LGTM, tho.

TheBlueMatt

Oops, one issue (we fail to send an error message when failing the channel) and a few nits.

TheBlueMatt · 2023-08-11T17:25:38Z

 peer_state.inbound_v1_channel_by_id.retain(|chan_id, chan| process_unfunded_channel_tick(chan_id, &mut chan.context, &mut chan.unfunded_context));

+ for (chan_id, req) in peer_state.inbound_channel_request_by_id.iter_mut() {
+ if { req.ticks_remaining -= 1 ; req.ticks_remaining } <= 0 {


You can do this updating in the retain body.

Ope, hmm... after addressing the issue below I think that it might make more sense to have a separate loop. In particular, the retain lambda gets sad about the use of peer_state.

Ah, you can make it less sad by explicitly taking reference to the inner variables separately as individual variables before the retain, but its not all that critical.

Create a new table in 'peer_state' to maintain unaccepted inbound channels; i.e., a channel for which we've received an 'open_channel' message but that user code has not yet confirmed for acceptance. When user code accepts the channel (e.g. via 'accept_inbound_channel'), create the channel object and as before. Currently, the 'open_channel' message eagerly creates an InboundV1Channel object before determining if the channel should be accepted. Because this happens /before/ the channel has been assigned a user identity (which happens in the handler for OpenChannelRequest), the channel is assigned a random user identity. As part of the creation process, the channel's cryptographic material is initialized, which then uses this randomly generated value for the user's channel identity e.g. in SignerProvider::generate_channel_keys_id. By delaying the creation of the InboundV1Channel until /after/ the channel has been accepted, we ensure that we defer cryptographic initialization until we have given the user the opportunity to assign an identity to the channel.

dunxen

Thanks! Small nit.

dunxen · 2023-08-14T14:38:41Z

+ events::MessageSendEvent::HandleError {
+ node_id: counterparty_node_id,
+ action: msgs::ErrorAction::SendErrorMessage {
+ msg: msgs::ErrorMessage { channel_id: chan_id.clone(), data: "Channel force-closed".to_owned() }


I think we need a clearer description in the message here. I know we might not know the real reason for not letting the request time out, but maybe we could just say:
"Channel force-closed due to acceptance timeout" or something better. What do you think?
P.S. I've added the missing MessageSendEvent in #2496, which we I will rebase on main when this PR lands.

There's no conflict between the PRs, so landed that one.

Oh, right lol

TheBlueMatt

LGTM, thanks!

TheBlueMatt · 2023-08-14T22:12:49Z

 counterparty_node_id: PublicKey, our_supported_features: &ChannelTypeFeatures,
 their_features: &InitFeatures, msg: &msgs::OpenChannel, user_id: u128, config: &UserConfig,
- current_chain_height: u32, logger: &L, outbound_scid_alias: u64
+ current_chain_height: u32, logger: &L, outbound_scid_alias: u64, is_0conf: bool,


oubtound_scid_alias is now unused (always zero), so we should drop the argument here.

TheBlueMatt · 2023-08-15T22:14:59Z

+	/// the peer is the counterparty. If the channel is accepted, then the entry in this table is
+	/// removed, and an InboundV1Channel is created and placed in the `inbound_v1_channel_by_id` table. If
+	/// the channel is rejected, then the entry is simply removed.
+	pub(super) inbound_channel_request_by_id: HashMap<[u8; 32], InboundChannelRequest>,


Oops we also need to wipe this on peer disconnection and when handling errors from peers, I'll do that in a quick followup.

waterson mentioned this pull request Jul 18, 2023
Inbound channel requests cryptographic material before being accepted & assigned identity #2429
Closed

dunxen self-requested a review July 20, 2023 19:01

dunxen reviewed Jul 24, 2023
View reviewed changes

Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/functional_tests.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated

waterson force-pushed the create-channel-after-accept branch from 8e79049 to 96f52f5Compare July 31, 2023 16:58

waterson requested a review from dunxen July 31, 2023 17:58

waterson force-pushed the create-channel-after-accept branch from 41eb3b2 to ae0ce65Compare July 31, 2023 18:21

alecchendev reviewed Jul 31, 2023
View reviewed changes

Comment threadlightning/src/ln/shutdown_tests.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated

waterson force-pushed the create-channel-after-accept branch from ae0ce65 to 8411b57Compare July 31, 2023 19:25

waterson requested a review from alecchendev July 31, 2023 19:35

dunxen reviewed Aug 1, 2023
View reviewed changes

Comment threadlightning/src/ln/channel.rs Outdated

dunxen reviewed Aug 1, 2023
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated

wpaulino reviewed Aug 2, 2023
View reviewed changes

waterson force-pushed the create-channel-after-accept branch from 0df5629 to b90f6c1Compare August 2, 2023 22:22

waterson requested review from dunxen and wpaulino August 2, 2023 23:18

waterson commented Aug 2, 2023
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated

alecchendev reviewed Aug 4, 2023
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated

waterson force-pushed the create-channel-after-accept branch from 36cfb03 to da1bc11Compare August 9, 2023 15:40

wpaulino reviewed Aug 9, 2023
View reviewed changes

Comment threadlightning/src/ln/functional_tests.rs Outdated

waterson force-pushed the create-channel-after-accept branch from 25c4aa1 to 4e4bec7Compare August 10, 2023 13:52

dunxen previously approved these changes Aug 10, 2023
View reviewed changes

wpaulino previously approved these changes Aug 10, 2023
View reviewed changes

TheBlueMatt reviewed Aug 11, 2023
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated

waterson dismissed stale reviews from wpaulino and dunxen via 2888f76August 11, 2023 16:01

waterson force-pushed the create-channel-after-accept branch from 4e4bec7 to 2888f76Compare August 11, 2023 16:01

waterson requested review from TheBlueMatt and wpaulino August 11, 2023 16:02

dunxen previously approved these changes Aug 11, 2023
View reviewed changes

TheBlueMatt reviewed Aug 11, 2023
View reviewed changes

waterson dismissed dunxen’s stale review via 787975eAugust 14, 2023 02:11

waterson force-pushed the create-channel-after-accept branch 3 times, most recently from 74e3c4a to d53b2ffCompare August 14, 2023 02:33

waterson force-pushed the create-channel-after-accept branch from d53b2ff to 0184727Compare August 14, 2023 02:40

dunxen mentioned this pull request Aug 14, 2023
Use a single peer state map for all channel phases in peer state #2495
Merged

waterson requested a review from TheBlueMatt August 14, 2023 14:00

dunxen mentioned this pull request Aug 14, 2023
Send error message to peer if we drop an unfunded channel on timeout #2496
Merged

dunxen reviewed Aug 14, 2023
View reviewed changes

TheBlueMatt approved these changes Aug 14, 2023
View reviewed changes

wpaulino approved these changes Aug 15, 2023
View reviewed changes

TheBlueMatt reviewed Aug 15, 2023
View reviewed changes

TheBlueMatt merged commit fe0f845 into lightningdevkit:mainAug 15, 2023

TheBlueMatt mentioned this pull request Aug 15, 2023
Ensure we wipe pending un-accepted channel requests on err/discon. #2501
Merged

Uh oh!

Conversation

waterson commented Jul 18, 2023

Uh oh!

dunxen commented Jul 18, 2023

Uh oh!

waterson commented Jul 20, 2023

Uh oh!

codecov-commenter commented Jul 20, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dunxen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented Jul 30, 2023

Uh oh!

alecchendev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

waterson commented Jul 31, 2023

Uh oh!

dunxen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dunxen commented Aug 1, 2023

Uh oh!

waterson commented Aug 1, 2023

Uh oh!

dunxen commented Aug 2, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

waterson commented Aug 2, 2023

Uh oh!

dunxen commented Aug 2, 2023

Uh oh!

dunxen commented Aug 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!

Uh oh!

Uh oh!

waterson commented Aug 2, 2023

Uh oh!

Uh oh!

dunxen commented Aug 4, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

dunxen commented Aug 9, 2023

Uh oh!

wpaulino left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheBlueMatt commented Aug 11, 2023

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

codecov-commenter commented Jul 20, 2023•
edited
Loading

dunxen commented Aug 2, 2023•
edited
Loading

dunxen commented Aug 4, 2023•
edited
Loading