Skip to content

refactor(retry): move duplicated participant-selection into a shared package#4118

Merged
mswilkison merged 2 commits into
feat/frost-schnorr-migration-scaffoldfrom
fix/dedup-retry-selection-shared-package
Jun 27, 2026
Merged

refactor(retry): move duplicated participant-selection into a shared package#4118
mswilkison merged 2 commits into
feat/frost-schnorr-migration-scaffoldfrom
fix/dedup-retry-selection-shared-package

Conversation

@mswilkison

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #3866. Eliminates a byte-for-byte duplicate of the retry participant-selection algorithm by moving it into a shared, scheme-neutral package.

pkg/frost/retry/retry.go was a byte-for-byte copy of pkg/tecdsa/retry/retry.go — two hand-synchronized copies of the security-critical retry participant-selection algorithm (EvaluateRetryParticipantsForSigning / …ForKeyGeneration). A fix applied to one and not the other would silently make ECDSA and FROST select different qualified-operator sets for the same seed.

Why a shared package (vs. one importing the other)

This selection is structurally shared across schemes: it's the scheme-agnostic "pick the base signing group from the ready members" used for the initial attempt of every signing (and for DKG). FROST's per-attempt robustness diverges only on retries, via ROAST (NextAttempt) — the initial selection does not. ROAST is a transition-from-previous-attempt function and cannot produce attempt 0, so roastSigningParticipantSelector falls back to this selection for the initial attempt (ConsumeRoastTransitionForSelection returns ErrRoastSelectionFallBackToLegacy at roastAttemptNumber == 0). So the initial selection is permanently shared — neither tecdsa nor frost should own it.

Change

  • Move the algorithm byte-for-byte (verified diff-identical, no logic change) into a new neutral package pkg/protocol/retry (package retry, alongside pkg/protocol/group).
  • Repoint the only two callers — pkg/tbtc/dkg_loop.go (DKG) and pkg/tbtc/signing_loop_legacy_selector.go (FROST initial selection) — at it (import-path change only; usage retry.X unchanged).
  • Delete pkg/tecdsa/retry and pkg/frost/retry.
  • Keep the superset test (the tecdsa copy had a TestExcludeOperatorTripletsCountsRightOperatorSeats case the frost copy lacked).
  • Update three doc comments that referenced the old path.

Verification

  • diff confirms the moved algorithm is byte-identical to the original (pure move).
  • go build ./... clean (no dangling references to the deleted packages); builds clean under -tags 'frost_native frost_tbtc_signer cgo frost_roast_retry'.
  • gofmt + go vet clean; the moved pkg/protocol/retry test passes (incl. the superset case); the full pkg/tbtc suite passes.

No behavior change — a single source eliminates the silent-drift hazard.

Found during review of #3866.

🤖 Generated with Claude Code

…package

pkg/frost/retry/retry.go was a byte-for-byte copy of pkg/tecdsa/retry/retry.go --
two hand-synchronized copies of the security-critical retry participant-selection
algorithm (EvaluateRetryParticipantsForSigning / ...ForKeyGeneration). A fix
applied to one and not the other would silently make ECDSA and FROST select
different qualified-operator sets for the same seed.

This selection is structurally shared across schemes: it is the scheme-agnostic
"pick the base signing group from the ready members" used for the initial attempt
of every signing (and for DKG). FROST's per-attempt robustness diverges only on
retries, via ROAST (NextAttempt); the initial selection does not -- ROAST is a
transition-from-previous function and cannot produce attempt 0, so the initial
selection is permanently shared.

Move the algorithm (byte-for-byte, verified identical) into a neutral package,
pkg/protocol/retry, and point both callers (pkg/tbtc/dkg_loop.go and
signing_loop_legacy_selector.go -- the only two) at it. Delete pkg/tecdsa/retry
and pkg/frost/retry. Keep the superset test (the tecdsa copy had an extra
triplet-seat-count case the frost copy lacked). No behavior change; a single
source eliminates the drift hazard.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 26, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 9ac4de89-bdac-41f5-8402-b7dcb48fabf2

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/dedup-retry-selection-shared-package

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@mswilkison mswilkison merged commit d97b8fb into feat/frost-schnorr-migration-scaffold Jun 27, 2026
17 checks passed
@mswilkison mswilkison deleted the fix/dedup-retry-selection-shared-package branch June 27, 2026 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant