Skip to content

Persist negotiated splice candidates on reload#4653

Open
wpaulino wants to merge 1 commit into
lightningdevkit:mainfrom
wpaulino:pending-splice-persist
Open

Persist negotiated splice candidates on reload#4653
wpaulino wants to merge 1 commit into
lightningdevkit:mainfrom
wpaulino:pending-splice-persist

Conversation

@wpaulino
Copy link
Copy Markdown
Contributor

@wpaulino wpaulino commented Jun 1, 2026

Prior to supporting RBF, we would avoid persisting FundedChannel::pending_splice when there was a pending funding negotiation that could not be resumed on channel reestablishment. With the addition of RBF support, this would cause our previously negotiated splices (but still pending) to be dropped unintentionally.

Fixes the following payloads from #4636:

21ff19ffa2ffa200192222c21821b5b1ff5db1
21ff19ffa2ffa200192222c2193b00261ba3a3201b201821b5b1ff5db1
21ff19ffa2ffa200192222c2193b00201b20b11b203a1821b5b1ff5db1
21ff19ffa2ffa200192222c2191821b5b1ff5db1
0873a1ffa1101810b8ff
21ffffa219ffa2001922221821b5b1ff5db1
0873a1ffa1101810b6ff

@wpaulino wpaulino added this to the 0.3 milestone Jun 1, 2026
@wpaulino wpaulino requested a review from jkczyz June 1, 2026 22:12
@wpaulino wpaulino self-assigned this Jun 1, 2026
@ldk-reviews-bot
Copy link
Copy Markdown

ldk-reviews-bot commented Jun 1, 2026

👋 Thanks for assigning @jkczyz 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.

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

ldk-claude-review-bot commented Jun 1, 2026

I've thoroughly reviewed the entire PR diff, tracing through the macro expansion for write_tlv_fields!, the PendingFundingWriteable custom write logic, the manual Readable impl, the persistence filter logic in FundedChannel::write(), and the new test.

Key things verified:

  • Wire compatibility: write_tlv_fields! adds & before each field, so contributions[..contributions_len] becomes &[FundingContribution], which works with WithoutLength via the AsWriteableSlice for &[T] impl — producing identical bytes to the previous optional_vec encoding on Vec<T>.
  • Read path equivalence: The manual impl Readable for PendingFunding using _decode_and_build! is structurally identical to what impl_ser_tlv_based! generated (same macro, same field list).
  • Contributions trimming: saturating_sub(1) only fires when reset_funding_negotiation && funding_negotiation.is_some(), correctly dropping only the in-progress RBF round's contribution while preserving prior rounds.
  • Persistence filter: !reset_funding_negotiation || !self.pending_funding().is_empty() correctly keeps pending_splice when there are negotiated candidates (the core fix), while still dropping it when there's nothing to preserve.
  • Edge cases: When funding_negotiation is None + candidates non-empty → reset_funding_negotiation = false, so full pass-through. When funding_negotiation is None + candidates empty → should_persist = false, nothing written (with debug_assert).

No issues found.

Copy link
Copy Markdown
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment thread lightning/src/ln/channel.rs Outdated
Comment thread lightning/src/ln/channel.rs Outdated
@ldk-reviews-bot
Copy link
Copy Markdown

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

Prior to supporting RBF, we would avoid persisting
`FundedChannel::pending_splice` when there was a pending funding
negotiation that could not be resumed on channel reestablishment. With
the addition of RBF support, this would cause our previously negotiated
splices (but still pending) to be dropped unintentionally.
@wpaulino wpaulino force-pushed the pending-splice-persist branch from 181e264 to f3575c5 Compare June 3, 2026 18:50
@wpaulino wpaulino requested a review from jkczyz June 3, 2026 18:50
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.

4 participants