Merge pull request #1966 from ViktorTigerstrom/2023-01-store-channels-per-peer-followups

1507 followups

Author: TheBlueMatt

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -935,7 +935,7 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
 
 	// Note that the ordering of the events for different nodes is non-prescriptive, though the
 	// ordering of the two events that both go to nodes[2] have to stay in the same order.
-	let (nodes_0_event, events_3) = remove_first_msg_event_to_node(&nodes[0].node.get_our_node_id(), &events_3);
+	let nodes_0_event = remove_first_msg_event_to_node(&nodes[0].node.get_our_node_id(), &mut events_3);
 	let messages_a = match nodes_0_event {
 		MessageSendEvent::UpdateHTLCs { node_id, mut updates } => {
 			assert_eq!(node_id, nodes[0].node.get_our_node_id());
@@ -949,12 +949,12 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
 		_ => panic!("Unexpected event type!"),
 	};
 
-	let (nodes_2_event, events_3) = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &events_3);
+	let nodes_2_event = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &mut events_3);
 	let send_event_b = SendEvent::from_event(nodes_2_event);
 	assert_eq!(send_event_b.node_id, nodes[2].node.get_our_node_id());
 
 	let raa = if test_ignore_second_cs {
-		let (nodes_2_event, _events_3) = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &events_3);
+		let nodes_2_event = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &mut events_3);
 		match nodes_2_event {
 			MessageSendEvent::SendRevokeAndACK { node_id, msg } => {
 				assert_eq!(node_id, nodes[2].node.get_our_node_id());

lightning/src/ln/channelmanager.rs

@@ -572,12 +572,12 @@ macro_rules! get_htlc_update_msgs {
 }
 
 /// Fetches the first `msg_event` to the passed `node_id` in the passed `msg_events` vec.
-/// Returns the `msg_event`, along with an updated `msg_events` vec with the message removed.
+/// Returns the `msg_event`.
 ///
 /// Note that even though `BroadcastChannelAnnouncement` and `BroadcastChannelUpdate`
 /// `msg_events` are stored under specific peers, this function does not fetch such `msg_events` as
 /// such messages are intended to all peers.
-pub fn remove_first_msg_event_to_node(msg_node_id: &PublicKey, msg_events: &Vec<MessageSendEvent>) -> (MessageSendEvent, Vec<MessageSendEvent>) {
+pub fn remove_first_msg_event_to_node(msg_node_id: &PublicKey, msg_events: &mut Vec<MessageSendEvent>) -> MessageSendEvent {
 	let ev_index = msg_events.iter().position(|e| { match e {
 		MessageSendEvent::SendAcceptChannel { node_id, .. } => {
 			node_id == msg_node_id
@@ -644,9 +644,7 @@ pub fn remove_first_msg_event_to_node(msg_node_id: &PublicKey, msg_events: &Vec<
 		},
 	}});
 	if ev_index.is_some() {
-		let mut updated_msg_events = msg_events.to_vec();
-		let ev = updated_msg_events.remove(ev_index.unwrap());
-		(ev, updated_msg_events)
+		msg_events.remove(ev_index.unwrap())
 	} else {
 		panic!("Couldn't find any MessageSendEvent to the node!")
 	}
@@ -1486,9 +1484,9 @@ pub fn do_main_commitment_signed_dance(node_a: &Node<'_, '_, '_>, node_b: &Node<
 	check_added_monitors!(node_b, 1);
 	node_b.node.handle_commitment_signed(&node_a.node.get_our_node_id(), &as_commitment_signed);
 	let (bs_revoke_and_ack, extra_msg_option) = {
-		let events = node_b.node.get_and_clear_pending_msg_events();
+		let mut events = node_b.node.get_and_clear_pending_msg_events();
 		assert!(events.len() <= 2);
-		let (node_a_event, events) = remove_first_msg_event_to_node(&node_a.node.get_our_node_id(), &events);
+		let node_a_event = remove_first_msg_event_to_node(&node_a.node.get_our_node_id(), &mut events);
 		(match node_a_event {
 			MessageSendEvent::SendRevokeAndACK { ref node_id, ref msg } => {
 				assert_eq!(*node_id, node_a.node.get_our_node_id());
@@ -1943,8 +1941,7 @@ pub fn pass_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_rou
 	let mut events = origin_node.node.get_and_clear_pending_msg_events();
 	assert_eq!(events.len(), expected_route.len());
 	for (path_idx, expected_path) in expected_route.iter().enumerate() {
-		let (ev, updated_events) = remove_first_msg_event_to_node(&expected_path[0].node.get_our_node_id(), &events);
-		events = updated_events;
+		let ev = remove_first_msg_event_to_node(&expected_path[0].node.get_our_node_id(), &mut events);
 		// Once we've gotten through all the HTLCs, the last one should result in a
 		// PaymentClaimable (but each previous one should not!), .
 		let expect_payment = path_idx == expected_route.len() - 1;
@@ -2003,9 +2000,8 @@ pub fn do_claim_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>,
 	} else {
 		for expected_path in expected_paths.iter() {
 			// For MPP payments, we always want the message to the first node in the path.
-			let (ev, updated_events) = remove_first_msg_event_to_node(&expected_path[0].node.get_our_node_id(), &events);
+			let ev = remove_first_msg_event_to_node(&expected_path[0].node.get_our_node_id(), &mut events);
 			per_path_msgs.push(msgs_from_ev!(&ev));
-			events = updated_events;
 		}
 	}
 

lightning/src/ln/functional_test_utils.rs

@@ -2747,7 +2747,7 @@ fn test_htlc_on_chain_success() {
 		},
 		_ => panic!()
 	}
-	let events = nodes[1].node.get_and_clear_pending_msg_events();
+	let mut events = nodes[1].node.get_and_clear_pending_msg_events();
 	{
 		let mut added_monitors = nodes[1].chain_monitor.added_monitors.lock().unwrap();
 		assert_eq!(added_monitors.len(), 2);
@@ -2757,8 +2757,8 @@ fn test_htlc_on_chain_success() {
 	}
 	assert_eq!(events.len(), 3);
 
-	let (nodes_2_event, events) = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &events);
-	let (nodes_0_event, events) = remove_first_msg_event_to_node(&nodes[0].node.get_our_node_id(), &events);
+	let nodes_2_event = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &mut events);
+	let nodes_0_event = remove_first_msg_event_to_node(&nodes[0].node.get_our_node_id(), &mut events);
 
 	match nodes_2_event {
 		MessageSendEvent::HandleError { action: ErrorAction::SendErrorMessage { .. }, node_id: _ } => {},
@@ -3196,14 +3196,15 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use
 			_ => panic!("Unexpected event"),
 		}
 	}
+
 	nodes[1].node.process_pending_htlc_forwards();
 	check_added_monitors!(nodes[1], 1);
 
 	let mut events = nodes[1].node.get_and_clear_pending_msg_events();
 	assert_eq!(events.len(), if deliver_bs_raa { 4 } else { 3 });
 
-	let events = if deliver_bs_raa {
-		let (nodes_2_event, events) = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &events);
+	if deliver_bs_raa {
+		let nodes_2_event = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &mut events);
 		match nodes_2_event {
 			MessageSendEvent::UpdateHTLCs { ref node_id, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fail_htlcs, ref update_fulfill_htlcs, ref update_fail_malformed_htlcs, .. } } => {
 				assert_eq!(nodes[2].node.get_our_node_id(), *node_id);
@@ -3214,10 +3215,9 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use
 			},
 			_ => panic!("Unexpected event"),
 		}
-		events
-	} else { events };
+	}
 
-	let (nodes_2_event, events) = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &events);
+	let nodes_2_event = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &mut events);
 	match nodes_2_event {
 		MessageSendEvent::HandleError { action: ErrorAction::SendErrorMessage { msg: msgs::ErrorMessage { channel_id, ref data } }, node_id: _ } => {
 			assert_eq!(channel_id, chan_2.2);
@@ -3226,7 +3226,7 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use
 		_ => panic!("Unexpected event"),
 	}
 
-	let (nodes_0_event, events) = remove_first_msg_event_to_node(&nodes[0].node.get_our_node_id(), &events);
+	let nodes_0_event = remove_first_msg_event_to_node(&nodes[0].node.get_our_node_id(), &mut events);
 	match nodes_0_event {
 		MessageSendEvent::UpdateHTLCs { ref node_id, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fail_htlcs, ref update_fulfill_htlcs, ref update_fail_malformed_htlcs, ref commitment_signed, .. } } => {
 			assert!(update_add_htlcs.is_empty());
@@ -3839,9 +3839,10 @@ fn do_test_drop_messages_peer_disconnect(messages_delivered: u8, simulate_broken
 	if messages_delivered == 1 || messages_delivered == 2 {
 		expect_payment_path_successful!(nodes[0]);
 	}
-
-	nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
-	nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
+	if messages_delivered <= 5 {
+		nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
+		nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
+	}
 	reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
 
 	if messages_delivered > 2 {
@@ -4641,10 +4642,10 @@ fn test_onchain_to_onchain_claim() {
 		_ => panic!("Unexpected event"),
 	}
 	check_added_monitors!(nodes[1], 1);
-	let msg_events = nodes[1].node.get_and_clear_pending_msg_events();
+	let mut msg_events = nodes[1].node.get_and_clear_pending_msg_events();
 	assert_eq!(msg_events.len(), 3);
-	let (nodes_2_event, msg_events) = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &msg_events);
-	let (nodes_0_event, msg_events) = remove_first_msg_event_to_node(&nodes[0].node.get_our_node_id(), &msg_events);
+	let nodes_2_event = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &mut msg_events);
+	let nodes_0_event = remove_first_msg_event_to_node(&nodes[0].node.get_our_node_id(), &mut msg_events);
 
 	match nodes_2_event {
 		MessageSendEvent::HandleError { action: ErrorAction::SendErrorMessage { .. }, node_id: _ } => {},
@@ -9210,8 +9211,6 @@ fn test_keysend_payments_to_private_node() {
 
 	let payer_pubkey = nodes[0].node.get_our_node_id();
 	let payee_pubkey = nodes[1].node.get_our_node_id();
-	nodes[0].node.peer_connected(&payee_pubkey, &msgs::Init { features: nodes[1].node.init_features(), remote_network_address: None }).unwrap();
-	nodes[1].node.peer_connected(&payer_pubkey, &msgs::Init { features: nodes[0].node.init_features(), remote_network_address: None }).unwrap();
 
 	let _chan = create_chan_between_nodes(&nodes[0], &nodes[1]);
 	let route_params = RouteParameters {
@@ -9285,7 +9284,7 @@ fn test_double_partial_claim() {
 
 	let mut events = nodes[0].node.get_and_clear_pending_msg_events();
 	assert_eq!(events.len(), 2);
-	let (node_1_msgs, _events) = remove_first_msg_event_to_node(&nodes[1].node.get_our_node_id(), &events);
+	let node_1_msgs = remove_first_msg_event_to_node(&nodes[1].node.get_our_node_id(), &mut events);
 	pass_along_path(&nodes[0], &[&nodes[1], &nodes[3]], 15_000_000, payment_hash, Some(payment_secret), node_1_msgs, false, None);
 
 	// At this point nodes[3] has received one half of the payment, and the user goes to handle

lightning/src/ln/functional_tests.rs

@@ -153,11 +153,11 @@ fn mpp_retry() {
 	assert_eq!(events.len(), 2);
 
 	// Pass half of the payment along the success path.
-	let (success_path_msgs, mut events) = remove_first_msg_event_to_node(&nodes[1].node.get_our_node_id(), &events);
+	let success_path_msgs = remove_first_msg_event_to_node(&nodes[1].node.get_our_node_id(), &mut events);
 	pass_along_path(&nodes[0], &[&nodes[1], &nodes[3]], 2_000_000, payment_hash, Some(payment_secret), success_path_msgs, false, None);
 
 	// Add the HTLC along the first hop.
-	let (fail_path_msgs_1, _events) = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &events);
+	let fail_path_msgs_1 = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &mut events);
 	let (update_add, commitment_signed) = match fail_path_msgs_1 {
 		MessageSendEvent::UpdateHTLCs { node_id: _, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fulfill_htlcs, ref update_fail_htlcs, ref update_fail_malformed_htlcs, ref update_fee, ref commitment_signed } } => {
 			assert_eq!(update_add_htlcs.len(), 1);
@@ -237,7 +237,7 @@ fn do_mpp_receive_timeout(send_partial_mpp: bool) {
 	assert_eq!(events.len(), 2);
 
 	// Pass half of the payment along the first path.
-	let (node_1_msgs, mut events) = remove_first_msg_event_to_node(&nodes[1].node.get_our_node_id(), &events);
+	let node_1_msgs = remove_first_msg_event_to_node(&nodes[1].node.get_our_node_id(), &mut events);
 	pass_along_path(&nodes[0], &[&nodes[1], &nodes[3]], 200_000, payment_hash, Some(payment_secret), node_1_msgs, false, None);
 
 	if send_partial_mpp {
@@ -265,7 +265,7 @@ fn do_mpp_receive_timeout(send_partial_mpp: bool) {
 		expect_payment_failed_conditions(&nodes[0], payment_hash, false, PaymentFailedConditions::new().mpp_parts_remain().expected_htlc_error_data(23, &[][..]));
 	} else {
 		// Pass half of the payment along the second path.
-		let (node_2_msgs, _events) = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &events);
+		let node_2_msgs = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &mut events);
 		pass_along_path(&nodes[0], &[&nodes[2], &nodes[3]], 200_000, payment_hash, Some(payment_secret), node_2_msgs, true, None);
 
 		// Even after MPP_TIMEOUT_TICKS we should not timeout the MPP if we have all the parts

lightning/src/ln/payment_tests.rs

@@ -701,8 +701,8 @@ fn do_test_partial_claim_before_restart(persist_both_monitors: bool) {
 	// Send the payment through to nodes[3] *without* clearing the PaymentClaimable event
 	let mut send_events = nodes[0].node.get_and_clear_pending_msg_events();
 	assert_eq!(send_events.len(), 2);
-	let (node_1_msgs, mut send_events) = remove_first_msg_event_to_node(&nodes[1].node.get_our_node_id(), &send_events);
-	let (node_2_msgs, _send_events) = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &send_events);
+	let node_1_msgs = remove_first_msg_event_to_node(&nodes[1].node.get_our_node_id(), &mut send_events);
+	let node_2_msgs = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &mut send_events);
 	do_pass_along_path(&nodes[0], &[&nodes[1], &nodes[3]], 15_000_000, payment_hash, Some(payment_secret), node_1_msgs, true, false, None);
 	do_pass_along_path(&nodes[0], &[&nodes[2], &nodes[3]], 15_000_000, payment_hash, Some(payment_secret), node_2_msgs, true, false, None);
 

lightning/src/ln/reload_tests.rs

@@ -13,8 +13,8 @@ use crate::chain::channelmonitor::ANTI_REORG_DELAY;
 use crate::chain::transaction::OutPoint;
 use crate::chain::Confirm;
 use crate::ln::channelmanager::ChannelManager;
-use crate::ln::msgs::ChannelMessageHandler;
-use crate::util::events::{Event, MessageSendEvent, MessageSendEventsProvider, ClosureReason, HTLCDestination};
+use crate::ln::msgs::{ChannelMessageHandler, Init};
+use crate::util::events::{Event, MessageSendEventsProvider, ClosureReason, HTLCDestination};
 use crate::util::test_utils;
 use crate::util::ser::Writeable;
 
@@ -374,6 +374,12 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_
 	nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().clear();
 
 	// Now check that we can create a new channel
+	if reload_node && nodes[0].node.per_peer_state.read().unwrap().len() == 0 {
+		// If we dropped the channel before reloading the node, nodes[1] was also dropped from
+		// nodes[0] storage, and hence not connected again on startup. We therefore need to
+		// reconnect to the node before attempting to create a new channel.
+		nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &Init { features: nodes[1].node.init_features(), remote_network_address: None }).unwrap();
+	}
 	create_announced_chan_between_nodes(&nodes, 0, 1);
 	send_payment(&nodes[0], &[&nodes[1]], 8000000);
 }