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.
|
I've re-verified the current HEAD against my prior thorough review. The serialization redesign (odd TLVs 9/11/13, even gate 14), the No new issues found. Cross-cutting item carried from 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 |
| .map(|pending_splice| pending_splice.details(&self.context, best_block_height)) | ||
| // A contribution committed via `funding_contributed` sits in `quiescent_action` until | ||
| // quiescence is reached and it becomes a negotiation; surface it as the queued contribution. | ||
| let queued_contribution = match &self.quiescent_action { |
There was a problem hiding this comment.
Haven't reviewed the full PR yet. Can do that if this isn't already merged tmrw. But I think this is what the fuzzer needs.
jkczyz
force-pushed
the
2026-06-splice-details-fable
branch
2 times, most recently
from
8e8a3ef to
0b47b8b
Compare
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
0b47b8b to
6bec475
Compare
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
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!
| .then(|| self.pending_funding.negotiation_contribution.as_ref()) | ||
| .flatten(); | ||
| let candidates = &self.pending_funding.negotiated_candidates; | ||
| debug_assert!( |
There was a problem hiding this comment.
This assert is repeated below. As it is an important one, it might be worth extracting it in a helper.
| // At most one candidate can confirm, as they all double-spend the same input. | ||
| let confirmed_candidate = self.negotiated_candidates.iter().find_map(|candidate| { | ||
| let confirmations = candidate.funding.get_funding_tx_confirmations(best_block_height); | ||
| (confirmations > 0).then(|| { |
There was a problem hiding this comment.
Does this work with zero-conf?
| pub current_dust_exposure_msat: Option<u64>, | ||
| /// Details of any pending splice attempts on this channel. | ||
| /// | ||
| /// This includes a contribution we have committed but not yet begun negotiating, any splice |
There was a problem hiding this comment.
This repeats the comment on SpliceDetails. Maybe keep it short here.
| /// [`ChannelManager::funding_contributed`] but which has not yet begun negotiating, as it is | ||
| /// awaiting quiescence with the counterparty, if any. | ||
| /// | ||
| /// Whether we end up the initiator of the resulting negotiation is not settled until quiescence |
There was a problem hiding this comment.
This might be a bit too much explanation. Just that it's the queued contrib i probably enough?
| pub received_splice_locked_txid: Option<Txid>, | ||
| } | ||
|
|
||
| impl_ser_tlv_based!(SpliceDetails, { |
There was a problem hiding this comment.
Reading this I was wondering what the use case is for serializing this snapshot. ChannelDetails is serializable, but not sure if I understand why.
| assert!(result.is_err(), "LDK 0.2 should fail to decode a ShortChannelId variant"); | ||
| } | ||
|
|
||
| fn downgrade_setup_single_splice() -> (Vec<u8>, Vec<u8>, Vec<u8>, Vec<u8>) { |
There was a problem hiding this comment.
I'd move these tests into commit 1 which contains the code under test.
| //! LDK. | ||
|
|
||
| use lightning_0_2::commitment_signed_dance as commitment_signed_dance_0_2; | ||
| use lightning_0_2::events::bump_transaction::sync::WalletSourceSync as WalletSourceSync_0_2; |
| (node_0_ser, node_1_ser, mon_0_ser, mon_1_ser) | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
One missing case came up: a first splice serialized while funding_negotiation is AwaitingSignatures: no negotiated candidate yet, but negotiation_contribution is present. The current tests cover negotiated candidates, but not the TLV 13 in-flight contribution path.
| "pending_splice should have last_funding_feerate or funding_negotiation", | ||
| ); | ||
| let min_rbf_feerate = prev_feerate.map(min_rbf_feerate); | ||
| let prev_feerate = match prev_feerate { |
There was a problem hiding this comment.
I am not sure if this commit is substantial enough. Perhaps just squash it into commit 1 too.
| * A channel with a pending splice negotiated before upgrading from a prior LDK | ||
| version (e.g. 0.2) cannot be RBF'ed: `ChannelManager::splice_channel` | ||
| now returns an `APIError::APIMisuseError`, as the prior feerate and our | ||
| contribution needed to derive the RBF feerate floor are not persisted by |
There was a problem hiding this comment.
Is our contribution needed to derive the feerate floor? I think more general it must not be missing to do RBF
|
🔔 2nd Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
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.