fix(tbtc/signer): honor legacy bare aggregate completion markers (re-review)

Codex re-review (P2, valid): the previous commit made the aggregate completion
marker message-bound (attempt_id@digest), but a completion persisted by the
pre-binding engine is stored as the BARE attempt_id. The new checks only looked for
the bound form, so after an upgrade a previously completed attempt looked unfinished
- repeat InteractiveAggregate and the Round2 completion gate would no longer fail
closed for it.

Add interactive_attempt_aggregated(markers, attempt_id, digest) = bound form OR a
legacy bare attempt_id marker (fail-closed on read), mirroring the consumed-marker
helper. Use it at the three read sites (the Round2 completion gate and both
interactive_aggregate pre-checks). New writes stay bound-only, so the durable record
migrates forward on the next completion while legacy completions stay final.

Test: interactive_honors_legacy_bare_aggregate_completion_marker injects a bare
attempt_id marker (a pre-upgrade completion) and asserts Round2 then fails closed
with InteractiveAttemptAlreadyAggregated. All 295 lib tests pass; cargo fmt clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Author: mswilkison

pkg/tbtc/signer/src/engine/interactive.rs

@@ -48,6 +48,22 @@ pub(crate) fn interactive_aggregated_marker(attempt_id: &str, message_digest_hex
     format!("{attempt_id}@{message_digest_hex}")
 }
 
+// Aggregate-completion check honoring legacy markers: the new bound form for THIS
+// message, OR a legacy bare attempt_id marker (written by the pre-binding engine and
+// reloaded from durable state) - the latter fail-closed, exactly like the consumed
+// markers, so a completion persisted before this format change stays final (no repeat
+// aggregate, no fresh Round2 share) after an upgrade.
+pub(crate) fn interactive_attempt_aggregated(
+    markers: &HashSet<String>,
+    attempt_id: &str,
+    message_digest_hex: &str,
+) -> bool {
+    markers.contains(&interactive_aggregated_marker(
+        attempt_id,
+        message_digest_hex,
+    )) || markers.contains(attempt_id)
+}
+
 pub fn interactive_session_open(
     mut request: InteractiveSessionOpenRequest,
 ) -> Result<InteractiveSessionOpenResult, EngineError> {
@@ -499,9 +515,11 @@ pub fn interactive_round2(
     let attempt_finalized = member_attempt_message_digest
         .as_deref()
         .is_some_and(|digest| {
-            session
-                .aggregated_interactive_attempt_markers
-                .contains(&interactive_aggregated_marker(&attempt_id, digest))
+            interactive_attempt_aggregated(
+                &session.aggregated_interactive_attempt_markers,
+                &attempt_id,
+                digest,
+            )
         });
     if attempt_finalized {
         if let Some(mut removed) = session
@@ -706,8 +724,8 @@ pub fn interactive_aggregate(
     // The completion marker binds attempt_id to THIS aggregated message digest, so a
     // valid aggregate over a different message cannot finalize this attempt id (and
     // so cannot, via the Round2 completion gate, preempt an unrelated live attempt).
-    let aggregated_marker =
-        interactive_aggregated_marker(&attempt_id, &hash_hex(signing_package.message().as_slice()));
+    let aggregated_message_digest = hash_hex(signing_package.message().as_slice());
+    let aggregated_marker = interactive_aggregated_marker(&attempt_id, &aggregated_message_digest);
     let signature_shares =
         decode_signature_share_map("InteractiveAggregate", &request.signature_shares)?;
     let mut taproot_merkle_root_hex = request.taproot_merkle_root_hex.clone();
@@ -736,10 +754,11 @@ pub fn interactive_aggregate(
         // 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(&aggregated_marker)
-        {
+        if interactive_attempt_aggregated(
+            &session.aggregated_interactive_attempt_markers,
+            &attempt_id,
+            &aggregated_message_digest,
+        ) {
             return Err(EngineError::InteractiveAttemptAlreadyAggregated {
                 session_id: request.session_id.clone(),
                 attempt_id,
@@ -847,10 +866,11 @@ pub fn interactive_aggregate(
     // 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(&aggregated_marker)
-    {
+    if interactive_attempt_aggregated(
+        &session.aggregated_interactive_attempt_markers,
+        &attempt_id,
+        &aggregated_message_digest,
+    ) {
         return Err(EngineError::InteractiveAttemptAlreadyAggregated {
             session_id: request.session_id.clone(),
             attempt_id,

pkg/tbtc/signer/src/engine/tests.rs

@@ -11934,6 +11934,65 @@ fn interactive_open_advances_only_the_opening_member_attempt() {
     assert!(m2_reopen.idempotent);
 }
 
+#[test]
+fn interactive_honors_legacy_bare_aggregate_completion_marker() {
+    // Backward compat: a completion persisted by the pre-binding engine is the BARE
+    // attempt_id (not attempt_id@digest). After an upgrade it must still finalize the
+    // attempt fail-closed - the Round2 completion gate refuses a fresh share for it.
+    let _guard = lock_test_state();
+    reset_for_tests();
+
+    let key_packages = interactive_test_key_packages();
+    let session_id = "interactive-legacy-aggregate-marker";
+    let key_group = "interactive-test-key-group";
+    let message = [0x66u8; 32];
+    let included = [1u16, 2];
+
+    let opened = open_interactive_for_test(session_id, key_group, &message, &included, 1, 1, 2)
+        .expect("member 1 opens");
+    let c1 = interactive_round1(InteractiveRound1Request {
+        session_id: session_id.to_string(),
+        attempt_id: opened.attempt_id.clone(),
+        member_identifier: 1,
+    })
+    .expect("member 1 round 1");
+
+    // Simulate a completion persisted by the pre-binding engine: the BARE attempt_id.
+    {
+        let mut guard = state().expect("state").lock().expect("lock");
+        guard
+            .sessions
+            .get_mut(session_id)
+            .expect("session")
+            .aggregated_interactive_attempt_markers
+            .insert(opened.attempt_id.clone());
+    }
+
+    // Round2 must treat the bare legacy marker as a completed attempt and fail closed
+    // before any share is released.
+    let package = interactive_package_for_test(
+        &message,
+        vec![NativeFrostCommitment {
+            identifier: key_packages[&1].identifier.clone(),
+            data_hex: c1.commitments_hex.clone(),
+        }],
+    );
+    let refused = interactive_round2(InteractiveRound2Request {
+        session_id: session_id.to_string(),
+        attempt_id: opened.attempt_id.clone(),
+        member_identifier: 1,
+        signing_package_hex: package,
+    })
+    .expect_err("a legacy bare completion marker must finalize the attempt");
+    assert!(
+        matches!(
+            refused,
+            EngineError::InteractiveAttemptAlreadyAggregated { .. }
+        ),
+        "unexpected error: {refused:?}"
+    );
+}
+
 #[test]
 fn interactive_round1_is_idempotent_until_consumed() {
     let _guard = lock_test_state();