Use a single peer state map for all channel phases in peer state #2495

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

lightning/src/ln/channelmanager.rs

alecchendev

Got maybe ~half way through

lightning/src/ln/channelmanager.rs

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

lightning/src/ln/channelmanager.rs

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

lightning/src/ln/channelmanager.rs

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

lightning/src/ln/channelmanager.rs

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

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

lightning/src/ln/channelmanager.rs

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

lightning/src/ln/channelmanager.rs

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

Thanks for the review. Addressing.

lightning/src/ln/channelmanager.rs

wpaulino

LGTM, just a few minor comments

lightning/src/ln/channelmanager.rs

TheBlueMatt

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.

lightning/src/ln/channelmanager.rs

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.

wpaulino

LGTM, just a couple more nits

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

lightning/src/ln/channelmanager.rs

@@ -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

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

lightning/src/ln/channelmanager.rs

-		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

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

lightning/src/ln/channelmanager.rs

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

wpaulino · 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.

dunxen · 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 d0d158a Compare 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 eba38d8 Compare August 15, 2023 12:39

dunxen force-pushed the 2023-07-channelenummap branch from eba38d8 to 963540f Compare 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 63d294a Compare August 16, 2023 08:44

vladimirfomene reviewed Aug 18, 2023

View reviewed changes

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

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

alecchendev reviewed Aug 18, 2023

View reviewed changes

dunxen force-pushed the 2023-07-channelenummap branch from 63d294a to 1802e22 Compare 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 7ca24f6 Compare August 22, 2023 17:46

TheBlueMatt reviewed Aug 22, 2023

View reviewed changes

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

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

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

wpaulino reviewed Aug 22, 2023

View reviewed changes

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

TheBlueMatt reviewed Aug 29, 2023

View reviewed changes

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

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

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

dunxen force-pushed the 2023-07-channelenummap branch 3 times, most recently from b559f33 to 592db11 Compare 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

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

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

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

TheBlueMatt reviewed Sep 7, 2023

View reviewed changes

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

TheBlueMatt reviewed Sep 7, 2023

View reviewed changes

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

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

dunxen added 2 commits September 8, 2023 10:19

Remove outdated Channel TODO

0cdd72c

dunxen force-pushed the 2023-07-channelenummap branch from 775eef4 to 9a1692e Compare 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

dunxen 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 88db0b8 Compare 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:main Sep 9, 2023
12 of 14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a single peer state map for all channel phases in peer state #2495

Use a single peer state map for all channel phases in peer state #2495

dunxen commented Aug 14, 2023 •

edited

Loading

codecov-commenter commented Aug 14, 2023 •

edited

Loading

TheBlueMatt commented Aug 14, 2023

dunxen commented Aug 15, 2023

TheBlueMatt commented Aug 16, 2023

vladimirfomene left a comment

alecchendev left a comment

alecchendev left a comment

alecchendev Aug 22, 2023

dunxen Aug 26, 2023

wpaulino Aug 22, 2023

dunxen Aug 23, 2023

wpaulino Aug 23, 2023

TheBlueMatt Aug 23, 2023

dunxen Aug 23, 2023

dunxen commented Aug 23, 2023

wpaulino left a comment

TheBlueMatt left a comment

wpaulino left a comment

wpaulino Sep 8, 2023

wpaulino Sep 8, 2023

wpaulino Sep 8, 2023

dunxen Sep 8, 2023

wpaulino commented Sep 8, 2023

dunxen commented Sep 8, 2023

Use a single peer state map for all channel phases in peer state #2495

Use a single peer state map for all channel phases in peer state #2495

Conversation

dunxen commented Aug 14, 2023 • edited Loading

codecov-commenter commented Aug 14, 2023 • edited Loading

Codecov Report

TheBlueMatt commented Aug 14, 2023

dunxen commented Aug 15, 2023

TheBlueMatt commented Aug 16, 2023

vladimirfomene left a comment

Choose a reason for hiding this comment

alecchendev left a comment

Choose a reason for hiding this comment

alecchendev 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

Choose a reason for hiding this comment

dunxen commented Aug 23, 2023

wpaulino left a comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

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

wpaulino commented Sep 8, 2023

dunxen commented Sep 8, 2023

dunxen commented Aug 14, 2023 •

edited

Loading

codecov-commenter commented Aug 14, 2023 •

edited

Loading