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.
|
No Edit tool available; I'll skip the memory update and finalize the review. I re-reviewed the current state (commit 1e1ecb7), focusing on changes since my prior pass: the serialization redesign, the No new issues found. Verified in this pass:
The two issues from my earlier passes (even field for the in-flight contribution; the length-based 0.2-recovery heuristic) remain resolved by the redesign. Cross-cutting carry-over (raised previously, not re-posted): 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
|
🔔 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. |
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.96% +2.44%
==========================================
Files 137 161 +24
Lines 77446 111828 +34382
Branches 77446 111828 +34382
==========================================
+ Hits 65456 97252 +31796
- Misses 9949 12065 +2116
- Partials 2041 2511 +470
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
| 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.
jkczyz
force-pushed
the
2026-06-splice-details-fable
branch
from
a9861ef to
9619e39
Compare
| } else { | ||
| panic!("expected FundingTransactionReadyForSigning, got {event:?}"); | ||
| }; | ||
| let partially_signed = nodes[0].wallet_source.sign_tx(unsigned_tx).unwrap(); |
There was a problem hiding this comment.
Using the unconfirmed funding expands the test quite a bit. Seeing that, I am not sure it is worth it for just testing SpliceDetails. Perhaps the helper could be adapted instead. Or go back to the confirmed funding shape. But can also leave as is.
There was a problem hiding this comment.
Added a parameter for this and then tacked on a refactor commit to make the call sites more readable with a SignInteractiveFundingTxArgs builder.
| // 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.
Shouldn't this just err? we have explicit handling for this on the write side and this isn't allowed.
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
9619e39 to
b3f17bb
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>
The signing helper had accumulated four boolean/option parameters beyond the two nodes, so call sites passed opaque positional `false`s and bare `None`s whose meaning was unclear without consulting the signature. Replace the two overloaded functions with a single `sign_interactive_funding_tx` taking a `SignInteractiveFundingTxArgs` builder, mirroring `PassAlongPathArgs`: `new(initiator, acceptor)` defaults to a first-attempt splice on a confirmed channel with no acceptor contribution, and each non-default behavior is opted into by a named method (`zero_conf`, `with_acceptor_contribution`, `replacing`, `with_unconfirmed_prior_funding`). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jkczyz
force-pushed
the
2026-06-splice-details-fable
branch
from
b3f17bb to
1e1ecb7
Compare
jkczyz
left a comment
jkczyz
left a comment
There was a problem hiding this comment.
Went ahead and squashed the fixup
| // 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; |
| } else { | ||
| panic!("expected FundingTransactionReadyForSigning, got {event:?}"); | ||
| }; | ||
| let partially_signed = nodes[0].wallet_source.sign_tx(unsigned_tx).unwrap(); |
There was a problem hiding this comment.
Added a parameter for this and then tacked on a refactor commit to make the call sites more readable with a SignInteractiveFundingTxArgs builder.
|
@jkczyz do you have a diff link with the changes? This is a big PR. Tried to find the diff, but it looks like there might have been a rebase too. That refactor commit might be better in a follow up maybe, at this stage of the PR. |
|
Ok, got the diff. Not a rebase, just confused by the added commit. |
joostjager
left a comment
joostjager
left a comment
There was a problem hiding this comment.
Changes look good to me. I just skimmed the final refactor commit. As mentioned above, I think it is better to leave out at this stage.
|
|
||
| let tx = { | ||
| let mut initiator_txn = initiator.tx_broadcaster.txn_broadcast_with_types(); | ||
| if let Some(prior_funding_txid) = prior_funding_txid { |
There was a problem hiding this comment.
The word prior, isn't that a splice-specific term? Aren't we just talking about the unconf'ed funding tx?
| /// Arguments for [`sign_interactive_funding_tx`]. [`SignInteractiveFundingTxArgs::new`] defaults to a | ||
| /// first-attempt splice on a confirmed channel where only the initiator contributes; augment with the | ||
| /// builder methods as the scenario requires. | ||
| pub struct SignInteractiveFundingTxArgs<'a, 'b, 'c, 'd> { |
There was a problem hiding this comment.
It's generally cleaner to do a refactor commit first, and then actual changes that the PR is about. To avoid rewriting things.
| pub struct SignInteractiveFundingTxArgs<'a, 'b, 'c, 'd> { | ||
| pub initiator: &'a Node<'b, 'c, 'd>, | ||
| pub acceptor: &'a Node<'b, 'c, 'd>, | ||
| pub is_0conf: bool, |
There was a problem hiding this comment.
Make private because there are already methods to set this?
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Sorry it took me a bit to get through this.
| /// | ||
| /// [`ChannelManager::funding_contributed`]: crate::ln::channelmanager::ChannelManager::funding_contributed | ||
| /// [`negotiation`]: Self::negotiation | ||
| pub queued_contribution: Option<FundingContribution>, |
There was a problem hiding this comment.
Why isn't this just a state and this returned in negotiation? Most of the fields in SpliceNegotiationDetails are Options already, its not really all that different.
| pub confirmations_required: u32, | ||
| /// Whether we have sent `splice_locked` for this candidate, i.e., we consider it to have | ||
| /// sufficient confirmations. The `splice_locked` we sent always refers to this confirmed | ||
| /// candidate, so it is tracked here rather than as a separate txid. | ||
| pub splice_locked_sent: bool, |
There was a problem hiding this comment.
Its a bit confusing, and not really explained, that in fact we might consider a splice locked and ready to confirm but the counterparty might not. I also don't really love making explicit reference to the lightning P2P message that we happen to be using - we never really explain what "sending splice_locked" actually means anywhere here.
|
|
||
| // Drain the counterparty's pending `splice_locked` so teardown is clean; completing the lock is | ||
| // exercised elsewhere. | ||
| nodes[1].node.get_and_clear_pending_msg_events(); |
There was a problem hiding this comment.
In general please don't write tests that just fetch messages and ignore them. Its not more LoC to write get_event_msg!(nodes[1], MessageSendEvent::SpliceLocked, node_a_id).
But specifically for this test I don't see any tests that check the state of the splice details set after a 0conf is locked in, so it seems like this really should be extended.
| }) | ||
| } | ||
|
|
||
| fn to_details<SP: SignerProvider>( |
There was a problem hiding this comment.
Might be worth making sure chanmon_consistency calls list_channels (maybe via the check-invariants function) after each step so hit this in as many states as possible.
| } | ||
|
|
||
| #[test] | ||
| #[should_panic(expected = "UnknownRequiredFeature")] |
There was a problem hiding this comment.
nit: might be nicer to do a catch_panic on the line we expect the error on so that its contained.
6 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.