Do not fail to load ChannelManager when we see claiming payments

When we begin claiming a payment, we move the tracking of it from
`claimable_payments` to `claiming_payments`. This ensures we only
ever have one payment which is in the process of being claimed with
a given payment hash at a time and lets us keep track of when all
parts have been claimed with their `ChannelMonitor`s.

However, on startup, we check that failing to move a payment from
`claimable_payments` to `claiming_payments` we check that it is not
present in `claiming_payments`. This is fine if the payment doesn't
exist, but if the payment has already started being claimed, this
will fail and we'll refuse to deserialize the `ChannelManager`
(with a `debug_assert` failure in debug mode).

Here we resolve this by checking if a payment is already being
claimed before we attempt to initiate claiming and skip the failing
check in that case.

Author: TheBlueMatt

lightning/src/ln/channelmanager.rs

@@ -14857,21 +14857,27 @@ where
 						// preimages eventually timing out from ChannelMonitors to prevent us from
 						// doing so forever.
 
-						let claim_found =
-							channel_manager.claimable_payments.lock().unwrap().begin_claiming_payment(
-								payment_hash, &channel_manager.node_signer, &channel_manager.logger,
-								&channel_manager.inbound_payment_id_secret, true,
-							);
-						if claim_found.is_err() {
-							let mut claimable_payments = channel_manager.claimable_payments.lock().unwrap();
-							match claimable_payments.pending_claiming_payments.entry(payment_hash) {
-								hash_map::Entry::Occupied(_) => {
-									debug_assert!(false, "Entry was added in begin_claiming_payment");
-									return Err(DecodeError::InvalidValue);
-								},
-								hash_map::Entry::Vacant(entry) => {
-									entry.insert(payment_claim.claiming_payment);
-								},
+						let already_claiming = {
+							let payments = channel_manager.claimable_payments.lock().unwrap();
+							payments.pending_claiming_payments.contains_key(&payment_hash)
+						};
+						if !already_claiming {
+							let claim_found =
+								channel_manager.claimable_payments.lock().unwrap().begin_claiming_payment(
+									payment_hash, &channel_manager.node_signer, &channel_manager.logger,
+									&channel_manager.inbound_payment_id_secret, true,
+								);
+							if claim_found.is_err() {
+								let mut claimable_payments = channel_manager.claimable_payments.lock().unwrap();
+								match claimable_payments.pending_claiming_payments.entry(payment_hash) {
+									hash_map::Entry::Occupied(_) => {
+										debug_assert!(false, "Entry was added in begin_claiming_payment");
+										return Err(DecodeError::InvalidValue);
+									},
+									hash_map::Entry::Vacant(entry) => {
+										entry.insert(payment_claim.claiming_payment);
+									},
+								}
 							}
 						}
 

lightning/src/ln/reload_tests.rs

@@ -783,7 +783,7 @@ fn test_forwardable_regen() {
 	claim_payment(&nodes[0], &[&nodes[1], &nodes[2]], payment_preimage_2);
 }
 
-fn do_test_partial_claim_before_restart(persist_both_monitors: bool) {
+fn do_test_partial_claim_before_restart(persist_both_monitors: bool, double_restart: bool) {
 	// Test what happens if a node receives an MPP payment, claims it, but crashes before
 	// persisting the ChannelManager. If `persist_both_monitors` is false, also crash after only
 	// updating one of the two channels' ChannelMonitors. As a result, on startup, we'll (a) still
@@ -799,11 +799,11 @@ fn do_test_partial_claim_before_restart(persist_both_monitors: bool) {
 	// definitely claimed.
 	let chanmon_cfgs = create_chanmon_cfgs(4);
 	let node_cfgs = create_node_cfgs(4, &chanmon_cfgs);
-	let persister;
-	let new_chain_monitor;
+	let (persist_d_1, persist_d_2);
+	let (chain_d_1, chain_d_2);
 
 	let node_chanmgrs = create_node_chanmgrs(4, &node_cfgs, &[None, None, None, None]);
-	let nodes_3_deserialized;
+	let (node_d_1, node_d_2);
 
 	let mut nodes = create_network(4, &node_cfgs, &node_chanmgrs);
 
@@ -878,7 +878,14 @@ fn do_test_partial_claim_before_restart(persist_both_monitors: bool) {
 	}
 
 	// Now restart nodes[3].
-	reload_node!(nodes[3], original_manager, &[&updated_monitor.0, &original_monitor.0], persister, new_chain_monitor, nodes_3_deserialized);
+	reload_node!(nodes[3], original_manager.clone(), &[&updated_monitor.0, &original_monitor.0], persit_d_1, chain_d_1, node_d_1);
+
+	if double_restart {
+		// Previously, we had a bug where we'd fail to reload if we re-persist the `ChannelManager`
+		// without updating any `ChannelMonitor`s as we'd fail to double-initiate the claim replay.
+		// We test that here ensuring that we can reload again.
+		reload_node!(nodes[3], node_d_1.encode(), &[&updated_monitor.0, &original_monitor.0], persist_d_2, chain_d_2, node_d_2);
+	}
 
 	// Until the startup background events are processed (in `get_and_clear_pending_events`,
 	// below), the preimage is not copied to the non-persisted monitor...
@@ -973,8 +980,10 @@ fn do_test_partial_claim_before_restart(persist_both_monitors: bool) {
 
 #[test]
 fn test_partial_claim_before_restart() {
-	do_test_partial_claim_before_restart(false);
-	do_test_partial_claim_before_restart(true);
+	do_test_partial_claim_before_restart(false, false);
+	do_test_partial_claim_before_restart(false, true);
+	do_test_partial_claim_before_restart(true, false);
+	do_test_partial_claim_before_restart(true, true);
 }
 
 fn do_forwarded_payment_no_manager_persistence(use_cs_commitment: bool, claim_htlc: bool, use_intercept: bool) {