Clear duplicate monitor-pending RAA on signer resend
What changed, and why it matters
This commit fixes a bug in the Lightning Dev Kit where, after a peer reconnects, the same cryptographic 'revoke_and_ack' message could be generated and sent twice. The duplicate would contain a secret the peer had already accepted, so the peer would reject it and force-close the payment channel. The fix clears an internal 'monitor-pending' flag when the signer-resume path actually produces the message, preventing the later duplicate.
Apply the patch. The change is small, targeted, and includes a regression test. Nodes running with async signers or under reconnect/monitor-delay stress are most exposed; upgrading avoids unnecessary channel force-closures.
Security signals we found
Duplicate cryptographic revocation secret sent to peer
Peer force-closes channel on rejection of stale revocation secret
State-flag collision between signer-pending and monitor-pending resend paths
Fuzz target (chanmon_consistency) discovered the trigger ordering
Regression test added: test_signer_unblocked_clears_monitor_pending_raa_after_reestablish
Evidence from the diff
In rust-lightning, two flags can independently indicate that an owed revoke_and_ack (RAA) should be resent: signer_pending_revoke_and_ack (signer couldn’t produce the secret yet) and monitor_pending_revoke_and_ack (monitor update blocked during reconnect). A fuzz target found an ordering where both flags referred to the same RAA. If the signer unblocked before the held monitor update completed, signer_maybe_unblocked sent the RAA, but monitor_pending_revoke_and_ack remained set. When the monitor later completed, monitor_updating_restored sent the same RAA again. The peer, already one state ahead, rejected the duplicate revocation secret and force-closed. The patch clears monitor_pending_revoke_and_ack in the signer-resume path only when revoke_and_ack is actually being returned, and adds a regression test reproducing the exact interleaving.
Changed components
lightning/src/ln/channel.rslightning/src/ln/async_signer_tests.rsInspect captured patch +213 / −1
diff --git a/lightning/src/ln/async_signer_tests.rs b/lightning/src/ln/async_signer_tests.rs
index 8edff20..f36c197 100644
--- a/lightning/src/ln/async_signer_tests.rs
+++ b/lightning/src/ln/async_signer_tests.rs
@@ -18,7 +18,7 @@ use bitcoin::{Amount, TxOut};
use crate::chain::channelmonitor::LATENCY_GRACE_PERIOD_BLOCKS;
use crate::chain::ChannelMonitorUpdateStatus;
-use crate::events::{ClosureReason, Event};
+use crate::events::{ClosureReason, Event, HTLCHandlingFailureType};
use crate::ln::chan_utils::ClosingTransaction;
use crate::ln::channel::DISCONNECT_PEER_AWAITING_RESPONSE_TICKS;
use crate::ln::channel_state::{ChannelDetails, ChannelShutdownState};
@@ -498,6 +498,211 @@ fn test_async_raa_peer_disconnect() {
do_test_async_raa_peer_disconnect(UnblockSignerAcrossDisconnectCase::BeforeReestablish, false);
}
+#[test]
+fn test_signer_unblocked_clears_monitor_pending_raa_after_reestablish() {
+ let chanmon_cfgs = create_chanmon_cfgs(3);
+ let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
+ let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
+ let mut nodes = create_network(3, &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 node_c_id = nodes[2].node.get_our_node_id();
+
+ create_announced_chan_between_nodes(&nodes, 0, 1);
+ let chan_bc = create_announced_chan_between_nodes(&nodes, 1, 2);
+
+ // Rebalance so that node C can send a payment back through node B later in the test.
+ send_payment(&nodes[0], &[&nodes[1], &nodes[2]], 5_000_000);
+
+ // Put the B-C channel into AwaitingRAA by having C fail a payment backwards and retaining C's
+ // final RAA instead of delivering it to B immediately.
+ let (_, payment_hash_1, ..) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 1_000_000);
+ nodes[2].node.fail_htlc_backwards(&payment_hash_1);
+ expect_and_process_pending_htlcs_and_htlc_handling_failed(
+ &nodes[2],
+ &[HTLCHandlingFailureType::Receive { payment_hash: payment_hash_1 }],
+ );
+ check_added_monitors(&nodes[2], 1);
+
+ let updates = get_htlc_update_msgs(&nodes[2], &node_b_id);
+ assert!(updates.update_add_htlcs.is_empty());
+ assert_eq!(updates.update_fail_htlcs.len(), 1);
+ assert!(updates.update_fail_malformed_htlcs.is_empty());
+ assert!(updates.update_fee.is_none());
+ nodes[1].node.handle_update_fail_htlc(node_c_id, &updates.update_fail_htlcs[0]);
+
+ let pending_c_raa =
+ commitment_signed_dance_return_raa(&nodes[1], &nodes[2], &updates.commitment_signed, false);
+ check_added_monitors(&nodes[0], 0);
+
+ // While B is waiting for C's RAA, forward another A-to-C payment. B accepts it on the A-B
+ // channel, but cannot forward it over B-C yet, so it is held in B's holding cell.
+ let (route, payment_hash_2, _payment_preimage_2, payment_secret_2) =
+ get_route_and_payment_hash!(nodes[0], nodes[2], 1_000_000);
+ let onion_2 = RecipientOnionFields::secret_only(payment_secret_2, 1_000_000);
+ let id_2 = PaymentId(payment_hash_2.0);
+ nodes[0].node.send_payment_with_route(route, payment_hash_2, onion_2, id_2).unwrap();
+ check_added_monitors(&nodes[0], 1);
+
+ let send_event = SendEvent::from_node(&nodes[0]);
+ assert_eq!(send_event.node_id, node_b_id);
+ assert_eq!(send_event.msgs.len(), 1);
+ nodes[1].node.handle_update_add_htlc(node_a_id, &send_event.msgs[0]);
+ do_commitment_signed_dance(&nodes[1], &nodes[0], &send_event.commitment_msg, false, false);
+
+ expect_and_process_pending_htlcs(&nodes[1], false);
+ check_added_monitors(&nodes[1], 0);
+ assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
+
+ // Now make B owe C an RAA whose monitor update has already completed, but whose RAA cannot be
+ // constructed because B's signer is unavailable.
+ let (route, payment_hash_3, _payment_preimage_3, payment_secret_3) =
+ get_route_and_payment_hash!(nodes[2], nodes[0], 1_000_000);
+ let onion_3 = RecipientOnionFields::secret_only(payment_secret_3, 1_000_000);
+ let id_3 = PaymentId(payment_hash_3.0);
+ nodes[2].node.send_payment_with_route(route, payment_hash_3, onion_3, id_3).unwrap();
+ check_added_monitors(&nodes[2], 1);
+
+ let send_event = SendEvent::from_node(&nodes[2]);
+ assert_eq!(send_event.node_id, node_b_id);
+ assert_eq!(send_event.msgs.len(), 1);
+ nodes[1].node.handle_update_add_htlc(node_c_id, &send_event.msgs[0]);
+ nodes[1].disable_channel_signer_op(&node_c_id, &chan_bc.2, SignerOp::ReleaseCommitmentSecret);
+ nodes[1].node.handle_commitment_signed_batch_test(node_c_id, &send_event.commitment_msg);
+ check_added_monitors(&nodes[1], 1);
+ assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
+
+ // Deliver C's earlier RAA to B while monitor updating is blocked. This frees B's holding-cell
+ // HTLC and leaves a monitor update in flight.
+ chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress);
+ nodes[1].node.handle_revoke_and_ack(node_c_id, &pending_c_raa);
+ assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
+ assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
+ check_added_monitors(&nodes[1], 1);
+
+ nodes[1].node.peer_disconnected(node_c_id);
+ nodes[2].node.peer_disconnected(node_b_id);
+
+ let init_msg = msgs::Init {
+ features: nodes[2].node.init_features(),
+ networks: None,
+ remote_network_address: None,
+ };
+ nodes[1].node.peer_connected(node_c_id, &init_msg, true).unwrap();
+ let bs_reestablish = get_chan_reestablish_msgs!(nodes[1], nodes[2]);
+ assert_eq!(bs_reestablish.len(), 1);
+ let init_msg = msgs::Init {
+ features: nodes[1].node.init_features(),
+ networks: None,
+ remote_network_address: None,
+ };
+ nodes[2].node.peer_connected(node_b_id, &init_msg, false).unwrap();
+ let cs_reestablish = get_chan_reestablish_msgs!(nodes[2], nodes[1]);
+ assert_eq!(cs_reestablish.len(), 1);
+
+ nodes[1].node.handle_channel_reestablish(node_c_id, &cs_reestablish[0]);
+
+ // The signer-pending path now generates the owed RAA before the held monitor update
+ // completes.
+ nodes[1].enable_channel_signer_op(&node_c_id, &chan_bc.2, SignerOp::ReleaseCommitmentSecret);
+ nodes[1].node.signer_unblocked(Some((node_c_id, chan_bc.2)));
+ let (_, signer_revoke_and_ack, signer_commitment_update, _, _, _, _, _) =
+ handle_chan_reestablish_msgs!(nodes[1], nodes[2]);
+ assert!(signer_revoke_and_ack.is_some());
+
+ // Once the held monitor update completes, B must not generate the same RAA a second time via
+ // the monitor-pending path.
+ chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::Completed);
+ let (latest_update, _) = nodes[1].chain_monitor.get_latest_mon_update_id(chan_bc.2);
+ nodes[1].chain_monitor.chain_monitor.force_channel_monitor_updated(chan_bc.2, latest_update);
+ check_added_monitors(&nodes[1], 0);
+ let (_, duplicate_revoke_and_ack, monitor_commitment_update, _, _, _, _, _) =
+ handle_chan_reestablish_msgs!(nodes[1], nodes[2]);
+ assert!(duplicate_revoke_and_ack.is_none());
+
+ nodes[2].node.handle_channel_reestablish(node_b_id, &bs_reestablish[0]);
+ let (_, c_revoke_and_ack, c_commitment_update, _, _, _, _, _) =
+ handle_chan_reestablish_msgs!(nodes[2], nodes[1]);
+ assert!(c_revoke_and_ack.is_none());
+ assert!(c_commitment_update.is_none());
+
+ nodes[2].node.handle_revoke_and_ack(node_b_id, &signer_revoke_and_ack.unwrap());
+ check_added_monitors(&nodes[2], 1);
+
+ let commitment_update = signer_commitment_update.or(monitor_commitment_update);
+ if let Some(commitment_update) = commitment_update {
+ let send_event = SendEvent::from_commitment_update(node_c_id, chan_bc.2, commitment_update);
+ assert_eq!(send_event.node_id, node_c_id);
+ for update_add in send_event.msgs {
+ nodes[2].node.handle_update_add_htlc(node_b_id, &update_add);
+ }
+ nodes[2].node.handle_commitment_signed_batch_test(node_b_id, &send_event.commitment_msg);
+ check_added_monitors(&nodes[2], 1);
+ let (c_raa, c_commitment_signed) = get_revoke_commit_msgs(&nodes[2], &node_b_id);
+ nodes[1].node.handle_revoke_and_ack(node_c_id, &c_raa);
+ check_added_monitors(&nodes[1], 1);
+ nodes[1].node.handle_commitment_signed_batch_test(node_c_id, &c_commitment_signed);
+ check_added_monitors(&nodes[1], 1);
+ let b_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, node_c_id);
+ nodes[2].node.handle_revoke_and_ack(node_b_id, &b_raa);
+ check_added_monitors(&nodes[2], 1);
+ }
+
+ let (route, final_payment_hash, _final_payment_preimage, final_payment_secret) =
+ get_route_and_payment_hash!(nodes[1], nodes[2], 100_000);
+ let final_payment_id = PaymentId(final_payment_hash.0);
+ nodes[1]
+ .node
+ .send_payment_with_route(
+ route,
+ final_payment_hash,
+ RecipientOnionFields::secret_only(final_payment_secret, 100_000),
+ final_payment_id,
+ )
+ .unwrap();
+ check_added_monitors(&nodes[1], 1);
+ let final_payment_event = nodes[1].node.get_and_clear_pending_msg_events().remove(0);
+ match &final_payment_event {
+ MessageSendEvent::UpdateHTLCs { node_id, .. } => assert_eq!(*node_id, node_c_id),
+ _ => panic!("Unexpected event"),
+ }
+ do_pass_along_path(
+ PassAlongPathArgs::new(
+ &nodes[1],
+ &[&nodes[2]],
+ 100_000,
+ final_payment_hash,
+ final_payment_event,
+ )
+ .with_payment_secret(final_payment_secret)
+ .without_clearing_recipient_events(),
+ );
+
+ let claimable_events = nodes[2].node.get_and_clear_pending_events();
+ let final_claimable = claimable_events
+ .iter()
+ .find(|event| {
+ matches!(
+ event,
+ Event::PaymentClaimable { payment_hash, .. } if *payment_hash == final_payment_hash
+ )
+ })
+ .unwrap();
+ check_payment_claimable(
+ final_claimable,
+ final_payment_hash,
+ final_payment_secret,
+ 100_000,
+ None,
+ node_c_id,
+ );
+ expect_htlc_failure_conditions(
+ nodes[1].node.get_and_clear_pending_events(),
+ &[HTLCHandlingFailureType::Forward { node_id: Some(node_c_id), channel_id: chan_bc.2 }],
+ );
+}
+
fn do_test_async_raa_peer_disconnect(
test_case: UnblockSignerAcrossDisconnectCase, raa_blocked_by_commit_point: bool,
) {
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 3df6f5f..d0fc940 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -10259,6 +10259,13 @@ where
self.context.signer_pending_commitment_update = true;
commitment_update = None;
}
+ if revoke_and_ack.is_some() {
+ // If signer-pending state regenerated an RAA, the monitor update for that RAA was
+ // already persisted before we set `signer_pending_revoke_and_ack`. Thus, if reconnect
+ // also marked the same RAA monitor-pending while another monitor update was in flight,
+ // the RAA we're returning here satisfies that monitor-pending resend.
+ self.context.monitor_pending_revoke_and_ack = false;
+ }
let (closing_signed, signed_closing_tx, shutdown_result) = if self.context.signer_pending_closing {
debug_assert!(self.context.last_sent_closing_fee.is_some());
Why this scored 66/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.