Add pending SpliceDetails to ChannelDetails#4687
Conversation
|
👋 Thanks for assigning @joostjager as a reviewer! |
| // Data written before the in-flight round's contribution was stored separately kept it | ||
| // as the last entry while a negotiation was pending. | ||
| if negotiation_contribution.is_none() && contributions.len() > fundings.len() { | ||
| negotiation_contribution = contributions.pop(); | ||
| } | ||
| // An in-flight contribution is only meaningful while its negotiation round is alive; | ||
| // rounds not surviving serialization round trips have their events handled separately. | ||
| if funding_negotiation.is_none() { | ||
| negotiation_contribution = None; | ||
| } |
There was a problem hiding this comment.
This heuristic for recovering the in-flight contribution from LDK 0.2 data is insufficient when there are leading counterparty-only (None) candidates.
In the old format the in-flight (AwaitingSignatures) round's contribution was appended as the last entry of contributions while a negotiation was pending, but it is not in negotiated_candidates yet. Since contributions form a suffix, with K contributed prior candidates and M total prior candidates (M >= K), the old contributions length is K + 1 (the +1 being the in-flight). This heuristic only pops when K + 1 > M, i.e. only when K == M (no leading None candidates).
When M > K (at least one leading counterparty-only candidate), K + 1 <= M, so the pop is skipped and:
negotiation_contributionstaysNone(the in-flight contribution is lost), andcontrib_offset = M - (K+1)mis-assigns: one prior candidate atcontrib_offsetwrongly receives a contribution and the in-flight contribution is consumed into the candidate list.
Concrete minimal case from old 0.2 data: candidates=[None] (counterparty-only splice) followed by a we-contribute RBF round at AwaitingSignatures → old bytes are fundings=[f0], contributions=[c]. Here 1 > 1 is false, so this reads back as candidate[0].contribution = Some(c) and negotiation_contribution = None, both incorrect (should be candidate[0].contribution = None, negotiation_contribution = Some(c)). This is exactly the first-contribution-on-RBF flow exercised by the new test, and would corrupt contribution tracking / splice_funding_failed and later trip the suffix debug_assert! in splice_funding_negotiated.
The presence of an in-flight contribution in old data can't be detected by length alone here; it needs to be tied to funding_negotiation.is_some() together with the suffix structure.
There was a problem hiding this comment.
I don't think this is an issue, 0.2 doesn't support RBF so it can't have multiple candidates/contributions.
|
Memory is current. No update needed — the design matches my notes and HEAD is consistent. No issues found. The serialization redesign resolves both of my prior inline findings (the even-field-12 in-flight contribution and the length-based recovery heuristic). The contribution now lives in odd TLV 13 (skippable by 0.2), TLV 11 is authoritative, and the legacy fallback maps every TLV-3 funding to
Cross-cutting carry-over (already raised in the prior pass, not re-posted inline): the |
|
🔔 1st Reminder Hey @wpaulino! This PR has been waiting for your review. |
| // Data written before the in-flight round's contribution was stored separately kept it | ||
| // as the last entry while a negotiation was pending. | ||
| if negotiation_contribution.is_none() && contributions.len() > fundings.len() { | ||
| negotiation_contribution = contributions.pop(); | ||
| } | ||
| // An in-flight contribution is only meaningful while its negotiation round is alive; | ||
| // rounds not surviving serialization round trips have their events handled separately. | ||
| if funding_negotiation.is_none() { | ||
| negotiation_contribution = None; | ||
| } |
There was a problem hiding this comment.
I don't think this is an issue, 0.2 doesn't support RBF so it can't have multiple candidates/contributions.
jkczyz
force-pushed
the
2026-06-splice-details-fable
branch
from
30fdc02 to
3f954c5
Compare
|
🔔 1st Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
| } | ||
|
|
||
| #[test] | ||
| fn upgrade_single_splice_from_0_2() { |
There was a problem hiding this comment.
Let's expand this test to also attempt an RBF after upgrading?
There was a problem hiding this comment.
Hmm, seems like we won't be able to RBF as we can't determine if we've contributed. At least we can't without losing our prior contributions. I added a guard to prevent this, which was previously a debug_assert on the last funding feerate.
| } | ||
|
|
||
| #[test] | ||
| fn downgrade_single_splice_loads_on_0_2() { |
There was a problem hiding this comment.
Can we still RBF if we negotiate the splice on 0.3, downgrade to 0.2, and upgrade back? I assume no since we're missing the contribution.
There was a problem hiding this comment.
Yeah, same as upgrading from 0.2. See other comment.
| /// Note that a negotiation which has not yet reached | ||
| /// [`SpliceNegotiationStatus::AwaitingSignatures`] does not survive a restart, so this only | ||
| /// reflects in-memory negotiation state. | ||
| pub negotiation: Option<SpliceNegotiationDetails>, |
There was a problem hiding this comment.
Would it be possible to also surface the state after funding_contributed but before actual splice negotiation? This might be useful to tighten fuzz invariants.
Suggestion from #4699 (comment)
There was a problem hiding this comment.
Yeah, good idea. If they called funding_contributed successfully, it should show up here even if we haven't reached quiescence.
There was a problem hiding this comment.
Great, will then tighten the invariant SpliceDetails when this lands
jkczyz
force-pushed
the
2026-06-splice-details-fable
branch
from
22eba7d to
984f4cc
Compare
|
LGTM after squash+rebase |
jkczyz
force-pushed
the
2026-06-splice-details-fable
branch
3 times, most recently
from
0b47b8b to
6bec475
Compare
jkczyz
force-pushed
the
2026-06-splice-details-fable
branch
from
6bec475 to
0c00583
Compare
|
Rebased, squashed, and tidied a few things up: https://github.com/lightningdevkit/rust-lightning/compare/8e8a3efad232aa8410383731a28ccadd1ad2dc9c..0c0058328349e2ccc9249693f8f120dbee3845ac |
There was a problem hiding this comment.
PR title is underselling the whole serialization/migration change, and I think it could have been two PRs. This one is large, maybe too large.
No big findings other than potentially the zero-conf flow. Will go and try out SpliceDetails with the fuzzer!
|
🔔 2nd Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
|
🔔 1st Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
PendingFunding tracked our splice contributions in a compact list implicitly aligned to the tail of the negotiated candidates, with the in-flight negotiation round's contribution as the implicit last entry. Every consumer had to re-derive this positional relationship, which is easy to get wrong -- e.g., attributing an in-flight round's contribution to a completed counterparty-only candidate. Instead, store each candidate's contribution with the candidate itself and give the in-flight round's contribution its own field, making such misattribution unrepresentable. The contributions still form a suffix of the candidates -- once a round includes our contribution, every subsequent round carries it forward (possibly feerate-adjusted) so the splice intention is never lost -- which is now asserted when a round completes. Serialize this so a single (non-RBF) pending splice stays loadable by LDK 0.2 while RBF is refused loudly. 0.2 predates per-candidate contributions, the in-flight contribution, and the last-negotiated feerate, so writing any of them in an even (required) TLV would make 0.2 refuse even a single splice it can otherwise operate. The legacy TLV 3 therefore carries only the first candidate's funding -- the single-splice view 0.2 reads -- while the full candidate list, the in-flight contribution, and the feerate go in odd TLVs that 0.2 skips. An even gate TLV is written only when there is more than one negotiation round (RBF), so 0.2 loads single splices and refuses RBF, which it cannot operate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jkczyz
force-pushed
the
2026-06-splice-details-fable
branch
from
0c00583 to
a9861ef
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4687 +/- ##
==========================================
+ Coverage 84.51% 86.97% +2.45%
==========================================
Files 137 161 +24
Lines 77446 111811 +34365
Branches 77446 111811 +34365
==========================================
+ Hits 65456 97248 +31792
- Misses 9949 12054 +2105
- Partials 2041 2509 +468
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
joostjager
left a comment
joostjager
left a comment
There was a problem hiding this comment.
Looks good! No blockers
| // The `splice_locked` we sent always refers to the confirmed candidate, as it is | ||
| // cleared if that candidate is ever unconfirmed by a reorg. | ||
| let splice_locked_sent = self.sent_funding_txid == Some(txid); | ||
| (confirmations > 0 || splice_locked_sent).then(|| ConfirmedSpliceCandidate { |
There was a problem hiding this comment.
Bit unusual shape perhaps
There was a problem hiding this comment.
Used an explicit return None instead.
| let (funding_tx, channel_id) = | ||
| open_zero_conf_channel_with_value(&nodes[0], &nodes[1], None, initial_channel_value_sat, 0); | ||
| mine_transaction(&nodes[0], &funding_tx); | ||
| mine_transaction(&nodes[1], &funding_tx); |
There was a problem hiding this comment.
Isn't the sharper version of this test also not mining the original finding?
There was a problem hiding this comment.
Yeah, just needed to unroll the helpers. Claude wanted to be lazy.
| let chan_id_bytes; | ||
| let (v3_mgr, v3_mon); | ||
| { | ||
| let chanmon_cfgs = create_chanmon_cfgs(2); |
There was a problem hiding this comment.
Is this downgrade_setup_single_splice again?
There was a problem hiding this comment.
Looks like the only real difference is needing the channel id bytes for use later. Updated the helper to return it.
A channel may have splice attempts in progress: a contribution we have committed but not yet begun negotiating, one under negotiation with the counterparty, and any negotiated transactions (the original splice and any RBF replacements) waiting on confirmations. This state was only observable through events and the broadcaster's TransactionType::InteractiveFunding, neither of which can be queried on demand. Add an optional splice_details field to ChannelDetails exposing: the negotiation status and our contribution to it; a contribution queued ahead of negotiation; the negotiated candidates (txid, post-splice channel value, and our contribution); the single candidate that has confirmed, with its confirmation progress and whether we have sent splice_locked for it; and the txid of any splice_locked received from the counterparty. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add tests exercising the 0.2/current wire boundary for pending splices: - A current node with a single pending splice (whether or not we contributed to it) is loadable by LDK 0.2. - A current node with a splice under RBF is refused by 0.2 via the even RBF-gate TLV. - A single pending splice written by 0.2 is read by current with no contribution recorded, since 0.2 never tracked one. The downgrade reload configs enable anchors so 0.2 accepts the current channel type rather than refusing it before the splice state is reached. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rsion A pending splice negotiated before an upgrade from a prior LDK version (e.g. 0.2) returns without its feerate or our contribution: 0.2 persists neither and drops the odd TLVs that carry them. splice_channel derives the RBF feerate floor from the prior splice's feerate and assumed it was always present via a debug assertion, so attempting to splice such a channel tripped that assertion. Return a clean error instead, refusing to splice a channel whose pending splice lacks the metadata to reconstruct the prior request. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jkczyz
force-pushed
the
2026-06-splice-details-fable
branch
from
a9861ef to
9619e39
Compare
5 participants
Wallets like LDK Node need to show a channel's pending splice state (e.g., when displaying channel details), but it is currently only observable by tracking events or the broadcaster's
TransactionType::InteractiveFunding, neither of which can be queried on demand.This adds an optional
splice_detailsfield toChannelDetailsexposing that state: any splice/RBF round under negotiation (status, feerate, our contribution, txid and post-splice value once known) and any negotiated candidates awaiting confirmations, along with thesplice_lockedtxids exchanged.The first commit refactors
PendingFundingto store each round's contribution with its negotiated candidate instead of in a tail-aligned parallel list, which every consumer had to realign (and which this API initially got wrong). The on-disk format is unchanged and remains readable by LDK 0.2.