Uh oh!
There was an error while loading. Please reload this page.
Fix stale pre-splice announcement_signatures during reestablish - #4577
Conversation
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
The ordering is clear:
The flow is correct. The fix properly skips pre-splice announcement_sigs and lets the promotion path generate the correct post-splice ones. I've thoroughly reviewed every part of the diff. Both fixes are logically sound, and the tests cover the regression scenarios well. My prior review was accurate. No issues found. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #4577 +/- ##
==========================================
+ Coverage 87.08% 87.17% +0.08%
==========================================
Files 161 161 Lines 109255 109251 -4 Branches 109255 109251 -4 ==========================================
+ Hits 95147 95235 +88 + Misses 11627 11543 -84 + Partials 2481 2473 -8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ldk-reviews-bot
commented
Apr 27, 2026
🔔 1st Reminder Hey @TheBlueMatt@wpaulino! This PR has been waiting for your review. |
1 similar comment
ldk-reviews-bot
commented
Apr 27, 2026
🔔 1st Reminder Hey @TheBlueMatt@wpaulino! This PR has been waiting for your review. |
| .map(|funding_locked| { | ||
| self.pending_funding() | ||
| .iter() | ||
| .any(|funding| funding.get_funding_txid() == Some(funding_locked.txid)) |
There was a problem hiding this comment.
This should work, but wouldn't the more correct check be against PendingFunding::sent_funding_txid?
97e9bc2 to
d3691faCompare| // path) rather than the pre-splice funding (via the reestablish handler). | ||
| let mut reconnect_args = ReconnectArgs::new(&nodes[0], &nodes[1]); | ||
| reconnect_args.expect_renegotiated_funding_locked_monitor_update = (true, true); | ||
| reconnect_args.send_announcement_sigs = (true, true); |
There was a problem hiding this comment.
This test is kinda lazy, we should actually test that we're sending the right one.
| // an old `announcement_signatures` across a splice handoff. | ||
| let stale_sigs = msgs::AnnouncementSignatures { | ||
| channel_id, | ||
| short_channel_id: pre_splice_scid, |
There was a problem hiding this comment.
This is fine, but is there an actual case where we can hit this that we can test rather than just testing the synthetic message?
There was a problem hiding this comment.
No, because the first commit fixes LDK. Eclair had a bug when trying to maintain backwards compatibility with the older protocol used with Phoenix. The spec says SHOULD send a warning but we were closing the channel. t-bast advised ignoring the message like they do.
When a splice transaction confirms on both sides while peers are disconnected, each peer's `channel_reestablish` carries `my_current_funding_locked` with the splice txid. In the reestablish handler, `get_announcement_sigs` was called before the inferred `splice_locked` was processed and the splice was promoted, so `self.funding` still pointed to the pre-splice scope. If `announcement_sigs_state` was `NotSent`, the generated `announcement_signatures` carried the pre-splice `short_channel_id` and bitcoin key — which the peer (having already promoted via its own inferred `splice_locked`) would verify against the post-splice `UnsignedChannelAnnouncement`, failing the signature check and force-closing. Skip the pre-promotion call when `my_current_funding_locked` matches the splice we've already confirmed — i.e. `pending_splice.sent_funding_txid` is set and equals the peer's locked txid. `maybe_promote_splice_funding` emits correct post-splice signatures after the inferred `splice_locked` is processed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
A peer may transmit `announcement_signatures` signed over a stale `short_channel_id` — most plausibly a retransmission or a peer implementation whose view hasn't caught up to our post-splice promotion. Verifying such sigs against the current `UnsignedChannelAnnouncement` (built from `self.funding`) always fails the hash check, which previously produced a force-close. BOLT lightningdevkit#7 does not require closing in this situation; the mismatch is expected across splice handoffs. Short-circuit with `ChannelError::Ignore` when `msg.short_channel_id` doesn't match the current funding's scid, leaving the genuine invalid-signature paths in place for sigs that actually target our current scid. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
d3691fa to
80528b1Compare
Two related issues surfaced during interop testing against Eclair's splicing implementation, both around stale
announcement_signatureswhensplice_lockedis implicit — inferred frommy_current_funding_lockedinchannel_reestablish. Either could force-close an otherwise healthy channel.Send side: the reestablish handler called
get_announcement_sigsbefore processing the inferredsplice_locked, so ifannouncement_sigs_state == NotSentit emitted sigs over the pre-spliceshort_channel_id. The peer, having already promoted, would fail verification and force-close.Receive side: the
announcement_signatureshandler treated a mismatchedshort_channel_idas a signature verification failure and force-closed. BOLT7 does not require closing; a peer can legitimately retransmit pre-splice sigs around promotion.