Exit quiescence when `splice_init` and `tx_init_rbf` are rejected by jkczyz · Pull Request #4495 · lightningdevkit/rust-lightning

jkczyz · 2026-03-18T18:48:42Z

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 · 2026-03-18T18:48:47Z

👋 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 · 2026-03-31T01:21:31Z

Codecov Report

❌ Patch coverage is 94.67456% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.14%. Comparing base (2313bd5) to head (d2f422b).
⚠️ Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	89.74%	7 Missing and 1 partial ⚠️
lightning/src/ln/channelmanager.rs	98.90%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4495 +/- ##
==========================================
+ Coverage 87.08% 87.14% +0.06% 
==========================================
Files 161 161 Lines 109255 109245 -10 Branches 109255 109245 -10 ==========================================
+ Hits 95147 95205 +58 + Misses 11627 11569 -58 + Partials 2481 2471 -10

Flag	Coverage Δ
fuzzing-fake-hashes	`31.08% <14.79%> (+0.10%)`	⬆️
fuzzing-real-hashes	`22.59% <3.55%> (-0.06%)`	⬇️
tests	`86.23% <94.67%> (+0.06%)`	⬆️

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 · 2026-04-02T17:12:22Z

+	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 }
+	}


In splice_init, resolve_queued_contribution (which can return Abort(FeeRateTooHigh)) is called beforevalidate_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(&mutself,err:ChannelError) -> InteractiveTxMsgError{
let exited_quiescence =
ifmatches!(err,ChannelError::Abort(_)){self.exit_quiescence()}else{false};
InteractiveTxMsgError{ err,splice_funding_failed:None, exited_quiescence }
}
fn quiescent_negotiation_err(&mutself,err:ChannelError) -> InteractiveTxMsgError{
let exited_quiescence = ifmatches!(err,ChannelError::Abort(_))
&&self.context.channel_state.is_quiescent()
{
self.exit_quiescence()
}else{
false
};
InteractiveTxMsgError{ err,splice_funding_failed:None, exited_quiescence }
}

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 · 2026-04-02T17:12:33Z

After a thorough review of the entire diff, including cross-referencing the existing codebase for correctness of validation ordering, error handling flows, and quiescence state transitions, I found no new issues beyond what was covered in my prior review comments.

Prior comment corrections:

My prior comment on channel.rs:9394 was incorrect — exit_quiescence() at line 9396 is inside the outer if let Some(pending_splice) block but after the inner if/else, so it correctly runs for both the success and debug_assert!(false) branches.
My prior comment on channelmanager.rs:12182 was incorrect — the code at lines 12156-12164 already uses the handle_interactive_tx_msg_err helper.
My prior comment on channel.rs:14284 about quiescent_negotiation_err's debug_assert!(is_quiescent()) is a valid defensive concern, but after verifying the code, both validate_splice_init (line 12894) and validate_tx_init_rbf (line 13064) are called before any Abort-producing code paths, ensuring the assert is safe with the current code.

No issues found.

jkczyz · 2026-04-02T23:09:37Z

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 · 2026-04-02T23:19:28Z

Rebased

ldk-reviews-bot · 2026-04-06T00:00:18Z

🔔 1st Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

TheBlueMatt · 2026-04-06T16:12:08Z

+ debug_assert!(!err.exited_quiescence || matches!(err.err, ChannelError::Abort(_)));
+
+ MsgHandleErrInternal::from_chan_no_close(err.err, channel_id)
+ .with_exited_quiescence(err.exited_quiescence)


This does feel a bit like a leaky abstraction - can we move exited_quiescence into the ChannelError and just call it needs_holding_cell_release? Even moreso because the variable doesn't make sense if we disconnect, close, or send an error.

Ok, I think we can infer it from tx_abort. At least I managed to get Claude to convince me that. Also, renamed the variables to needs_holding_cell_release and include similar inference elsewhere.
Let me know if this is looks good. I can make it a fixup if you prefer. May need a careful look, though.

ldk-reviews-bot · 2026-04-08T00:01:01Z

🔔 2nd Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-11T00:00:33Z

🔔 3rd Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-13T00:01:17Z

🔔 4th Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-15T00:02:23Z

🔔 5th Reminder

Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

wpaulino

Feel free to squash

wpaulino · 2026-04-15T17:14:20Z

+ let their_funding_contribution = match msg.funding_output_contribution {
+ Some(value) => SignedAmount::from_sat(value),
+ None => SignedAmount::ZERO,


I'm a bit confused why this field is even optional in the spec, and why we would allow the counterparty to initiate an RBF when they're not contributing anything. I guess we could have a queued splice with pending counterparty-initiated splice, but the feerate is too high for us, so we help the counterparty confirm their splice, such that we can try ours once it becomes locked?

Yeah, or we just changed our mind and want to back out. Similarly for dual funding.

ldk-reviews-bot · 2026-04-16T22:12:29Z

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-20T00:00:44Z

🔔 2nd Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-22T00:01:34Z

🔔 3rd Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

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>

jkczyz

Rebased and squashed. Some test assertions may need to change once #4514 is merged. Left TODOs.

jkczyz · 2026-04-23T14:22:37Z

+ let their_funding_contribution = match msg.funding_output_contribution {
+ Some(value) => SignedAmount::from_sat(value),
+ None => SignedAmount::ZERO,


Yeah, or we just changed our mind and want to back out. Similarly for dual funding.

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, internal_tx_init_rbf, and internal_tx_complete. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

The `exited_quiescence` field on `MsgHandleErrInternal` and `InteractiveTxMsgError` is a leaky abstraction -- the channelmanager error handling shouldn't know about quiescence, only whether the holding cell needs to be released. Infer this from the presence of a `tx_abort` instead, since exiting quiescence via an error always produces one. Remove `exited_quiescence` from `InteractiveTxMsgError`, `MsgHandleErrInternal`, and the return type of `Channel::tx_abort`, along with the `with_exited_quiescence` builder. For unfunded v2 channels, `tx_abort` may be present without quiescence having been exited, but the holding cell release is a no-op since an unfunded channel won't have any HTLCs. Similarly, the unreachable `debug_assert!(false)` branch in `fail_interactive_tx_negotiation` for funded channels produces a `tx_abort` without exiting quiescence, but the holding cell release is a no-op since the channel is still quiescent. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Several code paths exit quiescence by calling `clear_quiescent()` directly without also clearing the disconnect timer via `mark_response_received()`. This causes the timer to fire after the splice completes or is aborted, spuriously disconnecting the peer. Replace `clear_quiescent()` with `exit_quiescence()` in `on_tx_signatures_exchange`, `reset_pending_splice_state`, and `peer_connected_get_handshake`, which clears both the quiescent state and the disconnect timer. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

ldk-reviews-bot · 2026-04-25T00:00:32Z

🔔 4th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-27T00:01:41Z

🔔 5th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

jkczyz · 2026-03-18T18:48:42Z

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 · 2026-03-18T18:48:47Z

👋 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 · 2026-03-31T01:21:31Z

Codecov Report

❌ Patch coverage is 94.67456% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.14%. Comparing base (2313bd5) to head (d2f422b).
⚠️ Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	89.74%	7 Missing and 1 partial ⚠️
lightning/src/ln/channelmanager.rs	98.90%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4495 +/- ##
==========================================
+ Coverage 87.08% 87.14% +0.06% 
==========================================
Files 161 161 Lines 109255 109245 -10 Branches 109255 109245 -10 ==========================================
+ Hits 95147 95205 +58 + Misses 11627 11569 -58 + Partials 2481 2471 -10

Flag	Coverage Δ
fuzzing-fake-hashes	`31.08% <14.79%> (+0.10%)`	⬆️
fuzzing-real-hashes	`22.59% <3.55%> (-0.06%)`	⬇️
tests	`86.23% <94.67%> (+0.06%)`	⬆️

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 · 2026-04-02T17:12:22Z

+	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 }
+	}


In splice_init, resolve_queued_contribution (which can return Abort(FeeRateTooHigh)) is called beforevalidate_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(&mutself,err:ChannelError) -> InteractiveTxMsgError{
let exited_quiescence =
ifmatches!(err,ChannelError::Abort(_)){self.exit_quiescence()}else{false};
InteractiveTxMsgError{ err,splice_funding_failed:None, exited_quiescence }
}
fn quiescent_negotiation_err(&mutself,err:ChannelError) -> InteractiveTxMsgError{
let exited_quiescence = ifmatches!(err,ChannelError::Abort(_))
&&self.context.channel_state.is_quiescent()
{
self.exit_quiescence()
}else{
false
};
InteractiveTxMsgError{ err,splice_funding_failed:None, exited_quiescence }
}

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 · 2026-04-02T17:12:33Z

After a thorough review of the entire diff, including cross-referencing the existing codebase for correctness of validation ordering, error handling flows, and quiescence state transitions, I found no new issues beyond what was covered in my prior review comments.

Prior comment corrections:

My prior comment on channel.rs:9394 was incorrect — exit_quiescence() at line 9396 is inside the outer if let Some(pending_splice) block but after the inner if/else, so it correctly runs for both the success and debug_assert!(false) branches.
My prior comment on channelmanager.rs:12182 was incorrect — the code at lines 12156-12164 already uses the handle_interactive_tx_msg_err helper.
My prior comment on channel.rs:14284 about quiescent_negotiation_err's debug_assert!(is_quiescent()) is a valid defensive concern, but after verifying the code, both validate_splice_init (line 12894) and validate_tx_init_rbf (line 13064) are called before any Abort-producing code paths, ensuring the assert is safe with the current code.

No issues found.

jkczyz · 2026-04-02T23:09:37Z

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 · 2026-04-02T23:19:28Z

Rebased

ldk-reviews-bot · 2026-04-06T00:00:18Z

🔔 1st Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

TheBlueMatt · 2026-04-06T16:12:08Z

+ debug_assert!(!err.exited_quiescence || matches!(err.err, ChannelError::Abort(_)));
+
+ MsgHandleErrInternal::from_chan_no_close(err.err, channel_id)
+ .with_exited_quiescence(err.exited_quiescence)


This does feel a bit like a leaky abstraction - can we move exited_quiescence into the ChannelError and just call it needs_holding_cell_release? Even moreso because the variable doesn't make sense if we disconnect, close, or send an error.

Ok, I think we can infer it from tx_abort. At least I managed to get Claude to convince me that. Also, renamed the variables to needs_holding_cell_release and include similar inference elsewhere.
Let me know if this is looks good. I can make it a fixup if you prefer. May need a careful look, though.

ldk-reviews-bot · 2026-04-08T00:01:01Z

🔔 2nd Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-11T00:00:33Z

🔔 3rd Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-13T00:01:17Z

🔔 4th Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-15T00:02:23Z

🔔 5th Reminder

Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

wpaulino

Feel free to squash

wpaulino · 2026-04-15T17:14:20Z

+ let their_funding_contribution = match msg.funding_output_contribution {
+ Some(value) => SignedAmount::from_sat(value),
+ None => SignedAmount::ZERO,


I'm a bit confused why this field is even optional in the spec, and why we would allow the counterparty to initiate an RBF when they're not contributing anything. I guess we could have a queued splice with pending counterparty-initiated splice, but the feerate is too high for us, so we help the counterparty confirm their splice, such that we can try ours once it becomes locked?

Yeah, or we just changed our mind and want to back out. Similarly for dual funding.

ldk-reviews-bot · 2026-04-16T22:12:29Z

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-20T00:00:44Z

🔔 2nd Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-22T00:01:34Z

🔔 3rd Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

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>

jkczyz

Rebased and squashed. Some test assertions may need to change once #4514 is merged. Left TODOs.

jkczyz · 2026-04-23T14:22:37Z

+ let their_funding_contribution = match msg.funding_output_contribution {
+ Some(value) => SignedAmount::from_sat(value),
+ None => SignedAmount::ZERO,


Yeah, or we just changed our mind and want to back out. Similarly for dual funding.

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, internal_tx_init_rbf, and internal_tx_complete. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

The `exited_quiescence` field on `MsgHandleErrInternal` and `InteractiveTxMsgError` is a leaky abstraction -- the channelmanager error handling shouldn't know about quiescence, only whether the holding cell needs to be released. Infer this from the presence of a `tx_abort` instead, since exiting quiescence via an error always produces one. Remove `exited_quiescence` from `InteractiveTxMsgError`, `MsgHandleErrInternal`, and the return type of `Channel::tx_abort`, along with the `with_exited_quiescence` builder. For unfunded v2 channels, `tx_abort` may be present without quiescence having been exited, but the holding cell release is a no-op since an unfunded channel won't have any HTLCs. Similarly, the unreachable `debug_assert!(false)` branch in `fail_interactive_tx_negotiation` for funded channels produces a `tx_abort` without exiting quiescence, but the holding cell release is a no-op since the channel is still quiescent. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Several code paths exit quiescence by calling `clear_quiescent()` directly without also clearing the disconnect timer via `mark_response_received()`. This causes the timer to fire after the splice completes or is aborted, spuriously disconnecting the peer. Replace `clear_quiescent()` with `exit_quiescence()` in `on_tx_signatures_exchange`, `reset_pending_splice_state`, and `peer_connected_get_handshake`, which clears both the quiescent state and the disconnect timer. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

ldk-reviews-bot · 2026-04-25T00:00:32Z

🔔 4th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-27T00:01:41Z

🔔 5th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

jkczyz · 2026-03-18T18:48:42Z

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 · 2026-03-18T18:48:47Z

👋 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 · 2026-03-31T01:21:31Z

Codecov Report

❌ Patch coverage is 94.67456% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.14%. Comparing base (2313bd5) to head (d2f422b).
⚠️ Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	89.74%	7 Missing and 1 partial ⚠️
lightning/src/ln/channelmanager.rs	98.90%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4495 +/- ##
==========================================
+ Coverage 87.08% 87.14% +0.06% 
==========================================
Files 161 161 Lines 109255 109245 -10 Branches 109255 109245 -10 ==========================================
+ Hits 95147 95205 +58 + Misses 11627 11569 -58 + Partials 2481 2471 -10

Flag	Coverage Δ
fuzzing-fake-hashes	`31.08% <14.79%> (+0.10%)`	⬆️
fuzzing-real-hashes	`22.59% <3.55%> (-0.06%)`	⬇️
tests	`86.23% <94.67%> (+0.06%)`	⬆️

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 · 2026-04-02T17:12:22Z

+	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 }
+	}


In splice_init, resolve_queued_contribution (which can return Abort(FeeRateTooHigh)) is called beforevalidate_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(&mutself,err:ChannelError) -> InteractiveTxMsgError{
let exited_quiescence =
ifmatches!(err,ChannelError::Abort(_)){self.exit_quiescence()}else{false};
InteractiveTxMsgError{ err,splice_funding_failed:None, exited_quiescence }
}
fn quiescent_negotiation_err(&mutself,err:ChannelError) -> InteractiveTxMsgError{
let exited_quiescence = ifmatches!(err,ChannelError::Abort(_))
&&self.context.channel_state.is_quiescent()
{
self.exit_quiescence()
}else{
false
};
InteractiveTxMsgError{ err,splice_funding_failed:None, exited_quiescence }
}

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 · 2026-04-02T17:12:33Z

After a thorough review of the entire diff, including cross-referencing the existing codebase for correctness of validation ordering, error handling flows, and quiescence state transitions, I found no new issues beyond what was covered in my prior review comments.

Prior comment corrections:

My prior comment on channel.rs:9394 was incorrect — exit_quiescence() at line 9396 is inside the outer if let Some(pending_splice) block but after the inner if/else, so it correctly runs for both the success and debug_assert!(false) branches.
My prior comment on channelmanager.rs:12182 was incorrect — the code at lines 12156-12164 already uses the handle_interactive_tx_msg_err helper.
My prior comment on channel.rs:14284 about quiescent_negotiation_err's debug_assert!(is_quiescent()) is a valid defensive concern, but after verifying the code, both validate_splice_init (line 12894) and validate_tx_init_rbf (line 13064) are called before any Abort-producing code paths, ensuring the assert is safe with the current code.

No issues found.

jkczyz · 2026-04-02T23:09:37Z

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 · 2026-04-02T23:19:28Z

Rebased

ldk-reviews-bot · 2026-04-06T00:00:18Z

🔔 1st Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

TheBlueMatt · 2026-04-06T16:12:08Z

+ debug_assert!(!err.exited_quiescence || matches!(err.err, ChannelError::Abort(_)));
+
+ MsgHandleErrInternal::from_chan_no_close(err.err, channel_id)
+ .with_exited_quiescence(err.exited_quiescence)


This does feel a bit like a leaky abstraction - can we move exited_quiescence into the ChannelError and just call it needs_holding_cell_release? Even moreso because the variable doesn't make sense if we disconnect, close, or send an error.

Ok, I think we can infer it from tx_abort. At least I managed to get Claude to convince me that. Also, renamed the variables to needs_holding_cell_release and include similar inference elsewhere.
Let me know if this is looks good. I can make it a fixup if you prefer. May need a careful look, though.

ldk-reviews-bot · 2026-04-08T00:01:01Z

🔔 2nd Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-11T00:00:33Z

🔔 3rd Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-13T00:01:17Z

🔔 4th Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-15T00:02:23Z

🔔 5th Reminder

Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

wpaulino

Feel free to squash

wpaulino · 2026-04-15T17:14:20Z

+ let their_funding_contribution = match msg.funding_output_contribution {
+ Some(value) => SignedAmount::from_sat(value),
+ None => SignedAmount::ZERO,


I'm a bit confused why this field is even optional in the spec, and why we would allow the counterparty to initiate an RBF when they're not contributing anything. I guess we could have a queued splice with pending counterparty-initiated splice, but the feerate is too high for us, so we help the counterparty confirm their splice, such that we can try ours once it becomes locked?

Yeah, or we just changed our mind and want to back out. Similarly for dual funding.

ldk-reviews-bot · 2026-04-16T22:12:29Z

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-20T00:00:44Z

🔔 2nd Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-22T00:01:34Z

🔔 3rd Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

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>

jkczyz

Rebased and squashed. Some test assertions may need to change once #4514 is merged. Left TODOs.

jkczyz · 2026-04-23T14:22:37Z

+ let their_funding_contribution = match msg.funding_output_contribution {
+ Some(value) => SignedAmount::from_sat(value),
+ None => SignedAmount::ZERO,


Yeah, or we just changed our mind and want to back out. Similarly for dual funding.

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, internal_tx_init_rbf, and internal_tx_complete. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

The `exited_quiescence` field on `MsgHandleErrInternal` and `InteractiveTxMsgError` is a leaky abstraction -- the channelmanager error handling shouldn't know about quiescence, only whether the holding cell needs to be released. Infer this from the presence of a `tx_abort` instead, since exiting quiescence via an error always produces one. Remove `exited_quiescence` from `InteractiveTxMsgError`, `MsgHandleErrInternal`, and the return type of `Channel::tx_abort`, along with the `with_exited_quiescence` builder. For unfunded v2 channels, `tx_abort` may be present without quiescence having been exited, but the holding cell release is a no-op since an unfunded channel won't have any HTLCs. Similarly, the unreachable `debug_assert!(false)` branch in `fail_interactive_tx_negotiation` for funded channels produces a `tx_abort` without exiting quiescence, but the holding cell release is a no-op since the channel is still quiescent. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Several code paths exit quiescence by calling `clear_quiescent()` directly without also clearing the disconnect timer via `mark_response_received()`. This causes the timer to fire after the splice completes or is aborted, spuriously disconnecting the peer. Replace `clear_quiescent()` with `exit_quiescence()` in `on_tx_signatures_exchange`, `reset_pending_splice_state`, and `peer_connected_get_handshake`, which clears both the quiescent state and the disconnect timer. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

ldk-reviews-bot · 2026-04-25T00:00:32Z

🔔 4th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-27T00:01:41Z

🔔 5th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

jkczyz · 2026-03-18T18:48:42Z

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 · 2026-03-18T18:48:47Z

👋 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 · 2026-03-31T01:21:31Z

Codecov Report

❌ Patch coverage is 94.67456% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.14%. Comparing base (2313bd5) to head (d2f422b).
⚠️ Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	89.74%	7 Missing and 1 partial ⚠️
lightning/src/ln/channelmanager.rs	98.90%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4495 +/- ##
==========================================
+ Coverage 87.08% 87.14% +0.06% 
==========================================
Files 161 161 Lines 109255 109245 -10 Branches 109255 109245 -10 ==========================================
+ Hits 95147 95205 +58 + Misses 11627 11569 -58 + Partials 2481 2471 -10

Flag	Coverage Δ
fuzzing-fake-hashes	`31.08% <14.79%> (+0.10%)`	⬆️
fuzzing-real-hashes	`22.59% <3.55%> (-0.06%)`	⬇️
tests	`86.23% <94.67%> (+0.06%)`	⬆️

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 · 2026-04-02T17:12:22Z

+	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 }
+	}


In splice_init, resolve_queued_contribution (which can return Abort(FeeRateTooHigh)) is called beforevalidate_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(&mutself,err:ChannelError) -> InteractiveTxMsgError{
let exited_quiescence =
ifmatches!(err,ChannelError::Abort(_)){self.exit_quiescence()}else{false};
InteractiveTxMsgError{ err,splice_funding_failed:None, exited_quiescence }
}
fn quiescent_negotiation_err(&mutself,err:ChannelError) -> InteractiveTxMsgError{
let exited_quiescence = ifmatches!(err,ChannelError::Abort(_))
&&self.context.channel_state.is_quiescent()
{
self.exit_quiescence()
}else{
false
};
InteractiveTxMsgError{ err,splice_funding_failed:None, exited_quiescence }
}

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 · 2026-04-02T17:12:33Z

After a thorough review of the entire diff, including cross-referencing the existing codebase for correctness of validation ordering, error handling flows, and quiescence state transitions, I found no new issues beyond what was covered in my prior review comments.

Prior comment corrections:

My prior comment on channel.rs:9394 was incorrect — exit_quiescence() at line 9396 is inside the outer if let Some(pending_splice) block but after the inner if/else, so it correctly runs for both the success and debug_assert!(false) branches.
My prior comment on channelmanager.rs:12182 was incorrect — the code at lines 12156-12164 already uses the handle_interactive_tx_msg_err helper.
My prior comment on channel.rs:14284 about quiescent_negotiation_err's debug_assert!(is_quiescent()) is a valid defensive concern, but after verifying the code, both validate_splice_init (line 12894) and validate_tx_init_rbf (line 13064) are called before any Abort-producing code paths, ensuring the assert is safe with the current code.

No issues found.

jkczyz · 2026-04-02T23:09:37Z

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 · 2026-04-02T23:19:28Z

Rebased

ldk-reviews-bot · 2026-04-06T00:00:18Z

🔔 1st Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

TheBlueMatt · 2026-04-06T16:12:08Z

+ debug_assert!(!err.exited_quiescence || matches!(err.err, ChannelError::Abort(_)));
+
+ MsgHandleErrInternal::from_chan_no_close(err.err, channel_id)
+ .with_exited_quiescence(err.exited_quiescence)


This does feel a bit like a leaky abstraction - can we move exited_quiescence into the ChannelError and just call it needs_holding_cell_release? Even moreso because the variable doesn't make sense if we disconnect, close, or send an error.

Ok, I think we can infer it from tx_abort. At least I managed to get Claude to convince me that. Also, renamed the variables to needs_holding_cell_release and include similar inference elsewhere.
Let me know if this is looks good. I can make it a fixup if you prefer. May need a careful look, though.

ldk-reviews-bot · 2026-04-08T00:01:01Z

🔔 2nd Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-11T00:00:33Z

🔔 3rd Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-13T00:01:17Z

🔔 4th Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-15T00:02:23Z

🔔 5th Reminder

Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

wpaulino

Feel free to squash

wpaulino · 2026-04-15T17:14:20Z

+ let their_funding_contribution = match msg.funding_output_contribution {
+ Some(value) => SignedAmount::from_sat(value),
+ None => SignedAmount::ZERO,


I'm a bit confused why this field is even optional in the spec, and why we would allow the counterparty to initiate an RBF when they're not contributing anything. I guess we could have a queued splice with pending counterparty-initiated splice, but the feerate is too high for us, so we help the counterparty confirm their splice, such that we can try ours once it becomes locked?

Yeah, or we just changed our mind and want to back out. Similarly for dual funding.

ldk-reviews-bot · 2026-04-16T22:12:29Z

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-20T00:00:44Z

🔔 2nd Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-22T00:01:34Z

🔔 3rd Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

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>

jkczyz

Rebased and squashed. Some test assertions may need to change once #4514 is merged. Left TODOs.

jkczyz · 2026-04-23T14:22:37Z

+ let their_funding_contribution = match msg.funding_output_contribution {
+ Some(value) => SignedAmount::from_sat(value),
+ None => SignedAmount::ZERO,


Yeah, or we just changed our mind and want to back out. Similarly for dual funding.

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, internal_tx_init_rbf, and internal_tx_complete. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

The `exited_quiescence` field on `MsgHandleErrInternal` and `InteractiveTxMsgError` is a leaky abstraction -- the channelmanager error handling shouldn't know about quiescence, only whether the holding cell needs to be released. Infer this from the presence of a `tx_abort` instead, since exiting quiescence via an error always produces one. Remove `exited_quiescence` from `InteractiveTxMsgError`, `MsgHandleErrInternal`, and the return type of `Channel::tx_abort`, along with the `with_exited_quiescence` builder. For unfunded v2 channels, `tx_abort` may be present without quiescence having been exited, but the holding cell release is a no-op since an unfunded channel won't have any HTLCs. Similarly, the unreachable `debug_assert!(false)` branch in `fail_interactive_tx_negotiation` for funded channels produces a `tx_abort` without exiting quiescence, but the holding cell release is a no-op since the channel is still quiescent. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Several code paths exit quiescence by calling `clear_quiescent()` directly without also clearing the disconnect timer via `mark_response_received()`. This causes the timer to fire after the splice completes or is aborted, spuriously disconnecting the peer. Replace `clear_quiescent()` with `exit_quiescence()` in `on_tx_signatures_exchange`, `reset_pending_splice_state`, and `peer_connected_get_handshake`, which clears both the quiescent state and the disconnect timer. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

ldk-reviews-bot · 2026-04-25T00:00:32Z

🔔 4th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-27T00:01:41Z

🔔 5th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

jkczyz · 2026-03-18T18:48:42Z

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 · 2026-03-18T18:48:47Z

👋 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 · 2026-03-31T01:21:31Z

Codecov Report

❌ Patch coverage is 94.67456% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.14%. Comparing base (2313bd5) to head (d2f422b).
⚠️ Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	89.74%	7 Missing and 1 partial ⚠️
lightning/src/ln/channelmanager.rs	98.90%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4495 +/- ##
==========================================
+ Coverage 87.08% 87.14% +0.06% 
==========================================
Files 161 161 Lines 109255 109245 -10 Branches 109255 109245 -10 ==========================================
+ Hits 95147 95205 +58 + Misses 11627 11569 -58 + Partials 2481 2471 -10

Flag	Coverage Δ
fuzzing-fake-hashes	`31.08% <14.79%> (+0.10%)`	⬆️
fuzzing-real-hashes	`22.59% <3.55%> (-0.06%)`	⬇️
tests	`86.23% <94.67%> (+0.06%)`	⬆️

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 · 2026-04-02T17:12:22Z

+	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 }
+	}


In splice_init, resolve_queued_contribution (which can return Abort(FeeRateTooHigh)) is called beforevalidate_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(&mutself,err:ChannelError) -> InteractiveTxMsgError{
let exited_quiescence =
ifmatches!(err,ChannelError::Abort(_)){self.exit_quiescence()}else{false};
InteractiveTxMsgError{ err,splice_funding_failed:None, exited_quiescence }
}
fn quiescent_negotiation_err(&mutself,err:ChannelError) -> InteractiveTxMsgError{
let exited_quiescence = ifmatches!(err,ChannelError::Abort(_))
&&self.context.channel_state.is_quiescent()
{
self.exit_quiescence()
}else{
false
};
InteractiveTxMsgError{ err,splice_funding_failed:None, exited_quiescence }
}

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 · 2026-04-02T17:12:33Z

After a thorough review of the entire diff, including cross-referencing the existing codebase for correctness of validation ordering, error handling flows, and quiescence state transitions, I found no new issues beyond what was covered in my prior review comments.

Prior comment corrections:

My prior comment on channel.rs:9394 was incorrect — exit_quiescence() at line 9396 is inside the outer if let Some(pending_splice) block but after the inner if/else, so it correctly runs for both the success and debug_assert!(false) branches.
My prior comment on channelmanager.rs:12182 was incorrect — the code at lines 12156-12164 already uses the handle_interactive_tx_msg_err helper.
My prior comment on channel.rs:14284 about quiescent_negotiation_err's debug_assert!(is_quiescent()) is a valid defensive concern, but after verifying the code, both validate_splice_init (line 12894) and validate_tx_init_rbf (line 13064) are called before any Abort-producing code paths, ensuring the assert is safe with the current code.

No issues found.

jkczyz · 2026-04-02T23:09:37Z

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 · 2026-04-02T23:19:28Z

Rebased

ldk-reviews-bot · 2026-04-06T00:00:18Z

🔔 1st Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

TheBlueMatt · 2026-04-06T16:12:08Z

+ debug_assert!(!err.exited_quiescence || matches!(err.err, ChannelError::Abort(_)));
+
+ MsgHandleErrInternal::from_chan_no_close(err.err, channel_id)
+ .with_exited_quiescence(err.exited_quiescence)


This does feel a bit like a leaky abstraction - can we move exited_quiescence into the ChannelError and just call it needs_holding_cell_release? Even moreso because the variable doesn't make sense if we disconnect, close, or send an error.

Ok, I think we can infer it from tx_abort. At least I managed to get Claude to convince me that. Also, renamed the variables to needs_holding_cell_release and include similar inference elsewhere.
Let me know if this is looks good. I can make it a fixup if you prefer. May need a careful look, though.

ldk-reviews-bot · 2026-04-08T00:01:01Z

🔔 2nd Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-11T00:00:33Z

🔔 3rd Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-13T00:01:17Z

🔔 4th Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-15T00:02:23Z

🔔 5th Reminder

Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

wpaulino

Feel free to squash

wpaulino · 2026-04-15T17:14:20Z

+ let their_funding_contribution = match msg.funding_output_contribution {
+ Some(value) => SignedAmount::from_sat(value),
+ None => SignedAmount::ZERO,


I'm a bit confused why this field is even optional in the spec, and why we would allow the counterparty to initiate an RBF when they're not contributing anything. I guess we could have a queued splice with pending counterparty-initiated splice, but the feerate is too high for us, so we help the counterparty confirm their splice, such that we can try ours once it becomes locked?

Yeah, or we just changed our mind and want to back out. Similarly for dual funding.

ldk-reviews-bot · 2026-04-16T22:12:29Z

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-20T00:00:44Z

🔔 2nd Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-22T00:01:34Z

🔔 3rd Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

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>

jkczyz

Rebased and squashed. Some test assertions may need to change once #4514 is merged. Left TODOs.

jkczyz · 2026-04-23T14:22:37Z

+ let their_funding_contribution = match msg.funding_output_contribution {
+ Some(value) => SignedAmount::from_sat(value),
+ None => SignedAmount::ZERO,


Yeah, or we just changed our mind and want to back out. Similarly for dual funding.

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, internal_tx_init_rbf, and internal_tx_complete. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

The `exited_quiescence` field on `MsgHandleErrInternal` and `InteractiveTxMsgError` is a leaky abstraction -- the channelmanager error handling shouldn't know about quiescence, only whether the holding cell needs to be released. Infer this from the presence of a `tx_abort` instead, since exiting quiescence via an error always produces one. Remove `exited_quiescence` from `InteractiveTxMsgError`, `MsgHandleErrInternal`, and the return type of `Channel::tx_abort`, along with the `with_exited_quiescence` builder. For unfunded v2 channels, `tx_abort` may be present without quiescence having been exited, but the holding cell release is a no-op since an unfunded channel won't have any HTLCs. Similarly, the unreachable `debug_assert!(false)` branch in `fail_interactive_tx_negotiation` for funded channels produces a `tx_abort` without exiting quiescence, but the holding cell release is a no-op since the channel is still quiescent. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Several code paths exit quiescence by calling `clear_quiescent()` directly without also clearing the disconnect timer via `mark_response_received()`. This causes the timer to fire after the splice completes or is aborted, spuriously disconnecting the peer. Replace `clear_quiescent()` with `exit_quiescence()` in `on_tx_signatures_exchange`, `reset_pending_splice_state`, and `peer_connected_get_handshake`, which clears both the quiescent state and the disconnect timer. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

ldk-reviews-bot · 2026-04-25T00:00:32Z

🔔 4th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-27T00:01:41Z

🔔 5th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

jkczyz · 2026-03-18T18:48:42Z

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 · 2026-03-18T18:48:47Z

👋 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 · 2026-03-31T01:21:31Z

Codecov Report

❌ Patch coverage is 94.67456% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.14%. Comparing base (2313bd5) to head (d2f422b).
⚠️ Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	89.74%	7 Missing and 1 partial ⚠️
lightning/src/ln/channelmanager.rs	98.90%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4495 +/- ##
==========================================
+ Coverage 87.08% 87.14% +0.06% 
==========================================
Files 161 161 Lines 109255 109245 -10 Branches 109255 109245 -10 ==========================================
+ Hits 95147 95205 +58 + Misses 11627 11569 -58 + Partials 2481 2471 -10

Flag	Coverage Δ
fuzzing-fake-hashes	`31.08% <14.79%> (+0.10%)`	⬆️
fuzzing-real-hashes	`22.59% <3.55%> (-0.06%)`	⬇️
tests	`86.23% <94.67%> (+0.06%)`	⬆️

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 · 2026-04-02T17:12:22Z

+	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 }
+	}


In splice_init, resolve_queued_contribution (which can return Abort(FeeRateTooHigh)) is called beforevalidate_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(&mutself,err:ChannelError) -> InteractiveTxMsgError{
let exited_quiescence =
ifmatches!(err,ChannelError::Abort(_)){self.exit_quiescence()}else{false};
InteractiveTxMsgError{ err,splice_funding_failed:None, exited_quiescence }
}
fn quiescent_negotiation_err(&mutself,err:ChannelError) -> InteractiveTxMsgError{
let exited_quiescence = ifmatches!(err,ChannelError::Abort(_))
&&self.context.channel_state.is_quiescent()
{
self.exit_quiescence()
}else{
false
};
InteractiveTxMsgError{ err,splice_funding_failed:None, exited_quiescence }
}

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 · 2026-04-02T17:12:33Z

After a thorough review of the entire diff, including cross-referencing the existing codebase for correctness of validation ordering, error handling flows, and quiescence state transitions, I found no new issues beyond what was covered in my prior review comments.

Prior comment corrections:

My prior comment on channel.rs:9394 was incorrect — exit_quiescence() at line 9396 is inside the outer if let Some(pending_splice) block but after the inner if/else, so it correctly runs for both the success and debug_assert!(false) branches.
My prior comment on channelmanager.rs:12182 was incorrect — the code at lines 12156-12164 already uses the handle_interactive_tx_msg_err helper.
My prior comment on channel.rs:14284 about quiescent_negotiation_err's debug_assert!(is_quiescent()) is a valid defensive concern, but after verifying the code, both validate_splice_init (line 12894) and validate_tx_init_rbf (line 13064) are called before any Abort-producing code paths, ensuring the assert is safe with the current code.

No issues found.

jkczyz · 2026-04-02T23:09:37Z

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 · 2026-04-02T23:19:28Z

Rebased

ldk-reviews-bot · 2026-04-06T00:00:18Z

🔔 1st Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

TheBlueMatt · 2026-04-06T16:12:08Z

+ debug_assert!(!err.exited_quiescence || matches!(err.err, ChannelError::Abort(_)));
+
+ MsgHandleErrInternal::from_chan_no_close(err.err, channel_id)
+ .with_exited_quiescence(err.exited_quiescence)


This does feel a bit like a leaky abstraction - can we move exited_quiescence into the ChannelError and just call it needs_holding_cell_release? Even moreso because the variable doesn't make sense if we disconnect, close, or send an error.

Ok, I think we can infer it from tx_abort. At least I managed to get Claude to convince me that. Also, renamed the variables to needs_holding_cell_release and include similar inference elsewhere.
Let me know if this is looks good. I can make it a fixup if you prefer. May need a careful look, though.

ldk-reviews-bot · 2026-04-08T00:01:01Z

🔔 2nd Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-11T00:00:33Z

🔔 3rd Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-13T00:01:17Z

🔔 4th Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-15T00:02:23Z

🔔 5th Reminder

Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

wpaulino

Feel free to squash

wpaulino · 2026-04-15T17:14:20Z

+ let their_funding_contribution = match msg.funding_output_contribution {
+ Some(value) => SignedAmount::from_sat(value),
+ None => SignedAmount::ZERO,


I'm a bit confused why this field is even optional in the spec, and why we would allow the counterparty to initiate an RBF when they're not contributing anything. I guess we could have a queued splice with pending counterparty-initiated splice, but the feerate is too high for us, so we help the counterparty confirm their splice, such that we can try ours once it becomes locked?

Yeah, or we just changed our mind and want to back out. Similarly for dual funding.

ldk-reviews-bot · 2026-04-16T22:12:29Z

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-20T00:00:44Z

🔔 2nd Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-22T00:01:34Z

🔔 3rd Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

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>

jkczyz

Rebased and squashed. Some test assertions may need to change once #4514 is merged. Left TODOs.

jkczyz · 2026-04-23T14:22:37Z

+ let their_funding_contribution = match msg.funding_output_contribution {
+ Some(value) => SignedAmount::from_sat(value),
+ None => SignedAmount::ZERO,


Yeah, or we just changed our mind and want to back out. Similarly for dual funding.

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, internal_tx_init_rbf, and internal_tx_complete. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

The `exited_quiescence` field on `MsgHandleErrInternal` and `InteractiveTxMsgError` is a leaky abstraction -- the channelmanager error handling shouldn't know about quiescence, only whether the holding cell needs to be released. Infer this from the presence of a `tx_abort` instead, since exiting quiescence via an error always produces one. Remove `exited_quiescence` from `InteractiveTxMsgError`, `MsgHandleErrInternal`, and the return type of `Channel::tx_abort`, along with the `with_exited_quiescence` builder. For unfunded v2 channels, `tx_abort` may be present without quiescence having been exited, but the holding cell release is a no-op since an unfunded channel won't have any HTLCs. Similarly, the unreachable `debug_assert!(false)` branch in `fail_interactive_tx_negotiation` for funded channels produces a `tx_abort` without exiting quiescence, but the holding cell release is a no-op since the channel is still quiescent. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Several code paths exit quiescence by calling `clear_quiescent()` directly without also clearing the disconnect timer via `mark_response_received()`. This causes the timer to fire after the splice completes or is aborted, spuriously disconnecting the peer. Replace `clear_quiescent()` with `exit_quiescence()` in `on_tx_signatures_exchange`, `reset_pending_splice_state`, and `peer_connected_get_handshake`, which clears both the quiescent state and the disconnect timer. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

ldk-reviews-bot · 2026-04-25T00:00:32Z

🔔 4th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-27T00:01:41Z

🔔 5th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

jkczyz · 2026-03-18T18:48:42Z

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 · 2026-03-18T18:48:47Z

👋 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 · 2026-03-31T01:21:31Z

Codecov Report

❌ Patch coverage is 94.67456% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.14%. Comparing base (2313bd5) to head (d2f422b).
⚠️ Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	89.74%	7 Missing and 1 partial ⚠️
lightning/src/ln/channelmanager.rs	98.90%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4495 +/- ##
==========================================
+ Coverage 87.08% 87.14% +0.06% 
==========================================
Files 161 161 Lines 109255 109245 -10 Branches 109255 109245 -10 ==========================================
+ Hits 95147 95205 +58 + Misses 11627 11569 -58 + Partials 2481 2471 -10

Flag	Coverage Δ
fuzzing-fake-hashes	`31.08% <14.79%> (+0.10%)`	⬆️
fuzzing-real-hashes	`22.59% <3.55%> (-0.06%)`	⬇️
tests	`86.23% <94.67%> (+0.06%)`	⬆️

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 · 2026-04-02T17:12:22Z

+	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 }
+	}


In splice_init, resolve_queued_contribution (which can return Abort(FeeRateTooHigh)) is called beforevalidate_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(&mutself,err:ChannelError) -> InteractiveTxMsgError{
let exited_quiescence =
ifmatches!(err,ChannelError::Abort(_)){self.exit_quiescence()}else{false};
InteractiveTxMsgError{ err,splice_funding_failed:None, exited_quiescence }
}
fn quiescent_negotiation_err(&mutself,err:ChannelError) -> InteractiveTxMsgError{
let exited_quiescence = ifmatches!(err,ChannelError::Abort(_))
&&self.context.channel_state.is_quiescent()
{
self.exit_quiescence()
}else{
false
};
InteractiveTxMsgError{ err,splice_funding_failed:None, exited_quiescence }
}

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 · 2026-04-02T17:12:33Z

After a thorough review of the entire diff, including cross-referencing the existing codebase for correctness of validation ordering, error handling flows, and quiescence state transitions, I found no new issues beyond what was covered in my prior review comments.

Prior comment corrections:

My prior comment on channel.rs:9394 was incorrect — exit_quiescence() at line 9396 is inside the outer if let Some(pending_splice) block but after the inner if/else, so it correctly runs for both the success and debug_assert!(false) branches.
My prior comment on channelmanager.rs:12182 was incorrect — the code at lines 12156-12164 already uses the handle_interactive_tx_msg_err helper.
My prior comment on channel.rs:14284 about quiescent_negotiation_err's debug_assert!(is_quiescent()) is a valid defensive concern, but after verifying the code, both validate_splice_init (line 12894) and validate_tx_init_rbf (line 13064) are called before any Abort-producing code paths, ensuring the assert is safe with the current code.

No issues found.

jkczyz · 2026-04-02T23:09:37Z

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 · 2026-04-02T23:19:28Z

Rebased

ldk-reviews-bot · 2026-04-06T00:00:18Z

🔔 1st Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

TheBlueMatt · 2026-04-06T16:12:08Z

+ debug_assert!(!err.exited_quiescence || matches!(err.err, ChannelError::Abort(_)));
+
+ MsgHandleErrInternal::from_chan_no_close(err.err, channel_id)
+ .with_exited_quiescence(err.exited_quiescence)


This does feel a bit like a leaky abstraction - can we move exited_quiescence into the ChannelError and just call it needs_holding_cell_release? Even moreso because the variable doesn't make sense if we disconnect, close, or send an error.

Ok, I think we can infer it from tx_abort. At least I managed to get Claude to convince me that. Also, renamed the variables to needs_holding_cell_release and include similar inference elsewhere.
Let me know if this is looks good. I can make it a fixup if you prefer. May need a careful look, though.

ldk-reviews-bot · 2026-04-08T00:01:01Z

🔔 2nd Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-11T00:00:33Z

🔔 3rd Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-13T00:01:17Z

🔔 4th Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-15T00:02:23Z

🔔 5th Reminder

Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

wpaulino

Feel free to squash

wpaulino · 2026-04-15T17:14:20Z

+ let their_funding_contribution = match msg.funding_output_contribution {
+ Some(value) => SignedAmount::from_sat(value),
+ None => SignedAmount::ZERO,


I'm a bit confused why this field is even optional in the spec, and why we would allow the counterparty to initiate an RBF when they're not contributing anything. I guess we could have a queued splice with pending counterparty-initiated splice, but the feerate is too high for us, so we help the counterparty confirm their splice, such that we can try ours once it becomes locked?

Yeah, or we just changed our mind and want to back out. Similarly for dual funding.

ldk-reviews-bot · 2026-04-16T22:12:29Z

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-20T00:00:44Z

🔔 2nd Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-22T00:01:34Z

🔔 3rd Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

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>

jkczyz

Rebased and squashed. Some test assertions may need to change once #4514 is merged. Left TODOs.

jkczyz · 2026-04-23T14:22:37Z

+ let their_funding_contribution = match msg.funding_output_contribution {
+ Some(value) => SignedAmount::from_sat(value),
+ None => SignedAmount::ZERO,


Yeah, or we just changed our mind and want to back out. Similarly for dual funding.

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, internal_tx_init_rbf, and internal_tx_complete. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

The `exited_quiescence` field on `MsgHandleErrInternal` and `InteractiveTxMsgError` is a leaky abstraction -- the channelmanager error handling shouldn't know about quiescence, only whether the holding cell needs to be released. Infer this from the presence of a `tx_abort` instead, since exiting quiescence via an error always produces one. Remove `exited_quiescence` from `InteractiveTxMsgError`, `MsgHandleErrInternal`, and the return type of `Channel::tx_abort`, along with the `with_exited_quiescence` builder. For unfunded v2 channels, `tx_abort` may be present without quiescence having been exited, but the holding cell release is a no-op since an unfunded channel won't have any HTLCs. Similarly, the unreachable `debug_assert!(false)` branch in `fail_interactive_tx_negotiation` for funded channels produces a `tx_abort` without exiting quiescence, but the holding cell release is a no-op since the channel is still quiescent. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Several code paths exit quiescence by calling `clear_quiescent()` directly without also clearing the disconnect timer via `mark_response_received()`. This causes the timer to fire after the splice completes or is aborted, spuriously disconnecting the peer. Replace `clear_quiescent()` with `exit_quiescence()` in `on_tx_signatures_exchange`, `reset_pending_splice_state`, and `peer_connected_get_handshake`, which clears both the quiescent state and the disconnect timer. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

ldk-reviews-bot · 2026-04-25T00:00:32Z

🔔 4th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-27T00:01:41Z

🔔 5th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

jkczyz · 2026-03-18T18:48:42Z

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 · 2026-03-18T18:48:47Z

👋 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 · 2026-03-31T01:21:31Z

Codecov Report

❌ Patch coverage is 94.67456% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.14%. Comparing base (2313bd5) to head (d2f422b).
⚠️ Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	89.74%	7 Missing and 1 partial ⚠️
lightning/src/ln/channelmanager.rs	98.90%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4495 +/- ##
==========================================
+ Coverage 87.08% 87.14% +0.06% 
==========================================
Files 161 161 Lines 109255 109245 -10 Branches 109255 109245 -10 ==========================================
+ Hits 95147 95205 +58 + Misses 11627 11569 -58 + Partials 2481 2471 -10

Flag	Coverage Δ
fuzzing-fake-hashes	`31.08% <14.79%> (+0.10%)`	⬆️
fuzzing-real-hashes	`22.59% <3.55%> (-0.06%)`	⬇️
tests	`86.23% <94.67%> (+0.06%)`	⬆️

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 · 2026-04-02T17:12:22Z

+	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 }
+	}


In splice_init, resolve_queued_contribution (which can return Abort(FeeRateTooHigh)) is called beforevalidate_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(&mutself,err:ChannelError) -> InteractiveTxMsgError{
let exited_quiescence =
ifmatches!(err,ChannelError::Abort(_)){self.exit_quiescence()}else{false};
InteractiveTxMsgError{ err,splice_funding_failed:None, exited_quiescence }
}
fn quiescent_negotiation_err(&mutself,err:ChannelError) -> InteractiveTxMsgError{
let exited_quiescence = ifmatches!(err,ChannelError::Abort(_))
&&self.context.channel_state.is_quiescent()
{
self.exit_quiescence()
}else{
false
};
InteractiveTxMsgError{ err,splice_funding_failed:None, exited_quiescence }
}

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 · 2026-04-02T17:12:33Z

After a thorough review of the entire diff, including cross-referencing the existing codebase for correctness of validation ordering, error handling flows, and quiescence state transitions, I found no new issues beyond what was covered in my prior review comments.

Prior comment corrections:

My prior comment on channel.rs:9394 was incorrect — exit_quiescence() at line 9396 is inside the outer if let Some(pending_splice) block but after the inner if/else, so it correctly runs for both the success and debug_assert!(false) branches.
My prior comment on channelmanager.rs:12182 was incorrect — the code at lines 12156-12164 already uses the handle_interactive_tx_msg_err helper.
My prior comment on channel.rs:14284 about quiescent_negotiation_err's debug_assert!(is_quiescent()) is a valid defensive concern, but after verifying the code, both validate_splice_init (line 12894) and validate_tx_init_rbf (line 13064) are called before any Abort-producing code paths, ensuring the assert is safe with the current code.

No issues found.

jkczyz · 2026-04-02T23:09:37Z

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 · 2026-04-02T23:19:28Z

Rebased

ldk-reviews-bot · 2026-04-06T00:00:18Z

🔔 1st Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

TheBlueMatt · 2026-04-06T16:12:08Z

+ debug_assert!(!err.exited_quiescence || matches!(err.err, ChannelError::Abort(_)));
+
+ MsgHandleErrInternal::from_chan_no_close(err.err, channel_id)
+ .with_exited_quiescence(err.exited_quiescence)


This does feel a bit like a leaky abstraction - can we move exited_quiescence into the ChannelError and just call it needs_holding_cell_release? Even moreso because the variable doesn't make sense if we disconnect, close, or send an error.

Ok, I think we can infer it from tx_abort. At least I managed to get Claude to convince me that. Also, renamed the variables to needs_holding_cell_release and include similar inference elsewhere.
Let me know if this is looks good. I can make it a fixup if you prefer. May need a careful look, though.

ldk-reviews-bot · 2026-04-08T00:01:01Z

🔔 2nd Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-11T00:00:33Z

🔔 3rd Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-13T00:01:17Z

🔔 4th Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-15T00:02:23Z

🔔 5th Reminder

Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

wpaulino

Feel free to squash

wpaulino · 2026-04-15T17:14:20Z

+ let their_funding_contribution = match msg.funding_output_contribution {
+ Some(value) => SignedAmount::from_sat(value),
+ None => SignedAmount::ZERO,


I'm a bit confused why this field is even optional in the spec, and why we would allow the counterparty to initiate an RBF when they're not contributing anything. I guess we could have a queued splice with pending counterparty-initiated splice, but the feerate is too high for us, so we help the counterparty confirm their splice, such that we can try ours once it becomes locked?

Yeah, or we just changed our mind and want to back out. Similarly for dual funding.

ldk-reviews-bot · 2026-04-16T22:12:29Z

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-20T00:00:44Z

🔔 2nd Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-22T00:01:34Z

🔔 3rd Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

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>

jkczyz

Rebased and squashed. Some test assertions may need to change once #4514 is merged. Left TODOs.

jkczyz · 2026-04-23T14:22:37Z

+ let their_funding_contribution = match msg.funding_output_contribution {
+ Some(value) => SignedAmount::from_sat(value),
+ None => SignedAmount::ZERO,


Yeah, or we just changed our mind and want to back out. Similarly for dual funding.

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, internal_tx_init_rbf, and internal_tx_complete. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

The `exited_quiescence` field on `MsgHandleErrInternal` and `InteractiveTxMsgError` is a leaky abstraction -- the channelmanager error handling shouldn't know about quiescence, only whether the holding cell needs to be released. Infer this from the presence of a `tx_abort` instead, since exiting quiescence via an error always produces one. Remove `exited_quiescence` from `InteractiveTxMsgError`, `MsgHandleErrInternal`, and the return type of `Channel::tx_abort`, along with the `with_exited_quiescence` builder. For unfunded v2 channels, `tx_abort` may be present without quiescence having been exited, but the holding cell release is a no-op since an unfunded channel won't have any HTLCs. Similarly, the unreachable `debug_assert!(false)` branch in `fail_interactive_tx_negotiation` for funded channels produces a `tx_abort` without exiting quiescence, but the holding cell release is a no-op since the channel is still quiescent. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Several code paths exit quiescence by calling `clear_quiescent()` directly without also clearing the disconnect timer via `mark_response_received()`. This causes the timer to fire after the splice completes or is aborted, spuriously disconnecting the peer. Replace `clear_quiescent()` with `exit_quiescence()` in `on_tx_signatures_exchange`, `reset_pending_splice_state`, and `peer_connected_get_handshake`, which clears both the quiescent state and the disconnect timer. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

ldk-reviews-bot · 2026-04-25T00:00:32Z

🔔 4th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-04-27T00:01:41Z

🔔 5th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

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 rejectedMar 18, 2026

jkczyz force-pushed the 2026-03-splicing-rbf-follow-ups branch from a5d670c to 57aad34Compare March 18, 2026 18:57

jkczyz added this to Weekly Goals Mar 19, 2026

jkczyz self-assigned this Mar 19, 2026

jkczyz mentioned this pull request Mar 19, 2026
Splicing / Dual Funding #4284
Open
50 tasks

jkczyz force-pushed the 2026-03-splicing-rbf-follow-ups branch 3 times, most recently from 8cdc480 to 959b553Compare March 31, 2026 00:16

jkczyz force-pushed the 2026-03-splicing-rbf-follow-ups branch from 959b553 to 69a1a1cCompare March 31, 2026 22:28

jkczyz marked this pull request as ready for review April 2, 2026 17:03

ldk-reviews-bot requested a review from wpaulino April 2, 2026 17:04

ldk-claude-review-bot reviewed Apr 2, 2026
View reviewed changes

jkczyz commented Apr 2, 2026
View reviewed changes

jkczyz force-pushed the 2026-03-splicing-rbf-follow-ups branch from d275551 to 3c55d3cCompare April 2, 2026 23:16

wpaulino reviewed Apr 3, 2026
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/splicing_tests.rs

jkczyz requested a review from wpaulino April 3, 2026 21:23

jkczyz force-pushed the 2026-03-splicing-rbf-follow-ups branch from 3c55d3c to e4b1c6bCompare April 3, 2026 21:51

TheBlueMatt reviewed Apr 6, 2026
View reviewed changes

ldk-claude-review-bot reviewed Apr 8, 2026
View reviewed changes

Comment threadlightning/src/ln/channel.rs

jkczyz force-pushed the 2026-03-splicing-rbf-follow-ups branch from b861c68 to bdc17dcCompare April 8, 2026 14:04

jkczyz requested a review from TheBlueMatt April 14, 2026 22:11

wpaulino reviewed Apr 15, 2026
View reviewed changes

jkczyz mentioned this pull request Apr 22, 2026
Allow cancellation of pending splice funding negotiations #4490
Merged

jkczyzand others added 2 commits April 23, 2026 09:24

jkczyz force-pushed the 2026-03-splicing-rbf-follow-ups branch from bdc17dc to 528b80fCompare April 23, 2026 14:24

jkczyz commented Apr 23, 2026
View reviewed changes

ldk-claude-review-bot reviewed Apr 23, 2026
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated

jkczyzand others added 3 commits April 23, 2026 10:22

jkczyz force-pushed the 2026-03-splicing-rbf-follow-ups branch from 528b80f to d2f422bCompare April 23, 2026 15:25

TheBlueMatt approved these changes Apr 28, 2026
View reviewed changes

jkczyz requested a review from wpaulino April 28, 2026 14:22

wpaulino approved these changes Apr 28, 2026
View reviewed changes

wpaulino merged commit f23b8e6 into lightningdevkit:mainApr 28, 2026
24 of 25 checks passed

github-project-automationBot moved this to Done in Weekly Goals Apr 28, 2026