fix(frost/roast): evaluate attempt feasibility against permanent exclusions only#4116
Merged
mswilkison merged 2 commits intoJun 27, 2026
Conversation
…usions only
NextAttempt's infeasibility check used the post-parking IncludedSet (which
subtracts transiently-parked and silenced members) to decide PERMANENT session
failure (ErrAttemptInfeasible). Silence-parking has no accuser-quorum gate, so a
single transient mass-silence event -- or one byzantine member that is the
elected coordinator for one attempt and omits snapshots -- could drop the
IncludedSet below threshold and permanently kill a signing session, even though
the original signer set could still complete it. This contradicts the file's own
step-4 contract ("silence parking is strictly transient; a falsely-silenced
honest peer recovers without intervention") and ErrAttemptInfeasible's own doc
("the session can no longer make progress with the original signer set"), and it
routes a permanent failure around the very accuser-quorum defense that exists to
stop a byzantine minority from grinding the group to ErrAttemptInfeasible.
Evaluate feasibility against the permanently-available set (original \\
ExcludedSet): only permanent exclusions (established reject/conflict/
equivocation) can render a session infeasible. The next attempt's IncludedSet is
still feasible \\ parkSet (it may fall below threshold for one attempt; the
parked members are reinstated next attempt). When parking would empty the
IncludedSet, reinstate the parked members rather than producing an
unconstructable empty attempt. Pathological grinding under sustained malicious
silence stays bounded by the outer tBTC signingAttemptsLimit.
The two existing tests asserted the buggy behavior (silence below threshold ->
permanent fail); retarget them to genuine permanent-exclusion infeasibility and
add transient-silence-recovers coverage (single-step, full harness, and a
two-attempt park-then-reinstate cycle).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…sient parking After the transient-silence feasibility fix, NextAttempt's infeasibility test is on the non-excluded (feasible) set, not the post-parking IncludedSet: a next IncludedSet can fall below threshold due to transient parking without returning ErrAttemptInfeasible (parked members reinstate next attempt). Update the ErrAttemptInfeasible doc + the NextAttempt step-6 contract (and the now-misleading 'included set below threshold' error string) to match. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
d47cde3
into
feat/frost-schnorr-migration-scaffold
17 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #3866 (Go FROST/ROAST coordinator). Fixes a robustness bug in the ROAST
NextAttemptpolicy where a transient event could permanently kill a signing session.computeNextAttempt's infeasibility check used the post-parkingIncludedSet— which subtracts transiently-parked and silenced members — to decide permanent session failure (ErrAttemptInfeasible). Silence-parking deliberately has no accuser-quorum gate (it's meant to be strictly transient), so a single transient mass-silence event — or one byzantine member that is the elected coordinator for one attempt and omits snapshots — could drop theIncludedSetbelow threshold and permanently fail the session, even though the original signer set could still complete it.This contradicts the file's own design contract and hands a single byzantine member (elected coordinator for one attempt) the power to permanently kill a session — exactly the grind-to-
ErrAttemptInfeasibleoutcome the accuser-quorum machinery exists to prevent, routed around via the ungated silence path:ErrAttemptInfeasibledoc: returned when "the session can no longer make progress with the original signer set."Fix
Evaluate feasibility against the permanently-available set (
original \ ExcludedSet): only permanent exclusions (established reject / conflict / coordinator-equivocation) can render a session infeasible. The next attempt'sIncludedSetis stillfeasible \ parkSet— it may fall below threshold for one attempt, but the parked members are reinstated next attempt (burning one attempt instead of failing the session). When parking would empty theIncludedSet, the parked members are reinstated now rather than producing an unconstructable empty attempt.Pathological grinding under sustained malicious silence stays bounded by the outer tBTC
signingAttemptsLimit— the inner chain no longer needs a permanent-fail to stop grinding.Tests
The two existing tests asserted the buggy behavior (silence below threshold → permanent fail, using a degenerate n-of-n config). Retargeted to genuine infeasibility and added recovery coverage:
TestNextAttempt_InfeasibilityWhenPermanentExclusionsBelowThreshold— permanent exclusion below threshold still fails (correctly).TestSoak_TransientSilenceBelowThresholdRecovers— full multi-coordinator harness: silence does not fail; silenced members are parked, not excluded.TestNextAttempt_TransientSilenceBelowThresholdDoesNotPermanentlyFail— unit: silence parks (transient), never excludes (permanent).TestNextAttempt_TransientSilenceRecoversAcrossTwoAttempts— end-to-end: silence → park → reinstate, included set returns to full.The "original signer set preserved" invariant (
|Inc|+|Exc|+|Park|= original size) holds in both branches.gofmt,go vet, and the fullpkg/frost/roastsuite pass; builds clean untagged and under-tags 'frost_native frost_tbtc_signer cgo frost_roast_retry'.Found during review of #3866.
🤖 Generated with Claude Code