MPP claim HTLC fulfills stuck in holding cell after node restart

[joostjager]

When a node claims an MPP payment across multiple channels and restarts before all preimage monitor updates have been persisted, the HTLC fulfills get permanently stuck in the holding cell. The channels remain operational but with reduced capacity, as the stuck HTLCs are counted as in-flight.

Root cause

Two persistence gaps in channelmanager.rs:

1. RAAMonitorUpdateBlockingAction::ClaimedMPPPayment (line 1685) is explicitly excluded from serialization (unread_variants at line 1699). The comment says it is "inferred from ChannelMonitor state", but no such inference happens on reload.

2. MonitorUpdateCompletionAction::PaymentClaimed.pending_mpp_claim (line 1464) is serialized as None (static_value, None at line 1500). After reload, the PaymentClaimed action fires at line 10204 but skips the MPP coordination block at line 10216 because pending_mpp_claim is None.

After restart, channels whose preimage monitor updates had already been persisted lose the state needed to coordinate the MPP claim release. Their holding cell HTLC fulfills are never freed.

How to trigger

Any node that claims an MPP payment across multiple channels and restarts before all preimage monitor updates are persisted is vulnerable. With any async KVStore implementation where each channel's monitor is a separate key, writes completing at different times is expected behavior. A crash between any two of those writes produces the required asymmetric state.

Also reproducible via chanmon_consistency fuzz target with input 0270801109191109191f1f10b6ff.

Unit test

The following test (in reload_tests.rs) reproduces the bug. Two nodes with two channels, MPP payment, fee bump to put channels in AWAITING_REMOTE_REVOKE, claim with InProgress preimage updates, asymmetric monitor snapshot, reload. After full settlement, both channels still have stuck pending HTLCs.

#[test]
fn test_mpp_claim_htlc_stuck_after_reload() {
	let chanmon_cfgs = create_chanmon_cfgs(2);
	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
	let persister;
	let new_chain_monitor;
	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
	let nodes_1_deserialized;
	let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);

	let chan_a = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100_000, 0);
	let chan_b = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100_000, 0);
	let chan_id_a = chan_a.2;
	let chan_id_b = chan_b.2;

	macro_rules! clear_monitors {
		($node:expr) => {
			$node.chain_monitor.added_monitors.lock().unwrap().clear();
		};
	}

	// Send an MPP payment that splits across both channels.
	let amt_msat = 15_000_000;
	let (route, payment_hash, payment_preimage, payment_secret) =
		get_route_and_payment_hash!(nodes[0], nodes[1], amt_msat);
	assert_eq!(route.paths.len(), 2, "Expected 2-way MPP split");
	send_along_route_with_secret(
		&nodes[0], route, &[&[&nodes[1]], &[&nodes[1]]],
		amt_msat, payment_hash, payment_secret,
	);

	// Fee bump to put channels in AWAITING_REMOTE_REVOKE. This ensures that when
	// claim_funds is called, the fulfills go to the holding cell instead of being
	// sent immediately.
	{
		let mut fee_est = chanmon_cfgs[0].fee_estimator.sat_per_kw.lock().unwrap();
		*fee_est *= 2;
	}
	nodes[0].node.timer_tick_occurred();
	check_added_monitors(&nodes[0], 2);

	let id_0 = nodes[0].node.get_our_node_id();
	let id_1 = nodes[1].node.get_our_node_id();

	// Deliver fee updates to node 1.
	let fee_msgs = nodes[0].node.get_and_clear_pending_msg_events();
	for ev in &fee_msgs {
		if let MessageSendEvent::UpdateHTLCs { updates, .. } = ev {
			nodes[1].node.handle_update_fee(id_0, updates.update_fee.as_ref().unwrap());
			nodes[1].node.handle_commitment_signed_batch_test(id_0, &updates.commitment_signed);
			clear_monitors!(nodes[1]);
		}
	}

	// Deliver node 1's revoke_and_ack but hold back the commitment_signed.
	let node1_msgs = nodes[1].node.get_and_clear_pending_msg_events();
	let mut cs_msgs = Vec::new();
	for ev in &node1_msgs {
		match ev {
			MessageSendEvent::SendRevokeAndACK { msg, .. } => {
				nodes[0].node.handle_revoke_and_ack(id_1, msg);
				clear_monitors!(nodes[0]);
			},
			MessageSendEvent::UpdateHTLCs { updates, .. } => {
				cs_msgs.push(updates.commitment_signed.clone());
			},
			_ => {},
		}
	}

	// Deliver node 0's revoke_and_ack to node 1.
	let node0_msgs = nodes[0].node.get_and_clear_pending_msg_events();
	for ev in &node0_msgs {
		if let MessageSendEvent::SendRevokeAndACK { msg, .. } = ev {
			nodes[1].node.handle_revoke_and_ack(id_0, msg);
			clear_monitors!(nodes[1]);
		}
	}

	// Snapshot channel b's monitor BEFORE the claim. ChainMonitor applies updates
	// to its internal copy immediately regardless of InProgress, so snapshotting
	// after the claim would give both monitors the same state. This simulates a
	// crash where channel a's monitor write completed but channel b's didn't.
	let mon_b_serialized = get_monitor!(nodes[1], chan_id_b).encode();

	// Claim with InProgress preimage updates.
	chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress);
	chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress);
	nodes[1].node.claim_funds(payment_preimage);
	check_added_monitors(&nodes[1], 2);

	// Complete preimage update for channel a only.
	let (update_id_a, _) = get_latest_mon_update_id(&nodes[1], chan_id_a);
	nodes[1].chain_monitor.chain_monitor.force_channel_monitor_updated(chan_id_a, update_id_a);

	// Deliver the held commitment_signed messages from the fee update.
	for cs in &cs_msgs {
		nodes[1].node.handle_commitment_signed_batch_test(id_0, cs);
		clear_monitors!(nodes[1]);
	}

	// Serialize and reload.
	let node_1_serialized = nodes[1].node.encode();
	let mon_a_serialized = get_monitor!(nodes[1], chan_id_a).encode();

	nodes[0].node.peer_disconnected(id_1);
	reload_node!(
		nodes[1], node_1_serialized,
		&[&mon_a_serialized, &mon_b_serialized],
		persister, new_chain_monitor, nodes_1_deserialized
	);

	// Reconnect.
	let id_1 = nodes[1].node.get_our_node_id();
	nodes[0].node.peer_connected(id_1,
		&msgs::Init { features: nodes[1].node.init_features(), networks: None,
			remote_network_address: None }, true).unwrap();
	nodes[1].node.peer_connected(id_0,
		&msgs::Init { features: nodes[0].node.init_features(), networks: None,
			remote_network_address: None }, false).unwrap();

	let reestablish_0 = nodes[0].node.get_and_clear_pending_msg_events();
	let reestablish_1 = nodes[1].node.get_and_clear_pending_msg_events();
	for ev in &reestablish_1 {
		if let MessageSendEvent::SendChannelReestablish { msg, .. } = ev {
			nodes[0].node.handle_channel_reestablish(id_1, msg);
		}
	}
	for ev in &reestablish_0 {
		if let MessageSendEvent::SendChannelReestablish { msg, .. } = ev {
			nodes[1].node.handle_channel_reestablish(id_0, msg);
		}
	}
	clear_monitors!(nodes[0]);
	clear_monitors!(nodes[1]);

	// Exchange all pending messages until convergence.
	for _ in 0..20 {
		let events_0 = nodes[0].node.get_and_clear_pending_msg_events();
		let events_1 = nodes[1].node.get_and_clear_pending_msg_events();
		if events_0.is_empty() && events_1.is_empty() { break; }
		for ev in &events_0 {
			match ev {
				MessageSendEvent::UpdateHTLCs { updates, .. } => {
					for u in &updates.update_fulfill_htlcs {
						nodes[1].node.handle_update_fulfill_htlc(id_0, u.clone());
					}
					if let Some(ref fee) = updates.update_fee {
						nodes[1].node.handle_update_fee(id_0, fee);
					}
					nodes[1].node.handle_commitment_signed_batch_test(
						id_0, &updates.commitment_signed);
					clear_monitors!(nodes[1]);
				},
				MessageSendEvent::SendRevokeAndACK { msg, .. } => {
					nodes[1].node.handle_revoke_and_ack(id_0, msg);
					clear_monitors!(nodes[1]);
				},
				_ => {},
			}
		}
		for ev in &events_1 {
			match ev {
				MessageSendEvent::UpdateHTLCs { updates, .. } => {
					for u in &updates.update_fulfill_htlcs {
						nodes[0].node.handle_update_fulfill_htlc(id_1, u.clone());
					}
					nodes[0].node.handle_commitment_signed_batch_test(
						id_1, &updates.commitment_signed);
					clear_monitors!(nodes[0]);
				},
				MessageSendEvent::SendRevokeAndACK { msg, .. } => {
					nodes[0].node.handle_revoke_and_ack(id_1, msg);
					clear_monitors!(nodes[0]);
				},
				_ => {},
			}
		}
		nodes[0].node.get_and_clear_pending_events();
		nodes[1].node.get_and_clear_pending_events();
		clear_monitors!(nodes[0]);
		clear_monitors!(nodes[1]);
	}

	// Both channels still have pending outbound HTLCs. The fulfills are stuck
	// in node 1's holding cell because ClaimedMPPPayment state was lost on reload.
	// When fixed: assert!(pending.is_empty());
	let pending: Vec<_> = nodes[0].node.list_channels().iter()
		.filter(|c| c.channel_id == chan_id_a || c.channel_id == chan_id_b)
		.filter(|c| !c.pending_outbound_htlcs.is_empty())
		.map(|c| c.channel_id)
		.collect();
	assert_eq!(pending.len(), 2,
		"Expected 2 channels with stuck pending HTLCs. If 0, the bug is fixed!");
}

[hoepeyemi]

Is this still open for contribution @joostjager

[joostjager]

Fix in progress in #4520