Use a single peer state map for all channel phases in peer state by dunxen · Pull Request #2495 · lightningdevkit/rust-lightning

dunxen · 2023-08-14T11:32:15Z

Will unblock progress on #2302.

codecov-commenter · 2023-08-14T12:54:25Z

Codecov Report

Patch coverage: 83.80% and project coverage change: -0.15%⚠️

Comparison is base (44b9c54) 90.63% compared to head (9a1692e) 90.48%.
Report is 21 commits behind head on main.

❗ Current head 9a1692e differs from pull request most recent head 88db0b8. Consider uploading reports for the commit 88db0b8 to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2495 +/- ##
==========================================
- Coverage 90.63% 90.48% -0.15% 
==========================================
Files 110 112 +2 Lines 58199 58429 +230 Branches 58199 58429 +230 ==========================================
+ Hits 52747 52870 +123 - Misses 5452 5559 +107

Files Changed	Coverage Δ
lightning/src/ln/channelmanager.rs	`86.71% <ø> (-0.44%)`	⬇️
lightning/src/util/test_utils.rs	`68.70% <0.00%> (-4.91%)`	⬇️
lightning/src/ln/functional_test_utils.rs	`88.91% <33.33%> (-0.29%)`	⬇️
lightning-persister/src/utils.rs	`71.42% <71.42%> (ø)`
lightning/src/util/persist.rs	`84.78% <80.55%> (-15.22%)`	⬇️
lightning-background-processor/src/lib.rs	`81.41% <82.05%> (-0.91%)`	⬇️
lightning-persister/src/fs_store.rs	`87.71% <87.71%> (ø)`
lightning/src/ln/msgs.rs	`86.20% <89.77%> (ø)`
lightning/src/ln/chanmon_update_fail_tests.rs	`98.67% <90.00%> (-0.05%)`	⬇️
lightning/src/util/message_signing.rs	`92.30% <90.47%> (ø)`
... and 8 more

... and 1 file with indirect coverage changes

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

TheBlueMatt · 2023-08-14T22:28:25Z

Was nice while it lasted, but, yea, I think this makes sense 😢

dunxen · 2023-08-15T09:47:47Z

Was nice while it lasted, but, yea, I think this makes sense 😢

Yeah, there were some benefits to the multi-map approach, but I believe there's just too many negatives and harder to consolidate. The one con to single map is that there is added verbosity in places but I think we can impl helper methods on a newtype for the map to make things more readable and dedup stuff.

TheBlueMatt · 2023-08-16T00:14:25Z

#2428 landed 🎉

vladimirfomene

Just a few questions

alecchendev

Got maybe ~half way through

alecchendev

Mainly was trying to look at where we match every variant vs. _ (or do an if let) since one of the nice things with this PR is to have the compiler notify us of important places when adding new ChannelPhases, and these places seem to be good (or at least lean towards being stricter) AFAICT, might have to think on these more though. Otherwise, mainly just some smaller style comments and whatnot

alecchendev · 2023-08-22T01:35:22Z

+ hash_map::Entry::Occupied(mut chan_phase_entry) => {
+ if let ChannelPhase::Funded(chan) = chan_phase_entry.get_mut() {
+ let (closing_signed, tx) = try_chan_phase_entry!(self, chan.closing_signed(&self.fee_estimator, &msg), chan_phase_entry);


but I think we can impl helper methods on a newtype for the map to make things more readable and dedup stuff.
One thing I noticed while reviewing is there are a good amount of places where we want to use the chan_phase_entry and the chan, and only match on one variant, and otherwise if it's vacant in the hashmap or a different variant, we return roughly the same error. This might be a pattern where a helper/macro could simplify some things, not sure if it's best fit for this PR or a followup but just an idea :)

Keeping this for a followup for now, but might tack it on in this PR still.

wpaulino · 2023-08-22T22:38:03Z

- }
+ for channel in peer_state.channel_by_id.iter().filter_map(
+ |(_, phase)| if let ChannelPhase::Funded(channel) = phase {
+ if channel.context.is_funding_initiated() { Some(channel) } else { None }


Checking is_funding_initiated seems redundant?

There's a case where we're the channel opener and the unfunded channel becomes a funded Channel when we generate the funding transaction and before we receive funding_signed. Then is_funding_initiated() would return false.
I think you had suggested we only promote a channel to funded when we receive funding_signed and create a monitor for it, but the consensus was that we do it when we generate the txid-based channel ID so we don't mix them in the maps (and some other stuff). But now we're back to having one map so they would be mixed anyway. So maybe we go back to that and then can get rid of is_funding_initiated()?

Yeah I think we should.

Irrespective of if we change when a channel becomes funded (which we can change, I think, sgtm), we'll still have this issue here with dual-funded channels where one channel has completed its monitor update and another has not. See discussion at #2486, but lets at least leave this LoC. If we want to move the post for when a channel becomes funded, should we do that in a separate PR?

Yeah I wasn't thinking of changing it here. If we do end up shifting it then it'll be a separate PR after dual funding is considered. Focus is to get this PR through and get that V2 establishment PR ready for review and make progress there.

dunxen · 2023-08-23T12:36:32Z

Thanks for the review. Addressing.

ghost

LGTM, just a few minor comments

ghost

LGTM aside from @wpaulino's comments and two nits below. Feel free to squash as you go I think, would like to land this tomorrow.

We introduce the `ChannelPhase` enum which will contain the different channel structs wrapped by each of its variants so that we can place these within a single `channel_by_id` map in `peer_state` in the following commits. This will reduce the number of map lookup operations we need to do in `ChannelManager`'s various methods. It will also make certain channel counting logic easier to reason about with less risk of forgetting to modify logic when new channels structs are introduced for V2 channel establishment.

ghost

LGTM, just a couple more nits

ghost · 2023-09-08T17:33:16Z

@@ -2057,7 +2113,21 @@ macro_rules! handle_new_monitor_update {
 })
 	} };
 	($self: ident, $funding_txo: expr, $update: expr, $peer_state_lock: expr, $peer_state: expr, $per_peer_state_lock: expr, $chan_entry: expr) => {
- handle_new_monitor_update!($self, $funding_txo, $update, $peer_state_lock, $peer_state, $per_peer_state_lock, $chan_entry.get_mut(), MANUALLY_REMOVING, $chan_entry.remove_entry())
+ if let ChannelPhase::Funded(chan) = $chan_entry.get_mut() {
+ handle_new_monitor_update!($self, $funding_txo, $update, $peer_state_lock, $peer_state, $per_peer_state_lock, chan, MANUALLY_REMOVING, { 


Nit: extra blank space at end of line

ghost · 2023-09-08T17:33:41Z

- handle_new_monitor_update!($self, $funding_txo, $update, $peer_state_lock, $peer_state, $per_peer_state_lock, $chan_entry.get_mut(), MANUALLY_REMOVING, $chan_entry.remove_entry())
+ if let ChannelPhase::Funded(chan) = $chan_entry.get_mut() {
+ handle_new_monitor_update!($self, $funding_txo, $update, $peer_state_lock, $peer_state, $per_peer_state_lock, chan, MANUALLY_REMOVING, { 
+ if let ChannelPhase::Funded(chan) = $chan_entry.remove() { chan } else { unreachable!() }


Remove without variant check

ghost · 2023-09-08T17:43:01Z

+ let (_, err) = convert_chan_phase_err!($self, ChannelError::Close(
+ "Cannot update monitor for unfunded channels as they don't have monitors yet".into()),
+ $chan_entry.get_mut(), &channel_id);
+ $chan_entry.remove();
+ Err(err)


I think we can just use try_chan_phase_entry?

I think it leads to an early return and messes things up here.

ghost · 2023-09-08T17:45:59Z

Whenever possible, if not using fixups, please keep rebases as their own push when also addressing feedback so that it's easier to tell what changed.

ghost · 2023-09-08T21:05:20Z

Whenever possible, if not using fixups, please keep rebases as their own push when also addressing feedback so that it's easier to tell what changed.

Sorry about that.

dunxen force-pushed the 2023-07-channelenummap branch from 9bd6169 to d0d158aCompare August 14, 2023 13:44

dunxen added blocked on dependent pr Dual-funding labels Aug 15, 2023

dunxen force-pushed the 2023-07-channelenummap branch from d0d158a to eba38d8Compare August 15, 2023 12:39

dunxen force-pushed the 2023-07-channelenummap branch from eba38d8 to 963540fCompare August 16, 2023 07:32

dunxen removed the blocked on dependent pr label Aug 16, 2023

dunxen marked this pull request as ready for review August 16, 2023 07:46

dunxen force-pushed the 2023-07-channelenummap branch from 963540f to 63d294aCompare August 16, 2023 08:44

vladimirfomene reviewed Aug 18, 2023
View reviewed changes

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

alecchendev reviewed Aug 18, 2023
View reviewed changes

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

dunxen force-pushed the 2023-07-channelenummap branch from 63d294a to 1802e22Compare August 20, 2023 21:15

alecchendev reviewed Aug 22, 2023
View reviewed changes

dunxen force-pushed the 2023-07-channelenummap branch 2 times, most recently from 45cfaf1 to 7ca24f6Compare August 22, 2023 17:46

TheBlueMatt reviewed Aug 22, 2023
View reviewed changes

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

wpaulino reviewed Aug 22, 2023
View reviewed changes

dunxen force-pushed the 2023-07-channelenummap branch 3 times, most recently from c67d033 to ad7b75eCompare August 27, 2023 13:06

TheBlueMatt reviewed Aug 29, 2023
View reviewed changes

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

dunxen force-pushed the 2023-07-channelenummap branch 3 times, most recently from b559f33 to 592db11Compare September 5, 2023 19:13

wpaulino reviewed Sep 6, 2023
View reviewed changes

TheBlueMatt mentioned this pull request Sep 6, 2023
Handle retrying sign_counterparty_commitment failures #2558
Merged

TheBlueMatt reviewed Sep 6, 2023
View reviewed changes

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

dunxen force-pushed the 2023-07-channelenummap branch from 592db11 to 775eef4Compare September 7, 2023 12:29

TheBlueMatt reviewed Sep 7, 2023
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated

TheBlueMatt reviewed Sep 7, 2023
View reviewed changes

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

Duncan Dean added 2 commits September 8, 2023 10:19

Remove outdated Channel TODO
0cdd72c

dunxen force-pushed the 2023-07-channelenummap branch from 775eef4 to 9a1692eCompare September 8, 2023 08:37

wpaulino reviewed Sep 8, 2023
View reviewed changes

TheBlueMatt mentioned this pull request Sep 8, 2023
Drop the ChannelMonitorUpdateStatus::PermanentFailure variant #2562
Merged

Duncan Dean added 2 commits September 8, 2023 23:10

Refactor ChannelManager with ChannelPhase
15199a6

Remove v1 peer state channel maps & refactor with ChannelPhase
88db0b8

dunxen force-pushed the 2023-07-channelenummap branch from 9a1692e to 88db0b8Compare September 8, 2023 21:12

TheBlueMatt approved these changes Sep 8, 2023
View reviewed changes

wpaulino approved these changes Sep 9, 2023
View reviewed changes

TheBlueMatt merged commit 6436232 into lightningdevkit:mainSep 9, 2023

dunxen deleted the 2023-07-channelenummap branch January 20, 2025 08:31

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

TheBlueMatt commented Aug 14, 2023

Uh oh!

dunxen commented Aug 15, 2023

Uh oh!

TheBlueMatt commented Aug 16, 2023

Uh oh!

vladimirfomene left a comment

Choose a reason for hiding this comment

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!

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!

Uh oh!

Uh oh!

alecchendevAug 22, 2023

Choose a reason for hiding this comment

Uh oh!

dunxenAug 26, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wpaulinoAug 22, 2023

Choose a reason for hiding this comment

Uh oh!

dunxenAug 23, 2023

Choose a reason for hiding this comment

Uh oh!

wpaulinoAug 23, 2023

Choose a reason for hiding this comment

Uh oh!

TheBlueMattAug 23, 2023

Choose a reason for hiding this comment

Uh oh!

dunxenAug 23, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dunxen commented Aug 23, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

dunxen commented Aug 14, 2023•
edited
Loading

codecov-commenter commented Aug 14, 2023•
edited
Loading