feat(tbtc/signer): Rust FROST/ROAST signer (distributed DKG, interactive-only signing, FFI ABI 2.0) - #4005
Open
mswilkison wants to merge 318 commits into
Open
feat(tbtc/signer): Rust FROST/ROAST signer (distributed DKG, interactive-only signing, FFI ABI 2.0)#4005mswilkison wants to merge 318 commits into
mswilkison wants to merge 318 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
![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
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
mswilkison
added a commit
that referenced
this pull request
May 26, 2026
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
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
mswilkison
added a commit
that referenced
this pull request
Jun 6, 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.
This was referenced Jun 10, 2026
mswilkison
added a commit
that referenced
this pull request
Jun 11, 2026
…4026) Stacked on #3866 (base: `feat/frost-schnorr-migration-scaffold`). ## What Adds `TestSelectCoordinator_CrossLanguagePinnedVectors` to `pkg/frost/roast/coordinator_test.go`, pinning concrete `SelectCoordinator` outputs for fixed `(members, seed, attempt)` tuples. ## Why The Rust signer (PR #4005) ports Go's `math/rand` shuffle semantics in `pkg/tbtc/signer/src/go_math_rand.rs` and pins exact expected coordinators in `select_coordinator_matches_known_keep_core_vectors` (seed `6879463052285329321`). The Go suite, however, only asserted *properties* (determinism, input-order independence, seed/attempt sensitivity) — never concrete outputs. That asymmetry means a Go-side semantic change (e.g. migrating to `math/rand/v2`, changing the shuffle, or altering the `attemptSeed + attemptNumber` composition) would pass the entire Go suite while silently breaking coordinator agreement with the Rust engine — a network-fracturing liveness failure that would only surface in mixed-version soak testing. This PR pins the exact same vectors as the Rust test (verified locally that current Go code produces them), plus pins the previously value-free `(seed=333, attempt=4)` case to its concrete result. Either side drifting now fails its own unit suite. ## Review note (no code change) While verifying parity I noticed the two layers derive the legacy `int64` shuffle seed differently today: - Go RFC-21 layer: `foldAttemptSeed(SHA256(DkgGroupPublicKey || SessionID || MessageDigest))` (first 8 bytes, BE), 0-based `AttemptNumber`. - Rust engine strict-mode validation (`roast_attempt_seed_from_message_digest_hex`): first 8 bytes of the **raw message digest**, with a 1-based `attempt_number`. Not a live bug — keep-core does not yet send `attempt_context` over the FFI, and Rust strict mode is opt-in — but when a later phase wires RFC-21 attempt contexts into the Rust engine's `validate_attempt_context`, the two expected-coordinator computations will disagree unless one side is aligned first. Flagging so it lands on the integration checklist rather than in a testnet incident.
mswilkison
added a commit
that referenced
this pull request
Jun 11, 2026
…nce test (#4027) Stacked on #4005 (base: `extraction/frost-signer-mirror-2026-05-26`). Companion to #4026 on the Go branch. ## What `select_coordinator_is_input_order_independent` in `pkg/tbtc/signer/src/go_math_rand.rs` asserted only that two input orderings of the member set agree — not what they agree on. This pins the concrete result (`Some(4)` for `(members=[1..6], seed=333, attempt=4)`), matching the value now pinned on the Go side in `TestSelectCoordinator_CrossLanguagePinnedVectors` (#4026). ## Why Coordinator selection must agree byte-for-byte between Go's `SelectCoordinator` and this Rust port of Go's `math/rand` shuffle, or honest nodes elect different coordinators and the ROAST liveness path fractures. With the vector sets symmetric across both test suites, either implementation drifting fails its own unit tests instead of surfacing in mixed-version soak testing. Verified locally: `cargo test --lib go_math_rand` passes, `cargo fmt --check` clean.
This was referenced Jun 11, 2026
Merged
mswilkison
added a commit
that referenced
this pull request
Jun 11, 2026
…inal (#4033) Stacked on #4005 (base: `extraction/frost-signer-mirror-2026-05-26`). Implements the dependency-pin item from the review feedback ("get off the `=3.0.0-rc.0` pin"). `frost-secp256k1-tr 3.0.0` final (plus `frost-core`/`frost-rerandomized` 3.0.0) is published and unyanked on crates.io; the engine was anchored to the release candidate, which receives no post-release fixes. This moves the exact pin to the final release — same exact-pin discipline, correct anchor. Verified: full signer suite passes unchanged against the final (244 tests, clippy clean), confirming no rc.0→final API or behavior drift. Remaining half of the review item for the rollout gates: record which ZF/external audit reports cover `frost-core` 3.x and the `secp256k1-tr` ciphersuite specifically (the audited lineage claim in the readiness docs should cite the exact report and version range). 🤖 Generated with [Claude Code](https://claude.com/claude-code)
mswilkison
added a commit
that referenced
this pull request
Jun 11, 2026
…ffle corpus (#4035) Stacked on #4005 (base: `extraction/frost-signer-mirror-2026-05-26`). Rust half of the corpus-based differential parity item from the review; pairs with the Go-side PR #4034. Adds `testdata/coordinator_shuffle_corpus.json` — a byte-identical copy of the canonical 600-case corpus generated from keep-core's Go `SelectCoordinator` — and `select_coordinator_matches_cross_language_differential_corpus`, which replays every case through the `go_math_rand` port: 216 integer-boundary cases (seeds 0/±1/`i64::MIN`/`i64::MAX`/the #4026 pin seed; wrapping `seed + attempt` composition up to `u32::MAX`; unsorted and reversed member inputs pinning the internal sort) plus 384 generated sweeps over set sizes 1..255 with full-range seeds. All 600 cases replay identically today — direct evidence the `math/rand` port is bit-exact across the boundary regions where ports diverge first. Any future drift in source seeding, Fisher-Yates order, `int31n` bounds, sign handling, wrapping, or sorting fails this suite on the drifting side. Full signer suite passes (245 tests); clippy/rustfmt clean. Mirror note: port back to the tBTC monorepo signer with the next extraction sync. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
mswilkison
added a commit
that referenced
this pull request
Jun 11, 2026
…+ cross-language conformance vectors (#4031) Stacked on #4005 (base: `extraction/frost-signer-mirror-2026-05-26`). Implements item 3 of the review feedback (duplicated, divergent protocol constants) — Rust half; pairs with the Go-side PR #4030 stacked on #3866. ## Problem Flagged in #4026: the engine validated attempt contexts using `int64_be(MessageDigest[0..8])` with the 1-based wire attempt number (the legacy `signingAttemptSeed` convention), while the Go RFC-21 layer derives `fold(SHA256(KeyGroup ‖ SessionID ‖ MessageDigest))` with 0-based attempt numbers. At Phase-7 wiring, every Go-derived attempt context would fail the engine's strict-mode `validate_attempt_context` — a deterministic, network-wide liveness failure invisible to either side's property tests. ## What changed - **`roast_attempt_shuffle_seed(key_group, session_id, message_digest_hex)`** implements the normative RFC-21 Annex A derivation (see #4030). The key-group handle — this engine's hex-encoded serialized group verifying key — feeds the hash as an opaque UTF-8 string, exactly matching keep-core's `attempt.DeriveAttemptSeed` + `foldAttemptSeed` composition, including the strict 32-byte digest requirement. - **`validate_attempt_context` now takes the session's key group** (threaded from `dkg.key_group` at StartSignRound and the session's `DkgResult` at FinalizeSignRound) and composes the shuffle source with the **0-based** RFC-21 attempt number. The FFI wire encoding stays 1-based (`attempt_number >= 1` still enforced; `wire = AttemptNumber + 1`); the engine subtracts one before composition, per the annex. - **`testdata/coordinator_seed_vectors.json`** — byte-identical copy of the canonical file generated from the Go implementation. `coordinator_seed_derivation_matches_cross_language_vectors` pins, for all ten vectors: the folded seed (including negative values, so an unsigned port cannot pass), the selected coordinator (including the n=100 production-shape set), the 0-/1-based wire mapping, and end-to-end strict-mode `validate_attempt_context` acceptance of a context built from the wire encoding. Either language drifting now fails its own unit suite. - **`docs/roast-coordinator-seed-derivation.md`** mirrors the normative annex for signer-side readers, with the regen/copy procedure. - The coordinator-mismatch test derives the provably-wrong coordinator instead of hardcoding member 1 (which, under the new seed, happened to become the correct selection — exactly the class of silent assumption these vectors exist to catch). ## Notes - Mixed-version note: engines on the old derivation reject contexts produced under the new one (and vice versa) — strict-mode attempt contexts are not yet produced by the Go layer in any deployment, so this is pre-wiring cleanup with no live-fleet impact. - The attempt-context vector suite (`roast-attempt-context-v1.json`) is unaffected: it pins fingerprint/attempt-id domains with the coordinator as an *input*. - Port back to the tBTC monorepo signer alongside the next extraction sync. ## Tests Full suite: 245 passed, 0 failed; clippy and rustfmt clean. New conformance test exercises all ten cross-language vectors. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
mswilkison
added a commit
that referenced
this pull request
Jun 11, 2026
…transitional signing out of production (#4028) Stacked on #4005 (base: `extraction/frost-signer-mirror-2026-05-26`). Implements item 1 of the review feedback on the FROST/ROAST stack (nonce rollback safety). ## Problem The transitional `StartSignRound`/`FinalizeSignRound` flow derives round-1 nonces deterministically from `H("round-nonce", signing_share, session_id, round_id, message, participant_id)`. Nonce-reuse safety therefore rested on two indirect properties: 1. **`round_id` schema integrity** — the participants set, Taproot tweak, and attempt context were bound only through `derive_round_id`. Any future change to that derivation (or an encoding collision in it) could let two different FROST transcripts share a nonce seed, which is the share-extraction condition. 2. **Consumed-round registry integrity** — the on-disk registries were the replay boundary. A VM snapshot restore, backup restore, or state replicated to a second host silently re-arms consumed rounds. ## What changed **1. Total direct binding (`RoundNonceBinding`, domain `round-nonce-v2`).** The nonce seed now directly binds every value that enters the FROST binding factor, challenge, Lagrange interpolation set, or key-material selection: group verifying key, Taproot merkle root, canonical signing-participant set — in addition to the existing signing share, session, round, message, and participant id. The struct carries a documented invariant so the next transcript input added to this flow gets added to the seed in the same change. Consequence: a rolled-back or cloned state can only ever repeat an *identical* transcript (producing the identical signature — no new information), never the same nonce under a different challenge. Nonce safety no longer depends on `round_id` schema or registry integrity at all. **2. Production hard-gate on the deterministic-nonce entry points.** Dealer DKG was already blocked in production, but that gate only fires at session *creation*: persisted state created under a development profile could be carried into a production-profile process and signed with. `StartSignRound`/`FinalizeSignRound` now reject in the production profile with reason `transitional_deterministic_signing_disabled_in_production`, making the OS-random interactive FROST path the only production signing path regardless of how on-disk state was created. ## Why not the per-boot RAM-only salt suggested in the review Exploration showed the transitional flow has every member independently derive **all** participants' commitments with zero round-1 exchange (members exchange only signature shares), so cross-machine determinism is load-bearing: a per-machine salt would break every multi-member bootstrap session. The two changes above implement the same goal — *rollback costs liveness, never keys* — within the flow's actual architecture: (1) removes the nonce-reuse class structurally, (2) removes production exposure structurally. The stateless interactive path (`GenerateNoncesAndCommitments`) already draws from OS randomness and holds nonces only in caller RAM. ## Tests - `deterministic_round_nonce_and_commitment_binds_full_transcript`: identical binding re-derives identical commitments; each of 7 binding inputs (message, tweak root, participants set, group key, session, round, participant) independently changes the commitment. - `start_sign_round_rejects_transitional_signing_in_production_profile` / `finalize_sign_round_rejects_transitional_signing_in_production_profile`: the state-smuggling scenario — dev-created dealer session, production-profile process — rejects at both entry points, even with the strict-mode env flag explicitly disabled. - `production_profile_forces_roast_strict_mode_without_env_flag` repurposed to assert the strict-mode forcing at the helper level (the FFI-level path it previously exercised is now unreachable in production by design). - Full suite: 246 passed, 0 failed; clippy and rustfmt clean. ## Notes for the mirror - The seed domain bump (`round-nonce` → `round-nonce-v2`) changes derived commitments for identical inputs; a mixed-version fleet cannot co-sign transitional rounds mid-rollout. Dev/staging-only flow, so the cost is a failed attempt until the fleet converges. - Port back to the tBTC monorepo signer alongside the next extraction sync.
mswilkison
added a commit
that referenced
this pull request
Jun 11, 2026
…x A) + cross-language conformance vectors (#4030) Stacked on #3866 (base: `feat/frost-schnorr-migration-scaffold`). Implements item 3 of the review feedback (duplicated, divergent protocol constants) — Go half; the Rust half is the paired PR stacked on #4005. ## Problem The coordinator-shuffle seed derivation exists twice, in two languages, on two branches, with no single source of truth — and the two copies disagree (flagged in #4026): | | seed | attempt numbering | |---|---|---| | Go RFC-21 layer | `fold(SHA256(KeyGroup ‖ SessionID ‖ MessageDigest))` | 0-based | | Rust engine validation | `int64_be(MessageDigest[0..8])` (legacy `signingAttemptSeed` convention) | 1-based wire | At Phase-7 wiring, every Go-derived attempt context would fail the Rust engine's strict-mode validation — a network-fracturing liveness failure that property tests on either side cannot catch. ## What this PR does (Go half) 1. **RFC-21 Annex A (normative)** — single normative definition of the derivation: inputs (including the exact `KeyGroupBytes` definition for `FrostTBTCSignerV1` material — the UTF-8 bytes of the hex key-group handle, treated opaquely), the 0-based composition with the two's-complement-wrapping addition, the `wire = AttemptNumber + 1` FFI mapping, and the accepted non-goals (unframed concatenation, first-8-byte fold, grindability bounds) with rationale. The Go derivation is adopted as normative: it binds key group + session + digest rather than the digest alone, and the live `pkg/tbtc` signing loop's legacy convention is explicitly documented as the thing Phase 7 migrates *from*. 2. **Generated conformance vectors** — `pkg/frost/roast/testdata/coordinator_seed_vectors.json`: ten end-to-end vectors (folded seed int64 + selected coordinator) covering attempts 0/1/3/5/7, sparse and production-size (n=100) member sets, opaque key-group handles, and negative folded seeds. Regenerated from the deterministic input matrix via `ROAST_SEED_VECTORS_REGEN=1 go test -run TestRegenerateCoordinatorSeedVectors` — generation-from-spec rather than hand-pinning, per the review. 3. **Conformance test** — `TestCoordinatorSeedDerivation_ConformanceVectors` pins `DeriveAttemptSeed → foldAttemptSeed → SelectCoordinator` end to end against the file, asserts the wire-mapping invariant on every vector, and requires at least one negative-seed pin so an unsigned-integer port cannot pass. The paired Rust PR switches the engine to this derivation (subtracting 1 from the wire attempt number before composition) and consumes a byte-identical copy of the vector file, so either side drifting fails its own CI rather than fracturing coordinator agreement in a mixed deployment. No behavior change on the Go side — it was already normative-conformant; this PR makes that the *specified* behavior and pins it. ## Tests `go test ./pkg/frost/...` passes; vectors verified present with 7 negative-seed pins out of 10. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
mswilkison
added a commit
that referenced
this pull request
Jun 11, 2026
…e coordinator shuffle (#4034) Stacked on #3866 (base: `feat/frost-schnorr-migration-scaffold`). Implements the review item "widen the Go↔Rust math/rand parity from a handful of pinned vectors to corpus-based differential fuzzing" — Go half; the Rust consumer is the paired PR stacked on #4005. ## What A generated 600-case differential corpus over `SelectCoordinator` (`testdata/coordinator_shuffle_corpus.json`, 176 KB), replayed by `TestCoordinatorShuffle_DifferentialCorpus` here and by the identical byte-for-byte copy in the Rust signer's `go_math_rand` tests: - **216 boundary cases**: seeds {0, ±1, `i64::MIN/MAX`, `MIN+3`/`MAX−3`, the #4026 pin seed and its negation} × attempts {0, 1, 7, `u32::MAX`} — exercising the two's-complement wrapping `seed + attempt` composition — × six member sets including unsorted and reversed inputs (pinning the internal sort both implementations perform). - **384 generated cases**: fixed-seed generator sweeping set sizes 1..255 (the full `group.MemberIndex` range), full-range `int64` seeds, and small/large/extreme attempt numbers. Regeneration is deterministic and gated (`ROAST_SHUFFLE_CORPUS_REGEN=1`), so the corpus provably comes from the documented case matrix rather than hand-pinning. This complements #4030's Annex-A seed-derivation vectors: those pin the *derivation* end-to-end on 10 vectors; this corpus stress-pins the *shuffle port itself* — the actual cross-language landmine — at volume, including the integer-boundary regions where a port diverges first. Not full continuous fuzzing (no coverage-guided harness); it's the pragmatic corpus-differential version that rides the existing unit-test CI on both sides at negligible cost. A coverage-guided Go-oracle harness can layer on later if desired. ## Tests `go test ./pkg/frost/roast/...` passes (corpus replay + regeneration roundtrip verified). 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…eshold Two findings on the 7.1 path: P2 (liveness/DoS) - an interactive open that later expired or was aborted before Round2 left an otherwise-empty SessionState in the registry. Since open inserts new session IDs and ensure_session_insert_capacity counts every map entry, a caller could churn unique sessions until TBTC_SIGNER_MAX_SESSIONS filled, after which DKG / build_tx / new interactive sessions were rejected until restart. The TTL sweep and abort now drop a session that holds nothing durable once its live attempt is cleared, via a new SessionState::is_disposable that checks EVERY field so a session still carrying consumed markers or DKG material is never removed. P2 (verify-before-consume) - Open accepted a threshold below the key package's min_signers; Round2 would then accept a too-small signing package, persist the consumed marker, and only then have frost::round2::sign fail on the commitment count - burning the nonce for a validation error. Open now rejects threshold != key package min_signers before storing the session. Tests: open-then-abort churn under a 2-session cap stays bounded (no accumulation); the abort-sweep test now asserts the empty session is dropped, not just cleared; threshold-below-min_signers is rejected and the matching threshold opens. Full suite 264 passed / 1 ignored, clippy -D warnings clean, chaos suite green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Section 1 said the host "holds no signing secrets at any time," but section 3 maps the transitional DKG calls unchanged and the frozen Phase 7 spec still has the DKG APIs returning/accepting secret_package_hex through the host until the DKG-custody follow-up. So in deployments that run DKG through this transport the host still sees DKG secret material (review finding). Section 1 now scopes the property to the signing path and states explicitly that #4007 must treat the host<->sidecar signing interface as a secret boundary but NOT the DKG interface until DKG custody moves inside the sidecar - a precondition for the sidecar being a complete secret boundary. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t the request
Two findings, the first central to the whole effort:
P2 (secret boundary) - InteractiveSessionOpenRequest carried
key_package_hex, forcing the private FROST key package through the
host/FFI request buffer on every open. That directly contradicts the
frozen spec section 4 ("key shares already env/command-only ... no
secret signing material transits the Go/Rust interface") and would
leave the sidecar unable to provide the signing-secret boundary that
is the entire point of Phase 7. Open now resolves the member's key
package from the session's own DKG state (run_dkg-populated), exactly
like the coarse start_sign_round: the session must already exist with
completed DKG, the request carries no key material, and the threshold
is validated against the DKG threshold. The key_package fields are
removed from the request; the spec section 5 is corrected accordingly.
Because interactive sessions now always ride a DKG-populated session
and never create registry entries, the empty-session-churn fixed last
round is impossible by construction, so the is_disposable disposal
logic (and its churn test) is reverted as dead code; sweep/abort just
clear the live attempt and retain the DKG session.
P2 (rollback) - a delayed InteractiveSessionOpen for an older attempt
could replace a newer live attempt and wipe its nonces. Open now
replaces a different live attempt ONLY when the incoming
attempt_number strictly advances the live one; an older-or-equal
attempt is rejected.
Tests: aggregation, framing, replay, restart, persist-fault, TTL,
capacity, lifecycle, quarantine, firewall, and the new TOCTOU recheck
all rebuilt on DKG-seeded sessions (key material from engine state);
added open-requires-DKG-session and non-participant rejection;
threshold mismatch now reports the DKG threshold. Full suite 264
passed / 1 ignored, clippy -D warnings clean, chaos green.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixes the CI 'Signer Rust checks' job failure (fmt/clippy gate). No behavior change: reformatting plus mechanical clippy autofixes (option_map_unit_fn -> if let, or_insert_with(default) -> or_default(), identity_op literal simplification, dead struct-update-syntax removal where every field was already specified).
Fixes the CI 'Signer Rust checks' job failure (fmt/clippy gate). No behavior change: reformatting plus mechanical clippy autofixes (option_map_unit_fn -> if let, or_insert_with(default) -> or_default(), identity_op literal simplification, dead struct-update-syntax removal where every field was already specified).
Fixes the CI 'Signer Rust checks' job failure (fmt/clippy gate). No behavior change: reformatting plus mechanical clippy autofixes (option_map_unit_fn -> if let, or_insert_with(default) -> or_default(), identity_op literal simplification, dead struct-update-syntax removal where every field was already specified).
Fixes the CI 'Signer Rust checks' job failure (fmt/clippy gate). No behavior change: reformatting plus mechanical clippy autofixes (option_map_unit_fn -> if let, or_insert_with(default) -> or_default(), identity_op literal simplification, dead struct-update-syntax removal where every field was already specified).
…) (#4257) ## Summary Design-spike RFC for issue #4250: recovering FROST signing capability after operator share loss. ## Contents - `pkg/tbtc/signer/docs/rfc/0001-frost-refresh-recovery.md` — full design-spike RFC. - Two design options compared side-by-side: - **Share-extension reshare** (3-round protocol over the existing transport; no governance dependency). - **Dealer-mediated refresh with governance** (2-round protocol + governance round; depends on a signed DAO approval round). - Per option: protocol flow, FFI surface sketch, state and persistence changes, complexity / effort estimate, residual risks. - Side-by-side comparison and a recommendation. - Acceptance criteria restated for the implementer, plus open questions for human review. ## Why a design-spike, not an implementation The user explicitly chose to defer the implementation until a human selects between the two designs (see the original ask-vs-choose for #4250). The RFC is a decision document, not a contract for code. ## Out of scope - FROST library, governance contract, or transport changes. - Specific retry / re-share recovery semantics (this RFC focuses on the recovery vs refresh distinction and the human decision, not the wire format).
…4252) (#4259) ## Summary Decision-6 operator runbook for the staged FROST shadow-mode rollout, appended to the existing `roast-phase-5-rollout-runbook.md`. ## Contents - Three-stage rollout: 1. **Audit-only** — shadow-mode audit functions are enabled, the gaters are not. Operators compare audit-only output against the live signing path to validate the shadow-mode instrumentation before any gating change can affect signing. 2. **Canary** — shadow-mode gaters activate for a bounded subset of wallets (initial 10%, ramping per audit outcomes). Rollback path documented. 3. **Cutover** — full deployment of shadow-mode gaters, audit continues but is no longer the only path. - Per-stage: prerequisites, env-var knobs to set, expected audit telemetry, decision criteria for advancing to the next stage, and explicit rollback steps. - Failure modes: audit-mismatch spike, telemetry-collection outage, gater false-positive rate. Each has an explicit "stop and roll back" trigger. ## Why a runbook, not a config change The decision in the gap inventory was that operator SOPs come first, before the shadow-mode gaters from Decision 1 are wired into the runtime. This runbook is the document operators will follow once the wiring lands. ## Out of scope - The actual gater wiring (Decision 1 follow-up; covered by `gate_signing_output_under_shadow_mode` and `gate_dkg_output_under_shadow_mode` which are defined but not yet called from the runtime).
…cluster B) (#4262) ## Summary Log hygiene / injection-sanitization cluster of the lower-priority findings, plus one panic-hook restoration bug found while in this neighborhood. ## Changes - `audit.rs` — `unsupported reason` formatter no longer string-interpolates a raw `request.reason` value into the error. The mismatch path now normalizes both `reason` arguments (trim + lowercase) before comparison, and the rejection text only logs the normalized value. - `persistence.rs` — the `Internal` error surfaced after a successful AEAD decrypt no longer echoes the raw `serde_json::from_slice` error content. The error message is now a fixed string; field-level diagnostic info stays in the debug logs. - `ffi.rs` — panic-hook installation keeps the captured default hook in `Arc<Mutex<Option<...>>>` shared storage instead of moving it out via a single `.take()`. The old code would silently drop the original hook if `Box::new` panicked under allocator pressure during install (it had already been `take_hook()`-ed). The new code lets the failure path reliably restore the original hook from the shared cell. ## Tests - 278 lib tests pass, 1 pre-existing ignore unrelated to this change. ## Out of scope - General redaction-pipeline hardening (separate concern).
…re cycle (#4255 cluster C) (#4263) ## Summary Performance cluster of the lower-priority findings: avoid cloning the per-session pending-operation set twice in a single retire cycle. ## Change `retire_idle_per_message_session_ids` in `engine/state.rs` already snapshots `persistence_pending_session_ids()` for its own retire pass. The subsequent `compact_retired_per_message_sessions` call re-derived the same set under the same `PERSISTENCE_PENDING_OPERATIONS` mutex, re-cloning the full set (size scales with `max_sessions_limit`, default 1024) per cycle. Thread the already-cloned set through an optional `pending_session_ids: Option<&HashSet<String>>` parameter on `compact_retired_per_message_sessions_to_total` so the persist path clones once per cycle instead of twice. Callers without a pre-cloned snapshot still work via the `None` path that re-clones. ## Tests - 278 lib tests pass, 1 pre-existing ignore unrelated to this change. - 78 interactive + 3 persistence sub-suite tests pass. ## Out of scope - General persist-path profiling (separate concern). - Persist path on the cold-start path (this only addresses the hot retire cycle).
…uster D) (#4264) ## Summary Doc-drift and comment cleanup cluster of the lower-priority findings. ## Changes - `README.md` — clarifies what the crate provides for the ROAST coordinator vs what lives in the host-side Go layer (the coordinator failover loop), and drops the stale reference to the retired `phase5_roast` Criterion target's specific name (the target was retired along with the coarse operations it measured; the README now says so generically). - `docs/rust-rewrite-bootstrap.md` — adds a scope note flagging the FFI surface it documents (`frost_tbtc_run_dkg`, `frost_tbtc_start_sign_round`, `frost_tbtc_finalize_sign_round`) as historical; the present surface is the fine-grained `DkgPart1/2/3` plus the interactive `SessionOpen`/`Round1`/`Round2`/`Abort`/`Aggregate` family, documented in the README and `include/frost_tbtc.h`. - `engine/codec.rs` and `engine/interactive.rs` — reworded comments that referenced removed functions or described implementation history rather than current behavior. - `engine/tests.rs` — added a comment documenting why the intentionally-misspelled `polciy_max_output_count` field name in the `deny_unknown_fields` regression test must stay misspelled (correcting it would either pass the parse or fail for the wrong reason, defeating the test). ## Out of scope - New documentation (this is drift cleanup, not docs expansion).
… cluster E) (#4265) ## Summary Crypto / security investigation cluster of the lower-priority findings: two P2/P3 items investigated, confirmed non-issues, and recorded in place so future reviewers do not re-open them. ## Findings documented - `engine/audit.rs` (sighash differential-fuzzer reference leg, `input_index` byte order): confirmed correct (little-endian), verified against the rust-bitcoin 0.32.8 actual `SighashCache` encoding path with source-line references. Also notes the existing fuzz corpus is single-input only, so this specific property cannot be caught by the test alone — the LE choice is anchored on the rust-bitcoin source, not on the test passing. - `engine/policy.rs` (apparent "unconditional Ok when firewall disabled" near `BuildTaprootTx`): the apparent gap is not a bypass. The `tx_result` presence check and every deeper check in the function are one firewall, and disabling it correctly skips all of them by design. The reorder that would have moved the presence check before the early return was prototyped, measured, and reverted because it broke 58 interactive-session lifecycle tests that legitimately use the dev-mode escape hatch. Production is unaffected either way since `signing_policy_firewall_enforced()` always returns `true` in production. ## No code behavior changes Both items are documentation-only. The test suite is unchanged: 278 lib tests pass, 1 pre-existing ignore unrelated.
# C1 — Collapse interactive-session entry points Targets `origin/extraction/frost-signer-mirror-2026-05-26` (the source branch for PR #4005). Independent, can run in parallel with the other 4 spec PRs. This PR ships BOTH the deepening spec doc AND its implementation: a new design spec at `docs/specs/frost-signer-interactive-session-deepening.md`, and the corresponding Rust source change in `pkg/tbtc/signer/src/engine/interactive.rs` that introduces the `enter_phase()` / `flush_pending_marker()` helpers and wires them into all 5 phase-entry handlers. ## What this PR is Deepening spec + implementation for `pkg/tbtc/signer/src/engine/interactive.rs`, covering one of the six deepening candidates surfaced in the architecture review (Candidate 1, paired with the new-Strong Candidate 5). `interactive.rs` exposes five `pub fn` phase handlers (`interactive_session_open`, `interactive_round1`, `interactive_round2`, `interactive_aggregate`, `interactive_session_abort`) whose bodies interleave: - A **2-statement lock-prologue** (`state()?.lock().map_err(...)?` + `sweep_expired_interactive_state_durably(&mut guard)?`) that is **byte-identical at all 5 phase-entry sites** (interactive.rs:321, 789, 881, 1273, 1649). A 6th `"engine lock poisoned"` string at interactive.rs:1456 is a nested lock inside `interactive_aggregate`'s body, not a 6th prologue, and is out of scope. - A **marker-durability dance** (re-persist on a prior pending marker, then write the new one) at 3 sites (open / round2 / aggregate). - Phase-specific business logic. The implementation introduces 2 helpers inside `engine::interactive`: - `pub(crate) fn enter_phase() -> Result<MutexGuard<EngineState>, EngineError>` — captures only the truly-shared 2-statement lock-prologue. - `pub(crate) fn flush_pending_marker(guard, predicate)` — captures the marker-durability dance with caller-supplied predicate. Both are `pub(crate)` so the test suite can exercise them in isolation. Both stay inside `engine::interactive`; no new module is created. ## Key design decision: enter_phase does NOT bundle config-loading An earlier draft of this spec had `enter_phase()` optionally call `load_auto_quarantine_config()?` via a bool parameter. That design was rejected during verification: `open` (interactive.rs:324) and `round2` (interactive.rs:894) both currently call `load_auto_quarantine_config()?`, but **at different relative positions**. `open` calls it immediately after the lock-prologue; `round2` calls it AFTER its `flush_pending_marker(...)` step. Folding config-loading into `enter_phase` would run it before `round2`'s marker-durability flush — reordering a security-relevant durability repair behind a fallible config parse, so a misconfigured `TBTC_SIGNER_AUTO_QUARANTINE_FAULT_THRESHOLD_ENV`/allowlist env var could skip a fail-closed marker repair it doesn't skip today. … `enter_phase` returns the guard only. Each phase keeps its own `load_auto_quarantine_config()?` call at its current position. ## What stays unchanged - The five `pub fn` phase handlers keep their names and signatures. - All `engine::tests::*` paths under `scripts/run_phase5_chaos_suite.sh` stay valid (no test renames). - The `TBTC_SIGNER_ABI_*` constants are not bumped. - The `HardeningOperationLatencyGuard::success_only` placement (round1, round2, aggregate) is unchanged — adding it to `enter_phase` would mean the open and abort phases also paid a latency-tracking cost they don't pay today. ## Verification Spec was verified against the current source tree end-to-end. Verified facts that the spec encodes: - 5 phase-entry lock-prologues, byte-identical, at interactive.rs:321, 789, 881, 1273, 1649. - `load_auto_quarantine_config()` called only at open:324 and round2:894. - `HardeningOperationLatencyGuard::success_only` used at round1:778, round2:858, aggregate:1230 (3 sites). - `engine::tests::interactive_round2_persist_fault_leaves_nonces_live` at tests.rs:9847. - Marker-durability sites all 3 dance with phase-specific predicates. ## Dependencies None of the other 4 spec PRs (C2, C4, C5, C6) introduces a dependency on this one. C5 (`SessionState` grouping) touches the same `interactive.rs` file but at disjoint lines: C1 rewrites only the lock-prologue and marker-durability blocks at the top/end of each phase handler; C5 rewrites the ~89 field-access sites in the phase-specific business logic between them. A rebase is mechanical, not a semantic conflict. ## Files in this PR - `docs/specs/frost-signer-interactive-session-deepening.md` (195 lines, design doc) - `pkg/tbtc/signer/src/engine/interactive.rs` (+52 / −28): adds `enter_phase()` and `flush_pending_marker()` helpers; wires them into all 5 phase-entry handlers (`interactive_session_open`, `interactive_round1`, `interactive_round2`, `interactive_aggregate`, `interactive_session_abort`); collapses the 2-statement lock-prologue at each site to a single `enter_phase()?` call and the 3-line marker-durability dance at `round2` and `aggregate` to a single `flush_pending_marker(...)` call.
# C6 — Macro the FFI entry-point pattern Targets `origin/extraction/frost-signer-mirror-2026-05-26` (the source branch for PR #4005). Independent, can run in parallel with the other 4 spec PRs. This PR ships both the design spec AND the macro implementation in a single branch: - **Spec** — `docs/specs/frost-signer-ffi-macro-family.md` (new file, design doc). - **Implementation** — new `pkg/tbtc/signer/src/ffi_macros.rs` (66 lines, holds the three macros) and a 310-line rewrite of `pkg/tbtc/signer/src/lib.rs` that replaces the 26 mechanical entry-point bodies with 1-line macro invocations. - **Header narrative** — `pkg/tbtc/signer/include/frost_tbtc.h` gains the safety-narrative comments required by the spec's "C header documentation" section (mirrored from the Rust-side docs where the spec calls for it; moved for the 6 interactive entries per the "Safety-narrative location" subsection). The macro expansion is a pure source-form compression of the previous code: every existing call site behaves identically because the macro emits the exact body it replaces. ## What this PR is Deepening spec for `pkg/tbtc/signer/src/lib.rs` (was 1,566 lines before this PR; the macro refactor shrank the entry-point bodies, so the file is now 1,324 lines while still exposing all 29 symbols). The module exposes **29 `#[no_mangle] pub extern "C"` FFI entry points**. Of the 29 symbols in `include/frost_tbtc.h`: - **23 share the same six-line body**: `ffi_entry(|| { parse_request::<T>(..)?, engine::fn(request)?, serialize_response(&response) })` - **3 share a shorter, related no-request body** (`frost_tbtc_canary_rollout_status`, `frost_tbtc_roast_liveness_policy`, `frost_tbtc_hardening_metrics`) - **3 are outliers with distinct shapes** (`frost_tbtc_version`, `frost_tbtc_abi_version`, `frost_tbtc_free_buffer`) 23 + 3 = **26 macro-eligible entries**; 3 are hand-written outliers. Today the variation is hand-written, which means: - A new FFI entry (e.g. a future Phase-7.3 surface) is a 10-line copy-paste. - A future refactor that changes the parse / call / serialize pipeline must touch 26 entries. - The 3 outliers are visually similar to the 26 and could be silently introduced to the wrong shape. ## Proposed change Introduce a 3-macro family in a new `ffi_macros` module (`pkg/tbtc/signer/src/ffi_macros.rs`): ```rust // ffi_macros.rs macro_rules! ffi_request_response { ... } pub(crate) use ffi_request_response; macro_rules! ffi_no_request { ... } pub(crate) use ffi_no_request; macro_rules! ffi_no_request_infallible { ... } pub(crate) use ffi_no_request_infallible; ``` `lib.rs` gains `mod ffi_macros;` and each entry-point body becomes a 1-line invocation, e.g.: ```rust use crate::ffi_macros::ffi_request_response; ffi_request_response!(frost_tbtc_init_signer_config, InitSignerConfigRequest, engine::init_signer_config); ``` The 3 outliers stay hand-written with a one-liner comment explaining why the macro does not apply. ### Why `pub(crate) use` (and not `#[macro_export]` or `pub(crate) macro_rules!`) - **`#[macro_export]`** places the macro at the crate root as crate-public — usable by external dependents. The opposite of "macros are not exported to crate consumers". - **`pub(crate) macro_rules!`** is not valid syntax on stable Rust — E0364: `macro_rules!` has no visibility-modifier syntax. The `pub` on `macro_rules!` is gated behind the unstable, unstabilized `pub_macro_rules` feature (tracking issue rust-lang/rust#78855). - The correct stable pattern (since Rust 2018) is bare `macro_rules!` + explicit `pub(crate) use` re-export: ```rust macro_rules! ffi_request_response { ... } pub(crate) use ffi_request_response; ``` This makes the macro reachable at `crate::ffi_macros::ffi_request_response!` inside this crate without leaking it to external crates. ### Why `crate::ffi::...` paths (and not `$crate::ffi::...`) `$crate` resolves to the macro's defining crate at the expansion site — useful for `#[macro_export]`-exported macros used from external crates. This macro is `pub(crate) use`-scoped and only ever expands inside `pkg/tbtc/signer`, so plain `crate::` is simpler and equally correct. Every invocation site is inside this crate. ## Each macro's expansion is byte-equivalent to its hand-written body The macro expansion is a pure source-form compression of the current code — `cargo` produced no diff in behavior: - The 23 `ffi_request_response!` entries keep the six-line parse/call/serialize body. - The 1 `ffi_no_request!` entry and 2 `ffi_no_request_infallible!` entries keep their respective shorter bodies. - The 3 outliers hand-write their bodies with a one-liner comment. ## What stays unchanged - The FFI surface: same 29 `#[no_mangle] pub extern "C"` symbols, same signatures. The header symbol table is byte-for-byte equivalent modulo added documentation comments. - The `TBTC_SIGNER_ABI_*` constants are not bumped. - The `TbtcSignerResult` repr (`#[repr(C)]`) at ffi.rs:20 is unchanged. - The panic-redaction wrapper (`install_redacting_panic_hook`); the 3 outliers are carefully excluded so they cannot accidentally trigger the hook. - The request bytes validation (length cap, null-ptr check). - The `to_ffi_buffer` source-vec zeroize path. - The `free_buffer` ABI (`(ptr: *mut u8, len: usize) -> void`), header at include/frost_tbtc.h. - The `init_signer_config` global side effect on first call (panic-redaction hook installation). The spec notes this as a macro caveat for the reader. - The chaotic-suite test-path pins in `scripts/run_phase5_chaos_suite.sh`. ## Safety-narrative location The C header (`include/frost_tbtc.h`) gains expanded comments for each entry that previously had a Rust-side doc comment. The safety narrative moves: - "Caller MUST release `TbtcBuffer.buffer` exactly once via `frost_tbtc_free_buffer`" — STAYS in `frost_tbtc_free_buffer`'s Rust doc AND added to the C header (the C side is the boundary the bridge reads). - "Phase 7.1 hardened interactive signing session (frozen spec `docs/phase-7-interactive-session-spec-freeze.md`)" — moves to the C header for the 6 interactive entries. - "Reserved response shape for a future cryptographic share-refresh protocol" — STAYS in `frost_tbtc_refresh_shares` Rust doc; the C header gets a short comment. ## Verification Spec was verified against current code. Verified facts that the spec encodes (corrections from earlier drafts): - `lib.rs` had 29 `#[no_mangle]` entries (not 28 as in earlier draft) in 1,566 lines (not 1,558); after the macro refactor the file is 1,324 lines with the same 29 symbols. - 23 entries share the six-line body (not 26 as in earlier draft). - The 22-vs-23 table/text mismatch in earlier draft is resolved to 23. - `ffi::tests` submodule at ffi.rs:230-318 was 88 lines; this PR adds `outliers_are_hand_written` to it. - `TbtcSignerResult` has `#[repr(C)]` at ffi.rs:20. - `cargo check --tests` is green. The new regression test passes. ## Dependencies None of the other 4 spec PRs (C1, C2, C4, C5) introduces a dependency on this one. This PR touches `lib.rs` (the entry-point bodies), `include/frost_tbtc.h` (added safety-narrative comments only — no signature changes), and the new `ffi_macros.rs` file — no engine source. ## Files in this PR - `docs/specs/frost-signer-ffi-macro-family.md` (new, 259 lines, design doc) - `pkg/tbtc/signer/src/ffi_macros.rs` (new, 66 lines, three macros + their `pub(crate) use` re-exports) - `pkg/tbtc/signer/src/lib.rs` (rewritten entry-point bodies: 310 lines of change, 26 entries converted) - `pkg/tbtc/signer/src/ffi.rs` (no source change; new `outliers_are_hand_written` regression test added in the existing `tests` submodule) - `pkg/tbtc/signer/include/frost_tbtc.h` (comments-only: ownership-contract block for `frost_tbtc_free_buffer`, mirrored "Reserved response shape" prefix for `frost_tbtc_refresh_shares`, and the exact `Phase 7.1 hardened interactive signing session (frozen spec ...)` text for the 6 interactive entries) ## Regression test The spec's `Testing Decisions` section promised a `ffi::tests::outliers_are_hand_written` regression test that verifies the 3 outliers remain hand-written (i.e., they are not silently routed through one of the three macros in a future refactor). That test is added in this PR: it reads `lib.rs` via `include_str!`, finds each outlier function, and asserts that an explanatory comment is adjacent to its definition and that none of the three macro invocations appears inside its body.
…026-05-26' into pr-4258
#4255 cluster A) (#4261) ## Summary Validation and bounds hardening cluster of the lower-priority findings from the PR #4005 review, focused on FFI-adjacent inputs. ## Changes - `DkgPart1Request.min_signers >= 2` check in `frost_ops.rs` before any downstream use. - Bounded deserializers on FFI-adjacent vec / map fields in `api.rs` (`round1_packages`, `round2_packages`, `verifying_shares`, `commitments`, `signature_shares`, `AttemptTransitionTelemetry` and `DeriveInteractiveAttemptContextRequest` u16 vecs). Prevents unbounded allocation from a hostile or malfunctioning host. - `deny_unknown_fields` on `AdmissionCandidate` and `AdmissionOverridePayload` in the admission_checker binary, plus `deny_unknown_fields` on `AdmissionOverrideArtifact` (the operator-supplied override artifact), so silently-ignored typos in operator-authored JSON are rejected. - Capped `OverrideReplayRegistry` entry count and added a 1 MiB cap on JSON input file size in the admission_checker. - Reject `approved_at_unix` values before the year-2001 epoch floor in `apply_dao_override`, closing a reverse-time-travel bypass of the override TTL guard. - Tightened the `ErrorResponse` deserializer caps: `code` / `recovery_class` narrow from `deserialize_bounded_hex` (64 KiB) to `deserialize_bounded_ascii` (256 bytes), and `message` narrows from `deserialize_bounded_hex` (64 KiB) to `deserialize_bounded_message_ascii` (4 KiB). These are short human-readable identifiers / detail strings, not hex payloads, so the smaller caps (with an explicit `is_ascii()` check now matching the docstring "ASCII" claim) keep the in-memory footprint bounded without losing any legitimate payload. ## Tests - 295 lib tests pass, 1 pre-existing ignore unrelated to this change. - 37 admission_checker tests pass. - New tests added for the new security behaviors: `apply_dao_override` year-2001 floor rejection, `OverrideReplayRegistry` cap rejection (with re-keyed existing-key refresh still allowed), `AdmissionCandidate` / `AdmissionOverridePayload` / `AdmissionOverrideArtifact` unknown-field rejection, 1 MiB JSON input cap rejection, `DkgPart1Request.min_signers >= 2` boundary check (reject 0, reject 1, reject > max_signers, accept at threshold), and over-cap rejection for the new bounded deserializer helpers (verifying_shares_map value & entry count, u16 vec, ascii, message ascii, round1_packages_vec). - New `MAX_JSON_INPUT_FILE_BYTES`, `MAX_ASCII_CODES_CHARS`, `MAX_BOUNDED_MAP_ENTRIES`, and `MAX_VERIFYING_SHARE_HEX_CHARS` constants are documented in the source where they are enforced. ## Out of scope - General deserialization cap policy for non-FFI types (the bounded deserializers here target only the FFI-boundary types). - Lint/clippy cleanup of unused imports (separate cluster).
…026-05-26' into pr-4258
…) (#4258) ## Summary Implements GitHub issue #4251: bounded concurrent interactive attempts per signing session. ## Behavior - Distinct members can now hold independent live attempts concurrently within the same per-message session, so a coordinator failover advancing one member's attempt no longer collides with another member's still-live attempt on the prior round. - Caps concurrent attempts per session at `n - t + 1` (n = DKG participant count, t = threshold). Open past the cap fails closed with `concurrent_attempt_cap_exceeded`. - Implementation: per-session `interactive_signing` restructured from a flat `member_identifier -> state` map to a nested `attempt_id -> member_identifier -> state` map. Empty attempt scopes are dropped immediately so the outer map only ever holds non-empty scopes; `n - t + 1` is the natural cap on the outer map. ## Fixes two correctness regressions surfaced by the full test run - **`interactive_round2` stopped inserting the durable consumed-attempt and aggregate-authorization markers** before persisting, silently breaking every Round2 -> Aggregate handoff (the only remaining authorization fallback also fails post-Round2 since Round2 clears `round1` state). Both inserts restored at the pre-persist point. - **`interactive_state_for_attempt_mut` lost the ability to distinguish** "member has no live attempt at all" from "member's live attempt moved to a different attempt_id" once the map became attempt-id keyed. Restored the distinction via a fallback scan of the other live attempt scopes, so a stale-attempt-id Round1/Round2 call returns the specific attempt-id mismatch instead of a generic not-found. ## Tests - 280 lib tests pass, 1 pre-existing ignore unrelated to this change. - New coverage: `interactive_two_attempts_coexist_same_session` (two attempts on the same session, sibling nonces survive while the primary is aggregated), `interactive_concurrent_attempt_cap_enforced` (cap rejects Open past `n - t + 1` with `concurrent_attempt_cap_exceeded`). - Pre-existing interactive tests (Round1 / Round2 / Aggregate / Abort / heartbeat / heartbeat-intent recheck / quarantine / persistence fault injection / etc.) all green. - Pre-existing non-interactive tests unaffected. - Also removed an unused `interactive_session_attempt_count` helper that was identical to `.len()` per its own doc comment. ## Out of scope - Cross-session cap (this is per-session only; cross-session budget is the existing `max_live_interactive_sessions_limit`). - Coordinator selection retry behavior (the `attempts` count is the cap, not a retry budget).
…4262 log-hygiene fix into envelope_io.rs)
# C2 — Carve persistence into seams Targets `origin/extraction/frost-signer-mirror-2026-05-26` (the source branch for PR #4005). **NOT independent** from PR #5 (C5 `SessionState` grouping) — both rewrite the bodies of the same two `TryFrom` impls in `persistence.rs`. Sequencing note in both PRs; hand-merge required at the `TryFrom` bodies. This PR ships two commits: a spec/doc commit (`spec(frost-signer): carve persistence into seams`) and the implementation commit (`feat(frost-signer): carve persistence into seams`) that performs the 4-module persistence split described in the spec. The implementation is byte-for-byte stable on the wire (on-disk file format and persisted schemas unchanged), and the full test suite passes; the body of this PR is the design doc + the matching restructuring. Subsequent commits beyond these two are doc-only corrections to the spec doc and the PR description itself. ## What this PR is Deepening spec for `pkg/tbtc/signer/src/engine/persistence.rs` (currently 2,389 lines). Splits one monolithic file into 4 `pub(crate)` modules under `engine::persistence/`: ``` schema_codec // pure TryFrom + encode/decode + schema-version validation envelope_io // file I/O, lock, atomic rename, corrupt recovery, backup retention key_provider // StateKeyProvider trait + EnvKeyProvider + CommandKeyProvider // + CachedStateKeyProvider (real production 3rd adapter) // + state_encryption_key_material() thin wrapper pending_ops // registry + snapshot covering + durable retry ``` The 4 concerns are independently testable but currently require each other in the test environment. The boundary between them is invisible — and the highest-stakes functions in the codebase (AEAD envelope verification, key-provider subprocess management, schema-version migration) are the ones that most need clean per-concern test surfaces. ## Trait shape (verified against current code) ```rust pub(crate) trait StateKeyProvider: Send + Sync { fn material(&self) -> Result<StateEncryptionKeyMaterial, EngineError>; fn key_id(&self) -> &str; } ``` Three real production adapters live in `key_provider`: - `EnvKeyProvider` — reads `TBTC_SIGNER_STATE_ENCRYPTION_KEY_HEX` env. - `CommandKeyProvider` — runs the configured `TBTC_SIGNER_STATE_KEY_COMMAND`, with a timeout enforced by `state_key_command_timeout_secs()`. - `CachedStateKeyProvider` — process-lifetime cache decorator wrapping any `Box<dyn StateKeyProvider>`; keyed on `key_id()` so a fresh `material()` is only invoked when the provider identity changes. **New** in this PR — the pre-split code re-resolved the key (and re-spawned `TBTC_SIGNER_STATE_KEY_COMMAND` for the command provider) on every `state_encryption_key_material()` call. The caching is a deliberate, beneficial behavior change; see the spec doc's `key_provider` section and `Risks` subsection for details. No test-only fake type moves into this module. Tests continue to define their own `StateKeyProvider` impls locally (current convention: `TrackingMockKeyProvider` + `StdArcMockProvider` newtype), wrapped through `CachedStateKeyProvider` for cache-coverage tests. ## Wired data flow ``` load_engine_state_from_storage (envelope_io) └── decode_persisted_state_storage_format (envelope_io) └── decode_encrypted_state_envelope (envelope_io) └── state_encryption_key_material (key_provider) ← decrypts the envelope └── decode_state (schema_codec) ← pure persist_engine_state_to_storage (envelope_io) └── state_encryption_key_material (key_provider) ← encrypts the envelope └── persist_engine_state_to_storage_with_key (envelope_io) └── encode_state (schema_codec) ← pure └── atomic state-file replacement (envelope_io) └── sync_state_file_parent_directory (envelope_io) └── clear_snapshot_covered_operations (pending_ops) ← only on successful replacement, not on load ``` `persist_engine_state_to_storage_with_key` is a 2nd `pub(crate)` envelope_io entry point that takes an already-resolved key — callers that need the key resolved under a specific lock ordering (e.g. round2's durable-marker write, which must resolve the key before writing the marker) call it directly and skip the wrapper's own key resolution. ## What stays unchanged - The on-disk file format. A state file written by the previous code loads cleanly with the new code, and vice versa. - The persisted schema: `PersistedEncryptedEngineStateEnvelope`, `PersistedEngineState`, `PersistedSessionState`, `PersistedKeyPackage` — field names, ordering, and serde tags preserved byte-for-byte. - The `TBTC_SIGNER_ABI_*` constants are not bumped. - The `STATE_FILE_LOCK` mutex and the `#[cfg(unix)]`/`#[cfg(not(unix))]` subprocess-group-keying branching are preserved. ## Sequencing with PR #5 (C5) Both PRs rewrite the bodies of `TryFrom<PersistedSessionState> for SessionState` (persistence.rs:1917-2233) and `TryFrom<&SessionState> for PersistedSessionState` (persistence.rs:2235 onward). This PR moves those bodies into `schema_codec.rs` unchanged in shape; PR #5 reshapes what they project into (flat fields → 6 substructures). **Merge order:** land whichever is easier to rebase the other onto (either order works structurally), then hand-merge the shared `TryFrom` bodies. Do not apply both to a worktree in parallel and expect a clean merge. ## Verification Spec was verified against current code. Verified facts that the spec encodes (corrections from earlier draft): - `persistence.rs` is 2,389 lines (not 2,256 as in the earlier draft). - `StateKeyProvider` trait methods are `material()` and `key_id() -> &str` (the earlier draft had fictional `resolve()` / `key_id() -> String`). - 3 real production adapters exist; no `InMemoryKeyProvider` (the earlier draft invented one). - `clear_snapshot_covered_operations` is `fn` (private) with parameter `engine_state: &EngineState`. Called from `persist_engine_state_to_storage` (success path), not from `load_engine_state_from_storage`. Earlier draft had it on the load path with the wrong caller. - `legacy_plaintext_state_permitted` stays `fn` (private); only called inside `envelope_io`. - `pending_build_taproot_tx_operation` (single `session_id` arg, name with `-taproot-` not `-tx_persistence_pending-`); the earlier draft name was wrong. - `clear_persistence_pending_operations` is `cfg(any(test, feature = "bench-restart-hook"))`, not just `cfg(test)`. - `state_lock_rejects_multi_process_contention` lives at tests.rs:5787 (not `state_file_lock_rejects_multi_process_contention` at line ~5700 as in the earlier draft). ## Files added Spec doc: - `docs/specs/frost-signer-persistence-deepening.md` (design doc, ~238 lines) Implementation (omitted from base commit `555f2c514`; added by the second commit on this branch): - `pkg/tbtc/signer/src/engine/persistence/mod.rs` (re-exports the four submodules) - `pkg/tbtc/signer/src/engine/persistence/schema_codec.rs` (pure TryFrom + encode/decode + schema-version validation) - `pkg/tbtc/signer/src/engine/persistence/envelope_io.rs` (file I/O, lock, atomic rename, corrupt recovery, backup retention) - `pkg/tbtc/signer/src/engine/persistence/key_provider.rs` (StateKeyProvider trait + EnvKeyProvider + CommandKeyProvider + CachedStateKeyProvider + subprocess machinery) - `pkg/tbtc/signer/src/engine/persistence/pending_ops.rs` (registry + snapshot covering + durable retry) Removed: - `pkg/tbtc/signer/src/engine/persistence.rs` (2,256 lines replaced by the four submodules above + `mod.rs`; contents preserved byte-for-byte across the split) ## Revisited risks (now landed in this PR) The 4 split modules talk to each other without a circular dependency: `envelope_io` calls `key_provider::state_encryption_key_material()` for both load and persist paths, and the cross-module `clear_snapshot_covered_operations` call from `envelope_io::persist_engine_state_to_storage` (success path only) to `pending_ops` is one-directional. The resolution path is `envelope_io → key_provider` and `envelope_io → pending_ops`; `key_provider` and `pending_ops` do not call back into `envelope_io`. The `mod.rs` orchestration preserves the pre-split call ordering.
# C5 — Split `SessionState` into 6 named substructures Targets `origin/extraction/frost-signer-mirror-2026-05-26` (the source branch for PR #4005). **NOT independent** from PR #2 (C2 persistence split) — both rewrite the bodies of the same two `TryFrom` impls in `persistence.rs`. Sequencing note in both PRs; hand-merge required at the `TryFrom` bodies. This PR ships both the deepening spec (`docs/specs/frost-signer-sessionstate-grouping.md`) AND the full implementation: 10 files, +842/-616, splitting `SessionState` into 6 named substructures and migrating every call site. Behaviorally a pure refactor (persisted schema byte-identical; cross-field invariants preserved verbatim); the risk lives in the migration's correctness, not its scope, so reviewers expecting a doc-only change should read the diff before relying on that label. ## What this PR is This PR splits `pkg/tbtc/signer/src/engine/state.rs`'s `SessionState` (a 31-field flat struct at state.rs:111-183 holding 6 unrelated concerns — Round2, Aggregate, Refresh, Audit, etc.) into 6 named substructures, still under `engine::state` (no new module): ``` SessionState { dkg: DkgSessionState, // 5 fields (request fingerprint, // key packages, public key package, // result, policy snapshot version) signing: LegacySigningSessionState, // 12 fields interactive: InteractiveSessionState, // 5 fields audit: AuditTrail, // 1 field lifecycle: LifecycleState, // 5 fields capacity_pins: OperationalState, // 3 fields } ``` The grouping is grounded in call-site co-location: each group is co-read at the call sites that exclusively touch that concern. ## What stays unchanged - The persisted schema `PersistedSessionState` is unchanged. Field names, ordering, serde tags preserved byte-for-byte. A state file written by the previous code loads cleanly with the new code, and vice versa. - The `TryFrom<PersistedSessionState> for SessionState` impl projects into the new substructures; the inverse `TryFrom<&SessionState> for PersistedSessionState` reads them and flattens back to the same 31 fields. - The `refresh_count = persisted.refresh_count.max(history.len() as u64)` legacy semantics are preserved (lifecycle.rs ignores this until a versioned cryptographic refresh protocol exists; the field is retained only for schema compatibility). - The wire schema version is not bumped. - The `TBTC_SIGNER_ABI_*` constants are not bumped. - The `Drop for InteractiveSigningState` implementation (state.rs:105-109) is unchanged — only its containing field path changes. ## Migration cost Roughly **147 production `.field` read/write sites** spread across production consumers: | File | `.field` sites | Notes | |------|---------------:|-------| | `interactive.rs` | ~89 | heaviest consumer; all 5 content groups touched | | `dkg.rs` | ~17 | `dkg.*` + `interactive.bound_key_group` | | `state.rs` | ~13 | mostly the exhaustive destructuring in `per_message_interactive_session` (state.rs:553-621) | | `lifecycle.rs` | ~13 | `lifecycle.*` + `dkg.result` + `interactive.bound_key_group` | | `transaction.rs` | ~9 | `signing.*` + `lifecycle.emergency_rekey_event` | | `verify_share.rs` | ~3 | `interactive.bound_key_group` + `dkg.public_key_package` | | `audit.rs` | ~2 | `audit.attempt_transition_records` at audit.rs:335 and audit.rs:397 | | `telemetry.rs` | ~1 | `lifecycle.emergency_rekey_event` at telemetry.rs:521 | | **Total (excl. persistence.rs, tests.rs)** | **~147** | | `persistence.rs` is excluded from this count: its `TryFrom` impls read `SessionState` AND `PersistedSessionState` under the same field names verbatim, so a plain-text grep cannot distinguish the two. Both `TryFrom` bodies are rewritten regardless (see "TryFrom projection" below and PR #2's "Sequencing with PR #5"). Struct literals: **21 in `tests.rs`** + **1 inside `TryFrom<PersistedSessionState>` impl in `persistence.rs`** = **22 total** rewrite to the nested form. The `PersistedSessionState { ... }` literal at **tests.rs:623** is the wire schema and intentionally unchanged. ## TryFrom projection (key invariant) The `TryFrom<PersistedSessionState> for SessionState` impl (persistence.rs:1917-2233 today) validates the consumed-marker registries and hex-decodes the DKG/sign-message fields exactly as it does today, then projects into the new substructures in a single literal. **`OperationalState` is NOT wholesale-defaulted.** Three real fields with mixed persistence: - `retired_interactive_at_unix` — **persisted** (round-tripped through both `TryFrom` impls). - `heartbeat_rate_limiter` — **transient** (not serialized; resets on restart). - `aggregate_eviction_pin` — **transient** (Arc<()> refcount pin; an in-flight Aggregate clones it under the engine lock). Defaulting the whole `OperationalState` to `::default()` would silently drop `retired_interactive_at_unix` on every restart — a wire-schema-breaking behavior change. ### Cross-field retirement invariant The existing invariant `if session.capacity_pins.retired_interactive_at_unix.is_some() && !per_message_interactive_session(&session) { return Err(...) }` (persistence.rs:2122-2129 post-implementation) spans 3 groups (`capacity_pins` for the retired timestamp, `interactive` and `dkg` via the discriminator). The spec's DoD promised a dedicated unit test pinning this exact TryFrom-level check; this PR ships that test (`engine::tests::persisted_session_state_rejects_retired_interactive_on_non_per_message_session`) so a future migration cannot silently drop or invert the invariant. A second test, `persisted_session_state_round_trip_preserves_capacity_pins_retired_interactive_at_unix`, pins that `OperationalState::retired_interactive_at_unix` survives a `TryFrom<&SessionState>` -> `TryFrom<PersistedSessionState>` round-trip (defense against a future maintainer wholesale-defaulting `OperationalState` in either direction). ## Sequencing with PR #2 (C2) Both PRs rewrite the bodies of `TryFrom<PersistedSessionState> for SessionState` and `TryFrom<&SessionState> for PersistedSessionState`. PR #2 moves those bodies into `schema_codec.rs` unchanged in shape; this PR reshapes what they project into. **Merge order:** land whichever is easier to rebase the other onto (either order works structurally), then hand-merge the shared `TryFrom` bodies. Do not apply both to a worktree in parallel and expect a clean merge. ## Sequencing with PR #1 (C1) Both touch `interactive.rs`, on disjoint lines: PR #1 rewrites only the lock-prologue and marker-durability blocks at the top/end of each phase handler; this PR rewrites the ~89 field-access sites in the phase-specific business logic between them, which PR #1 leaves untouched. A rebase is mechanical, not a semantic conflict. ## Verification Spec was verified against current code. Verified facts that the spec encodes (corrections from earlier drafts): - `SessionState` has 31 fields (state.rs:111-183), including `policy_snapshot_version: u32` at state.rs:161 — added since the original draft was written. Placed in `DkgSessionState` by name and doc comment (no other production reader besides `policy::current_policy_snapshot_version()` which is currently unreferenced; groups with `dkg` rather than `interactive`). - Legacy-signing group is **12 fields** not 10 (4 consumed-marker registries + sign request fingerprint, message bytes, round state, active attempt context, finalize request fingerprint, signature result, build-tx request fingerprint, transaction result). - 21 `SessionState{...}` literals in `tests.rs` (not 22 as in earlier draft); 1 `SessionState{...}` literal inside `TryFrom<PersistedSessionState>` impl in `persistence.rs` (not 2; the 2 reverse `TryFrom<&SessionState>` constructs `PersistedSessionState{...}` which is the wire schema and unchanged). - Cross-field retirement invariant at persistence.rs:2222-2230 (was 2093-2099 in earlier draft). - `engine::tests::persisted_session_state_round_trip_preserves_bound_key_group` at tests.rs:4615 (was ~4729 in earlier draft). - Audit trail reads at audit.rs:335 and audit.rs:397 (was audit.rs:302 in earlier draft, which was wrong). - Various other line citations corrected. ## Files added - `docs/specs/frost-signer-sessionstate-grouping.md` (design doc, the deepening spec) - 9 Rust source files updated by the implementation commit (10 files total including the spec): the new substructures plus all call-site migrations. See "Migration cost" above for the per-file site count and "Verification" below for the file:line citations.
…026-05-26' into spec/c4-policy-reject-funnel # Conflicts: # pkg/tbtc/signer/src/engine/tests.rs # pkg/tbtc/signer/src/engine/transaction.rs
# C4 — Funnel the policy `reject_*` family Targets `origin/extraction/frost-signer-mirror-2026-05-26` (the source branch for PR #4005). Independent, can run in parallel with the other 4 spec PRs. This PR ships BOTH the design doc and its implementation: a new `docs/specs/frost-signer-policy-reject-funnel.md` plus a 442-line Rust refactor that introduces the `reject_with` core in `pkg/tbtc/signer/src/engine/policy.rs` and wires it through `interactive.rs`, `telemetry.rs`, and `transaction.rs`. > **Note:** an earlier draft of the PR description labeled this a "pure doc/spec change — no Rust source touched". That was incorrect: this branch contains two commits, the second of which (`a25710457 feat(frost-signer): funnel the policy reject_* family`) is a real implementation change. The framing has been corrected; the rest of the substance below is unchanged. ## What this PR is Deepening spec for `pkg/tbtc/signer/src/engine/policy.rs`, plus the corresponding implementation. The module exposes 7 `reject_*` helpers across 44 production call sites. The signing family (3 helpers, 3 thin wrappers around `reject_signing_policy_with_metric`) already collapses into one funnel. The remaining duplication is concentrated in 3 uncompressed helpers: - `reject_admission_policy` (policy.rs:236) — 17 lines, own metric (`run_dkg_admission_reject_total`). - `reject_quarantine_policy` (policy.rs:510) — 13 lines. - `reject_lifecycle_policy<T>` (policy.rs:524) — 13 lines, generic over `T`. Plus 2 hand-constructed `EngineError::LifecyclePolicyRejected` sites that previously bypassed the log hook entirely: - `interactive.rs:1916` — emergency-rekey kill switch. - `transaction.rs:62` — `build_taproot_tx` entry. ## Proposed change Extract a private core `reject_with(stage, session_id, reason_code, detail) -> EngineError` inside `policy.rs`. **`reject_with` returns `EngineError` directly, not `Result<(), EngineError>`** — every stage always rejects, so returning the bare error is what makes a single-dispatch-point core actually fit (each of the 3 uncompressed helpers already returns the error directly today). The 7 existing helpers stay as `pub(crate)` 1-line wrappers, each `Err(reject_with(...))`: ``` pub(crate) fn reject_admission_policy(...) pub(crate) fn reject_quarantine_policy(...) pub(crate) fn reject_lifecycle_policy<T>(...) // unifies against any T fn reject_signing_policy_with_metric(...) pub(crate) fn reject_signing_policy(...) fn reject_heartbeat_signing_policy(...) fn reject_interactive_rate_limit_signing_policy(...) ``` The 2 hand-constructed sites route through `reject_lifecycle_policy(...)` so the `lifecycle_policy` log stage fires. Each caller's own `Result<T, EngineError>` unifies against the helper's return type: `T = ()` for `enforce_interactive_signing_gates`, `T = TransactionResult` for `build_taproot_tx`. `Err(reject_with(...))` type-checks for every wrapper — there is no `?` and no dead-code branch to reconcile. (An earlier draft wrote `Err(reject_with(...)?)`, which would have been a type bug: the `?` would always return early on the helper's `Err`, leaving the outer `Err(...)` unreachable.) ## Bug premise correction (kept in spec body) An earlier draft's `## Blocked` section claimed `interactive_rate_limit_reject_total` is referenced but not declared in `HardeningTelemetryState`. That premise was wrong: the field **is added by THIS PR** at `telemetry.rs:144` (`pub(crate) interactive_rate_limit_reject_total: u64`), and the implementation wires it through `reject_with` for the `SigningPolicyFirewall { metric: InteractiveRateLimit }` stage so the metric is ready before the parallel candidate's enforcement path lands. The `## Blocked` section was removed from the spec; the field is a new addition, not a pre-existing one. The PR also surfaces the new counter on the public `SignerHardeningMetricsResult` snapshot (with `#[serde(default)]` so existing JSON consumers do not break), so operators can observe the metric via `frost_tbtc_hardening_metrics` once the parallel candidate activates the enforcement path. ## Hand-constructed site corrections The hand-constructed sites use these exact fields (verified against the actual inline `Err` constructs): - `reason_code: "emergency_rekey_required"` (NOT `"emergency_rekey_active"` as in the earlier draft). - `detail`: a `format!` string carrying `emergency_rekey_event.triggered_at_unix` and `emergency_rekey_event.reason`, distinct per site (one prefixed with `"emergency rekey required for session [{}] since [{}]: {}"`, the other with `"build_taproot_tx blocked: emergency rekey required since [{}]: {}"`). ## What stays unchanged - The public `EngineError` variants (`AdmissionPolicyRejected`, `QuarantinePolicyRejected`, `LifecyclePolicyRejected`, `SigningPolicyRejected`, ...) — the bridge does not need to bump its ABI version. - The `log_policy_decision` function signature and behavior — the new core forwards to it. - The `enforce_signing_policy_firewall_inner` `charge_rate_limit: bool` parameterization — preserved, not collapsed. - The `record_canary_policy_outcome` call at `transaction.rs:254`. - The `TBTC_SIGNER_ABI_*` constants are not bumped. ## Test invariants - The 7 helper names are preserved, so the existing test assertions (`metrics.build_taproot_tx_policy_reject_total`, etc.) continue to work. - The `lifecycle_policy` log stage is now emitted for the emergency-rekey kill switch AND for the `build_taproot_tx` entry — the invariant "all rejections go through `log_policy_decision`" is enforced for the first time at those 2 sites. - New coverage in `engine::tests::lifecycle_rejection_fires_log_for_kill_switch_blocking_build_taproot_tx` and `engine::tests::lifecycle_rejection_fires_log_for_kill_switch_blocking_interactive_session` captures the `eprintln!` output of `log_policy_decision` and asserts the `stage=lifecycle_policy` marker is emitted with the `emergency_rekey_required` reason code at each of the two formerly hand-constructed sites. - The chaos suite (`scripts/run_phase5_chaos_suite.sh`) passes. ## Verification Spec was verified against current code. Verified facts that the spec encodes: - `policy.rs`:236/510/524 are the 3 uncompressed helpers' definition sites. - `interactive.rs:1916` (not 1914 as in earlier draft) is the emergency-rekey kill-switch inline `Err`. - `transaction.rs:62` is the `build_taproot_tx` entry inline `Err`. - `transaction.rs:253` is the `matches!(error, EngineError::SigningPolicyRejected { .. })` matches-arm — preserved by the change. - `telemetry.rs:144` (this PR) is the new `interactive_rate_limit_reject_total` field on `HardeningTelemetryState`, plus the corresponding field on `SignerHardeningMetricsResult`. ## Dependencies None of the other 4 spec PRs (C1, C2, C5, C6) introduces a dependency on this one. The C1 spec touches `interactive.rs` for a different reason — its only "touch" at `interactive.rs:1916` is the hand-constructed `Err` site that C4 rewrites; C1's restructuring is scoped to the lock-prologue and marker-durability blocks at the top/end of each of the 5 `pub fn` phase handlers, NOT to internal business-logic helpers like `enforce_interactive_signing_gates` (interactive.rs:1895) that C1 does not touch. ## Files added - `docs/specs/frost-signer-policy-reject-funnel.md` (214 lines, design doc) ## Files modified - `pkg/tbtc/signer/src/engine/policy.rs` — new `reject_with` core, 7 wrapper helpers, 1-line each. - `pkg/tbtc/signer/src/engine/interactive.rs` — emergency-rekey kill-switch site routes through `reject_lifecycle_policy`. - `pkg/tbtc/signer/src/engine/transaction.rs` — `build_taproot_tx` entry routes through `reject_lifecycle_policy`. - `pkg/tbtc/signer/src/engine/telemetry.rs` — new `interactive_rate_limit_reject_total` field on `HardeningTelemetryState`; field surfaced on the public `SignerHardeningMetricsResult` snapshot in `api.rs`. - `pkg/tbtc/signer/src/api.rs` — `interactive_rate_limit_reject_total` added to `SignerHardeningMetricsResult` (with `#[serde(default)]`). - `pkg/tbtc/signer/src/engine/tests.rs` — two new tests for the kill-switch log line; see "Test invariants" above. ## Open follow-ups (not in this PR) The parallel candidate that activates the interactive-rate-limit enforcement path is the only thing this PR does not ship. The funnel, the wrapper, the metric, the snapshot field, and the tests are all in place; the dead-code allowance on `reject_interactive_rate_limit_signing_policy` lifts the moment that candidate lands its caller.
…dential persistence Seven uses: lines across four actions (actions/cache, docker/build-push-action x3, docker/login-action, softprops/action-gh-release) pinned commit SHAs that do not exist upstream (404/422 against the GitHub API). Replaced each with the verified tagged-release commit and dropped the now-stale TODO markers. Also set persist-credentials: false on both checkout steps, matching the convention already used in tbtc-signer-formal.yml.
…crates CI only ran cargo deny check advisories/bans; the [sources] git/path-dependency ban and [licenses] allowlist in deny.toml were configured but never enforced. Switched the CI step to cargo deny check all. Added yanked = "deny" under [advisories] (the default is a non-blocking warn) and corrected a header comment that incorrectly claimed --locked already blocks yanked crates. Running check all locally surfaced a real yanked transitive dependency (spin 0.9.8 via frost-core); bumped it to the non-yanked 0.9.9 patch release.
…rpolation persist_distributed_dkg_key_package only checked that the caller's own verifying share matched the public key package; nothing verified that the OTHER participants' verifying shares Lagrange-interpolate to the same group verifying key. A malformed or buggy coordinator (the local, already-trusted Go host) could hand this node a self-inconsistent package. Enabled frost-core's internals feature (already a direct pinned dependency for aggregate_custom/CheaterDetection) to reach compute_lagrange_coefficient, and added a check that reconstructs the group verifying key from every participant's verifying share at x=0 and compares it byte-for-byte against the package's declared key before the package is trusted as signing material.
…ning surface Auto-quarantine enforcement (blocking members already present in the persisted quarantine set) genuinely works when TBTC_SIGNER_ENABLE_AUTO_QUARANTINE is set, but nothing in this build ever automatically populates that set: there is no in-process detector for coordinator-timeout or invalid-share-proof events, so the set can only be populated externally. The status field and several doc comments claimed automatic behavior that does not exist. - Renamed QuarantineStatusResult.auto_quarantine_enabled to quarantine_enforcement_armed and documented the manual-population requirement on it, on AutoQuarantineConfig, and on auto_quarantine_enabled(). - Wired auto_quarantine_enforcements_total to the real enforcement rejection site in roast.rs (it was previously always zero); removed the three other hardening counters (attempt_transition_total, coordinator_failover_total, auto_quarantine_fault_events_total) that have no writer anywhere in the crate. - Removed the two dead auto-quarantine penalty env knobs (TIMEOUT_PENALTY/INVALID_SHARE_PENALTY): accepted at init, never read by any loader. - Documented why admission_policy_enforced() does not force-enable in production, mirroring the rationale already present for the other two policy gates. - build_taproot_tx's emergency-rekey kill-switch check now resolves the wallet session the same way interactive_round2 already does (via a new get_any_emergency_rekey_event helper), instead of checking only the requested per-signing session. A wallet halted by trigger_emergency_rekey could otherwise still have a transaction assembled against it.
…fig regressions from corruption, harden the lock file Three persistence hardening gaps: - The AEAD state envelope had no freshness binding, so an attacker (or an ops mistake) able to write the state file could restore an older, still validly-encrypted backup with no key needed, un-consuming replay registries and clearing quarantine. Added a monotonic state_generation counter, bound into the AEAD's AAD and tracked in a sibling sidecar file (fsynced before rename, same durability discipline as the state file itself); loading a generation older than the recorded high-water mark is refused. Bumped the envelope schema version (3 -> 4) and kept the pre-generation format (_V3) decodable so existing deployments upgrade in place on next load instead of hard-failing. - A session-registry-size migration (e.g. an operator lowering TBTC_SIGNER_MAX_SESSIONS below the live active-session count) was routed through the same code path as genuine file corruption. Under quarantine_and_reset that would destroy DKG key material over a file that was never actually corrupt. Now returns a distinct, correctly-scoped error before reaching the corruption-recovery path. - The state lock file was opened with no explicit mode or symlink protection at a predictable path. Now opened with mode(0o600) and O_NOFOLLOW on Unix, matching the state envelope's own temp-file convention; O_EXCL intentionally omitted since re-acquiring an existing lock across process restarts is a normal path (flock is the actual cross-process gate).
…n and struct moves Round-1 nonce and key-package zeroization was applied to a moved-out local copy after .take()/.remove() (a bitwise move of the inline value out of a map slot); the original bytes resident in the map's backing allocation were never overwritten. Boxed the secret-bearing fields (Box<Zeroizing<SigningNonces>>, Box<Zeroizing<KeyPackage>>) so container moves relocate only a pointer and the zeroizing drop always reaches the same heap allocation. serde_json::to_vec's internal buffer-growth reallocations during response serialization were not zeroized (only the final buffer was, in to_ffi_buffer/free_buffer); DKG secret-package and key-package responses pass through several such reallocations at realistic participant counts. Added serialize_secret_response, which serializes into a capacity-presized buffer via serde_json::to_writer to make growth unlikely, and wired frost_tbtc_dkg_part2/frost_tbtc_dkg_part3 (the two FFI responses that actually carry secret material) through it via a new ffi_request_secret_response! macro.
…ok docs README's 'Current scope' still advertised the deleted coarse RunDKG / StartSignRound / FinalizeSignRound surface and a dead TBTC_SIGNER_ALLOW_BOOTSTRAP field as current; rewrote it against the actual frost_tbtc.h surface (DkgPart1/2/3, PersistDistributedDkgKeyPackage, interactive signing) and removed the StartSignRound request/response schema and error codes for the now-nonexistent op. Removed the dead allow_bootstrap field/env constant from the engine (config.rs/init_config.rs/api.rs) to match. The 'Canary promotion runbook' referenced env vars and a result field that exist nowhere in the crate; replaced it with a pointer to the real SLO gate env vars already documented above and to the security-rollout-gates doc. Updated the quarantine config docs and the phase-7 spec freeze's section 8 (bounded n-t+1 concurrency) with an EXECUTED note: the implementation already ships attempt-scoped concurrency caps under 'Phase 7.6' comments, ahead of the standalone mini-spec the phasing plan called for; this note stands in for it, matching the EXECUTED-note convention already used for section 7.
…d quarantine-counter fixes - load_engine_state_rejects_replayed_older_backup_by_generation: persists twice, restores the first (older) envelope over the live file, and asserts the reload is refused rather than silently accepted. - load_engine_state_distinguishes_session_over_limit_from_corruption: lowers TBTC_SIGNER_MAX_SESSIONS below an all-active session registry under quarantine_and_reset and asserts the file is left untouched with a config-regression error, not the destructive corruption-recovery path. - Extended the existing DAO-allowlist quarantine test to assert auto_quarantine_enforcements_total increments on a real enforcement rejection. - cleanup_test_state_artifacts now also removes the new generation sidecar file and its temp file, matching the existing lock/temp-file cleanup.
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
pkg/tbtc/signer/, the Rust crate behindlibfrost_tbtc: the FROST threshold-Schnorr signing engine for tBTC v2's migration from tECDSA to Taproot (BIP-340/341) wallets. The Go client consumes it over a versioned cgo/JSON FFI. The crate implements the per-participant distributed FROST DKG rounds, interactive ROAST signing sessions with member-custodied nonces, signature-share verification with attributable blame, Taproot transaction assembly under a signing-policy firewall, and encrypted signer-state persistence — plus the formal models, cross-language vectors, and CI gates that guard all of it.Two properties define the current surface. Signing is interactive-only: the transitional "coarse" one-shot signing path that earlier revisions of this branch carried (
frost_tbtc_run_dkg,frost_tbtc_start_sign_round,frost_tbtc_finalize_sign_round,frost_tbtc_generate_nonces_and_commitments,frost_tbtc_sign_share,frost_tbtc_aggregate) has been deleted. Removing exported symbols is an incompatible contract change, so the structured version reported byfrost_tbtc_abi_versionis now ABI 4.0 and Go bridges fail closed against an incompatible library (major 2 is this coarse-path deletion; major 3 added the additive typed heartbeat intent, its rate limit, and canary-evidence config as minors 3.1-3.3; major 4, in this PR, turns a validRefreshSharescall from a synthetic-success stub into a terminalcryptographic_refresh_not_supportederror, which is a breaking response-shape change for any ABI-3 bridge, hence another major bump rather than a minor). Key generation is a real distributed DKG:dkg_part1/2/3plus gated key-package persistence; the dealer-style bootstrap path is gone from the exported surface and is rejected outright under the production profile.This PR is self-contained: nothing in keep-core consumes the crate yet (the cgo bridge and node wiring live in #3866), so it can land independently and first.
Why this lives in keep-core
Per extraction plan v38 section 3.1:
Provenance
The initial import was extracted from
tlabs-xyz/tbtc:feat/frost-schnorr-migrationat frozen signed tagfrost-extraction-source-v1(commit52389bd5cccb5daeef195671feb7ca46be6e2f37; manifest: https://github.com/tlabs-xyz/tbtc/blob/frost-extraction-source-v1/extraction/frost-extraction-source-manifest.json). That tag audits the initial import only. Since then, all signer development has happened inthreshold-network/keep-core: this branch now carries ~200 commits, almost all landed through individually reviewed sub-PRs merged into it (e.g. #4011, #4018, #4028, #4031, #4036, #4051–#4055, #4062, #4068, #4077, #4098, #4104, #4111–#4114, #4123–#4129, #4136, #4137). keep-core is the source of truth for the signer.Scope
FFI contract and ABI (
src/lib.rs,src/ffi.rs,src/api.rs,include/frost_tbtc.h)frost_tbtc_free_buffer); FFI buffers are zeroized and the production panic hook never reflects raw panic payloads across the boundary.frost_tbtc_abi_versionreturns a structured{abi_major, abi_minor}contract version with documented bump rules — currently 4.0: major 2 is the coarse-path deletion below, major 3 added the additive heartbeat-intent/rate-limit/canary-evidence config (minors 3.1-3.3), and major 4 (this PR) turnsRefreshSharesfrom a synthetic-success stub into a terminal error, a breaking change for ABI-3 bridges.frost_tbtc_init_signer_config(TBTC_SIGNER_*knobs), validated fail-closed: an unknown profile or degenerate signing window is fatal.Distributed DKG (
engine/dkg.rs, persistence gates inengine/persistence.rs/engine/provenance.rs)dkg_part1/2/3per-participant round functions overfrost-secp256k1-tr(pinned to the 3.0.0 final release).persist_distributed_dkg_key_packageaccepts a completed distributed-DKG key package as signing material only behind provenance, admission-policy, and operator-quarantine gates, with participant-count and canonical-ID validation, and verifies the signing share derives to its public share before accepting it.Interactive ROAST signing (
engine/interactive.rs,engine/roast.rs,engine/verify_share.rs,engine/frost_ops.rs)(session_id, attempt_id, member_identifier)with durable consumption markers; multi-seat operators get member-keyed session state.interactive_aggregateself-verifies the tweaked (key-path or script-path) signature, is idempotent via completion markers bound to the message and taproot root, and emits candidate-culprit blame for invalid shares.verify_signature_sharebacks the Go-side Round2 share verifier, including tweaked-root equivalence.derive_interactive_attempt_contextgives the host a canonical attempt-context derivation so session ids stay in contract.go_math_rand.rspins bit-exact Gomath/randparity).Taproot transactions and signing policy (
engine/transaction.rs,engine/policy.rs)build_taproot_txassembles validated unsigned Taproot transactions from provided inputs/outputs.State and secret-material hardening (
engine/state.rs,engine/persistence.rs,engine/audit.rs,engine/telemetry.rs,src/bin/admission_checker.rs)admission_checkerbinary plus sample policy JSONs for operator-side admission validation.Formal verification and vectors (
docs/formal/models/,scripts/formal/,tests/,test/vectors/,testdata/)formal_verification_invariant test suite.CI (
.github/workflows/tbtc-signer-formal.yml)cargo fmt --check,cargo clippy --all-targets -- -D warnings,cargo test, all with--locked), Signer dependency audit (blocking cargo-deny RustSec advisory gate), Signer formal invariants, and TLA model checks. Actions are pinned to commit SHAs with checkout credential persistence disabled.Docs (
pkg/tbtc/signer/docs/)Misc
pkg/bitcoin/electrum: refresh the Fulcrum integration-test endpoint (incidental CI fix; the only change outsidepkg/tbtc/signer/and its workflow).Validation
cargo fmt -- --check,cargo clippy --all-targets -- -D warnings,cargo test,cargo test formal_verification_,scripts/formal/run_tla_models.sh).3e87d011…) → mint → redemption (86c978c6…) → second distributed DKG (with on-chain result submit/challenge/approve on the node side) → FROST→FROST moving funds (f3b951b0…).Relationship to companion PRs
libfrost_tbtcand fails closed on an ABI mismatch. DKG result digests are computed on both sides and checked for parity; the node aggregates the shares this signer produces. The two PRs are complementary; this one is unconsumed until feat(tbtc/node): FROST/ROAST Go node — distributed DKG, interactive signing loop, Taproot wallet lifecycle #3866 lands, so it can merge first.revealTaprootDepositP2TR deposits, full multi-mode BIP-341 key-path sighash fraud coverage (consuming the same fraud-vector file this PR ships), P2TR redeemer addresses, and SDK Taproot deposit support incl. testnet4.These three PRs comprise the FROST extraction into the canonical repos (plan v38).
Review notes
The diff is +32,305/−2 across 77 files, but it slices cleanly:
include/frost_tbtc.h(102 lines), thensrc/api.rsandsrc/lib.rs— the exported symbols, JSON envelopes, error codes, and the ABI versioning rules.engine/mod.rs,engine/lifecycle.rs,engine/state.rs,engine/codec.rs.engine/dkg.rsplus the persistence/provenance gates inengine/persistence.rsandengine/provenance.rs.engine/interactive.rs,engine/roast.rs,engine/verify_share.rs,engine/frost_ops.rs.engine/policy.rs,engine/init_config.rs,engine/config.rs,engine/telemetry.rs,engine/audit.rs,src/bin/admission_checker.rs.engine/tests.rs(~8.9k lines, organized by endpoint),tests/p2tr_signature_fraud_vectors.rs, and the vector/corpus JSON.Roughly 9k lines are engine logic, ~9.5k are tests, and the remainder is
Cargo.lock, docs, vectors, and CI. Nearly all commits landed via individually reviewed sub-PRs merged into this branch, so reviewing sub-PR-by-sub-PR is a practical alternative to reviewing the squashed diff.