Drop QuiescentError::with_negotiation_failure_reason#4608
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
|
I've thoroughly reviewed every hunk in this diff, including reading the surrounding context for the modified functions. My prior review already covered all sections comprehensively. No issues found. |
| /// | ||
| /// Outputs are compared by `script_pubkey` alone (not full `TxOut`), since values may | ||
| /// differ between rounds (e.g., a change output adjusted for a new feerate). As a | ||
| /// consequence, multiple contribution outputs sharing a `script_pubkey` are all |
There was a problem hiding this comment.
Just noting that this won't be allowed anymore after #4575
| // Only the test-only DoNothing action is expected here; production callers must | ||
| // short-circuit before reaching this branch. | ||
| #[cfg(any(test, fuzzing, feature = "_test_utils"))] | ||
| let is_allowed = matches!(action, QuiescentAction::DoNothing); |
There was a problem hiding this comment.
Not a huge deal, but why does this need to be allowed?
There was a problem hiding this comment.
There are some tests calling maybe_propose_quiescence with it.
There was a problem hiding this comment.
Sorry I meant why do we need to allow another DoNothing after one is already pending? Removing this doesn't seem to cause any test failures, is it a fuzzing thing?
There was a problem hiding this comment.
Ah... I may have mistakenly thought quiescence_tests was checking this. But indeed that doesn't seem to be the case.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4608 +/- ##
==========================================
+ Coverage 86.44% 86.47% +0.02%
==========================================
Files 159 159
Lines 109823 109821 -2
Branches 109823 109821 -2
==========================================
+ Hits 94941 94969 +28
+ Misses 12340 12315 -25
+ Partials 2542 2537 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jkczyz
force-pushed
the
2026-05-splice-rbf-fail-event-follow-ups
branch
from
b61f231 to
b2b0d43
Compare
|
🔔 1st Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
1 similar comment
|
🔔 1st Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @wpaulino! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @wpaulino! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @wpaulino! This PR has been waiting for your review. |
QuiescentError::FailSplice was built with a placeholder NegotiationFailureReason::Unknown and expected callers to chain a with_negotiation_failure_reason builder. Sites that forgot the chain leaked Unknown into Event::SpliceNegotiationFailed, and the pattern forced splice-specific reason vocabulary into the generic QuiescentAction helper. Each call site in propose_quiescence now picks the reason at construction. The pending-quiescent-action branch is unreachable, so it asserts unconditionally; the match retains arms for both action variants so release builds return a sensible error if the invariant is violated. abandon_quiescent_action returns SpliceFundingFailed directly without round-tripping through QuiescentError, since the reason was always discarded there. Make funding_contributed's pending-quiescent-action check exhaustive on QuiescentAction. A future variant produces a compile error here and at the matching arm in propose_quiescence, forcing the author to decide how it interacts with funding contribution. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The contribution returned in Event::SpliceNegotiationFailed may include inputs and outputs already committed to a prior negotiated (but not yet locked) splice transaction. Those overlapping items are intentionally omitted from the preceding Event::DiscardFunding to avoid prompting the user to reclaim UTXOs that are still in use elsewhere. The relationship was documented on the internal SpliceFundingFailed fields but lost when they were made private; surface it on the public event field doc. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The function compares outputs by script_pubkey alone, not full TxOut, so any contribution output sharing a script with an existing output is filtered regardless of value. This is intentional — a change output's value may shift between rounds (e.g., for a new feerate) and should still match. But the consequence isn't obvious: multiple contribution outputs sharing a script are all filtered together when any existing output uses that script. Document it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jkczyz
force-pushed
the
2026-05-splice-rbf-fail-event-follow-ups
branch
from
b2b0d43 to
f4139db
Compare
5 participants
Drops
QuiescentError::with_negotiation_failure_reasonas a follow-up to #4514. The placeholderUnknownreason it overrode was a footgun — sites that forgot to chain it could leakUnknownintoEvent::SpliceNegotiationFailed, and the pattern wired splice-specific reason vocabulary into the genericQuiescentActionmachinery. Each error-return site inpropose_quiescencenow picks its reason at construction.funding_contributed's pending-quiescent-action check is also made exhaustive onQuiescentAction, so a future variant produces a compile error there.Also includes two minor doc clarifications from the #4514 review: the relationship between
Event::SpliceNegotiationFailed::contributionandEvent::DiscardFunding, and the script_pubkey-only matching inFundingContribution::into_unique_contributions.