fix(tbtc/signer): sign-store followup - local compaction, hash-chain hardening, review fixes - #4271
Open
piotr-roslaniec wants to merge 20 commits into
Open
piotr-roslaniec wants to merge 20 commits into
piotr-roslaniec wants to merge 20 commits into
Conversation
D1 (Split): Drop ABI minor bump (4 -> 0) and remove the 11 new FFI symbols added by this branch. The internal engine work (anchor.rs, anchor_trust.rs, inventory.rs, store.rs additions) is preserved; the new FFI exports and ABI major version are deferred to a follow-up PR that re-exposes them with proper Go-side coordination. The PR's 'ABI 4.3 coordinated merge with #4199' framing is now moot. D2 (Rename): durable -> descriptor_bound on DurableStoreIdentityResult. Matches the existing C header doc which says 'fails closed if a live path, lock, store-ID, or state entry no longer matches its held no-follow descriptor.' P0 #3 (v1->v2 re-anchor): Error message at retired_v1_state_witness_journal_error no longer references a 'documented v1->v2 witness re-anchor' that did not exist. The recovery procedure is now inline in the error. New retired_v1_state_witness_journal_recovery_steps() exposes the same procedure as a Rust function for test coverage. New pkg/tbtc/signer/docs/signer-store-v1-to-v2-migration-runbook.md is the operator runbook for v1 stores that hit this code path. P1 (validate_state_image 3x): New validate_state_image_with_digest helper accepts a precomputed digest, avoiding 2 redundant state-file reads + 2 SHA-256 hashes per persist. The first two revalidates in replace_state use the new helper; the third (after rename) still uses the original. P1/P2 (Hardening): cfg(not(unix)) guards on StateFileLock::acquire now return a clear 'requires Unix; not supported on this platform' error at construction time (distinguishable from 'store corrupted'). validate_entry_name now rejects '.' and '..' (preventing path traversal if a future caller bypasses canonicalize). unique_temp_name drops PID from the temp-name format (16-byte OsRng suffix is sufficient to prevent name collisions; PID leak no longer useful for same-uid attackers). P1/P2 (Tests): 16 new tests in tests.rs + 2 new tests in store.rs, plus 2 existing tests strengthened. Covers: orphan temp recovery, short file non-recovery, cross-mount restore, mid-journal modification, cache invalidation, O_NOFOLLOW symlink rejection for all entry paths, mode 0600 on all entries, dkg_share_epoch rollback prevention, inventory retention (rotation/eviction/multi-wallet), FFI symbol exports match ABI, 0x24*32 fingerprint vector, mid-record torn-repair, production-realistic state_witness_max_records test, production default rotation threshold test. 4 tests marked #[ignore] with explanation pending setup clarification. Test results: cargo test --lib -- --test-threads=1 -> 345 passed, 7 ignored, 0 failed. Clippy: cargo clippy --all-targets --all-features -- -D warnings -> No issues found. cargo fmt --check -> clean. Ref: agent-docs/reviews/pr-4198/findings.json (multi-agent-review of this branch) Ref: agent-docs/gap-inventory.md (D1, D2, C1 decisions)
Lists the four P0/P1 items deferred to this branch: - Per-record hash chain for witness journal (P0 #2) - Compaction implementation for witness journal (P0 #4) - Trust journal rotation/compaction (P1) - witness_history unbounded growth (P1) Each item references its multi-agent-review location and a brief implementation plan. Reference to the implementation commit on the main PR branch (b976a46) and the gap-inventory decisions (D1, D2, C1).
Every fixed-width witness record now carries a 32-byte chain_hash field that commits to all preceding records via a domain-separated SHA-256 link (chain_hash[i+1] = H(domain || chain_hash[i] || record[i+1])), making any historical tamper with the journal detectable on reload even when the state-commitment chain itself is unchanged. Records grow from 105 to 137 bytes; the segment header layout is unchanged so the frozen Go/Rust cross-language 472-byte header vector is preserved and cross-segment chains are anchored by anchoring the first record of each new segment to the previous segment's header_commitment, rather than to zeros. Bump the plain witness magic from TBTCWITNESSv2 to TBTCWITNESSv3 and add an actionable v2 retirement error so any v2-format journal left on disk by an older build is rejected with a one-time migration runbook instead of failing closed with a generic partial-record error. Old v2 journals must be renamed aside (NOT deleted) and the signer restarted; the new build regenerates at generation 1, accepting the v2->v3 break as a migration event.
Companion to the per-record hash chain implementation: the v2 record layout (105-byte records, no per-record chaining) is now retired and rejected at startup with an actionable migration error. This runbook mirrors the v1-to-v2 runbook format and gives an operator the exact shell commands to verify the magic, rename the retired journal aside, restart under the new ABI, and confirm the new v3 fingerprint. It also calls out the Go-side pin requirement so the rollout is coordinated across the threshold set, and points at the unchanged segment header layout so the cross-language byte vector stays valid.
…d advisory flock - Remove DurableStoreIdentityResult: the FFI symbol was already dropped in D1 (ABI reversal) so the 12-field result struct is unreferenced. - Add redacted_internal_error helper and apply it to the lock-file and state-directory error paths in acquire_with_mode, so production profiles do not leak absolute on-disk paths through the FFI error channel. - Add advisory_exclusive_lock (best-effort flock) on the durable store ID and the state witness journal, complementing the existing state lock with a defense-in-depth guard against a second process that bypasses the lock file. - Cover the redaction helper with a unit test that exercises both production (redacted) and development (verbose) branches.
…through F-34)
Addresses the second review pass tracked under agent-docs/reviews/ for
this branch (findings.json).
P0:
- F-01: implement local witness-journal compaction for unanchored signers
(compact_witness_journal_local, recover_state_witness_compaction). Fixes
a permanent write-lockout once the record ceiling is reached with no
signed anchor configured. Retires the previous segment immediately after
publish, matching the existing signed-rotation convention, so
revalidate_store_entries's steady-state invariant holds.
- F-08 folded into F-01: the ceiling error message no longer points at a
checkpoint ABI with zero FFI exports.
P1:
- F-02: add hash-chain tamper/domain/v2-rejection/cross-segment tests.
- F-17: correct signer-api-contract-decision-brief.md/README.md's FFI
surface claims (round-level DKG/signing-package, not coarse session API)
and reconcile the ABI major version number.
P2:
- F-03: move error redaction to the FFI boundary (ffi_redacted_message),
scoped to Internal only -- Validation is user-facing business-rule text
and must not be redacted.
- F-07: add a records-based ceiling to the trust journal alongside the
existing byte cap.
- F-09: harden backup/quarantine directory access with openat/O_NOFOLLOW,
with relative-path (AT_FDCWD) support for the bare-state-path case.
- F-14: pin a frozen cross-language vector for the record chain-hash domain.
- F-16/F-27/F-34: rewrite FOLLOWUP.md to reflect actual landed state.
- F-21: document the frozen 472-byte segment header wire format instead of
refactoring it (cross-language contract, not safe to change).
- F-22: collapse v1/v2 retired-journal helpers into parameterized versions.
- F-23: rename rotation tests/comments that misused 'compaction'.
- F-24: narrow module-wide #![allow(dead_code)] to the specific orphaned
FFI symbols in policy/state/dkg/codec/inventory/api.
- F-05/F-06/F-18/F-26: new compaction runbook, hash-chain security-model
and filesystem-dependency notes, mark the secret-material plan superseded.
P3:
- F-11: gate the corrupted-state eprintln! behind the production profile.
- F-15: add the missing post-rename DKG-retirement crash-injection test.
- F-30: encode_state_witness_record returns a stack array, not a Vec.
- F-31: rewrite two evergreen-comment violations.
- F-32: add Status: fields to decision-brief.md and rust-rewrite-bootstrap.md.
Bugs found and fixed during verification (not in the original findings):
- Recursive stack overflow: compact_witness_journal_local's own PREPARE/
COMMIT append re-entered ensure_witness_record_capacity, re-triggering
compaction. Split append_witness_record into a checked wrapper plus an
unchecked core the compaction path uses for its own terminal records.
- prepare_witness computed the next witness before ensuring capacity, so a
mid-call compaction (which advances the tip) made the pre-computed
witness stale. prepare_witness now takes the state-image digest and
computes the witness after ensuring capacity/compaction.
- A test asserted the rotated segment's second record chained directly
from header_commitment; the chain is sequential (record 1 chains from
record 0's hash), not every record independently from the header.
- open_state_directory_nofollow required an absolute path, breaking the
bare-filename ('.'-parent) state path test; added an AT_FDCWD-based
traversal for relative paths.
cargo fmt --check, cargo clippy --all-targets -- -D warnings, cargo test,
cargo test formal_verification_, and cargo deny check advisories all pass.
|
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: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 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 |
The v1.8.0 tla2tools.jar release asset was rebuilt upstream (updated 2026-08-11 per the GitHub release API), so the pinned SHA-256 no longer matched the download and CI's TLA model checks job failed closed as designed. Verified the new hash against the official release URL (https://github.com/tlaplus/tlaplus/releases/download/v1.8.0/tla2tools.jar) independently before re-pinning it. Ran pkg/tbtc/signer/scripts/formal/run_tla_models.sh locally with the updated pin; all models (RoastAttemptStateMachine, RoastRolloutPolicy, StateKeyProviderPolicy incl. production config, TeeEnforcementModes) pass with no errors found.
Fixes 30 confirmed findings from the PR review plus one incidental correctness gap discovered while repairing test coverage: - P0: local-compaction crash-recovery reachability + unconditional rename bug in recover_state_witness_compaction, with new boundary fixture test - P0: align docs with the code's actual immediate-retirement behavior for .state-witness.previous after compaction - P1: explicit store_is_anchored gate on the zero-signature marker, trust-journal record-count ceiling, ABI major bump to 5, redacted FFI error detail now reaches stderr, symlink-hardening test coverage, and doc corrections across three operator runbooks - P2/P3: dead-code cleanup, sentinel comparison fix, permission self-heal restore, errno/symlink hardening in directory enumeration, and numerous doc/comment accuracy fixes - New: removed the incremental witness-journal verification cache entirely (witness_anchor_matches et al.) after discovering its stat-based staleness check can be defeated by same-tick timestamp collisions on back-to-back writes, allowing same-length mid-journal corruption on a live open store to go undetected; every witness-tip access now performs a full re-parse
…ontinuity across compaction boundary - Extract publish_state_witness_segment as the single shared publication routine used by both rotate_state_witness_segment_inner (anchored rotation) and compact_witness_journal_local (local compaction), eliminating the reimplementation that caused the two recovery state machines to diverge. Anchored-rotation behavior is byte-for-byte unchanged (frozen 472-byte header vector test still passes). - Thread the retiring segment's terminal per-record chain hash into the new segment's genesis seed for local compaction, via the synthetic acknowledgement's previous_event_root field, giving the unanchored+compaction path real cross-segment tamper evidence it previously lacked. Verified by a new test that forges the threaded link on disk and confirms reopen fails closed.
…n gap The removal of the incremental witness-journal cache (previous commit) made every append_witness_record call, and every call routed through revalidate_store_entries, perform a full O(journal-length) reparse. Measured: 200 sequential writes took 3.9s, the next 200 took 33.3s -- O(N) per write / O(N^2) total, unusable near the 262,144-record ceiling. Fix: - append_witness_record/append_witness_record_unchecked now do O(1) checks only (pre-append size match, post-append read-back of just the new record) instead of a full reparse. - revalidate_store_entries no longer forces a full reparse internally, restoring O(1) writes end to end (replace_state confirmed flat via isolated timing probe: ~3.3-3.6ms/call across 200/400/800 sequential writes, no growth). - The full-verify guarantee moved to explicit, first-statement calls on every public read-oriented entrypoint instead: state_witness_tip(), identity(), identity_for_load(), read_state(), read_state_for_load() (the actual production state-load path used by persistence.rs), state_witness_tip_snapshot(), state_anchor_trust_head_snapshot(), and state_anchor_bootstrap_facts_snapshot(). Two of these (identity, read_state_for_load) had no full-verify at all after the incremental cache was removed -- a live-open-store read via either would have silently returned unverified, potentially-tampered state. New tests prove both now fail closed on a live store the instant a non-tail journal record is corrupted.
…description The guard was never mode == Ordinary in code -- recover_state_witness_compaction is invoked whenever anchor_metadata.is_none(), unconditionally across every acquire mode. This drift was introduced when the doc-fix and code-fix commits ran concurrently; re-verified directly against the current acquire_with_mode call site before correcting.
…full-verify reads identity()/identity_for_load() were given the same full-witness-reparse guarantee as state_witness_tip()/read_state_for_load() in the prior commit. Measured at the real 262,144-record production ceiling: a single identity() call cost ~4.3s. identity() is called by ensure_state_file_lock() -- 'the front door for every stateful signer operation' -- before every read AND write, so every operation near the ceiling paid this tax, not just repeated read calls. DurableStoreIdentity (store_id + structural filesystem/lock/canonical- path fingerprints) never actually depends on witness-journal record content -- its own doc comment states these fields 'do not enter the state-commitment transcript'. Reverted identity()/identity_for_load()/ read_state() to their O(1) pre-fix behavior; this loses no guarantee about the DATA they return, since that data was never journal-content- derived. state_witness_tip(), read_state_for_load(), state_witness_tip_snapshot(), state_anchor_trust_head_snapshot(), and state_anchor_bootstrap_facts_snapshot() are untouched and keep the full-reparse guarantee. Verified: identity() at the 262,144-record ceiling now 110us (was 4.3s); state_witness_tip()/read_state_for_load() at ceiling unchanged at ~4s each (by design); front-door (identity) + replace_state loop flat at ~3.3-3.6ms/call across 200/400/800 iterations (isolated, single-threaded run to rule out system-load noise). Deleted identity_catches_middle_of_journal_corruption_on_already_open_store: its premise (identity() independently detects journal corruption) no longer holds and was never part of identity()'s real data contract. read_state_for_load's equivalent regression test is unchanged and still covers the actual production state-load path.
…entrypoints state_witness_tip_snapshot(), state_anchor_trust_head_snapshot(), and state_anchor_bootstrap_facts_snapshot() all carry the full-witness-reparse guarantee but had no dedicated regression test proving it -- only state_witness_tip() and read_state_for_load() did. Add one test per entrypoint, each corrupting a non-tail journal record on a live open store and asserting the corresponding entrypoint fails closed, so a future refactor of the shared verification path cannot silently drop the guarantee for these three with a green suite. state_anchor_bootstrap_facts_snapshot's test corrects for its pristine- genesis-only precondition by corrupting the non-tail PREPARE record of the mandatory 2-record genesis pair, and uses acquire_for_bootstrap_facts (not acquire+replace_state, which would make the store permanently non-pristine). state_anchor_trust_head_snapshot's test first bootstraps a real trust transition (it errors with no trust head at all, so a plain unanchored store cannot exercise it), matching the existing bootstrap_trust_transition_succeeds_on_first_call_and_ordinary_reopen setup in store.rs.
…ath split
- FOLLOWUP.md: line 53 ('Original issue') described the pre-fix state
using the function's name AT THAT TIME (ensure_witness_record_capacity)
-- restored the historical name with a pointer to the current one,
rather than overwriting history with the post-P3-8 name.
- identity_for_load()'s doc comment claimed it still validates 'the
witness journal' -- no longer true after the front-door/data-read
split; corrected to state it validates descriptors only, and that
full witness-journal re-verification now lives exclusively in the
data-bearing load call (read_state_for_load).
- state.rs's ensure_state_file_lock() comment made the same false claim
('a lock, store-ID, directory, witness, or state replacement...
cannot be hidden') about identity() specifically -- removed 'witness'
from that list and added an explicit note on why (cost vs. the data
identity() actually returns), matching the reasoning already recorded
in store.rs.
…cksum - clippy (stable toolchain, newer than what was locally installed) flags chunks_exact() with a constant chunk size under -D warnings; switch to as_chunks::<TBTC_SIGNER_STATE_WITNESS_RECORD_LENGTH>() in the witness-journal parser and adjust apply_state_witness_record's call site (as_chunks yields fixed-size arrays, not byte slices). - tla2tools.jar checksum mismatch in CI: the upstream v1.8.0 release asset was replaced by the tlaplus project between 2026-09-08 (when the previous pin was verified) and 2026-09-09, changing its bytes under the same version tag. Re-verified via the same procedure (download from the release URL, confirm SHA-1 against the release notes, pin the resulting SHA-256) and documented the re-upload so a future re-pin doesn't assume 'same tag' implies 'same bytes'.
…I test
provisioning_config_ffi_is_startup_only_and_capability_minimal spawns
a subprocess via Command::env(...) without holding the test-isolation
mutex (lock_test_state()) that every other TBTC_SIGNER_*-touching test
acquires. Root-caused via signer_env_var() branch instrumentation:
persisted_engine_state_rejects_session_registry_over_limit's own
env::set_var(MAX_SESSIONS, "2") -> env::var() round trip, taken
entirely within its own held guard, was observed flipping from
Some("2") back to None microseconds later with no other guarded test
able to have caused it - only this unguarded subprocess spawn was
running concurrently. Reproduced the CI failure locally (~30-40% flake
rate under --test-threads=4), fixed by acquiring the same guard, then
confirmed 25/25 clean release-mode runs versus the prior flake rate.
…IONS env production_default_state_witness_max_records_is_sane called clear_state_storage_policy_overrides() (which removes TBTC_SIGNER_MAX_SESSIONS among other vars) without holding lock_test_state(). An exhaustive scan for this class of bug (any #[test] fn calling clear_state_storage_policy_overrides, establish_clean_signer_test_env, reset_for_tests, or a direct TBTC_SIGNER_* env::set_var/remove_var without lock_test_state() somewhere in its body, across tests.rs/lib.rs/ffi.rs/store.rs/ anchor.rs/init_config.rs) found this as the only remaining instance after the previous commit's fix. Corrected that commit's comment, which overclaimed exclusivity before this second instance was found. Verified 8/8 clean debug-mode full-suite runs (CI's actual profile, not just release) after both fixes, ~155-166s each, 364 passed + 3 ignored consistently.
piotr-roslaniec
changed the base branch from
dev
to
codex/signer-store-identity-abi
September 9, 2026 11:10
All four originally-deferred items it tracked are implemented and verified against current source: per-record hash chain (P0#2), local compaction for unanchored signers (P0#4), trust journal records ceiling (P1, partial), and witness_history bound (already mitigated via rotation/compaction). One named test reference was stale (renamed since the doc was written) but the covering test exists under its current name. Removed the two dangling references that would otherwise point at a deleted file: the compaction runbook's cross-reference and a tests.rs comment.
…ow-annotating Empirical strip test (all 40 #[allow(dead_code)] in the crate removed, CI's exact 'cargo clippy --all-targets -- -D warnings' re-run) showed 80+31 compile errors, confirming the remaining 34 net-added annotations in this PR are load-bearing: reachability cascades from a small set of FFI-pending entry points (state_witness_tip, persist_distributed_dkg_key_package, etc, all called only from test code pending a future PR wiring frost_tbtc_*anchor*/*checkpoint*/*trust* symbols) down through everything they call, including the two EngineError variants they construct. Three items were independent of that chain and had zero live or test-only callers anywhere: - EnvVarGuard::unset() in lib.rs - dead test helper, deleted. - CoordinatorSeedVectorFile::description and CoordinatorShuffleCorpusFile::description - deserialized from JSON test fixtures but never read; neither struct has #[serde(deny_unknown_fields)], so removing the field does not break deserialization of the existing corpus JSON (the key is silently ignored, same as any other unrecognized field). Verified: fmt/clippy clean, 365/365 tests pass (matches baseline).
…d of allow-annotating" This reverts commit f1b02a5.
This branch has not been deployed
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.
Summary
Follow-up to #4198 (
codex/signer-store-identity-abi). Implements the four items originally deferred from that review's findings, plus a second multi-agent-review pass over the resulting branch, with all confirmed findings fixed.P0 (was a permanent write-lockout): unanchored signers now perform local witness-journal compaction when the record ceiling is reached, instead of failing closed forever with no recovery path.
P0 #2 (per-record hash chain): already landed earlier in this branch; this PR adds the missing tamper/domain/cross-segment/frozen-vector test coverage for it.
P1 trust journal / P1 witness_history: trust journal gets a records-based ceiling alongside its byte cap; witness_history growth was already mitigated by the pre-existing rotation path (documented, not a separate code change).
Plus: FFI-boundary error redaction moved to a single chokepoint (scoped to
Internalonly --Validationis user-facing text, not path-bearing); backup/quarantine directory access hardened against symlink swaps; segment-header/retired-journal-helper simplifications; narrowed#[allow(dead_code)]scoping; corrected FFI-surface documentation; new compaction runbook.Full finding-by-finding detail is in the commit message.
Verification
cargo fmt --manifest-path pkg/tbtc/signer/Cargo.toml -- --checkcargo clippy --locked --manifest-path pkg/tbtc/signer/Cargo.toml --all-targets -- -D warningscargo test --locked --manifest-path pkg/tbtc/signer/Cargo.toml(386 passed, 7 ignored)cargo test --locked --manifest-path pkg/tbtc/signer/Cargo.toml formal_verification_cargo deny check advisoriesAll pass locally; CI runs the same gates plus TLA model checks.