From 501dffb45c44f352056f2296b4ad95e30edb135f Mon Sep 17 00:00:00 2001 From: maclane Date: Sat, 27 Jun 2026 20:48:57 -0400 Subject: [PATCH 1/3] fix(signer): defer sign-round clear + persist before idempotent serve Fixes two Codex-flagged P2 state-consistency holes in start_sign_round. 1. Persist failure left in-memory state diverged from durable state. After the fresh-round path mutated the session (consumed-replay markers, round state), a failed persist returned an error but the canonical idempotent serve then returned signature shares WITHOUT persisting -- so a restart could replay the round with no durable consumed marker. The idempotent cached serve now persists before serving when the round is not yet durable, tracked by a process-local SIGN_ROUND_PERSIST_PENDING marker; when the original persist already succeeded it still serves cached without persisting, preserving the 'idempotent replay survives a state-key-provider outage' property build_taproot_tx relies on (rollback is impossible -- the transition clear zeroizes the prior round material). 2. Active attempt cleared before later validation could fail. On an authorized ROAST attempt advance, clear_active_sign_round_for_attempt_transition ran before the fresh-path checks (participant resolution, included-set equality, quarantine, consumed-replay, share construction). A malformed advance that passed authorization but failed a later check destroyed the in-memory active round with no validated/persisted replacement, so the next StartSignRound could start a fresh attempt without transition evidence until a restart. The clear is now deferred until every fallible check has passed, just before the replacement round is installed and persisted. Adds two regression tests, both verified to fail against the pre-fix code. Design validated with Codex. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/tbtc/signer/src/engine/signing.rs | 75 +++++++++-- pkg/tbtc/signer/src/engine/tests.rs | 174 ++++++++++++++++++++++++++ 2 files changed, 241 insertions(+), 8 deletions(-) diff --git a/pkg/tbtc/signer/src/engine/signing.rs b/pkg/tbtc/signer/src/engine/signing.rs index 96d0c87fd1..8435a1dee5 100644 --- a/pkg/tbtc/signer/src/engine/signing.rs +++ b/pkg/tbtc/signer/src/engine/signing.rs @@ -5,6 +5,18 @@ use super::*; pub(crate) const BOOTSTRAP_SYNTHETIC_CONTRIBUTION_DOMAIN: &str = "tbtc-signer-bootstrap-contribution-v1"; +/// Process-local marker: set when `start_sign_round` establishes a round in +/// memory whose durable persist has not yet succeeded, and cleared after a +/// successful `start_sign_round` persist. It lets the idempotent cached serve +/// persist ONLY when the round is not yet durable -- closing the +/// lost-consumed-marker hole on the original-persist-failure path -- while still +/// serving the cached round WITHOUT persisting (so an idempotent replay survives +/// a state-key-provider outage, matching `build_taproot_tx`) when the original +/// persist already succeeded. Read/written only while the ENGINE_STATE guard is +/// held, so `Relaxed` ordering suffices. +static SIGN_ROUND_PERSIST_PENDING: std::sync::atomic::AtomicBool = + std::sync::atomic::AtomicBool::new(false); + pub fn start_sign_round(mut request: StartSignRoundRequest) -> Result { record_hardening_telemetry(|telemetry| { telemetry.start_sign_round_calls_total = @@ -122,6 +134,10 @@ pub fn start_sign_round(mut request: StartSignRoundRequest) -> Result Result Result Result Result Result Result Date: Sun, 28 Jun 2026 10:05:53 -0400 Subject: [PATCH 2/3] fix(signer): clear sign-round persist marker on any successful persist Codex re-review: the SIGN_ROUND_PERSIST_PENDING marker was process-global but cleared only at start_sign_round's own persist sites. If a start_sign_round persist failed (marker set) and a later UNRELATED successful persist (e.g. a DKG for another session) then wrote the whole engine state -- making that round durable -- the marker stayed stale-true. A subsequent idempotent replay of the now-durable round during a state-key-provider outage would re-enter the persist branch, try to persist again, and fail instead of serving the cached round. Move the marker into the persistence module and clear it inside persist_engine_state_to_storage_with_key on any successful write, so any operation's successful persist clears it. start_sign_round sets it on a fresh-round mutation (mark_sign_round_persist_pending) and reads it in the idempotent serve (sign_round_persist_pending); the explicit clears at the start_sign_round persist sites are removed (the persist clears it now). Adds a regression test (verified to fail against the pre-fix code) covering the unrelated-persist-then-outage replay. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/tbtc/signer/src/engine/persistence.rs | 35 ++++++++ pkg/tbtc/signer/src/engine/signing.rs | 29 +++---- pkg/tbtc/signer/src/engine/tests.rs | 98 +++++++++++++++++++++++ 3 files changed, 144 insertions(+), 18 deletions(-) diff --git a/pkg/tbtc/signer/src/engine/persistence.rs b/pkg/tbtc/signer/src/engine/persistence.rs index afa1568021..a9129db119 100644 --- a/pkg/tbtc/signer/src/engine/persistence.rs +++ b/pkg/tbtc/signer/src/engine/persistence.rs @@ -1043,6 +1043,31 @@ pub(crate) fn persist_engine_state_to_storage( persist_engine_state_to_storage_with_key(engine_state, &key_material) } +/// Process-local marker tracking whether `start_sign_round` has established a +/// round in memory whose durable persist has not yet succeeded. It lets the +/// idempotent cached serve persist ONLY when the round is not yet durable (so a +/// lost-consumed-marker hole on the original-persist-failure path is closed), +/// while still serving the cached round WITHOUT persisting -- so an idempotent +/// replay survives a state-key-provider outage -- once the round is durable. +/// SET by `start_sign_round` on a fresh-round mutation and CLEARED by ANY +/// successful persist (see `persist_engine_state_to_storage_with_key`): a +/// successful persist writes the whole engine state, so the round becomes durable +/// regardless of which operation triggered it. Accessed only while the +/// ENGINE_STATE guard is held, so `Relaxed` ordering suffices. +static SIGN_ROUND_PERSIST_PENDING: std::sync::atomic::AtomicBool = + std::sync::atomic::AtomicBool::new(false); + +/// Marks that `start_sign_round` established an in-memory round not yet durably +/// persisted. Cleared by the next successful persist of any kind. +pub(crate) fn mark_sign_round_persist_pending() { + SIGN_ROUND_PERSIST_PENDING.store(true, std::sync::atomic::Ordering::Relaxed); +} + +/// Returns true while a `start_sign_round` round is in memory but not yet durable. +pub(crate) fn sign_round_persist_pending() -> bool { + SIGN_ROUND_PERSIST_PENDING.load(std::sync::atomic::Ordering::Relaxed) +} + pub(crate) fn persist_engine_state_to_storage_with_key( engine_state: &EngineState, key_material: &StateEncryptionKeyMaterial, @@ -1122,6 +1147,16 @@ pub(crate) fn persist_engine_state_to_storage_with_key( } bytes.zeroize(); + if persist_result.is_ok() { + // A successful persist writes the entire engine state durably -- including + // any in-memory start_sign_round round -- so the sign-round persist-pending + // marker no longer applies, regardless of which operation triggered this + // persist. Clearing it here (rather than only at the start_sign_round + // persist sites) keeps an idempotent replay of an already-durable round + // from being forced to re-persist, which would otherwise fail during a + // state-key-provider outage. + SIGN_ROUND_PERSIST_PENDING.store(false, std::sync::atomic::Ordering::Relaxed); + } persist_result } diff --git a/pkg/tbtc/signer/src/engine/signing.rs b/pkg/tbtc/signer/src/engine/signing.rs index 8435a1dee5..c5e9470158 100644 --- a/pkg/tbtc/signer/src/engine/signing.rs +++ b/pkg/tbtc/signer/src/engine/signing.rs @@ -5,17 +5,11 @@ use super::*; pub(crate) const BOOTSTRAP_SYNTHETIC_CONTRIBUTION_DOMAIN: &str = "tbtc-signer-bootstrap-contribution-v1"; -/// Process-local marker: set when `start_sign_round` establishes a round in -/// memory whose durable persist has not yet succeeded, and cleared after a -/// successful `start_sign_round` persist. It lets the idempotent cached serve -/// persist ONLY when the round is not yet durable -- closing the -/// lost-consumed-marker hole on the original-persist-failure path -- while still -/// serving the cached round WITHOUT persisting (so an idempotent replay survives -/// a state-key-provider outage, matching `build_taproot_tx`) when the original -/// persist already succeeded. Read/written only while the ENGINE_STATE guard is -/// held, so `Relaxed` ordering suffices. -static SIGN_ROUND_PERSIST_PENDING: std::sync::atomic::AtomicBool = - std::sync::atomic::AtomicBool::new(false); +// The sign-round persist-pending marker lives in the persistence module +// (`mark_sign_round_persist_pending` / `sign_round_persist_pending`) so that ANY +// successful persist clears it, not only `start_sign_round`'s own -- otherwise a +// later unrelated persist that makes the round durable would leave the marker +// stale and force an idempotent replay to re-persist during a state-key outage. pub fn start_sign_round(mut request: StartSignRoundRequest) -> Result { record_hardening_telemetry(|telemetry| { @@ -270,11 +264,10 @@ pub fn start_sign_round(mut request: StartSignRoundRequest) -> Result Result Result Date: Sun, 28 Jun 2026 16:27:38 -0400 Subject: [PATCH 3/3] fix(signer): scope sign-round persist-pending marker per session Codex re-review: the process-global SIGN_ROUND_PERSIST_PENDING bit conflated all sessions. After one session's StartSignRound established a round and failed to persist, the bit stayed set process-wide, so the idempotent cached-serve branch -- which consulted it for EVERY session -- would force an UNRELATED, already- durable session's replay to re-persist and fail during the same state-key outage instead of serving its durable cached shares. An availability regression. Replace the global AtomicBool with a per-session set (SIGN_ROUND_PERSIST_PENDING_SESSIONS: OnceLock>>) keyed by session_id: mark the specific session on a fresh-round mutation, consult that session in the cached-serve branch, and clear the WHOLE set on any successful persist (a persist writes the entire engine state, so every in-memory round becomes durable at once -- preserving the cross-operation durability the prior commit established). reset_for_tests clears the set explicitly. Both invariants stay intact: a non-durable round's replay still re-persists before serving (durability); a durable round's replay still serves without persisting (availability); a different session's failed persist no longer drags down an unrelated durable session. Adds a regression test (verified to fail against the global bool) and corrects the marker doc comment (mutual exclusion is the inner mutex, not the ENGINE_STATE guard, since clear also runs off-guard at startup and in test reset). Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/tbtc/signer/src/engine/persistence.rs | 88 +++++++++++++------- pkg/tbtc/signer/src/engine/signing.rs | 32 +++++--- pkg/tbtc/signer/src/engine/tests.rs | 98 ++++++++++++++++++++++- pkg/tbtc/signer/src/engine/testsupport.rs | 1 + 4 files changed, 177 insertions(+), 42 deletions(-) diff --git a/pkg/tbtc/signer/src/engine/persistence.rs b/pkg/tbtc/signer/src/engine/persistence.rs index a9129db119..badca73bc7 100644 --- a/pkg/tbtc/signer/src/engine/persistence.rs +++ b/pkg/tbtc/signer/src/engine/persistence.rs @@ -1043,29 +1043,61 @@ pub(crate) fn persist_engine_state_to_storage( persist_engine_state_to_storage_with_key(engine_state, &key_material) } -/// Process-local marker tracking whether `start_sign_round` has established a -/// round in memory whose durable persist has not yet succeeded. It lets the -/// idempotent cached serve persist ONLY when the round is not yet durable (so a -/// lost-consumed-marker hole on the original-persist-failure path is closed), -/// while still serving the cached round WITHOUT persisting -- so an idempotent -/// replay survives a state-key-provider outage -- once the round is durable. -/// SET by `start_sign_round` on a fresh-round mutation and CLEARED by ANY -/// successful persist (see `persist_engine_state_to_storage_with_key`): a -/// successful persist writes the whole engine state, so the round becomes durable -/// regardless of which operation triggered it. Accessed only while the -/// ENGINE_STATE guard is held, so `Relaxed` ordering suffices. -static SIGN_ROUND_PERSIST_PENDING: std::sync::atomic::AtomicBool = - std::sync::atomic::AtomicBool::new(false); - -/// Marks that `start_sign_round` established an in-memory round not yet durably -/// persisted. Cleared by the next successful persist of any kind. -pub(crate) fn mark_sign_round_persist_pending() { - SIGN_ROUND_PERSIST_PENDING.store(true, std::sync::atomic::Ordering::Relaxed); +/// Process-local set of session IDs whose `start_sign_round` round is established +/// in memory but whose durable persist has not yet succeeded. It lets the +/// idempotent cached serve persist ONLY for a session whose round is not yet +/// durable (closing the lost-consumed-marker hole on the original-persist-failure +/// path), while still serving an already-durable session's cached round WITHOUT +/// persisting -- so an idempotent replay survives a state-key-provider outage. +/// +/// The marker is scoped PER SESSION, not process-wide: one session's failed +/// persist must NOT force an unrelated, already-durable session's idempotent +/// replay to re-persist (and fail) during the same outage. A session is INSERTED +/// by `start_sign_round` on a fresh-round mutation and the whole set is CLEARED +/// by ANY successful persist (see `persist_engine_state_to_storage_with_key`): a +/// successful persist writes the entire engine state, so every in-memory round +/// becomes durable at once, regardless of which operation triggered it. +/// +/// Mutual exclusion is provided by the inner mutex itself, NOT by the ENGINE_STATE +/// guard. `mark`/`pending` and the common clear-on-persist do run under that +/// guard, but the clear ALSO runs off-guard -- on the startup legacy-envelope +/// rewrite (`load_engine_state_from_storage` persists before serving begins) and +/// in `reset_for_tests`. Those off-guard accesses are single-threaded, so the +/// mutex is effectively uncontended, but it must not be downgraded to a +/// non-locking primitive on the assumption that ENGINE_STATE serializes access. +static SIGN_ROUND_PERSIST_PENDING_SESSIONS: OnceLock>> = OnceLock::new(); + +fn sign_round_persist_pending_sessions() -> &'static Mutex> { + SIGN_ROUND_PERSIST_PENDING_SESSIONS.get_or_init(|| Mutex::new(BTreeSet::new())) } -/// Returns true while a `start_sign_round` round is in memory but not yet durable. -pub(crate) fn sign_round_persist_pending() -> bool { - SIGN_ROUND_PERSIST_PENDING.load(std::sync::atomic::Ordering::Relaxed) +/// Marks that `start_sign_round` established an in-memory round for `session_id` +/// that is not yet durably persisted. Cleared by the next successful persist of +/// any kind. +pub(crate) fn mark_sign_round_persist_pending(session_id: &str) { + sign_round_persist_pending_sessions() + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .insert(session_id.to_string()); +} + +/// Returns true while `session_id`'s `start_sign_round` round is in memory but +/// not yet durable. +pub(crate) fn sign_round_persist_pending(session_id: &str) -> bool { + sign_round_persist_pending_sessions() + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .contains(session_id) +} + +/// Clears every pending sign-round marker. Called on any successful persist (the +/// whole engine state -- and thus every in-memory round -- is now durable) and on +/// test reset. +pub(crate) fn clear_sign_round_persist_pending() { + sign_round_persist_pending_sessions() + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .clear(); } pub(crate) fn persist_engine_state_to_storage_with_key( @@ -1149,13 +1181,13 @@ pub(crate) fn persist_engine_state_to_storage_with_key( bytes.zeroize(); if persist_result.is_ok() { // A successful persist writes the entire engine state durably -- including - // any in-memory start_sign_round round -- so the sign-round persist-pending - // marker no longer applies, regardless of which operation triggered this - // persist. Clearing it here (rather than only at the start_sign_round - // persist sites) keeps an idempotent replay of an already-durable round - // from being forced to re-persist, which would otherwise fail during a - // state-key-provider outage. - SIGN_ROUND_PERSIST_PENDING.store(false, std::sync::atomic::Ordering::Relaxed); + // every in-memory start_sign_round round -- so no session's persist-pending + // marker still applies, regardless of which operation triggered this + // persist. Clearing the whole set here (rather than only at the + // start_sign_round persist sites) keeps an idempotent replay of an + // already-durable round from being forced to re-persist, which would + // otherwise fail during a state-key-provider outage. + clear_sign_round_persist_pending(); } persist_result } diff --git a/pkg/tbtc/signer/src/engine/signing.rs b/pkg/tbtc/signer/src/engine/signing.rs index c5e9470158..e07b4b220f 100644 --- a/pkg/tbtc/signer/src/engine/signing.rs +++ b/pkg/tbtc/signer/src/engine/signing.rs @@ -5,11 +5,14 @@ use super::*; pub(crate) const BOOTSTRAP_SYNTHETIC_CONTRIBUTION_DOMAIN: &str = "tbtc-signer-bootstrap-contribution-v1"; -// The sign-round persist-pending marker lives in the persistence module -// (`mark_sign_round_persist_pending` / `sign_round_persist_pending`) so that ANY -// successful persist clears it, not only `start_sign_round`'s own -- otherwise a -// later unrelated persist that makes the round durable would leave the marker -// stale and force an idempotent replay to re-persist during a state-key outage. +// The sign-round persist-pending markers live in the persistence module +// (`mark_sign_round_persist_pending` / `sign_round_persist_pending`), keyed PER +// SESSION. ANY successful persist clears them all, not only `start_sign_round`'s +// own -- otherwise a later unrelated persist that makes the round durable would +// leave the marker stale and force an idempotent replay to re-persist during a +// state-key outage. They are keyed per session so one session's failed persist +// cannot force an unrelated, already-durable session's replay to re-persist (and +// fail) during the same outage. pub fn start_sign_round(mut request: StartSignRoundRequest) -> Result { record_hardening_telemetry(|telemetry| { @@ -264,9 +267,11 @@ pub fn start_sign_round(mut request: StartSignRoundRequest) -> Result Result