From 35a42c38eda51b8eb9b36dbf080c0c3fea832338 Mon Sep 17 00:00:00 2001 From: maclane Date: Fri, 26 Jun 2026 16:35:18 -0400 Subject: [PATCH 1/2] fix(frost/roast): evaluate attempt feasibility against permanent exclusions 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) --- .../roast/multi_coordinator_soak_test.go | 32 +++++-- pkg/frost/roast/next_attempt.go | 42 +++++++-- pkg/frost/roast/next_attempt_test.go | 93 ++++++++++++++++++- 3 files changed, 145 insertions(+), 22 deletions(-) diff --git a/pkg/frost/roast/multi_coordinator_soak_test.go b/pkg/frost/roast/multi_coordinator_soak_test.go index 18caa65e5c..333c68a75f 100644 --- a/pkg/frost/roast/multi_coordinator_soak_test.go +++ b/pkg/frost/roast/multi_coordinator_soak_test.go @@ -347,14 +347,16 @@ func TestSoak_ParkedMemberIsReinstatedNextAttempt(t *testing.T) { } } -func TestSoak_InfeasibilityWhenBelowThreshold(t *testing.T) { +func TestSoak_TransientSilenceBelowThresholdRecovers(t *testing.T) { members := []group.MemberIndex{1, 2, 3, 4, 5} nodes := newSoakHarness(t, members) prev := soakStartingContext(t, members) - // Threshold = 5 (all members required). Silence two members. - // Next attempt's IncludedSet would be 3 (= 5 - 2 silenced), below 5. - // NextAttempt must return ErrAttemptInfeasible. + // Threshold = 5 (all members required). Silence two members, dropping the + // next attempt's IncludedSet to 3 (= 5 - 2 silenced), below threshold. No + // member is permanently excluded, so NextAttempt must NOT fail: each honest + // node transiently parks the silenced members for reinstatement by a later + // attempt. silence := map[group.MemberIndex]bool{ 4: true, 5: true, @@ -393,15 +395,29 @@ func TestSoak_InfeasibilityWhenBelowThreshold(t *testing.T) { } bundle, _ := aggregator.node.coord.AggregateBundle(aggregator.handle) - // Verify each non-coordinator's NextAttempt returns infeasible. + // Each honest node's NextAttempt must succeed (not fail closed) and park the + // silenced members transiently rather than excluding them permanently. for _, b := range begins { - _, err := b.node.coord.NextAttempt(b.handle, bundle, 5, []byte{0x01}) - if !errors.Is(err, ErrAttemptInfeasible) { + next, err := b.node.coord.NextAttempt(b.handle, bundle, 5, []byte{0x01}) + if err != nil { t.Fatalf( - "node %d NextAttempt: expected ErrAttemptInfeasible; got %v", + "node %d NextAttempt: transient silence must not fail the session; got %v", b.node.self, err, ) } + if !memberSliceContains(next.TransientlyParked, 4) || + !memberSliceContains(next.TransientlyParked, 5) { + t.Fatalf( + "node %d: silenced members 4 and 5 must be transiently parked; got %v", + b.node.self, next.TransientlyParked, + ) + } + if len(next.ExcludedSet) != 0 { + t.Fatalf( + "node %d: silence must not permanently exclude; got %v", + b.node.self, next.ExcludedSet, + ) + } } } diff --git a/pkg/frost/roast/next_attempt.go b/pkg/frost/roast/next_attempt.go index 42d4aa63d1..7a1e279fe7 100644 --- a/pkg/frost/roast/next_attempt.go +++ b/pkg/frost/roast/next_attempt.go @@ -242,23 +242,47 @@ func computeNextAttempt( // (5) Reinstate previously parked members by re-including them // (unless newly permanently excluded or re-parked). - included := original.sorted() - included = filterOut(included, exclSet) - included = filterOut(included, parkSet) - - // (6) Infeasibility check. - if threshold > 0 && uint(len(included)) < threshold { + // + // Feasibility (step 6) is judged against the PERMANENTLY-available set -- + // the original signer set minus permanent exclusions -- NOT the + // post-parking included set. Transiently-parked members (overflow and + // silence) are reinstated by a later attempt (steps 3-4), so counting them + // as gone here would let a single transient mass-silence event, or one + // byzantine elected coordinator that omits snapshots, permanently fail a + // session the original signer set can still complete -- exactly the + // grind-to-ErrAttemptInfeasible failure the accuser quorum exists to + // prevent, here via the ungated silence-parking path. Permanent exclusion + // is the only thing that can render a session infeasible. + feasible := filterOut(original.sorted(), exclSet) + + // (6) Infeasibility check: only when permanent exclusions leave fewer than + // threshold members can no future attempt ever reach the threshold. + if threshold > 0 && uint(len(feasible)) < threshold { return attempt.AttemptContext{}, fmt.Errorf( - "%w: %d eligible, threshold %d", + "%w: %d non-excluded, threshold %d", ErrAttemptInfeasible, - len(included), + len(feasible), threshold, ) } + // The next attempt's included set is the feasible set minus this attempt's + // transient parking. It may fall below threshold (the parked members are + // reinstated next attempt); that burns one attempt rather than failing the + // session. + included := filterOut(feasible, parkSet) + nextParked := parkSet.sorted() + if len(included) == 0 { + // Every non-excluded member is transiently parked this attempt. Parking + // is strictly transient and an AttemptContext requires a non-empty + // included set, so reinstate the parked members now (they retry this + // attempt) rather than producing an empty, unconstructable attempt. + included = feasible + nextParked = nil + } + // Convert ExcludedSet to its canonical (sorted, deduped) slice. nextExcluded := exclSet.sorted() - nextParked := parkSet.sorted() next, err := attempt.NewAttemptContextWithParking( prev.SessionID, diff --git a/pkg/frost/roast/next_attempt_test.go b/pkg/frost/roast/next_attempt_test.go index 216941747a..1723f6d3ed 100644 --- a/pkg/frost/roast/next_attempt_test.go +++ b/pkg/frost/roast/next_attempt_test.go @@ -387,18 +387,101 @@ func TestNextAttempt_PolicyIsDeterministic(t *testing.T) { } } -func TestNextAttempt_InfeasibilityWhenBelowThreshold(t *testing.T) { +func TestNextAttempt_InfeasibilityWhenPermanentExclusionsBelowThreshold(t *testing.T) { f := newNextAttemptFixture() - f.threshold = 5 // Require all 5 members. - // Silently lose 2 members -> only 3 remain in IncludedSet, below - // threshold of 5. - f.bundleSenders = []group.MemberIndex{1, 2, 3} + f.threshold = 5 // n-of-n: the accuser quorum is 1, so one reject establishes. + // Permanently exclude member 3 via an established reject accusation. Only 4 + // non-excluded members remain, below the threshold of 5, and a permanently + // excluded member is never reinstated -- so the session is genuinely + // infeasible. + f.rejects[1] = map[group.MemberIndex]uint{3: 1} _, err := computeNextAttempt(f.prev(t), f.bundle(t), f.threshold, f.dkgGroupPublicKey, fakeVerifier{}) if !errors.Is(err, ErrAttemptInfeasible) { t.Fatalf("expected ErrAttemptInfeasible, got %v", err) } } +func TestNextAttempt_TransientSilenceBelowThresholdDoesNotPermanentlyFail(t *testing.T) { + f := newNextAttemptFixture() + f.threshold = 5 // Require all 5 members. + // Silently lose 2 members -> the next attempt's included set drops to 3 + // (below threshold), but NO member is permanently excluded. The session + // must NOT be declared infeasible: the silenced members are transiently + // parked and a later attempt reinstates them. + f.bundleSenders = []group.MemberIndex{1, 2, 3} + next, err := computeNextAttempt(f.prev(t), f.bundle(t), f.threshold, f.dkgGroupPublicKey, fakeVerifier{}) + if err != nil { + t.Fatalf("transient silence must not permanently fail the session, got %v", err) + } + if !memberSliceContains(next.TransientlyParked, 4) || + !memberSliceContains(next.TransientlyParked, 5) { + t.Fatalf( + "silenced members 4 and 5 must be transiently parked; got parked %v", + next.TransientlyParked, + ) + } + if len(next.ExcludedSet) != 0 { + t.Fatalf( + "transient silence must not permanently exclude; got excluded %v", + next.ExcludedSet, + ) + } +} + +func TestNextAttempt_TransientSilenceRecoversAcrossTwoAttempts(t *testing.T) { + f := newNextAttemptFixture() + f.threshold = 5 + + // Attempt N -> N+1: members 4 and 5 are silent. They are parked (not + // excluded) and the next attempt's included set is {1,2,3} (sub-threshold), + // but the session is not failed. + f.bundleSenders = []group.MemberIndex{1, 2, 3} + attemptN1, err := computeNextAttempt( + f.prev(t), f.bundle(t), f.threshold, f.dkgGroupPublicKey, fakeVerifier{}, + ) + if err != nil { + t.Fatalf("attempt N+1: transient silence must not fail, got %v", err) + } + if !memberSliceContains(attemptN1.TransientlyParked, 4) || + !memberSliceContains(attemptN1.TransientlyParked, 5) { + t.Fatalf( + "attempt N+1: members 4 and 5 must be parked; got %v", + attemptN1.TransientlyParked, + ) + } + + // Attempt N+1 -> N+2: the previously-parked members are reinstated, so the + // included set returns to all five and the session recovers without + // intervention. The bundle defaults to N+1's included set {1,2,3}, which all + // respond. + g := newNextAttemptFixture() + g.threshold = 5 + g.included = attemptN1.IncludedSet + g.excluded = attemptN1.ExcludedSet + g.parked = attemptN1.TransientlyParked + g.attemptNumber = attemptN1.AttemptNumber + attemptN2, err := computeNextAttempt( + g.prev(t), g.bundle(t), g.threshold, g.dkgGroupPublicKey, fakeVerifier{}, + ) + if err != nil { + t.Fatalf("attempt N+2: %v", err) + } + for _, m := range []group.MemberIndex{1, 2, 3, 4, 5} { + if !memberSliceContains(attemptN2.IncludedSet, m) { + t.Fatalf( + "attempt N+2 must reinstate all members; missing %d, got included %v", + m, attemptN2.IncludedSet, + ) + } + } + if len(attemptN2.TransientlyParked) != 0 { + t.Fatalf( + "attempt N+2 should have no parked members; got %v", + attemptN2.TransientlyParked, + ) + } +} + func TestNextAttempt_ThresholdZeroDisablesInfeasibilityCheck(t *testing.T) { f := newNextAttemptFixture() f.threshold = 0 From 7037b818570528adbbaae3167953b8e8d4e6b9d6 Mon Sep 17 00:00:00 2001 From: maclane Date: Sat, 27 Jun 2026 16:30:39 -0400 Subject: [PATCH 2/2] docs(frost/roast): correct the ErrAttemptInfeasible contract for transient 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) --- pkg/frost/roast/next_attempt.go | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/pkg/frost/roast/next_attempt.go b/pkg/frost/roast/next_attempt.go index 7a1e279fe7..1865be1d5f 100644 --- a/pkg/frost/roast/next_attempt.go +++ b/pkg/frost/roast/next_attempt.go @@ -59,13 +59,19 @@ func ExclusionAccuserQuorum(groupSize, threshold uint) uint { return groupSize - threshold + 1 } -// ErrAttemptInfeasible is returned by NextAttempt when the next -// attempt's IncludedSet would drop below the signing threshold t and -// the session can no longer make progress with the original signer -// set. Callers must surface this to the application layer: the -// session is permanently failed. +// ErrAttemptInfeasible is returned by NextAttempt when PERMANENT exclusions +// leave fewer than threshold non-excluded members, so no future attempt can +// ever reach the signing threshold t and the session can no longer make +// progress with the original signer set. Callers must surface this to the +// application layer: the session is permanently failed. +// +// It is NOT returned merely because the next attempt's IncludedSet falls below +// threshold due to TRANSIENT parking (silence/overflow): parked members are +// reinstated on the following attempt, so a sub-threshold included set burns +// one attempt rather than failing the session. The infeasibility test is on the +// non-excluded (feasible) set, not the post-parking included set. var ErrAttemptInfeasible = errors.New( - "coordinator: next attempt is infeasible -- included set below threshold", + "coordinator: next attempt is infeasible -- non-excluded members below threshold", ) // NextAttempt computes the deterministic next attempt context from a @@ -117,8 +123,12 @@ var ErrAttemptInfeasible = errors.New( // TransientlyParked set automatically rejoin the next attempt's // IncludedSet (unless they are now permanently excluded). // -// 6. Infeasibility: if the next attempt's IncludedSet would have -// fewer than threshold members, return ErrAttemptInfeasible. +// 6. Infeasibility: if PERMANENT exclusions leave fewer than threshold +// non-excluded members -- so no future attempt can reach the +// threshold -- return ErrAttemptInfeasible. A next IncludedSet that +// falls below threshold only because of TRANSIENT parking is NOT +// infeasible: the parked members rejoin on the following attempt +// (step 5), so it burns one attempt rather than failing the session. // // Verifiability roadmap: permanent exclusion on a *single* piece of // evidence becomes sound once the wire format carries