From b74f209fb6d81ea7b70f08ed3ed136c811434fa2 Mon Sep 17 00:00:00 2001 From: maclane Date: Sat, 13 Jun 2026 17:12:13 -0400 Subject: [PATCH 1/6] feat(tbtc/signer): Phase 7.2b-1 InteractiveAggregate completion marker Adds a durable per-attempt "aggregated" marker so re-aggregating a completed interactive attempt is rejected rather than recomputed (frozen Phase 7 spec; Phase 7.2b design section 6). Engine-side only: no blame and no envelopes, preserving the crypto-only engine boundary (the Q1 correction) - all envelope verification and authoritative blame stay Go-side. - New EngineError::InteractiveAttemptAlreadyAggregated (code interactive_attempt_already_aggregated, recovery_class recoverable). - aggregated_interactive_attempt_markers: HashSet on SessionState, mirrored as Vec on PersistedSessionState with serde(default) for backward-compat with pre-7.2b state, bounded via the existing consumed-registry helpers and wired through both persistence conversions. - interactive_aggregate pre-checks the marker before recomputing, keeps the aggregation lock-free, then re-acquires the lock, re-checks (a concurrent-duplicate race guard), inserts the marker, and persists with rollback-on-failure - mirroring the Round2 consume-before-release pattern - before reporting success. The design's section 9 durable-wallet-pubkey-package-retention question (needed by the 7.2b-3 verify-share FFI) is confirmed already satisfied: the DKG public key package persists on the session and survives the interactive-attempt TTL sweep, so no new persistence is added here. Tests: repeat-aggregate rejected; completion marker survives restart+reload; error code/recovery/message pinned. Co-Authored-By: Claude Opus 4.8 --- pkg/tbtc/signer/src/engine/interactive.rs | 60 ++++++++ pkg/tbtc/signer/src/engine/persistence.rs | 33 +++++ pkg/tbtc/signer/src/engine/state.rs | 7 + pkg/tbtc/signer/src/engine/tests.rs | 168 ++++++++++++++++++++++ pkg/tbtc/signer/src/errors.rs | 32 +++++ 5 files changed, 300 insertions(+) diff --git a/pkg/tbtc/signer/src/engine/interactive.rs b/pkg/tbtc/signer/src/engine/interactive.rs index 211dda0404..d1980855b5 100644 --- a/pkg/tbtc/signer/src/engine/interactive.rs +++ b/pkg/tbtc/signer/src/engine/interactive.rs @@ -612,6 +612,18 @@ pub fn interactive_aggregate( session_id: request.session_id.clone(), } })?; + // Reject a repeat aggregate of an already-completed attempt before + // recomputing (Phase 7.2b design section 6). The marker is durable, + // so a completed attempt stays completed across restart. + if session + .aggregated_interactive_attempt_markers + .contains(&attempt_id) + { + return Err(EngineError::InteractiveAttemptAlreadyAggregated { + session_id: request.session_id.clone(), + attempt_id, + }); + } if session.dkg_result.is_none() { return Err(EngineError::DkgNotReady { session_id: request.session_id.clone(), @@ -676,6 +688,54 @@ pub fn interactive_aggregate( .serialize() .map_err(|e| EngineError::Internal(format!("failed to serialize aggregate: {e}")))?; + // Record the durable completion marker before reporting success, so a + // repeat InteractiveAggregate for this attempt is rejected rather than + // recomputed (Phase 7.2b design section 6). The engine lock was dropped + // for the aggregation crypto above; re-acquire it to mark and persist. + // This is a completion marker, not a security gate (the aggregate is + // deterministic over public data): a concurrent duplicate that raced past + // the pre-check recomputed the identical signature, so re-check the marker + // here and let the loser report the attempt already complete instead of + // persisting twice. Persist before success; on persist failure roll the + // marker back and fail closed, leaving no half-recorded completion. + let mut guard = state()? + .lock() + .map_err(|_| EngineError::Internal("engine lock poisoned".to_string()))?; + let session = guard.sessions.get_mut(&request.session_id).ok_or_else(|| { + EngineError::SessionNotFound { + session_id: request.session_id.clone(), + } + })?; + if session + .aggregated_interactive_attempt_markers + .contains(&attempt_id) + { + return Err(EngineError::InteractiveAttemptAlreadyAggregated { + session_id: request.session_id.clone(), + attempt_id, + }); + } + ensure_consumed_registry_insert_capacity( + &session.aggregated_interactive_attempt_markers, + &attempt_id, + "aggregated_interactive_attempt_markers", + &request.session_id, + )?; + session + .aggregated_interactive_attempt_markers + .insert(attempt_id.clone()); + if let Err(persist_error) = persist_engine_state_to_storage(&guard) { + let session = guard + .sessions + .get_mut(&request.session_id) + .expect("session existed under the held engine lock"); + session + .aggregated_interactive_attempt_markers + .remove(&attempt_id); + return Err(persist_error); + } + drop(guard); + record_hardening_telemetry(|telemetry| { telemetry.interactive_aggregate_success_total = telemetry .interactive_aggregate_success_total diff --git a/pkg/tbtc/signer/src/engine/persistence.rs b/pkg/tbtc/signer/src/engine/persistence.rs index 9a6200345e..36eaa928d7 100644 --- a/pkg/tbtc/signer/src/engine/persistence.rs +++ b/pkg/tbtc/signer/src/engine/persistence.rs @@ -44,6 +44,11 @@ pub(crate) struct PersistedSessionState { // interactive state, including nonces, never persists). #[serde(default, skip_serializing_if = "Vec::is_empty")] pub(crate) consumed_interactive_attempt_markers: Vec, + // Phase 7.2b InteractiveAggregate completion markers (see SessionState). + // serde(default) keeps state written before 7.2b loadable: an absent + // field deserializes to an empty set. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub(crate) aggregated_interactive_attempt_markers: Vec, } #[derive(Clone, Debug, Deserialize, Serialize)] @@ -1286,6 +1291,26 @@ impl TryFrom for SessionState { consumed_interactive_attempt_markers.len(), "consumed_interactive_attempt_markers", )?; + + let mut aggregated_interactive_attempt_markers = HashSet::new(); + for attempt_marker in persisted.aggregated_interactive_attempt_markers { + if attempt_marker.is_empty() { + return Err(EngineError::Internal( + "persisted aggregated interactive attempt marker must be non-empty".to_string(), + )); + } + + if !aggregated_interactive_attempt_markers.insert(attempt_marker.clone()) { + return Err(EngineError::Internal(format!( + "duplicate persisted aggregated interactive attempt marker [{}]", + attempt_marker + ))); + } + } + ensure_consumed_registry_persisted_bound( + aggregated_interactive_attempt_markers.len(), + "aggregated_interactive_attempt_markers", + )?; if persisted.attempt_transition_records.len() > TBTC_SIGNER_MAX_ATTEMPT_TRANSITION_RECORDS_PER_SESSION { @@ -1345,6 +1370,7 @@ impl TryFrom for SessionState { // only the consumption markers survive. interactive_signing: None, consumed_interactive_attempt_markers, + aggregated_interactive_attempt_markers, }) } } @@ -1455,6 +1481,12 @@ impl TryFrom<&SessionState> for PersistedSessionState { .cloned() .collect::>(); consumed_interactive_attempt_markers.sort_unstable(); + let mut aggregated_interactive_attempt_markers = session_state + .aggregated_interactive_attempt_markers + .iter() + .cloned() + .collect::>(); + aggregated_interactive_attempt_markers.sort_unstable(); Ok(PersistedSessionState { dkg_request_fingerprint: session_state.dkg_request_fingerprint.clone(), @@ -1479,6 +1511,7 @@ impl TryFrom<&SessionState> for PersistedSessionState { refresh_history: session_state.refresh_history.clone(), emergency_rekey_event: session_state.emergency_rekey_event.clone(), consumed_interactive_attempt_markers, + aggregated_interactive_attempt_markers, }) } } diff --git a/pkg/tbtc/signer/src/engine/state.rs b/pkg/tbtc/signer/src/engine/state.rs index 43fe1dc3d0..fed4e22b3e 100644 --- a/pkg/tbtc/signer/src/engine/state.rs +++ b/pkg/tbtc/signer/src/engine/state.rs @@ -111,6 +111,13 @@ pub(crate) struct SessionState { pub(crate) emergency_rekey_event: Option, pub(crate) interactive_signing: Option, pub(crate) consumed_interactive_attempt_markers: HashSet, + // Phase 7.2b InteractiveAggregate completion markers: an attempt whose + // aggregate signature has been produced is recorded here so a repeat + // InteractiveAggregate is rejected rather than recomputed. Durable like + // the consumed markers (markers-only durability) and bounded the same + // way. Not security-load-bearing - aggregate is deterministic over public + // data - but the frozen Phase 7 spec marks the session complete. + pub(crate) aggregated_interactive_attempt_markers: HashSet, } #[derive(Default)] diff --git a/pkg/tbtc/signer/src/engine/tests.rs b/pkg/tbtc/signer/src/engine/tests.rs index 22b533ab7d..f82cf72ef0 100644 --- a/pkg/tbtc/signer/src/engine/tests.rs +++ b/pkg/tbtc/signer/src/engine/tests.rs @@ -702,6 +702,7 @@ fn persisted_session_state_fixture() -> PersistedSessionState { refresh_history: vec![], emergency_rekey_event: None, consumed_interactive_attempt_markers: vec![], + aggregated_interactive_attempt_markers: vec![], } } @@ -12840,6 +12841,173 @@ fn interactive_aggregate_produces_and_self_verifies_bip340() { .expect("interactive aggregate yields a valid BIP-340 signature"); } +#[test] +fn interactive_aggregate_rejects_repeat_aggregate_of_completed_attempt() { + let _guard = lock_test_state(); + reset_for_tests(); + + let session_id = "interactive-aggregate-repeat"; + let key_group = "interactive-test-key-group"; + let message = [0x4eu8; 32]; + let included = [1u16, 2]; + let key_packages = ensure_interactive_dkg_session(session_id, key_group); + + let opened = open_interactive_for_test(session_id, key_group, &message, &included, 1, 1, 2) + .expect("opens"); + let round1 = interactive_round1(InteractiveRound1Request { + session_id: session_id.to_string(), + attempt_id: opened.attempt_id.clone(), + member_identifier: 1, + }) + .expect("round 1"); + let member2 = generate_nonces_and_commitments(GenerateNoncesAndCommitmentsRequest { + key_package_identifier: key_packages[&2].identifier.clone(), + key_package_hex: key_packages[&2].data_hex.clone(), + }) + .expect("member 2 nonces"); + let signing_package_hex = interactive_package_for_test( + &message, + vec![ + NativeFrostCommitment { + identifier: key_packages[&1].identifier.clone(), + data_hex: round1.commitments_hex, + }, + member2.commitment.clone(), + ], + ); + let round2 = interactive_round2(InteractiveRound2Request { + session_id: session_id.to_string(), + attempt_id: opened.attempt_id.clone(), + member_identifier: 1, + signing_package_hex: signing_package_hex.clone(), + }) + .expect("round 2 share"); + let member2_share = sign_share(SignShareRequest { + signing_package_hex: signing_package_hex.clone(), + nonces_hex: member2.nonces_hex, + key_package_identifier: key_packages[&2].identifier.clone(), + key_package_hex: key_packages[&2].data_hex.clone(), + }) + .expect("member 2 share"); + + let aggregate_request = InteractiveAggregateRequest { + session_id: session_id.to_string(), + attempt_id: opened.attempt_id.clone(), + signing_package_hex, + signature_shares: vec![ + crate::api::NativeFrostSignatureShare { + identifier: key_packages[&1].identifier.clone(), + data_hex: round2.signature_share_hex, + }, + member2_share.signature_share, + ], + taproot_merkle_root_hex: None, + }; + + // First aggregate completes the attempt. + interactive_aggregate(aggregate_request.clone()).expect("first interactive aggregate"); + + // A second aggregate for the same attempt is rejected by the durable + // completion marker rather than recomputed (Phase 7.2b design section 6). + let err = interactive_aggregate(aggregate_request) + .expect_err("re-aggregating a completed attempt must be rejected"); + assert!( + matches!( + err, + EngineError::InteractiveAttemptAlreadyAggregated { ref attempt_id, .. } + if *attempt_id == opened.attempt_id + ), + "unexpected error: {err:?}" + ); + assert_eq!(err.code(), "interactive_attempt_already_aggregated"); + assert_eq!(err.recovery_class(), "recoverable"); +} + +#[test] +fn interactive_aggregate_completion_marker_survives_process_restart() { + let _guard = lock_test_state(); + let state_path = configure_test_state_path("interactive_aggregate_marker_restart"); + reset_for_tests(); + + let session_id = "interactive-aggregate-marker-restart"; + let key_group = "interactive-test-key-group"; + let message = [0x4fu8; 32]; + let included = [1u16, 2]; + let key_packages = ensure_interactive_dkg_session(session_id, key_group); + + let opened = open_interactive_for_test(session_id, key_group, &message, &included, 1, 1, 2) + .expect("opens"); + let round1 = interactive_round1(InteractiveRound1Request { + session_id: session_id.to_string(), + attempt_id: opened.attempt_id.clone(), + member_identifier: 1, + }) + .expect("round 1"); + let member2 = generate_nonces_and_commitments(GenerateNoncesAndCommitmentsRequest { + key_package_identifier: key_packages[&2].identifier.clone(), + key_package_hex: key_packages[&2].data_hex.clone(), + }) + .expect("member 2 nonces"); + let signing_package_hex = interactive_package_for_test( + &message, + vec![ + NativeFrostCommitment { + identifier: key_packages[&1].identifier.clone(), + data_hex: round1.commitments_hex, + }, + member2.commitment.clone(), + ], + ); + let round2 = interactive_round2(InteractiveRound2Request { + session_id: session_id.to_string(), + attempt_id: opened.attempt_id.clone(), + member_identifier: 1, + signing_package_hex: signing_package_hex.clone(), + }) + .expect("round 2 share"); + let member2_share = sign_share(SignShareRequest { + signing_package_hex: signing_package_hex.clone(), + nonces_hex: member2.nonces_hex, + key_package_identifier: key_packages[&2].identifier.clone(), + key_package_hex: key_packages[&2].data_hex.clone(), + }) + .expect("member 2 share"); + + let aggregate_request = InteractiveAggregateRequest { + session_id: session_id.to_string(), + attempt_id: opened.attempt_id.clone(), + signing_package_hex, + signature_shares: vec![ + crate::api::NativeFrostSignatureShare { + identifier: key_packages[&1].identifier.clone(), + data_hex: round2.signature_share_hex, + }, + member2_share.signature_share, + ], + taproot_merkle_root_hex: None, + }; + interactive_aggregate(aggregate_request.clone()).expect("first interactive aggregate"); + + // The completion marker is the only durable interactive artifact (live + // nonce state is gone after restart by construction). It must survive a + // reload so a replayed aggregate is still rejected - this also exercises + // the marker's persistence round-trip (serialize + reload validation). + simulate_process_restart_for_tests(); + reload_state_from_storage_for_tests(); + + let err = interactive_aggregate(aggregate_request) + .expect_err("a completed attempt must stay completed across restart"); + assert!( + matches!(err, EngineError::InteractiveAttemptAlreadyAggregated { .. }), + "unexpected error: {err:?}" + ); + assert_eq!(err.code(), "interactive_attempt_already_aggregated"); + + reset_for_tests(); + cleanup_test_state_artifacts(&state_path); + clear_state_storage_policy_overrides(); +} + #[test] fn interactive_aggregate_rejects_invalid_share_fail_closed() { let _guard = lock_test_state(); diff --git a/pkg/tbtc/signer/src/errors.rs b/pkg/tbtc/signer/src/errors.rs index 636d9ccf82..f3d28c6906 100644 --- a/pkg/tbtc/signer/src/errors.rs +++ b/pkg/tbtc/signer/src/errors.rs @@ -82,6 +82,17 @@ pub enum EngineError { session_id: String, attempt_id: String, }, + /// Returned when InteractiveAggregate is invoked again for an attempt that + /// already produced an aggregate signature in this session. The per-attempt + /// "aggregated" marker is durable, so a completed attempt stays completed + /// across restart; re-aggregation is rejected rather than recomputed. + /// Distinct code so callers match on + /// `interactive_attempt_already_aggregated` rather than the message. + #[error("interactive attempt [{attempt_id}] already aggregated in session [{session_id}]")] + InteractiveAttemptAlreadyAggregated { + session_id: String, + attempt_id: String, + }, #[error("internal error: {0}")] Internal(String), } @@ -104,6 +115,9 @@ impl EngineError { Self::ConsumedAttemptReplay { .. } => "consumed_attempt_replay", Self::ConsumedRoundReplay { .. } => "consumed_round_replay", Self::ConsumedNonceReplay { .. } => "consumed_nonce_replay", + Self::InteractiveAttemptAlreadyAggregated { .. } => { + "interactive_attempt_already_aggregated" + } Self::Internal(_) => "internal_error", } } @@ -128,6 +142,10 @@ impl EngineError { Self::ConsumedAttemptReplay { .. } => "recoverable", Self::ConsumedRoundReplay { .. } => "recoverable", Self::ConsumedNonceReplay { .. } => "recoverable", + // The aggregate is deterministic over public data and the attempt + // is durably marked complete; a re-aggregation request is a benign + // duplicate the caller should not retry, not an engine fault. + Self::InteractiveAttemptAlreadyAggregated { .. } => "recoverable", Self::SessionFinalized { .. } => "terminal", Self::SessionNotFound { .. } => "terminal", Self::Internal(_) => "terminal", @@ -170,6 +188,20 @@ mod tests { ); } + #[test] + fn interactive_attempt_already_aggregated_has_stable_code_and_message_format() { + let err = EngineError::InteractiveAttemptAlreadyAggregated { + session_id: "session-a".to_string(), + attempt_id: "attempt-1".to_string(), + }; + assert_eq!(err.code(), "interactive_attempt_already_aggregated"); + assert_eq!(err.recovery_class(), "recoverable"); + assert_eq!( + err.to_string(), + "interactive attempt [attempt-1] already aggregated in session [session-a]", + ); + } + #[test] fn recovery_class_maps_retryable_and_terminal_errors() { assert_eq!( From 131c642e89c64322b826be06ce48efff12a420f1 Mon Sep 17 00:00:00 2001 From: maclane Date: Sat, 13 Jun 2026 17:42:46 -0400 Subject: [PATCH 2/6] fix(tbtc/signer): make InteractiveAggregate idempotent (Codex P2) Addresses Codex's review of #4055: the completion marker made a successfully-computed aggregate unrecoverable from the engine if the host lost the FFI response after the marker persisted - a liveness/idempotency regression and a restart-divergence. Per section 6's own 'deterministic over public data, not security-load-bearing' rationale, rejecting a retry bought nothing, so the engine now re-emits instead of erroring. The per-attempt completion marker becomes a completion record: aggregated_interactive_attempt_signatures maps attempt_id -> the public aggregate signature hex (BTreeMap on SessionState + PersistedSessionState, serde(default) backward-compat, bounded as before via a shared length-based capacity helper). A repeat InteractiveAggregate returns the stored signature; a concurrent duplicate that raced past the pre-check returns the identical signature without double-storing; success telemetry counts only a freshly recorded aggregation. The persisted signature is public (it goes on-chain), so this respects the never-persist-secrets freeze. EngineError::InteractiveAttemptAlreadyAggregated is removed (re-aggregation no longer errors). Tests updated: a repeat aggregate (immediate and across restart+reload) returns the same signature. fmt + clippy --all-targets --all-features -D + full suite (273) + Phase-5 chaos suite green. Co-Authored-By: Claude Opus 4.8 --- pkg/tbtc/signer/src/engine/interactive.rs | 105 +++++++++++----------- pkg/tbtc/signer/src/engine/persistence.rs | 42 ++++----- pkg/tbtc/signer/src/engine/state.rs | 38 +++++--- pkg/tbtc/signer/src/engine/tests.rs | 53 +++++------ pkg/tbtc/signer/src/errors.rs | 32 ------- 5 files changed, 124 insertions(+), 146 deletions(-) diff --git a/pkg/tbtc/signer/src/engine/interactive.rs b/pkg/tbtc/signer/src/engine/interactive.rs index d1980855b5..37bcf4f255 100644 --- a/pkg/tbtc/signer/src/engine/interactive.rs +++ b/pkg/tbtc/signer/src/engine/interactive.rs @@ -612,16 +612,20 @@ pub fn interactive_aggregate( session_id: request.session_id.clone(), } })?; - // Reject a repeat aggregate of an already-completed attempt before - // recomputing (Phase 7.2b design section 6). The marker is durable, - // so a completed attempt stays completed across restart. - if session - .aggregated_interactive_attempt_markers - .contains(&attempt_id) + // Idempotent re-emission (Phase 7.2b design section 6): if this attempt + // already aggregated, return the stored signature without recomputing. + // The record is durable, so the signature stays recoverable from the + // engine across restart - a host that lost the FFI response need not + // spend a fresh signing attempt to reproduce a signature the engine + // already holds. + if let Some(signature_hex) = session + .aggregated_interactive_attempt_signatures + .get(&attempt_id) { - return Err(EngineError::InteractiveAttemptAlreadyAggregated { + return Ok(InteractiveAggregateResult { session_id: request.session_id.clone(), - attempt_id, + attempt_id: attempt_id.clone(), + signature_hex: signature_hex.clone(), }); } if session.dkg_result.is_none() { @@ -687,17 +691,18 @@ pub fn interactive_aggregate( let signature_bytes = signature .serialize() .map_err(|e| EngineError::Internal(format!("failed to serialize aggregate: {e}")))?; - - // Record the durable completion marker before reporting success, so a - // repeat InteractiveAggregate for this attempt is rejected rather than - // recomputed (Phase 7.2b design section 6). The engine lock was dropped - // for the aggregation crypto above; re-acquire it to mark and persist. - // This is a completion marker, not a security gate (the aggregate is - // deterministic over public data): a concurrent duplicate that raced past - // the pre-check recomputed the identical signature, so re-check the marker - // here and let the loser report the attempt already complete instead of - // persisting twice. Persist before success; on persist failure roll the - // marker back and fail closed, leaving no half-recorded completion. + let signature_hex = hex::encode(signature_bytes); + + // Record the aggregate signature for this attempt before reporting success, + // so a repeat InteractiveAggregate returns the same signature (idempotent) + // rather than recomputing (Phase 7.2b design section 6). The engine lock was + // dropped for the aggregation crypto above; re-acquire it to record and + // persist. The aggregate is deterministic over public data, so a concurrent + // duplicate that raced past the pre-check recomputed the identical + // signature: if the record already exists we return ours without storing + // twice. Persist before reporting success; on persist failure roll the + // record back and fail closed, leaving no half-recorded completion. Count a + // success only for a freshly recorded aggregation, not a re-emission. let mut guard = state()? .lock() .map_err(|_| EngineError::Internal("engine lock poisoned".to_string()))?; @@ -706,46 +711,44 @@ pub fn interactive_aggregate( session_id: request.session_id.clone(), } })?; - if session - .aggregated_interactive_attempt_markers - .contains(&attempt_id) - { - return Err(EngineError::InteractiveAttemptAlreadyAggregated { - session_id: request.session_id.clone(), - attempt_id, - }); - } - ensure_consumed_registry_insert_capacity( - &session.aggregated_interactive_attempt_markers, - &attempt_id, - "aggregated_interactive_attempt_markers", - &request.session_id, - )?; - session - .aggregated_interactive_attempt_markers - .insert(attempt_id.clone()); - if let Err(persist_error) = persist_engine_state_to_storage(&guard) { - let session = guard - .sessions - .get_mut(&request.session_id) - .expect("session existed under the held engine lock"); + let recorded_new = !session + .aggregated_interactive_attempt_signatures + .contains_key(&attempt_id); + if recorded_new { + ensure_consumed_registry_capacity_for_insert( + session.aggregated_interactive_attempt_signatures.len(), + false, + "aggregated_interactive_attempt_signatures", + &request.session_id, + )?; session - .aggregated_interactive_attempt_markers - .remove(&attempt_id); - return Err(persist_error); + .aggregated_interactive_attempt_signatures + .insert(attempt_id.clone(), signature_hex.clone()); + if let Err(persist_error) = persist_engine_state_to_storage(&guard) { + let session = guard + .sessions + .get_mut(&request.session_id) + .expect("session existed under the held engine lock"); + session + .aggregated_interactive_attempt_signatures + .remove(&attempt_id); + return Err(persist_error); + } } drop(guard); - record_hardening_telemetry(|telemetry| { - telemetry.interactive_aggregate_success_total = telemetry - .interactive_aggregate_success_total - .saturating_add(1); - }); + if recorded_new { + record_hardening_telemetry(|telemetry| { + telemetry.interactive_aggregate_success_total = telemetry + .interactive_aggregate_success_total + .saturating_add(1); + }); + } Ok(InteractiveAggregateResult { session_id: request.session_id, attempt_id, - signature_hex: hex::encode(signature_bytes), + signature_hex, }) } diff --git a/pkg/tbtc/signer/src/engine/persistence.rs b/pkg/tbtc/signer/src/engine/persistence.rs index 36eaa928d7..1db3d40f03 100644 --- a/pkg/tbtc/signer/src/engine/persistence.rs +++ b/pkg/tbtc/signer/src/engine/persistence.rs @@ -44,11 +44,11 @@ pub(crate) struct PersistedSessionState { // interactive state, including nonces, never persists). #[serde(default, skip_serializing_if = "Vec::is_empty")] pub(crate) consumed_interactive_attempt_markers: Vec, - // Phase 7.2b InteractiveAggregate completion markers (see SessionState). - // serde(default) keeps state written before 7.2b loadable: an absent - // field deserializes to an empty set. - #[serde(default, skip_serializing_if = "Vec::is_empty")] - pub(crate) aggregated_interactive_attempt_markers: Vec, + // Phase 7.2b InteractiveAggregate completion record (see SessionState): + // attempt_id -> aggregate signature hex. serde(default) keeps state written + // before 7.2b loadable: an absent field deserializes to an empty map. + #[serde(default, skip_serializing_if = "BTreeMap::is_empty")] + pub(crate) aggregated_interactive_attempt_signatures: BTreeMap, } #[derive(Clone, Debug, Deserialize, Serialize)] @@ -1292,24 +1292,23 @@ impl TryFrom for SessionState { "consumed_interactive_attempt_markers", )?; - let mut aggregated_interactive_attempt_markers = HashSet::new(); - for attempt_marker in persisted.aggregated_interactive_attempt_markers { - if attempt_marker.is_empty() { + let mut aggregated_interactive_attempt_signatures = BTreeMap::new(); + for (attempt_id, signature_hex) in persisted.aggregated_interactive_attempt_signatures { + if attempt_id.is_empty() { return Err(EngineError::Internal( - "persisted aggregated interactive attempt marker must be non-empty".to_string(), + "persisted aggregated interactive attempt id must be non-empty".to_string(), )); } - - if !aggregated_interactive_attempt_markers.insert(attempt_marker.clone()) { + if signature_hex.is_empty() { return Err(EngineError::Internal(format!( - "duplicate persisted aggregated interactive attempt marker [{}]", - attempt_marker + "persisted aggregated interactive attempt [{attempt_id}] signature must be non-empty" ))); } + aggregated_interactive_attempt_signatures.insert(attempt_id, signature_hex); } ensure_consumed_registry_persisted_bound( - aggregated_interactive_attempt_markers.len(), - "aggregated_interactive_attempt_markers", + aggregated_interactive_attempt_signatures.len(), + "aggregated_interactive_attempt_signatures", )?; if persisted.attempt_transition_records.len() > TBTC_SIGNER_MAX_ATTEMPT_TRANSITION_RECORDS_PER_SESSION @@ -1370,7 +1369,7 @@ impl TryFrom for SessionState { // only the consumption markers survive. interactive_signing: None, consumed_interactive_attempt_markers, - aggregated_interactive_attempt_markers, + aggregated_interactive_attempt_signatures, }) } } @@ -1481,12 +1480,9 @@ impl TryFrom<&SessionState> for PersistedSessionState { .cloned() .collect::>(); consumed_interactive_attempt_markers.sort_unstable(); - let mut aggregated_interactive_attempt_markers = session_state - .aggregated_interactive_attempt_markers - .iter() - .cloned() - .collect::>(); - aggregated_interactive_attempt_markers.sort_unstable(); + let aggregated_interactive_attempt_signatures = session_state + .aggregated_interactive_attempt_signatures + .clone(); Ok(PersistedSessionState { dkg_request_fingerprint: session_state.dkg_request_fingerprint.clone(), @@ -1511,7 +1507,7 @@ impl TryFrom<&SessionState> for PersistedSessionState { refresh_history: session_state.refresh_history.clone(), emergency_rekey_event: session_state.emergency_rekey_event.clone(), consumed_interactive_attempt_markers, - aggregated_interactive_attempt_markers, + aggregated_interactive_attempt_signatures, }) } } diff --git a/pkg/tbtc/signer/src/engine/state.rs b/pkg/tbtc/signer/src/engine/state.rs index fed4e22b3e..75ec48f4e2 100644 --- a/pkg/tbtc/signer/src/engine/state.rs +++ b/pkg/tbtc/signer/src/engine/state.rs @@ -111,13 +111,15 @@ pub(crate) struct SessionState { pub(crate) emergency_rekey_event: Option, pub(crate) interactive_signing: Option, pub(crate) consumed_interactive_attempt_markers: HashSet, - // Phase 7.2b InteractiveAggregate completion markers: an attempt whose - // aggregate signature has been produced is recorded here so a repeat - // InteractiveAggregate is rejected rather than recomputed. Durable like - // the consumed markers (markers-only durability) and bounded the same - // way. Not security-load-bearing - aggregate is deterministic over public - // data - but the frozen Phase 7 spec marks the session complete. - pub(crate) aggregated_interactive_attempt_markers: HashSet, + // Phase 7.2b InteractiveAggregate completion record: maps a completed + // attempt to the aggregate signature hex it produced, so a repeat + // InteractiveAggregate returns the same signature (idempotent) rather than + // recomputing. Durable like the consumed markers (markers-only durability) + // and bounded the same way. Not security-load-bearing - the aggregate is + // deterministic over public data and the signature is public - but the + // engine stays the source of truth for a completed attempt's signature, so + // a host that loses the FFI response recovers it without a new attempt. + pub(crate) aggregated_interactive_attempt_signatures: BTreeMap, } #[derive(Default)] @@ -442,12 +444,28 @@ pub(crate) fn ensure_consumed_registry_insert_capacity( registry_name: &str, session_id: &str, ) -> Result<(), EngineError> { - if !registry.contains(entry) - && registry.len() >= TBTC_SIGNER_MAX_CONSUMED_REGISTRY_ENTRIES_PER_SESSION + ensure_consumed_registry_capacity_for_insert( + registry.len(), + registry.contains(entry), + registry_name, + session_id, + ) +} + +// Length-based core shared by the set-keyed consumed registries and the +// map-keyed Phase 7.2b aggregated-signature record, so both enforce one bound. +pub(crate) fn ensure_consumed_registry_capacity_for_insert( + registry_len: usize, + entry_already_present: bool, + registry_name: &str, + session_id: &str, +) -> Result<(), EngineError> { + if !entry_already_present + && registry_len >= TBTC_SIGNER_MAX_CONSUMED_REGISTRY_ENTRIES_PER_SESSION { return Err(EngineError::Internal(format!( "{registry_name} registry size [{}] reached max [{}] for session [{}]; use a new session_id", - registry.len(), + registry_len, TBTC_SIGNER_MAX_CONSUMED_REGISTRY_ENTRIES_PER_SESSION, session_id ))); diff --git a/pkg/tbtc/signer/src/engine/tests.rs b/pkg/tbtc/signer/src/engine/tests.rs index f82cf72ef0..8c47312522 100644 --- a/pkg/tbtc/signer/src/engine/tests.rs +++ b/pkg/tbtc/signer/src/engine/tests.rs @@ -702,7 +702,7 @@ fn persisted_session_state_fixture() -> PersistedSessionState { refresh_history: vec![], emergency_rekey_event: None, consumed_interactive_attempt_markers: vec![], - aggregated_interactive_attempt_markers: vec![], + aggregated_interactive_attempt_signatures: Default::default(), } } @@ -12842,7 +12842,7 @@ fn interactive_aggregate_produces_and_self_verifies_bip340() { } #[test] -fn interactive_aggregate_rejects_repeat_aggregate_of_completed_attempt() { +fn interactive_aggregate_is_idempotent_for_completed_attempt() { let _guard = lock_test_state(); reset_for_tests(); @@ -12905,26 +12905,20 @@ fn interactive_aggregate_rejects_repeat_aggregate_of_completed_attempt() { }; // First aggregate completes the attempt. - interactive_aggregate(aggregate_request.clone()).expect("first interactive aggregate"); + let first = + interactive_aggregate(aggregate_request.clone()).expect("first interactive aggregate"); - // A second aggregate for the same attempt is rejected by the durable - // completion marker rather than recomputed (Phase 7.2b design section 6). - let err = interactive_aggregate(aggregate_request) - .expect_err("re-aggregating a completed attempt must be rejected"); - assert!( - matches!( - err, - EngineError::InteractiveAttemptAlreadyAggregated { ref attempt_id, .. } - if *attempt_id == opened.attempt_id - ), - "unexpected error: {err:?}" - ); - assert_eq!(err.code(), "interactive_attempt_already_aggregated"); - assert_eq!(err.recovery_class(), "recoverable"); + // A second aggregate for the same attempt returns the SAME signature + // (idempotent re-emission) rather than recomputing or erroring (Phase 7.2b + // design section 6). + let second = interactive_aggregate(aggregate_request) + .expect("re-aggregating a completed attempt returns the stored signature"); + assert_eq!(second.attempt_id, opened.attempt_id); + assert_eq!(second.signature_hex, first.signature_hex); } #[test] -fn interactive_aggregate_completion_marker_survives_process_restart() { +fn interactive_aggregate_signature_recoverable_across_restart() { let _guard = lock_test_state(); let state_path = configure_test_state_path("interactive_aggregate_marker_restart"); reset_for_tests(); @@ -12986,22 +12980,21 @@ fn interactive_aggregate_completion_marker_survives_process_restart() { ], taproot_merkle_root_hex: None, }; - interactive_aggregate(aggregate_request.clone()).expect("first interactive aggregate"); + let first = + interactive_aggregate(aggregate_request.clone()).expect("first interactive aggregate"); - // The completion marker is the only durable interactive artifact (live - // nonce state is gone after restart by construction). It must survive a - // reload so a replayed aggregate is still rejected - this also exercises - // the marker's persistence round-trip (serialize + reload validation). + // The completion record (the aggregate signature) is the only durable + // interactive artifact (live nonce state is gone after restart by + // construction). It must survive a reload so the engine re-emits the + // identical signature instead of forcing a brand-new signing attempt - + // this also exercises the record's persistence round-trip (serialize + + // reload validation). simulate_process_restart_for_tests(); reload_state_from_storage_for_tests(); - let err = interactive_aggregate(aggregate_request) - .expect_err("a completed attempt must stay completed across restart"); - assert!( - matches!(err, EngineError::InteractiveAttemptAlreadyAggregated { .. }), - "unexpected error: {err:?}" - ); - assert_eq!(err.code(), "interactive_attempt_already_aggregated"); + let after_restart = interactive_aggregate(aggregate_request) + .expect("a completed attempt's signature is recoverable across restart"); + assert_eq!(after_restart.signature_hex, first.signature_hex); reset_for_tests(); cleanup_test_state_artifacts(&state_path); diff --git a/pkg/tbtc/signer/src/errors.rs b/pkg/tbtc/signer/src/errors.rs index f3d28c6906..636d9ccf82 100644 --- a/pkg/tbtc/signer/src/errors.rs +++ b/pkg/tbtc/signer/src/errors.rs @@ -82,17 +82,6 @@ pub enum EngineError { session_id: String, attempt_id: String, }, - /// Returned when InteractiveAggregate is invoked again for an attempt that - /// already produced an aggregate signature in this session. The per-attempt - /// "aggregated" marker is durable, so a completed attempt stays completed - /// across restart; re-aggregation is rejected rather than recomputed. - /// Distinct code so callers match on - /// `interactive_attempt_already_aggregated` rather than the message. - #[error("interactive attempt [{attempt_id}] already aggregated in session [{session_id}]")] - InteractiveAttemptAlreadyAggregated { - session_id: String, - attempt_id: String, - }, #[error("internal error: {0}")] Internal(String), } @@ -115,9 +104,6 @@ impl EngineError { Self::ConsumedAttemptReplay { .. } => "consumed_attempt_replay", Self::ConsumedRoundReplay { .. } => "consumed_round_replay", Self::ConsumedNonceReplay { .. } => "consumed_nonce_replay", - Self::InteractiveAttemptAlreadyAggregated { .. } => { - "interactive_attempt_already_aggregated" - } Self::Internal(_) => "internal_error", } } @@ -142,10 +128,6 @@ impl EngineError { Self::ConsumedAttemptReplay { .. } => "recoverable", Self::ConsumedRoundReplay { .. } => "recoverable", Self::ConsumedNonceReplay { .. } => "recoverable", - // The aggregate is deterministic over public data and the attempt - // is durably marked complete; a re-aggregation request is a benign - // duplicate the caller should not retry, not an engine fault. - Self::InteractiveAttemptAlreadyAggregated { .. } => "recoverable", Self::SessionFinalized { .. } => "terminal", Self::SessionNotFound { .. } => "terminal", Self::Internal(_) => "terminal", @@ -188,20 +170,6 @@ mod tests { ); } - #[test] - fn interactive_attempt_already_aggregated_has_stable_code_and_message_format() { - let err = EngineError::InteractiveAttemptAlreadyAggregated { - session_id: "session-a".to_string(), - attempt_id: "attempt-1".to_string(), - }; - assert_eq!(err.code(), "interactive_attempt_already_aggregated"); - assert_eq!(err.recovery_class(), "recoverable"); - assert_eq!( - err.to_string(), - "interactive attempt [attempt-1] already aggregated in session [session-a]", - ); - } - #[test] fn recovery_class_maps_retryable_and_terminal_errors() { assert_eq!( From c50ac59fdfe6a4f482f5529557c4497730dd049c Mon Sep 17 00:00:00 2001 From: maclane Date: Sat, 13 Jun 2026 18:09:10 -0400 Subject: [PATCH 3/6] fix(tbtc/signer): return persisted signature on aggregate race (Codex/Gemini P2) Both bots' re-review converged on a race in the idempotency follow-up: when two InteractiveAggregate calls for the same attempt race past the empty pre-check, the loser found the record already present, skipped the store, but still returned the signature IT recomputed. Different responsive subsets can produce different VALID signatures for the same attempt, so the value a caller received could diverge from the persisted one that restarts and later re-emissions return - breaking the idempotent completion-record contract. The post-lock path now returns the canonical PERSISTED signature when the attempt is already recorded (early return, no re-store, no success count), matching the pre-check; dropped the recorded_new flag. Strengthened the idempotency test to overwrite the stored record with a sentinel and assert the repeat returns exactly that (proving return-of-recorded-value, which a plain equality check could not distinguish from a deterministic recompute). fmt + clippy --all-targets --all-features -D + full suite (273) + Phase-5 chaos green. Co-Authored-By: Claude Opus 4.8 --- pkg/tbtc/signer/src/engine/interactive.rs | 72 ++++++++++++----------- pkg/tbtc/signer/src/engine/tests.rs | 22 +++++-- 2 files changed, 56 insertions(+), 38 deletions(-) diff --git a/pkg/tbtc/signer/src/engine/interactive.rs b/pkg/tbtc/signer/src/engine/interactive.rs index 37bcf4f255..cfe071f799 100644 --- a/pkg/tbtc/signer/src/engine/interactive.rs +++ b/pkg/tbtc/signer/src/engine/interactive.rs @@ -697,12 +697,8 @@ pub fn interactive_aggregate( // so a repeat InteractiveAggregate returns the same signature (idempotent) // rather than recomputing (Phase 7.2b design section 6). The engine lock was // dropped for the aggregation crypto above; re-acquire it to record and - // persist. The aggregate is deterministic over public data, so a concurrent - // duplicate that raced past the pre-check recomputed the identical - // signature: if the record already exists we return ours without storing - // twice. Persist before reporting success; on persist failure roll the - // record back and fail closed, leaving no half-recorded completion. Count a - // success only for a freshly recorded aggregation, not a re-emission. + // persist. Persist before reporting success; on persist failure roll the + // record back and fail closed, leaving no half-recorded completion. let mut guard = state()? .lock() .map_err(|_| EngineError::Internal("engine lock poisoned".to_string()))?; @@ -711,39 +707,49 @@ pub fn interactive_aggregate( session_id: request.session_id.clone(), } })?; - let recorded_new = !session + // A concurrent aggregate that raced past the pre-check may already have + // recorded this attempt. Different responsive subsets can each produce a + // VALID but distinct signature for the same attempt, so return the + // canonical PERSISTED signature, not the one this call just recomputed - + // otherwise the value a caller receives could diverge from what restarts + // and later re-emissions return. No re-store and no success count here: the + // call that recorded the attempt already counted it. + if let Some(existing) = session .aggregated_interactive_attempt_signatures - .contains_key(&attempt_id); - if recorded_new { - ensure_consumed_registry_capacity_for_insert( - session.aggregated_interactive_attempt_signatures.len(), - false, - "aggregated_interactive_attempt_signatures", - &request.session_id, - )?; + .get(&attempt_id) + { + return Ok(InteractiveAggregateResult { + session_id: request.session_id.clone(), + attempt_id: attempt_id.clone(), + signature_hex: existing.clone(), + }); + } + ensure_consumed_registry_capacity_for_insert( + session.aggregated_interactive_attempt_signatures.len(), + false, + "aggregated_interactive_attempt_signatures", + &request.session_id, + )?; + session + .aggregated_interactive_attempt_signatures + .insert(attempt_id.clone(), signature_hex.clone()); + if let Err(persist_error) = persist_engine_state_to_storage(&guard) { + let session = guard + .sessions + .get_mut(&request.session_id) + .expect("session existed under the held engine lock"); session .aggregated_interactive_attempt_signatures - .insert(attempt_id.clone(), signature_hex.clone()); - if let Err(persist_error) = persist_engine_state_to_storage(&guard) { - let session = guard - .sessions - .get_mut(&request.session_id) - .expect("session existed under the held engine lock"); - session - .aggregated_interactive_attempt_signatures - .remove(&attempt_id); - return Err(persist_error); - } + .remove(&attempt_id); + return Err(persist_error); } drop(guard); - if recorded_new { - record_hardening_telemetry(|telemetry| { - telemetry.interactive_aggregate_success_total = telemetry - .interactive_aggregate_success_total - .saturating_add(1); - }); - } + record_hardening_telemetry(|telemetry| { + telemetry.interactive_aggregate_success_total = telemetry + .interactive_aggregate_success_total + .saturating_add(1); + }); Ok(InteractiveAggregateResult { session_id: request.session_id, diff --git a/pkg/tbtc/signer/src/engine/tests.rs b/pkg/tbtc/signer/src/engine/tests.rs index 8c47312522..2b211e5be4 100644 --- a/pkg/tbtc/signer/src/engine/tests.rs +++ b/pkg/tbtc/signer/src/engine/tests.rs @@ -12904,17 +12904,29 @@ fn interactive_aggregate_is_idempotent_for_completed_attempt() { taproot_merkle_root_hex: None, }; - // First aggregate completes the attempt. + // First aggregate completes the attempt and records its signature. let first = interactive_aggregate(aggregate_request.clone()).expect("first interactive aggregate"); - // A second aggregate for the same attempt returns the SAME signature - // (idempotent re-emission) rather than recomputing or erroring (Phase 7.2b - // design section 6). + // A repeat aggregate returns the PERSISTED signature, not a recompute + // (Phase 7.2b design section 6). A plain equality check would pass even on + // a recompute (aggregate is deterministic), so overwrite the stored record + // with a sentinel and confirm the repeat returns exactly that - the same + // return-the-recorded-value property the post-race path relies on. + { + let mut guard = state().expect("engine state").lock().expect("engine lock"); + guard + .sessions + .get_mut(session_id) + .expect("session") + .aggregated_interactive_attempt_signatures + .insert(opened.attempt_id.clone(), "00sentinel".to_string()); + } let second = interactive_aggregate(aggregate_request) .expect("re-aggregating a completed attempt returns the stored signature"); assert_eq!(second.attempt_id, opened.attempt_id); - assert_eq!(second.signature_hex, first.signature_hex); + assert_eq!(second.signature_hex, "00sentinel"); + assert_ne!(second.signature_hex, first.signature_hex); } #[test] From 8bca31e98b3feff1517227c96dcf19ba70833e48 Mon Sep 17 00:00:00 2001 From: maclane Date: Sat, 13 Jun 2026 18:30:20 -0400 Subject: [PATCH 4/6] fix(tbtc/signer): validate completion record against request on re-emit (Codex P2) Codex's 3rd-round re-review: the completion record is keyed by attempt_id, which does NOT bind taproot_merkle_root, and the coordinator is in the threat model. A reused attempt_id carrying a different root/message would re-emit the stored signature, which would not verify under the caller's requested tweaked key. Re-emission (both the pre-check and the post-race collision path) now returns the recorded signature only when it verifies under THIS request's tweaked group key and message; a reused attempt_id with mismatched aggregate inputs is rejected with a validation error rather than handed a non-verifying signature. Added recorded_aggregate_matches_request (decode + tweak + verify, mirroring the aggregate self-verify). Test rework: dropped the garbage sentinel (now correctly rejected by the verify), kept the same-inputs idempotent + restart re-emission tests, added a mismatched-root rejection assertion. fmt + clippy -D + full suite (273) + Phase-5 chaos green. Co-Authored-By: Claude Opus 4.8 --- pkg/tbtc/signer/src/engine/interactive.rs | 111 +++++++++++++++++----- pkg/tbtc/signer/src/engine/tests.rs | 39 ++++---- 2 files changed, 106 insertions(+), 44 deletions(-) diff --git a/pkg/tbtc/signer/src/engine/interactive.rs b/pkg/tbtc/signer/src/engine/interactive.rs index cfe071f799..dad773670a 100644 --- a/pkg/tbtc/signer/src/engine/interactive.rs +++ b/pkg/tbtc/signer/src/engine/interactive.rs @@ -561,6 +561,34 @@ pub fn interactive_round2( }) } +// Does a recorded aggregate signature verify under THIS request's tweaked +// group key and message? The completion record (section 6) is keyed by +// attempt_id, which does NOT bind the taproot root, and the coordinator may be +// adversarial - so the stored signature is re-emitted only when it is actually +// valid for the caller's package/root, never a signature that would fail to +// verify for these inputs. +fn recorded_aggregate_matches_request( + public_key_package: &frost::keys::PublicKeyPackage, + taproot_merkle_root: Option<&[u8; 32]>, + signing_package: &frost::SigningPackage, + recorded_signature_hex: &str, +) -> bool { + let Ok(signature_bytes) = hex::decode(recorded_signature_hex) else { + return false; + }; + let Ok(signature) = frost::Signature::deserialize(&signature_bytes) else { + return false; + }; + let verification_key_package = match taproot_merkle_root { + Some(root) => public_key_package.clone().tweak(Some(root.as_slice())), + None => public_key_package.clone(), + }; + verification_key_package + .verifying_key() + .verify(signing_package.message().as_slice(), &signature) + .is_ok() +} + pub fn interactive_aggregate( request: InteractiveAggregateRequest, ) -> Result { @@ -612,34 +640,50 @@ pub fn interactive_aggregate( session_id: request.session_id.clone(), } })?; - // Idempotent re-emission (Phase 7.2b design section 6): if this attempt - // already aggregated, return the stored signature without recomputing. - // The record is durable, so the signature stays recoverable from the - // engine across restart - a host that lost the FFI response need not - // spend a fresh signing attempt to reproduce a signature the engine - // already holds. - if let Some(signature_hex) = session - .aggregated_interactive_attempt_signatures - .get(&attempt_id) - { - return Ok(InteractiveAggregateResult { - session_id: request.session_id.clone(), - attempt_id: attempt_id.clone(), - signature_hex: signature_hex.clone(), - }); - } if session.dkg_result.is_none() { return Err(EngineError::DkgNotReady { session_id: request.session_id.clone(), }); } - session + let public_key_package = session .dkg_public_key_package .as_ref() .ok_or_else(|| { EngineError::Internal("missing DKG public key package cache".to_string()) })? - .clone() + .clone(); + // Idempotent re-emission (Phase 7.2b design section 6): a completed + // attempt returns its recorded signature without recomputing, so the + // signature stays recoverable from the engine across restart - a host + // that lost the FFI response need not spend a fresh signing attempt. + // The record is keyed by attempt_id, which does NOT bind the taproot + // root, and the coordinator may be adversarial, so re-emit ONLY when the + // stored signature actually verifies under THIS request's tweaked group + // key and message; a reused attempt_id carrying different aggregate + // inputs is rejected rather than handed a signature that fails for them. + if let Some(existing) = session + .aggregated_interactive_attempt_signatures + .get(&attempt_id) + { + if recorded_aggregate_matches_request( + &public_key_package, + taproot_merkle_root.as_ref(), + &signing_package, + existing, + ) { + return Ok(InteractiveAggregateResult { + session_id: request.session_id.clone(), + attempt_id: attempt_id.clone(), + signature_hex: existing.clone(), + }); + } + return Err(EngineError::Validation(format!( + "InteractiveAggregate: attempt [{attempt_id}] already aggregated under a \ + different package/root; reuse of an attempt_id for different aggregate \ + inputs is rejected" + ))); + } + public_key_package }; drop(guard); @@ -710,19 +754,34 @@ pub fn interactive_aggregate( // A concurrent aggregate that raced past the pre-check may already have // recorded this attempt. Different responsive subsets can each produce a // VALID but distinct signature for the same attempt, so return the - // canonical PERSISTED signature, not the one this call just recomputed - + // canonical PERSISTED signature when it verifies under this request's + // tweaked key and message (not the one this call just recomputed) - // otherwise the value a caller receives could diverge from what restarts - // and later re-emissions return. No re-store and no success count here: the - // call that recorded the attempt already counted it. + // and later re-emissions return. A record that does NOT verify for these + // inputs means the attempt_id was reused for a different package/root: + // reject it. No re-store and no success count on re-emission: the call that + // recorded the attempt already counted it. if let Some(existing) = session .aggregated_interactive_attempt_signatures .get(&attempt_id) { - return Ok(InteractiveAggregateResult { - session_id: request.session_id.clone(), - attempt_id: attempt_id.clone(), - signature_hex: existing.clone(), - }); + if recorded_aggregate_matches_request( + &public_key_package, + taproot_merkle_root.as_ref(), + &signing_package, + existing, + ) { + return Ok(InteractiveAggregateResult { + session_id: request.session_id.clone(), + attempt_id: attempt_id.clone(), + signature_hex: existing.clone(), + }); + } + return Err(EngineError::Validation(format!( + "InteractiveAggregate: attempt [{attempt_id}] already aggregated under a \ + different package/root; reuse of an attempt_id for different aggregate \ + inputs is rejected" + ))); } ensure_consumed_registry_capacity_for_insert( session.aggregated_interactive_attempt_signatures.len(), diff --git a/pkg/tbtc/signer/src/engine/tests.rs b/pkg/tbtc/signer/src/engine/tests.rs index 2b211e5be4..d251eefb08 100644 --- a/pkg/tbtc/signer/src/engine/tests.rs +++ b/pkg/tbtc/signer/src/engine/tests.rs @@ -12908,25 +12908,28 @@ fn interactive_aggregate_is_idempotent_for_completed_attempt() { let first = interactive_aggregate(aggregate_request.clone()).expect("first interactive aggregate"); - // A repeat aggregate returns the PERSISTED signature, not a recompute - // (Phase 7.2b design section 6). A plain equality check would pass even on - // a recompute (aggregate is deterministic), so overwrite the stored record - // with a sentinel and confirm the repeat returns exactly that - the same - // return-the-recorded-value property the post-race path relies on. - { - let mut guard = state().expect("engine state").lock().expect("engine lock"); - guard - .sessions - .get_mut(session_id) - .expect("session") - .aggregated_interactive_attempt_signatures - .insert(opened.attempt_id.clone(), "00sentinel".to_string()); - } - let second = interactive_aggregate(aggregate_request) - .expect("re-aggregating a completed attempt returns the stored signature"); + // Re-aggregating the SAME attempt with the SAME inputs returns the recorded + // signature (idempotent re-emission, Phase 7.2b design section 6). + let second = interactive_aggregate(aggregate_request.clone()) + .expect("re-aggregating with the same inputs returns the recorded signature"); assert_eq!(second.attempt_id, opened.attempt_id); - assert_eq!(second.signature_hex, "00sentinel"); - assert_ne!(second.signature_hex, first.signature_hex); + assert_eq!(second.signature_hex, first.signature_hex); + + // Reusing the attempt_id with a DIFFERENT taproot root is rejected, not + // handed the recorded key-path signature (which would not verify under the + // tweaked key). The completion record is keyed by attempt_id, which does + // not bind the root, so re-emission is validated against the request's + // package/root before returning. + let mismatched_root_request = InteractiveAggregateRequest { + taproot_merkle_root_hex: Some("11".repeat(32)), + ..aggregate_request + }; + let err = interactive_aggregate(mismatched_root_request) + .expect_err("reusing an attempt_id with a different root must be rejected"); + assert!( + matches!(err, EngineError::Validation(ref m) if m.contains("different package/root")), + "unexpected error: {err:?}" + ); } #[test] From 77a16a2bae6823c1d945576e71c21b48db1069f2 Mon Sep 17 00:00:00 2001 From: maclane Date: Sat, 13 Jun 2026 18:45:18 -0400 Subject: [PATCH 5/6] fix(tbtc/signer): reject empty attempt_id in InteractiveAggregate (Codex P2) Codex 4th-round re-review: a completed aggregate persists a completion record keyed by attempt_id, and the reload path rejects an empty key, so an empty attempt_id (malformed or malicious) could write durable state that fails to reload - bricking the engine on restart. interactive_aggregate now rejects an empty attempt_id up front (before any decode/aggregate/store), matching the loader's non-empty invariant. Not idempotency-specific: the write path simply didn't enforce what the read path requires. Test: aggregate with an empty attempt_id returns a validation error and persists nothing. fmt + clippy -D + full suite (274) + Phase-5 chaos green. Co-Authored-By: Claude Opus 4.8 --- pkg/tbtc/signer/src/engine/interactive.rs | 9 +++++++++ pkg/tbtc/signer/src/engine/tests.rs | 24 +++++++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/pkg/tbtc/signer/src/engine/interactive.rs b/pkg/tbtc/signer/src/engine/interactive.rs index dad773670a..5bc1321a3e 100644 --- a/pkg/tbtc/signer/src/engine/interactive.rs +++ b/pkg/tbtc/signer/src/engine/interactive.rs @@ -602,6 +602,15 @@ pub fn interactive_aggregate( enforce_provenance_gate()?; validate_session_id(&request.session_id)?; let attempt_id = canonical_attempt_id(&request.attempt_id); + // A completed aggregate persists a completion record keyed by attempt_id, + // and the reload path rejects an empty key; reject an empty attempt_id here + // too so a malformed (or malicious) request cannot write durable state that + // fails to reload after a restart. + if attempt_id.is_empty() { + return Err(EngineError::Validation( + "InteractiveAggregate: attempt_id must not be empty".to_string(), + )); + } let mut signing_package_bytes = decode_hex_field( "InteractiveAggregate", diff --git a/pkg/tbtc/signer/src/engine/tests.rs b/pkg/tbtc/signer/src/engine/tests.rs index d251eefb08..2fcab6997c 100644 --- a/pkg/tbtc/signer/src/engine/tests.rs +++ b/pkg/tbtc/signer/src/engine/tests.rs @@ -12932,6 +12932,30 @@ fn interactive_aggregate_is_idempotent_for_completed_attempt() { ); } +#[test] +fn interactive_aggregate_rejects_empty_attempt_id() { + let _guard = lock_test_state(); + reset_for_tests(); + + // An empty attempt_id must be rejected before anything is persisted: the + // completion record is keyed by attempt_id and the reload path rejects an + // empty key, so persisting one would brick restart. The guard runs before + // the signing-package/share decode, so the placeholder inputs are never + // reached. + let err = interactive_aggregate(InteractiveAggregateRequest { + session_id: "interactive-aggregate-empty-attempt".to_string(), + attempt_id: String::new(), + signing_package_hex: String::new(), + signature_shares: vec![], + taproot_merkle_root_hex: None, + }) + .expect_err("an empty attempt_id must be rejected"); + assert!( + matches!(err, EngineError::Validation(ref m) if m.contains("attempt_id must not be empty")), + "unexpected error: {err:?}" + ); +} + #[test] fn interactive_aggregate_signature_recoverable_across_restart() { let _guard = lock_test_state(); From 2e062ff51fc96571f6a0fd8a7c3910197ef3b061 Mon Sep 17 00:00:00 2001 From: maclane Date: Sat, 13 Jun 2026 19:28:33 -0400 Subject: [PATCH 6/6] refactor(tbtc/signer): simplify 7.2b-1 to completion marker + reject Per design decision after 5 review rounds on the re-emission path: the InteractiveAggregate completion record reverts from idempotent signature re-emission to a simple completion MARKER that rejects re-aggregation of a completed attempt (InteractiveAttemptAlreadyAggregated). Matches frozen spec section 6 ('mark the session complete'); a lost signature is recovered with a fresh ROAST attempt, not re-aggregate. Eliminates the re-emission surface that accumulated findings: the concurrent-race signature divergence, the request package/root binding on re-emit, and the lost-shares recovery gap - none apply when a completed attempt is simply rejected. The empty-attempt_id guard + restart-safety rationale are kept (the marker set's loader still rejects empty keys). - aggregated_interactive_attempt_markers: HashSet on SessionState / Vec on PersistedSessionState (serde default, bounded, sorted), replacing the attempt_id->signature map. - interactive_aggregate rejects a completed attempt at the pre-check and the post-aggregation re-check; no signature stored or re-emitted. - Re-added EngineError::InteractiveAttemptAlreadyAggregated. Tests: re-aggregation rejected (immediate + across restart+reload); empty attempt_id rejected. fmt + clippy -D + full suite (275) + chaos green. Co-Authored-By: Claude Opus 4.8 --- pkg/tbtc/signer/src/engine/interactive.rs | 150 ++++++---------------- pkg/tbtc/signer/src/engine/persistence.rs | 42 +++--- pkg/tbtc/signer/src/engine/state.rs | 39 ++---- pkg/tbtc/signer/src/engine/tests.rs | 66 +++++----- pkg/tbtc/signer/src/errors.rs | 33 +++++ 5 files changed, 138 insertions(+), 192 deletions(-) diff --git a/pkg/tbtc/signer/src/engine/interactive.rs b/pkg/tbtc/signer/src/engine/interactive.rs index 5bc1321a3e..ebe932cda5 100644 --- a/pkg/tbtc/signer/src/engine/interactive.rs +++ b/pkg/tbtc/signer/src/engine/interactive.rs @@ -561,34 +561,6 @@ pub fn interactive_round2( }) } -// Does a recorded aggregate signature verify under THIS request's tweaked -// group key and message? The completion record (section 6) is keyed by -// attempt_id, which does NOT bind the taproot root, and the coordinator may be -// adversarial - so the stored signature is re-emitted only when it is actually -// valid for the caller's package/root, never a signature that would fail to -// verify for these inputs. -fn recorded_aggregate_matches_request( - public_key_package: &frost::keys::PublicKeyPackage, - taproot_merkle_root: Option<&[u8; 32]>, - signing_package: &frost::SigningPackage, - recorded_signature_hex: &str, -) -> bool { - let Ok(signature_bytes) = hex::decode(recorded_signature_hex) else { - return false; - }; - let Ok(signature) = frost::Signature::deserialize(&signature_bytes) else { - return false; - }; - let verification_key_package = match taproot_merkle_root { - Some(root) => public_key_package.clone().tweak(Some(root.as_slice())), - None => public_key_package.clone(), - }; - verification_key_package - .verifying_key() - .verify(signing_package.message().as_slice(), &signature) - .is_ok() -} - pub fn interactive_aggregate( request: InteractiveAggregateRequest, ) -> Result { @@ -602,10 +574,10 @@ pub fn interactive_aggregate( enforce_provenance_gate()?; validate_session_id(&request.session_id)?; let attempt_id = canonical_attempt_id(&request.attempt_id); - // A completed aggregate persists a completion record keyed by attempt_id, - // and the reload path rejects an empty key; reject an empty attempt_id here - // too so a malformed (or malicious) request cannot write durable state that - // fails to reload after a restart. + // The completion marker persists attempt_id, and the reload path rejects an + // empty key; reject an empty attempt_id here too so a malformed (or + // malicious) request cannot write durable state that fails to reload after + // a restart. if attempt_id.is_empty() { return Err(EngineError::Validation( "InteractiveAggregate: attempt_id must not be empty".to_string(), @@ -649,50 +621,30 @@ pub fn interactive_aggregate( session_id: request.session_id.clone(), } })?; + // Reject a completed attempt: re-aggregation is not a recovery path (a + // lost signature is recovered with a fresh attempt), and the marker is + // durable so a completed attempt stays rejected across a restart. + if session + .aggregated_interactive_attempt_markers + .contains(&attempt_id) + { + return Err(EngineError::InteractiveAttemptAlreadyAggregated { + session_id: request.session_id.clone(), + attempt_id, + }); + } if session.dkg_result.is_none() { return Err(EngineError::DkgNotReady { session_id: request.session_id.clone(), }); } - let public_key_package = session + session .dkg_public_key_package .as_ref() .ok_or_else(|| { EngineError::Internal("missing DKG public key package cache".to_string()) })? - .clone(); - // Idempotent re-emission (Phase 7.2b design section 6): a completed - // attempt returns its recorded signature without recomputing, so the - // signature stays recoverable from the engine across restart - a host - // that lost the FFI response need not spend a fresh signing attempt. - // The record is keyed by attempt_id, which does NOT bind the taproot - // root, and the coordinator may be adversarial, so re-emit ONLY when the - // stored signature actually verifies under THIS request's tweaked group - // key and message; a reused attempt_id carrying different aggregate - // inputs is rejected rather than handed a signature that fails for them. - if let Some(existing) = session - .aggregated_interactive_attempt_signatures - .get(&attempt_id) - { - if recorded_aggregate_matches_request( - &public_key_package, - taproot_merkle_root.as_ref(), - &signing_package, - existing, - ) { - return Ok(InteractiveAggregateResult { - session_id: request.session_id.clone(), - attempt_id: attempt_id.clone(), - signature_hex: existing.clone(), - }); - } - return Err(EngineError::Validation(format!( - "InteractiveAggregate: attempt [{attempt_id}] already aggregated under a \ - different package/root; reuse of an attempt_id for different aggregate \ - inputs is rejected" - ))); - } - public_key_package + .clone() }; drop(guard); @@ -746,12 +698,12 @@ pub fn interactive_aggregate( .map_err(|e| EngineError::Internal(format!("failed to serialize aggregate: {e}")))?; let signature_hex = hex::encode(signature_bytes); - // Record the aggregate signature for this attempt before reporting success, - // so a repeat InteractiveAggregate returns the same signature (idempotent) - // rather than recomputing (Phase 7.2b design section 6). The engine lock was - // dropped for the aggregation crypto above; re-acquire it to record and - // persist. Persist before reporting success; on persist failure roll the - // record back and fail closed, leaving no half-recorded completion. + // Mark the attempt complete before reporting success, so a repeat + // InteractiveAggregate is rejected rather than recomputed (Phase 7.2b design + // section 6). The engine lock was dropped for the aggregation crypto above; + // re-acquire it, re-check the marker (a concurrent aggregate may have + // completed first), insert it, and persist before reporting success; on + // persist failure roll the marker back and fail closed. let mut guard = state()? .lock() .map_err(|_| EngineError::Internal("engine lock poisoned".to_string()))?; @@ -760,54 +712,34 @@ pub fn interactive_aggregate( session_id: request.session_id.clone(), } })?; - // A concurrent aggregate that raced past the pre-check may already have - // recorded this attempt. Different responsive subsets can each produce a - // VALID but distinct signature for the same attempt, so return the - // canonical PERSISTED signature when it verifies under this request's - // tweaked key and message (not the one this call just recomputed) - - // otherwise the value a caller receives could diverge from what restarts - // and later re-emissions return. A record that does NOT verify for these - // inputs means the attempt_id was reused for a different package/root: - // reject it. No re-store and no success count on re-emission: the call that - // recorded the attempt already counted it. - if let Some(existing) = session - .aggregated_interactive_attempt_signatures - .get(&attempt_id) + // A concurrent aggregate that raced past the pre-check may have completed + // this attempt first; if the marker is now present, reject this call's + // re-aggregation - the winner already produced the attempt's signature. + if session + .aggregated_interactive_attempt_markers + .contains(&attempt_id) { - if recorded_aggregate_matches_request( - &public_key_package, - taproot_merkle_root.as_ref(), - &signing_package, - existing, - ) { - return Ok(InteractiveAggregateResult { - session_id: request.session_id.clone(), - attempt_id: attempt_id.clone(), - signature_hex: existing.clone(), - }); - } - return Err(EngineError::Validation(format!( - "InteractiveAggregate: attempt [{attempt_id}] already aggregated under a \ - different package/root; reuse of an attempt_id for different aggregate \ - inputs is rejected" - ))); + return Err(EngineError::InteractiveAttemptAlreadyAggregated { + session_id: request.session_id.clone(), + attempt_id, + }); } - ensure_consumed_registry_capacity_for_insert( - session.aggregated_interactive_attempt_signatures.len(), - false, - "aggregated_interactive_attempt_signatures", + ensure_consumed_registry_insert_capacity( + &session.aggregated_interactive_attempt_markers, + &attempt_id, + "aggregated_interactive_attempt_markers", &request.session_id, )?; session - .aggregated_interactive_attempt_signatures - .insert(attempt_id.clone(), signature_hex.clone()); + .aggregated_interactive_attempt_markers + .insert(attempt_id.clone()); if let Err(persist_error) = persist_engine_state_to_storage(&guard) { let session = guard .sessions .get_mut(&request.session_id) .expect("session existed under the held engine lock"); session - .aggregated_interactive_attempt_signatures + .aggregated_interactive_attempt_markers .remove(&attempt_id); return Err(persist_error); } diff --git a/pkg/tbtc/signer/src/engine/persistence.rs b/pkg/tbtc/signer/src/engine/persistence.rs index 1db3d40f03..ce68b66094 100644 --- a/pkg/tbtc/signer/src/engine/persistence.rs +++ b/pkg/tbtc/signer/src/engine/persistence.rs @@ -44,11 +44,11 @@ pub(crate) struct PersistedSessionState { // interactive state, including nonces, never persists). #[serde(default, skip_serializing_if = "Vec::is_empty")] pub(crate) consumed_interactive_attempt_markers: Vec, - // Phase 7.2b InteractiveAggregate completion record (see SessionState): - // attempt_id -> aggregate signature hex. serde(default) keeps state written - // before 7.2b loadable: an absent field deserializes to an empty map. - #[serde(default, skip_serializing_if = "BTreeMap::is_empty")] - pub(crate) aggregated_interactive_attempt_signatures: BTreeMap, + // Phase 7.2b InteractiveAggregate completion markers (see SessionState). + // serde(default) keeps state written before 7.2b loadable: an absent field + // deserializes to an empty set. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub(crate) aggregated_interactive_attempt_markers: Vec, } #[derive(Clone, Debug, Deserialize, Serialize)] @@ -1292,23 +1292,24 @@ impl TryFrom for SessionState { "consumed_interactive_attempt_markers", )?; - let mut aggregated_interactive_attempt_signatures = BTreeMap::new(); - for (attempt_id, signature_hex) in persisted.aggregated_interactive_attempt_signatures { - if attempt_id.is_empty() { + let mut aggregated_interactive_attempt_markers = HashSet::new(); + for attempt_marker in persisted.aggregated_interactive_attempt_markers { + if attempt_marker.is_empty() { return Err(EngineError::Internal( - "persisted aggregated interactive attempt id must be non-empty".to_string(), + "persisted aggregated interactive attempt marker must be non-empty".to_string(), )); } - if signature_hex.is_empty() { + + if !aggregated_interactive_attempt_markers.insert(attempt_marker.clone()) { return Err(EngineError::Internal(format!( - "persisted aggregated interactive attempt [{attempt_id}] signature must be non-empty" + "duplicate persisted aggregated interactive attempt marker [{}]", + attempt_marker ))); } - aggregated_interactive_attempt_signatures.insert(attempt_id, signature_hex); } ensure_consumed_registry_persisted_bound( - aggregated_interactive_attempt_signatures.len(), - "aggregated_interactive_attempt_signatures", + aggregated_interactive_attempt_markers.len(), + "aggregated_interactive_attempt_markers", )?; if persisted.attempt_transition_records.len() > TBTC_SIGNER_MAX_ATTEMPT_TRANSITION_RECORDS_PER_SESSION @@ -1369,7 +1370,7 @@ impl TryFrom for SessionState { // only the consumption markers survive. interactive_signing: None, consumed_interactive_attempt_markers, - aggregated_interactive_attempt_signatures, + aggregated_interactive_attempt_markers, }) } } @@ -1480,9 +1481,12 @@ impl TryFrom<&SessionState> for PersistedSessionState { .cloned() .collect::>(); consumed_interactive_attempt_markers.sort_unstable(); - let aggregated_interactive_attempt_signatures = session_state - .aggregated_interactive_attempt_signatures - .clone(); + let mut aggregated_interactive_attempt_markers = session_state + .aggregated_interactive_attempt_markers + .iter() + .cloned() + .collect::>(); + aggregated_interactive_attempt_markers.sort_unstable(); Ok(PersistedSessionState { dkg_request_fingerprint: session_state.dkg_request_fingerprint.clone(), @@ -1507,7 +1511,7 @@ impl TryFrom<&SessionState> for PersistedSessionState { refresh_history: session_state.refresh_history.clone(), emergency_rekey_event: session_state.emergency_rekey_event.clone(), consumed_interactive_attempt_markers, - aggregated_interactive_attempt_signatures, + aggregated_interactive_attempt_markers, }) } } diff --git a/pkg/tbtc/signer/src/engine/state.rs b/pkg/tbtc/signer/src/engine/state.rs index 75ec48f4e2..ab52ab36e1 100644 --- a/pkg/tbtc/signer/src/engine/state.rs +++ b/pkg/tbtc/signer/src/engine/state.rs @@ -111,15 +111,14 @@ pub(crate) struct SessionState { pub(crate) emergency_rekey_event: Option, pub(crate) interactive_signing: Option, pub(crate) consumed_interactive_attempt_markers: HashSet, - // Phase 7.2b InteractiveAggregate completion record: maps a completed - // attempt to the aggregate signature hex it produced, so a repeat - // InteractiveAggregate returns the same signature (idempotent) rather than - // recomputing. Durable like the consumed markers (markers-only durability) - // and bounded the same way. Not security-load-bearing - the aggregate is - // deterministic over public data and the signature is public - but the - // engine stays the source of truth for a completed attempt's signature, so - // a host that loses the FFI response recovers it without a new attempt. - pub(crate) aggregated_interactive_attempt_signatures: BTreeMap, + // Phase 7.2b InteractiveAggregate completion markers: an attempt whose + // aggregate signature has been produced is recorded here so a repeat + // InteractiveAggregate is rejected (re-aggregation is not a recovery path; + // a lost signature is recovered with a fresh attempt). Durable like the + // consumed markers (markers-only durability) and bounded the same way; not + // security-load-bearing (the aggregate is deterministic over public data), + // but the frozen Phase 7 spec marks the session complete. + pub(crate) aggregated_interactive_attempt_markers: HashSet, } #[derive(Default)] @@ -444,28 +443,12 @@ pub(crate) fn ensure_consumed_registry_insert_capacity( registry_name: &str, session_id: &str, ) -> Result<(), EngineError> { - ensure_consumed_registry_capacity_for_insert( - registry.len(), - registry.contains(entry), - registry_name, - session_id, - ) -} - -// Length-based core shared by the set-keyed consumed registries and the -// map-keyed Phase 7.2b aggregated-signature record, so both enforce one bound. -pub(crate) fn ensure_consumed_registry_capacity_for_insert( - registry_len: usize, - entry_already_present: bool, - registry_name: &str, - session_id: &str, -) -> Result<(), EngineError> { - if !entry_already_present - && registry_len >= TBTC_SIGNER_MAX_CONSUMED_REGISTRY_ENTRIES_PER_SESSION + if !registry.contains(entry) + && registry.len() >= TBTC_SIGNER_MAX_CONSUMED_REGISTRY_ENTRIES_PER_SESSION { return Err(EngineError::Internal(format!( "{registry_name} registry size [{}] reached max [{}] for session [{}]; use a new session_id", - registry_len, + registry.len(), TBTC_SIGNER_MAX_CONSUMED_REGISTRY_ENTRIES_PER_SESSION, session_id ))); diff --git a/pkg/tbtc/signer/src/engine/tests.rs b/pkg/tbtc/signer/src/engine/tests.rs index 2fcab6997c..60f8a2d2cb 100644 --- a/pkg/tbtc/signer/src/engine/tests.rs +++ b/pkg/tbtc/signer/src/engine/tests.rs @@ -702,7 +702,7 @@ fn persisted_session_state_fixture() -> PersistedSessionState { refresh_history: vec![], emergency_rekey_event: None, consumed_interactive_attempt_markers: vec![], - aggregated_interactive_attempt_signatures: Default::default(), + aggregated_interactive_attempt_markers: vec![], } } @@ -12842,7 +12842,7 @@ fn interactive_aggregate_produces_and_self_verifies_bip340() { } #[test] -fn interactive_aggregate_is_idempotent_for_completed_attempt() { +fn interactive_aggregate_rejects_repeat_aggregate_of_completed_attempt() { let _guard = lock_test_state(); reset_for_tests(); @@ -12904,32 +12904,25 @@ fn interactive_aggregate_is_idempotent_for_completed_attempt() { taproot_merkle_root_hex: None, }; - // First aggregate completes the attempt and records its signature. - let first = - interactive_aggregate(aggregate_request.clone()).expect("first interactive aggregate"); + // First aggregate completes the attempt. + interactive_aggregate(aggregate_request.clone()).expect("first interactive aggregate"); - // Re-aggregating the SAME attempt with the SAME inputs returns the recorded - // signature (idempotent re-emission, Phase 7.2b design section 6). - let second = interactive_aggregate(aggregate_request.clone()) - .expect("re-aggregating with the same inputs returns the recorded signature"); - assert_eq!(second.attempt_id, opened.attempt_id); - assert_eq!(second.signature_hex, first.signature_hex); - - // Reusing the attempt_id with a DIFFERENT taproot root is rejected, not - // handed the recorded key-path signature (which would not verify under the - // tweaked key). The completion record is keyed by attempt_id, which does - // not bind the root, so re-emission is validated against the request's - // package/root before returning. - let mismatched_root_request = InteractiveAggregateRequest { - taproot_merkle_root_hex: Some("11".repeat(32)), - ..aggregate_request - }; - let err = interactive_aggregate(mismatched_root_request) - .expect_err("reusing an attempt_id with a different root must be rejected"); + // Re-aggregating a completed attempt is rejected by the durable completion + // marker rather than recomputed (re-aggregation is not a recovery path; a + // lost signature is recovered with a fresh attempt). Phase 7.2b design + // section 6. + let err = interactive_aggregate(aggregate_request) + .expect_err("re-aggregating a completed attempt must be rejected"); assert!( - matches!(err, EngineError::Validation(ref m) if m.contains("different package/root")), + matches!( + err, + EngineError::InteractiveAttemptAlreadyAggregated { ref attempt_id, .. } + if *attempt_id == opened.attempt_id + ), "unexpected error: {err:?}" ); + assert_eq!(err.code(), "interactive_attempt_already_aggregated"); + assert_eq!(err.recovery_class(), "recoverable"); } #[test] @@ -12957,7 +12950,7 @@ fn interactive_aggregate_rejects_empty_attempt_id() { } #[test] -fn interactive_aggregate_signature_recoverable_across_restart() { +fn interactive_aggregate_completion_marker_survives_process_restart() { let _guard = lock_test_state(); let state_path = configure_test_state_path("interactive_aggregate_marker_restart"); reset_for_tests(); @@ -13019,21 +13012,22 @@ fn interactive_aggregate_signature_recoverable_across_restart() { ], taproot_merkle_root_hex: None, }; - let first = - interactive_aggregate(aggregate_request.clone()).expect("first interactive aggregate"); + interactive_aggregate(aggregate_request.clone()).expect("first interactive aggregate"); - // The completion record (the aggregate signature) is the only durable - // interactive artifact (live nonce state is gone after restart by - // construction). It must survive a reload so the engine re-emits the - // identical signature instead of forcing a brand-new signing attempt - - // this also exercises the record's persistence round-trip (serialize + - // reload validation). + // The completion marker is the only durable interactive artifact (live + // nonce state is gone after restart by construction). It must survive a + // reload so a replayed aggregate is still rejected - this also exercises + // the marker's persistence round-trip (serialize + reload validation). simulate_process_restart_for_tests(); reload_state_from_storage_for_tests(); - let after_restart = interactive_aggregate(aggregate_request) - .expect("a completed attempt's signature is recoverable across restart"); - assert_eq!(after_restart.signature_hex, first.signature_hex); + let err = interactive_aggregate(aggregate_request) + .expect_err("a completed attempt must stay completed across restart"); + assert!( + matches!(err, EngineError::InteractiveAttemptAlreadyAggregated { .. }), + "unexpected error: {err:?}" + ); + assert_eq!(err.code(), "interactive_attempt_already_aggregated"); reset_for_tests(); cleanup_test_state_artifacts(&state_path); diff --git a/pkg/tbtc/signer/src/errors.rs b/pkg/tbtc/signer/src/errors.rs index 636d9ccf82..34a0671ee7 100644 --- a/pkg/tbtc/signer/src/errors.rs +++ b/pkg/tbtc/signer/src/errors.rs @@ -82,6 +82,18 @@ pub enum EngineError { session_id: String, attempt_id: String, }, + /// Returned when InteractiveAggregate is invoked again for an attempt that + /// already produced an aggregate signature in this session. The per-attempt + /// "aggregated" marker is durable, so a completed attempt stays completed + /// across restart; re-aggregation is rejected rather than recomputed + /// (a lost signature is recovered with a fresh attempt, not by replay). + /// Distinct code so callers match on + /// `interactive_attempt_already_aggregated` rather than the message. + #[error("interactive attempt [{attempt_id}] already aggregated in session [{session_id}]")] + InteractiveAttemptAlreadyAggregated { + session_id: String, + attempt_id: String, + }, #[error("internal error: {0}")] Internal(String), } @@ -104,6 +116,9 @@ impl EngineError { Self::ConsumedAttemptReplay { .. } => "consumed_attempt_replay", Self::ConsumedRoundReplay { .. } => "consumed_round_replay", Self::ConsumedNonceReplay { .. } => "consumed_nonce_replay", + Self::InteractiveAttemptAlreadyAggregated { .. } => { + "interactive_attempt_already_aggregated" + } Self::Internal(_) => "internal_error", } } @@ -128,6 +143,10 @@ impl EngineError { Self::ConsumedAttemptReplay { .. } => "recoverable", Self::ConsumedRoundReplay { .. } => "recoverable", Self::ConsumedNonceReplay { .. } => "recoverable", + // The aggregate is deterministic over public data and the attempt + // is durably marked complete; a re-aggregation request is a benign + // duplicate the caller should not retry, not an engine fault. + Self::InteractiveAttemptAlreadyAggregated { .. } => "recoverable", Self::SessionFinalized { .. } => "terminal", Self::SessionNotFound { .. } => "terminal", Self::Internal(_) => "terminal", @@ -170,6 +189,20 @@ mod tests { ); } + #[test] + fn interactive_attempt_already_aggregated_has_stable_code_and_message_format() { + let err = EngineError::InteractiveAttemptAlreadyAggregated { + session_id: "session-a".to_string(), + attempt_id: "attempt-1".to_string(), + }; + assert_eq!(err.code(), "interactive_attempt_already_aggregated"); + assert_eq!(err.recovery_class(), "recoverable"); + assert_eq!( + err.to_string(), + "interactive attempt [attempt-1] already aggregated in session [session-a]", + ); + } + #[test] fn recovery_class_maps_retryable_and_terminal_errors() { assert_eq!(