Skip to content

Exit quiescence when splice_init and tx_init_rbf are rejected#4495

Open
jkczyz wants to merge 4 commits intolightningdevkit:mainfrom
jkczyz:2026-03-splicing-rbf-follow-ups
Open

Exit quiescence when splice_init and tx_init_rbf are rejected#4495
jkczyz wants to merge 4 commits intolightningdevkit:mainfrom
jkczyz:2026-03-splicing-rbf-follow-ups

Conversation

@jkczyz
Copy link
Copy Markdown
Contributor

@jkczyz jkczyz commented Mar 18, 2026

Summary

  • Fix quiescence not being exited when tx_init_rbf or splice_init is rejected with Abort (e.g., insufficient RBF feerate, negotiation in progress), which left the channel stuck in a quiescent state
  • Refactor both handlers to return InteractiveTxMsgError, reusing the same pattern already used by the interactive TX message handlers, with a shared handle_interactive_tx_msg_err helper in channelmanager

Test plan

  • Existing test_splice_rbf_insufficient_feerate updated to verify quiescence is properly exited after tx_abort
  • Existing test_splice_feerate_too_high updated to verify quiescence is properly exited after splice_init rejection
  • All splicing tests pass

🤖 Generated with Claude Code

Based on #4494.

@ldk-reviews-bot
Copy link
Copy Markdown

ldk-reviews-bot commented Mar 18, 2026

I've assigned @wpaulino 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.

@jkczyz jkczyz changed the title Exit quiescence when splice_init and tx_init_rbf are rejected Exit quiescence when splice_init and tx_init_rbf are rejected Mar 18, 2026
@jkczyz jkczyz force-pushed the 2026-03-splicing-rbf-follow-ups branch from a5d670c to 57aad34 Compare March 18, 2026 18:57
@jkczyz jkczyz self-assigned this Mar 19, 2026
@jkczyz jkczyz mentioned this pull request Mar 19, 2026
36 tasks
@jkczyz jkczyz force-pushed the 2026-03-splicing-rbf-follow-ups branch 3 times, most recently from 8cdc480 to 959b553 Compare March 31, 2026 00:16
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 31, 2026

Codecov Report

❌ Patch coverage is 61.79775% with 34 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.22%. Comparing base (8d00139) to head (959b553).
⚠️ Report is 111 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/ln/channelmanager.rs 42.85% 30 Missing and 2 partials ⚠️
lightning/src/ln/channel.rs 92.30% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4495      +/-   ##
==========================================
- Coverage   86.24%   86.22%   -0.02%     
==========================================
  Files         160      161       +1     
  Lines      107909   108651     +742     
  Branches   107909   108651     +742     
==========================================
+ Hits        93061    93681     +620     
- Misses      12212    12339     +127     
+ Partials     2636     2631       -5     
Flag Coverage Δ
tests 86.22% <61.79%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jkczyz jkczyz force-pushed the 2026-03-splicing-rbf-follow-ups branch from 959b553 to 69a1a1c Compare March 31, 2026 22:28
@jkczyz jkczyz marked this pull request as ready for review April 2, 2026 17:03
@ldk-reviews-bot ldk-reviews-bot requested a review from wpaulino April 2, 2026 17:04
Comment on lines +13970 to +13974
fn quiescent_negotiation_err(&mut self, err: ChannelError) -> InteractiveTxMsgError {
let exited_quiescence =
if matches!(err, ChannelError::Abort(_)) { self.exit_quiescence() } else { false };
InteractiveTxMsgError { err, splice_funding_failed: None, exited_quiescence }
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In splice_init, resolve_queued_contribution (which can return Abort(FeeRateTooHigh)) is called before validate_splice_init checks is_quiescent(). If a misbehaving peer sends splice_init during the STFU handshake (before quiescence is established), and the holder has a queued contribution with a low max_feerate, the Abort error would flow into quiescent_negotiation_errexit_quiescence(), which has debug_assert!(!is_local_stfu_sent()) / debug_assert!(!is_remote_stfu_sent()).

Those asserts would fire because the STFU flags are still set (handshake incomplete). In release builds, exit_quiescence would harmlessly return false, but in debug/test builds this would panic.

Same issue applies to tx_init_rbf at line 12762 (and line 12780).

Consider guarding with an is_quiescent() check before calling exit_quiescence:

Suggested change
fn quiescent_negotiation_err(&mut self, err: ChannelError) -> InteractiveTxMsgError {
let exited_quiescence =
if matches!(err, ChannelError::Abort(_)) { self.exit_quiescence() } else { false };
InteractiveTxMsgError { err, splice_funding_failed: None, exited_quiescence }
}
fn quiescent_negotiation_err(&mut self, err: ChannelError) -> InteractiveTxMsgError {
let exited_quiescence = if matches!(err, ChannelError::Abort(_))
&& self.context.channel_state.is_quiescent()
{
self.exit_quiescence()
} else {
false
};
InteractiveTxMsgError { err, splice_funding_failed: None, exited_quiescence }
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed, though I took the opportunity to have validate_splice_init and validate_ tx_init_rbf no longer call validate_splice_contributions and FundingScope::for_splice. Rather they are called at those call sites. This let's us check if we are quiescent before checking fee rates when calling resolve_queued_contribution.

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

ldk-claude-review-bot commented Apr 2, 2026

I've completed my thorough review of all changes in this PR. The code is well-structured and correct.

Key verifications performed:

  • validate_splice_init and validate_tx_init_rbf both check is_quiescent() first, returning WarnAndDisconnect (not Abort) for non-quiescent channels — ensuring the debug_assert!(is_quiescent()) in quiescent_negotiation_err can never fire
  • All Abort-producing paths (resolve_queued_contribution, InsufficientRbfFeerate, NegotiationInProgress, FeeRateTooHigh) are only reachable after quiescence is confirmed
  • handle_interactive_tx_msg_err is a faithful extraction of the inline code from internal_tx_msg, with identical semantics
  • from_chan_no_close is safe here because no ChannelError::Close can be produced by these paths
  • resolve_queued_contribution is side-effect-free (&self), so the reordering (after validation) is safe
  • exit_quiescencemark_response_received + clear_quiescent is the correct cleanup
  • The exited_quiescence flag correctly triggers check_free_peer_holding_cells in handle_error
  • validate_splice_contributions removal from validate_tx_init_rbf / validate_splice_init and re-addition in the callers preserves identical validation logic and parameter values
  • Tests correctly verify quiescence exit, STFU re-proposal, and misbehaving peer handling

No new issues found. My prior review comment (about the debug_assert in quiescent_negotiation_err) is effectively resolved by the latest commit (3c55d3c) that defers resolve_queued_contribution until after validation — all Abort errors now only occur in quiescent state.

Comment on lines 12688 to 12690
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Note that we'll now abort an in-progress RBF if the counterparty misbehaves by sending us tx_init_rbf. The spec isn't really clear on what to do here. There's a similar case in validate_splice_init where we'll disconnect if we already have a pending splice (even if negotiation is still in progress). The spec does indicate this should be done. So maybe both of these are fine?

jkczyz and others added 4 commits April 2, 2026 18:11
When tx_init_rbf is rejected with ChannelError::Abort (e.g.,
insufficient RBF feerate, negotiation in progress, feerate too high),
the error is converted to a tx_abort message but quiescence is never
exited and holding cells are never freed. This leaves the channel stuck
in a quiescent state.

Fix this by intercepting ChannelError::Abort before try_channel_entry!
in internal_tx_init_rbf, calling exit_quiescence on the channel, and
returning the error with exited_quiescence set so that handle_error
frees holding cells. Also make exit_quiescence available in non-test
builds by removing its cfg gate.

Update tests to use the proper RBF initiation flow (with tampered
feerates) so that handle_tx_abort correctly echoes the abort and exits
quiescence, rather than manually crafting tx_init_rbf messages that
leave node 0 without proper negotiation state.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The same bug fixed in the prior commit for tx_init_rbf also exists in
internal_splice_init: when splice_init triggers FeeRateTooHigh in
resolve_queued_contribution, the ChannelError::Abort goes through
try_channel_entry! without exiting quiescence.

Apply the same fix: intercept ChannelError::Abort before
try_channel_entry!, call exit_quiescence, and return the error with
exited_quiescence set.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The prior two commits manually intercepted ChannelError::Abort in the
channelmanager handlers for splice_init and tx_init_rbf to exit
quiescence before returning, since the channel methods didn't signal
this themselves. The interactive TX message handlers already solved this
by returning InteractiveTxMsgError which bundles exited_quiescence into
the error type.

Apply the same pattern: change splice_init and tx_init_rbf to return
InteractiveTxMsgError, adding a quiescent_negotiation_err helper on
FundedChannel that exits quiescence for Abort errors and passes through
other variants unchanged. Extract handle_interactive_tx_msg_err in
channelmanager to deduplicate the error handling across internal_tx_msg,
internal_splice_init, and internal_tx_init_rbf.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
validate_splice_init and validate_tx_init_rbf check preconditions
including quiescence. Move them before resolve_queued_contribution
so that a misbehaving peer sending splice_init or tx_init_rbf before
quiescence is rejected early. This also moves validate_splice_contributions
and FundingScope construction into the callers since they depend on the
resolved contribution.

Have validate_tx_init_rbf return the last candidate's funding pubkeys
so the caller can construct FundingScope without re-accessing
pending_splice.

Add a debug_assert in quiescent_negotiation_err that Abort errors only
occur when the channel is quiescent, since exit_quiescence requires it.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@jkczyz jkczyz force-pushed the 2026-03-splicing-rbf-follow-ups branch from d275551 to 3c55d3c Compare April 2, 2026 23:16
@jkczyz
Copy link
Copy Markdown
Contributor Author

jkczyz commented Apr 2, 2026

Rebased

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

3 participants