Post-#3897 fixes and cleanups

fuzz/src/full_stack.rs

@@ -1182,7 +1182,7 @@ fn two_peer_forwarding_seed() -> Vec<u8> {
 
 	// broadcast funding transaction
 	ext_from_hex("0b", &mut test);
-	// by now client should have sent a channel_ready (CHECK 4: SendChannelReady to 03020000 for chan 3f000000)
+	// by now client should have sent a channel_ready (CHECK 4: SendChannelReady to 03020000 for chan 2f000000)
 
 	// inbound read from peer id 1 of len 18
 	ext_from_hex("030112", &mut test);
@@ -1441,7 +1441,7 @@ fn two_peer_forwarding_seed() -> Vec<u8> {
 	// inbound read from peer id 0 of len 193
 	ext_from_hex("0300c1", &mut test);
 	// end of update_add_htlc from 0 to 1 via client and mac
-	ext_from_hex("ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff 5300000000000000000000000000000000000000000000000000000000000000 03000000000000000000000000000000", &mut test);
+	ext_from_hex("ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff 5200000000000000000000000000000000000000000000000000000000000000 03000000000000000000000000000000", &mut test);
 
 	// inbound read from peer id 0 of len 18
 	ext_from_hex("030012", &mut test);

lightning/src/ln/channel.rs

@@ -9608,7 +9608,9 @@ where
 	}
 
 	pub fn get_revoked_counterparty_commitment_transaction_number(&self) -> u64 {
-		self.context.cur_counterparty_commitment_transaction_number + 2
+		let ret = self.context.cur_counterparty_commitment_transaction_number + 2;
+		debug_assert_eq!(self.context.commitment_secrets.get_min_seen_secret(), ret);
+		ret
 	}
 
 	#[cfg(any(test, feature = "_externalize_tests"))]

lightning/src/ln/channelmanager.rs

@@ -17542,134 +17542,6 @@ mod tests {
 		}
 	}
 
-	#[test]
-	#[cfg(peer_storage)]
-	fn test_peer_storage() {
-		let chanmon_cfgs = create_chanmon_cfgs(2);
-		let (persister, chain_monitor);
-		let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
-		let nodes_0_deserialized;
-		let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
-		let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
-
-		let (_, _, cid, _) = create_announced_chan_between_nodes(&nodes, 0, 1);
-		send_payment(&nodes[0], &[&nodes[1]], 1000);
-		let nodes_0_serialized = nodes[0].node.encode();
-		let old_state_monitor = get_monitor!(nodes[0], cid).encode();
-		send_payment(&nodes[0], &[&nodes[1]], 10000);
-		send_payment(&nodes[0], &[&nodes[1]], 9999);
-
-		// Update peer storage with latest commitment txns
-		connect_blocks(&nodes[0], 1);
-		connect_blocks(&nodes[0], 1);
-
-		let peer_storage_msg_events_node0 =
-			nodes[0].chain_monitor.chain_monitor.get_and_clear_pending_msg_events();
-		let peer_storage_msg_events_node1 =
-			nodes[1].chain_monitor.chain_monitor.get_and_clear_pending_msg_events();
-		assert_ne!(peer_storage_msg_events_node0.len(), 0);
-		assert_ne!(peer_storage_msg_events_node1.len(), 0);
-
-		for ps_msg in peer_storage_msg_events_node0 {
-			match ps_msg {
-				MessageSendEvent::SendPeerStorage { ref node_id, ref msg } => {
-					assert_eq!(*node_id, nodes[1].node.get_our_node_id());
-					nodes[1].node.handle_peer_storage(nodes[0].node.get_our_node_id(), msg.clone());
-				},
-				_ => panic!("Unexpected event"),
-			}
-		}
-
-		for ps_msg in peer_storage_msg_events_node1 {
-			match ps_msg {
-				MessageSendEvent::SendPeerStorage { ref node_id, ref msg } => {
-					assert_eq!(*node_id, nodes[0].node.get_our_node_id());
-					nodes[0].node.handle_peer_storage(nodes[1].node.get_our_node_id(), msg.clone());
-				},
-				_ => panic!("Unexpected event"),
-			}
-		}
-
-		nodes[0].node.peer_disconnected(nodes[1].node.get_our_node_id());
-		nodes[1].node.peer_disconnected(nodes[0].node.get_our_node_id());
-
-		// Reload Node!
-		// TODO: Handle the case where we've completely forgotten about an active channel.
-		reload_node!(
-			nodes[0],
-			test_default_channel_config(),
-			&nodes_0_serialized,
-			&[&old_state_monitor[..]],
-			persister,
-			chain_monitor,
-			nodes_0_deserialized
-		);
-
-		nodes[0]
-			.node
-			.peer_connected(
-				nodes[1].node.get_our_node_id(),
-				&msgs::Init {
-					features: nodes[1].node.init_features(),
-					networks: None,
-					remote_network_address: None,
-				},
-				true,
-			)
-			.unwrap();
-
-		nodes[1]
-			.node
-			.peer_connected(
-				nodes[0].node.get_our_node_id(),
-				&msgs::Init {
-					features: nodes[0].node.init_features(),
-					networks: None,
-					remote_network_address: None,
-				},
-				false,
-			)
-			.unwrap();
-
-		let node_1_events = nodes[1].node.get_and_clear_pending_msg_events();
-		assert_eq!(node_1_events.len(), 2);
-
-		// Since, node-0 does not have any memory it would not send any message.
-		let node_0_events = nodes[0].node.get_and_clear_pending_msg_events();
-		assert_eq!(node_0_events.len(), 1);
-
-		match node_0_events[0] {
-			MessageSendEvent::SendChannelReestablish { ref node_id, .. } => {
-				assert_eq!(*node_id, nodes[1].node.get_our_node_id());
-				// nodes[0] would send a bogus channel reestablish, so there's no need to handle this.
-			},
-			_ => panic!("Unexpected event"),
-		}
-
-		for msg in node_1_events {
-			if let MessageSendEvent::SendChannelReestablish { ref node_id, ref msg } = msg {
-				nodes[0].node.handle_channel_reestablish(nodes[1].node.get_our_node_id(), msg);
-				assert_eq!(*node_id, nodes[0].node.get_our_node_id());
-			} else if let MessageSendEvent::SendPeerStorageRetrieval { ref node_id, ref msg } = msg
-			{
-				assert_eq!(*node_id, nodes[0].node.get_our_node_id());
-				// Should Panic here!
-				let res = std::panic::catch_unwind(|| {
-					nodes[0]
-						.node
-						.handle_peer_storage_retrieval(nodes[1].node.get_our_node_id(), msg.clone())
-				});
-				assert!(res.is_err());
-				break;
-			} else {
-				panic!("Unexpected event")
-			}
-		}
-		// When we panic'd, we expect to panic on `Drop`.
-		let res = std::panic::catch_unwind(|| drop(nodes));
-		assert!(res.is_err());
-	}
-
 	#[test]
 	#[rustfmt::skip]
 	fn test_keysend_dup_payment_hash() {

lightning/src/ln/reload_tests.rs

@@ -1305,3 +1305,118 @@ fn test_htlc_localremoved_persistence() {
 	let htlc_fail_msg_after_reload = msgs.2.unwrap().update_fail_htlcs[0].clone();
 	assert_eq!(htlc_fail_msg, htlc_fail_msg_after_reload);
 }
+
+
+
+#[test]
+#[cfg(peer_storage)]
+fn test_peer_storage() {
+	let chanmon_cfgs = create_chanmon_cfgs(2);
+	let (persister, chain_monitor);
+	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+	let nodes_0_deserialized;
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+	let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+	let node_a_id = nodes[0].node.get_our_node_id();
+	let node_b_id = nodes[1].node.get_our_node_id();
+
+	let (_, _, cid, _) = create_announced_chan_between_nodes(&nodes, 0, 1);
+	send_payment(&nodes[0], &[&nodes[1]], 1000);
+	let nodes_0_serialized = nodes[0].node.encode();
+	let old_state_monitor = get_monitor!(nodes[0], cid).encode();
+	send_payment(&nodes[0], &[&nodes[1]], 10000);
+	send_payment(&nodes[0], &[&nodes[1]], 9999);
+
+	// Update peer storage with latest commitment txns
+	connect_blocks(&nodes[0], 1);
+	connect_blocks(&nodes[0], 1);
+
+	let peer_storage_msg_events_node0 =
+		nodes[0].chain_monitor.chain_monitor.get_and_clear_pending_msg_events();
+	let peer_storage_msg_events_node1 =
+		nodes[1].chain_monitor.chain_monitor.get_and_clear_pending_msg_events();
+	assert_ne!(peer_storage_msg_events_node0.len(), 0);
+	assert_ne!(peer_storage_msg_events_node1.len(), 0);
+
+	for ps_msg in peer_storage_msg_events_node0 {
+		match ps_msg {
+			MessageSendEvent::SendPeerStorage { ref node_id, ref msg } => {
+				assert_eq!(*node_id, node_b_id);
+				nodes[1].node.handle_peer_storage(node_a_id, msg.clone());
+			},
+			_ => panic!("Unexpected event"),
+		}
+	}
+
+	for ps_msg in peer_storage_msg_events_node1 {
+		match ps_msg {
+			MessageSendEvent::SendPeerStorage { ref node_id, ref msg } => {
+				assert_eq!(*node_id, node_a_id);
+				nodes[0].node.handle_peer_storage(node_b_id, msg.clone());
+			},
+			_ => panic!("Unexpected event"),
+		}
+	}
+
+	nodes[0].node.peer_disconnected(node_b_id);
+	nodes[1].node.peer_disconnected(node_a_id);
+
+	// Reload Node!
+	// TODO: Handle the case where we've completely forgotten about an active channel.
+	reload_node!(
+		nodes[0],
+		test_default_channel_config(),
+		&nodes_0_serialized,
+		&[&old_state_monitor[..]],
+		persister,
+		chain_monitor,
+		nodes_0_deserialized
+	);
+
+	let init_msg = msgs::Init {
+		features: nodes[1].node.init_features(),
+		networks: None,
+		remote_network_address: None,
+	};
+
+	nodes[0].node.peer_connected(node_b_id, &init_msg, true).unwrap();
+	nodes[1].node.peer_connected(node_a_id, &init_msg, true).unwrap();
+
+	let node_1_events = nodes[1].node.get_and_clear_pending_msg_events();
+	assert_eq!(node_1_events.len(), 2);
+
+	let node_0_events = nodes[0].node.get_and_clear_pending_msg_events();
+	assert_eq!(node_0_events.len(), 1);
+
+	match node_0_events[0] {
+		MessageSendEvent::SendChannelReestablish { ref node_id, .. } => {
+			assert_eq!(*node_id, node_b_id);
+			// nodes[0] would send a stale channel reestablish, so there's no need to handle this.
+		},
+		_ => panic!("Unexpected event"),
+	}
+
+	if let MessageSendEvent::SendPeerStorageRetrieval { node_id, msg } = &node_1_events[0] {
+		assert_eq!(*node_id, node_a_id);
+		// Should Panic here!
+		let res = std::panic::catch_unwind(|| {
+			nodes[0].node.handle_peer_storage_retrieval(node_b_id, msg.clone());
+		});
+		assert!(res.is_err());
+	} else {
+		panic!("Unexpected event {node_1_events:?}")
+	}
+
+	if let MessageSendEvent::SendChannelReestablish { .. } = &node_1_events[1] {
+		// After the `peer_storage_retreival` message would come a `channel_reestablish` (which
+		// would also cause nodes[0] to panic) but it already went down due to lost state so
+		// there's nothing to deliver.
+	} else {
+		panic!("Unexpected event {node_1_events:?}")
+	}
+	// When we panic'd, we expect to panic on `Drop`.
+	let res = std::panic::catch_unwind(|| drop(nodes));
+	assert!(res.is_err());
+}
+