Lower strictness of pending monitor update while awaiting tx_signatures
What changed, and why it matters
This commit fixes a logic bug in the Lightning Dev Kit's channel splicing code. Previously, the code wrongly assumed that no unrelated monitor update could be pending when a splice `tx_signatures` message arrived while the channel was quiescent. In reality, an unrelated HTLC preimage could trigger a pending monitor update at the same time. The fix relaxes the strictness so the protocol does not panic or get stuck, and adds a regression test covering the scenario.
Review the relaxed assertions to ensure they fully cover all monitor update variants that may legitimately be pending during `tx_signatures` handling. Run the new regression test and fuzzing around splice + HTLC preimage races. Consider whether any other quiescence checks elsewhere make the same overly strict assumption.
Security signals we found
Assertion relaxation in state-machine handling of splice `tx_signatures`
Race condition between unrelated HTLC preimage monitor update and splice signature exchange
Potential panic or protocol stall due to overly strict debug assertion
Regression test added for async monitor update during splicing
Evidence from the diff
The change is in lightning/src/ln/channel.rs in the splice/tx_signatures handling path. It replaces a blanket debug_assert!(!is_monitor_update_in_progress()) with a more precise assertion: if we are awaiting a monitor update, then monitor_pending_tx_signatures must not be set (i.e., the pending update must not be the splice signature update itself) when receiving tx_signatures. It also changes the condition for suppressing our own tx_signatures emission to require both is_awaiting_monitor_update() and monitor_pending_tx_signatures. A new test in splicing_tests.rs reproduces the race: a forwarded HTLC preimage causes an unrelated monitor update to be in flight while delayed tx_signatures are delivered, confirming the channel no longer panics or stalls.
Changed components
lightning/src/ln/channel.rslightning/src/ln/splicing_tests.rsLDK channel state machine for splicingMonitor update pending logic during `tx_signatures` exchangeInspect captured patch +174 / −4
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index ef8b8a4..d609f89 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -9530,8 +9530,10 @@ where
&mut self, funding_tx_signed: &mut FundingTxSigned, funding_tx: Transaction,
best_block_height: u32, logger: &WithChannelContext<'a, L>,
) {
- debug_assert!(!self.context.channel_state.is_monitor_update_in_progress());
- debug_assert!(!self.context.channel_state.is_awaiting_remote_revoke());
+ debug_assert!(
+ !self.is_awaiting_monitor_update() || !self.context.monitor_pending_tx_signatures
+ );
+ debug_assert!(!self.context.is_waiting_on_peer_pending_channel_update());
if let Some(pending_splice) = self.pending_splice.as_mut() {
if let Some(FundingNegotiation::AwaitingSignatures {
@@ -9684,11 +9686,11 @@ where
splice_negotiated: None,
splice_locked: None,
};
- if self.is_awaiting_monitor_update() {
+ if self.is_awaiting_monitor_update() && self.context.monitor_pending_tx_signatures {
// Although the user may have already provided our `tx_signatures`, we must not send
// them if we're waiting for the monitor to durably persist the counterparty's signature
// for our initial commitment post-splice.
- debug_assert!(self.context.monitor_pending_tx_signatures);
+ debug_assert!(holder_tx_signatures.is_some());
log_debug!(
logger,
"Waiting for async monitor update to complete prior to releasing our tx_signatures"
diff --git a/lightning/src/ln/splicing_tests.rs b/lightning/src/ln/splicing_tests.rs
index f480c4e..21b5303 100644
--- a/lightning/src/ln/splicing_tests.rs
+++ b/lightning/src/ln/splicing_tests.rs
@@ -34,6 +34,7 @@ use crate::types::string::UntrustedString;
use crate::util::config::UserConfig;
use crate::util::errors::APIError;
use crate::util::ser::Writeable;
+use crate::util::test_channel_signer::SignerOp;
use crate::util::wallet_utils::{
CoinSelection, CoinSelectionSourceSync, ConfirmedUtxo, Input, WalletSourceSync, WalletSync,
};
@@ -10248,3 +10249,170 @@ fn test_splice_out_maximum_includes_pending_claimed_inbound_htlc() {
assert!(nodes[1].node.splice_channel(&channel_id, &node_id_0).is_ok());
}
+
+#[test]
+fn test_async_splice_receives_tx_signatures_while_unrelated_monitor_update_pending() {
+ 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 nodes = create_network(3, &node_cfgs, &node_chanmgrs);
+
+ let channel_id = create_announced_chan_between_nodes(&nodes, 0, 1).2;
+ create_announced_chan_between_nodes(&nodes, 1, 2);
+
+ let (initiator, acceptor) = (&nodes[0], &nodes[1]);
+ let initiator_node_id = initiator.node.get_our_node_id();
+ let acceptor_node_id = acceptor.node.get_our_node_id();
+ let final_node_id = nodes[2].node.get_our_node_id();
+
+ // Leave a forwarded HTLC across A-B and B-C. Later, C will reveal the
+ // preimage so B has to persist an unrelated preimage update on A-B while the
+ // delayed splice `tx_signatures` are still in flight.
+ let (payment_preimage, payment_hash, ..) =
+ route_payment(initiator, &[acceptor, &nodes[2]], 1_000_000);
+
+ // Keep the A-B splice from completing immediately at B. The disabled
+ // counterparty-commitment signer forces B to wait for both the signer and
+ // the splice monitor update before it can send `tx_signatures`.
+ acceptor.disable_channel_signer_op(
+ &initiator_node_id,
+ &channel_id,
+ SignerOp::SignCounterpartyCommitment,
+ );
+
+ let outputs = vec![TxOut {
+ value: Amount::from_sat(1_000),
+ script_pubkey: initiator.wallet_source.get_change_script().unwrap(),
+ }];
+ let contribution = initiate_splice_out(initiator, acceptor, channel_id, outputs).unwrap();
+ negotiate_splice_tx(initiator, acceptor, channel_id, contribution);
+
+ let event = get_event!(initiator, Event::FundingTransactionReadyForSigning);
+ if let Event::FundingTransactionReadyForSigning { unsigned_transaction, .. } = event {
+ let partially_signed_tx = initiator.wallet_source.sign_tx(unsigned_transaction).unwrap();
+ initiator
+ .node
+ .funding_transaction_signed(&channel_id, &acceptor_node_id, partially_signed_tx)
+ .unwrap();
+ }
+
+ let initiator_commit_sig = get_htlc_update_msgs(initiator, &acceptor_node_id);
+
+ // B accepts A's splice commitment, but the monitor update remains pending.
+ // This is the async-signing window that normally guards emission of B's
+ // `tx_signatures`.
+ chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress);
+ acceptor
+ .node
+ .handle_commitment_signed(initiator_node_id, &initiator_commit_sig.commitment_signed[0]);
+ check_added_monitors(acceptor, 1);
+ assert!(acceptor.node.get_and_clear_pending_msg_events().is_empty());
+
+ // Unblock only the signer side first. B can now produce its splice
+ // `commitment_signed`, but still must not send `tx_signatures` until the
+ // monitor update above completes.
+ acceptor.enable_channel_signer_op(
+ &initiator_node_id,
+ &channel_id,
+ SignerOp::SignCounterpartyCommitment,
+ );
+ acceptor.node.signer_unblocked(None);
+
+ let msg_events = acceptor.node.get_and_clear_pending_msg_events();
+ assert_eq!(msg_events.len(), 1, "{msg_events:?}");
+ if let MessageSendEvent::UpdateHTLCs { updates, .. } = &msg_events[0] {
+ initiator.node.handle_commitment_signed(acceptor_node_id, &updates.commitment_signed[0]);
+ check_added_monitors(initiator, 1);
+ } else {
+ panic!("Unexpected event");
+ }
+
+ // Completing B's splice monitor update releases B's `tx_signatures`. This
+ // is the update for which `monitor_pending_tx_signatures` is expected to be
+ // set.
+ acceptor.chain_monitor.complete_sole_pending_chan_update(&channel_id);
+
+ let acceptor_tx_signatures =
+ get_event_msg!(acceptor, MessageSendEvent::SendTxSignatures, initiator_node_id);
+ initiator.node.handle_tx_signatures(acceptor_node_id, &acceptor_tx_signatures);
+
+ // A can now fully sign and broadcast the splice transaction. Save A's
+ // reciprocal `tx_signatures` instead of delivering them to B, so B later
+ // sees an old splice message after another monitor update has started.
+ let delayed_initiator_tx_signatures =
+ get_event_msg!(initiator, MessageSendEvent::SendTxSignatures, acceptor_node_id);
+ let mut broadcasted = initiator.tx_broadcaster.txn_broadcast();
+ assert_eq!(broadcasted.len(), 1, "{broadcasted:?}");
+ let splice_tx = broadcasted.pop().unwrap();
+
+ // Confirm the splice on both A and B before B receives A's delayed
+ // `tx_signatures`. This mirrors the fuzz timeline where one side's
+ // broadcast can reach chain before the reciprocal message reaches its peer.
+ mine_transaction(initiator, &splice_tx);
+ mine_transaction(acceptor, &splice_tx);
+ let _ = get_event!(initiator, Event::SpliceNegotiated);
+
+ // Claiming the forwarded payment at C creates an HTLC fulfill that B must
+ // propagate backward over the same A-B channel that is being spliced.
+ nodes[2].node.claim_funds(payment_preimage);
+ check_added_monitors(&nodes[2], 1);
+ expect_payment_claimed!(nodes[2], payment_hash, 1_000_000);
+
+ let mut commitment_update = get_htlc_update_msgs(&nodes[2], &acceptor_node_id);
+ assert_eq!(commitment_update.update_fulfill_htlcs.len(), 1);
+ // Deliver only the fulfill to B and make B's A-B monitor update stay
+ // in-flight. This monitor update is unrelated to splice tx signatures: it
+ // durably records the payment preimage so B can safely settle the incoming
+ // HTLC from A.
+ chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress);
+ acceptor.node.handle_update_fulfill_htlc(
+ final_node_id,
+ commitment_update.update_fulfill_htlcs.remove(0),
+ );
+ check_added_monitors(acceptor, 1);
+ assert!(acceptor.node.get_and_clear_pending_msg_events().is_empty());
+
+ // Deliver A's delayed splice `tx_signatures` while B is waiting on the unrelated HTLC-preimage
+ // monitor update. B's `tx_signatures` was already released, so there's no message to send and
+ // we should expect the splice negotiation to complete.
+ acceptor.node.handle_tx_signatures(initiator_node_id, &delayed_initiator_tx_signatures);
+ expect_splice_pending_event(acceptor, &initiator_node_id);
+
+ // Finally, drive the state machines to completion.
+ acceptor.chain_monitor.complete_sole_pending_chan_update(&channel_id);
+ let mut update_fulfill = get_htlc_update_msgs(acceptor, &initiator_node_id);
+ check_added_monitors(acceptor, 1);
+ let payment_forwarded = get_event!(acceptor, Event::PaymentForwarded);
+ expect_payment_forwarded(
+ payment_forwarded,
+ acceptor,
+ initiator,
+ &nodes[2],
+ Some(1000),
+ None,
+ false,
+ false,
+ false,
+ );
+
+ do_commitment_signed_dance(
+ acceptor,
+ &nodes[2],
+ &commitment_update.commitment_signed,
+ false,
+ false,
+ );
+
+ initiator.node.handle_update_fulfill_htlc(
+ acceptor_node_id,
+ update_fulfill.update_fulfill_htlcs.remove(0),
+ );
+ do_commitment_signed_dance(
+ initiator,
+ acceptor,
+ &update_fulfill.commitment_signed,
+ false,
+ false,
+ );
+ expect_payment_sent(initiator, payment_preimage, None, true, true);
+}
Why this scored 57/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.