Setup Support for delaying `ChannelMonitorUpdate` flight until an `Event` completes #2111

TheBlueMatt · 2023-03-17T05:20:09Z

This does all the work that will be required to pause ChannelMonitorUpdate flight until an Event completes, but doesn't actually use the new behavior. Sadly, I think we should consider splitting this work across release, as the commit Track an EventCompletionAction for after an Event is processed indicates -

Note that this gets dangerously close to breaking forwards
compatibility - if we have an `Event` with an
`EventCompletionAction` tied to it, we persist a new, even, TLV in
the `ChannelManager`. Hopefully this should be uncommon, as it
implies an `Event` was delayed until after a full round-trip to a
peer.

Still, I'm open to discussion on it - Events generally should be reliably processed before a full round-trip happens, and it sucks to fix major bugs across releases just over worry of a hopefully-very-rare race breaking forwards, but not backwards, compat.

codecov-commenter · 2023-03-17T07:44:13Z

Codecov Report

Patch coverage: 69.12% and project coverage change: +0.56 🎉

Comparison is base (88c63e9) 91.57% compared to head (880d88e) 92.14%.

❗ Current head 880d88e differs from pull request most recent head 9c36735. Consider uploading reports for the commit 9c36735 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    #2111      +/-   ##
==========================================
+ Coverage   91.57%   92.14%   +0.56%     
==========================================
  Files         104      105       +1     
  Lines       51930    61222    +9292     
  Branches    51930    61222    +9292     
==========================================
+ Hits        47556    56410    +8854     
- Misses       4374     4812     +438

Impacted Files	Coverage Δ
lightning/src/ln/channelmanager.rs	`90.47% <50.00%> (+1.66%)`	⬆️
lightning/src/ln/channel.rs	`92.64% <92.70%> (+2.62%)`	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	`98.61% <100.00%> (ø)`
lightning/src/ln/functional_tests.rs	`98.25% <100.00%> (ø)`
lightning/src/ln/outbound_payment.rs	`89.56% <100.00%> (-0.08%)`	⬇️

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

lightning/src/ln/channel.rs

wpaulino · 2023-03-17T15:57:40Z

lightning/src/ln/channel.rs

@@ -3230,8 +3246,11 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
 			}
 			log_debug!(logger, "Received valid commitment_signed from peer in channel {}, updated HTLC state but awaiting a monitor update resolution to reply.",
 				log_bytes!(self.channel_id));
-			self.pending_monitor_updates.push(monitor_update);
-			return Ok(self.pending_monitor_updates.last().unwrap());
+			let fly_monitor = self.pending_monitor_updates.iter().all(|upd| upd.flown);


Do you see any benefit in tracking the flown/dispatched pending updates in a separate Vec?

Uhhhhhhhhhh. Maybe? Not sure yet, honestly. It would clean up a bit of code, and make some other code a bit more complicated, kinda a wash, but I'm not 100% sure where this is gonna end up , I think this is all of the holding logic we're gonna need for it, but if we find some other bug in there in the future we could end up with more going on in the struct and a two-struct approach will be confusing.

lightning/src/ln/channelmanager.rs

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

lightning/src/ln/channel.rs

wpaulino

Feel free to squash the existing fixups

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

wpaulino · 2023-04-05T21:34:58Z

Feel free to squash, will give it a final pass after a second reviewer.

TheBlueMatt · 2023-04-06T18:09:50Z

Squashed without further changes.

TheBlueMatt · 2023-04-13T19:45:21Z

Fixed a silent rebase conflict as well as a bug that the further upstream lockorder detection surfaced.

TheBlueMatt · 2023-04-17T20:11:27Z

Actually, gonna slip this cause it should really go in the same release as its followup, which isn't gonna make 115.

lightning/src/ln/channel.rs

TheBlueMatt · 2023-04-28T16:29:01Z

Rebased.

wpaulino · 2023-05-02T01:16:37Z

Will do another full pass after squash

valentinewallace

Looks good

lightning/src/ln/channelmanager.rs

lightning/src/ln/channel.rs

valentinewallace · 2023-05-01T21:39:17Z

lightning/src/ln/channelmanager.rs

-					$chan.get_latest_monitor_update_id() == $update_id
-				{
+				$chan.complete_one_mon_update($update_id);
+				if $chan.no_monitor_updates_pending() {


I think we can use this check in channel_monitor_updated too (maybe redundant with @wpaulino's comment above). Or move this check into the handle_monitor_update_completion macro

Yea, I want to clean this logic up, mind if I do it in a followup, though? I have a handful of changes I'm working on to the monitor completion logic here that I can incorporate this into.

In the coming commits, we need to delay `ChannelMonitorUpdate`s until future actions (specifically `Event` handling). However, because we should only notify users once of a given `ChannelMonitorUpdate` and they must be provided in-order, we need to track which ones have or have not been given to users and, once updating resumes, fly the ones that haven't already made it to users. To do this we simply add a `bool` in the `ChannelMonitorUpdate` set stored in the `Channel` which indicates if an update flew and decline to provide new updates back to the `ChannelManager` if any updates have their flown bit unset. Further, because we'll now by releasing `ChannelMonitorUpdate`s which were already stored in the pending list, we now need to support getting a `Completed` result for a monitor which isn't the only pending monitor (or even out of order), thus we also rewrite the way monitor updates are marked completed.

This will allow us to block `ChannelMonitorUpdate`s on `Event` processing in the next commit. Note that this gets dangerously close to breaking forwards compatibility - if we have an `Event` with an `EventCompletionAction` tied to it, we persist a new, even, TLV in the `ChannelManager`. Hopefully this should be uncommon, as it implies an `Event` was delayed until after a full round-trip to a peer.

This adds handling of the new `EventCompletionAction`s after `Event`s are handled, letting `ChannelMonitorUpdate`s which were blocked fly after a relevant `Event`.

The previous commits set up the ability for us to hold `ChannelMonitorUpdate`s which are pending until we're ready to pass them to users and have them be applied. However, if the `ChannelManager` is persisted while we're waiting to give the user a `ChannelMonitorUpdate` we'll be confused on restart - seeing our latest `ChannelMonitor` state as stale compared to our `ChannelManager` - a critical error. Luckily the solution is trivial, we simply need to store the pending `ChannelMonitorUpdate` state and load it with the `ChannelManager` data, allowing stale monitors on load as long as we have the missing pending updates between where we are and the latest `ChannelMonitor` state.

TheBlueMatt · 2023-05-02T18:00:38Z

Squashed without further changes.

valentinewallace · 2023-05-02T22:08:44Z

Looks good after @wpaulino takes another look!

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2023-05-04T19:49:26Z

oops sorry that commit was buggy, sec

commit was buggy

TheBlueMatt · 2023-05-04T19:51:50Z

Actually, just gonna drop the last/new commit, its...hard, so we need to (a) handle_post_event_actions after we pending_events.drain(..num_events) as we only actually handle the post-event action if the event has been removed and (b) need to unset pending_events_processor and check if there are any remaining events both inside the same lock. Thus its kinda hard to change it around all that much without breaking it.

TheBlueMatt · 2023-05-04T19:53:01Z

The tests on #2112 failed with that commit, specifically, like all the tests.

TheBlueMatt added this to the 0.0.115 milestone Mar 17, 2023

TheBlueMatt mentioned this pull request Mar 17, 2023

Delay RAA-after-next processing until PaymentSent is are handled #2112

Merged

TheBlueMatt force-pushed the 2023-03-sent-persist-order-prep branch 3 times, most recently from 2aae7c8 to 91707fb Compare March 17, 2023 07:04

TheBlueMatt force-pushed the 2023-03-sent-persist-order-prep branch from 91707fb to c595331 Compare March 17, 2023 21:09

wpaulino self-requested a review March 21, 2023 20:02

TheBlueMatt force-pushed the 2023-03-sent-persist-order-prep branch 3 times, most recently from 57bebe7 to 9852b84 Compare March 28, 2023 22:01

wpaulino reviewed Mar 30, 2023

View reviewed changes

TheBlueMatt force-pushed the 2023-03-sent-persist-order-prep branch from 9852b84 to 2ba5289 Compare March 30, 2023 23:53

wpaulino reviewed Apr 5, 2023

View reviewed changes

TheBlueMatt force-pushed the 2023-03-sent-persist-order-prep branch 2 times, most recently from ae76ed0 to 08ae599 Compare April 5, 2023 18:48

wpaulino added the Seeking Code Review label Apr 5, 2023

jkczyz requested review from valentinewallace and jkczyz April 6, 2023 17:30

TheBlueMatt force-pushed the 2023-03-sent-persist-order-prep branch from 08ae599 to d2d3036 Compare April 6, 2023 18:09

This was referenced Apr 7, 2023

Add infra to block ChannelMonitorUpdates on forwarded claims #2167

Merged

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

Merged

TheBlueMatt force-pushed the 2023-03-sent-persist-order-prep branch from d2d3036 to 207af69 Compare April 12, 2023 17:05

This was referenced Apr 12, 2023

Genericize Test Functions #2177

Merged

Split prefunded Channel into Inbound/Outbound channels #2077

Merged

TheBlueMatt force-pushed the 2023-03-sent-persist-order-prep branch from 207af69 to 7014813 Compare April 13, 2023 19:44

TheBlueMatt force-pushed the 2023-03-sent-persist-order-prep branch from 7014813 to a626549 Compare April 13, 2023 21:03

TheBlueMatt added this to the 0.0.116 milestone Apr 17, 2023

valentinewallace reviewed Apr 17, 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

TheBlueMatt force-pushed the 2023-03-sent-persist-order-prep branch from 6f1263d to 87bcf91 Compare April 17, 2023 20:55

wpaulino self-requested a review April 18, 2023 17:29

TheBlueMatt force-pushed the 2023-03-sent-persist-order-prep branch from 87bcf91 to 51c3b37 Compare April 28, 2023 04:55

valentinewallace reviewed May 2, 2023

View reviewed changes

TheBlueMatt added 4 commits May 2, 2023 17:59

Handle EventCompletionActions after events complete

756b5aa

This adds handling of the new `EventCompletionAction`s after `Event`s are handled, letting `ChannelMonitorUpdate`s which were blocked fly after a relevant `Event`.

TheBlueMatt force-pushed the 2023-03-sent-persist-order-prep branch from 51c3b37 to 9dfe42c Compare May 2, 2023 18:00

wpaulino reviewed May 3, 2023

View reviewed changes

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

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

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

wpaulino reviewed May 4, 2023

View reviewed changes

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

TheBlueMatt force-pushed the 2023-03-sent-persist-order-prep branch from 880d88e to 9c36735 Compare May 4, 2023 19:29

wpaulino previously approved these changes May 4, 2023

View reviewed changes

valentinewallace previously approved these changes May 4, 2023

View reviewed changes

TheBlueMatt dismissed wpaulino’s stale review via 9dfe42c May 4, 2023 19:52

TheBlueMatt force-pushed the 2023-03-sent-persist-order-prep branch from 9c36735 to 9dfe42c Compare May 4, 2023 19:52

valentinewallace approved these changes May 4, 2023

View reviewed changes

wpaulino approved these changes May 4, 2023

View reviewed changes

TheBlueMatt merged commit e94647c into lightningdevkit:main May 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Setup Support for delaying `ChannelMonitorUpdate` flight until an `Event` completes #2111

Setup Support for delaying `ChannelMonitorUpdate` flight until an `Event` completes #2111

TheBlueMatt commented Mar 17, 2023

codecov-commenter commented Mar 17, 2023 •

edited

Loading

wpaulino Mar 17, 2023

TheBlueMatt Mar 30, 2023

wpaulino left a comment

wpaulino commented Apr 5, 2023

TheBlueMatt commented Apr 6, 2023

TheBlueMatt commented Apr 13, 2023

TheBlueMatt commented Apr 17, 2023

TheBlueMatt commented Apr 28, 2023

wpaulino commented May 2, 2023

valentinewallace left a comment

valentinewallace May 1, 2023

TheBlueMatt May 2, 2023

TheBlueMatt commented May 2, 2023

valentinewallace commented May 2, 2023

TheBlueMatt commented May 4, 2023

TheBlueMatt commented May 4, 2023

TheBlueMatt commented May 4, 2023

Setup Support for delaying ChannelMonitorUpdate flight until an Event completes #2111

Setup Support for delaying ChannelMonitorUpdate flight until an Event completes #2111

Conversation

TheBlueMatt commented Mar 17, 2023

codecov-commenter commented Mar 17, 2023 • edited Loading

Codecov Report

wpaulino Mar 17, 2023

Choose a reason for hiding this comment

TheBlueMatt Mar 30, 2023

Choose a reason for hiding this comment

wpaulino left a comment

Choose a reason for hiding this comment

wpaulino commented Apr 5, 2023

TheBlueMatt commented Apr 6, 2023

TheBlueMatt commented Apr 13, 2023

TheBlueMatt commented Apr 17, 2023

TheBlueMatt commented Apr 28, 2023

wpaulino commented May 2, 2023

valentinewallace left a comment

Choose a reason for hiding this comment

valentinewallace May 1, 2023

Choose a reason for hiding this comment

TheBlueMatt May 2, 2023

Choose a reason for hiding this comment

TheBlueMatt commented May 2, 2023

valentinewallace commented May 2, 2023

TheBlueMatt commented May 4, 2023

TheBlueMatt commented May 4, 2023

TheBlueMatt commented May 4, 2023

Setup Support for delaying `ChannelMonitorUpdate` flight until an `Event` completes #2111

Setup Support for delaying `ChannelMonitorUpdate` flight until an `Event` completes #2111

codecov-commenter commented Mar 17, 2023 •

edited

Loading