feat(tbtc/signer): FROST/ROAST Rust signer#4005
Conversation
Lands the Rust signer at pkg/tbtc/signer/ alongside the existing Go DKG coordinator. Mirrors the signer slice of tlabs-xyz/tbtc:feat/frost-schnorr-migration (PR #10) at frozen tag frost-extraction-source-v1. Per extraction plan v38 §3.1, the signer co-locates with keep-core because: (a) HSM enforcement is external to signer code (standard PKCS#11/KMIP client), doesn't force a separate audit lifecycle; (b) coordinator coupling dominates (B-2 DKG coordinator already lives in this repo via PR #3866); (c) TEE adoption later is reversible without forcing a repo split. Layout - pkg/tbtc/signer/ — Rust crate with own Cargo.toml - pkg/tbtc/signer/docs/ — signer + ROAST + TEE specs - pkg/tbtc/signer/docs/formal/models/ — ROAST + TEE TLA+ models - pkg/tbtc/signer/scripts/formal/ — ROAST vector + TLA runner - pkg/tbtc/signer/test/vectors/ — roast-attempt-context-v1.json Files (49 total) - 47 mirror status (Rust source, signer docs, ROAST docs, TLA models, test vector, etc.) - 2 allowlisted-divergence: - pkg/tbtc/signer/scripts/formal/check_roast_attempt_context_vectors.mjs (path normalization to signer-repo paths) - pkg/tbtc/signer/scripts/formal/run_tla_models.sh (MODELS_PATH env var refactor; default pkg/tbtc/signer/docs/formal/models/) Provenance - Source repository: tlabs-xyz/tbtc - Source branch: feat/frost-schnorr-migration - Source tag (frozen): frost-extraction-source-v1 - Source commit (H): 52389bd5cccb5daeef195671feb7ca46be6e2f37 - Source manifest: extraction/frost-extraction-source-manifest.json (manifestSha256: f7295fb738104501eb6c0c2447a42122ceb5f684c7a7c5dfb50ecb0bde3a0ea0) - Source PR(s): #425 (tbtc-signer error codes) + the full FROST migration series; complete list per source manifest's commit range over tools/tbtc-signer/ and signer-adjacent docs/scripts. Build wiring (follow-up) This PR lands the signer source code, docs, vectors, and scripts. Rust toolchain CI integration (cargo build/test/clippy/fmt as a separate CI job alongside the existing Go jobs) is a follow-up — Go workspace ignores non-Go subdirectories automatically; the cargo crate at pkg/tbtc/signer/Cargo.toml is independently buildable. Known TBD (resolved pre-merge) - 2 allowlisted-divergence files have expectedTargetSha256 = <TBD> in the source manifest. Path normalization transformations need to be applied to make the scripts run in pkg/tbtc/signer/ context; this PR ships the source verbatim, follow-up commits on this branch apply the transformations before merge. - Dual signoff required on each allowlisted-divergence entry per plan v38 §4.2 (extraction lead + canonical repo maintainer). Verification (pre-merge per plan v38 §7.2) - 47 mirror files: sha256 equality between git show frost-extraction-source-v1:<sourcePath> and this PR's content at pkg/tbtc/signer/<targetPath>. - 2 allowlisted-divergence files: sha256 == expectedTargetSha256 (recorded post-transformation in source manifest). - PR-scoped rogue-file check: git diff --name-only main...HEAD only contains files in manifest's fileMap with targetKey "signer". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces ChangesCore Signer Implementation
Admission Checking & Policy Enforcement
Benchmarking & Formal Test Vectors
TLA+ Formal Verification Models
Design Documentation & Specifications
Build Configuration & Scripts
🎯 4 (Complex) | ⏱️ ~75 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
…ripts
Per extraction plan v38 §4.4, allowlisted-divergence files require
content normalization for the canonical context. This commit applies
the transformations declared in the source manifest for the 2 signer-
side allowlisted-divergence entries.
Transformations
- pkg/tbtc/signer/scripts/formal/check_roast_attempt_context_vectors.mjs:
Rewrite vector path from
`docs/frost-migration/test-vectors/roast-attempt-context-v1.json`
to canonical signer layout
`test/vectors/roast-attempt-context-v1.json`
(both relative to rootDir which is two levels up from the script
location; in canonical context that's pkg/tbtc/signer/)
- pkg/tbtc/signer/scripts/formal/run_tla_models.sh:
Rewrite MODEL_DIR default from
`$ROOT_DIR/docs/frost-migration/formal-verification/models`
to canonical signer layout
`$ROOT_DIR/docs/formal/models`
Plus MODELS_PATH env-var override for alternate environments (CI
matrices, local dev trees). ROOT_DIR is unchanged
(`$(dirname $BASH_SOURCE)/../..` resolves to pkg/tbtc/signer/ here).
Verification
- Both files retain identical behavior to their monorepo counterparts
when invoked from canonical signer layout
- Comments added documenting the path normalization with reference back
to the source manifest's allowlisted-divergence status
Recompute expectedTargetSha256 for both entries in the source manifest
and collect dual signoff before merge per plan v38 §4.2.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (3)
pkg/tbtc/signer/src/api.rs (1)
196-202: 💤 Low valueMissing
#[serde(default, skip_serializing_if)]onscript_tree_hex.All other
Option<T>fields in this file use#[serde(default, skip_serializing_if = "Option::is_none")], butscript_tree_hexdoes not. This inconsistency means the field will serialize asnullwhen absent, rather than being omitted.Suggested fix
pub struct BuildTaprootTxRequest { pub session_id: String, pub inputs: Vec<TxInput>, pub outputs: Vec<TxOutput>, + #[serde(default, skip_serializing_if = "Option::is_none")] pub script_tree_hex: Option<String>, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/tbtc/signer/src/api.rs` around lines 196 - 202, The BuildTaprootTxRequest struct's script_tree_hex Option field lacks the serde attributes used elsewhere; update the declaration of BuildTaprootTxRequest so the script_tree_hex field is annotated with #[serde(default, skip_serializing_if = "Option::is_none")] to match other Option<T> fields (preserving Clone/Debug/Deserialize/Serialize behavior) so it is omitted from serialized output when None instead of serializing as null.pkg/tbtc/signer/src/lib.rs (1)
391-421: 💤 Low value
std::env::set_varandstd::env::remove_varare not thread-safe.These functions are unsound in multi-threaded contexts and deprecated since Rust 1.66. While the tests appear to serialize access via
lock_test_state(), this guard must be held across all env mutations and checks within a test to prevent races with parallel test threads.Current usage appears safe given the locking pattern, but this is fragile. Consider using a dedicated test configuration mechanism that doesn't rely on process-wide environment mutation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/tbtc/signer/src/lib.rs` around lines 391 - 421, EnvVarGuard's methods (EnvVarGuard::set, EnvVarGuard::unset) and its Drop rely on std::env::set_var/remove_var which are process-wide and not thread-safe; replace this pattern with a test-scoped, non-global solution such as using a crate that provides scoped environment variables (e.g., temp_env or similar) or refactor tests to accept an injected configuration object instead of mutating process env; update usages to acquire and hold the new scoped guard for the entire duration of tests that need env changes (or pass a Config struct into functions under test) and remove direct calls to std::env::set_var/remove_var and the EnvVarGuard Drop behavior to avoid races.pkg/tbtc/signer/src/bin/admission_checker.rs (1)
257-276: 💤 Low valueConsider cleaning up the temp file if rename fails.
If
fs::renamefails (e.g., cross-filesystem move or permissions issue), the temp file remains on disk. Adding a cleanup attempt in the error path would improve robustness.♻️ Proposed cleanup on error
fs::write(&tmp_path, serialized).map_err(|error| { format!( "failed to write override replay registry temp file [{}]: {error}", tmp_path.display() ) })?; - fs::rename(&tmp_path, path).map_err(|error| { - format!( - "failed to persist override replay registry [{}]: {error}", - path.display() - ) - }) + fs::rename(&tmp_path, path).map_err(|error| { + let _ = fs::remove_file(&tmp_path); // Best-effort cleanup + format!( + "failed to persist override replay registry [{}]: {error}", + path.display() + ) + }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/tbtc/signer/src/bin/admission_checker.rs` around lines 257 - 276, persist_override_replay_registry currently leaves the temporary file (tmp_path) if fs::rename fails; modify the rename error path to attempt cleanup of tmp_path before returning the error. Specifically, call fs::remove_file(&tmp_path) (ignoring or logging its result) inside the Err branch that handles the rename failure so the function still returns the original formatted error for fs::rename but also tries to remove the leftover tmp file; reference persist_override_replay_registry, path, tmp_path, and fs::rename when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/tbtc/signer/docs/formal/models/README.md`:
- Around line 32-51: Update the incorrect repository paths in the traceability
matrix of pkg/tbtc/signer/docs/formal/models/README.md so links point to the
actual implementation and docs in this repo: change references to
tools/tbtc-signer/src/engine.rs to pkg/tbtc/signer/src/engine.rs for the entries
mentioning RoastAttemptStateMachine.tla (validate_attempt_context, replay
guards) and StateKeyProviderPolicy.tla (decode_encrypted_state_envelope,
encode_encrypted_state_envelope); change
docs/frost-migration/tee-whitelisted-signer-enforcement-plan.md to
pkg/tbtc/signer/docs/tee-whitelisted-signer-enforcement-plan.md for
TeeEnforcementModes.tla; and change
docs/frost-migration/roast-phase-5-security-rollout-gates.md to
pkg/tbtc/signer/docs/roast-phase-5-security-rollout-gates.md for
RoastRolloutPolicy.tla so readers can locate the referenced code and policy
docs.
In `@pkg/tbtc/signer/docs/roast-implementation-plan.md`:
- Line 93: Update the broken doc links in
pkg/tbtc/signer/docs/roast-implementation-plan.md by replacing repo-external
paths like `docs/frost-migration/roast-phase-0-spec-freeze.md` and any
`tools/tbtc-signer/...` references with the correct repo-local paths under
pkg/tbtc/signer/docs (or use correct relative paths from this markdown file);
search the file for all occurrences of `docs/frost-migration/...` and
`tools/tbtc-signer/...` (including the instances similar to the shown
`docs/frost-migration/roast-phase-0-spec-freeze.md`) and normalize each link so
it points to the new location in this package, preserving anchor fragments and
updating any link text as needed.
In `@pkg/tbtc/signer/docs/roast-phase-5-rollout-runbook.md`:
- Around line 26-29: Update the incorrect crate path used in the runbook
commands: replace occurrences of "cd tools/tbtc-signer" with "cd
pkg/tbtc/signer" for the benchmark command (`cargo bench --features
bench-restart-hook --bench phase5_roast`) and the chaos suite script invocation
(`./scripts/run_phase5_chaos_suite.sh`) so the commands run from the correct
crate directory.
In `@pkg/tbtc/signer/docs/roast-phase-5-security-rollout-gates.md`:
- Around line 92-99: Update stale repository paths in
roast-phase-5-security-rollout-gates.md: replace any occurrences of the old
tooling path "tools/tbtc-signer" with the new location "pkg/tbtc/signer" (e.g.,
update the run command `cd tools/tbtc-signer && cargo bench --features
bench-restart-hook --bench phase5_roast` to `cd pkg/tbtc/signer ...`), and
update any links referencing
`docs/frost-migration/roast-phase-5-baseline-calibration.md` to the document’s
new location in the repo (search for the exact link text and swap to the correct
path). Ensure all instances at the reported locations (around the run command
and the listed links) are changed consistently so operators following the
runbook hit the correct files and commands.
In `@pkg/tbtc/signer/docs/rust-rewrite-bootstrap.md`:
- Around line 10-13: The documentation still references the old crate path
"tools/tbtc-signer" and the validation command using that path; update every
occurrence to "pkg/tbtc/signer" in rust-rewrite-bootstrap.md (including the
header lines that list the crate, the C ABI include path `include/frost_tbtc.h`,
and any validation/build commands) so links and commands point to the colocated
pkg/tbtc/signer location; search for "tools/tbtc-signer" and replace with
"pkg/tbtc/signer" and verify the validation command and any examples reference
the new path.
In `@pkg/tbtc/signer/docs/signer-api-contract-decision-brief.md`:
- Around line 43-47: Update the two referenced paths in
signer-api-contract-decision-brief.md so they point to the mirrored crate
locations: replace `docs/frost-migration/rust-rewrite-bootstrap.md` with
`pkg/tbtc/signer/docs/frost-migration/rust-rewrite-bootstrap.md` and replace
`tools/tbtc-signer/src/lib.rs` with `pkg/tbtc/signer/src/lib.rs` (look for the
occurrences shown around the paragraph mentioning the bootstrap Rust crate and
file: `tools/tbtc-signer/src/lib.rs` and update those strings accordingly).
In `@pkg/tbtc/signer/docs/tbtc-signer-secret-material-hardening-plan.md`:
- Around line 6-7: The doc still uses the old crate path string
`tools/tbtc-signer`; update that scope reference to `pkg/tbtc/signer` throughout
the file (tbtc-signer-secret-material-hardening-plan.md) so the plan points to
the mirrored crate location, and scan for any other occurrences of
`tools/tbtc-signer` in this document to replace with `pkg/tbtc/signer`.
In `@pkg/tbtc/signer/docs/tee-whitelisted-signer-enforcement-plan.md`:
- Around line 296-298: Update the two broken cross-doc links in
pkg/tbtc/signer/docs/tee-whitelisted-signer-enforcement-plan.md (currently
referencing docs/frost-migration/roast-phase-5-security-rollout-gates.md and
docs/frost-migration/roast-phase-5-rollout-runbook.md on lines ~296–297) to
point to their correct locations inside this PR:
pkg/tbtc/signer/docs/roast-phase-5-security-rollout-gates.md and
pkg/tbtc/signer/docs/roast-phase-5-rollout-runbook.md so cross-document
navigation resolves correctly.
In `@pkg/tbtc/signer/README.md`:
- Around line 53-54: Update the README.md occurrence(s) that reference the old
path string "tools/tbtc-signer" to the new crate location "pkg/tbtc/signer":
search for and replace that path in all command snippets, code blocks, and file
references (e.g., cargo build/cd commands and any path bullets) so every
instance uses "pkg/tbtc/signer" consistently; ensure both shell commands and
prose file paths are updated, and run a quick grep for "tools/tbtc-signer" to
confirm no remaining references.
In `@pkg/tbtc/signer/scripts/admission-policy-v1.sample.json`:
- Line 8: Replace the non-hex placeholder value for the JSON key
dao_override_trust_root_pubkey_hex with a syntactically valid 32-byte hex string
(64 lowercase hex characters) so sample files and copy/paste validation don't
break; update the sample value to something like a 64-character hex placeholder
and leave replacement guidance in the README or nearby comment explaining it
must be replaced with the real x-only pubkey hex.
In `@pkg/tbtc/signer/scripts/formal/check_roast_attempt_context_vectors.mjs`:
- Around line 15-18: vectorsPath currently resolves to
"docs/frost-migration/test-vectors/roast-attempt-context-v1.json" and will fail
because the vectors live under "test/vectors"; update the path.join call that
constructs vectorsPath (using rootDir and the filename) to point to
"test/vectors/roast-attempt-context-v1.json" so the script loads the correct
file; ensure the change is made where vectorsPath is declared and used in this
module.
In `@pkg/tbtc/signer/scripts/formal/run_tla_models.sh`:
- Around line 4-5: The MODEL_DIR assignment in run_tla_models.sh is pointing to
the wrong path; update the MODEL_DIR variable (currently computed relative to
ROOT_DIR) to "$ROOT_DIR/docs/formal/models" so the directory existence check in
the script (around the directory existence test near line 20) succeeds; modify
the MODEL_DIR definition in the script (look for the MODEL_DIR variable
assignment and references) to use the corrected path and ensure any subsequent
uses of MODEL_DIR still reference this updated variable.
In `@pkg/tbtc/signer/tests/p2tr_signature_fraud_vectors.rs`:
- Around line 375-376: The test constructs vectors_path using the vectors_path
variable in pkg/tbtc/signer/tests/p2tr_signature_fraud_vectors.rs which
currently joins
"../../docs/frost-migration/test-vectors/p2tr-signature-fraud-v0.json" relative
to CARGO_MANIFEST_DIR and points to a non-existent file; fix by either adding
the missing p2tr-signature-fraud-v0.json to the repo at that path or update the
vectors_path join to the correct relative path where the JSON actually lives
(adjust the "../" segments or point to the canonical test-vectors location),
ensuring the variable name vectors_path and its usage remain unchanged.
---
Nitpick comments:
In `@pkg/tbtc/signer/src/api.rs`:
- Around line 196-202: The BuildTaprootTxRequest struct's script_tree_hex Option
field lacks the serde attributes used elsewhere; update the declaration of
BuildTaprootTxRequest so the script_tree_hex field is annotated with
#[serde(default, skip_serializing_if = "Option::is_none")] to match other
Option<T> fields (preserving Clone/Debug/Deserialize/Serialize behavior) so it
is omitted from serialized output when None instead of serializing as null.
In `@pkg/tbtc/signer/src/bin/admission_checker.rs`:
- Around line 257-276: persist_override_replay_registry currently leaves the
temporary file (tmp_path) if fs::rename fails; modify the rename error path to
attempt cleanup of tmp_path before returning the error. Specifically, call
fs::remove_file(&tmp_path) (ignoring or logging its result) inside the Err
branch that handles the rename failure so the function still returns the
original formatted error for fs::rename but also tries to remove the leftover
tmp file; reference persist_override_replay_registry, path, tmp_path, and
fs::rename when making the change.
In `@pkg/tbtc/signer/src/lib.rs`:
- Around line 391-421: EnvVarGuard's methods (EnvVarGuard::set,
EnvVarGuard::unset) and its Drop rely on std::env::set_var/remove_var which are
process-wide and not thread-safe; replace this pattern with a test-scoped,
non-global solution such as using a crate that provides scoped environment
variables (e.g., temp_env or similar) or refactor tests to accept an injected
configuration object instead of mutating process env; update usages to acquire
and hold the new scoped guard for the entire duration of tests that need env
changes (or pass a Config struct into functions under test) and remove direct
calls to std::env::set_var/remove_var and the EnvVarGuard Drop behavior to avoid
races.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c0224eef-499a-42a2-a346-f06ef353278b
⛔ Files ignored due to path filters (1)
pkg/tbtc/signer/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (48)
pkg/tbtc/signer/.gitignorepkg/tbtc/signer/Cargo.tomlpkg/tbtc/signer/README.mdpkg/tbtc/signer/benches/phase5_roast.rspkg/tbtc/signer/build.shpkg/tbtc/signer/docs/formal/models/README.mdpkg/tbtc/signer/docs/formal/models/RoastAttemptStateMachine.cfgpkg/tbtc/signer/docs/formal/models/RoastAttemptStateMachine.tlapkg/tbtc/signer/docs/formal/models/RoastRolloutPolicy.cfgpkg/tbtc/signer/docs/formal/models/RoastRolloutPolicy.tlapkg/tbtc/signer/docs/formal/models/StateKeyProviderPolicy.cfgpkg/tbtc/signer/docs/formal/models/StateKeyProviderPolicy.production.cfgpkg/tbtc/signer/docs/formal/models/StateKeyProviderPolicy.tlapkg/tbtc/signer/docs/formal/models/TeeEnforcementModes.cfgpkg/tbtc/signer/docs/formal/models/TeeEnforcementModes.tlapkg/tbtc/signer/docs/permissioned-signer-hardening-rfc.mdpkg/tbtc/signer/docs/roast-implementation-plan.mdpkg/tbtc/signer/docs/roast-phase-0-spec-freeze.mdpkg/tbtc/signer/docs/roast-phase-1.5-consumed-registry-integration.mdpkg/tbtc/signer/docs/roast-phase-2-coordinator-policy-enforcement.mdpkg/tbtc/signer/docs/roast-phase-3-attempt-transcript-replay-hardening.mdpkg/tbtc/signer/docs/roast-phase-4-liveness-policy-recovery.mdpkg/tbtc/signer/docs/roast-phase-5-baseline-calibration.mdpkg/tbtc/signer/docs/roast-phase-5-rollout-runbook.mdpkg/tbtc/signer/docs/roast-phase-5-security-rollout-gates.mdpkg/tbtc/signer/docs/rust-rewrite-bootstrap.mdpkg/tbtc/signer/docs/signer-api-contract-decision-brief.mdpkg/tbtc/signer/docs/tbtc-signer-secret-material-hardening-plan.mdpkg/tbtc/signer/docs/tee-whitelisted-signer-enforcement-plan.mdpkg/tbtc/signer/docs/true-late-t-of-n-finalize-considerations.mdpkg/tbtc/signer/include/frost_tbtc.hpkg/tbtc/signer/scripts/admission-candidate.sample.jsonpkg/tbtc/signer/scripts/admission-existing.sample.jsonpkg/tbtc/signer/scripts/admission-override-registry.sample.jsonpkg/tbtc/signer/scripts/admission-override.sample.jsonpkg/tbtc/signer/scripts/admission-policy-v1.sample.jsonpkg/tbtc/signer/scripts/formal/check_roast_attempt_context_vectors.mjspkg/tbtc/signer/scripts/formal/run_tla_models.shpkg/tbtc/signer/scripts/run_phase5_chaos_suite.shpkg/tbtc/signer/src/api.rspkg/tbtc/signer/src/bin/admission_checker.rspkg/tbtc/signer/src/engine.rspkg/tbtc/signer/src/errors.rspkg/tbtc/signer/src/ffi.rspkg/tbtc/signer/src/go_math_rand.rspkg/tbtc/signer/src/lib.rspkg/tbtc/signer/test/vectors/roast-attempt-context-v1.jsonpkg/tbtc/signer/tests/p2tr_signature_fraud_vectors.rs
Adds a focused workflow that runs the Rust signer's formal-invariant test suite + TLA model checks. Moved from threshold-network/tbtc-v2/.github/workflows/ci-formal-verification.yml (jobs `signer-formal-invariants` + `tla-model-checks`) per extraction plan v38 §3.1 — the signer code lives here at pkg/tbtc/signer/, not in tbtc-v2, so the CI jobs that exercise it belong here too. Jobs - signer-formal-invariants: cargo test --manifest-path pkg/tbtc/signer/ Cargo.toml formal_verification_ (filter to formal-only cases) - tla-model-checks: pkg/tbtc/signer/scripts/formal/run_tla_models.sh (iterates over .cfg files in pkg/tbtc/signer/docs/formal/models/ and runs TLC against each; MODELS_PATH env var allows override per the path-normalization commit b84b574c on this branch) Triggers - pull_request on pkg/tbtc/signer/** changes + this workflow file - schedule nightly at 05:23 UTC (mirrors monorepo's pattern of running formal invariants both on PRs and nightly) - workflow_dispatch for manual runs Related changes in companion PR threshold-network/tbtc-v2#971: - Removed these jobs from canonical tbtc-v2's ci-formal-verification.yml - Added a comment in that file pointing here Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/tbtc-signer-formal.yml:
- Around line 25-26: The checkout steps using actions/checkout@v4 persist git
credentials by default; update each Checkout step (the uses: actions/checkout@v4
entries) to add with: persist-credentials: false so credentials are not stored
in the runner after checkout. Ensure both occurrences of actions/checkout@v4 in
the workflow are modified accordingly.
- Line 26: Update the GitHub Actions workflow to pin action versions and harden
checkout credentials: replace occurrences of actions/checkout@v4 with
actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 and add with:
persist-credentials: false to both checkout steps that use checkout, replace
dtolnay/rust-toolchain@stable with
dtolnay/rust-toolchain@29eef336d9b2848a0b548edc03f92a220660cdb8, and replace
actions/setup-java@v4 with
actions/setup-java@c1e323688fd81a25caa38c78aa6df2d33d3e20d9 so the workflow pins
SHA-based commits and disables persisting credentials on checkout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 88553e79-6db4-4e95-98fe-00c56e1bc640
📒 Files selected for processing (1)
.github/workflows/tbtc-signer-formal.yml
Resolves CI failures on PR #4005 (signer mirror): 1. TLA model checks: run_tla_models.sh lacked executable bit at canonical HEAD. CI ran the script directly (no `bash` prefix), which fails with `Permission denied`. Fixed via `git update-index --chmod=+x`. 2. Signer formal invariants: engine.rs's formal_verification_roast_attempt_context_shared_vectors_match_ expected_values test referenced vectors at a path stale from the umbrella's docs/frost-migration/test-vectors/ layout. The manifest places the vector at the canonical-signer test/vectors/ subdir (pkg/tbtc/signer/test/vectors/roast-attempt-context-v1.json per the source-to-target map). Updated the `PathBuf::from(env!("CARGO_MANIFEST_DIR")).join(...)` argument from `../../docs/frost-migration/test-vectors/roast-attempt-context-v1.json` (umbrella-relative) to `test/vectors/roast-attempt-context-v1.json` (signer-CARGO_MANIFEST_DIR-relative, where the vector actually lives at canonical HEAD). Verified locally: - ls -l shows executable bit set on run_tla_models.sh - engine.rs path now resolves to the correct mirror location - Vector exists at pkg/tbtc/signer/test/vectors/roast-attempt-context-v1.json Same fix needs to be applied to PR #4007 (stacked on #4005) in a follow-up commit on its branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR #4005 signer formal invariants test formal_verification_p2tr_signature_fraud_vectors_match_bitcoin_crate was failing because: 1. The umbrella source manifest only mapped p2tr-signature-fraud-v0.json to the tbtc-v2 target (docs/test-vectors/). It was not mirrored to the keep-core (signer) target. 2. tests/p2tr_signature_fraud_vectors.rs referenced the vector at the umbrella-relative path `../../docs/frost-migration/test-vectors/p2tr-signature-fraud-v0.json` (CARGO_MANIFEST_DIR-relative -> repo-root + docs/frost-migration/...). That directory does not exist on canonical keep-core. This is a structural omission in the manifest, not a divergence: the cross-language vector test exists in both Solidity (tbtc-v2 side) and Rust (signer side), and the vector is genuinely needed in both places to verify cross-implementation consistency. Fix - Mirror p2tr-signature-fraud-v0.json (598 lines, byte-identical content from tbtc-v2 mirror at docs/test-vectors/) to pkg/tbtc/signer/test/vectors/p2tr-signature-fraud-v0.json. - Update the test path in tests/p2tr_signature_fraud_vectors.rs from `../../docs/frost-migration/test-vectors/p2tr-signature-fraud-v0.json` to `test/vectors/p2tr-signature-fraud-v0.json` (CARGO_MANIFEST_DIR = pkg/tbtc/signer/, so test/vectors/... resolves correctly). Two stale comment references remain in - pkg/tbtc/signer/docs/roast-implementation-plan.md:265 - pkg/tbtc/signer/scripts/formal/check_roast_attempt_context_vectors.mjs:19 Both are comment-only doc pointers to the source layout; they do not affect runtime. Left as-is to preserve the umbrella -> canonical provenance trail. Manifest update follows in a stacked PR on tlabs-xyz/tbtc#10: - Add p2tr-signature-fraud-v0.json -> signer target mapping (test/vectors/p2tr-signature-fraud-v0.json). - Reclassify tests/p2tr_signature_fraud_vectors.rs from mirror to allowlisted-divergence (path differs from umbrella). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Stacked on extraction/frost-signer-mirror-2026-05-26 / PR #4005. Adds optional taproot_merkle_root_hex to start/finalize signing rounds, binds it into request fingerprints and round IDs, signs and aggregates with frost-secp256k1-tr Taproot tweaks, and verifies tweaked aggregates in tests. Verification: cargo test in pkg/tbtc/signer.
…ntine recheck)
Codex re-review of the signer FFI surface:
1. [P1] Secret request fields not zeroized (api.rs): SignShareRequest.nonces_hex
and key_package_hex deserialized into plain Strings dropped without
zeroization, retaining the one-time nonce / private key package in freed heap.
Wrap both in Zeroizing<String> (wiped on drop) + a redacting Debug so the
secrets no longer leak via {:?} either.
2. [P1] Panic hook defeats payload redaction (ffi.rs): catch_unwind does not
suppress Rust's default panic hook, so a panic payload (path/config/secret)
prints to stderr before being converted to the redacted ErrorResponse. Install
a panic hook at the FFI boundary that redacts the payload outside development.
3. [P2] Quarantine not rechecked on cached-round reuse (signing.rs): the matched-
fingerprint reuse branch returned a fresh share without the new-round path's
enforce_not_quarantined_identifiers check, so a participant quarantined after
the round opened could still obtain a share. Apply the check on reuse too.
Verified: cargo test (301 passed) + clippy -D warnings + fmt clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…pty history Codex re-review: legacy state written before refresh_history existed can carry refresh_request_fingerprint/refresh_result with an EMPTY refresh_history. The fingerprint backfill used refresh_history.last_mut(), which is None for empty history, so the previous fingerprint was not recorded and a delayed retry of that pre-upgrade refresh was re-executed as a new refresh. When there is no record to backfill onto, synthesize one from the cached refresh_result (prior epoch, keeping history strictly increasing) carrying the previous fingerprint, so stale retries stay rejected. Adds a regression test for the empty-history legacy shape. Verified: cargo test (302 passed) + clippy -D warnings + fmt clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…h env Two Codex-flagged issues in the non-interactive signing path: 1. start_sign_round fingerprinted and derived the round id from the raw request.message_hex string. hex::decode accepts mixed casing, so two calls carrying identical message bytes but different hex casing derived different fingerprints/round ids and a semantically identical retry was rejected as a SessionConflict. Lowercase message_hex right after it validates as hex, mirroring the interactive path (interactive.rs), so the fingerprint and derive_round_id are casing-independent. 2. The README-documented `cargo bench --features bench-restart-hook --bench phase5_roast` failed in a clean shell: ensure_benchmark_environment never set TBTC_SIGNER_PROFILE (missing -> production) or TBTC_SIGNER_STATE_ENCRYPTION_KEY_HEX (required by the default `env` state-key provider), so the first RunDkg persist aborted. Seed profile=development, provider=env, and a fixed dev key. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The prior commit lowercases message_hex before computing every canonical and legacy fingerprint. That regressed the upgrade path: when a node upgrades with an active sign round that a previous build started using mixed/uppercase message_hex, the persisted sign_request_fingerprint was computed over that original casing. The same retry then matched none of the fingerprints and fell through to SessionConflict instead of reusing the cached round. Capture the pre-canonicalization casing and, only when it differs from the lowercased form, compute the four legacy fingerprint shapes over the original casing and accept them in the cached-round match. A match migrates the stored fingerprint to the canonical lowercase form, so the compatibility cost is paid once per in-flight round. Adds a regression test that persists a mixed-case fingerprint and asserts the retry reuses the round and migrates it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…d result The empty-history synthesize branch was guarded by `else if let Some(previous_result)`, so a legacy/degraded state that kept refresh_request_fingerprint but whose refresh_result deserialized to None (and whose refresh_history is empty) fell through: neither the last_mut() backfill nor the synthesize branch ran. The next refresh then overwrote the only copy of the old fingerprint, so a delayed retry of the pre-upgrade request was treated as fresh and advanced the epoch — a replay. Make the branch unconditional: synthesize a history record carrying the previous fingerprint whether or not a cached result is present. Prefer the result's epoch (when non-zero and below the new refresh) and share count; otherwise fall back to refresh_epoch-1 (min 1) and a zero share count. refresh_epoch_counter is persisted, so a prior accepted refresh implies refresh_epoch >= 2 and the fallback stays non-zero and strictly below the new record, keeping history monotonic. Adds a regression test (empty history + refresh_result=None) verified to fail against the pre-fix code (the stale retry was re-executed instead of rejected). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…riodic reshare (#4124) ## Summary Three findings from the #4005 review (the multi-agent custody review + Codex's review), fixed and verified. ### 1. [HIGH] Plaintext state fallback was accepted unconditionally — `persistence.rs` `decode_persisted_state_storage_format` fell back to parsing an **unencrypted, unauthenticated** `PersistedEngineState` whenever the bytes weren't a valid AEAD envelope — with no gate at all. The AEAD envelope is the only integrity control for state-at-rest, so anyone who could write the state file could forge it (cleared `consumed_*` replay markers → resume-into-double-sign, or an attacker `dkg_key_package` → admission bypass) **without holding the state-encryption key**. Per the secret-material hardening plan, the plaintext path is now **emergency-rollback-only**: compile-time disabled in release builds (`cfg!(debug_assertions)`), rejected in production profiles, and gated behind an explicit `TBTC_SIGNER_PERMIT_PLAINTEXT_STATE_ROLLBACK` opt-in. The two tests that load plaintext (migration + schema-mismatch) opt in, and a negative assertion pins the fail-closed default. ### 2. [Codex P1] Secret FFI buffers left unwiped on free — `ffi.rs` `free_buffer` now `zeroize`s the buffer before deallocation, so secret material returned by nonce/DKG/key-package endpoints (e.g. `frost_tbtc_generate_nonces_and_commitments`) is wiped rather than left in allocator memory when a caller forgets to clear it. (`zeroize` is a volatile wipe the optimizer can't elide.) ### 3. [Codex P2] Periodic reshare was a one-shot — `lifecycle.rs` After the first `refresh_shares`, any later reshare with updated `current_shares` had a different fingerprint and was rejected as `SessionConflict`, so a long-lived session could never advance `refresh_history`/`refresh_count`/cadence past the first refresh. A different fingerprint is now treated as a new periodic refresh; idempotent replay of the *same* fingerprint is unchanged. ## Deferred (flagged for the team) - **Signing-policy firewall production force-on** (my MEDIUM finding): mirroring the provenance gate is correct in principle, but force-enabling the firewall makes its *entire* policy config (allowed script classes, output caps, …) mandatory in production — a deployment-contract change. The verifier flagged it as needing product confirmation, so it's left as-is pending that decision. - **State-at-rest anti-rollback freshness counter** and **quarantine-reset replay-marker loss**: design-level (needs an external monotonic anchor) / inherent to that opt-in policy (default is fail-closed). ## Verification - `cargo test` — **297 passed, 0 failed**. - `cargo clippy --all-targets -- -D warnings` — **clean (exit 0)**. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…t credential persistence Resolves the zizmor/CodeRabbit findings on the signer formal-verification workflow: unpinned mutable action tags and persisted git credentials in checkout steps. All `uses:` references now pin full commit SHAs with human-readable version comments (actions/checkout v4.3.1, dtolnay/rust-toolchain stable head, EmbarkStudios/cargo-deny-action v2.0.20, actions/setup-java v4.8.0), and every checkout step sets persist-credentials: false so the ephemeral token is not written to the local git config. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ction The stateless FROST FFI primitives `generate_nonces_and_commitments` and `sign_share` in `frost_ops.rs` hand a one-time signing nonce pair out to the host and later accept it back as opaque `nonces_hex`. That leaves nonce custody -- and the single-use invariant -- entirely with the caller (the `SignShareRequest` contract even states the caller "is cryptographically responsible for single use"). A host bug or compromise that replays one `nonces_hex` across two distinct signing packages produces two Schnorr shares under the same nonce, which algebraically solves for the long-term secret share. The deterministic StartSignRound/FinalizeSignRound path is already fenced off in production by `enforce_transitional_signing_disabled_in_production`, but these stateless primitives gated only on the provenance attestation and had no production-profile guard -- a fail-closed asymmetry. A production signer could therefore be driven through the host-custody nonce path and inherit that catastrophic blast radius. Add `enforce_stateless_nonce_primitives_disabled_in_production`, mirroring the deterministic guard's style (same `signer_profile_is_production()` predicate, same `EngineError::LifecyclePolicyRejected` variant and message shape), and invoke it in both primitives immediately after the provenance gate -- before any secret key package or nonce is deserialized. Under the production profile both refuse with reason_code `stateless_nonce_primitives_disabled_in_production`; production signing must use the interactive FROST path (`interactive.rs`), where the engine keeps nonce custody and enforces durable single-use consumption markers. Non-production/test profiles retain current behavior. No current production caller reaches these paths (defense-in-depth before activation). Adds a focused test proving both primitives fail closed under the production profile with valid provenance -- feeding the same secret inputs that succeed under development -- and still work under the development profile. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codex P2 review on #4127 flagged that SHA-pinning dtolnay/rust-toolchain could make it install the SHA as a toolchain, since older versions of the action derive the default toolchain from the action ref. Verified against the pinned version (4be7066): its action.yml already defaults `toolchain` to `stable`, so the Setup Rust / formal jobs were not actually broken (they pass on this branch). Still, name the toolchain explicitly on both Setup Rust steps: it is self-documenting and independent of the pinned action's default, which is the safe form for a SHA-pinned toolchain action. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t credential persistence (#4127) ## Summary Hardens `.github/workflows/tbtc-signer-formal.yml` (introduced in #4005) against supply-chain and credential-exposure risks: - Every `actions/checkout` step now sets `persist-credentials: false`, so the ephemeral `GITHUB_TOKEN` is not persisted into the local git config after checkout. None of the jobs perform authenticated git operations after checkout, so nothing relies on the persisted credential. - Every `uses:` reference is pinned to a full 40-hex commit SHA with a trailing comment recording the human-readable version, replacing mutable tag/branch refs. No action major versions were changed — each pin is the current commit behind the ref that was already in use. This addresses the two open CodeRabbit review threads on #4005 (persisted checkout credentials + unpinned action refs on `.github/workflows/tbtc-signer-formal.yml`). ## Pinned actions | Action | Old ref | Pinned SHA (version) | | --- | --- | --- | | `actions/checkout` | `v4` | `34e114876b0b11c390a56381ad16ebd13914f8d5` (v4.3.1) | | `dtolnay/rust-toolchain` | `stable` | `4be7066ada62dd38de10e7b70166bc74ed198c30` (stable branch head) | | `EmbarkStudios/cargo-deny-action` | `v2` | `bb137d7af7e4fb67e5f82a49c4fce4fad40782fe` (v2.0.20) | | `actions/setup-java` | `v4` | `c1e323688fd81a25caa38c78aa6df2d33d3e20d9` (v4.8.0) | ## Notes - Stacked on `extraction/frost-signer-mirror-2026-05-26` (PR #4005); only the workflow file is touched. - YAML validated locally after the edit. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…e production profile (#4129) ## Finding (MEDIUM, fail-closed asymmetry — defense-in-depth) The stateless FROST FFI primitives `generate_nonces_and_commitments` and `sign_share` in `pkg/tbtc/signer/src/engine/frost_ops.rs` hand a one-time signing nonce pair out to the host and later accept it back as opaque `nonces_hex`. Nonce custody — and the single-use invariant — is left entirely with the caller. The `SignShareRequest` contract even documents this: the caller *"is cryptographically responsible for single use."* A host bug or compromise that replays one `nonces_hex` across two distinct signing packages yields two Schnorr shares under the same nonce, which algebraically solves for the long-term secret share. Catastrophic blast radius if reached. The **deterministic** StartSignRound/FinalizeSignRound path is already fenced off in production by `enforce_transitional_signing_disabled_in_production`. The **stateless** primitives, by contrast, gated only on the provenance attestation (`enforce_provenance_gate`) and had **no** production-profile guard — a fail-closed asymmetry. A production signer could therefore be driven through the host-custody nonce path and inherit that blast radius. No current production caller reaches these paths; this is defense-in-depth before activation. ## Guard added New `enforce_stateless_nonce_primitives_disabled_in_production(operation)` in `frost_ops.rs`, mirroring the deterministic guard's style: - same predicate: `signer_profile_is_production()` - same error type/shape: `EngineError::LifecyclePolicyRejected` - reason_code: `stateless_nonce_primitives_disabled_in_production` Under the production profile it fails closed (returns the error); non-production/test profiles keep current behavior. ## The two call sites Invoked immediately after `enforce_provenance_gate()?` — before any secret key package or nonce is deserialized — in: - `generate_nonces_and_commitments` (`enforce_stateless_nonce_primitives_disabled_in_production("GenerateNoncesAndCommitments")`) - `sign_share` (`enforce_stateless_nonce_primitives_disabled_in_production("SignShare")`) The deterministic path is unchanged. ## Test `engine::tests::stateless_nonce_primitives_reject_under_production_profile`: - Under the development profile, builds real key packages + nonces + signing package and confirms both primitives succeed. - Flips to the production profile (with valid provenance so the new gate — not the provenance gate — is what fires) and feeds the **same** secret inputs: both primitives return `LifecyclePolicyRejected` with reason_code `stateless_nonce_primitives_disabled_in_production`, before any secret is deserialized (the gate is the second statement of each primitive). ## Validation (from `pkg/tbtc/signer`) - `cargo fmt -- --check` — clean - `cargo clippy --all-targets -- -D warnings` — clean - `cargo test` — 319 passed (lib), 25 passed (admission_checker bin), 1 passed (p2tr integration), 0 failed 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…aterial The interactive signing path loads keys from the engine's persisted session state (dkg_key_packages by member_identifier + the public key package). The dealer run_dkg persists ALL key packages because it generates them; a REAL distributed FROST DKG runs Part1/2/3 across nodes and each node's Part3 returns only its OWN secret key package, which was previously discarded - so a distributed-DKG wallet could not sign. - Add frost_tbtc_persist_distributed_dkg_key_package (engine persist_distributed_dkg_key_package): store this node's single key package (keyed by its participant identifier) + the group public key package into the session, derive the key group from the verifying key, and persist. Idempotent for the same key group, conflicting for a different one. No production gate: this is the real distributed path, unlike the dealer run_dkg. - Interactive signing OPEN validated the included participants against dkg_key_packages, which a distributed node only has its OWN entry in. Validate against the public key package's verifying shares (the full participant set) instead - equivalent for the dealer, correct for distributed. Rust dkg unit tests pass; the rebuilt lib passes the keep-core real-cgo interactive-signing and distributed-DKG tests. Go bridge + node wiring follow. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ession persist_distributed_dkg_key_package overwrote dkg_key_packages with a single entry and returned early on a repeat call, so a multi-seat operator persisting several local seats of the SAME distributed DKG kept only the first seat's key package - the others could not open an interactive signing session. Merge each seat's key package into the session instead (same key group -> accumulate; a different key group for the session is still a conflict). A single-seat node is unchanged (one entry); this makes a multi-seat operator - and the single-process end-to-end test - able to sign with every local seat. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… unit tests Review follow-ups on the persist op: - Enforce enforce_provenance_gate() before decoding or persisting any key material - the same gate run_dkg and every interactive op enforce. The op writes signing material to durable state that interactive signing trusts after restart, so an unattested runtime (e.g. production without a valid attestation) must not be able to install distributed-DKG signing material. (Codex/review P1.) - Add engine unit tests: multi-seat key packages accumulate under one session; an identifier/participant mismatch and a conflicting key group are rejected; and the provenance gate rejects persistence when attestation is required. The test helper normalizes DKG material to even-Y exactly as dkg_part3 does. - rustfmt. Full Rust suite (303) green; the rebuilt lib keeps the keep-core distributed-DKG persist->interactive-sign e2e green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…or distributed persist (review) Three review follow-ups on the distributed-DKG persist op: - Enforce the DKG admission policy (min participants/threshold, required and allowlisted identifiers) over the participant set DERIVED from the public key package's verifying shares, the same gate run_dkg enforces. Refactor enforce_admission_policy into a shared enforce_admission_policy_for over the raw primitives so both paths use identical logic. Otherwise a caller could persist a group that omits a required participant or includes a non-allowlisted one, and interactive signing would later trust it. (Codex P1.) - Validate the key package against the group public key package before storing: matching identifier, embedded min_signers == threshold, group verifying key, and this participant's verifying share. An inconsistent package (e.g. a 3-of-N key package stored under threshold 2) previously passed persist and burned the attempt at interactive Round2 share release. (Codex P2.) - Bump the FFI ABI minor to 1 (additive symbol frost_tbtc_persist_distributed_dkg_key_package) and update the pinned ABI test, so a bridge that needs the symbol can require abi_minor >= 1 and fail-close against an older lib instead of failing at symbol lookup. (Codex P2.) Adds engine tests for the threshold mismatch and the admission rejection. Full Rust suite (304) green; the rebuilt lib keeps the keep-core distributed-DKG persist->interactive-sign e2e green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…Ds in persist (review) Three more review follow-ups on the distributed-DKG persist op: - Reject any public-key-package verifying-share identifier that does not round-trip to a u16 participant identifier. Previously filter_map silently dropped a non-canonical identifier from the admission allowlist/required checks while it still counted toward the group, so a package could carry allowed u16 members plus an extra non-allowlisted FROST identifier and still persist. A non-canonical identifier cannot be a real group member, so fail closed. (Codex P2.) - Validate participant_count against the authoritative public-package participant set. A 3-member DKG could previously be installed as participant_count=2, leaving downstream consumers of DkgResult with the wrong group size. (Codex P2.) - Clear the global admission-policy env at the end of the admission test (and before, for isolation); reset_for_tests does not clear these overrides, so the leak could make a later test exercise the wrong policy. (Codex P2.) Full Rust suite (304) green; the rebuilt lib keeps the keep-core distributed-DKG persist->interactive-sign e2e green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… public package on accumulate (review) Two more correctness fixes on the distributed-DKG persist op: - Verify the SECRET signing share derives to the key package's PUBLIC verifying share (VerifyingShare::from(signing_share) == verifying_share). The previous checks only trusted the embedded verifying share, but Round2 signs with the signing share and deserialization does not prove the scalar matches; a corrupt key package could persist with a public share matching the group while storing an unrelated signing share, then open and burn signing attempts producing shares that never verify. (Codex P2.) - On accumulate (a second local seat of the same session), require the SAME threshold, participant count, and public key package - not just the same group verifying key. Otherwise a second seat validated against a different submitted public package would leave the session's stored public material inconsistent with the newly inserted key, breaking later signing. (Codex P2.) Adds regression tests: a crafted signing/verifying-share mismatch and a same-key-group-different-shares accumulate. Full Rust suite (306) green; the rebuilt lib keeps the keep-core distributed-DKG e2e + multi-node tests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ibuted DKG (review) When auto-quarantine is configured and an operator is already quarantined, the dealer run_dkg refuses to include it (enforce_not_quarantined_identifiers), but the distributed-DKG persist path applied only admission policy. So a distributed DKG whose group includes a quarantined operator could be persisted and then trusted by later interactive signing. Run the same quarantine check over the participant set derived from the public key package before storing, honoring the DAO allowlist exactly as run_dkg does. Adds a regression test. (Codex P2.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…cross sessions The interactive signing path required the DKG key material to live under the per-signing session_id (SessionNotFound / DkgNotReady otherwise). But DKG persists under the wallet's DKG session, while interactive ROAST signing runs under a fresh per-message RoastSessionID - so a distributed-DKG wallet (signable ONLY via the interactive path; the dealer path uses coarse re-derive) could never sign: the signing session never held the material. Single-session tests missed it by persisting and signing under one id. Fix: treat DKG key material as the WALLET-level asset it is, keyed by key_group, not by session_id. InteractiveSessionOpen resolves the wallet session by key_group (its own session when co-located, else the session whose completed DKG produced the key_group), reads the key material AND the wallet-level policy gates (emergency rekey / finalization / tx-binding) from it, and creates the per-signing session on first Open - storing ONLY per-signing state there (no secret copy). The attempt context is still validated against request.session_id, so coordinator/attempt derivation is unchanged (per the Go ROAST layer's model). InteractiveAggregate and verify_share resolve the same wallet material by key_group (via the session's bound_key_group), reading per-signing completion markers from the signing session. DkgNotReady now means "no wallet key for this key_group". - state.rs: SessionState.bound_key_group (transient; set at Open, not persisted - the in-memory attempt it serves does not survive restart either). - interactive.rs: resolve_wallet_session_id helper; Open/Aggregate split wallet vs per-signing state; verify_share.rs likewise. - New cross-session regression test (persist under session A, sign under session B, same key_group -> valid BIP-340 signature, and the signing session holds NO copy of the DKG material). Updated two tests whose SessionNotFound expectation became DkgNotReady under the new semantics. Approach vetted with Codex. 308 engine tests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…session at Round2 Follow-up to the cross-session key_group resolution: InteractiveSessionOpen was updated to read the wallet-level policy gates from the resolved wallet session, but interactive_round2 - the moment this node's secret FROST share is actually released - was NOT. Round2 fed enforce_interactive_signing_gates the emergency_rekey_event and finalize_request_fingerprint from the PER-SIGNING session (request.session_id). For a distributed-DKG wallet that session is a fresh SessionState created at Open with no policy state, while emergency rekey / finalization are recorded on the WALLET (DKG) session. So the Round2 kill-switch re-check - which exists precisely to stop a rekey/finalization recorded AFTER Open - silently FAILED OPEN on the only signing path distributed-DKG wallets have: a watchtower could trigger an emergency rekey after Open and the share would still be released. Fix: at Round2, resolve the wallet session by the signing session's bound_key_group (as Open/Aggregate already do) and read emergency_rekey_event / finalize_request_fingerprint / tx_result from it. Co-located sessions resolve to themselves, so no behavior change there. New regression test interactive_round2_rechecks_gates_at_share_release_across_sessions: DKG under a wallet session, sign under a DISTINCT session, record the rekey on the wallet session -> Round2 blocks with emergency_rekey_required and does NOT consume the nonce; clearing it lets the attempt complete. (Without the fix Round2 would fail open and the test's expect_err would fail.) 309 engine tests green; Go cross-session e2e still signs. Caught by adversarial review. The refuted duplicate-key_group scan nondeterminism (P3) stays a documented non-issue: DKG yields a fresh key per run, so at most one session ever carries a given key_group. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-rekey writer CI fixes: - rustfmt: reflow interactive.rs to satisfy `cargo fmt --check` (whitespace only; the cross-session changes were not fmt-clean). - dependency audit: bump crossbeam-epoch 0.9.18 -> 0.9.20 for RUSTSEC-2026-0204 (invalid pointer deref in fmt::Pointer). Dev-only transitive dep (criterion -> rayon -> crossbeam-deque); lockfile-only, no code/API impact. Defense-in-depth (the writer-side counterpart to the Round2 gate fix): emergency rekey is a WALLET-level kill switch, and interactive readers resolve it from the wallet session by key_group. trigger_emergency_rekey keyed the event by the literal request.session_id; if a caller passed a per-signing session id (a distinct RoastSessionID bound to a wallet key) the event would land where no signing path looks. Now it resolves the target to the WALLET session by key_group (a session that already holds the DKG resolves to itself, so co-located callers are unchanged), so writer and reader can never diverge. New test trigger_emergency_rekey_on_signing_session_records_on_wallet_session. 310 engine tests green. Note: the "TLA model checks" CI failure is an unrelated infra issue - the pinned tla2tools-v1.8.0.jar checksum drifted upstream (download hash != expected); no TLA or script change in these PRs touches it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…session InteractiveSessionOpen creates the per-signing session on first Open (for a distinct RoastSessionID) via entry().or_insert(), but skipped ensure_session_insert_capacity - unlike every other session-creating path (run_dkg, persist, refresh, build_tx). So with a fresh RoastSessionID per message the registry could grow past TBTC_SIGNER_MAX_SESSIONS, and Round2 could then persist an over-limit registry that the reload path rejects (persisted_engine_state_rejects_session_registry_over_limit) - stranding the node's state. The per-member interactive cap bounds live nonces, not total sessions, so it did not cover this. Call ensure_session_insert_capacity before the insert (a reopen of an existing session is exempt, so co-located callers are unchanged). New test interactive_open_cross_session_respects_the_session_cap. 311 engine tests green. Caught by review (Codex). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…clone; refuse cross-wallet session rebinding Secret-lifetime scrubbing (completeness sweep of the sign path). serde deserializes FFI request hex into OWNED Strings, independent of the C buffer the Go caller scrubs; several held secret material and were left to drop un-wiped: - persist_distributed_dkg_key_package: zeroize request.key_package.data_hex (the secret share hex) after decode, and bind+zeroize the Copy SigningShare extracted for the verifying-share derivation check (frost SigningShare is Copy + DefaultIsZeroes, not ZeroizeOnDrop). - generate_nonces_and_commitments / sign_share: zeroize the request key_package_hex (secret share) and nonces_hex (one-time nonces) on success AND error paths. Mirrors the decoded-buffer scrubbing these ops already do. Session isolation. A per-signing session is keyed by RoastSessionID (message/root/start-block), NOT key_group, so two wallets signing the same digest at the same block on a node holding members of both can collide on one session id. InteractiveSessionOpen rebound bound_key_group unconditionally; a live member of the first wallet would then resolve the wrong wallet material at Round2/Aggregate. Now Open refuses to rebind a session to a different key group while any member is mid-signing under the current one (idle sessions and same-key-group co-signers unaffected). Test interactive_open_refuses_to_rebind_a_live_session_to_a_different_key_group. Caught by review (Codex). 312 engine tests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t request hex on all paths P1 (session key-group isolation). InteractiveSessionOpen bound/rebound a per-signing session to request.key_group without fully checking the session's existing wallet. Two gaps: (a) the rebind guard keyed on live interactive_signing entries, but that set is empty in the consumed-but-unaggregated window after a member's Round2, so a colliding Open could still rebind bound_key_group; (b) opening a session that already holds a DKG result for wallet A with request.key_group = wallet B installed B while dkg_result stayed A, and later resolution prefers dkg_result, so Round2/Aggregate/ verify_share would enforce A's policy and material while signing B's share - bypassing B's rekey/finalization gates and mis-verifying honest B shares. Both roast-session ids are message/root/block-derived, not key_group-derived, so two wallets can collide on one id. Fix: a session belongs to exactly ONE key group for its lifetime - reject an Open whose key_group differs from the session's established one (dkg_result key group if co-located, else the bound one), regardless of live members. Keeps dkg_result and bound_key_group mutually consistent so later resolution is always correct. Tests: ...refuses_to_rebind_a_live_session... and ...refuses_to_bind_through_another_wallets_dkg_session. P2 (secret request-String scrubbing on all paths). serde deserializes FFI request hex into owned Strings that hold secret material (signing share, one-time nonces). The manual scrubs landed AFTER earlier fallible checks (persist: admission/quarantine; sign_share: signing-package/nonces decode), so an early return dropped the secret un-wiped. Move each secret field into a Zeroizing holder up front (mem::take) so it is wiped on EVERY return path. Applies to persist_distributed_dkg_key_package, generate_nonces_and_commitments, and sign_share. (Per the coarse-path usage map, the latter two have no production caller today - the interactive path supersedes them - but the leak was real; closed uniformly.) Caught by review (Codex). 313 engine tests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…s a restart bound_key_group was reset to None on reload, on the reasoning that the live attempt it serves does not survive a restart anyway. But InteractiveAggregate/verify_share run AFTER a member's Round2 frees the live entry, using the durable consumed markers plus coordinator-held shares - and they resolve the wallet by key_group. For a distributed-DKG wallet the signing session has no dkg_result, so bound_key_group is the ONLY link back to the wallet DKG. A restart between Round2 (shares consumed, markers written) and InteractiveAggregate left both None -> DkgNotReady -> the collected shares are stranded and the attempt must be fully re-run. Persist bound_key_group alongside the consumed/aggregate markers (it is public - a key group id, not secret; serde(default) keeps old state loadable), and restore it on reload. This also keeps the one-key-group-per-session guard effective across a restart. Test persisted_session_state_round_trip_preserves_bound_key_group. Caught by review (Codex). 314 engine tests green; cross-session e2e green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…on_copy) CI's `cargo clippy --all-targets -- -D warnings` (Signer Rust checks) failed on lints a newer clippy enforces: - resolve_wallet_session_id used .map_or(false, |dkg| dkg.key_group == kg); switch to .is_some_and(...) (unnecessary_map_or). - the corrupt-key-package test cloned a Copy SigningShare/VerifyingKey; dereference instead (clone_on_copy). No behavior change. 314 engine tests green; clippy clean locally. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ble across sessions (#4136) ## What Makes a real **distributed FROST DKG** produce usable signing material, and makes that material signable through the interactive path — the signer half of the distributed-DKG node wiring (#4135). - **Persist op** `frost_tbtc_persist_distributed_dkg_key_package`: stores this node's own `Part3` key package (keyed by its participant identifier) + the group public key package into engine state, derives the key group from the verifying key, and **accumulates** across a node's local seats. Hardened with a provenance gate, admission policy (canonical big-endian identifiers, participant-count match), quarantine enforcement, key-package consistency (identifier / `min_signers` == threshold / verifying-key / verifying-share-from-signing-share derivation), and idempotent-vs-conflict semantics per key group. ABI bumped to **1.1** (additive symbol; the Go bridge fail-closes against an older lib). - **Wallet-keyed cross-session signing**: DKG material is a **wallet-level asset keyed by key group**, not by session id. Interactive ROAST signing runs under a fresh per-message session distinct from the DKG session, so `InteractiveSessionOpen` / `Round2` / `interactive_aggregate` / `verify_share` resolve the wallet material by key group (via the signing session's `bound_key_group`) and create the per-signing session on Open holding only per-signing state — **no secret is copied in**. Without this, distributed-DKG wallets, signable *only* via the interactive path, could never sign. - **Wallet-level policy gates** (emergency rekey / finalization / tx-binding) are read from the resolved wallet session at **both** Open and the Round2 share-release moment, so a kill switch recorded after Open still fires cross-session; `trigger_emergency_rekey` resolves the wallet session too (writer/reader symmetry). - **Robustness**: interactive Open honors the total-session cap like every other session-creating path; the included-participants check validates against the public key package (the full participant set), correct for a distributed node that caches only its own secret share. ## Test 311 engine tests, including a cross-session persist→sign round trip (persist under session A, sign under session B → valid BIP-340 signature, signing session holds no material copy), the wallet-session kill-switch re-check across sessions, the writer redirect, the session-cap enforcement on Open, and the full persist hardening (quarantine, admission, key-package consistency, accumulate). Also proven end-to-end from Go via #4135's multi-node real-transport e2e. `cargo fmt` / dependency audit clean. ## Companion The Go orchestrator, node wiring, FFI bridge, and multi-node real-transport e2e are #4135. Draft until the end-to-end path lands there. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Summary
Lands the Rust FROST/ROAST signer at
pkg/tbtc/signer/alongside the existing Go DKG coordinator. The initial import was extracted fromtlabs-xyz/tbtc:feat/frost-schnorr-migrationat frozen signed tagfrost-extraction-source-v1, then this PR applied targeted canonical-path, CI, and reviewer hardening follow-ups in the canonical keep-core repo.This is no longer a pure byte-for-byte mirror PR. The post-freeze changes are intentional canonical keep-core changes (canonical-path normalization, CI wiring, and reviewer hardening). After this PR lands,
threshold-network/keep-coreis the source of truth for the signer; the extracted monorepo tag is provenance for the initial import only.Source-PR Provenance
tlabs-xyz/tbtcfeat/frost-schnorr-migrationfrost-extraction-source-v1(signed annotated tag by maclane)52389bd5cccb5daeef195671feb7ca46be6e2f37The monorepo provenance above is used only to audit the initial import. Ongoing signer development, fixes, CI, and reviews happen in
threshold-network/keep-core.Why This Lives In keep-core
Per extraction plan v38 section 3.1, the signer co-locates with keep-core because:
Layout Introduced
CI Coverage
This PR now runs:
cargo fmt --manifest-path pkg/tbtc/signer/Cargo.toml -- --checkcargo clippy --manifest-path pkg/tbtc/signer/Cargo.toml --all-targets -- -D warningscargo test --manifest-path pkg/tbtc/signer/Cargo.tomlcargo test --manifest-path pkg/tbtc/signer/Cargo.toml formal_verification_pkg/tbtc/signer/scripts/formal/run_tla_models.shVerification
Latest local verification after review fixes:
cargo fmt --manifest-path pkg/tbtc/signer/Cargo.toml -- --checkcargo clippy --manifest-path pkg/tbtc/signer/Cargo.toml --all-targets -- -D warningsTBTC_SIGNER_STATE_PATH=/tmp/tbtc-signer-ci-state-local-4005.json cargo test --manifest-path pkg/tbtc/signer/Cargo.tomlcargo test --manifest-path pkg/tbtc/signer/Cargo.toml formal_verification_Latest GitHub CI on this branch is green, including
Signer Rust checks,Signer formal invariants, andTLA model checks.Relationship To PR #3866
PR #3866 lands the Go-side DKG coordinator. This PR lands the Rust-side signer. They are complementary protocol implementations:
PR #3866 and this PR can land independently in either order.
Plan Context
This is one of the mergeable mirror PRs comprising the FROST extraction, alongside tbtc-v2, tbtc-subgraph, and tbtc-v3-indexer mirror PRs.