Move in-flight ChannelMonitorUpdates to ChannelManager by TheBlueMatt · Pull Request #2362 · lightningdevkit/rust-lightning

TheBlueMatt · 2023-06-19T06:33:36Z

Because ChannelMonitorUpdates can be generated for a
channel which is already closed, and must still be tracked
through their completion, storing them in a Channel
doesn't make sense - we'd have to have a redundant place to
put them post-closure and handle both storage locations
equivalently.

Instead, here, we move to storing in-flight
ChannelMonitorUpdates to the ChannelManager, leaving
blocked ChannelMonitorUpdates in the Channel as they
were.

This is the first (and largest) step towards #2168, and after this we're at least clear of the serialization-breaking parts, so could in theory cut a test version even without fully fixing 2168 (which needs fixing for 116 though).

Most of the work for #2317

lightning/src/ln/channelmanager.rs

codecov-commenter · 2023-06-20T02:24:06Z

Codecov Report

Patch coverage: 83.20% and project coverage change: -0.02 ⚠️

Comparison is base (15b1c9b) 90.30% compared to head (9c3ad28) 90.29%.

❗ 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    #2362      +/-   ##
==========================================
- Coverage   90.30%   90.29%   -0.02%     
==========================================
  Files         106      106              
  Lines       54747    54920     +173     
  Branches    54747    54920     +173     
==========================================
+ Hits        49441    49591     +150     
- Misses       5306     5329      +23

Impacted Files	Coverage Δ
lightning/src/util/ser.rs	`85.07% <0.00%> (-0.14%)`	⬇️
lightning/src/ln/channel.rs	`89.42% <83.33%> (-0.08%)`	⬇️
lightning/src/ln/channelmanager.rs	`86.19% <83.96%> (-0.11%)`	⬇️

... and 13 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.

dunxen

Concept ACK. Just reviewing f52ed23 a bit more in-depth.

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

dunxen

LGTM. Can't see anything else glaringly wrong from my knowledge of monitor updates.

valentinewallace · 2023-06-21T18:32:35Z

lightning/src/ln/channelmanager.rs

+			// filter for uniqueness here.
+			in_flight_updates.push($update);
+		}
+		let update_res = $self.chain_monitor.update_channel($funding_txo, in_flight_updates.last().unwrap());


Isn't it only correct to call last() if we actually pushed an update above?

I mean if the vec contains something then it should also be safe?

Isn't it possible that we've already called update_channel with the last() monitor update, though?

Hmmmm, yea, maybe, I updated the code to handle it. We shouldn't be double-calling, but indeed I think there's maybe a case where we have two background updates to apply and we just apply the last() twice.

lightning/src/ln/channelmanager.rs

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

alecchendev

Was mostly just absorbing the context here, nothing outstanding to comment on

lightning/src/ln/channelmanager.rs

In the coming commits we'll move to storing in-flight `ChannelMonitorUpdate`s in the `ChannelManager` rather in the `Channel` (which will then only retain `ChannelMonitorUpdate`s which have not yet been released/are blocked. This will simplify handling of pending `ChannelMonitorUpdate` after a channel has closed by not having to move them into the `ChannelManager`.

Most of the calls to the `handle_new_monitor_update` macro had the exact same pattern - calling `update_monitor` followed by the macro. Given that common pattern will grow to first pushing the new monitor onto an in-flight set and then calling `update_monitor` unifying the pattern into a single macro now avoids more code churn in the coming commits.

By giving up on a tiny bit of parallelism and tweaking the return types, we can make the `handle_new_monitor_update` macro a bit clearer - now the only cases where its called after a monitor was updated was when the monitor was initially committed.

TheBlueMatt · 2023-06-21T22:52:43Z

Rebased.

lightning/src/ln/channelmanager.rs

valentinewallace · 2023-06-22T18:02:13Z

lightning/src/ln/channelmanager.rs

+			// filter for uniqueness here.
+			in_flight_updates.push($update);
+		}
+		let update_res = $self.chain_monitor.update_channel($funding_txo, in_flight_updates.last().unwrap());


Isn't it possible that we've already called update_channel with the last() monitor update, though?

valentinewallace

LGTM after this.

lightning/src/ln/channelmanager.rs

wpaulino

Feel free to squash.

wpaulino · 2023-06-23T18:01:46Z

lightning/src/ln/channelmanager.rs

-			highest_applied_update_id, channel.get().context.get_latest_monitor_update_id());
+		let remaining_in_flight =
+			if let Some(pending) = peer_state.in_flight_monitor_updates.get_mut(funding_txo) {
+				pending.retain(|upd| upd.update_id > highest_applied_update_id);


If there are multiple updates in flight, would you call channel_monitor_updated once after they're all done, or after each one is done? If the latter, and they complete out of order, it seems this doesn't work?

Only after they're all done, this is implemented in ChainMonitor. We should probably move to doing it per-update, but we previously didn't track the updates in-flight so couldn't.

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2023-06-23T18:24:35Z

Fixed spelling and squashed:

$ git diff-tree -U1 d18668b4 cd76e97c
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 942998840..cff623caa 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -8399,3 +8399,3 @@ where
 		//
-		// Because tha actual handling of the in-flight updates is the same, its macro'ized here:
+		// Because the actual handling of the in-flight updates is the same, it's macro'ized here:
 		let mut pending_background_events = Vec::new();

TheBlueMatt · 2023-06-23T18:43:56Z

Oops, fixed one more bug @wpaulino pointed out that got introduced in one of the fixups:

$ git diff-tree -U1 cd76e97c e089a404
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 36260f1d2..69a69e7a1 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -865,3 +865,3 @@ pub(super) struct ChannelContext<Signer: ChannelSigner> {
 	/// If we can't release a [`ChannelMonitorUpdate`] until some external action completes, we
-	/// store it here and only release it to [`ChannelManager`] once it asks for it.
+	/// store it here and only release it to the `ChannelManager` once it asks for it.
 	blocked_monitor_updates: Vec<PendingChannelMonitorUpdate>,
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index cff623caa..f38c6c13d 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -1923,3 +1923,3 @@ macro_rules! handle_new_monitor_update {
 			{
-				in_flight_updates.pop();
+				let _ = in_flight_updates.remove(idx);
 				if in_flight_updates.is_empty() && $chan.blocked_monitor_updates_pending() == 0 {

Because `ChannelMonitorUpdate`s can be generated for a channel which is already closed, and must still be tracked through their completion, storing them in a `Channel` doesn't make sense - we'd have to have a redundant place to put them post-closure and handle both storage locations equivalently. Instead, here, we move to storing in-flight `ChannelMonitorUpdate`s to the `ChannelManager`, leaving blocked `ChannelMonitorUpdate`s in the `Channel` as they were.

Now that all `ChannelMonitorUpdate`s stored in `Channel` are blocked we don't need a bool to track it.

`Channel::get_latest_complete_monitor_update_id` no longer refers to complete updates, but rather ones which were passed to the `ChannelManager` and which the `CHannel` no longer knows about. Thus, we rename it `get_latest_unblocked_monitor_update_id`.

To differentiate between in-flight pending completion and blocked updates.

TheBlueMatt · 2023-06-23T19:25:56Z

Grr, intermediate commit had intermediate state, had to shuffle around one line diff, but no total change:

$ git diff-tree -U1 e089a404 9c3ad28f
$

TheBlueMatt added this to the 0.0.116 milestone Jun 19, 2023

TheBlueMatt linked an issue Jun 19, 2023 that may be closed by this pull request

Make Channel::pending_monitor_updates TLV even #2317

Closed

TheBlueMatt commented Jun 19, 2023

View reviewed changes

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

TheBlueMatt force-pushed the 2023-06-unblocked-mons-in-manager branch from 486a65e to f732734 Compare June 20, 2023 02:23

This was referenced Jun 20, 2023

Block the mon update removing a preimage until upstream mon writes #2169

Merged

Re-claim forwarded HTLCs on startup #2364

Merged

TheBlueMatt added the Seeking Code Review label Jun 20, 2023

dunxen reviewed Jun 20, 2023

View reviewed changes

TheBlueMatt removed a link to an issue Jun 20, 2023

Make Channel::pending_monitor_updates TLV even #2317

Closed

TheBlueMatt modified the milestones: 0.0.116, 0.0.116alpha Jun 20, 2023

wpaulino reviewed Jun 20, 2023

View reviewed changes

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

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

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

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

TheBlueMatt force-pushed the 2023-06-unblocked-mons-in-manager branch 4 times, most recently from 32a31af to 3fe91e1 Compare June 21, 2023 00:06

dunxen reviewed Jun 21, 2023

View reviewed changes

valentinewallace reviewed Jun 21, 2023

View reviewed changes

TheBlueMatt force-pushed the 2023-06-unblocked-mons-in-manager branch from 3fe91e1 to e721025 Compare June 21, 2023 20:55

alecchendev reviewed Jun 21, 2023

View reviewed changes

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

TheBlueMatt added 4 commits June 21, 2023 22:37

Drop the now-unused push_blockable_mon_update

5e528ff

TheBlueMatt force-pushed the 2023-06-unblocked-mons-in-manager branch from e721025 to 2704bf3 Compare June 21, 2023 22:52

valentinewallace reviewed Jun 22, 2023

View reviewed changes

TheBlueMatt force-pushed the 2023-06-unblocked-mons-in-manager branch from 2704bf3 to 653c606 Compare June 22, 2023 20:49

valentinewallace reviewed Jun 23, 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

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

TheBlueMatt force-pushed the 2023-06-unblocked-mons-in-manager branch from 653c606 to d18668b Compare June 23, 2023 17:53

wpaulino reviewed Jun 23, 2023

View reviewed changes

TheBlueMatt force-pushed the 2023-06-unblocked-mons-in-manager branch from d18668b to cd76e97 Compare June 23, 2023 18:24

wpaulino previously approved these changes Jun 23, 2023

View reviewed changes

valentinewallace previously approved these changes Jun 23, 2023

View reviewed changes

TheBlueMatt dismissed stale reviews from valentinewallace and wpaulino via e089a40 June 23, 2023 18:43

TheBlueMatt force-pushed the 2023-06-unblocked-mons-in-manager branch from cd76e97 to e089a40 Compare June 23, 2023 18:43

wpaulino approved these changes Jun 23, 2023

View reviewed changes

valentinewallace approved these changes Jun 23, 2023

View reviewed changes

TheBlueMatt added 5 commits June 23, 2023 19:25

Drop the now-unused update_id param to monitor update macros

e186593

Remove the blocked param on ChannelMonitorUpdates in Channel

60e671b

Now that all `ChannelMonitorUpdate`s stored in `Channel` are blocked we don't need a bool to track it.

Rename Channel::pending_monitor_updates to blocked

9c3ad28

To differentiate between in-flight pending completion and blocked updates.

TheBlueMatt force-pushed the 2023-06-unblocked-mons-in-manager branch from e089a40 to 9c3ad28 Compare June 23, 2023 19:25

wpaulino approved these changes Jun 23, 2023

View reviewed changes

valentinewallace approved these changes Jun 23, 2023

View reviewed changes

TheBlueMatt merged commit f833448 into lightningdevkit:main Jun 23, 2023

TheBlueMatt modified the milestones: 0.0.116alpha, 0.0.116 Jun 24, 2023

Conversation

TheBlueMatt commented Jun 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Jun 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!

dunxen left a comment

Choose a reason for hiding this comment

Uh oh!

valentinewallace Jun 21, 2023

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Jun 21, 2023

Choose a reason for hiding this comment

Uh oh!

valentinewallace Jun 22, 2023

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Jun 22, 2023

Choose a reason for hiding this comment

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!

TheBlueMatt commented Jun 21, 2023

Uh oh!

Uh oh!

Uh oh!

valentinewallace Jun 22, 2023

Choose a reason for hiding this comment

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wpaulino left a comment

Choose a reason for hiding this comment

Uh oh!

wpaulino Jun 23, 2023

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Jun 23, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheBlueMatt commented Jun 23, 2023

Uh oh!

TheBlueMatt commented Jun 23, 2023

Uh oh!

TheBlueMatt commented Jun 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

TheBlueMatt commented Jun 19, 2023 •

edited

Loading

codecov-commenter commented Jun 20, 2023 •

edited

Loading

TheBlueMatt commented Jun 23, 2023 •

edited

Loading