From c7524adf0720e9367a390b71a1a275b970384913 Mon Sep 17 00:00:00 2001 From: maclane Date: Fri, 19 Jun 2026 16:52:10 -0400 Subject: [PATCH 1/2] feat(frost): finalize interactive signing over the first t responsive committers (7.3 t-of-included PR2/3) The interactive ROAST signing runner waited for ALL included members in round 1 and round 2, so one slow or offline member stalled the attempt to its ctx deadline -> fail -> ROAST retry. Make the attempt finalize over the first t responsive committers instead: - Coordinator: collectCommitments now collects until EXACTLY t (the threshold) and stops, builds the FROST package over that t-subset, and carries the chosen members in the package's signed signer_ids (PR1's field), ascending+distinct. - Non-coordinator signer (in signer_ids): runs round 2 and collects round-2 shares from the package's signer set, not the full included set. - Observer (committed in round 1 but not in signer_ids): does NOT run round 2; it aggregates the subset's broadcast shares publicly, marks the attempt succeeded, and returns the signature - and still aborts the engine session on success to drop its unconsumed round-1 nonces (the success path previously suppressed the abort, which leaked an observer's nonces). t-of-included stays inert until participant selection oversizes the included set past the threshold (it currently trims to exactly honestThreshold), so the machinery is harmless and dormant until MacLane enables oversizing. PR3 will make signingDoneCheck (pkg/tbtc) threshold/package-set aware. Also harden the runner constructor to reject threshold > included-set size, the new round-1 collection loop's termination invariant. All behind the frost_native pre-prod build tags. Co-Authored-By: Claude Opus 4.8 --- .../roast_runner_engine_frost_native_test.go | 34 +- .../signing/roast_runner_frost_native.go | 205 ++++++++--- .../signing/roast_runner_frost_native_test.go | 337 +++++++++++++++++- 3 files changed, 508 insertions(+), 68 deletions(-) diff --git a/pkg/frost/signing/roast_runner_engine_frost_native_test.go b/pkg/frost/signing/roast_runner_engine_frost_native_test.go index 7ecdfdf6bd..45b9eb9e94 100644 --- a/pkg/frost/signing/roast_runner_engine_frost_native_test.go +++ b/pkg/frost/signing/roast_runner_engine_frost_native_test.go @@ -33,15 +33,16 @@ type fakeInteractiveSigningEngine struct { commitmentsByMember map[uint16][]byte shareByMember map[uint16][]byte - mu sync.Mutex - openCalls int - round1Calls int - newPackageCalls int - round2Calls int - aggregateCalls int - abortCalls int - deriveCalls int - lastAggregateShares []nativeFROSTSignatureShare + mu sync.Mutex + openCalls int + round1Calls int + newPackageCalls int + round2Calls int + aggregateCalls int + abortCalls int + deriveCalls int + lastAggregateShares []nativeFROSTSignatureShare + lastNewPackageCommitments []nativeFROSTCommitment } func (f *fakeInteractiveSigningEngine) DeriveInteractiveAttemptContext( @@ -93,6 +94,20 @@ func (f *fakeInteractiveSigningEngine) abortCallCount() int { return f.abortCalls } +func (f *fakeInteractiveSigningEngine) round2CallCount() int { + f.mu.Lock() + defer f.mu.Unlock() + return f.round2Calls +} + +// newPackageCommitments returns a copy of the commitments the engine last built a +// signing package over - the chosen t-subset for the coordinator. +func (f *fakeInteractiveSigningEngine) newPackageCommitments() []nativeFROSTCommitment { + f.mu.Lock() + defer f.mu.Unlock() + return append([]nativeFROSTCommitment(nil), f.lastNewPackageCommitments...) +} + func newFakeInteractiveSigningEngine() *fakeInteractiveSigningEngine { return &fakeInteractiveSigningEngine{ attemptID: "attempt-1", @@ -153,6 +168,7 @@ func (f *fakeInteractiveSigningEngine) NewSigningPackage( ) ([]byte, error) { f.mu.Lock() f.newPackageCalls++ + f.lastNewPackageCommitments = append([]nativeFROSTCommitment(nil), commitments...) f.mu.Unlock() return f.signingPackage, nil } diff --git a/pkg/frost/signing/roast_runner_frost_native.go b/pkg/frost/signing/roast_runner_frost_native.go index a943d4f393..1eca316c54 100644 --- a/pkg/frost/signing/roast_runner_frost_native.go +++ b/pkg/frost/signing/roast_runner_frost_native.go @@ -6,6 +6,7 @@ import ( "bytes" "context" "fmt" + "sort" "github.com/keep-network/keep-core/pkg/frost/roast" "github.com/keep-network/keep-core/pkg/frost/roast/attempt" @@ -30,9 +31,11 @@ import ( // only as sound as that authentication. // 2. Delivery must not let a slow or flooding peer block an honest broadcaster // indefinitely. The runner does not fully drain every stream - it bounds the -// equivocation drains, and the coordinator never reads its own package -// stream - so the transport must apply backpressure or drop, never block -// forever, on an undrained or oversubscribed stream. +// equivocation drains, the coordinator never reads its own package stream, +// and for t-of-included finalize the coordinator stops reading commitments +// once t have arrived while non-coordinators never read commitments at all - +// so the transport must apply backpressure or drop, never block forever, on +// an undrained or oversubscribed stream. // // Round-1 commitments are unsigned: with authenticated senders the worst case is // a member's own bad or equivocated commitment, which surfaces as a round-2 @@ -89,6 +92,17 @@ func newInteractiveSigningRunner( "roast runner: member %d is not in the attempt's included set", member, ) } + // The coordinator's round-1 collection proceeds until t responsive committers, + // so t must not exceed the included set - otherwise the subset can never form + // and the coordinator would block to the ctx deadline on every attempt. A + // well-formed attempt always selects at least threshold members; fail fast at + // construction rather than silently degrade into timeout-driven retries. + if int(threshold) > len(attemptCtx.IncludedSet) { + return nil, fmt.Errorf( + "roast runner: threshold [%d] exceeds the attempt's included set size [%d]", + threshold, len(attemptCtx.IncludedSet), + ) + } // The signed message is the binding's MessageDigest, derived here rather than // accepted as a parameter: a caller-supplied message that diverged from the // digest the attempt (and its package/share envelopes) is bound to could mark @@ -169,21 +183,32 @@ func (r *interactiveSigningRunner) Run(ctx context.Context) ([]byte, error) { attemptID := open.AttemptID // Cleanup on conclusion, by outcome: - // - SUCCESS: prune this attempt's round-2 collector state per the + // - SUCCESS as a SIGNER: prune this attempt's round-2 collector state per the // prune-on-conclusion contract (nothing needs it), so a collector reused - // across attempts does not retain concluded attempts indefinitely. - // - FAILURE / early exit: abort the engine session so it drops this - // attempt's resident secret nonces, but do NOT prune the collector. A - // failure path (the root-divergence return below, or - once it lands - an - // aggregate share-verification failure) retains signed evidence that the - // blame/retry path must still read via CoordinatorPackageProofs / - // ClassifyCandidateCulprits; the caller prunes after snapshotting or - // propagating it. + // across attempts does not retain concluded attempts indefinitely. Round 2 + // already consumed our round-1 nonces, so no abort is needed. + // - SUCCESS as an OBSERVER: a member that committed in round 1 but was not in + // the chosen signing subset obtains the signature by aggregating the + // subset's broadcast shares WITHOUT signing, so its round-1 nonces are + // still resident in the engine. Prune the collector AND abort the session + // to drop those nonces - the success branch otherwise suppresses the abort. + // - FAILURE / early exit: abort the engine session so it drops this attempt's + // resident secret nonces, but do NOT prune the collector. A failure path + // (the root-divergence return below, or an aggregate share-verification + // failure) retains signed evidence that the blame/retry path must still + // read via CoordinatorPackageProofs / ClassifyCandidateCulprits; the caller + // prunes after snapshotting or propagating it. // Best-effort: neither may mask the run's real outcome. succeeded := false + // signedRound2 records whether this node ran round 2 and thereby consumed its + // round-1 nonces; an observer never does, so it must still abort on success. + signedRound2 := false defer func() { if succeeded { r.collector.PruneAttempt(contextHash[:]) + if !signedRound2 { + _, _ = r.engine.InteractiveSessionAbort(binding.SessionID(), &attemptID) + } return } _, _ = r.engine.InteractiveSessionAbort(binding.SessionID(), &attemptID) @@ -202,10 +227,16 @@ func (r *interactiveSigningRunner) Run(ctx context.Context) ([]byte, error) { } r.broadcast(RunnerMsgCommitments, contextHash, ownCommitments) - // 4. Collect every included member's commitments. + // 4. Only the elected coordinator collects commitments - it alone builds the + // signing package. It gathers the first t responsive committers (its own + // already seeded) and builds the package over exactly that subset, so a member + // past the t-th to commit (slow or offline) never stalls the attempt. Every + // other member broadcast its own commitment above and now awaits the package. commitments := map[group.MemberIndex][]byte{r.member: ownCommitments} - if err := r.collectCommitments(ctx, r.sub.Commitments(), contextHash, includedSet, commitments); err != nil { - return nil, fmt.Errorf("roast runner: collect commitments: %w", err) + if r.member == elected { + if err := r.collectCommitments(ctx, r.sub.Commitments(), contextHash, includedSet, commitments); err != nil { + return nil, fmt.Errorf("roast runner: collect commitments: %w", err) + } } // 5. The elected coordinator builds, signs, and broadcasts the signing @@ -240,31 +271,55 @@ func (r *interactiveSigningRunner) Run(ctx context.Context) ([]byte, error) { return nil, fmt.Errorf("roast runner: signing package taproot root diverges from the bound session root") } - // 6. Round 2: our signature share, recorded locally and broadcast. - ownShare, err := r.engine.InteractiveRound2(binding.SessionID(), attemptID, uint16(r.member), pkg.SigningPackageBytes) - if err != nil { - return nil, fmt.Errorf("roast runner: round 2: %w", err) - } - ownSubmission, ownSubmissionEnvelope, err := r.signShareSubmission(pkg, contextHash, elected, ownShare) - if err != nil { - return nil, err - } - if err := r.collector.RecordShareSubmission(ownSubmission); err != nil { - return nil, fmt.Errorf("roast runner: record own share submission: %w", err) + // 6. The package names the chosen signing subset (the first t responsive + // committers the coordinator built it over). A local member IN that subset is + // a signer; one NOT in it is an OBSERVER - it committed in round 1 but was not + // chosen, so it does not sign and only aggregates the subset's broadcast + // shares. An empty signer set is the full-included flow (back-compat / no + // oversizing): every included member signs. SignerIDs() is safe here - + // Unmarshal and AuthenticateSigningPackage (via RecordSigningPackage) both + // Validate, so each id is a real, ascending, distinct member index. + signerSet := pkg.SignerIDs() + if len(signerSet) == 0 { + signerSet = includedSet + } + + // 7. Round 2 (signers only): our signature share, recorded locally and + // broadcast. An observer skips round 2 entirely, leaving its round-1 nonces + // resident - the cleanup defer aborts them on success. + shares := map[group.MemberIndex][]byte{} + if memberInSet(r.member, signerSet) { + ownShare, err := r.engine.InteractiveRound2(binding.SessionID(), attemptID, uint16(r.member), pkg.SigningPackageBytes) + if err != nil { + return nil, fmt.Errorf("roast runner: round 2: %w", err) + } + // Round 2 consumed our round-1 nonces: a successful signer prunes without + // aborting; only a non-signing observer still needs the abort. + signedRound2 = true + ownSubmission, ownSubmissionEnvelope, err := r.signShareSubmission(pkg, contextHash, elected, ownShare) + if err != nil { + return nil, err + } + if err := r.collector.RecordShareSubmission(ownSubmission); err != nil { + return nil, fmt.Errorf("roast runner: record own share submission: %w", err) + } + r.broadcast(RunnerMsgShareSubmission, contextHash, ownSubmissionEnvelope) + shares[r.member] = ownShare } - r.broadcast(RunnerMsgShareSubmission, contextHash, ownSubmissionEnvelope) - // 7. Collect a share from every signer in the package (own already in), as - // inner FROST share bytes. The package was built from the whole responsive - // included set, so the aggregate needs a share from each of them (silent- - // member subsetting is the retry path's concern, not the happy flow). - shares := map[group.MemberIndex][]byte{r.member: ownShare} - if err := r.collectShares(ctx, r.sub.Shares(), contextHash, includedSet, shares); err != nil { + // 8. Collect a round-2 share from every member in the chosen signing subset (a + // signer already has its own; an observer collects all t), as inner FROST + // share bytes. A subset signer that never broadcasts a share stalls the + // attempt to the ctx deadline -> fail -> the existing ROAST retry path. + if err := r.collectShares(ctx, r.sub.Shares(), contextHash, signerSet, shares); err != nil { return nil, fmt.Errorf("roast runner: collect shares: %w", err) } - // 8. Aggregate. A share-verification failure surfaces the typed error with - // candidate culprits for the (separate) blame path. + // 9. Aggregate the subset's shares. Aggregation is a public operation over the + // package and the t broadcast shares, so signers and observers alike obtain + // the same signature; an observer aggregates against its own open session + // without having contributed a share. A share-verification failure surfaces + // the typed error with candidate culprits for the (separate) blame path. signature, err := r.engine.InteractiveAggregate( binding.SessionID(), attemptID, @@ -276,13 +331,13 @@ func (r *interactiveSigningRunner) Run(ctx context.Context) ([]byte, error) { return nil, fmt.Errorf("roast runner: aggregate: %w", err) } - // 9. Mark the attempt succeeded so the cleanup path produces no transition + // 10. Mark the attempt succeeded so the cleanup path produces no transition // bundle for a completed attempt. if err := r.coordinator.MarkSucceeded(binding.Handle()); err != nil { return nil, fmt.Errorf("roast runner: mark succeeded: %w", err) } - // Aggregation consumed the nonces and the attempt is finalized; suppress the - // deferred abort. + // The attempt is finalized; the cleanup defer prunes. A signer's nonces are + // spent so it does not abort; an observer aborts via the !signedRound2 branch. succeeded = true return signature, nil } @@ -338,7 +393,13 @@ func (r *interactiveSigningRunner) obtainSigningPackage( if err != nil { return nil, fmt.Errorf("roast runner: new signing package: %w", err) } - envelope, err := r.signSigningPackage(contextHash, elected, frostPackage) + // The chosen signing subset is exactly the members whose commitments the + // package was built over (the first t responsive committers). Carry it in + // signer_ids so non-coordinators learn which members to await shares from and + // which committed members are observers. The FROST SigningPackageBytes is the + // cryptographic source of truth, so this is a liveness hint, never blame. + signerIDs := sortedMemberIndices(commitments) + envelope, err := r.signSigningPackage(contextHash, elected, signerIDs, frostPackage) if err != nil { return nil, err } @@ -349,12 +410,14 @@ func (r *interactiveSigningRunner) obtainSigningPackage( func (r *interactiveSigningRunner) signSigningPackage( contextHash [attempt.MessageDigestLength]byte, elected group.MemberIndex, + signerIDs []group.MemberIndex, frostPackage []byte, ) ([]byte, error) { pkg := &roast.SigningPackage{ AttemptContextHash: append([]byte(nil), contextHash[:]...), CoordinatorIDValue: uint32(elected), SigningPackageBytes: frostPackage, + SignerIDsValue: memberIndicesToUint32(signerIDs), } if root := r.attempt.TaprootMerkleRoot(); root != nil { pkg.TaprootMerkleRoot = append([]byte(nil), root[:]...) @@ -408,8 +471,14 @@ func (r *interactiveSigningRunner) signShareSubmission( return sub, envelope, nil } -// collectCommitments fills `into` with every included member's commitments -// (own already seeded), taking the first body per sender. +// collectCommitments (coordinator only) fills `into` with the first t responsive +// committers' round-1 commitments - the coordinator's own already seeded, then +// the first t-1 included peers to arrive - and STOPS at t (r.threshold). The +// coordinator builds the FROST package over exactly this t-subset, so a member +// past the t-th to commit (slow or offline) never stalls the attempt: collection +// proceeds the instant t have committed. If ctx expires first (fewer than t ever +// commit), the run fails into the existing ROAST retry path. t <= len(included) +// always, so the loop terminates once t included members have committed. func (r *interactiveSigningRunner) collectCommitments( ctx context.Context, stream <-chan RunnerMessage, @@ -418,7 +487,7 @@ func (r *interactiveSigningRunner) collectCommitments( into map[group.MemberIndex][]byte, ) error { included := setOf(includedSet) - for len(into) < len(included) { + for len(into) < int(r.threshold) { select { case <-ctx.Done(): return ctx.Err() @@ -443,24 +512,28 @@ func (r *interactiveSigningRunner) collectCommitments( return nil } -// collectShares fills `into` with each included member's inner FROST share -// bytes (own already seeded), counting the first accepted body per sender, and -// retains EVERY well-formed share in the collector - including a sender's later, -// body-different ones - which is where member equivocation is detected. +// collectShares fills `into` with each share from the chosen signing subset +// (`signerSet`) as inner FROST share bytes - a signer's own already seeded, an +// observer's none - counting the first accepted body per sender, and retains +// EVERY well-formed share in the collector (including a sender's later, +// body-different ones) which is where member equivocation is detected. It +// collects over the signer set, NOT the full included set: a committed member +// the coordinator did not choose is an observer that contributes no share, so +// waiting for it would stall every attempt that omitted an offline member. func (r *interactiveSigningRunner) collectShares( ctx context.Context, stream <-chan RunnerMessage, contextHash [attempt.MessageDigestLength]byte, - includedSet []group.MemberIndex, + signerSet []group.MemberIndex, into map[group.MemberIndex][]byte, ) error { - included := setOf(includedSet) - for len(into) < len(included) { + signers := setOf(signerSet) + for len(into) < len(signers) { select { case <-ctx.Done(): return ctx.Err() case msg := <-stream: - r.recordShareMessage(msg, contextHash, included, into) + r.recordShareMessage(msg, contextHash, signers, into) } } // `into` is full, but the slot-filling share may have body-different @@ -472,7 +545,7 @@ func (r *interactiveSigningRunner) collectShares( for i, n := 0, len(stream); i < n; i++ { select { case msg := <-stream: - r.recordShareMessage(msg, contextHash, included, into) + r.recordShareMessage(msg, contextHash, signers, into) default: return nil } @@ -491,13 +564,15 @@ func (r *interactiveSigningRunner) collectShares( func (r *interactiveSigningRunner) recordShareMessage( msg RunnerMessage, contextHash [attempt.MessageDigestLength]byte, - included map[group.MemberIndex]struct{}, + signers map[group.MemberIndex]struct{}, into map[group.MemberIndex][]byte, ) { if msg.Attempt != contextHash { return } - if _, want := included[msg.Sender]; !want { + // Only shares from the chosen signing subset count toward this aggregate; a + // share from a committed-but-unchosen observer (or any non-signer) is ignored. + if _, want := signers[msg.Sender]; !want { return } var sub roast.ShareSubmission @@ -684,3 +759,27 @@ func setOf(members []group.MemberIndex) map[group.MemberIndex]struct{} { } return out } + +// sortedMemberIndices returns the keys of a member-indexed map in ascending +// order - the chosen signing subset the coordinator carries in the package's +// signer_ids, which SigningPackage.Validate requires strictly ascending (the map +// keys are distinct, so sorting yields a strictly-ascending list). +func sortedMemberIndices(m map[group.MemberIndex][]byte) []group.MemberIndex { + out := make([]group.MemberIndex, 0, len(m)) + for member := range m { + out = append(out, member) + } + sort.Slice(out, func(i, j int) bool { return out[i] < out[j] }) + return out +} + +// memberIndicesToUint32 widens an ascending member-index slice to the wire +// (uint32) form SigningPackage.SignerIDsValue carries. Widening uint8 -> +// uint32 is lossless, and ascending/distinct order is preserved. +func memberIndicesToUint32(members []group.MemberIndex) []uint32 { + out := make([]uint32, 0, len(members)) + for _, member := range members { + out = append(out, uint32(member)) + } + return out +} diff --git a/pkg/frost/signing/roast_runner_frost_native_test.go b/pkg/frost/signing/roast_runner_frost_native_test.go index bedcf37ec6..2858ddd432 100644 --- a/pkg/frost/signing/roast_runner_frost_native_test.go +++ b/pkg/frost/signing/roast_runner_frost_native_test.go @@ -127,10 +127,150 @@ func (h harness) runAndAssertAllSucceed(t *testing.T) { } } +// The happy path over an oversized group (group size 3, threshold 2): the +// coordinator finalizes over the first t responsive committers and the remaining +// committed member is an observer that aggregates the subset's broadcast shares. +// Which member observes depends on bus timing, but every node obtains the +// signature and reaches Succeeded. The deterministic subset/observer dynamics are +// pinned in TestInteractiveSigningRunner_OversizedAllOnline_FinalizesOverThreshold +// and the offline/observer tests below. +// runMembers runs only the given members' runners concurrently (the rest stay +// offline / non-responsive, never committing) and returns each member's +// signature and error keyed by member index. +func (h harness) runMembers(t *testing.T, members []group.MemberIndex) (map[group.MemberIndex][]byte, map[group.MemberIndex]error) { + t.Helper() + runCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + var mu sync.Mutex + sigs := map[group.MemberIndex][]byte{} + errs := map[group.MemberIndex]error{} + var wg sync.WaitGroup + for _, m := range members { + wg.Add(1) + go func(member group.MemberIndex) { + defer wg.Done() + sig, err := h.runners[member-1].Run(runCtx) + mu.Lock() + sigs[member] = sig + errs[member] = err + mu.Unlock() + }(m) + } + wg.Wait() + return sigs, errs +} + func TestInteractiveSigningRunner_HappyPath(t *testing.T) { buildInteractiveSigningHarness(t, 3, 2).runAndAssertAllSucceed(t) } +// A non-responsive (offline) included member must NOT stall the attempt: the +// coordinator finalizes over the first t responsive committers. Group size 3, +// threshold 2, one NON-COORDINATOR member never runs (never commits), so the two +// responsive members - the coordinator plus one peer - finalize over the t-subset +// and both succeed, and the coordinator's package omits the offline member. +func TestInteractiveSigningRunner_FinalizesOverResponsiveThresholdSubset(t *testing.T) { + h := buildInteractiveSigningHarness(t, 3, 2) + elected := h.runners[0].attempt.ElectedCoordinator() + + var offline group.MemberIndex + for _, m := range h.includedSet { + if m != elected { + offline = m + break + } + } + online := make([]group.MemberIndex, 0, 2) + for _, m := range h.includedSet { + if m != offline { + online = append(online, m) + } + } + + sigs, errs := h.runMembers(t, online) + for _, m := range online { + if errs[m] != nil { + t.Fatalf("online member %d failed: %v", m, errs[m]) + } + if string(sigs[m]) != "fake-bip340-signature" { + t.Fatalf("online member %d unexpected signature: %q", m, sigs[m]) + } + state, err := h.coords[m-1].State(h.handles[m-1]) + if err != nil { + t.Fatalf("member %d state: %v", m, err) + } + if state != roast.AttemptStateSucceeded { + t.Fatalf("member %d: expected Succeeded, got %v", m, state) + } + } + + // The coordinator built the package over exactly the t responsive committers; + // the offline member is absent from the signing subset. + coordCommitments := h.engines[elected-1].newPackageCommitments() + if len(coordCommitments) != 2 { + t.Fatalf("coordinator built package over %d commitments, want t=2", len(coordCommitments)) + } + gotIDs := map[string]struct{}{} + for _, c := range coordCommitments { + gotIDs[c.Identifier] = struct{}{} + } + if _, present := gotIDs[fmt.Sprintf("frost-id-%d", offline)]; present { + t.Fatalf("offline member %d was included in the signing subset: %v", offline, gotIDs) + } + for _, m := range online { + if _, present := gotIDs[fmt.Sprintf("frost-id-%d", m)]; !present { + t.Fatalf("online member %d missing from the signing subset: %v", m, gotIDs) + } + } +} + +// An oversized all-online group (size 3, threshold 2): every member commits, the +// coordinator finalizes over exactly t of them, and the remaining committed +// member observes. All three obtain the signature and reach Succeeded; exactly t +// members sign round 2 and the n-t observers abort their unconsumed round-1 +// nonces. The coordinator's broadcast package names exactly t ascending signers. +func TestInteractiveSigningRunner_OversizedAllOnline_FinalizesOverThreshold(t *testing.T) { + const n, threshold = 3, 2 + h := buildInteractiveSigningHarness(t, n, threshold) + + // A sniffer subscribed before any Run captures the coordinator's broadcast + // package so the test can inspect its signer_ids. + sniffer := h.bus.Subscribe() + elected := h.runners[0].attempt.ElectedCoordinator() + + h.runAndAssertAllSucceed(t) + + totalRound2, totalAbort := 0, 0 + for _, e := range h.engines { + totalRound2 += e.round2CallCount() + totalAbort += e.abortCallCount() + } + if totalRound2 != threshold { + t.Fatalf("expected exactly t=%d round-2 signers, got %d", threshold, totalRound2) + } + // The n-t observers each abort their unconsumed round-1 nonces; signers do not. + if totalAbort != n-threshold { + t.Fatalf("expected %d observer aborts, got %d", n-threshold, totalAbort) + } + + signerIDs := captureCoordinatorSignerIDs(t, sniffer, elected, h.contextHash) + if len(signerIDs) != threshold { + t.Fatalf("package signer_ids = %v, want exactly t=%d", signerIDs, threshold) + } + for i := 1; i < len(signerIDs); i++ { + if signerIDs[i] <= signerIDs[i-1] { + t.Fatalf("signer_ids not strictly ascending: %v", signerIDs) + } + } + includedSetMap := setOf(h.includedSet) + for _, id := range signerIDs { + if _, ok := includedSetMap[id]; !ok { + t.Fatalf("signer id %d not in included set %v", id, h.includedSet) + } + } +} + // A concluded attempt must leave no retained round-2 collector state, per the // collector's prune-on-conclusion contract (else a long-lived collector reused // across attempts accumulates every attempt's envelopes). The collector exposes @@ -267,8 +407,9 @@ func TestInteractiveSigningRunner_AbortsNativeAttemptOnEarlyExit(t *testing.T) { t.Fatalf("runner: %v", err) } - // No second node ever broadcasts, so Run blocks in collectCommitments until - // the short deadline fires - an early exit with the session already open. + // No second node ever broadcasts, so Run blocks waiting for round 1 to reach + // the threshold (as coordinator) or for the coordinator's package (otherwise) + // until the short deadline fires - an early exit with the session already open. runCtx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond) defer cancel() if _, err := runner.Run(runCtx); err == nil { @@ -325,17 +466,19 @@ func TestInteractiveSigningRunner_RejectsEngineCoordinatorMismatch(t *testing.T) } } -// The happy path must key signing-package commitments and aggregate shares by the -// engine-derived FROST identifiers, not a Go-fabricated placeholder. +// The happy path must key aggregate shares by the engine-derived FROST +// identifiers, not a Go-fabricated placeholder. A full-included attempt +// (threshold == group size) has every member sign, so the aggregate carries +// every member's engine-derived identifier (no observer subsetting to vary it). func TestInteractiveSigningRunner_UsesEngineDerivedFrostIdentifiers(t *testing.T) { - h := buildInteractiveSigningHarness(t, 3, 2) + h := buildInteractiveSigningHarness(t, 2, 2) h.runAndAssertAllSucceed(t) got := map[string]struct{}{} for _, share := range h.engines[0].lastAggregateShares { got[share.Identifier] = struct{}{} } - for _, want := range []string{"frost-id-1", "frost-id-2", "frost-id-3"} { + for _, want := range []string{"frost-id-1", "frost-id-2"} { if _, ok := got[want]; !ok { t.Fatalf("aggregate missing engine-derived identifier %q; got %v", want, got) } @@ -837,6 +980,10 @@ func TestNewInteractiveSigningRunner_RejectsInvalidConstruction(t *testing.T) { "member not included": func() (*interactiveSigningRunner, error) { return newInteractiveSigningRunner(ara, 99, 2, engine, collector, coord, signer, bus) }, + "threshold exceeds included set": func() (*interactiveSigningRunner, error) { + // included set has 3 members; a threshold of 4 can never form a subset. + return newInteractiveSigningRunner(ara, 1, 4, engine, collector, coord, signer, bus) + }, } for name, build := range tests { t.Run(name, func(t *testing.T) { @@ -846,3 +993,181 @@ func TestNewInteractiveSigningRunner_RejectsInvalidConstruction(t *testing.T) { }) } } + +// An OBSERVER - a member that committed in round 1 but is NOT in the package's +// signer set - must aggregate the subset's broadcast shares to obtain the +// signature, mark its attempt Succeeded, and return the signature WITHOUT running +// round 2; and even on success it must abort the engine session to drop its +// unconsumed round-1 nonces (success otherwise suppresses the abort, leaking +// them). Driven deterministically: the coordinator's signed package (signer_ids +// excluding this member) and the signers' shares are pre-injected on the bus. +func TestInteractiveSigningRunner_ObserverAggregatesAndAbortsWithoutSigning(t *testing.T) { + included := []group.MemberIndex{1, 2, 3} + dkgKey := []byte{0x01, 0x02} + ctx, err := attempt.NewAttemptContext( + "session-1", "key-group-1", dkgKey, + [attempt.MessageDigestLength]byte{0x42}, 0, included, nil, + ) + if err != nil { + t.Fatalf("attempt context: %v", err) + } + signer := fixedTestSigner{} + verifier := roast.NoOpSignatureVerifier() + bus := NewInProcessRunnerBus(16) + contextHash := ctx.Hash() + + // The election is deterministic for the attempt; probe it so the observer is a + // NON-coordinator (a coordinator is always a signer) and the signer subset is + // the other two members. + probe := roast.NewInMemoryCoordinatorWithSigning(1, signer, verifier) + probeHandle, err := probe.BeginAttempt(ctx) + if err != nil { + t.Fatalf("probe begin: %v", err) + } + probeAttempt, err := NewActiveRoastAttempt(probe, probeHandle, ctx, "session-1", nil, dkgKey) + if err != nil { + t.Fatalf("probe active attempt: %v", err) + } + elected := probeAttempt.ElectedCoordinator() + + var observer group.MemberIndex + for _, m := range included { + if m != elected { + observer = m + break + } + } + // included is ascending, so iterating it (skipping the observer) yields an + // ascending signer subset - what signer_ids requires. + signers := make([]group.MemberIndex, 0, 2) + for _, m := range included { + if m != observer { + signers = append(signers, m) + } + } + + coord := roast.NewInMemoryCoordinatorWithSigning(observer, signer, verifier) + handle, err := coord.BeginAttempt(ctx) + if err != nil { + t.Fatalf("begin attempt: %v", err) + } + ara, err := NewActiveRoastAttempt(coord, handle, ctx, "session-1", nil, dkgKey) + if err != nil { + t.Fatalf("active attempt: %v", err) + } + engine := newFakeInteractiveSigningEngine() + engine.coordinatorIdentifier = uint16(ara.ElectedCoordinator()) + runner, err := newInteractiveSigningRunner( + ara, observer, 2, engine, roast.NewRound2Collector(verifier), coord, signer, bus, + ) + if err != nil { + t.Fatalf("runner: %v", err) + } + + // Pre-inject the coordinator's signed package (signer_ids = the two signers, + // excluding the observer) plus both signers' shares, so the observer's await + // and share collection complete from the bus. + pkgEnvelope, pkgBodyHash := craftSigningPackageWithSigners( + t, contextHash, elected, []byte("fake-signing-package"), signers, signer, + ) + bus.Broadcast(RunnerMessage{Type: RunnerMsgSigningPackage, Sender: elected, Attempt: contextHash, Payload: pkgEnvelope}) + for _, s := range signers { + shareEnvelope := craftShareSubmission( + t, contextHash, s, elected, pkgBodyHash, []byte(fmt.Sprintf("share-%d", s)), signer, + ) + bus.Broadcast(RunnerMessage{Type: RunnerMsgShareSubmission, Sender: s, Attempt: contextHash, Payload: shareEnvelope}) + } + + runCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + sig, err := runner.Run(runCtx) + if err != nil { + t.Fatalf("observer run failed: %v", err) + } + if string(sig) != "fake-bip340-signature" { + t.Fatalf("observer unexpected signature: %q", sig) + } + if engine.round2CallCount() != 0 { + t.Fatalf("observer ran round 2 %d times, want 0 (it is not a signer)", engine.round2CallCount()) + } + if engine.abortCallCount() != 1 { + t.Fatalf("observer aborted %d times, want exactly 1 (drop unconsumed round-1 nonces on success)", engine.abortCallCount()) + } + state, err := coord.State(handle) + if err != nil { + t.Fatalf("observer state: %v", err) + } + if state != roast.AttemptStateSucceeded { + t.Fatalf("observer expected Succeeded, got %v", state) + } +} + +// captureCoordinatorSignerIDs drains a sniffer subscriber's signing-package +// stream for the elected coordinator's package for the attempt and returns its +// signer_ids. It fails the test if no such package was observed. +func captureCoordinatorSignerIDs( + t *testing.T, + sniffer *RunnerBusSubscriber, + elected group.MemberIndex, + contextHash [attempt.MessageDigestLength]byte, +) []group.MemberIndex { + t.Helper() + for { + select { + case msg := <-sniffer.SigningPackages(): + if msg.Sender != elected || msg.Attempt != contextHash { + continue + } + pkg := &roast.SigningPackage{} + if err := pkg.Unmarshal(msg.Payload); err != nil { + t.Fatalf("unmarshal coordinator package: %v", err) + } + return pkg.SignerIDs() + default: + t.Fatal("coordinator package not observed on the bus") + return nil + } + } +} + +// craftSigningPackageWithSigners builds a coordinator-signed package carrying an +// explicit signer subset (signer_ids, which must be ascending/distinct), and +// returns its envelope and body hash. +func craftSigningPackageWithSigners( + t *testing.T, + contextHash [attempt.MessageDigestLength]byte, + elected group.MemberIndex, + body []byte, + signers []group.MemberIndex, + signer roast.Signer, +) ([]byte, [32]byte) { + t.Helper() + signerIDs := make([]uint32, 0, len(signers)) + for _, m := range signers { + signerIDs = append(signerIDs, uint32(m)) + } + pkg := &roast.SigningPackage{ + AttemptContextHash: append([]byte(nil), contextHash[:]...), + CoordinatorIDValue: uint32(elected), + SigningPackageBytes: append([]byte(nil), body...), + SignerIDsValue: signerIDs, + } + payload, err := pkg.SignableBytes() + if err != nil { + t.Fatalf("signing package signable bytes: %v", err) + } + sig, err := signer.Sign(payload) + if err != nil { + t.Fatalf("sign signing package: %v", err) + } + pkg.CoordinatorSignature = sig + envelope, err := pkg.Marshal() + if err != nil { + t.Fatalf("marshal signing package: %v", err) + } + bodyHash, err := pkg.BodyHash() + if err != nil { + t.Fatalf("signing package body hash: %v", err) + } + return envelope, bodyHash +} From 2d915a73ea1a1520968eefffcd6131959a0fb1a1 Mon Sep 17 00:00:00 2001 From: maclane Date: Fri, 19 Jun 2026 17:07:35 -0400 Subject: [PATCH 2/2] fix(frost): retain included non-signers' divergent shares as equivocation evidence Fold of Codex review P2 on #4093. recordShareMessage filtered by the signer set before RecordShareSubmission, so an included member that is NOT in the chosen signing subset (an observer) had its share dropped before the collector could retain it. In a targeted coordinator-equivocation case where that member was handed a different package and broadcasts a divergent share, the collector is designed to keep it as EquivocationKindDivergentShare evidence for the f+1 blame/transition path - and the signer-set filter was discarding it. Split the membership gate: retain (hand to RecordShareSubmission) any INCLUDED member's well-formed share, but only COUNT signer-set shares toward the aggregate. The collector already enforces included-membership and keeps divergent shares separately; the runner now caches the included set (includedMembers) and gates retention by it, restoring the pre-subset evidence surface while keeping t-of-included counting. Added TestInteractiveSigningRunner_RetainsObserverDivergentShareAsEvidence: an included observer's divergent share is retained (EquivocationKindDivergentShare emitted) but not counted. Gemini approved the PR; this addresses the sole Codex finding. Co-Authored-By: Claude Opus 4.8 --- .../signing/roast_runner_frost_native.go | 70 ++++++++++++------- .../signing/roast_runner_frost_native_test.go | 46 ++++++++++++ 2 files changed, 92 insertions(+), 24 deletions(-) diff --git a/pkg/frost/signing/roast_runner_frost_native.go b/pkg/frost/signing/roast_runner_frost_native.go index 1eca316c54..fd4d87d4ac 100644 --- a/pkg/frost/signing/roast_runner_frost_native.go +++ b/pkg/frost/signing/roast_runner_frost_native.go @@ -50,11 +50,17 @@ type interactiveSigningRunner struct { // message inconsistent with the attempt it is bound to. messageDigest []byte threshold uint16 - engine interactiveSigningEngine - collector *roast.Round2Collector - coordinator roast.Coordinator - signer roast.Signer - bus RunnerBus + // includedMembers is the attempt's included set as a lookup, cached at + // construction. It gates which shares the collector retains as evidence (any + // included member's, even a non-signer observer's divergent share), distinct + // from the per-attempt signer set that gates which shares count toward the + // aggregate. + includedMembers map[group.MemberIndex]struct{} + engine interactiveSigningEngine + collector *roast.Round2Collector + coordinator roast.Coordinator + signer roast.Signer + bus RunnerBus // sub is established at construction (before any Run broadcasts) so a node // never misses a peer message broadcast before it subscribed. sub *RunnerBusSubscriber @@ -108,16 +114,17 @@ func newInteractiveSigningRunner( // digest the attempt (and its package/share envelopes) is bound to could mark // an attempt for digest A succeeded with a signature over digest B. return &interactiveSigningRunner{ - attempt: attempt, - member: member, - messageDigest: append([]byte(nil), attemptCtx.MessageDigest[:]...), - threshold: threshold, - engine: engine, - collector: collector, - coordinator: coordinator, - signer: signer, - bus: bus, - sub: bus.Subscribe(), + attempt: attempt, + member: member, + messageDigest: append([]byte(nil), attemptCtx.MessageDigest[:]...), + threshold: threshold, + includedMembers: setOf(attemptCtx.IncludedSet), + engine: engine, + collector: collector, + coordinator: coordinator, + signer: signer, + bus: bus, + sub: bus.Subscribe(), }, nil } @@ -554,13 +561,20 @@ func (r *interactiveSigningRunner) collectShares( } // recordShareMessage validates a share-submission bus message, retains it in the -// collector, and counts it toward `into` when it is the sender's first accepted -// share. Recording BEFORE the already-collected check is what lets the collector -// observe member equivocation (a body-different second signed share -> +// collector, and counts it toward `into` only when the sender is in the chosen +// signing subset and it is the sender's first accepted share. Retention is gated +// by INCLUDED-set membership, not the signer set: the collector deliberately +// keeps an included non-signer's DIVERGENT share - a targeted coordinator- +// equivocation victim that signed a different package - as +// EquivocationKindDivergentShare evidence for the f+1 blame/transition path, so +// the runner must hand any included member's well-formed share to the collector +// even though only signer-set shares count toward the aggregate. Recording BEFORE +// the already-collected check is what lets the collector observe member +// equivocation (a body-different second signed share -> // EquivocationKindShareConflict / DivergentShare); a divergent / conflicting / -// unauthenticated share is retained where applicable but never counted. -// Retention is bounded (the collector keeps the first per submitter and only -// emits on the rest), and the bus delivers only body-different duplicates. +// unauthenticated share is retained where applicable but never counted. Retention +// is bounded (the collector keeps the first per submitter and only emits on the +// rest), and the bus delivers only body-different duplicates. func (r *interactiveSigningRunner) recordShareMessage( msg RunnerMessage, contextHash [attempt.MessageDigestLength]byte, @@ -570,9 +584,11 @@ func (r *interactiveSigningRunner) recordShareMessage( if msg.Attempt != contextHash { return } - // Only shares from the chosen signing subset count toward this aggregate; a - // share from a committed-but-unchosen observer (or any non-signer) is ignored. - if _, want := signers[msg.Sender]; !want { + // Retain shares from any INCLUDED member, not just the signing subset: an + // included non-signer's divergent share is targeted-equivocation evidence the + // collector keeps. An outsider (not in the included set) is dropped here rather + // than handed to the collector, which would reject it as not-included anyway. + if _, included := r.includedMembers[msg.Sender]; !included { return } var sub roast.ShareSubmission @@ -594,6 +610,12 @@ func (r *interactiveSigningRunner) recordShareMessage( return } recordErr := r.collector.RecordShareSubmission(&sub) + // Only the chosen signing subset's shares count toward this aggregate; a + // committed-but-unchosen observer's share is retained above as evidence but + // never counted. + if _, isSigner := signers[msg.Sender]; !isSigner { + return + } if _, have := into[msg.Sender]; have { return } diff --git a/pkg/frost/signing/roast_runner_frost_native_test.go b/pkg/frost/signing/roast_runner_frost_native_test.go index 2858ddd432..7eea79b79c 100644 --- a/pkg/frost/signing/roast_runner_frost_native_test.go +++ b/pkg/frost/signing/roast_runner_frost_native_test.go @@ -1102,6 +1102,52 @@ func TestInteractiveSigningRunner_ObserverAggregatesAndAbortsWithoutSigning(t *t } } +// An included NON-SIGNER (observer) that broadcasts a DIVERGENT share - a +// targeted coordinator-equivocation victim that signed a different package - must +// still be RETAINED by the collector as EquivocationKindDivergentShare evidence +// for the f+1 blame/transition path, even though its share does not count toward +// the aggregate (it is not in the signing subset). Retention is gated by +// included-set membership, not the signer set. +func TestInteractiveSigningRunner_RetainsObserverDivergentShareAsEvidence(t *testing.T) { + included := []group.MemberIndex{1, 2, 3} + runner, collector, contextHash, elected := buildEquivocationRunner(t, included) + signer := fixedTestSigner{} + if err := collector.BeginAttempt(contextHash[:], elected, included); err != nil { + t.Fatalf("collector begin: %v", err) + } + // Authoritative package over the signing subset {1,2}; its body hash binds + // accepted shares. + authEnvelope, _ := craftSigningPackage(t, contextHash, elected, []byte("authoritative-package"), signer) + authPkg := &roast.SigningPackage{} + if err := authPkg.Unmarshal(authEnvelope); err != nil { + t.Fatalf("unmarshal authoritative: %v", err) + } + if err := collector.RecordSigningPackage(authPkg); err != nil { + t.Fatalf("record authoritative: %v", err) + } + + // Member 3 is an included observer (not in the {1,2} signing subset) but was + // handed a DIFFERENT package, so it broadcasts a share bound to that other + // package's body hash - divergent vs the authoritative package. + _, otherBodyHash := craftSigningPackage(t, contextHash, elected, []byte("equivocating-package-for-3"), signer) + divergentShare := craftShareSubmission(t, contextHash, 3, elected, otherBodyHash, []byte("share-3-divergent"), signer) + msg := RunnerMessage{Type: RunnerMsgShareSubmission, Sender: 3, Attempt: contextHash, Payload: divergentShare} + + evidence := captureEquivocationEvidence(t) + into := map[group.MemberIndex][]byte{} + // Signer set excludes member 3 (the observer); counting must skip it while + // retention must not. + runner.recordShareMessage(msg, contextHash, setOf([]group.MemberIndex{1, 2}), into) + + if _, counted := into[3]; counted { + t.Fatal("observer (non-signer) share was counted toward the aggregate") + } + got := evidence() + if len(got) != 1 || got[0].Kind != roast.EquivocationKindDivergentShare || got[0].Sender != 3 { + t.Fatalf("expected one divergent-share evidence retained from member 3, got %+v", got) + } +} + // captureCoordinatorSignerIDs drains a sniffer subscriber's signing-package // stream for the elected coordinator's package for the attempt and returns its // signer_ids. It fails the test if no such package was observed.