Add infra to block ChannelMonitorUpdates on forwarded claims #2167

TheBlueMatt · 2023-04-07T00:35:00Z

When we forward a payment and receive an update_fulfill_htlc
message from the downstream channel, we immediately claim the HTLC
on the upstream channel, before even doing a commitment_signed
dance on the downstream channel. This implies that our
ChannelMonitorUpdates "go out" in the right order - first we
ensure we'll get our money by writing the preimage down, then we
write the update that resolves giving money on the downstream node.

This is safe as long as ChannelMonitorUpdates complete in the
order in which they are generated, but of course looking forward we
want to support asynchronous updates, which may complete in any
order.

Here we add infrastructure to handle downstream
ChannelMonitorUpdates which are blocked on an upstream
preimage-containing one. We don't yet actually do the blocking which
will come in a future commit.

~~Based on #2111,~~ the follow up is be based on #2112 plus this.

codecov-commenter · 2023-04-07T02:43:50Z

Codecov Report

Patch coverage: 79.81% and project coverage change: +1.17 🎉

Comparison is base (9e542ec) 90.94% compared to head (394f54d) 92.11%.

❗ 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    #2167      +/-   ##
==========================================
+ Coverage   90.94%   92.11%   +1.17%     
==========================================
  Files         104      104              
  Lines       52750    66889   +14139     
  Branches    52750    66889   +14139     
==========================================
+ Hits        47971    61612   +13641     
- Misses       4779     5277     +498

Impacted Files	Coverage Δ
lightning/src/ln/channelmanager.rs	`89.24% <76.53%> (+2.11%)`	⬆️
lightning/src/ln/channel.rs	`92.85% <92.85%> (+3.04%)`	⬆️
lightning/src/ln/payment_tests.rs	`99.26% <100.00%> (+1.69%)`	⬆️
lightning/src/ln/priv_short_conf_tests.rs	`97.60% <100.00%> (+<0.01%)`	⬆️
lightning/src/ln/reload_tests.rs	`95.63% <100.00%> (+0.02%)`	⬆️
lightning/src/ln/reorg_tests.rs	`100.00% <100.00%> (ø)`
lightning/src/sync/nostd_sync.rs	`100.00% <100.00%> (ø)`

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

TheBlueMatt · 2023-04-17T17:15:45Z

Slipping to 116.

TheBlueMatt · 2023-05-04T21:47:48Z

Rebased, now no longer based on anything.

lightning/src/ln/channelmanager.rs

wpaulino · 2023-05-08T20:40:44Z

lightning/src/ln/channelmanager.rs

+													// The ChannelMonitor that gave us this
+													// preimage is for a now-closed channel -
+													// no further updates to that channel can
+													// happen which would result in the
+													// preimage being removed, thus we're
+													// guaranteed to regenerate this claim on
+													// restart as long as the source monitor
+													// sticks around.


Does that mean we need to further keep monitors around even if they have zero claimable balances until we can unblock any dependent channels?

Uhhh, yea, kinda. I mean only until the upstream channelmonitor gets its update persisted, but indeed we don't currently have any infrastructure to let the user know whether that's the case when pruning the downstream monitor. We may want to add something like that (eg "no removing monitors while any monitors are syncing") but a few blocks of extra time should suffice in most cases.

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2023-05-09T21:27:58Z

Will rebase this on #2287 in a day or two, but that should go first i think.

lightning/src/ln/channelmanager.rs

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

wpaulino · 2023-05-23T22:01:24Z

lightning/src/ln/channel.rs

@@ -5044,10 +5044,20 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
 		self.pending_monitor_updates.is_empty()
 	}

+	pub fn complete_all_mon_updates_through(&mut self, update_id: u64) {
+		self.pending_monitor_updates.retain(|upd| upd.update.update_id > update_id);


Somewhat unrelated to this method, but can a counterparty force us to drop these updates in any way and then play a commitment onchain for which we'd need one of those dropped updates to claim funds?

That shouldn't be the case - if a monitor update is blocked/hasn't completed we should never ever ever give our peer whatever message would be required to broadcast the state included in that message. This is obviously different for payment preimages, which is why they "jump the queue".

wpaulino · 2023-05-23T22:04:20Z

lightning/src/ln/channelmanager.rs

+	/// completes a monitor update containing the payment preimage. In that case, after the inbound
+	/// edge completes, we will surface an [`Event::PaymentForwarded`] as well as unblock the
+	/// outbound edge.
+	EmitEventAndFreeOtherChannel {


Why not just name it PaymentForwarded and rename the action below to downstream_action? That fully describes the action we're performing after the update completes.

wpaulino · 2023-05-23T22:11:01Z

lightning/src/ln/channelmanager.rs

+		// support async monitor updates even in LDK 0.0.115 and once we do we'll require no
+		// downgrades to prior versions. Thus, while this would break on downgrade, we don't
+		// support it even without downgrade, so if it breaks its not on us ¯\_(ツ)_/¯.
+		(1, downstream_counterparty_and_funding_outpoint, option),


So that means we could make this even once we land the follow-up PR as part of 117?

There's no mechanism currently to do that. Sadly, downstream_counterparty_and_funding_outpoint is always set, so we can't make it even or we break all downgrades. Instead, we should make channel's pending_monitor_updates even, which it for some reason currently isn't. We'll want to do that for 116 - #2317

TheBlueMatt · 2023-05-24T04:17:01Z

Still need to do a few more followups but pushed a bunch of the more trivial changes here.

dunxen · 2023-05-24T16:51:28Z

Haven't looked at tests yet, but the rest LGTM so far.

valentinewallace · 2023-05-24T18:36:28Z

Still need to do a few more followups but pushed a bunch of the more trivial changes here.

Did you mean to push @TheBlueMatt? Also feel free to squash IMO.

TheBlueMatt · 2023-05-26T03:17:59Z

I did.

Our `no-std` locks simply panic if a lock cannot be taken as there should be no lock contention in a single-threaded environment. However, the `held_by_thread` debug methods were delegating to the lock methods which resulted in a panic when asserting that a lock *is* held by the current thread. Instead, they are updated here to call the relevant `RefCell` testing methods.

This allows us to make the `force_shutdown` definition less verbose

In the coming commits we'll need the counterparty node_id when handling a background monitor update as we may need to resume normal channel operation as a result. Thus, we go ahead and pipe it through from the shutdown end, as it makes the codepaths consistent. Sadly, the monitor-originated shutdown case doesn't allow for a required counterparty node_id as some versions of LDK didn't have it present in the ChannelMonitor.

Rather than letting `AChannelManager` be bounded by all traits being `Sized` we make them explicitly `?Sized`. We also make the trait no longer test-only as it will be used in a coming commit.

TheBlueMatt · 2023-05-30T18:16:06Z

Squashed with one further commit added at the top to fix the no-std held_by_thread debug method.

wpaulino

LGTM pending the follow-up work, but CI needs a small fix.

`BackgroundEvent` was used to store `ChannelMonitorUpdate`s which result in a channel force-close, avoiding relying on `ChannelMonitor`s having been loaded while `ChannelManager` block-connection methods are called during startup. In the coming commit(s) we'll also generate non-channel-closing `ChannelMonitorUpdate`s during startup, which will need to be replayed prior to any other `ChannelMonitorUpdate`s generated from normal operation. In the next commit we'll handle that by handling `BackgroundEvent`s immediately after locking the `total_consistency_lock`.

When we generated a `ChannelMonitorUpdate` during `ChannelManager` deserialization, we must ensure that it gets processed before any other `ChannelMonitorUpdate`s. The obvious hook for this is when taking the `total_consistency_lock`, which makes it unlikely we'll regress by forgetting this. Here we add that call in the `PersistenceNotifierGuard`, with a test-only atomic bool to test that this criteria is met.

If a `ChannelMonitorUpdate` was created and given to the user but left uncompleted when the `ChannelManager` is persisted prior to a restart, the user likely lost the `ChannelMonitorUpdate`(s). Thus, we need to replay them for the user, which we do here using the new `BackgroundEvent::MonitorUpdateRegeneratedOnStartup` variant.

When we forward a payment and receive an `update_fulfill_htlc` message from the downstream channel, we immediately claim the HTLC on the upstream channel, before even doing a `commitment_signed` dance on the downstream channel. This implies that our `ChannelMonitorUpdate`s "go out" in the right order - first we ensure we'll get our money by writing the preimage down, then we write the update that resolves giving money on the downstream node. This is safe as long as `ChannelMonitorUpdate`s complete in the order in which they are generated, but of course looking forward we want to support asynchronous updates, which may complete in any order. Here we add infrastructure to handle downstream `ChannelMonitorUpdate`s which are blocked on an upstream preimage-containing one. We don't yet actually do the blocking which will come in a future commit.

TheBlueMatt · 2023-05-30T23:05:57Z

Oops, sorry, doc bug on an intermediary commit, shuffled diff around across commits to fix it.

dunxen

LGTM.

Bit off topic: I'm happy to wait for follow-ups to this one before considering #2077 for merge as it'll probably be much less hassle for you.

Also, with current work on V2 establishment at #2302, it gives me a chance to "vet" the utility of splitting those channels.

valentinewallace

Nothing blocking!

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

valentinewallace · 2023-05-30T09:42:59Z

lightning/src/ln/channelmanager.rs

+	) -> bool {
+		actions_blocking_raa_monitor_updates
+			.get(&channel_funding_outpoint.to_channel_id()).map(|v| !v.is_empty()).unwrap_or(false)
+		|| self.pending_events.lock().unwrap().iter().any(|(_, action)| {


Would it be cleaner to get rid of EventCompletionAction::ReleaseRAAChannelMonitorUpdate and store that data in actions_blocking_raa_monitor_updates now, so RAA monitor upd blockers are only stored in one place?

Hmm, maybe? I really hate the actions_blocking_raa_monitor_updates thing - its redundant state in two places that has to always be in sync, vs the event stuff is one place that isn't redundant and checked in-place - its comparatively harder to screw up.

The only reason for actions_blocking_raa_monitor_updates is that without it we'd have to lock + walk each peer and all our channels to figure out if we're being blocked anywhere. Originally I was gonna do that if we found a blocking action but figured it was overengineering, but either way I'd kinda rather not have the issue twice, even if we have to have it once.

TheBlueMatt · 2023-05-31T22:48:14Z

Bit off topic: I'm happy to wait for follow-ups to this one before considering #2077 for merge as it'll probably be much less hassle for you.

I don't think its worth waiting, some of the followups have to wait for 0.0.117, and the next round of followups that do have to go in 116 I havent finished writing yet 😭

TheBlueMatt · 2023-05-31T22:48:31Z

Gonna get this out of the way. Given some followups are required for 116 anyway will address the above doc comments there.

dunxen · 2023-05-31T23:16:46Z

I don't think its worth waiting, some of the followups have to wait for 0.0.117, and the next round of followups that do have to go in 116 I havent finished writing yet 😭

Ok, I'll rebase in the morning and we'll see.

TheBlueMatt added this to the 0.0.115 milestone Apr 7, 2023

TheBlueMatt assigned wpaulino and valentinewallace Apr 7, 2023

This was referenced Apr 7, 2023

Handle Channel force-close with pending blocked monitors blocking another channel #2168

Closed

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

Merged

TheBlueMatt added the blocked on dependent pr label Apr 7, 2023

TheBlueMatt modified the milestones: 0.0.115, 0.0.116 Apr 17, 2023

wpaulino self-requested a review April 17, 2023 17:34

tnull mentioned this pull request Apr 18, 2023

Allow async events processing without holding total_consistency_lock #2199

Merged

TheBlueMatt force-pushed the 2023-04-monitor-e-monitor-prep branch from 13c72ab to 8624570 Compare May 4, 2023 21:46

TheBlueMatt removed the blocked on dependent pr label May 4, 2023

TheBlueMatt force-pushed the 2023-04-monitor-e-monitor-prep branch 2 times, most recently from 37a595d to ab7c327 Compare May 4, 2023 23:34

wpaulino mentioned this pull request May 8, 2023

Split prefunded Channel into Inbound/Outbound channels #2077

Merged

wpaulino reviewed May 8, 2023

View reviewed changes

TheBlueMatt mentioned this pull request May 9, 2023

Stop persisting background shutdown monitor updates #2287

Merged

TheBlueMatt added the blocked on dependent pr label May 9, 2023

valentinewallace reviewed May 9, 2023

View reviewed changes

valentinewallace removed the blocked on dependent pr label May 10, 2023

TheBlueMatt mentioned this pull request May 10, 2023

Prune monitors with zero claimable balances upon persisting #2236

Closed

TheBlueMatt force-pushed the 2023-04-monitor-e-monitor-prep branch 2 times, most recently from 3c3fb09 to 6f0c817 Compare May 10, 2023 18:31

valentinewallace reviewed May 11, 2023

View reviewed changes

TheBlueMatt force-pushed the 2023-04-monitor-e-monitor-prep branch 2 times, most recently from 34c6292 to bdd8f78 Compare May 12, 2023 05:20

wpaulino reviewed May 23, 2023

View reviewed changes

TheBlueMatt force-pushed the 2023-04-monitor-e-monitor-prep branch from 2935e00 to 9f2e9f1 Compare May 26, 2023 03:17

TheBlueMatt added 4 commits May 30, 2023 18:15

Move the ShutdownResult type alias to channel.rs

3ce1a5e

This allows us to make the `force_shutdown` definition less verbose

Make AChannelManager trait slightly more generic and always on

a298912

Rather than letting `AChannelManager` be bounded by all traits being `Sized` we make them explicitly `?Sized`. We also make the trait no longer test-only as it will be used in a coming commit.

TheBlueMatt force-pushed the 2023-04-monitor-e-monitor-prep branch from 9f2e9f1 to 5509788 Compare May 30, 2023 18:15

wpaulino mentioned this pull request May 30, 2023

Explore adding a startup method to ChannelManager #2326

Open

wpaulino reviewed May 30, 2023

View reviewed changes

TheBlueMatt added 4 commits May 30, 2023 23:00

TheBlueMatt force-pushed the 2023-04-monitor-e-monitor-prep branch from 5509788 to 394f54d Compare May 30, 2023 23:05

wpaulino approved these changes May 31, 2023

View reviewed changes

dunxen approved these changes May 31, 2023

View reviewed changes

valentinewallace approved these changes May 31, 2023

View reviewed changes

TheBlueMatt merged commit 32eb894 into lightningdevkit:main May 31, 2023

TheBlueMatt mentioned this pull request Jun 20, 2023

Re-claim forwarded HTLCs on startup #2364

Merged

TheBlueMatt mentioned this pull request Jul 5, 2023

Handle pre-startup and closed-channel monitor update completion actions #2391

Merged

TheBlueMatt mentioned this pull request Oct 13, 2024

Async Persistence TODOs #3052

Open

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add infra to block ChannelMonitorUpdates on forwarded claims #2167

Add infra to block ChannelMonitorUpdates on forwarded claims #2167

TheBlueMatt commented Apr 7, 2023 •

edited

Loading

codecov-commenter commented Apr 7, 2023 •

edited

Loading

TheBlueMatt commented Apr 17, 2023

TheBlueMatt commented May 4, 2023

wpaulino May 8, 2023

TheBlueMatt May 10, 2023

TheBlueMatt commented May 9, 2023

wpaulino May 23, 2023

TheBlueMatt May 24, 2023

wpaulino May 23, 2023

wpaulino May 23, 2023

TheBlueMatt May 24, 2023

TheBlueMatt commented May 24, 2023

dunxen commented May 24, 2023

valentinewallace commented May 24, 2023

TheBlueMatt commented May 26, 2023

TheBlueMatt commented May 30, 2023

wpaulino left a comment

TheBlueMatt commented May 30, 2023

dunxen left a comment

valentinewallace left a comment

valentinewallace May 30, 2023

TheBlueMatt May 31, 2023

TheBlueMatt commented May 31, 2023

TheBlueMatt commented May 31, 2023

dunxen commented May 31, 2023

Add infra to block ChannelMonitorUpdates on forwarded claims #2167

Add infra to block ChannelMonitorUpdates on forwarded claims #2167

Conversation

TheBlueMatt commented Apr 7, 2023 • edited Loading

codecov-commenter commented Apr 7, 2023 • edited Loading

Codecov Report

TheBlueMatt commented Apr 17, 2023

TheBlueMatt commented May 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented May 9, 2023

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

TheBlueMatt commented May 24, 2023

dunxen commented May 24, 2023

valentinewallace commented May 24, 2023

TheBlueMatt commented May 26, 2023

TheBlueMatt commented May 30, 2023

wpaulino left a comment

Choose a reason for hiding this comment

TheBlueMatt commented May 30, 2023

dunxen left a comment

Choose a reason for hiding this comment

valentinewallace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented May 31, 2023

TheBlueMatt commented May 31, 2023

dunxen commented May 31, 2023

TheBlueMatt commented Apr 7, 2023 •

edited

Loading

codecov-commenter commented Apr 7, 2023 •

edited

Loading