Correct source_node_id and channel_id in PaymentForwarded event

Author: atalw

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -1102,7 +1102,7 @@ fn test_monitor_update_fail_reestablish() {
 	assert!(updates.update_fee.is_none());
 	assert_eq!(updates.update_fulfill_htlcs.len(), 1);
 	nodes[1].node.handle_update_fulfill_htlc(&nodes[2].node.get_our_node_id(), &updates.update_fulfill_htlcs[0]);
-	expect_payment_forwarded!(nodes[1], chan_1.2, Some(1000), false);
+	expect_payment_forwarded!(nodes[1], nodes[0], chan_1.2, Some(1000), false);
 	check_added_monitors!(nodes[1], 1);
 	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
 	commitment_signed_dance!(nodes[1], nodes[2], updates.commitment_signed, false);
@@ -2087,7 +2087,7 @@ fn test_fail_htlc_on_broadcast_after_claim() {
 	nodes[1].node.handle_update_fulfill_htlc(&nodes[2].node.get_our_node_id(), &cs_updates.update_fulfill_htlcs[0]);
 	let bs_updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
 	check_added_monitors!(nodes[1], 1);
-	expect_payment_forwarded!(nodes[1], chan_id_1, Some(1000), false);
+	expect_payment_forwarded!(nodes[1], nodes[0], chan_id_1, Some(1000), false);
 
 	mine_transaction(&nodes[1], &bs_txn[0]);
 	check_closed_event!(nodes[1], 1, ClosureReason::CommitmentTxConfirmed);
@@ -2468,7 +2468,7 @@ fn do_test_reconnect_dup_htlc_claims(htlc_status: HTLCStatusAtDupClaim, second_f
 		assert_eq!(fulfill_msg, cs_updates.update_fulfill_htlcs[0]);
 	}
 	nodes[1].node.handle_update_fulfill_htlc(&nodes[2].node.get_our_node_id(), &fulfill_msg);
-	expect_payment_forwarded!(nodes[1], chan_id_1, Some(1000), false);
+	expect_payment_forwarded!(nodes[1], nodes[0], chan_id_1, Some(1000), false);
 	check_added_monitors!(nodes[1], 1);
 
 	let mut bs_updates = None;

lightning/src/ln/channelmanager.rs

@@ -4278,7 +4278,6 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 					// update to. Instead, we simply document in `PaymentForwarded` that this
 					// can happen.
 				}
-				mem::drop(channel_state_lock);
 				if let ClaimFundsFromHop::MonitorUpdateFail(pk, err, _) = res {
 					let result: Result<(), _> = Err(err);
 					let _ = handle_error!(self, result, pk);
@@ -4292,15 +4291,14 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 
 						let mut pending_events = self.pending_events.lock().unwrap();
 
-						let source_node_id = self.get_our_node_id();
-
 						let channel_id = prev_outpoint.to_channel_id(); 
-						let chan_ab_id = [54, 1, 27, 147, 208, 68, 174, 120, 153, 166, 147, 131, 196, 197, 137, 125, 20, 169, 146, 130, 47, 96, 90, 106, 226, 118, 1, 231, 93, 113, 103, 164];
-						let chan_bc_id = [62, 94, 18, 107, 70, 218, 72, 107, 73, 188, 156, 196, 17, 42, 11, 32, 206, 143, 17, 229, 106, 81, 201, 215, 188, 141, 176, 146, 55, 122, 100, 36];
-						// Given 3 nodes A, B, and C, if C is claiming the HTLC with B, channel_id is of A<-->B
-						// Check line 5076 of functional_tests.rs
-						assert_eq!(chan_ab_id, channel_id);
-						println!("prev_outpoint_channel_id: {:?}", channel_id);
+						let channel = match channel_state_lock.by_id.entry(channel_id) {
+							hash_map::Entry::Occupied(chan) => chan,
+							hash_map::Entry::Vacant(_) => {
+								panic!("Channel doesn't exist")
+							}
+						};
+						let source_node_id = channel.get().get_counterparty_node_id();
 
 						pending_events.push(events::Event::PaymentForwarded {
 							source_node_id,
@@ -4310,6 +4308,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 						});
 					}
 				}
+				mem::drop(channel_state_lock);
 			},
 		}
 	}

lightning/src/ln/functional_test_utils.rs

@@ -1328,13 +1328,13 @@ macro_rules! expect_payment_path_successful {
 }
 
 macro_rules! expect_payment_forwarded {
-	($node: expr, $channel_id: expr, $expected_fee: expr, $upstream_force_closed: expr) => {
+	($node: expr, $source_node: expr, $channel_id: expr, $expected_fee: expr, $upstream_force_closed: expr) => {
 		let events = $node.node.get_and_clear_pending_events();
 		assert_eq!(events.len(), 1);
 		match events[0] {
 			Event::PaymentForwarded { source_node_id, channel_id, fee_earned_msat, claim_from_onchain_tx } => {
-				assert_eq!(source_node_id, $node.node.get_our_node_id());
-				// assert_eq!(channel_id, $channel_id);
+				assert_eq!(source_node_id, $source_node.node.get_our_node_id());
+				assert_eq!(channel_id, $channel_id);
 				assert_eq!(fee_earned_msat, $expected_fee);
 				assert_eq!(claim_from_onchain_tx, $upstream_force_closed);
 			},
@@ -1575,12 +1575,11 @@ pub fn do_claim_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>,
 			}
 		}
 		macro_rules! mid_update_fulfill_dance {
-			($node: expr, $prev_node: expr, $next_channel_id: expr, $new_msgs: expr) => {
+			($node: expr, $prev_node: expr, $next_node: expr, $next_channel_id: expr, $new_msgs: expr) => {
 				{
-					// TODO channel_id is not correct. Causes tests to fail.
 					$node.node.handle_update_fulfill_htlc(&$prev_node.node.get_our_node_id(), &next_msgs.as_ref().unwrap().0);
 					let fee = $node.node.channel_state.lock().unwrap().by_id.get(&next_msgs.as_ref().unwrap().0.channel_id).unwrap().config.forwarding_fee_base_msat;
-					expect_payment_forwarded!($node, $next_channel_id, Some(fee as u64), false);
+					expect_payment_forwarded!($node, $next_node, $next_channel_id, Some(fee as u64), false);
 					expected_total_fee_msat += fee as u64;
 					check_added_monitors!($node, 1);
 					let new_next_msgs = if $new_msgs {
@@ -1599,23 +1598,23 @@ pub fn do_claim_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>,
 			}
 		}
 
-		// [1, 2, 3, 4, 5, 6]
-		// prev_node = 6
-		// len = 6
-		// [6, 5, 4, 3, 2, 1] (1 = 5) (need to get node 4 => idx = 1 => need to get index 3 => len-1-idx-1
-		// [6, 5, 4, 3, 2, 1] (2 = 4) (need to get node 3 => idx = 2 => need to get index 2 => len-1-idx-1
-		// [6, 5, 4, 3, 2, 1] (3 = 3) (need to get node 3 => idx = 3 => len-1-idx => need to index 1 => len-1-idx-1
 		let mut prev_node = expected_route.last().unwrap();
-		let mut prev_channel_id = match prev_node.node.list_usable_channels().iter().find(|&x| x.counterparty.node_id == expected_next_node) {
-			Some(channel) => channel.channel_id,
-			None => panic!("Coudn't find channel")
-		};
-
 		for (idx, node) in expected_route.iter().rev().enumerate().skip(1) {
 			assert_eq!(expected_next_node, node.node.get_our_node_id());
 			let update_next_msgs = !skip_last || idx != expected_route.len() - 1;
 			if next_msgs.is_some() {
-				mid_update_fulfill_dance!(node, prev_node, prev_channel_id, update_next_msgs);
+				// Since we are traversing in reverse, next_node is actually the previous node
+				let next_node: &Node;
+				if idx == expected_route.len() - 1 {
+					next_node = origin_node;
+				} else {
+					next_node = expected_route[idx - 1];
+				}
+				let next_channel_id = match node.node.list_usable_channels().iter().find(|&x| x.counterparty.node_id == next_node.node.get_our_node_id()) {
+					Some(channel) => channel.channel_id,
+					None => panic!("Uh oh! Coudn't find the channel")
+				};
+				mid_update_fulfill_dance!(node, prev_node, next_node, next_channel_id, update_next_msgs);
 			} else {
 				assert!(!update_next_msgs);
 				assert!(node.node.get_and_clear_pending_msg_events().is_empty());
@@ -1624,10 +1623,6 @@ pub fn do_claim_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>,
 				assert_eq!(expected_next_node, origin_node.node.get_our_node_id());
 			}
 
-			prev_channel_id = match node.node.list_usable_channels().iter().find(|&x| x.counterparty.node_id == prev_node.node.get_our_node_id()) {
-				Some(channel) => channel.channel_id,
-				None => panic!("Couldn't find channel")
-			};
 			prev_node = node;
 		}
 

lightning/src/ln/functional_tests.rs

@@ -2684,8 +2684,8 @@ fn test_htlc_on_chain_success() {
 		Event::ChannelClosed { reason: ClosureReason::CommitmentTxConfirmed, .. } => {}
 		_ => panic!("Unexpected event"),
 	}
-	let node_id = nodes[1].node.get_our_node_id();
-	let chan_id = chan_2.2;
+	let node_id = nodes[0].node.get_our_node_id();
+	let chan_id = chan_1.2;
 	match forwarded_events[1] {
 		Event::PaymentForwarded { source_node_id, channel_id, fee_earned_msat, claim_from_onchain_tx } => {
 			assert_eq!(source_node_id, node_id);
@@ -5125,7 +5125,7 @@ fn test_onchain_to_onchain_claim() {
 	}
 	match events[1] {
 		Event::PaymentForwarded { source_node_id, channel_id, fee_earned_msat, claim_from_onchain_tx } => {
-			assert_eq!(source_node_id, nodes[1].node.get_our_node_id());
+			assert_eq!(source_node_id, nodes[0].node.get_our_node_id());
 			assert_eq!(channel_id, chan_1.2);
 			assert_eq!(fee_earned_msat, Some(1000));
 			assert_eq!(claim_from_onchain_tx, true);
@@ -5293,7 +5293,7 @@ fn test_duplicate_payment_hash_one_failure_one_success() {
 	// Note that the fee paid is effectively double as the HTLC value (including the nodes[1] fee
 	// and nodes[2] fee) is rounded down and then claimed in full.
 	mine_transaction(&nodes[1], &htlc_success_txn[0]);
-	expect_payment_forwarded!(nodes[1], chan_1.2 ,Some(196*2), true);
+	expect_payment_forwarded!(nodes[1], nodes[0], chan_1.2 ,Some(196*2), true);
 	let updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
 	assert!(updates.update_add_htlcs.is_empty());
 	assert!(updates.update_fail_htlcs.is_empty());
@@ -8871,7 +8871,7 @@ fn do_test_onchain_htlc_settlement_after_close(broadcast_alice: bool, go_onchain
 	assert_eq!(carol_updates.update_fulfill_htlcs.len(), 1);
 
 	nodes[1].node.handle_update_fulfill_htlc(&nodes[2].node.get_our_node_id(), &carol_updates.update_fulfill_htlcs[0]);
-	expect_payment_forwarded!(nodes[1], chan_ab.2, if go_onchain_before_fulfill || force_closing_node == 1 { None } else { Some(1000) }, false);
+	expect_payment_forwarded!(nodes[1], nodes[0], chan_ab.2, if go_onchain_before_fulfill || force_closing_node == 1 { None } else { Some(1000) }, false);
 	// If Alice broadcasted but Bob doesn't know yet, here he prepares to tell her about the preimage.
 	if !go_onchain_before_fulfill && broadcast_alice {
 		let events = nodes[1].node.get_and_clear_pending_msg_events();
@@ -8979,9 +8979,9 @@ fn do_test_onchain_htlc_settlement_after_close(broadcast_alice: bool, go_onchain
 
 #[test]
 fn test_onchain_htlc_settlement_after_close() {
-	// do_test_onchain_htlc_settlement_after_close(true, true);
-	// do_test_onchain_htlc_settlement_after_close(false, true); // Technically redundant, but may as well
-	// do_test_onchain_htlc_settlement_after_close(true, false);
+	do_test_onchain_htlc_settlement_after_close(true, true);
+	do_test_onchain_htlc_settlement_after_close(false, true); // Technically redundant, but may as well
+	do_test_onchain_htlc_settlement_after_close(true, false);
 	do_test_onchain_htlc_settlement_after_close(false, false);
 }
 

lightning/src/ln/payment_tests.rs

@@ -495,7 +495,7 @@ fn do_retry_with_no_persist(confirm_before_reload: bool) {
 	let bs_htlc_claim_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
 	assert_eq!(bs_htlc_claim_txn.len(), 1);
 	check_spends!(bs_htlc_claim_txn[0], as_commitment_tx);
-	expect_payment_forwarded!(nodes[1], chan_id, None, false);
+	expect_payment_forwarded!(nodes[1], nodes[0], chan_id, None, false);
 
 	if !confirm_before_reload {
 		mine_transaction(&nodes[0], &as_commitment_tx);

lightning/src/ln/reorg_tests.rs

@@ -138,7 +138,7 @@ fn do_test_onchain_htlc_reorg(local_commitment: bool, claim: bool) {
 		// ChannelManager only polls chain::Watch::release_pending_monitor_events when we
 		// probe it for events, so we probe non-message events here (which should just be the
 		// PaymentForwarded event).
-		expect_payment_forwarded!(nodes[1], chan_1.2, Some(1000), true);
+		expect_payment_forwarded!(nodes[1], nodes[0], chan_1.2, Some(1000), true);
 	} else {
 		// Confirm the timeout tx and check that we fail the HTLC backwards
 		let block = Block {

lightning/src/ln/shutdown_tests.rs

@@ -110,7 +110,7 @@ fn updates_shutdown_wait() {
 	assert!(updates.update_fee.is_none());
 	assert_eq!(updates.update_fulfill_htlcs.len(), 1);
 	nodes[1].node.handle_update_fulfill_htlc(&nodes[2].node.get_our_node_id(), &updates.update_fulfill_htlcs[0]);
-	expect_payment_forwarded!(nodes[1], chan_1.2, Some(1000), false);
+	expect_payment_forwarded!(nodes[1], nodes[0], chan_1.2, Some(1000), false);
 	check_added_monitors!(nodes[1], 1);
 	let updates_2 = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
 	commitment_signed_dance!(nodes[1], nodes[2], updates.commitment_signed, false);
@@ -279,7 +279,7 @@ fn do_test_shutdown_rebroadcast(recv_count: u8) {
 	assert!(updates.update_fee.is_none());
 	assert_eq!(updates.update_fulfill_htlcs.len(), 1);
 	nodes[1].node.handle_update_fulfill_htlc(&nodes[2].node.get_our_node_id(), &updates.update_fulfill_htlcs[0]);
-	expect_payment_forwarded!(nodes[1], chan_1.2, Some(1000), false);
+	expect_payment_forwarded!(nodes[1], nodes[0], chan_1.2, Some(1000), false);
 	check_added_monitors!(nodes[1], 1);
 	let updates_2 = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
 	commitment_signed_dance!(nodes[1], nodes[2], updates.commitment_signed, false);