Ensure monitors are not archived if they have a preimage we need

When a `ChannelMonitor` sees a payment preimage on chain for an
outbound HTLC, it creates a `MonitorEvent` containing the preimage
to pass to the inbound edge. The inclusion of the transaction
containing the payment preimage (plus six confirmations) also
results in the corresponding `Balance` being removed from the live
balance set, allowing the `ChannelMonitor` to be pruned (after a
further 4032 blocks).

While `MonitorEvent`s should always be processed in a timely
manner, if a node is suffering from a bug  where they are not, its
possible for 4038 blocks to pass with the preimage-containing
`MonitorEvent` still pending. If that happens, its possible the
`ChannelMonitor` is archived even though the preimage in it is
needed in another channel (or `ChannelMonitor`), causing funds
loss.

Luckily the fix is simple - check for pending events before
allowing a `ChannelMonitor` to be archived.

Fixes #2153

Author: TheBlueMatt

lightning/src/chain/channelmonitor.rs

@@ -1993,7 +1993,10 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 	}
 
 	/// Checks if the monitor is fully resolved. Resolved monitor is one that has claimed all of
-	/// its outputs and balances (i.e. [`Self::get_claimable_balances`] returns an empty set).
+	/// its outputs and balances (i.e. [`Self::get_claimable_balances`] returns an empty set) and
+	/// which does not have any payment preimages for HTLCs which are still pending on other
+	/// channels.
+	///
 	/// Additionally may update state to track when the balances set became empty.
 	///
 	/// This function returns a tuple of two booleans, the first indicating whether the monitor is
@@ -2012,33 +2015,41 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 			is_all_funds_claimed = false;
 		}
 
+		// As long as HTLCs remain unresolved, they'll be present as a `Balance`. After that point,
+		// if they contained a preimage, an event will appear in `pending_monitor_events` which,
+		// once processed, implies the preimage exists in the corresponding inbound channel.
+		let preimages_not_needed_elsewhere = inner.pending_monitor_events.is_empty();
+
 		const BLOCKS_THRESHOLD: u32 = 4032; // ~four weeks
-		match (inner.balances_empty_height, is_all_funds_claimed) {
-			(Some(balances_empty_height), true) => {
+		match (inner.balances_empty_height, is_all_funds_claimed, preimages_not_needed_elsewhere) {
+			(Some(balances_empty_height), true, true) => {
 				// Claimed all funds, check if reached the blocks threshold.
 				(current_height >= balances_empty_height + BLOCKS_THRESHOLD, false)
 			},
-			(Some(_), false) => {
-				// previously assumed we claimed all funds, but we have new funds to claim.
-				// Should not happen in practice.
-				debug_assert!(false, "Thought we were done claiming funds, but claimable_balances now has entries");
+			(Some(_), false, _)|(Some(_), _, false) => {
+				// previously assumed we claimed all funds, but we have new funds to claim or
+				// preimages are suddenly needed (because of a duplicate-hash HTLC).
+				// This should never happen as once the `Balance`s and preimages are clear, we
+				// should never create new ones.
+				debug_assert!(false,
+					"Thought we were done claiming funds, but claimable_balances now has entries");
 				log_error!(logger,
 					"WARNING: LDK thought it was done claiming all the available funds in the ChannelMonitor for channel {}, but later decided it had more to claim. This is potentially an important bug in LDK, please report it at https://github.com/lightningdevkit/rust-lightning/issues/new",
 					inner.get_funding_txo().0);
 				inner.balances_empty_height = None;
 				(false, true)
 			},
-			(None, true) => {
-				// Claimed all funds but `balances_empty_height` is None. It is set to the
-				// current block height.
+			(None, true, true) => {
+				// Claimed all funds and preimages can be deleted, but `balances_empty_height` is
+				// None. It is set to the current block height.
 				log_debug!(logger,
 					"ChannelMonitor funded at {} is now fully resolved. It will become archivable in {} blocks",
 					inner.get_funding_txo().0, BLOCKS_THRESHOLD);
 				inner.balances_empty_height = Some(current_height);
 				(false, true)
 			},
-			(None, false) => {
-				// Have funds to claim.
+			(None, false, _)|(None, _, false) => {
+				// Have funds to claim or our preimages are still needed.
 				(false, false)
 			},
 		}

lightning/src/ln/monitor_tests.rs

@@ -161,7 +161,7 @@ fn revoked_output_htlc_resolution_timing() {
 
 #[test]
 fn archive_fully_resolved_monitors() {
-	// Test we can archive fully resolved channel monitor.
+	// Test we archive fully resolved channel monitors at the right time.
 	let chanmon_cfgs = create_chanmon_cfgs(2);
 	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
 	let mut user_config = test_default_channel_config();
@@ -171,46 +171,130 @@ fn archive_fully_resolved_monitors() {
 	let (_, _, chan_id, funding_tx) =
 		create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 1_000_000);
 
-	nodes[0].node.close_channel(&chan_id, &nodes[1].node.get_our_node_id()).unwrap();
-	let node_0_shutdown = get_event_msg!(nodes[0], MessageSendEvent::SendShutdown, nodes[1].node.get_our_node_id());
-	nodes[1].node.handle_shutdown(nodes[0].node.get_our_node_id(), &node_0_shutdown);
-	let node_1_shutdown = get_event_msg!(nodes[1], MessageSendEvent::SendShutdown, nodes[0].node.get_our_node_id());
-	nodes[0].node.handle_shutdown(nodes[1].node.get_our_node_id(), &node_1_shutdown);
+	let (payment_preimage, payment_hash, ..) = route_payment(&nodes[0], &[&nodes[1]], 10_000_000);
 
-	let node_0_closing_signed = get_event_msg!(nodes[0], MessageSendEvent::SendClosingSigned, nodes[1].node.get_our_node_id());
-	nodes[1].node.handle_closing_signed(nodes[0].node.get_our_node_id(), &node_0_closing_signed);
-	let node_1_closing_signed = get_event_msg!(nodes[1], MessageSendEvent::SendClosingSigned, nodes[0].node.get_our_node_id());
-	nodes[0].node.handle_closing_signed(nodes[1].node.get_our_node_id(), &node_1_closing_signed);
-	let (_, node_0_2nd_closing_signed) = get_closing_signed_broadcast!(nodes[0].node, nodes[1].node.get_our_node_id());
-	nodes[1].node.handle_closing_signed(nodes[0].node.get_our_node_id(), &node_0_2nd_closing_signed.unwrap());
-	let (_, _) = get_closing_signed_broadcast!(nodes[1].node, nodes[0].node.get_our_node_id());
+	nodes[0].node.force_close_broadcasting_latest_txn(&chan_id, &nodes[1].node.get_our_node_id(), "".to_owned()).unwrap();
+	check_added_monitors!(nodes[0], 1);
+	check_closed_broadcast!(nodes[0], true);
+	check_closed_event!(nodes[0], 1, ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true) }, [nodes[1].node.get_our_node_id()], 1_000_000);
 
-	let shutdown_tx = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
+	let commitment_tx = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
+	assert_eq!(commitment_tx.len(), 1);
 
-	mine_transaction(&nodes[0], &shutdown_tx[0]);
-	mine_transaction(&nodes[1], &shutdown_tx[0]);
+	mine_transaction(&nodes[0], &commitment_tx[0]);
+	mine_transaction(&nodes[1], &commitment_tx[0]);
+	let reason = ClosureReason::CommitmentTxConfirmed;
+	check_closed_event!(nodes[1], 1, reason, [nodes[0].node.get_our_node_id()], 1_000_000);
+	check_closed_broadcast(&nodes[1], 1, true);
+	check_added_monitors(&nodes[1], 1);
 
-	connect_blocks(&nodes[0], 6);
+	connect_blocks(&nodes[0], BREAKDOWN_TIMEOUT as u32);
 	connect_blocks(&nodes[1], 6);
 
-	check_closed_event!(nodes[0], 1, ClosureReason::LocallyInitiatedCooperativeClosure, [nodes[1].node.get_our_node_id()], 1000000);
-	check_closed_event!(nodes[1], 1, ClosureReason::CounterpartyInitiatedCooperativeClosure, [nodes[0].node.get_our_node_id()], 1000000);
+	// After the commitment transaction reaches enough confirmations for nodes[0] to claim its
+	// balance, both nodes should still have a pending `Balance` as the HTLC has not yet resolved.
+	assert!(!nodes[0].chain_monitor.chain_monitor.get_claimable_balances(&[]).is_empty());
+	assert!(!nodes[1].chain_monitor.chain_monitor.get_claimable_balances(&[]).is_empty());
+
+	let spendable_event = nodes[0].chain_monitor.chain_monitor.get_and_clear_pending_events();
+	assert_eq!(spendable_event.len(), 1);
+	if let Event::SpendableOutputs { .. } = spendable_event[0] {} else { panic!(); }
 
+	// Until the `Balance` set of both monitors goes empty, calling
+	// `archive_fully_resolved_channel_monitors` will do nothing (though we don't bother to observe
+	// that except later by checking that the monitors are archived at the exact correct block
+	// height).
+	nodes[0].chain_monitor.chain_monitor.archive_fully_resolved_channel_monitors();
 	assert_eq!(nodes[0].chain_monitor.chain_monitor.list_monitors().len(), 1);
-	// First archive should set balances_empty_height to current block height
+	nodes[1].chain_monitor.chain_monitor.archive_fully_resolved_channel_monitors();
+	assert_eq!(nodes[1].chain_monitor.chain_monitor.list_monitors().len(), 1);
+
+	nodes[1].node.claim_funds(payment_preimage);
+	check_added_monitors(&nodes[1], 1);
+	expect_payment_claimed!(nodes[1], payment_hash, 10_000_000);
+	let htlc_claim_tx = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
+	assert_eq!(htlc_claim_tx.len(), 1);
+
+	mine_transaction(&nodes[0], &htlc_claim_tx[0]);
+	mine_transaction(&nodes[1], &htlc_claim_tx[0]);
+
+	// Both nodes should retain the pending `Balance` until the HTLC resolution transaction has six
+	// confirmations
+	assert!(!nodes[0].chain_monitor.chain_monitor.get_claimable_balances(&[]).is_empty());
+	assert!(!nodes[1].chain_monitor.chain_monitor.get_claimable_balances(&[]).is_empty());
+
+	// Until the `Balance` set of both monitors goes empty, calling
+	// `archive_fully_resolved_channel_monitors` will do nothing (though we don't bother to observe
+	// that except later by checking that the monitors are archived at the exact correct block
+	// height).
+	nodes[0].chain_monitor.chain_monitor.archive_fully_resolved_channel_monitors();
+	assert_eq!(nodes[0].chain_monitor.chain_monitor.list_monitors().len(), 1);
+	nodes[1].chain_monitor.chain_monitor.archive_fully_resolved_channel_monitors();
+	assert_eq!(nodes[1].chain_monitor.chain_monitor.list_monitors().len(), 1);
+
+	connect_blocks(&nodes[0], 5);
+	connect_blocks(&nodes[1], 5);
+
+	assert!(nodes[0].chain_monitor.chain_monitor.get_claimable_balances(&[]).is_empty());
+	assert!(nodes[1].chain_monitor.chain_monitor.get_claimable_balances(&[]).is_empty());
+
+	// At this point, both nodes have no more `Balance`s, but nodes[0]'s `ChannelMonitor` still
+	// hasn't had the `MonitorEvent` that contains the preimage claimed by the `ChannelManager`.
+	// Thus, calling `archive_fully_resolved_channel_monitors` and waiting 4032 blocks will not
+	// result in the `ChannelMonitor` being archived.
 	nodes[0].chain_monitor.chain_monitor.archive_fully_resolved_channel_monitors();
 	assert_eq!(nodes[0].chain_monitor.chain_monitor.list_monitors().len(), 1);
 	connect_blocks(&nodes[0], 4032);
-	// Second call after 4032 blocks, should archive the monitor
 	nodes[0].chain_monitor.chain_monitor.archive_fully_resolved_channel_monitors();
-	// Should have no monitors left
+	assert_eq!(nodes[0].chain_monitor.chain_monitor.list_monitors().len(), 1);
+
+	// ...however, nodes[1]'s `ChannelMonitor` is ready to be archived, and will be in exactly 4032
+	// blocks.
+	nodes[1].chain_monitor.chain_monitor.archive_fully_resolved_channel_monitors();
+	assert_eq!(nodes[1].chain_monitor.chain_monitor.list_monitors().len(), 1);
+	connect_blocks(&nodes[1], 4031);
+	nodes[1].chain_monitor.chain_monitor.archive_fully_resolved_channel_monitors();
+	assert_eq!(nodes[1].chain_monitor.chain_monitor.list_monitors().len(), 1);
+	connect_blocks(&nodes[1], 1);
+	nodes[1].chain_monitor.chain_monitor.archive_fully_resolved_channel_monitors();
+	assert_eq!(nodes[1].chain_monitor.chain_monitor.list_monitors().len(), 0);
+
+	// Finally, we process the pending `MonitorEvent` from nodes[0], allowing the `ChannelMonitor`
+	// to be archived 4032 blocks later.
+	expect_payment_sent(&nodes[0], payment_preimage, None, true, false);
+	nodes[0].chain_monitor.chain_monitor.archive_fully_resolved_channel_monitors();
+	assert_eq!(nodes[0].chain_monitor.chain_monitor.list_monitors().len(), 1);
+	connect_blocks(&nodes[0], 4031);
+	nodes[0].chain_monitor.chain_monitor.archive_fully_resolved_channel_monitors();
+	assert_eq!(nodes[0].chain_monitor.chain_monitor.list_monitors().len(), 1);
+	connect_blocks(&nodes[0], 1);
+	nodes[0].chain_monitor.chain_monitor.archive_fully_resolved_channel_monitors();
 	assert_eq!(nodes[0].chain_monitor.chain_monitor.list_monitors().len(), 0);
+
 	// Remove the corresponding outputs and transactions the chain source is
 	// watching. This is to make sure the `Drop` function assertions pass.
-	nodes.get_mut(0).unwrap().chain_source.remove_watched_txn_and_outputs(
-		OutPoint { txid: funding_tx.compute_txid(), index: 0 },
-		funding_tx.output[0].script_pubkey.clone()
-	);
+	for node in nodes {
+		node.chain_source.remove_watched_txn_and_outputs(
+			OutPoint { txid: funding_tx.compute_txid(), index: 0 },
+			funding_tx.output[0].script_pubkey.clone()
+		);
+		node.chain_source.remove_watched_txn_and_outputs(
+			OutPoint { txid: commitment_tx[0].compute_txid(), index: 0 },
+			commitment_tx[0].output[0].script_pubkey.clone()
+		);
+		node.chain_source.remove_watched_txn_and_outputs(
+			OutPoint { txid: commitment_tx[0].compute_txid(), index: 1 },
+			commitment_tx[0].output[1].script_pubkey.clone()
+		);
+		node.chain_source.remove_watched_txn_and_outputs(
+			OutPoint { txid: commitment_tx[0].compute_txid(), index: 2 },
+			commitment_tx[0].output[2].script_pubkey.clone()
+		);
+		node.chain_source.remove_watched_txn_and_outputs(
+			OutPoint { txid: htlc_claim_tx[0].compute_txid(), index: 0 },
+			htlc_claim_tx[0].output[0].script_pubkey.clone()
+		);
+	}
 }
 
 fn do_chanmon_claim_value_coop_close(anchors: bool) {