fix(tbtc/signer): resolve the state-encryption key under the ENGINE_STATE guard, in write order

[mswilkison]

Summary

Resolves the at-rest state-encryption key under the held ENGINE_STATE guard, at the persist site, so key selection is ordered with the writes the guard already serializes.

This PR began as a perf change (resolve the key off the lock to keep the command-provider KMS subprocess from stalling ENGINE_STATE). A Codex review found that approach introduced a P1 state-loss bug, so the PR now lands the correct ordering instead.

The bug the off-lock approach introduced

ENGINE_STATE already serializes the persisted-state writes, but resolving the key before the lock left key selection out-of-order with the writes: a caller that resolved an older key (before a provider key-rotation) could win the last write after a concurrent caller already persisted the rotated key, tagging the single state file with the stale key-id. decode_encrypted_state_envelope accepts only the currently-resolved key-id, so a restart then rejects the file as a key-id mismatch and the persisted state is unreadable.

Fix

Resolve the key under the held guard, at the persist site — the last writer always encrypts with the then-current key. Non-marker sites call persist_engine_state_to_storage(&guard) (resolves under the guard, then persists). The two interactive marker sites resolve via state_encryption_key_material()? before inserting the durable consumed/aggregated marker (preserving fail-closed-before-marker + marker-rollback-on-persist-failure), then persist_..._with_key. The deferred require_resolved_state_key helper is removed.

Invariants preserved: idempotent replays / pre-persist rejections still return before any key resolution, so a transient key-provider outage cannot fail a non-persisting call (idempotent_build_tx_replay_survives_state_key_outage still holds).

Verification

Clean cargo build (no warnings); all 329 crate tests pass. An independent adversarial review confirmed all four ordering invariants at every converted site (0 correctness findings).

Tradeoff (tracked in #4121)

This reverses the off-lock optimization: under the command/KMS provider the subprocess now runs under the guard during actual persists (a bounded head-of-line stall; env provider is unaffected). Because the production profile mandates the command provider, this is the production path. Recovering off-lock safely needs added structure (a cheap provider key-id signal, a coordinated rotation protocol, or a partial-recovery persist-lock) — analyzed and tracked in #4121, gated on measuring whether the stall actually bottlenecks throughput.

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: a137e131-7de9-4ac3-8003-5c582484ec78

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/signer-statekey-offlock

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[mswilkison]

Addressed the P1 (stale-key write-ordering race) in 19a3d8d.

Root cause confirmed: ENGINE_STATE already serializes the persisted-state writes, but the key was resolved before the lock — so key selection was out-of-order with the writes. A caller that resolved an older key (before a provider rotation) could win the last write after a concurrent caller already persisted the rotated key, tagging the single state file with the stale key-id; decode_encrypted_state_envelope accepts only the current key-id, so restart rejects it.

Fix: resolve the key under the held ENGINE_STATE guard, at the persist site, so key selection is in the same serialized order as the writes (the last writer always encrypts with the then-current key). Non-marker sites now call persist_engine_state_to_storage(&guard); the two interactive marker sites resolve via state_encryption_key_material()? before the marker insert (fail-closed-before-marker + marker-rollback intact). The deferred require_resolved_state_key helper is removed.

Invariants preserved: idempotent replays / pre-persist rejections still return before any key resolution (a key-provider outage can't fail a non-persisting call — the idempotent_build_tx_replay_survives_state_key_outage regression still holds). All 329 crate tests pass.

Tradeoff (explicitly flagged): this reverses the off-lock optimization this PR introduced — under the command provider the KMS subprocess now runs under the guard during actual persists (a bounded head-of-line stall; a hung command is bounded by terminate_state_key_command, and the env provider has no subprocess so is unaffected). Resolving under the guard is required for correctness because there's no cheap "is my key still current" signal and decode can't be made key-tolerant without storing old keys. Recovering off-lock behavior safely would need a cheap rotation signal from the key provider — left as a follow-up.

Reviewed via an independent adversarial pass: all four ordering invariants confirmed across every converted site; the only finding was this perf tradeoff (above).