Skip to content

splice_locked + channel_reestablish bug fixes#4654

Open
wpaulino wants to merge 3 commits into
lightningdevkit:mainfrom
wpaulino:splice-locked-reestablish-fixes
Open

splice_locked + channel_reestablish bug fixes#4654
wpaulino wants to merge 3 commits into
lightningdevkit:mainfrom
wpaulino:splice-locked-reestablish-fixes

Conversation

@wpaulino
Copy link
Copy Markdown
Contributor

@wpaulino wpaulino commented Jun 1, 2026

This PR features commits to fix three different issues found by the chanmon_consistency target around the interaction of splice_locked and channel_reestablish. It fixes the following payloads from #4363:

2a24b7a4b7a3ffabacb2bc41bcffffbc0facad3cfffffe01080000bcbc
2a24b7a4b7a3ffabbc41bcacb2ffffbc0facada3ffabbc41bcac00bcbc
2a24b7a4b7a3ffabacb2ffbc0facadfcffffffbc28a6bcffffffac1d00
3124b7a4b7a3ffffbcacbcffbc0fadabffbca6bcacadffffffbcbcffbc
2a24b7a4b7a3ffabacb2bcffbcffffbc0facadffffb8ff21bdffbcbcbc
2a24b7a4b7a3ffabacb2bc41bcffffbc0facadff08bcfffe000100bcbc
2a24b7a4b7a3ffabacb2bcffbcffffbc0facadc1a3ffabacb2bcffbcbc
2a24b7a4b7a3ffabacb2bcffbcffffbc0facadffffffabacb2bcffbcbc
3126b7a4b7a3ffaba6adbcffbcffbcbcad0faca6ffbcffffffbcbcbc
3124b7a4b7a3ffff24b85dabffbcadffbc0f1e1118acadffff0cffbc71
b4a6ffacffbcada1ffb8
ffa3ffacffbcadff
b4a6ffacffbcadfff1ff
b4a6ffacffbcadff11bc
b4a6ffacffbcadffbcbc
b4a6ffacffbcadff0cbc
b4a6ffacffbcadffabff
b4a6ffacffbcadabffa4
b4a6ffacffbcadffffb4
a4a6ffacffbcadfff1ff
b4a6ffacffbcadc4ffbc
b4a6ffacffbcadacffbc
b4a6ffacffbcadffacff
55a0ffabb4ffb4ac26ff
55a0ffabffb4ffb4acff
7aa4a6ffabb4ffb4acff
55a0ffabb8ffb4acff

wpaulino added 3 commits June 1, 2026 15:06
When a splice confirms while disconnected after our channel_reestablish
was generated, promotion clears pending_splice before their reestablish
arrives. Fall back to the current promoted splice funding txid so we
still send splice_locked after reestablishing.
If we have pending updates to send to our counterparty on
reestablishment, while also pending a `splice_locked` send, then we must
send our `splice_locked` first as the pending updates are considering
the post-splice-locked state.
A stale ChannelManager can be reloaded after a monitor update has
already completed in a prior runtime and released its post-update
messages to the counterparty. The latest ChannelMonitor is not stale,
but the serialized manager may still contain the old in-flight monitor
state and `monitor_pending_*` resend flags from before the completion
action ran.

This becomes observable when startup monitor-completion background
events are interleaved with splice promotion. On reload, the completed
monitor update is queued as a background event. If a splice confirmation
is processed before that background event fully resumes the channel,
splice promotion can create a new `RenegotiatedFundingLocked` monitor
update. The old completion is then blocked behind the new in-flight
splice update. Once the channel reconnects and the splice update
completes, `monitor_updating_restored` may consume the stale
`monitor_pending_revoke_and_ack` / `monitor_pending_commitment_signed`
flags and release a duplicate `revoke_and_ack` or `commitment_signed`.

The peer's `channel_reestablish` commitment numbers are authoritative
for this case. If `next_remote_commitment_number` says the peer is not
waiting for a `revoke_and_ack`, clear `monitor_pending_revoke_and_ack`.
Likewise, if `next_local_commitment_number` says the peer already has
our latest `commitment_signed`, clear
`monitor_pending_commitment_signed`.
@ldk-reviews-bot
Copy link
Copy Markdown

ldk-reviews-bot commented Jun 1, 2026

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@wpaulino wpaulino requested review from TheBlueMatt and jkczyz June 1, 2026 22:18
@wpaulino wpaulino self-assigned this Jun 1, 2026
@wpaulino wpaulino added this to the 0.3 milestone Jun 1, 2026
Comment on lines +10790 to 10802
}).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,
})
});
Copy link
Copy Markdown
Collaborator

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,
})

@ldk-claude-review-bot
Copy link
Copy Markdown
Collaborator

Review Summary

One issue found:

  • lightning/src/ln/channel.rs:10790-10802 — The or_else fallback for sending splice_locked for a promoted splice can also fire when self.pending_splice is Some (but sent_funding_txid was filtered out). In a sequential-splice scenario this would send splice_locked for an already-promoted splice's txid, which the peer could reject with a force-close. Adding self.pending_splice.is_none() to the guard would prevent this.

The remaining changes look correct:

  • Clearing monitor_pending_revoke_and_ack and monitor_pending_commitment_signed during reestablish when the peer's commitment numbers prove the messages were already delivered is sound. The reestablish proof is authoritative over stale ChannelManager state.
  • Moving splice_locked (and tx_signatures/tx_abort) before commitment updates in handle_channel_resumption is the right ordering — the peer needs to know about the splice promotion before processing commitment updates for the post-splice channel.
  • The two new tests cover the intended scenarios well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants