Skip to content

splice_locked + channel_reestablish bug fixes #4654

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wpaulino wants to merge 3 commits into
base: main
Choose a base branch
from
+329 −27
Open

splice_locked + channel_reestablish bug fixes #4654

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3b3ed67
Send splice_locked for promoted splice on reconnect
wpaulino Jun 1, 2026
dd6fdb0
Send splice_locked before reestablish commitment
wpaulino Jun 1, 2026
629f511
Clear stale monitor pending resends on reestablish
wpaulino Jun 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10714,6 +10714,9 @@ where
let required_revoke = if msg.next_remote_commitment_number == our_commitment_transaction {
// Remote isn't waiting on any RevokeAndACK from us!
// Note that if we need to repeat our ChannelReady we'll do that in the next if block.
// If a stale ChannelManager replayed a completed update, the monitor-pending state may
// still think we owe one; the reestablish proof is authoritative here.
self.context.monitor_pending_revoke_and_ack = false;
None
} else if msg.next_remote_commitment_number + 1 == our_commitment_transaction {
if self.context.channel_state.is_monitor_update_in_progress() {
Expand Down Expand Up @@ -10784,9 +10787,24 @@ where
channel_id: self.context.channel_id,
splice_txid,
})
}).or_else(|| {
// If a splice confirms after we've sent `channel_reestablish` but before we've
// received theirs, we may promote the splice while disconnected and clear
Copy link
Copy Markdown
Contributor

@jkczyz jkczyz Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the "we may promote the splice while disconnected and clear" part. Isn't the fact that we've sent channel_reestablish indicate we are connected? Should this have said, "we may have promoted the splice while disconnected and have cleared"?

Commit message is similarly confusing to me.

Copy link
Copy Markdown
Contributor Author

@wpaulino wpaulino Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disconnected as in the PEER_DISCONNECTED state flag, i.e., the channel not being reestablished yet.

Copy link
Copy Markdown
Contributor

@jkczyz jkczyz Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, ok. Could you rephrase the comment for clarity?

// `pending_splice`. We still need to send `splice_locked` after reestablishing
// unless the promoted txid was already included in our `channel_reestablish`.
let current_funding_txid = self.funding.get_funding_txid()?;
(self.funding.channel_transaction_parameters.splice_parent_funding_txid.is_some()
&& Some(current_funding_txid) != funding_locked_txid_sent_in_reestablish)
.then(|| msgs::SpliceLocked {
channel_id: self.context.channel_id,
splice_txid: current_funding_txid,
})
});
Comment on lines +10790 to 10802
Copy link
Copy Markdown
Collaborator

@ldk-claude-review-bot ldk-claude-review-bot Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This or_else fires whenever the first closure returns None, which happens not only when self.pending_splice is None (the intended case per the comment), but also when self.pending_splice is Some and sent_funding_txid was filtered out because it matched funding_locked_txid_sent_in_reestablish.

In a sequential-splice scenario (first splice promoted, second splice negotiated and confirmed locally, then reconnect), this would send splice_locked for the previously promoted splice's txid. If the peer has already promoted that splice and has a pending_splice for the second splice, their splice_locked handler would check negotiated_candidates for the old txid, not find it, and force-close the channel with "unknown splice funding txid".

Consider adding self.pending_splice.is_none() to the guard to match the comment's intent—this branch should only handle the case where pending_splice was cleared due to promotion:

Suggested change
}).or_else(|| {
// If a splice confirms after we've sent `channel_reestablish` but before we've
// received theirs, we may promote the splice while disconnected and clear
// `pending_splice`. We still need to send `splice_locked` after reestablishing
// unless the promoted txid was already included in our `channel_reestablish`.
let current_funding_txid = self.funding.get_funding_txid()?;
(self.funding.channel_transaction_parameters.splice_parent_funding_txid.is_some()
&& Some(current_funding_txid) != funding_locked_txid_sent_in_reestablish)
.then(|| msgs::SpliceLocked {
channel_id: self.context.channel_id,
splice_txid: current_funding_txid,
})
});
}).or_else(|| {
// If a splice confirms after we've sent `channel_reestablish` but before we've
// received theirs, we may promote the splice while disconnected and clear
// `pending_splice`. We still need to send `splice_locked` after reestablishing
// unless the promoted txid was already included in our `channel_reestablish`.
let current_funding_txid = self.funding.get_funding_txid()?;
(self.pending_splice.is_none()
&& self.funding.channel_transaction_parameters.splice_parent_funding_txid.is_some()
&& Some(current_funding_txid) != funding_locked_txid_sent_in_reestablish)
.then(|| msgs::SpliceLocked {
channel_id: self.context.channel_id,
splice_txid: current_funding_txid,
})


if msg.next_local_commitment_number == next_counterparty_commitment_number {
// Likewise, don't let a stale monitor-pending resend survive when they tell us
// they've already received our latest commitment.
self.context.monitor_pending_commitment_signed = false;
if required_revoke.is_some() || self.context.signer_pending_revoke_and_ack {
log_debug!(logger, "Reconnected with only lost outbound RAA");
} else {
Expand Down
52 changes: 26 additions & 26 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11052,6 +11052,32 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
}
}


if let Some(funding_tx_signed) = funding_tx_signed.as_ref() {
// These [`FundingTxSigned`] fields are only expected as a result of calling
// [`ChannelManager::funding_transaction_signed`].
debug_assert!(funding_tx_signed.commitment_signed.is_none());
debug_assert!(funding_tx_signed.counterparty_initial_commitment_signed_result.is_none());
}
if let Some(msg) = funding_tx_signed.as_mut().and_then(|v| v.tx_signatures.take()) {
pending_msg_events.push(MessageSendEvent::SendTxSignatures {
node_id: counterparty_node_id,
msg,
});
}
if let Some(msg) = tx_abort {
pending_msg_events.push(MessageSendEvent::SendTxAbort {
node_id: counterparty_node_id,
msg,
});
}
if let Some(msg) = funding_tx_signed.as_mut().and_then(|v| v.splice_locked.take()) {
pending_msg_events.push(MessageSendEvent::SendSpliceLocked {
node_id: counterparty_node_id,
msg,
});
}

macro_rules! handle_cs { () => {
if let Some(update) = commitment_update {
pending_msg_events.push(MessageSendEvent::UpdateHTLCs {
Expand Down Expand Up @@ -11080,25 +11106,6 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
},
}

if let Some(funding_tx_signed) = funding_tx_signed.as_ref() {
// These [`FundingTxSigned`] fields are only expected as a result of calling
// [`ChannelManager::funding_transaction_signed`].
debug_assert!(funding_tx_signed.commitment_signed.is_none());
debug_assert!(funding_tx_signed.counterparty_initial_commitment_signed_result.is_none());
}
if let Some(msg) = funding_tx_signed.as_mut().and_then(|v| v.tx_signatures.take()) {
pending_msg_events.push(MessageSendEvent::SendTxSignatures {
node_id: counterparty_node_id,
msg,
});
}
if let Some(msg) = tx_abort {
pending_msg_events.push(MessageSendEvent::SendTxAbort {
node_id: counterparty_node_id,
msg,
});
}

if let ChannelReadyOrder::SignaturesFirst = channel_ready_order {
if let Some(msg) = channel_ready {
self.send_channel_ready(pending_msg_events, channel, msg);
Expand All @@ -11111,13 +11118,6 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
});
}
}

if let Some(msg) = funding_tx_signed.as_mut().and_then(|v| v.splice_locked.take()) {
pending_msg_events.push(MessageSendEvent::SendSpliceLocked {
node_id: counterparty_node_id,
msg,
});
}
} else if let Some(msg) = channel_ready {
self.send_channel_ready(pending_msg_events, channel, msg);
}
Expand Down
2 changes: 1 addition & 1 deletion lightning/src/ln/functional_test_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1023,7 +1023,7 @@ pub fn get_updates_and_revoke<CM: AChannelManager, H: NodeHolder<CM = CM>>(
macro_rules! get_event_msg {
($node: expr, $event_type: path, $node_id: expr) => {{
let events = $node.node.get_and_clear_pending_msg_events();
assert_eq!(events.len(), 1);
assert_eq!(events.len(), 1, "{events:?}");
match events[0] {
$event_type { ref node_id, ref msg } => {
assert_eq!(*node_id, $node_id);
Expand Down
Loading
Loading