← Watch feed
Free holding cell upon handling a counterparty tx_abort
Public commit record
85/100 · Strong
What the developer wrote
Authored by Wilmer Paulino
Free holding cell upon handling a counterparty tx_abort
After cad88af, a few code paths that also lead to a quiescence exit were
not accounted for. This commit addresses the path where we exit
quiescence due to processing a counterparty's `tx_abort` message.
✓ Specific, descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context✓ Explains rationale or failure mode
What changed, and why it matters
This patch fixes a bug in LDK's Lightning splicing code. During a splice, the channel enters a quiet 'quiescence' state where new HTLCs (payments) are held in a temporary 'holding cell.' When the splice is aborted via a counterparty `tx_abort` message, the channel should exit quiescence and release any held payments. Previously, one code path forgot to free the holding cell, so legitimate queued payments could get stuck until another event triggered their release. The patch makes the `tx_abort` handler return whether quiescence was exited, and if so, immediately frees the holding cell. The included test verifies that a queued HTLC is sent out after `tx_abort`.
Recommended action
Treat as a reliability/availability bug fix rather than an active exploit. Review other quiescence exit paths to ensure holding cells are freed consistently. Users running splice-enabled nodes should upgrade to avoid stuck HTLCs during splice aborts.
Security signals we found
01
Logic error causing payment/HTLC stalls after splice abort
02
Missing cleanup of quiescence holding cell on a specific exit path
03
Regression test added demonstrating stuck-then-freed HTLC behavior
04
Follow-up to prior fix cad88af for related quiescence exit paths
Evidence from the diff
In channel.rs, Channel::tx_abort now returns a third tuple element exited_quiescence: bool, set to true only when a funded channel with a pending splice resets its splice state. In channelmanager.rs, internal_tx_abort uses that flag to call check_free_peer_holding_cells and then handle_holding_cell_free_result after dropping the per-peer lock. This ensures HTLCs buffered in the holding cell during quiescence are released when the splice aborts. A regression test in splicing_tests.rs queues an HTLC during a splice, aborts with tx_abort, and asserts that the resulting message events include both the echoed tx_abort and an UpdateHTLCs releasing the held HTLC.
Changed components
lightning/src/ln/channel.rslightning/src/ln/channelmanager.rslightning/src/ln/splicing_tests.rsLDK splicing/quiescence handlingHolding cell / HTLC queueingInspect captured patch +87 / −53
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index d1adbf7..4c31b69 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -2029,13 +2029,13 @@ where
pub fn tx_abort<L: Logger>(
&mut self, msg: &msgs::TxAbort, logger: &L,
- ) -> Result<(Option<msgs::TxAbort>, Option<SpliceFundingFailed>), ChannelError> {
+ ) -> Result<(Option<msgs::TxAbort>, Option<SpliceFundingFailed>, bool), ChannelError> {
// If we have not sent a `tx_abort` message for this negotiation previously, we need to echo
// back a tx_abort message according to the spec:
// https://github.com/lightning/bolts/blob/247e83d/02-peer-protocol.md?plain=1#L560-L561
// For rationale why we echo back `tx_abort`:
// https://github.com/lightning/bolts/blob/247e83d/02-peer-protocol.md?plain=1#L578-L580
- let (should_ack, splice_funding_failed) = match &mut self.phase {
+ let (should_ack, splice_funding_failed, exited_quiescence) = match &mut self.phase {
ChannelPhase::Undefined => unreachable!(),
ChannelPhase::UnfundedOutboundV1(_) | ChannelPhase::UnfundedInboundV1(_) => {
let err = "Got an unexpected tx_abort message: This is an unfunded channel created with V1 channel establishment";
@@ -2044,7 +2044,7 @@ where
ChannelPhase::UnfundedV2(pending_v2_channel) => {
let had_constructor =
pending_v2_channel.interactive_tx_constructor.take().is_some();
- (had_constructor, None)
+ (had_constructor, None, false)
},
ChannelPhase::Funded(funded_channel) => {
if funded_channel.has_pending_splice_awaiting_signatures()
@@ -2072,11 +2072,11 @@ where
.unwrap_or(false);
debug_assert!(has_funding_negotiation);
let splice_funding_failed = funded_channel.reset_pending_splice_state();
- (true, splice_funding_failed)
+ (true, splice_funding_failed, true)
} else {
// We were not tracking the pending funding negotiation state anymore, likely
// due to a disconnection or already having sent our own `tx_abort`.
- (false, None)
+ (false, None, false)
}
},
};
@@ -2092,7 +2092,7 @@ where
}
});
- Ok((tx_abort, splice_funding_failed))
+ Ok((tx_abort, splice_funding_failed, exited_quiescence))
}
#[rustfmt::skip]
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 67b2dc8..36fbd32 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -11632,55 +11632,68 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
fn internal_tx_abort(
&self, counterparty_node_id: &PublicKey, msg: &msgs::TxAbort,
) -> Result<NotifyOption, MsgHandleErrInternal> {
- let per_peer_state = self.per_peer_state.read().unwrap();
- let peer_state_mutex = per_peer_state.get(counterparty_node_id).ok_or_else(|| {
- debug_assert!(false);
- MsgHandleErrInternal::no_such_peer(counterparty_node_id, msg.channel_id)
- })?;
- let mut peer_state_lock = peer_state_mutex.lock().unwrap();
- let peer_state = &mut *peer_state_lock;
- match peer_state.channel_by_id.entry(msg.channel_id) {
- hash_map::Entry::Occupied(mut chan_entry) => {
- let res = chan_entry.get_mut().tx_abort(msg, &self.logger);
- let (tx_abort, splice_failed) =
- try_channel_entry!(self, peer_state, res, chan_entry);
+ let (result, holding_cell_res) = {
+ let per_peer_state = self.per_peer_state.read().unwrap();
+ let peer_state_mutex = per_peer_state.get(counterparty_node_id).ok_or_else(|| {
+ debug_assert!(false);
+ MsgHandleErrInternal::no_such_peer(counterparty_node_id, msg.channel_id)
+ })?;
+ let mut peer_state_lock = peer_state_mutex.lock().unwrap();
+ let peer_state = &mut *peer_state_lock;
+ match peer_state.channel_by_id.entry(msg.channel_id) {
+ hash_map::Entry::Occupied(mut chan_entry) => {
+ let res = chan_entry.get_mut().tx_abort(msg, &self.logger);
+ let (tx_abort, splice_failed, exited_quiescence) =
+ try_channel_entry!(self, peer_state, res, chan_entry);
- let persist = if tx_abort.is_some() || splice_failed.is_some() {
- NotifyOption::DoPersist
- } else {
- NotifyOption::SkipPersistNoEvents
- };
+ let persist = if tx_abort.is_some() || splice_failed.is_some() {
+ NotifyOption::DoPersist
+ } else {
+ NotifyOption::SkipPersistNoEvents
+ };
- if let Some(tx_abort_msg) = tx_abort {
- peer_state.pending_msg_events.push(MessageSendEvent::SendTxAbort {
- node_id: *counterparty_node_id,
- msg: tx_abort_msg,
- });
- }
+ if let Some(tx_abort_msg) = tx_abort {
+ peer_state.pending_msg_events.push(MessageSendEvent::SendTxAbort {
+ node_id: *counterparty_node_id,
+ msg: tx_abort_msg,
+ });
+ }
- if let Some(splice_funding_failed) = splice_failed {
- let pending_events = &mut self.pending_events.lock().unwrap();
- pending_events.push_back((
- events::Event::SpliceFailed {
- channel_id: msg.channel_id,
- counterparty_node_id: *counterparty_node_id,
- user_channel_id: chan_entry.get().context().get_user_id(),
- abandoned_funding_txo: splice_funding_failed.funding_txo,
- channel_type: splice_funding_failed.channel_type,
- contributed_inputs: splice_funding_failed.contributed_inputs,
- contributed_outputs: splice_funding_failed.contributed_outputs,
- },
- None,
- ));
- }
+ if let Some(splice_funding_failed) = splice_failed {
+ let pending_events = &mut self.pending_events.lock().unwrap();
+ pending_events.push_back((
+ events::Event::SpliceFailed {
+ channel_id: msg.channel_id,
+ counterparty_node_id: *counterparty_node_id,
+ user_channel_id: chan_entry.get().context().get_user_id(),
+ abandoned_funding_txo: splice_funding_failed.funding_txo,
+ channel_type: splice_funding_failed.channel_type,
+ contributed_inputs: splice_funding_failed.contributed_inputs,
+ contributed_outputs: splice_funding_failed.contributed_outputs,
+ },
+ None,
+ ));
+ }
- Ok(persist)
- },
- hash_map::Entry::Vacant(_) => Err(MsgHandleErrInternal::no_such_channel_for_peer(
- counterparty_node_id,
- msg.channel_id,
- )),
- }
+ let holding_cell_res = if exited_quiescence {
+ self.check_free_peer_holding_cells(peer_state)
+ } else {
+ Vec::new()
+ };
+ (Ok(persist), holding_cell_res)
+ },
+ hash_map::Entry::Vacant(_) => (
+ Err(MsgHandleErrInternal::no_such_channel_for_peer(
+ counterparty_node_id,
+ msg.channel_id,
+ )),
+ Vec::new(),
+ ),
+ }
+ };
+
+ self.handle_holding_cell_free_result(holding_cell_res);
+ result
}
#[rustfmt::skip]
diff --git a/lightning/src/ln/splicing_tests.rs b/lightning/src/ln/splicing_tests.rs
index c051f29..fc5c181 100644
--- a/lightning/src/ln/splicing_tests.rs
+++ b/lightning/src/ln/splicing_tests.rs
@@ -2037,6 +2037,13 @@ fn fail_splice_on_tx_abort() {
initiate_splice_in(initiator, acceptor, channel_id, Amount::from_sat(splice_in_amount));
let _ = complete_splice_handshake(initiator, acceptor);
+ // Queue an outgoing HTLC to the holding cell. It should be freed once we exit quiescence.
+ let (route, payment_hash, _payment_preimage, payment_secret) =
+ get_route_and_payment_hash!(initiator, acceptor, 1_000_000);
+ let onion = RecipientOnionFields::secret_only(payment_secret);
+ let payment_id = PaymentId(payment_hash.0);
+ initiator.node.send_payment_with_route(route, payment_hash, onion, payment_id).unwrap();
+
let tx_add_input =
get_event_msg!(initiator, MessageSendEvent::SendTxAddInput, node_id_acceptor);
acceptor.node.handle_tx_add_input(node_id_initiator, &tx_add_input);
@@ -2057,8 +2064,22 @@ fn fail_splice_on_tx_abort() {
_ => panic!("Expected Event::SpliceFailed"),
}
- let tx_abort = get_event_msg!(initiator, MessageSendEvent::SendTxAbort, node_id_acceptor);
- acceptor.node.handle_tx_abort(node_id_initiator, &tx_abort);
+ // We exit quiescence upon receiving `tx_abort`, so we should see our `tx_abort` echo and the
+ // holding cell be immediately freed.
+ let msg_events = initiator.node.get_and_clear_pending_msg_events();
+ assert_eq!(msg_events.len(), 2, "{msg_events:?}");
+ check_added_monitors(initiator, 1);
+ if let MessageSendEvent::SendTxAbort { msg, .. } = &msg_events[0] {
+ acceptor.node.handle_tx_abort(node_id_initiator, msg);
+ } else {
+ panic!("Unexpected event {:?}", msg_events[0]);
+ };
+ if let MessageSendEvent::UpdateHTLCs { updates, .. } = &msg_events[1] {
+ acceptor.node.handle_update_add_htlc(node_id_initiator, &updates.update_add_htlcs[0]);
+ do_commitment_signed_dance(acceptor, initiator, &updates.commitment_signed, false, false);
+ } else {
+ panic!("Unexpected event {:?}", msg_events[1]);
+ };
}
#[test]
Risk score
Our methodology →Why this scored 44/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.