feat(tbtc/signer): mirror FROST/ROAST Rust signer from tBTC monorepo#4005
Open
mswilkison wants to merge 153 commits into
Open
feat(tbtc/signer): mirror FROST/ROAST Rust signer from tBTC monorepo#4005mswilkison wants to merge 153 commits into
mswilkison wants to merge 153 commits into
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
✨ 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>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
Comment on lines
+25
to
+26
| - name: Checkout | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/tbtc-signer-formal.yml"
echo "== File exists? =="
ls -l "$FILE"
echo "== Lines 1-120 with numbers =="
nl -ba "$FILE" | sed -n '1,120p'Repository: threshold-network/keep-core
Length of output: 272
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/tbtc-signer-formal.yml"
echo "== File exists? =="
ls -l "$FILE"
echo "== Lines 1-120 with numbers =="
nl -ba "$FILE" | sed -n '1,120p'Repository: threshold-network/keep-core
Length of output: 272
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/tbtc-signer-formal.yml"
echo "== Showing lines 1-80 =="
awk 'NR>=1 && NR<=80 {printf "%4d: %s\n", NR, $0}' "$FILE"
echo "== Showing checkout-related lines (search) =="
rg -n "name:\s*Checkout|uses:\s*actions/checkout|persist-credentials" "$FILE" || trueRepository: threshold-network/keep-core
Length of output: 2302
Set persist-credentials: false on both actions/checkout@v4 steps
The current workflow checks out without disabling persisted git credentials; add with: persist-credentials: false to reduce credential exposure.
Suggested patch
- name: Checkout
uses: actions/checkout@v4
+ with:
+ persist-credentials: false
@@
- name: Checkout
uses: actions/checkout@v4
+ with:
+ persist-credentials: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: Checkout | |
| uses: actions/checkout@v4 | |
| - name: Checkout | |
| uses: actions/checkout@v4 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 25-26: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 26-26: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/tbtc-signer-formal.yml around lines 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.
| timeout-minutes: 30 | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for repo in actions/checkout actions/setup-java dtolnay/rust-toolchain; do
echo "=== $repo ==="
git ls-remote "https://github.com/${repo}.git" | head -n 5
echo
doneRepository: threshold-network/keep-core
Length of output: 535
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect workflow around the referenced lines
echo "=== Workflow file context ==="
nl -ba .github/workflows/tbtc-signer-formal.yml | sed -n '1,120p'
# 2) Resolve refs to commit SHAs (avoid SIGPIPE by capturing then slicing)
resolve_ref() {
local repo="$1" ref="$2"
local url="https://github.com/${repo}.git"
local tmp="/tmp/lsremote_${repo//\//_}_${ref//\//_}"
git ls-remote "$url" "$ref" > "$tmp" 2>/dev/null || true
echo "--- ${repo} ${ref} ---"
cat "$tmp" || true
echo
rm -f "$tmp" || true
}
echo "=== Resolve action refs to SHAs ==="
resolve_ref "actions/checkout" "refs/tags/v4"
resolve_ref "actions/checkout" "refs/heads/v4"
resolve_ref "actions/setup-java" "refs/tags/v4"
resolve_ref "actions/setup-java" "refs/heads/v4"
resolve_ref "dtolnay/rust-toolchain" "refs/heads/stable"
resolve_ref "dtolnay/rust-toolchain" "refs/tags/stable"Repository: threshold-network/keep-core
Length of output: 162
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Workflow file context (numbered) ==="
python3 - <<'PY'
from pathlib import Path
p = Path(".github/workflows/tbtc-signer-formal.yml")
lines = p.read_text().splitlines()
start, end = 1, min(len(lines), 140)
for i in range(start, end+1):
print(f"{i}\t{lines[i-1]}")
PY
echo
echo "=== Resolve action refs to commit SHAs ==="
resolve_ref() {
local repo="$1" ref="$2"
local url="https://github.com/${repo}.git"
echo "--- ${repo} ${ref} ---"
git ls-remote "$url" "$ref" || true
echo
}
resolve_ref "actions/checkout" "refs/tags/v4"
resolve_ref "actions/checkout" "refs/heads/v4"
resolve_ref "actions/setup-java" "refs/tags/v4"
resolve_ref "actions/setup-java" "refs/heads/v4"
resolve_ref "dtolnay/rust-toolchain" "refs/heads/stable"
resolve_ref "dtolnay/rust-toolchain" "refs/tags/stable"Repository: threshold-network/keep-core
Length of output: 2438
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Total lines: $(python3 - <<'PY'\nimport pathlib\np=pathlib.Path('.github/workflows/tbtc-signer-formal.yml')\nprint(len(p.read_text().splitlines()))\nPY)"
echo
echo "=== uses: occurrences ==="
rg -n "uses:" .github/workflows/tbtc-signer-formal.yml || true
echo
echo "=== persist-credentials occurrences ==="
rg -n "persist-credentials" .github/workflows/tbtc-signer-formal.yml || trueRepository: threshold-network/keep-core
Length of output: 133
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Total lines: $(python3 - <<'PY'
import pathlib
p=pathlib.Path('.github/workflows/tbtc-signer-formal.yml')
print(len(p.read_text().splitlines()))
PY)"
echo
echo "=== uses: occurrences ==="
rg -n "uses:" .github/workflows/tbtc-signer-formal.yml || true
echo
echo "=== persist-credentials occurrences ==="
rg -n "persist-credentials" .github/workflows/tbtc-signer-formal.yml || trueRepository: threshold-network/keep-core
Length of output: 424
Pin GitHub Actions to commit SHAs and harden checkout credentials.
actions/checkout@v4(lines 26, 44) ->actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5; also addwith: persist-credentials: falseto both checkout steps.dtolnay/rust-toolchain@stable(line 29) ->dtolnay/rust-toolchain@29eef336d9b2848a0b548edc03f92a220660cdb8.actions/setup-java@v4(line 47) ->actions/setup-java@c1e323688fd81a25caa38c78aa6df2d33d3e20d9.
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 25-26: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 26-26: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/tbtc-signer-formal.yml at 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.
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>
mswilkison
added a commit
that referenced
this pull request
May 26, 2026
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>
mswilkison
added a commit
that referenced
this pull request
May 26, 2026
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.
…iew gate The bootstrap doc's 'Implemented in this branch' section mixed this crate's PR with keep-core Go-side wiring tracked in separate PRs/commits. Add a scope note that the crate is standalone here (no cgo/libfrost consumer links it yet) and a mandatory production gate: any PR wiring a Go consumer must trigger a dedicated security re-review, since the custody-critical surface has so far been reviewed only as inert code.
…i-culprit e2e Review folding for PR #4062: - Codex [P2]: surface culprits as u16 Go member identifiers - the same space as excluded_member_identifiers / included_participants the Go host already keys on - not FROST go-string identifiers (which the engine reserves for raw frost-protocol artifacts fed back into frost). Drop AggregateCulprit; the error variant and ErrorResponse now carry candidate_culprits: Vec<u16>, mirroring excluded_member_identifiers (skip_serializing_if). Add frost_identifier_to_u16, the inverse of participant_identifier_to_frost_identifier (big-endian scalar). Foreign (non-u16) identifiers are dropped: they cannot be real group members. (Codex's cited spec does not exist in-repo, but the u16 convention is dominant and correct - AttemptExclusionEvidence is the precedent.) - Gemini [P3]: add interactive_aggregate_names_all_invalid_share_culprits, a real-crypto e2e where both members of a threshold-2 subset cheat and the aggregate names BOTH - proving AllCheaters end to end, not just the mapping. - The per-culprit reason field is dropped: the error code (aggregate_share_verification_failed) already names the reason, matching the AttemptExclusionEvidence idiom (struct-level reason + Vec<u16> members). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
) ## What On a failed `InteractiveAggregate`, the engine now names **every** member whose signature share did not verify, as **CANDIDATE culprits**, instead of returning an opaque `validation_error`. This is the engine-side input to the Go host's envelope-bound blame adjudication (frozen Phase 7.2b spec, §6). Builds on 7.2b-2 (the signed signing-package envelopes, #4056–#4061): the deferral comment in `interactive_aggregate` ("Attributable blame waits for the signed-package envelopes (Phase 7.2b)") is now resolved. ## How - **AllCheaters, not FirstCheater.** `frost-secp256k1-tr`'s `aggregate`/`aggregate_with_tweak` delegate to `frost_core::aggregate(.., FirstCheater)`, so a failure names only one member. This switches the call to `frost_core::aggregate_custom(.., &verification_key_package, CheaterDetection::AllCheaters)`. - `verification_key_package` is exactly the (taproot-tweaked, when a root is set) `PublicKeyPackage` the `_with_tweak` wrapper derives internally, so the call is **equivalent on the success path** (verified against the `frost-secp256k1-tr` source by review). - Cheater detection only runs **after** the aggregate signature itself fails to verify, so there is **no happy-path cost** vs FirstCheater. - `frost-core = "=3.0.0"` added as a direct, version-matched dependency (already transitive via `frost-secp256k1-tr`; no new code enters the build). - **New error variant.** `EngineError::AggregateShareVerificationFailed { session_id, attempt_id, candidate_culprits: Vec<u16> }` — code `aggregate_share_verification_failed`, recovery class `recoverable` (a fresh attempt excluding the culprits can succeed). Fail-closed: **no signature**. - **u16 Go member identifiers.** Culprits are reported as `Vec<u16>`, the same identifier space as `excluded_member_identifiers` / `included_participants` / the allowlists that the Go host already keys on (the FROST go-string form is reserved for raw frost-protocol artifacts fed back into frost). `frost_identifier_to_u16` inverts `participant_identifier_to_frost_identifier`; identifiers that don't fit a u16 are dropped (they can't be real group members). - **FFI.** `ErrorResponse` gains an additive `candidate_culprits: Vec<u16>` field, `#[serde(skip_serializing_if = "Vec::is_empty")]` — identical idiom to `excluded_member_identifiers`, omitted for every other error, so existing Go clients are unaffected. ## Architecture boundary (Q1, frozen) The list is **CANDIDATE only**. The engine verifies pure FROST shares against the group's own verifying material and **never inspects operator-signed envelopes**. A coordinator that aggregated honest shares against a *substituted* signing package or taproot root would make those honest shares fail and appear here — so the engine deliberately does not adjudicate fault. Authoritative, envelope-bound blame is the Go host's job at an **f+1 accuser quorum** (§6), consuming this candidate list (7.2b-4). ## Tests - `interactive_aggregate_rejects_invalid_share_fail_closed` (real-crypto e2e): an invalid share through the actual `aggregate_custom(AllCheaters)` tweak path yields `AggregateShareVerificationFailed` naming **exactly** the cheating member, not the honest one. - `interactive_aggregate_names_all_invalid_share_culprits` (real-crypto e2e): both members of a threshold-2 subset cheat → **both** are named, proving AllCheaters end-to-end (not FirstCheater, not truncated). - `frost_identifier_to_u16_inverts_participant_mapping`: round-trips across the low/high byte boundary (255 → 256). - `aggregate_share_verification_failed_code_message_and_culprits`: code / message / recovery class / accessor; non-aggregate errors expose no culprits. - `interactive_aggregate_produces_and_self_verifies_bip340`: success path unchanged. Full suite + `cargo fmt --check` + `cargo clippy` green. ## Review folding (Codex + Gemini) - **Codex [P2]** — culprits now use `u16` Go member ids (was FROST go-string), matching `excluded_member_identifiers`. (Codex cited a spec that does not exist in-repo, but the u16 convention is dominant and correct — `AttemptExclusionEvidence` is the precedent.) - **Gemini [P3]** — added the multi-culprit real-crypto e2e above. - Gemini independently verified the crypto equivalence against the `frost-secp256k1-tr` source, the Q1-boundary preservation, the `frost-core` dep alignment, and the FFI/serde back-compat. ## Scope / follow-ups - The coarse (non-interactive) `Aggregate` keeps its generic error — it has no attempt context; 7.2b is the interactive ROAST path. - The existing belt-and-suspenders self-verify after aggregation is retained (now redundant with `aggregate_custom`'s internal verify, but cheap defense-in-depth). - **7.2b-4 (Go):** consume `candidate_culprits` in the f+1-quorum, envelope-bound blame adjudication over the `Round2Collector` evidence. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…tion gate Upstream investigation (keep-core feat/frost-schnorr-migration-scaffold, pkg/frost/roast RFC-21) resolves the coordinator-seed grindability concern: a byzantine coordinator cannot fabricate exclusions -- NextAttempt is a deterministic, VerifyBundle-verified policy gated by an f+1 accuser quorum. The residual is that the signer's auto-quarantine is a fault-count threshold with a hex-only proof check and no accuser-corroboration, so it assumes the (not-yet-wired) Go ROAST layer establishes exclusions first. Record that as an explicit must-validate item on the consumer-activation re-review gate.
…cks Go Round2ShareVerifier) Add single-share FROST verification the Go host's Round2ShareVerifier (4a member-blame classifier) calls to re-verify a retained share against the attempt's authoritative package - the last seam in the blame stack, and the one piece that needs FROST crypto (Go has none). - engine::verify_signature_share resolves the group's public_key_package from the session's own DKG state (never the request), applies the taproot tweak EXACTLY as InteractiveAggregate (same canonicalize_taproot_merkle_root_hex + .tweak()), and calls frost_core::verify_signature_share against the tweaked verifying share + group key. By construction this matches what aggregate_custom(AllCheaters) concludes for the same share (pinned by an equivalence regression test). - Returns an explicit TRI-STATE ShareVerificationVerdict (valid/invalid/ indeterminate), per the Codex+Gemini design consult: only the engine can distinguish a member's malformed signed scalar (invalid -> blame) from a malformed package/context (indeterminate -> don't blame). Undecodable member share bytes -> invalid (self-incriminating); undecodable package / missing verifying share / session-not-ready / UnknownIdentifier -> indeterminate; Ok -> valid; InvalidSignatureShare -> invalid. - FFI export frost_tbtc_verify_signature_share + api.rs request/result. Tests: valid shares -> Valid, wrong-package share -> Invalid, and the equivalence guard (aggregate's AllCheaters culprit verdict matches verify's per-share verdicts); edges (undecodable share -> Invalid; missing member / undecodable package / unknown session -> Indeterminate). Full lib suite + fmt + clippy green. frost-core =3.0.0 (its verify_signature_share pre_commitment_aggregate fix; older rc's diverged from aggregation). Follow-up: the Go-side Round2ShareVerifier impl (scaffold) that calls this FFI; a script-path (tweaked-root) equivalence-test variant. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…aproot root
Codex+Gemini both raised two P2s on the engine verify_signature_share PR;
both confirmed valid against the mirror tree and folded:
- Declare frost_tbtc_verify_signature_share in the public C header
(include/frost_tbtc.h) alongside the other frost_tbtc_* exports, so
cgo/C consumers can reference the symbol. Carries the same framing
caveat as the interactive_aggregate declaration.
- A malformed taproot_merkle_root_hex (non-hex / not 32 bytes) is
coordinator/wallet-context input, never member-attributable, so it now
returns an in-band Indeterminate verdict instead of propagating a
validation error to the FFI channel. This honors the verdict contract
("a tri-state for every input"); the Go host never infers "don't blame"
from an error code. Locked by a new bad-root assertion in the verify test.
Also folds the self-review Medium (doc-only): the Invalid verdict is
framable by a mismatched package/root just like aggregate's candidate
culprits, so it is an INPUT to the Go host's f+1 envelope-bound
adjudication (Phase 7.2b spec section 6), not authoritative blame. Noted
on ShareVerificationVerdict::Invalid, the verify_share.rs header, and the
header-file declaration.
Gates: cargo build + fmt --check + clippy clean; 285 lib tests pass
(incl. the augmented verify_signature_share equivalence/edge test).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e_share Codex P2 (new, against mirror head): verify_signature_share takes the engine state lock but never called sweep_expired_interactive_state, unlike interactive_aggregate and the other interactive entry points. So if verify-share blame rechecks are the only post-expiry traffic into the engine, abandoned interactive-signing nonce state (secret material) could remain resident past its TTL -- a security-lifecycle regression. Fix: sweep right after acquiring the state lock, before reading sessions, mirroring InteractiveAggregate's discipline (and its rationale comment). The sweep purges only expired interactive-attempt nonce state, never the DKG state this endpoint reads, so it cannot affect the verdict. Gates: cargo build + fmt --check + clippy clean; 285 lib tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… established Codex P2 (round 3, against mirror head): verify_signature_share decoded signature_share_hex (undecodable -> Invalid) BEFORE resolving the session, completed DKG, and the member's membership in the group (all -> Indeterminate). So a malformed share for an unknown / not-ready session, or for a member_identifier not in the group, returned Invalid (member-attributable blame) before the engine had established the member even belongs to the session -- violating the fail-closed-against-blame contract (self-incriminating Invalid is only sound once membership context exists). Fix: move the share decode/deserialize to AFTER the verifying-share lookup, so undecodable share bytes yield Invalid only once session + DKG + membership are all established; in any ambiguous (missing-context) case the earlier Indeterminate exits fire first. Pure reorder of independent steps; no behavior change on the established-context path. Tests: added two ordering-contract assertions -- undecodable share for a non-member id -> Indeterminate, and undecodable share for an unknown session -> Indeterminate (both were Invalid before the reorder); the established-context undecodable case (member in group) stays Invalid. Gates: cargo build + fmt --check + clippy clean; 285 lib tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…d shares
Codex P2 (round 4, against mirror head): the verifying-share lookup checks
GROUP membership (verification_key_package.verifying_shares() = all DKG
members), but PACKAGE membership (is the member in THIS attempt's signing
package commitment set) was only checked inside frost_core::verify_signature_share
-> UnknownIdentifier -> Indeterminate, which runs AFTER the share decode. So a
member who is in the group but OMITTED from the package, paired with undecodable
signature_share_hex, hit the Invalid decode-failure branch first -> a member-blame
verdict before establishing the attempt's package even included that member.
This is the round-3 fix one level deeper: a package that omits the member is
coordinator/context input (the member never signed a share for a package they
are not in), so undecodable bytes must not read as self-incriminating there.
Fix: add an explicit package-membership guard (signing_package.signing_commitments()
contains member_identifier) before the share decode; on omission, return
Indeterminate. The decodable omitted-member case already mapped to Indeterminate
via frost_core's UnknownIdentifier - this makes it explicit and extends the same
verdict to the undecodable case.
Tests: member 3 is in the group (threshold 2 of {1,2,3}) but omitted from the
package (commitments {1,2}); both a decodable share AND undecodable bytes for
member 3 now assert Indeterminate (the latter was Invalid before this fix).
Gates: cargo build + fmt --check + clippy clean; 285 lib tests pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…Verifier) (#4068) ## What Phase 7.2b-4: an engine **single-share FROST verification** the Go host's `Round2ShareVerifier` (the 4a member-blame classifier) calls to re-verify a retained share against the attempt's authoritative package — the **last seam in the blame stack**, and the one piece that needs FROST crypto (Go has none). ## How - **`engine::verify_signature_share`** resolves the group's `PublicKeyPackage` from the session's own DKG state (never the request), applies the taproot tweak **exactly as `InteractiveAggregate`** (same `canonicalize_taproot_merkle_root_hex` + `.tweak()`), and calls `frost_core::verify_signature_share` against the tweaked verifying share + group key. By construction this matches what `aggregate_custom(AllCheaters)` concludes for the same share. - **Explicit tri-state `ShareVerificationVerdict`** (`valid`/`invalid`/`indeterminate`) — per the Codex+Gemini design consult, only the engine can distinguish a member's malformed signed scalar (`invalid` → blame) from a malformed package/context (`indeterminate` → don't blame): undecodable **member** share bytes / `InvalidSignatureShare` → `invalid`; undecodable signing **package** / missing-or-`UnknownIdentifier` verifying share / session-not-ready → `indeterminate`; `Ok` → `valid`. - FFI export `frost_tbtc_verify_signature_share` + `api.rs` request/result. (Go maps the verdict to `ShareVerificationResult`, and any FFI-transport error → `ShareIndeterminate`.) ## Why standalone + tweak-aware is sound (consult) `frost_core::verify_signature_share` reuses `pre_aggregate` and computes the same binding factors / group commitment / BIP-340 challenge / `verify_share` relation as `aggregate_custom`; the even-Y/R negation is encapsulated in `PublicKeyPackage::tweak()`. **frost-core `=3.0.0`** is required (its `verify_signature_share` was fixed to call `pre_commitment_aggregate()`; older rc's diverged) — pinned, and guarded by the equivalence test. ## Tests `verify_signature_share_verdicts_match_aggregate_and_handle_edges`: valid shares → `Valid`, wrong-package share → `Invalid`, the **equivalence guard** (aggregate's `AllCheaters` culprit verdict matches verify's per-share verdicts), and edges (undecodable share → `Invalid`; missing member / undecodable package / unknown session → `Indeterminate`). Full lib suite (285) + fmt + clippy green. ## Follow-ups - The Go-side `Round2ShareVerifier` impl (scaffold) that calls this FFI + registers it in the cgo bridge. - A script-path (tweaked-root) equivalence-test variant. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…alence test Companion to the None-root equivalence test from #4068: that pins verify_signature_share == aggregate on the untweaked path; this pins it on the taproot TWEAK path, where the even-Y / tweak machinery runs. Shares are produced with frost::round2::sign_with_tweak (== sign under a taproot-tweaked key package, the production taproot signing path) under a non-empty merkle root, and verify_signature_share / interactive_aggregate are driven with Some(root): - each honest tweaked share verifies Valid under the root; - the SAME tweaked share is Invalid under None - the clinching assertion: the root is materially applied to the verifying material, not ignored, so the Some(root) Valid verdicts are meaningful; - a bogus tweaked share (validly signed over a different package) is Invalid; - equivalence: interactive_aggregate's AllCheaters culprit verdict over [member 1 valid, member 2 bogus] with the same root names exactly the member verify_signature_share calls Invalid (member 2). Closes the tracked tweaked-root follow-up from the #4068 engine verify_signature_share work. Test-only; no production code change. Gates: cargo test --lib (286 pass + 1 pre-existing ignored) + fmt --check + clippy clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…4071) ## What Adds the **script-path (taproot-tweaked-root) companion** to the `verify_signature_share` equivalence test from #4068. That test pins `verify_signature_share == aggregate_custom(AllCheaters)` on the **untweaked (None-root)** path; this one pins the same equivalence on the **taproot tweak path**, where the even-Y / `.tweak()` machinery actually runs. **Test-only — no production code change.** Closes the tracked tweaked-root follow-up from the #4068 engine work. ## How Shares are produced with `frost::round2::sign_with_tweak` (≡ `sign` under a taproot-tweaked key package — the production taproot signing path) under a non-empty merkle root, and both `verify_signature_share` and `interactive_aggregate` are driven with `Some(root)`: - **Each honest tweaked share verifies `Valid`** under the root. - **The same tweaked share is `Invalid` under `None`** — the clinching assertion: the root is *materially* applied to the verifying material, not ignored, so the `Some(root)` `Valid` verdicts are meaningful (rules out a "tweak is a no-op" false pass). - **A bogus tweaked share** (validly signed over a *different* package) is `Invalid`. - **Equivalence:** `interactive_aggregate`'s `AllCheaters` culprit verdict over `[member 1 valid, member 2 bogus]` with the same root names exactly the member `verify_signature_share` calls `Invalid` (member 2), and not member 1. ## Why it matters The tweak path is exactly where standalone-verify vs aggregate could silently diverge (even-Y negation, tweak application order). #4068 proved the resolve/tweak expressions are textually identical and pinned the None path; this directly exercises the tweaked path end-to-end, so the parity is tested where it's hardest, not just where it's easy. ## Tests `cargo test --lib` (286 pass + 1 pre-existing ignored) + `cargo fmt --check` + `cargo clippy --all-targets` all clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Phase 7.3 derivation increment (PR 1 of 2: engine). Exposes the canonical
interactive attempt-context + FROST-identifier derivation from the engine so the
Go host never re-implements the domain-separated derivations (the cross-language
divergence class that bit the coordinator seed) - the Codex+Gemini consult's
Option B.
New stateless, secret-free FFI export
frost_tbtc_derive_interactive_attempt_context ->
engine::derive_interactive_attempt_context(session_id, message, key_group,
threshold, attempt_number [1-based wire], included_participants) ->
{ attempt_context, frost_identifiers }. It composes the existing primitives
(canonicalize + rfc21_message_digest + roast_attempt_shuffle_seed +
select_coordinator_identifier + roast_included_participants_fingerprint_hex +
roast_attempt_id_hex + the codec identifier encoding) and RE-VALIDATES the
derived context against strict-mode validate_attempt_context before returning, so
the host is guaranteed a context interactive_session_open will accept. No
DKG/nonce/session state, no secrets.
Tests: engine units (matches the standalone derivations incl. canonical ordering
+ identifier encoding; deterministic; rejects empty message / zero attempt /
threshold>set / duplicate / empty participants) + an FFI success round-trip.
cargo fmt + clippy --all-targets --all-features -D warnings clean; full lib suite
289 pass / 1 ignored.
The Go-host wiring (bridge wrapper + coordinator cross-check + replacing the
runner's three placeholders) is the next PR. Production real-engine signing stays
gated on the frost-secp256k1-tr =3.0.0 external audit.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… doc Self-review fold. The hand-maintained include/frost_tbtc.h declares every FFI export with its security/contract doc comment, but this PR added frost_tbtc_derive_interactive_attempt_context to lib.rs without the matching header declaration. The Go host resolves symbols by name via dlsym, so this is not a runtime break, but the header is the documented C contract auditors and consumers read - add the declaration (with a Phase 7.3 doc comment) so the contract stays complete. Also document that the request's `threshold` is a validation gate only (it does not feed the fingerprint/attempt_id/coordinator derivation). Doc/contract only - no behavior change. cargo fmt + derive tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The len >= threshold check is vacuously true for threshold == 0, so the helper returned a successful attempt context for a zero/uninitialized threshold even though interactive_session_open rejects threshold == 0 up front (and no DKG threshold can be zero). That breaks the helper's contract - it is meant to hand the host a context open will accept - and defers the failure to open. Reject threshold == 0 explicitly (matching interactive_session_open's "threshold must be non-zero"), before the len check, so a malformed request fails at the derivation seam. Added a zero-threshold rejection test case. cargo fmt + clippy --all-targets -D warnings clean; derive tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codex re-review (valid): the helper skipped the enforce_provenance_gate() and validate_session_id() checks that interactive_session_open - and every other engine endpoint, including the public-material-only verify_signature_share - runs before validating the attempt context. That let the derivation endpoint return protocol material on an unattested engine, or for a session_id open would reject (the session_id is also hashed into attempt_id), i.e. an enforcement/contract mismatch vs the path it feeds. I initially leaned to push back, reading the design consult's "no policy decision / stateless" as "skip the provenance gate" - but the convention is decisive: verify_signature_share (itself stateless, public-material-only) and the stateless frost_ops primitives all run enforce_provenance_gate() + validate_session_id() as their front door. The provenance gate is a UNIVERSAL engine front-door, not a per-operation policy; "stateless/no policy" meant no session/nonce state and no blame decision. So Codex is right. Add both checks at the top of derive_interactive_attempt_context, matching open's order. Tests: hold lock_test_state() (hermetic development-profile env, gate off) in the derivation tests so they exercise the math; add an empty session_id rejection case and a provenance-enforced rejection test (ProvenanceGateRejected) proving the front door is wired. cargo fmt + clippy --all-targets -D warnings clean; full lib suite 291 pass / 1 ignored. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
## Phase 7.3 — engine-side attempt-context derivation (Option B, PR 1 of 2) Per the Codex+Gemini design consult, the host must not re-implement the engine's domain-separated attempt-context derivations (the seed-divergence class). This PR exposes them from the engine. ### What - New FFI export `frost_tbtc_derive_interactive_attempt_context` → `engine::derive_interactive_attempt_context`. Stateless, secret-free: takes an attempt's public inputs, returns the canonical `AttemptContext` (coordinator, included-participants fingerprint, attempt id) + one canonical FROST identifier per participant. - Composes the existing ROAST primitives and **re-validates the derived context against strict-mode `validate_attempt_context`** before returning, so the host receives a context `interactive_session_open` is guaranteed to accept. ### Why The interactive runner ships placeholder fingerprint/attempt_id/identifier values documented as inert (only the fake engine is wired). The real engine strict-validates those fields; deriving them in Go would duplicate domain-separated hashing across languages. Exposing from the engine = single source of truth. ### Tests - Engine units: matches the standalone derivations (coordinator, fingerprint, attempt_id, canonical ordering, identifier encoding); deterministic; rejects empty message / zero attempt number / threshold>set / duplicate / empty participants. - FFI success round-trip (the helper succeeds with no DKG fixture). - `cargo fmt` + `clippy --all-targets --all-features -D warnings` clean; full lib suite 289 pass / 1 ignored. ### Scope / follow-up - Go-host wiring (bridge wrapper + Go-coordinator cross-check + replacing the runner's three placeholders + threading identifiers) is the next PR. - Production real-engine signing remains gated on the `frost-secp256k1-tr =3.0.0` external audit; this helper is parse/canonicalization surface only. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…t operators A multi-seat operator runs concurrent interactive signing per seat in ONE process, all sharing one member-independent SessionID. The engine state is a process-global singleton that held exactly ONE open interactive member per session (interactive_signing: Option<InteractiveSigningState>), so a second seat's InteractiveSessionOpen(same session, different member) failed with SessionConflict; round2 freed the whole state (wiping siblings); and the per-attempt consumed-nonce marker made a sibling's round2 falsely trip ConsumedNonceReplay. Key interactive_signing by member_identifier (BTreeMap<u16, InteractiveSigningState>), so local seats are independent - exactly as separate processes would be. Design reviewed by Gemini (cryptographically sound) and Codex (approve with the refinements folded below); both rejected a session-wide live-attempt model since it would let one seat erase another's nonces. - Per-MEMBER live attempt: open(M, strictly-newer attempt) replaces only M's entry (zeroizing M's old nonces); a stale/equal open for M is rejected, but a sibling on a different attempt is untouched. No FROST invariant requires seats on one attempt. - round2 removes only the member's entry (siblings stay live); the marker is the durable replay protection. - consumed_interactive_attempt_markers keyed per-(attempt_id, member_id) via the composite marker; a legacy bare attempt_id marker is honored FAIL-CLOSED on read (interactive_attempt_consumed). aggregated_interactive_attempt_markers stay per-attempt (the aggregate is one signature over public data). - abort is session-level over the map (remove all matching entries); TTL sweep is per-entry by opened_at_unix (siblings survive); capacity counts live member entries. - interactive_state_for_attempt_mut is a member-keyed lookup; an unopened member now yields "no live interactive attempt for member N". Live state still never persists (empty map on reload). Zeroization is preserved via InteractiveRound1State::Drop. New test interactive_multi_seat_two_members_one_process_aggregate_bip340 drives two seats through the interactive API in one process: both open the same attempt, Round1 independently, member 1's Round2 frees only its entry and writes only its marker (member 2 stays live and unblocked), and the two interactive shares aggregate to a valid BIP-340 signature. Existing single-member tests updated for the map (is_empty / member-keyed access / composite markers). All 292 lib tests pass; cargo fmt clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…view fold)
Codex review (P2, valid): the multi-seat conversion left Round2 gated only by the
per-member consumed marker. When an attempt completes (interactive_aggregate records
aggregated_interactive_attempt_markers) with one threshold subset, an open sibling
seat that never signed has NO per-member consumed marker - so it could still release
a fresh share for the finished attempt, and enough such shares could be aggregated
into a SECOND valid signature over the same message, contrary to the completion
marker's "recovery is a fresh attempt" rule. The single-member engine never had an
unsigned sibling, so this is new to multi-seat.
Gate Round2 on the aggregate-completion marker (InteractiveAttemptAlreadyAggregated)
in addition to the consumed marker, before any share is released.
Tests (folding remaining valid review findings):
- interactive_round2_refused_after_aggregate_for_unsigned_sibling: 3 seats open the
same attempt, {1,2} sign + interactive_aggregate completes it, then seat 3's Round2
(in a {1,3} subset it would otherwise be valid for) is refused. Also exercises
interactive_aggregate over two interactive multi-seat shares (a coverage gap from
the self-review).
- interactive_open_advances_only_the_opening_member_attempt: seat 1 advancing to a
newer attempt replaces only its own entry (fresh nonce state); a sibling on the
older attempt is untouched; a stale re-open is rejected for the advanced seat but
idempotent for the sibling.
Capacity note (self-review): the cap now counts member entries; the default (64) is
generous for realistic multi-seat and the semantics are documented at the check.
All 294 lib tests pass; cargo fmt clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ge (re-review)
Codex re-review on the Round2 completion gate:
[P1] The gate trusted aggregated_interactive_attempt_markers keyed by attempt_id
alone, but interactive_aggregate never binds attempt_id to the aggregated package/
message - it only checks the shares aggregate under the session key. So a caller
with any valid aggregate could replay it under a DIFFERENT live attempt's id, set
that marker, and the new gate would then refuse the real Round2 before its nonce is
consumed (an availability/DoS regression the single-member engine never had).
Fix: make the completion marker MESSAGE-BOUND - interactive_aggregated_marker =
"{attempt_id}@{message_digest}". interactive_aggregate writes it from the package it
actually aggregated (all five marker sites: both completion pre-checks, the capacity
guard, the insert, and the persist-rollback). The Round2 gate recomputes it from the
message THIS member opened, so it preempts Round2 only for a genuine same-message
completion; a replayed aggregate carrying a different message sets an unrelated
marker that cannot match. Same-message re-aggregation is still rejected (existing
restart/repeat tests pass unchanged - they re-aggregate the same message).
[P2] On that genuine completion rejection, the unsigned sibling's entry is now dead,
so free it (zeroizing its nonces) instead of letting it hold a live-member cap slot
until the TTL sweep. Only the matching member's entry is removed.
Tests: the multi-seat refusal test now also asserts the marker is bound
(attempt_id@digest, not the bare id) and that the refused sibling's entry is freed.
All 294 lib tests pass; cargo fmt clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…review) Codex re-review (P2, valid): the previous commit made the aggregate completion marker message-bound (attempt_id@digest), but a completion persisted by the pre-binding engine is stored as the BARE attempt_id. The new checks only looked for the bound form, so after an upgrade a previously completed attempt looked unfinished - repeat InteractiveAggregate and the Round2 completion gate would no longer fail closed for it. Add interactive_attempt_aggregated(markers, attempt_id, digest) = bound form OR a legacy bare attempt_id marker (fail-closed on read), mirroring the consumed-marker helper. Use it at the three read sites (the Round2 completion gate and both interactive_aggregate pre-checks). New writes stay bound-only, so the durable record migrates forward on the next completion while legacy completions stay final. Test: interactive_honors_legacy_bare_aggregate_completion_marker injects a bare attempt_id marker (a pre-upgrade completion) and asserts Round2 then fails closed with InteractiveAttemptAlreadyAggregated. All 295 lib tests pass; cargo fmt clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e-review)
Codex re-review (P1, valid): attempt_id is derived from session/message/attempt#/
coordinator/included - NOT the taproot root. So the message-bound completion marker
(attempt_id@message_digest) still collided across taproot tweaks: a completion
aggregated for one root (or key-path None) recorded the same marker the Round2 gate
checks for a live seat opened with a different root, wrongly preempting that seat's
Round2 (and zeroizing it) even though the signatures differ per tweak.
Bind the canonical taproot root into the marker too -
interactive_aggregated_marker(attempt_id, message_digest, taproot_root) =
"{attempt_id}@{message_digest}@{root}" (root = hex, or "keypath" for None, which
cannot collide with a 64-hex root). interactive_aggregate writes it from the root it
aggregated under; the Round2 gate recomputes it from the root THIS member opened
with. The completion marker now binds the full signing-task identity (attempt + msg
+ root); the legacy bare-id fallback is unchanged.
Test interactive_round2_completion_marker_binds_taproot_root: a completion recorded
for a different root does not preempt a key-path member's Round2, while the same-root
completion does. All 296 lib tests pass; cargo fmt clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e-review) Codex re-review (P2, valid): when a multi-seat attempt aggregates with a threshold subset that EXCLUDES a local member which opened/Round1'd the same attempt + root, that member never calls Round2 (it is not a signer), so the Round2 completion gate never runs for it - its interactive_signing entry (nonces, key, message) and its live-member capacity slot stayed resident until the 1h TTL or an explicit abort. interactive_aggregate's success path now frees the LOCAL siblings finalized by the completion: after persisting the marker, remove + zeroize every interactive_signing entry on (attempt_id, taproot root) - the signers' entries were already removed at their Round2, and a sibling on a DIFFERENT root is a distinct signing task and is left untouched. The Round2 completion gate stays as the defense for a sibling that RE-OPENS the finalized attempt and tries to sign. Test interactive_round2_refused_after_aggregate_for_unsigned_sibling now asserts the non-signing sibling is freed at aggregation, then re-opens it and confirms the gate still refuses its Round2. All 296 lib tests pass; cargo fmt clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…o (re-review) Codex re-review (P2, valid): the finalized-sibling cleanup added in the previous commit matched entries on (attempt_id, taproot root) but NOT the message. Because attempt_id is provided by the caller separately from the package, a mismatched aggregate - a valid signing package for message B submitted under a live message-A attempt's id (same root) - would delete the message-A seats' live nonce/commitment state, forcing that unrelated attempt to restart, even though the stored marker (now message-bound) correctly would not match its Round2. Match the FULL finalized identity in the cleanup filter: attempt_id AND hash_hex(entry.message_bytes) == aggregated_message_digest AND taproot root - the same (attempt + message + root) identity the completion marker binds. Test interactive_aggregate_cleanup_is_message_bound: a valid stateless aggregate over message B under message A's attempt id leaves the live message-A seat intact. All 297 lib tests pass; cargo fmt clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t operators (#4098) ## What Keys a session's interactive signing state by `member_identifier` (`interactive_signing: Option<InteractiveSigningState>` → `BTreeMap<u16, InteractiveSigningState>`) so a **multi-seat operator can run interactive signing for all its seats in one process**. ## Why A multi-seat operator runs concurrent interactive signing per seat, all sharing one member-independent `SessionID`, in one process (the engine state is a process-global singleton). Today that breaks three ways: the 2nd seat's `InteractiveSessionOpen(same session, different member)` → `SessionConflict`; `round2` frees the whole state (wiping siblings); and the per-attempt consumed-nonce marker makes a sibling's `round2` falsely trip `ConsumedNonceReplay`. Surfaced by the keep-core real-cgo e2e (#4097). ## Design (reviewed: Gemini sound, Codex approve-with-refinements) Both reviewers rejected a session-wide live-attempt model — it would let one seat erase another's nonces, the exact bug being removed. - **Per-member live attempt**: `open(M, strictly-newer attempt)` replaces only M's entry (zeroizing M's old nonces); a stale/equal open for M is rejected, but a sibling on a different attempt is untouched. Seats are independent — exactly as separate processes would be. - **round2** removes only the member's entry (siblings stay live); the durable marker carries replay protection. - **Consumed markers** keyed per-`(attempt_id, member_id)` (composite); a **legacy bare `attempt_id` marker is honored fail-closed** on read. `aggregated_interactive_attempt_markers` stay per-attempt (one signature per attempt over public data). - **abort** is session-level over the map (removes all matching entries); **TTL sweep** is per-entry by `opened_at_unix` (siblings survive); **capacity** counts live member entries (a new member is a slot; a same-member replacement isn't). - `interactive_state_for_attempt_mut` is a member-keyed lookup. Live state still never persists (empty map on reload). Zeroization preserved via `InteractiveRound1State::Drop`. ## Tests New `interactive_multi_seat_two_members_one_process_aggregate_bip340`: two seats open the same attempt in one process, Round1 independently, member 1's Round2 frees only its entry and writes only its marker (member 2 stays live and unblocked), and the two interactive shares **aggregate to a valid BIP-340 signature**. Existing single-member tests updated for the map. **All 292 lib tests pass; `cargo fmt` clean.** Follow-ups (additional edge tests Codex itemized — per-member attempt advance, abort-by-attempt over multiple members, capacity new-vs-replacement) can land in review. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…attempt edge cases Two follow-up edge tests Codex itemized for the multi-seat interactive engine fix (#4098), test-only: - interactive_capacity_counts_new_members_not_replacements: at a live-member cap of 1, a member advancing to a newer attempt replaces its own entry (no new slot, succeeds), while a DIFFERENT member is a new entry that trips the cap and fails closed - pinning that the cap counts member entries, not sessions. - interactive_abort_by_attempt_removes_all_members_on_that_attempt: abort with an attempt_id filter is session-level over the member map - it removes every local seat on that attempt while a sibling seat on a different attempt survives. All 299 lib tests pass; cargo fmt clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…attempt edge cases (#4100) ## What Two follow-up edge tests Codex itemized during the multi-seat interactive engine fix (#4098), test-only: - **`interactive_capacity_counts_new_members_not_replacements`** — at a live-member cap of 1: a member advancing to a newer attempt *replaces* its own entry (no new slot, succeeds), while a *different* member is a new entry that trips the cap and fails closed. Pins that the cap counts member entries, not sessions. - **`interactive_abort_by_attempt_removes_all_members_on_that_attempt`** — abort with an `attempt_id` filter is session-level over the member map: it removes every local seat on that attempt while a sibling seat on a *different* attempt survives. All 299 lib tests pass; `cargo fmt` clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…tc_abi_version)
The Go cgo bridge and libfrost_tbtc are compiled separately and linked at runtime via
dlsym; a symbol that resolves but changed MEANING (struct/JSON contract change) is
silent corruption. The existing frost_tbtc_version returns a human string
("tbtc-signer/0.1.0-bootstrap") that cannot be machine-checked.
Add frost_tbtc_abi_version() returning a structured FFI CONTRACT version
{abi_major, abi_minor} (api::FrostTbtcAbiVersionResult), so a consumer can fail closed
against an incompatible lib (the Go-side assertion lands separately). Starts at 1.0 -
NOT 0.x, to avoid semver pre-1.0 ambiguity. abi_major = any incompatible contract
change (C signatures, JSON field meaning, required fields, enum/status values,
serialization, memory ownership, crypto transcript semantics); abi_minor = a purely
additive, backward-compatible change old consumers safely ignore. A consumer requires
lib.abi_major == its_major AND lib.abi_minor >= the minor it needs. The human
frost_tbtc_version string stays informational and unchanged.
Design validated via Codex consult (major.minor; exact major, minimum minor; the
{abi_major, abi_minor} JSON is the frozen root compatibility surface).
300 lib tests pass (new: abi_version_reports_the_contract_version pins 1.0);
cargo fmt clean; the symbol is exported in the built cdylib.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codex #4104 P2: the new #[no_mangle] frost_tbtc_abi_version is part of the public C ABI, but header-based consumers that compile against include/frost_tbtc.h had no prototype for it - the exported symbol and the advertised header could drift. Add the declaration alongside frost_tbtc_version (the header is hand-maintained; no cbindgen). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tc_abi_version) (#4104) ## What Adds `frost_tbtc_abi_version()` — a structured **FFI contract version** `{abi_major, abi_minor}` — so the Go cgo bridge can **fail closed** against an incompatible `libfrost_tbtc` instead of silently misinterpreting a changed contract. The bridge and the lib are compiled separately and linked at runtime via `dlsym`; a symbol that still resolves but changed *meaning* (struct/JSON contract change) is silent corruption. The existing `frost_tbtc_version` returns a human string (`tbtc-signer/0.1.0-bootstrap`) that can't be machine-checked — this is the checkable companion. ## Scheme (Codex-validated) - **`abi_major`** — any *incompatible* change to the Go↔Rust contract: C signatures, JSON field meaning, required fields, enum/status values, serialization, memory ownership, crypto transcript semantics. - **`abi_minor`** — a purely *additive*, backward-compatible change old consumers safely ignore. - A consumer requires `lib.abi_major == its_major` **and** `lib.abi_minor >= the minor it needs`. - Starts at **1.0** (not 0.x, to avoid semver pre-1.0 ambiguity). The `{abi_major, abi_minor}` JSON is the **frozen root compatibility surface**. - `frost_tbtc_version` (human string) stays informational and unchanged. ## Notes - The **Go-side assertion** (fail-closed preflight + the `requiredMajor`/`requiredMinMinor` constants + a CI-gate check) lands as a separate keep-core PR that also bumps the signer pin to this commit. - 300 lib tests pass (new `abi_version_reports_the_contract_version` pins 1.0); `cargo fmt` clean; the symbol is exported in the built cdylib. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
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 divergences are intentional canonical keep-core changes and are recorded below. 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
Expanded Divergence Register
These are the intentional post-freeze divergences from
frost-extraction-source-v1.pkg/tbtc/signer/scripts/formal/check_roast_attempt_context_vectors.mjs7ceb3929b8403ebde515e7ac290414e2aa1fe6920085f1f4930dd2219d93a406pkg/tbtc/signer/scripts/formal/run_tla_models.sh64474409f2682fb903e26fee30613bc3d2a3542df4cebec6d86992112ab26b8ctla2tools.jarrelease-asset checksum.pkg/tbtc/signer/README.md3fe9046df81442a95139d9ced977df3ec05cca468bf77fdd185be48e73426539tools/tbtc-signertopkg/tbtc/signer; fixed local docs link.pkg/tbtc/signer/src/engine.rse149510ec9eca52290f09d26f5d16c684d0a3941134cc46d4c3484947d8bf0ddpkg/tbtc/signer/src/ffi.rs02bd12a7aab5d40e9e0eb55f8011caf86a3586eb795ab910179a4604b4335c14ptr::slice_from_raw_parts_mutinfree_buffer.pkg/tbtc/signer/src/lib.rse04f05b30ffa6bd9f282e63482c465945ebaacb31a989c6c045cb6dae418b24cpkg/tbtc/signer/tests/p2tr_signature_fraud_vectors.rs1c852f79ab683c03bf4846dd2ac58c37cf73d1645f1c4813034a16a0a39a04ddtest/vectorslayout..github/workflows/tbtc-signer-formal.yml623e9c8fab66497339c5ec88800c20afc68075a76680f4a8eb9ea7e327d0ce28pkg/tbtc/signer/test/vectors/p2tr-signature-fraud-v0.json4edc25113ef5a022af347eb0c6752b06f61f4baace94a13b91cf15042065b002Dual signoff for allowlisted divergences remains required before merge per plan v38 section 4.2:
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.