refactor(tbtc/signer): split 18k-line engine.rs into focused engine/ submodules#4036
Merged
mswilkison merged 2 commits intoJun 12, 2026
Conversation
…submodules Post-merge follow-up #2 from the June 2026 review stack (#4028-#4035): engine.rs absorbed four merges plus the round-nonce-v3 fix and every new PR was contending for the same 18,248-line file. Pure code move - no behavior change: - production code -> 16 thematic submodules under src/engine/ (state, persistence, config, policy, provenance, telemetry, lifecycle, audit, codec, frost_ops, nonce, roast, dkg, signing, transaction, testsupport); formerly-private items widened to pub(crate), and `mod engine` itself stays private in lib.rs, so the crate-external surface is identical - `mod tests` moved verbatim to engine/tests.rs: the module path engine::tests::* is unchanged, so run_phase5_chaos_suite.sh --exact filters and phase-doc test references stay valid - only semantic edit: the coordinator-seed-vectors include_str! path gains one ../ (the file now sits one directory deeper) Verified: cargo fmt --check; clippy --all-targets -D warnings; full suite 223 passed + 1 ignored / 24 / 1 - counts identical to the pre-split HEAD; formal_verification_ filter passes; all five chaos-suite --exact paths pass; testdata untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
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:
✨ 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 |
- point roast-coordinator-seed-derivation.md and formal/models/README.md at the functions' new submodule homes (roast.rs, signing.rs, persistence.rs); the formal README lines also dropped their stale monorepo tools/tbtc-signer/ path prefix - drop the per-file "Split from the former single-file engine.rs" provenance comments (mod.rs and git history record the split); keep the one-line module descriptions - tests.rs header now states the constraint instead of provenance: the file stays a single module because the chaos suite pins engine::tests::<name> paths with cargo test -- --exact Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
mswilkison
merged commit 97e3bb5
into
extraction/frost-signer-mirror-2026-05-26
18 checks passed
mswilkison
added a commit
that referenced
this pull request
Jun 12, 2026
…ig (#4037) Post-merge follow-up **#3** from the June 2026 review stack (#4028–#4035; #4036 landed the engine split that stages this change): move the `TBTC_SIGNER_*` env-var surface into an init-time FFI config struct, shrinking the ops/audit surface from ~40 scattered `std::env::var` reads to one explicit, validated installation at startup. ## What this adds New FFI entry `frost_tbtc_init_signer_config(request_ptr, request_len)` taking a typed JSON `InitSignerConfigRequest` (40 optional fields; field name = lowercased `TBTC_SIGNER_*` suffix). The host installs it once at startup. ## Semantics - **Wholesale source of truth.** Once installed, the environment is *not consulted* for any covered knob; an unset field means the built-in default. No per-knob mixing of config and env — split-brain configs can't exist. - **Fail-closed init.** `deny_unknown_fields` rejects typo'd knobs; enforcement-gated policy combinations (admission, signing-policy firewall, auto-quarantine) are validated at install by running the same loaders the runtime gates use, with rollback on rejection — a misconfigured signer fails at startup, not at first signing. - **Idempotent re-init** for an identical request (fingerprint match); conflicting re-init rejected. - **Secrets never ride the config FFI.** `TBTC_SIGNER_STATE_ENCRYPTION_KEY_HEX` is read exclusively from the dedicated env/command key-provider channel even when a config is installed (the one deliberate `std::env::var` left outside the chokepoint, commented at the read). - **Transitional compatibility.** With no config installed, `engine::signer_env_var` falls through to the process environment — existing hosts and the entire pre-existing test suite run unchanged; non-development profiles log a one-time warning suggesting the init FFI. ## Why parity is safe by construction The typed request converts to the same canonical strings the existing env parsers consume (`"true"`/`"false"`, decimal ints, comma-joined identifier lists), and every existing clamp/warn/reject path runs unchanged on identical inputs. The diff swaps `std::env::var(X)` → `signer_env_var(X)` at 31 sites and changes nothing else about how values are interpreted. Also deletes `lib.rs`'s duplicated profile/truthy parsing in favor of the engine's single implementation. Thanks to #4036, this lands as one new ~400-line module (`engine/init_config.rs`) plus one-line touches across `config/lifecycle/persistence/policy/provenance/state` — not an 18k-line-file churn. `engine/config.rs` remains the single home of the env-name constants. ## Verification - `cargo fmt --check` ✅; `cargo clippy --all-targets -- -D warnings` ✅ - Full suite **235 passed + 1 ignored / 24 / 1** — all 224 pre-existing tests pass unchanged (env-fallback parity), plus 11 new tests: config-over-env precedence, wholesale env-ignoring for unset fields, idempotent/conflicting re-init, invalid-profile rejection, install rollback on incomplete firewall policy, complete-admission-policy validation, secret-stays-on-env-channel, production-profile-forces-strict via config, `reset_for_tests` clearing, `deny_unknown_fields`, list/bool canonicalization, and an FFI round-trip - `--features bench-restart-hook` builds; chaos suite (5/5 `--exact` paths) and `formal_verification_` filter pass - `include/frost_tbtc.h` gains the symbol; README documents the contract ## Notes for reviewers - Knobs the runtime warn-and-defaults on (e.g. out-of-range timeouts) keep that behavior under config values — init validation only rejects what the runtime gates would reject. Tightening init further is possible later without breaking the contract. - Go-host adoption is a follow-up: this is additive ABI; nothing changes for hosts until they call the new entry. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
mswilkison
added a commit
that referenced
this pull request
Jun 12, 2026
…ence (#4040) Post-merge follow-up **#4** from the June 2026 review stack (#4028–#4035): replace `json.Marshal` as the canonical signed-bytes encoding for evidence snapshots/bundles — explicitly scheduled to land **before Phase 7 wiring ossifies the format**. (Items 2 and 3 landed as #4036/#4037 on the mirror branch; this is the Go-side sibling on the scaffold branch.) ## Why now The RFC-21 Layer B evidence signatures were computed over canonical JSON. That byte stability is a Go-implementation accident — field-order-stable `encoding/json` output — not a portable contract. The moment Phase 7 wires evidence verification into the Rust signer (or any second implementation appears), every verifier would need to replicate Go's exact JSON emission. No persisted or cross-component evidence exists yet, so the format can still change for free. ## Design: sign what you transmit, verify what you received New `pkg/frost/roast/gen/pb/evidence.proto`: - A snapshot travels as `SignedLocalEvidenceSnapshot{body, operator_signature}` where `body` is the serialized `LocalEvidenceSnapshotBody` — the operator signs those exact bytes. - A transition message travels as `SignedTransitionMessage{body, coordinator_signature}` whose `TransitionMessageBody` embeds every member's signed snapshot envelope **verbatim** (`repeated bytes signed_snapshots`) — the coordinator attests to the exact signed snapshots it assembled, in order. - Producers marshal a body **exactly once**, at signing time, and cache it; parsed messages retain received body/envelope bytes verbatim; verification always runs over exact received bytes. **Nothing in the evidence chain is ever re-encoded**, so signature validity never depends on any serializer's canonical form — across protobuf library versions or across languages. This deliberately sidesteps protobuf's own caveat that deterministic serialization is not canonical across implementations. - `Marshal` of a received message returns the received envelope verbatim — evidence bytes survive re-broadcast, including wire-legal but non-canonical encodings (pinned by a handcrafted reversed-field-order test). - `CanonicalSnapshotBytes`/`CanonicalBundleBytes` → `SignableBytes()` accessors; the coordinator's first-write-wins conflict check now compares exact signed bytes. ## Tests - Existing suite migrated off JSON fixtures: test-only encode helpers bypass production signing so every structural-rejection path (zero sender, bad hash length, unsorted/duplicate entries, oversize caps, bundle ordering/hash-binding) is still exercised at the wire level. - New `wire_test.go` pins the format's core properties: byte-preservation through unmarshal→re-marshal, verbatim snapshot-envelope embedding inside bundle bodies, producer-signed bytes == receiver-verified bytes, non-canonical-encoding survival, tampered-body verification failure. - `go build ./...`, `go vet`, `gofmt` clean; frost + tbtc package tests green. Generated with protoc 33.4 / protoc-gen-go v1.36.3 (matches the go.mod protobuf runtime v1.36.3). ## Docs RFC-21 "Evidence message format" decision rewritten: signed-body protobuf envelopes, with the retirement rationale for canonical JSON recorded. ## Notes for reviewers - The in-memory model types (`LocalEvidenceSnapshot`, `TransitionMessage`) are unchanged apart from two unexported byte caches; all call sites kept their shapes. - Immutability contract: evidence fields must not be mutated after `SignableBytes()` is first computed (documented on the cache fields); the aggregation flow already treats snapshots as immutable post-receipt. - Phase 7 cross-language note: the Rust signer will verify operator/coordinator signatures over `body` bytes and parse them with any protobuf implementation — no canonicalization requirements transfer. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Post-merge follow-up #2 from the June 2026 review stack (#4028–#4035).
engine.rswas deferred-split to avoid conflicting with the open stack; with the stack merged it had grown to 18,248 lines (absorbing four merges plus the round-nonce-v3 fix), and every new PR contends for the same file. This lands the split before anything new piles onto the monolith.What this is
A pure code move — no behavior change, no API change, no test-path change.
statepersistenceconfigTBTC_SIGNER_*env surface: const names, defaults, parsers, profile detectionpolicyprovenancetelemetrylifecycleauditcodecfrost_opsdkg_part1..3, nonces, signing package, share, aggregatenonceRoundNonceBinding+ deterministic round-nonce derivation (round-nonce-v3), isolated for auditroastdkgrun_dkgflow + transitional-dealer production gatessigningstart_sign_round/finalize_sign_roundflows, bootstrap synthetic contributionstransactiontestsupportlock_test_state,reset_for_tests, …)testsmod tests, moved verbatimDesign decisions
engine::tests::*paths are preserved.mod testsmoved as a single child module (engine/tests.rs), soscripts/run_phase5_chaos_suite.sh's fivecargo test … -- --exactfilters and everyengine::tests::<name>reference in the phase docs remain valid. Splitting tests further would force rewriting those contracts — left as an explicit team decision.pub(crate); each submodule opens withuse super::*;against glob re-exports inmod.rs. Sincelib.rskeepsmod engine;private, the crate-external surface is byte-identical. Per-module visibility tightening can happen incrementally later.config.rsdeliberately concentrates the env surface — it pre-stages follow-up Split Brain: Push solidity into a subdirectory, prep for Go merge #3 (moveTBTC_SIGNER_*env vars into an init-time FFI config struct) as a mostly-one-file change.include_str!("../testdata/coordinator_seed_vectors.json")in the tests gains one../because the file now sits one directory deeper.Verification
cargo fmt --check✅,cargo clippy --all-targets -- -D warnings✅cargo test formal_verification_✅ (5/5); all five chaos-suite--exactpaths ✅testdata/untouched — seed vectors and shuffle corpus remain byte-identicalgit diff d47f00989 --color-moved=zebra --color-moved-ws=ignore-all-spacerenders nearly the entire diff as moved lines;git blame -C -Cfollows history across the split.🤖 Generated with Claude Code