Disallow dual-sync-async persistence without restarting

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

Author: TheBlueMatt

fuzz/src/chanmon_consistency.rs

@@ -82,6 +82,8 @@ use bitcoin::secp256k1::{self, Message, PublicKey, Scalar, Secp256k1, SecretKey}
 
 use lightning::io::Cursor;
 use lightning::util::dyn_signer::DynSigner;
+
+use std::cell::RefCell;
 use std::cmp::{self, Ordering};
 use std::mem;
 use std::sync::atomic;
@@ -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);
+	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);
@@ -746,6 +751,7 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out, anchors: bool) {
 					Ok(ChannelMonitorUpdateStatus::Completed)
 				);
 			}
+			*chain_monitor.persister.update_ret.lock().unwrap() = *mon_style[$node_id].borrow();
 			res
 		}};
 	}
@@ -1393,28 +1399,22 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out, anchors: bool) {
 			// bit-twiddling mutations to have similar effects. This is probably overkill, but no
 			// harm in doing so.
 			0x00 => {
-				*monitor_a.persister.update_ret.lock().unwrap() =
-					ChannelMonitorUpdateStatus::InProgress
+				*mon_style[0].borrow_mut() = ChannelMonitorUpdateStatus::InProgress;
 			},
 			0x01 => {
-				*monitor_b.persister.update_ret.lock().unwrap() =
-					ChannelMonitorUpdateStatus::InProgress
+				*mon_style[1].borrow_mut() = ChannelMonitorUpdateStatus::InProgress;
 			},
 			0x02 => {
-				*monitor_c.persister.update_ret.lock().unwrap() =
-					ChannelMonitorUpdateStatus::InProgress
+				*mon_style[2].borrow_mut() = ChannelMonitorUpdateStatus::InProgress;
 			},
 			0x04 => {
-				*monitor_a.persister.update_ret.lock().unwrap() =
-					ChannelMonitorUpdateStatus::Completed
+				*mon_style[0].borrow_mut() = ChannelMonitorUpdateStatus::Completed;
 			},
 			0x05 => {
-				*monitor_b.persister.update_ret.lock().unwrap() =
-					ChannelMonitorUpdateStatus::Completed
+				*mon_style[1].borrow_mut() = ChannelMonitorUpdateStatus::Completed;
 			},
 			0x06 => {
-				*monitor_c.persister.update_ret.lock().unwrap() =
-					ChannelMonitorUpdateStatus::Completed
+				*mon_style[2].borrow_mut() = ChannelMonitorUpdateStatus::Completed;
 			},
 
 			0x08 => complete_all_monitor_updates(&monitor_a, &chan_1_id),
@@ -1722,21 +1722,8 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out, anchors: bool) {
 			0xff => {
 				// Test that no channel is in a stuck state where neither party can send funds even
 				// after we resolve all pending events.
-				// First make sure there are no pending monitor updates and further update
-				// operations complete.
-				*monitor_a.persister.update_ret.lock().unwrap() =
-					ChannelMonitorUpdateStatus::Completed;
-				*monitor_b.persister.update_ret.lock().unwrap() =
-					ChannelMonitorUpdateStatus::Completed;
-				*monitor_c.persister.update_ret.lock().unwrap() =
-					ChannelMonitorUpdateStatus::Completed;
-
-				complete_all_monitor_updates(&monitor_a, &chan_1_id);
-				complete_all_monitor_updates(&monitor_b, &chan_1_id);
-				complete_all_monitor_updates(&monitor_b, &chan_2_id);
-				complete_all_monitor_updates(&monitor_c, &chan_2_id);
-
-				// Next, make sure peers are all connected to each other
+
+				// First, make sure peers are all connected to each other
 				if chan_a_disconnected {
 					let init_1 = Init {
 						features: nodes[1].init_features(),
@@ -1769,42 +1756,65 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out, anchors: bool) {
 				}
 
 				macro_rules! process_all_events {
-					() => {
+					() => { {
+						let mut last_pass_no_updates = false;
 						for i in 0..std::usize::MAX {
 							if i == 100 {
 								panic!("It may take may iterations to settle the state, but it should not take forever");
 							}
+							// Next, make sure no monitor updates are pending
+							complete_all_monitor_updates(&monitor_a, &chan_1_id);
+							complete_all_monitor_updates(&monitor_b, &chan_1_id);
+							complete_all_monitor_updates(&monitor_b, &chan_2_id);
+							complete_all_monitor_updates(&monitor_c, &chan_2_id);
 							// Then, make sure any current forwards make their way to their destination
 							if process_msg_events!(0, false, ProcessMessages::AllMessages) {
+								last_pass_no_updates = false;
 								continue;
 							}
 							if process_msg_events!(1, false, ProcessMessages::AllMessages) {
+								last_pass_no_updates = false;
 								continue;
 							}
 							if process_msg_events!(2, false, ProcessMessages::AllMessages) {
+								last_pass_no_updates = false;
 								continue;
 							}
 							// ...making sure any pending PendingHTLCsForwardable events are handled and
 							// payments claimed.
 							if process_events!(0, false) {
+								last_pass_no_updates = false;
 								continue;
 							}
 							if process_events!(1, false) {
+								last_pass_no_updates = false;
 								continue;
 							}
 							if process_events!(2, false) {
+								last_pass_no_updates = false;
 								continue;
 							}
-							break;
+							if last_pass_no_updates {
+								// In some cases, we may generate a message to send in
+								// `process_msg_events`, but block sending until
+								// `complete_all_monitor_updates` gets called on the next
+								// iteration.
+								//
+								// Thus, we only exit if we manage two iterations with no messages
+								// or events to process.
+								break;
+							}
+							last_pass_no_updates = true;
 						}
-					};
+					} };
 				}
 
-				// At this point, we may be pending quiescence, so we'll process all messages to
-				// ensure we can complete its handshake. We'll then exit quiescence and process all
-				// messages again, to resolve any pending HTLCs (only irrevocably committed ones)
-				// before attempting to send more payments.
+				// We may be pending quiescence, so first process all messages to ensure we can
+				// complete the quiescence handshake.
 				process_all_events!();
+
+				// Then exit quiescence and process all messages again, to resolve any pending
+				// HTLCs (only irrevocably committed ones) before attempting to send more payments.
 				nodes[0].exit_quiescence(&nodes[1].get_our_node_id(), &chan_a_id).unwrap();
 				nodes[1].exit_quiescence(&nodes[0].get_our_node_id(), &chan_a_id).unwrap();
 				nodes[1].exit_quiescence(&nodes[2].get_our_node_id(), &chan_b_id).unwrap();

lightning/src/chain/mod.rs

@@ -209,6 +209,12 @@ pub enum ChannelMonitorUpdateStatus {
 	///
 	/// This includes performing any `fsync()` calls required to ensure the update is guaranteed to
 	/// be available on restart even if the application crashes.
+	///
+	/// If you return this variant, you cannot later return [`InProgress`] from the same instance of
+	/// [`Persist`]/[`Watch`] without first restarting.
+	///
+	/// [`InProgress`]: ChannelMonitorUpdateStatus::InProgress
+	/// [`Persist`]: chainmonitor::Persist
 	Completed,
 	/// Indicates that the update will happen asynchronously in the background or that a transient
 	/// failure occurred which is being retried in the background and will eventually complete.
@@ -234,7 +240,12 @@ pub enum ChannelMonitorUpdateStatus {
 	/// reliable, this feature is considered beta, and a handful of edge-cases remain. Until the
 	/// remaining cases are fixed, in rare cases, *using this feature may lead to funds loss*.
 	///
+	/// If you return this variant, you cannot later return [`Completed`] from the same instance of
+	/// [`Persist`]/[`Watch`] without first restarting.
+	///
 	/// [`InProgress`]: ChannelMonitorUpdateStatus::InProgress
+	/// [`Completed`]: ChannelMonitorUpdateStatus::Completed
+	/// [`Persist`]: chainmonitor::Persist
 	InProgress,
 	/// Indicates that an update has failed and will not complete at any point in the future.
 	///

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -3341,93 +3341,6 @@ fn test_durable_preimages_on_closed_channel() {
 	do_test_durable_preimages_on_closed_channel(false, false, false);
 }
 
-#[test]
-fn test_sync_async_persist_doesnt_hang() {
-	// Previously, we checked if a channel was a candidate for making forward progress based on if
-	// the `MonitorEvent::Completed` matched the channel's latest monitor update id. However, this
-	// could lead to a rare race when `ChannelMonitor`s were being persisted both synchronously and
-	// asynchronously leading to channel hangs.
-	//
-	// To hit this case, we need to generate a `MonitorEvent::Completed` prior to a new channel
-	// update, but which is only processed after the channel update.
-	let chanmon_cfgs = create_chanmon_cfgs(2);
-	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
-	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
-	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
-
-	let chan_id_ab = create_announced_chan_between_nodes(&nodes, 0, 1).2;
-
-	// Send two payments from A to B, then claim the first, marking the very last
-	// ChannelMonitorUpdate as InProgress...
-	let (payment_preimage_1, payment_hash_1, ..) = route_payment(&nodes[0], &[&nodes[1]], 1_000_000);
-	let (payment_preimage_2, payment_hash_2, ..) = route_payment(&nodes[0], &[&nodes[1]], 1_000_000);
-
-	nodes[1].node.claim_funds(payment_preimage_1);
-	check_added_monitors(&nodes[1], 1);
-	expect_payment_claimed!(nodes[1], payment_hash_1, 1_000_000);
-
-	let bs_updates = get_htlc_update_msgs(&nodes[1], &nodes[0].node.get_our_node_id());
-	nodes[0].node.handle_update_fulfill_htlc(nodes[1].node.get_our_node_id(), &bs_updates.update_fulfill_htlcs[0]);
-	expect_payment_sent(&nodes[0], payment_preimage_1, None, false, false);
-	nodes[0].node.handle_commitment_signed_batch_test(nodes[1].node.get_our_node_id(), &bs_updates.commitment_signed);
-	check_added_monitors(&nodes[0], 1);
-	let (as_raa, as_cs) = get_revoke_commit_msgs!(nodes[0], nodes[1].node.get_our_node_id());
-
-	nodes[1].node.handle_revoke_and_ack(nodes[0].node.get_our_node_id(), &as_raa);
-	check_added_monitors(&nodes[1], 1);
-	nodes[1].node.handle_commitment_signed_batch_test(nodes[0].node.get_our_node_id(), &as_cs);
-	check_added_monitors(&nodes[1], 1);
-
-	let bs_final_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[0].node.get_our_node_id());
-	chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress);
-	nodes[0].node.handle_revoke_and_ack(nodes[1].node.get_our_node_id(), &bs_final_raa);
-	check_added_monitors(&nodes[0], 1);
-
-	// Immediately complete the monitor update, but before the ChannelManager has a chance to see
-	// the MonitorEvent::Completed, create a channel update by receiving a claim on the second
-	// payment.
-	let (_, ab_update_id) = nodes[0].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_id_ab).unwrap().clone();
-	nodes[0].chain_monitor.chain_monitor.channel_monitor_updated(chan_id_ab, ab_update_id).unwrap();
-
-	nodes[1].node.claim_funds(payment_preimage_2);
-	check_added_monitors(&nodes[1], 1);
-	expect_payment_claimed!(nodes[1], payment_hash_2, 1_000_000);
-
-	let bs_updates = get_htlc_update_msgs(&nodes[1], &nodes[0].node.get_our_node_id());
-	nodes[0].node.handle_update_fulfill_htlc(nodes[1].node.get_our_node_id(), &bs_updates.update_fulfill_htlcs[0]);
-	nodes[0].node.handle_commitment_signed_batch_test(nodes[1].node.get_our_node_id(), &bs_updates.commitment_signed);
-	check_added_monitors(&nodes[0], 1);
-
-	// At this point, we have completed an extra `ChannelMonitorUpdate` but the `ChannelManager`
-	// hasn't yet seen our `MonitorEvent::Completed`. When we call
-	// `get_and_clear_pending_msg_events` here, the `ChannelManager` finally sees that event and
-	// should return the channel to normal operation.
-	let (as_raa, as_cs) = get_revoke_commit_msgs!(nodes[0], nodes[1].node.get_our_node_id());
-
-	// Now that we've completed our test, process the events we have queued up (which we were not
-	// able to check until now as they would have caused the `ChannelManager` to look at the
-	// pending `MonitorEvent`s).
-	let pending_events = nodes[0].node.get_and_clear_pending_events();
-	assert_eq!(pending_events.len(), 2);
-	if let Event::PaymentPathSuccessful { ref payment_hash, ..} = pending_events[1] {
-		assert_eq!(payment_hash.unwrap(), payment_hash_1);
-	} else { panic!(); }
-	if let Event::PaymentSent { ref payment_hash, ..} = pending_events[0] {
-		assert_eq!(*payment_hash, payment_hash_2);
-	} else { panic!(); }
-
-	// Finally, complete the claiming of the second payment
-	nodes[1].node.handle_revoke_and_ack(nodes[0].node.get_our_node_id(), &as_raa);
-	check_added_monitors(&nodes[1], 1);
-	nodes[1].node.handle_commitment_signed_batch_test(nodes[0].node.get_our_node_id(), &as_cs);
-	check_added_monitors(&nodes[1], 1);
-
-	let bs_final_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[0].node.get_our_node_id());
-	nodes[0].node.handle_revoke_and_ack(nodes[1].node.get_our_node_id(), &bs_final_raa);
-	check_added_monitors(&nodes[0], 1);
-	expect_payment_path_successful!(nodes[0]);
-}
-
 fn do_test_reload_mon_update_completion_actions(close_during_reload: bool) {
 	// Test that if a `ChannelMonitorUpdate` completes but a `ChannelManager` isn't serialized
 	// before restart we run the monitor update completion action on startup.

lightning/src/ln/channelmanager.rs

@@ -2569,6 +2569,13 @@ where
 	#[cfg(any(test, feature = "_test_utils"))]
 	pub(super) per_peer_state: FairRwLock<HashMap<PublicKey, Mutex<PeerState<SP>>>>,
 
+	/// We only support using one of [`ChannelMonitorUpdateStatus::InProgress`] and
+	/// [`ChannelMonitorUpdateStatus::Completed`] without restarting. Because the API does not
+	/// otherwise directly enforce this, we enforce it in non-test builds here by storing which one
+	/// is in use.
+	#[cfg(not(any(test, feature = "_externalize_tests")))]
+	monitor_update_type: AtomicUsize,
+
 	/// The set of events which we need to give to the user to handle. In some cases an event may
 	/// require some further action after the user handles it (currently only blocking a monitor
 	/// update from being handed to the user to ensure the included changes to the channel state
@@ -3312,11 +3319,19 @@ macro_rules! handle_new_monitor_update {
 				panic!("{}", err_str);
 			},
 			ChannelMonitorUpdateStatus::InProgress => {
+				#[cfg(not(any(test, feature = "_externalize_tests")))]
+				if $self.monitor_update_type.swap(1, Ordering::Relaxed) == 2 {
+					panic!("Cannot use both ChannelMonitorUpdateStatus modes InProgress and Completed without restart");
+				}
 				log_debug!($logger, "ChannelMonitor update for {} in flight, holding messages until the update completes.",
 					$channel_id);
 				false
 			},
 			ChannelMonitorUpdateStatus::Completed => {
+				#[cfg(not(any(test, feature = "_externalize_tests")))]
+				if $self.monitor_update_type.swap(2, Ordering::Relaxed) == 1 {
+					panic!("Cannot use both ChannelMonitorUpdateStatus modes InProgress and Completed without restart");
+				}
 				$completed;
 				true
 			},
@@ -3577,6 +3592,9 @@ where
 
 			per_peer_state: FairRwLock::new(new_hash_map()),
 
+			#[cfg(not(any(test, feature = "_externalize_tests")))]
+			monitor_update_type: AtomicUsize::new(0),
+
 			pending_events: Mutex::new(VecDeque::new()),
 			pending_events_processor: AtomicBool::new(false),
 			pending_background_events: Mutex::new(Vec::new()),
@@ -14747,6 +14765,9 @@ where
 
 			per_peer_state: FairRwLock::new(per_peer_state),
 
+			#[cfg(not(any(test, feature = "_externalize_tests")))]
+			monitor_update_type: AtomicUsize::new(0),
+
 			pending_events: Mutex::new(pending_events_read),
 			pending_events_processor: AtomicBool::new(false),
 			pending_background_events: Mutex::new(pending_background_events),