Clear pending flags when peer disconnects (fixup)

waterson · waterson · commit bdf6847632f0 · 2024-01-11T11:13:47.000-05:00
diff --git a/lightning/src/ln/async_signer_tests.rs b/lightning/src/ln/async_signer_tests.rs
@@ -1243,6 +1243,73 @@ fn peer_restart_with_blocked_signer_before_pending_payment() {
 	expect_payment_claimable!(nodes[1], payment_hash, payment_secret, 1_000_000);
 }
 
+#[test]
+fn no_stray_channel_reestablish() {
+	// Original fuzz trace.
+	// a0 Disable A’s signer.
+	// 2c Disconnect A and B, then restart A.
+	// 0e Reconnect A and B.
+	// 2d Disconnect A and B (and C), then restart B.
+	// a1 Unblock A’s signer get_per_commitment_point
+	// ff Reset.
+
+	let chanmon_cfgs = create_chanmon_cfgs(2);
+	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+	let alice_persister;
+	let bob_persister;
+	let alice_new_chain_monitor;
+	let bob_new_chain_monitor;
+
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+	let alice_deserialized;
+	let bob_deserialized;
+
+	let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+	let (_, _, channel_id, _) = create_announced_chan_between_nodes(&nodes, 0, 1);
+
+	// Turn off Alice's signer.
+	eprintln!("disabling alice's signer");
+	nodes[0].set_channel_signer_ops_available(
+		&nodes[1].node.get_our_node_id(), &channel_id,
+		ops::GET_PER_COMMITMENT_POINT | ops::RELEASE_COMMITMENT_SECRET | ops::SIGN_COUNTERPARTY_COMMITMENT,
+		false);
+
+	// Disconnect Bob and restart Alice
+	eprintln!("disconnecting bob");
+	nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id());
+
+	eprintln!("restarting alice");
+	{
+		let alice_serialized = nodes[0].node.encode();
+		let alice_monitor_serialized = get_monitor!(nodes[0], channel_id).encode();
+		reload_node!(nodes[0], *nodes[0].node.get_current_default_configuration(), &alice_serialized, &[&alice_monitor_serialized], alice_persister, alice_new_chain_monitor, alice_deserialized);
+	}
+
+	// Reconnect Alice and Bob.
+	eprintln!("reconnecting alice and bob");
+	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
+	}, false).unwrap();
+
+	// Disconnect Alice and restart Bob
+	eprintln!("disconnecting alice");
+	nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id());
+
+	eprintln!("restarting bob");
+	{
+		let bob_serialized = nodes[1].node.encode();
+		let bob_monitor_serialized = get_monitor!(nodes[1], channel_id).encode();
+		reload_node!(nodes[1], *nodes[1].node.get_current_default_configuration(), &bob_serialized, &[&bob_monitor_serialized], bob_persister, bob_new_chain_monitor, bob_deserialized);
+	}
+
+	eprintln!("unblocking alice's signer for get_per_commitment_point");
+	nodes[0].set_channel_signer_ops_available(&nodes[1].node.get_our_node_id(), &channel_id, ops::GET_PER_COMMITMENT_POINT, true);
+	nodes[0].node.signer_unblocked(None);
+
+	let events = nodes[0].node.get_and_clear_pending_msg_events();
+	assert!(events.is_empty(), "Expected no events from Alice, got {:?}", events);
+}
+
 #[test]
 fn dont_elide_channely_ready_from_state_1() {
 	// 1. Disable Alice's signer.
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
@@ -4216,6 +4216,26 @@ impl<SP: Deref> Channel<SP> where
 			return Err(())
 		}
 
+		// If we have requests pending to the signer, then clear out any of them that will be recomputed
+		// on `channel_reestablish`. This ensures we don't send them twice if the signer unblocks before
+		// we receive the counterparty's reestablish message.
+		if self.context.signer_pending_channel_reestablish {
+			log_trace!(logger, "Clearing signer_pending_channel_reestablish");
+			self.context.signer_pending_channel_reestablish = false;
+		}
+		if self.context.signer_pending_channel_ready {
+			log_trace!(logger, "Clearing signer_pending_channel_ready");
+			self.context.signer_pending_channel_ready = false;
+		}
+		if self.context.signer_pending_revoke_and_ack {
+			log_trace!(logger, "Clearing signer_pending_revoke_and_ack");
+			self.context.signer_pending_revoke_and_ack = false;
+		}
+		if self.context.signer_pending_commitment_update {
+			log_trace!(logger, "Clearing signer_pending_commitment_update");
+			self.context.signer_pending_commitment_update = false;
+		}
+
 		if self.context.channel_state.is_peer_disconnected() {
 			// While the below code should be idempotent, it's simpler to just return early, as
 			// redundant disconnect events can fire, though they should be rare.
@@ -4232,22 +4252,6 @@ impl<SP: Deref> Channel<SP> where
 		self.context.pending_counterparty_closing_signed = None;
 		self.context.closing_fee_limits = None;
 
-		// If we have requests pending to the signer, then clear out any of them that will be recomputed
-		// on `channel_reestablish`. This ensures we don't send them twice if the signer unblocks before
-		// we receive the counterparty's reestablish message.
-		if self.context.signer_pending_channel_ready {
-			log_trace!(logger, "clearing signer_pending_channel_ready");
-			self.context.signer_pending_channel_ready = false;
-		}
-		if self.context.signer_pending_revoke_and_ack {
-			log_trace!(logger, "clearing signer_pending_revoke_and_ack");
-			self.context.signer_pending_revoke_and_ack = false;
-		}
-		if self.context.signer_pending_commitment_update {
-			log_trace!(logger, "clearing signer_pending_commitment_update");
-			self.context.signer_pending_commitment_update = false;
-		}
-
 		let mut inbound_drop_count = 0;
 		self.context.pending_inbound_htlcs.retain(|htlc| {
 			match htlc.state {
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
@@ -7381,28 +7381,41 @@ where
 					let logger = WithChannelContext::from(&self.logger, &chan.context);
 					let msgs = chan.signer_maybe_unblocked(&&logger);
 					if let Some(msg) = msgs.channel_reestablish {
+						log_trace!(logger, "Queuing channel_reestablish to {}", node_id);
 						pending_msg_events.push(events::MessageSendEvent::SendChannelReestablish { node_id, msg });
 					}
 					match (msgs.commitment_update, msgs.raa) {
 						(Some(cu), Some(raa)) if msgs.order == RAACommitmentOrder::CommitmentFirst => {
+							log_trace!(logger, "Queuing update_htlcs to {}", node_id);
 							pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs { node_id, updates: cu });
+							log_trace!(logger, "Queuing revoke_and_ack to {}", node_id);
 							pending_msg_events.push(events::MessageSendEvent::SendRevokeAndACK { node_id, msg: raa });
 						},
 						(Some(cu), Some(raa)) if msgs.order == RAACommitmentOrder::RevokeAndACKFirst => {
+							log_trace!(logger, "Queuing revoke_and_ack to {}", node_id);
 							pending_msg_events.push(events::MessageSendEvent::SendRevokeAndACK { node_id, msg: raa });
+							log_trace!(logger, "Queuing update_htlcs to {}", node_id);
 							pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs { node_id, updates: cu });
 						},
-						(Some(cu), _) => pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs { node_id, updates: cu }),
-						(_, Some(raa)) => pending_msg_events.push(events::MessageSendEvent::SendRevokeAndACK { node_id, msg: raa }),
+						(Some(cu), _) => {
+							log_trace!(logger, "Queuing update_htlcs to {}", node_id);
+							pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs { node_id, updates: cu })
+						},
+						(_, Some(raa)) => {
+							log_trace!(logger, "Queuing revoke_and_ack to {}", node_id);
+							pending_msg_events.push(events::MessageSendEvent::SendRevokeAndACK { node_id, msg: raa })
+						},
 						(_, _) => (),
 					};
 					if let Some(msg) = msgs.funding_signed {
+						log_trace!(logger, "Queuing funding_signed to {}", node_id);
 						pending_msg_events.push(events::MessageSendEvent::SendFundingSigned {
 							node_id,
 							msg,
 						});
 					}
 					if let Some(msg) = msgs.channel_ready {
+						log_trace!(logger, "Queuing channel_ready to {}", node_id);
 						send_channel_ready!(self, pending_msg_events, chan, msg);
 					}
 				}
@@ -7411,6 +7424,7 @@ where
 					let msgs = chan.signer_maybe_unblocked(&&logger);
 					let node_id = phase.context().get_counterparty_node_id();
 					if let Some(msg) = msgs.accept_channel {
+						log_trace!(logger, "Queuing accept_channel to {}", node_id);
 						pending_msg_events.push(events::MessageSendEvent::SendAcceptChannel { node_id, msg });
 					}
 				}
@@ -7419,9 +7433,11 @@ where
 					let msgs = chan.signer_maybe_unblocked(&self.chain_hash, &&logger);
 					let node_id = phase.context().get_counterparty_node_id();
 					if let Some(msg) = msgs.open_channel {
+						log_trace!(logger, "Queuing open_channel to {}", node_id);
 						pending_msg_events.push(events::MessageSendEvent::SendOpenChannel { node_id, msg });
 					}
 					if let Some(msg) = msgs.funding_created {
+						log_trace!(logger, "Queuing funding_created to {}", node_id);
 						pending_msg_events.push(events::MessageSendEvent::SendFundingCreated { node_id, msg });
 					}
 				}
@@ -9050,6 +9066,7 @@ where
 				).for_each(|chan| {
 					let logger = WithChannelContext::from(&self.logger, &chan.context);
 					if let Some(msg) = chan.get_channel_reestablish(&&logger) {
+						log_trace!(logger, "Queuing channel_reestablish to {}", chan.context.get_counterparty_node_id());
 						pending_msg_events.push(events::MessageSendEvent::SendChannelReestablish {
 							node_id: chan.context.get_counterparty_node_id(),
 							msg,
