Fix missing user_channel_id in PaymentForwarded

Previously, if a forwarding node reloaded mid-HTLC-forward with a preimage in
the outbound edge monitor and the outbound edge channel still open, and
subsequently reclaimed the inbound HTLC backwards, the PaymetForwarded event
would be missing the next_user_channel_id field.

Author: valentinewallace

lightning/src/ln/channelmanager.rs

@@ -18719,14 +18719,16 @@ impl<
 				}
 			}
 			for (channel_id, monitor) in args.channel_monitors.iter() {
-				let mut is_channel_closed = true;
+				let (mut is_channel_closed, mut user_channel_id_opt) = (true, None);
 				let counterparty_node_id = monitor.get_counterparty_node_id();
 				if let Some(peer_state_mtx) = per_peer_state.get(&counterparty_node_id) {
 					let mut peer_state_lock = peer_state_mtx.lock().unwrap();
 					let peer_state = &mut *peer_state_lock;
-					is_channel_closed = !peer_state.channel_by_id.contains_key(channel_id);
-					if reconstruct_manager_from_monitors && !is_channel_closed {
-						if let Some(chan) = peer_state.channel_by_id.get(channel_id) {
+					if let Some(chan) = peer_state.channel_by_id.get(channel_id) {
+						is_channel_closed = false;
+						user_channel_id_opt = Some(chan.context().get_user_id());
+
+						if reconstruct_manager_from_monitors {
 							if let Some(funded_chan) = chan.as_funded() {
 								for (payment_hash, prev_hop) in funded_chan.outbound_htlc_forwards()
 								{
@@ -19026,7 +19028,7 @@ impl<
 
 								Some((htlc_source, payment_preimage, htlc.amount_msat,
 									is_channel_closed, monitor.get_counterparty_node_id(),
-									monitor.get_funding_txo(), monitor.channel_id(), None))
+									monitor.get_funding_txo(), monitor.channel_id(), user_channel_id_opt))
 							} else { None }
 						} else {
 							// If it was an outbound payment, we've handled it above - if a preimage

lightning/src/ln/reload_tests.rs

@@ -1883,7 +1883,22 @@ fn test_reload_node_with_preimage_in_monitor_claims_htlc() {
 	//
 	// Here we ensure that inbound HTLCs in case (b) above will not be failed backwards on manager
 	// reload.
+	do_test_reclaim_after_reload(true);
+}
+
+#[test]
+fn test_reclaim_after_reload_payment_forwarded_ev() {
+	// Test that when a forwarding node reloads:
+	// - mid-HTLC-forward
+	// - with a preimage in the outbound edge monitor
+	// - and the outbound edge channel is still open
+	// - and we subsequently re-claim the inbound HTLC backwards
+	// the `PaymentForwarded` event contains `next_user_channel_id`. Previously,
+	// `next_user_channel_id` would always be None even if the outbound edge channel was still open.
+	do_test_reclaim_after_reload(false);
+}
 
+fn do_test_reclaim_after_reload(outbound_htlc_fully_removed: bool) {
 	let chanmon_cfgs = create_chanmon_cfgs(3);
 	let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
 	let persister;
@@ -1924,29 +1939,22 @@ fn test_reload_node_with_preimage_in_monitor_claims_htlc() {
 	let updates_2_1 = get_htlc_update_msgs(&nodes[2], &node_1_id);
 	nodes[1].node.handle_update_fulfill_htlc(node_2_id, updates_2_1.update_fulfill_htlcs[0].clone());
 	check_added_monitors(&nodes[1], 1);
-	do_commitment_signed_dance(&nodes[1], &nodes[2], &updates_2_1.commitment_signed, false, false);
-	expect_payment_forwarded!(nodes[1], nodes[0], nodes[2], Some(1000), false, false);
 
-	// Clear the holding cell's claim entry on chan_0_1 before serialization.
-	// This simulates a crash where the HTLC was fully removed from the outbound edge but is still
-	// present on the inbound edge without a resolution.
-	nodes[1].node.test_clear_channel_holding_cell(node_0_id, chan_id_0_1);
+	if outbound_htlc_fully_removed {
+		// Complete the commitment dance so the HTLC is fully resolved on the outbound edge.
+		do_commitment_signed_dance(&nodes[1], &nodes[2], &updates_2_1.commitment_signed, false, false);
+		expect_payment_forwarded!(nodes[1], nodes[0], nodes[2], Some(1000), false, false);
+
+		// Clear the holding cell's claim entry on chan_0_1 before serialization.
+		// This simulates a crash where the HTLC was fully removed from the outbound edge but is still
+		// present on the inbound edge without a resolution.
+		nodes[1].node.test_clear_channel_holding_cell(node_0_id, chan_id_0_1);
+	}
 
-	// At this point:
-	// - The inbound HTLC on nodes[1] (from nodes[0]) is in ::Forwarded state
-	// - The preimage IS in nodes[1]'s monitor for chan_1_2
-	// - The outbound HTLC to nodes[2] is resolved
-	//
-	// Serialize nodes[1] state and monitors before reloading.
 	let node_1_serialized = nodes[1].node.encode();
 	let mon_0_1_serialized = get_monitor!(nodes[1], chan_id_0_1).encode();
 	let mon_1_2_serialized = get_monitor!(nodes[1], chan_id_1_2).encode();
 
-	// Reload nodes[1].
-	// During deserialization, we track inbound HTLCs that purport to already be forwarded on the
-	// outbound edge. If any are entirely missing from the outbound edge with no preimage available,
-	// they will be failed backwards. Otherwise, as in this case where a preimage is available, the
-	// payment should be claimed backwards.
 	reload_node!(
 		nodes[1],
 		node_1_serialized,
@@ -1958,9 +1966,11 @@ fn test_reload_node_with_preimage_in_monitor_claims_htlc() {
 	);
 
 	// When the claim is reconstructed during reload, a PaymentForwarded event is generated.
-	// Fetching events triggers the pending monitor update (adding preimage) to be applied.
 	expect_payment_forwarded!(nodes[1], nodes[0], nodes[2], Some(1000), false, false);
-	check_added_monitors(&nodes[1], 1);
+	if outbound_htlc_fully_removed {
+		// Fetching events triggers the pending monitor update (adding preimage) to be applied.
+		check_added_monitors(&nodes[1], 1);
+	}
 
 	// Reconnect nodes[1] to nodes[0]. The claim should be in nodes[1]'s holding cell.
 	let mut reconnect_args = ReconnectArgs::new(&nodes[1], &nodes[0]);