Disallow dual-sync-async persistence without restarting #3737

TheBlueMatt · 2025-04-15T15:32:02Z

In general, we don't expect users to persist
`ChannelMonitor[Update]`s both synchronously and asynchronously for
a single `ChannelManager` instance. If a user has implemented
asynchronous persistence, they should generally always use that,
as there is then no advantage to them to occasionally persist
synchronously.

Even still, in 920d96edb6595289902f287419de2d002e2dc2ee we fixed
some bugs related to such operation, and noted that "there isn't
much cost to supporting it". Sadly, this is not true.

Specifically, the dual-sync-async persistence flow is ill-defined
and difficult to define in away that a user can realistically
implement.

Consider the case of a `ChannelMonitorUpdate` which is persisted
asynchronously and while it is still being persisted a new
`ChannelMonitorUpdate` is created. If the second
`ChannelMonitorUpdate` is persisted synchronously, the
`ChannelManager` will be left with a single pending
`ChannelMonitorUpdate` which is not the latest.

If we were to then restart, the latest copy of the `ChannelMonitor`
would be that without any updates, but the `ChannelManager` has a
pending `ChannelMonitorUpdate` for the next update, but not the one
after that. The user would then have to handle the replayed
`ChannelMonitorUpdate` and then find the second
`ChannelMonitorUpdate` on disk and somehow know to replay that one
as well.

Further, we currently have a bug in handling this scenario as we'll
complete all pending post-update actions when the second
`ChannelMonitorUpdate` gets persisted synchronously, even though
the first `ChannelMonitorUpdate` is still pending. While we could
rather trivially fix these issues, addressing the larger API
question above is difficult and as we don't anticipate this
use-case being important, we just disable it here.

Note that we continue to support it internally as some 39 tests
rely on it.

Issue highlighted by (changes to the) chanmon_consistency fuzz
target (in the next commit).

ldk-reviews-bot · 2025-04-15T15:32:05Z

👋 Thanks for assigning @valentinewallace as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

lightning/src/ln/channelmanager.rs

valentinewallace · 2025-04-15T18:40:46Z

We may be able to remove the test test_sync_async_persist_doesnt_hang since it specifically covers async + sync persistence.

valentinewallace · 2025-04-16T17:26:29Z

Would it be a good take-a-Friday issue to fix the legacy tests that use both sync + async so we don't need to gate the panics anymore?

fuzz/src/chanmon_consistency.rs

lightning/src/chain/mod.rs

valentinewallace · 2025-04-16T18:05:15Z

lightning/src/ln/channelmanager.rs

+	/// We only support using one of [`ChannelMonitorUpdateStatus::InProgress`] and
+	/// [`ChannelMonitorUpdateStatus::Completed`] without restarting. Because the API does not


Some thoughts: is there a way to enforce this with the compiler? And is it a goal of the project to continue supporting both sync + async persistence forever?

I suppose we could have some kind of flag you set on startup which indicates which persistence method you're using, but I'm sure really sure that it would be an issue in practice - we expect users to use either Persist or some future AsyncPersist which will handle this stuff for them, and I don't really see why someone would ever persist sometimes-sync-sometimes-async - either your disk is slow and you need async or you don't.

We definitely want to support both at the project level, though, for the same reason we want to support async and sync everywhere in the project - Rust-async-native projects are becoming more common, but Rust-non-async projects also exist (as well as platforms where that's required) as well as our current bindings are not async.

We definitely want to support both at the project level, though, for the same reason we want to support async and sync everywhere in the project - Rust-async-native projects are becoming more common, but Rust-non-async projects also exist (as well as platforms where that's required) as well as our current bindings are not async.

To clarify, by sync + async persistence I mean supporting either returning ChannelMonitorUpdateStatus::Completed or ::InProgress. It feels like we expect most production users to persist data asynchronously and then later call channel_monitor_updated, so it could be reasonable to drop support for the ::Completed option down the line. Seems like users that are the exception to that can just call channel_monitor_updated immediately, as well.

Maybe? I imagine many small nodes will still want the simplicity of sync.

Maybe, seems worth exploring though. Bookmarking to discuss further.

Yea, maybe when we're super 100% on the stability of async persist? It wouldn't free up a lot of code though cause it's all the same pathway now anyway.

A luxury API for a minority group of sync users may not be the best choice long term?

But if sync users would need to use async with an immediate callback now, I think it is no longer so clear that async is not production ready?

Hopefully we can get to super 100% soon.

But if sync users would need to use async with an immediate callback now, I think it is no longer so clear that async is not production ready?

Hmm? This PR is not requiring that.

fuzz/src/chanmon_consistency.rs

valentinewallace · 2025-04-16T18:21:33Z

fuzz/src/chanmon_consistency.rs

+	let empty_node_a_ser = node_a.encode();
+	let empty_node_b_ser = node_b.encode();
+	let empty_node_c_ser = node_c.encode();


These don't seem to be used anywhere?

Oops, part of a later patch :)

codecov · 2025-04-16T20:30:45Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.26%. Comparing base (83e9e80) to head (cb6219d).
Report is 60 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3737      +/-   ##
==========================================
+ Coverage   89.12%   90.26%   +1.13%     
==========================================
  Files         156      158       +2     
  Lines      123514   134266   +10752     
  Branches   123514   134266   +10752     
==========================================
+ Hits       110086   121195   +11109     
+ Misses      10749    10536     -213     
+ Partials     2679     2535     -144

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

TheBlueMatt · 2025-04-16T20:35:08Z

Would it be a good take-a-Friday issue to fix the legacy tests that use both sync + async so we don't need to gate the panics anymore?

Yea, I imagine its a good bit of work, though.

wpaulino · 2025-04-28T17:18:05Z

Needs a squash

fuzz/src/chanmon_consistency.rs

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2025-04-29T13:43:23Z

Squashed with a minor comment fix:

$ git diff-tree -U1 ddccbdabd d009b3918
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index c86eed77b..54b607d88 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -2573,4 +2573,4 @@ where
 	/// [`ChannelMonitorUpdateStatus::Completed`] without restarting. Because the API does not
-	/// otherwise directly enforce this, we enforce it in debug builds here by storing which one is
-	/// in use.
+	/// otherwise directly enforce this, we enforce it in non-test builds here by storing which one
+	/// is in use.
 	#[cfg(not(test))]

wpaulino · 2025-04-29T17:56:15Z

These externalized tests seem to be failing:

- test_batch_funding_close_after_funding_signed
- test_batch_channel_open

In general, we don't expect users to persist `ChannelMonitor[Update]`s both synchronously and asynchronously for a single `ChannelManager` instance. If a user has implemented asynchronous persistence, they should generally always use that, as there is then no advantage to them to occasionally persist synchronously. Even still, in 920d96e we fixed some bugs related to such operation, and noted that "there isn't much cost to supporting it". Sadly, this is not true. Specifically, the dual-sync-async persistence flow is ill-defined and difficult to define in away that a user can realistically implement. Consider the case of a `ChannelMonitorUpdate` which is persisted asynchronously and while it is still being persisted a new `ChannelMonitorUpdate` is created. If the second `ChannelMonitorUpdate` is persisted synchronously, the `ChannelManager` will be left with a single pending `ChannelMonitorUpdate` which is not the latest. If we were to then restart, the latest copy of the `ChannelMonitor` would be that without any updates, but the `ChannelManager` has a pending `ChannelMonitorUpdate` for the next update, but not the one after that. The user would then have to handle the replayed `ChannelMonitorUpdate` and then find the second `ChannelMonitorUpdate` on disk and somehow know to replay that one as well. Further, we currently have a bug in handling this scenario as we'll complete all pending post-update actions when the second `ChannelMonitorUpdate` gets persisted synchronously, even though the first `ChannelMonitorUpdate` is still pending. While we could rather trivially fix these issues, addressing the larger API question above is difficult and as we don't anticipate this use-case being important, we just disable it here. Note that we continue to support it internally as some 39 tests rely on it. We do, however, remove test_sync_async_persist_doesnt_hang which was added specifically to test for this use-case, and we now do not support it. Issue highlighted by (changes to the) chanmon_consistency fuzz target (in the next commit).

TheBlueMatt · 2025-04-30T01:21:31Z

Oof, ugh, right. Squashed-pushed a fix:

$ git diff-tree -U1 d009b3918 cb6219df6
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 54b607d88..34751bfc9 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -2575,3 +2575,3 @@ where
 	/// is in use.
-	#[cfg(not(test))]
+	#[cfg(not(any(test, feature = "_externalize_tests")))]
 	monitor_update_type: AtomicUsize,
@@ -3321,3 +3321,3 @@ macro_rules! handle_new_monitor_update {
 			ChannelMonitorUpdateStatus::InProgress => {
-				#[cfg(not(test))]
+				#[cfg(not(any(test, feature = "_externalize_tests")))]
 				if $self.monitor_update_type.swap(1, Ordering::Relaxed) == 2 {
@@ -3330,3 +3330,3 @@ macro_rules! handle_new_monitor_update {
 			ChannelMonitorUpdateStatus::Completed => {
-				#[cfg(not(test))]
+				#[cfg(not(any(test, feature = "_externalize_tests")))]
 				if $self.monitor_update_type.swap(2, Ordering::Relaxed) == 1 {
@@ -3594,3 +3594,3 @@ where

-			#[cfg(not(test))]
+			#[cfg(not(any(test, feature = "_externalize_tests")))]
 			monitor_update_type: AtomicUsize::new(0),
@@ -14767,3 +14767,3 @@ where

-			#[cfg(not(test))]
+			#[cfg(not(any(test, feature = "_externalize_tests")))]
 			monitor_update_type: AtomicUsize::new(0),

joostjager

Wondering whether the problem where chan mgr will be left with a single pending update which is not the latest can also occur when purely async is used, but channel_monitor_updated are called in the reverse order?

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2025-04-30T15:04:59Z

Wondering whether the problem where chan mgr will be left with a single pending update which is not the latest can also occur when purely async is used, but channel_monitor_updated are called in the reverse order?

MonitorEvent::Completed is only allowed to signal "all updates to and through the given id have completed" to avoid this case. ChainMonitor tracks this and generates the right MonitorEvetns.

valentinewallace · 2025-05-01T19:44:28Z

LGTM after squash

TheBlueMatt · 2025-05-02T00:35:58Z

Re-dropped the constant after discussion.

joostjager

I just posted that drive-by comment, but was happy to review the full PR as requested as it is context that I probably need for my own future work too.

Just see what you want to do with the comments, as this PR was already in the later stages with two reviewers on it already.

joostjager · 2025-05-02T09:23:09Z

fuzz/src/chanmon_consistency.rs

-			for (channel_id, monitor) in monitors.iter() {
-				monitor_refs.insert(*channel_id, monitor);
-			}
+	let reload_node = |ser: &Vec<u8>,


Do this as the first commit to reduce total delta? I think it's better for review when refactors come before functional changes in a PR.

Is it really worth changing? Indeed, its generally nicer, but its three extra lines of diff and I wrote it the other way originally, not sure its worth bothering to change.

No, not blocking.

fuzz/src/chanmon_consistency.rs

joostjager · 2025-05-02T09:29:53Z

fuzz/src/chanmon_consistency.rs

@@ -680,8 +679,8 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out, anchors: bool) {
 	let mon_style = [default_mon_style.clone(), default_mon_style.clone(), default_mon_style];

 	macro_rules! reload_node {
-		($ser: expr, $node_id: expr, $old_monitors: expr, $keys_manager: expr, $fee_estimator: expr) => {{
-			let keys_manager = Arc::clone(&$keys_manager);
+		($ser: expr, $node_id: expr, $old_monitors: expr, $use_old_mons: expr, $keys: expr, $fee_estimator: expr) => {{


use_old_mons could be an enum for readability and then match 0x2a | 0x2b | 0x2c etc directly to enum values. Makes it all a bit more explicit with fewer explanation about +1 +2 required.

I went ahead and broke backwards compat so I don't think this is worth it anymore (its always just passing v).

joostjager · 2025-05-02T09:31:00Z

fuzz/src/chanmon_consistency.rs

@@ -1514,12 +1524,16 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out, anchors: bool) {
 					ab_events.clear();
 					ba_events.clear();
 				}
+				// Note that we ensure 0x2c represents "use oldest monitor" to retain backwards
+				// compatibility with existing fuzz corpuses by using setting (v + 1) % 3 == 0


Good one to keep in mind existing corpuses.

joostjager · 2025-05-02T09:47:17Z

fuzz/src/chanmon_consistency.rs

-		($ser: expr, $node_id: expr, $old_monitors: expr, $keys_manager: expr, $fee_estimator: expr) => {{
-			let keys_manager = Arc::clone(&$keys_manager);
+		($ser: expr, $node_id: expr, $old_monitors: expr, $use_old_mons: expr, $keys: expr, $fee_estimator: expr) => {{
+			let keys_manager = Arc::clone(&$keys);


I think the arc/clone/keys thing is an unrelated refactor?

Hmm? Those already existed (the only change is that the fee_estimator clone was changed to Arc::clone rather than the implicit clone for consistency, but its calling the same method).

joostjager · 2025-05-02T09:57:11Z

fuzz/src/chanmon_consistency.rs

@@ -674,6 +676,9 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out, anchors: bool) {
 		}};
 	}

+	let default_mon_style = RefCell::new(ChannelMonitorUpdateStatus::Completed);


Are the changes to this file a preparation for the next commit somehow and/or are there also independent refactors in here? I struggle a bit to see the link with the chanmgr change.

Its because we're no longer allowed to switch monitor connection styles mid-stream. So instead we're changing to storing the monitor style we'll start applying on the next node reload here, and applying it only on reload.

joostjager · 2025-05-02T09:58:53Z

fuzz/src/chanmon_consistency.rs

-							break;
+							if last_pass_no_updates {
+								// In some cases, we may generate a message to send in
+								// `process_msg_events`, but block sending until


Is this pre-existing?

No, because in the past we would switch to sync persist before we get here, but now we have to keep it async.

joostjager · 2025-05-02T10:06:39Z

lightning/src/ln/channelmanager.rs

+	/// We only support using one of [`ChannelMonitorUpdateStatus::InProgress`] and
+	/// [`ChannelMonitorUpdateStatus::Completed`] without restarting. Because the API does not


A luxury API for a minority group of sync users may not be the best choice long term?

But if sync users would need to use async with an immediate callback now, I think it is no longer so clear that async is not production ready?

Hopefully we can get to super 100% soon.

When we reload a node in the `chanmon_consistency` fuzzer, we always reload with the latest `ChannelMonitor` state which was confirmed as persisted to the running `ChannelManager`. This is nice in that it tests losing the latest `ChannelMonitor`, but there may also be bugs in the on-startup `ChannelMonitor` replay. Thus, here, we optionally reload with a newer `ChannelMonitor` than the last-persisted one. Note that this breaks backwards compat for existing `chanmon_consistency` tests, requiring that 0x2c bytes be replaced with 0xb1, 0x2d with 0xb4 and 0x2e with 0xbd.

Since we broke backwards compat and changed the values of bytes for reloading nodes in the `chanmon_consistency` fuzzer, here we move them down to the right position.

`chanmon_consistency` was originally written with lots of macros due to some misguided concept of code being unrolled at compile-time. This is, of course, a terrible idea not just for compile times but also for performance. Here, we make `reload_node` a function in anticipation of it being used in additional places in future work.

valentinewallace

Good with a squash

valentinewallace · 2025-05-02T15:47:09Z

fuzz/src/chanmon_consistency.rs

+			} else if use_old_mons % 3 == 1 {
+				// Reload with the second-oldest `ChannelMonitor`
+				let old_mon = prev_state.persisted_monitor;
+				prev_state.pending_monitors.drain(..).next().map(|(_, v)| v).unwrap_or(old_mon)


Could you comment why we're emptying pending_monitors rather than just removing the first one? We don't clear the vec below so it reads a bit inconsistent to me

valentinewallace · 2025-05-02T16:00:40Z

fuzz/src/chanmon_consistency.rs

+				// Note that we ensure 0x2d represents "use oldest monitor" to retain backwards
+				// compatibility with existing fuzz corpuses by using setting v % 3 == 0


This is outdated now

TheBlueMatt added this to the 0.2 milestone Apr 15, 2025

ldk-reviews-bot requested a review from valentinewallace April 15, 2025 15:42

wpaulino reviewed Apr 15, 2025

View reviewed changes

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

TheBlueMatt force-pushed the 2025-04-no-dual-sync-async branch from 4e41cb7 to 70f9409 Compare April 16, 2025 14:37

valentinewallace reviewed Apr 16, 2025

View reviewed changes

TheBlueMatt force-pushed the 2025-04-no-dual-sync-async branch from 70f9409 to ddccbda Compare April 16, 2025 19:58

valentinewallace reviewed Apr 28, 2025

View reviewed changes

fuzz/src/chanmon_consistency.rs Show resolved Hide resolved

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

TheBlueMatt force-pushed the 2025-04-no-dual-sync-async branch from ddccbda to d009b39 Compare April 29, 2025 13:42

TheBlueMatt requested review from wpaulino and valentinewallace April 29, 2025 13:43

valentinewallace previously approved these changes Apr 29, 2025

View reviewed changes

wpaulino previously approved these changes Apr 29, 2025

View reviewed changes

TheBlueMatt dismissed stale reviews from wpaulino and valentinewallace via cb6219d April 30, 2025 01:21

TheBlueMatt force-pushed the 2025-04-no-dual-sync-async branch from d009b39 to cb6219d Compare April 30, 2025 01:21

joostjager reviewed Apr 30, 2025

View reviewed changes

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

wpaulino previously approved these changes Apr 30, 2025

View reviewed changes

TheBlueMatt dismissed wpaulino’s stale review via 3805ca1 May 1, 2025 01:00

TheBlueMatt force-pushed the 2025-04-no-dual-sync-async branch from cb6219d to 3805ca1 Compare May 1, 2025 01:00

TheBlueMatt force-pushed the 2025-04-no-dual-sync-async branch from 3805ca1 to 90aec07 Compare May 2, 2025 00:35

TheBlueMatt requested review from joostjager and valentinewallace May 2, 2025 00:36

joostjager reviewed May 2, 2025

View reviewed changes

TheBlueMatt added 4 commits May 2, 2025 13:43

f reload channels differently

da9273d

Move reload_node fuzz entries into the right position

3b2e8e3

Since we broke backwards compat and changed the values of bytes for reloading nodes in the `chanmon_consistency` fuzzer, here we move them down to the right position.

TheBlueMatt force-pushed the 2025-04-no-dual-sync-async branch from 90aec07 to d27c161 Compare May 2, 2025 13:57

valentinewallace reviewed May 2, 2025

View reviewed changes

		/// We only support using one of [`ChannelMonitorUpdateStatus::InProgress`] and
		/// [`ChannelMonitorUpdateStatus::Completed`] without restarting. Because the API does not

		// Note that we ensure 0x2d represents "use oldest monitor" to retain backwards
		// compatibility with existing fuzz corpuses by using setting v % 3 == 0

Disallow dual-sync-async persistence without restarting #3737

Are you sure you want to change the base?

Disallow dual-sync-async persistence without restarting #3737

Conversation

TheBlueMatt commented Apr 15, 2025

ldk-reviews-bot commented Apr 15, 2025 • edited Loading

valentinewallace commented Apr 15, 2025

valentinewallace commented Apr 16, 2025

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 16, 2025 • edited Loading

Codecov Report

TheBlueMatt commented Apr 16, 2025

wpaulino commented Apr 28, 2025

TheBlueMatt commented Apr 29, 2025

wpaulino commented Apr 29, 2025

TheBlueMatt commented Apr 30, 2025

joostjager left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Apr 30, 2025

valentinewallace commented May 1, 2025

TheBlueMatt commented May 2, 2025

joostjager 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

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

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

ldk-reviews-bot commented Apr 15, 2025 •

edited

Loading

codecov bot commented Apr 16, 2025 •

edited

Loading