Exit quiescence when splice_init and tx_init_rbf are rejected

[jkczyz]

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]

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

[codecov]

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.

[ldk-claude-review-bot]

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_err → exit_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 }
	}

[jkczyz]

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]

I've completed a thorough review of the entire PR diff, examining every file and hunk for bugs, security issues, and logic errors.

Verification performed:

1. quiescent_negotiation_err debug_assert safety: validate_splice_init (line 12697) and validate_tx_init_rbf (line 12967) both check is_quiescent() first, returning WarnAndDisconnect for non-quiescent channels. Since these are called before resolve_queued_contribution and other Abort-producing paths, the debug_assert!(is_quiescent()) at line 14254 can never fire on a non-quiescent channel. My prior review concern is fully resolved by the reordering.

2. Reordering safety: resolve_queued_contribution takes &self (line 12822), so it's side-effect-free and reordering it after validation is safe.

3. handle_interactive_tx_msg_err extraction: Faithful extraction of the inline code from internal_tx_msg, with identical semantics. Lock ordering (acquiring pending_events while peer_state is held) matches existing patterns in the codebase (e.g., internal_tx_abort at line 12191).

4. No Close errors on these paths: All error returns from validate_splice_init, validate_tx_init_rbf, resolve_queued_contribution, and validate_splice_contributions produce only WarnAndDisconnect or Abort — never Close. So from_chan_no_close is used correctly.

5. validate_splice_contributions migration: Moved from validate_splice_init/validate_tx_init_rbf to their callers with identical parameters and error wrapping (WarnAndDisconnect).

6. exit_quiescence visibility: Correctly made non-test-only since quiescent_negotiation_err needs it in production.

7. exited_quiescence flow: Properly triggers check_free_peer_holding_cells via handle_error (line 4613-4626), matching the existing tx_abort handling pattern.

8. Tests: Cover the key scenarios — quiescence exit on rejection, STFU re-proposal when pending action exists, holding cell freeing, and misbehaving peer detection.

No issues found.

[jkczyz]

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]

Rebased

[wpaulino]

Hm I'm not sure we want to use this since the debug assertions there are reachable, but the mark_response_received call makes me realize we may need to do this elsewhere to avoid an unintentional disconnect (maybe write a test to confirm?).

[jkczyz]

Hm I'm not sure we want to use this since the debug assertions there are reachable,

The fixup commit makes sure that we don't return ChannelError::Abort until we know we are quiescent, so I think those asserts are unreachable from here?

but the mark_response_received call makes me realize we may need to do this elsewhere to avoid an unintentional disconnect (maybe write a test to confirm?).

Like when processing splice_init, splice_ack, tx_init_rbf, tx_ack_rbf, or interactive-tx messages?

One thing I noticed when exploring this as since we don't have a FundedChannel for v2 establishment, we use UNFUNDED_CHANNEL_AGE_LIMIT_TICKS to timeout instead. So updating sent_message_awaiting_response on the ChannelContext has no effect there. We'd effectively have two different mechanisms for timing out. Are we ok with that? Or should we try to use the UNFUNDED_CHANNEL_AGE_LIMIT_TICKS for splice funding, too?

[wpaulino]

We should include a payment such that the outbound HTLC gets sent out after the abort as a result of freeing the holding cell.