fix(tbtc/signer): bind the taproot root into the completion marker (re-review)

Codex re-review (P1, valid): attempt_id is derived from session/message/attempt#/
coordinator/included - NOT the taproot root. So the message-bound completion marker
(attempt_id@message_digest) still collided across taproot tweaks: a completion
aggregated for one root (or key-path None) recorded the same marker the Round2 gate
checks for a live seat opened with a different root, wrongly preempting that seat's
Round2 (and zeroizing it) even though the signatures differ per tweak.

Bind the canonical taproot root into the marker too -
interactive_aggregated_marker(attempt_id, message_digest, taproot_root) =
"{attempt_id}@{message_digest}@{root}" (root = hex, or "keypath" for None, which
cannot collide with a 64-hex root). interactive_aggregate writes it from the root it
aggregated under; the Round2 gate recomputes it from the root THIS member opened
with. The completion marker now binds the full signing-task identity (attempt + msg
+ root); the legacy bare-id fallback is unchanged.

Test interactive_round2_completion_marker_binds_taproot_root: a completion recorded
for a different root does not preempt a key-path member's Round2, while the same-root
completion does. All 296 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

@@ -44,8 +44,17 @@ pub(crate) fn interactive_attempt_consumed(
 // aggregate preempt an unrelated live attempt's Round2 (the Round2 completion gate).
 // interactive_aggregate writes it from the package it aggregated; Round2 and the
 // re-aggregate guard recompute it from the message they actually hold.
-pub(crate) fn interactive_aggregated_marker(attempt_id: &str, message_digest_hex: &str) -> String {
-    format!("{attempt_id}@{message_digest_hex}")
+pub(crate) fn interactive_aggregated_marker(
+    attempt_id: &str,
+    message_digest_hex: &str,
+    taproot_merkle_root: Option<&[u8; 32]>,
+) -> String {
+    // The signature differs per taproot tweak, so the completion is per (message,
+    // root). "keypath" (a key-path / None spend) cannot collide with a 64-hex root.
+    let root = taproot_merkle_root
+        .map(hex::encode)
+        .unwrap_or_else(|| "keypath".to_string());
+    format!("{attempt_id}@{message_digest_hex}@{root}")
 }
 
 // Aggregate-completion check honoring legacy markers: the new bound form for THIS
@@ -57,10 +66,12 @@ pub(crate) fn interactive_attempt_aggregated(
     markers: &HashSet<String>,
     attempt_id: &str,
     message_digest_hex: &str,
+    taproot_merkle_root: Option<&[u8; 32]>,
 ) -> bool {
     markers.contains(&interactive_aggregated_marker(
         attempt_id,
         message_digest_hex,
+        taproot_merkle_root,
     )) || markers.contains(attempt_id)
 }
 
@@ -507,20 +518,22 @@ pub fn interactive_round2(
     // member's live Round2. It fires only for a genuine same-message completion; the
     // member's entry for that finalized attempt is then dead, so free it (zeroizing
     // its nonces) rather than holding a live-member slot until the TTL sweep.
-    let member_attempt_message_digest = session
+    let member_attempt_finalization = session
         .interactive_signing
         .get(&request.member_identifier)
         .filter(|entry| entry.attempt_context.attempt_id == attempt_id)
-        .map(|entry| hash_hex(&entry.message_bytes));
-    let attempt_finalized = member_attempt_message_digest
-        .as_deref()
-        .is_some_and(|digest| {
-            interactive_attempt_aggregated(
-                &session.aggregated_interactive_attempt_markers,
-                &attempt_id,
-                digest,
-            )
-        });
+        .map(|entry| (hash_hex(&entry.message_bytes), entry.taproot_merkle_root));
+    let attempt_finalized =
+        member_attempt_finalization
+            .as_ref()
+            .is_some_and(|(digest, taproot_merkle_root)| {
+                interactive_attempt_aggregated(
+                    &session.aggregated_interactive_attempt_markers,
+                    &attempt_id,
+                    digest,
+                    taproot_merkle_root.as_ref(),
+                )
+            });
     if attempt_finalized {
         if let Some(mut removed) = session
             .interactive_signing
@@ -721,15 +734,20 @@ pub fn interactive_aggregate(
             "InteractiveAggregate: invalid signing package: {e}"
         ))
     })?;
-    // 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_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();
     let taproot_merkle_root = canonicalize_taproot_merkle_root_hex(&mut taproot_merkle_root_hex)?;
+    // The completion marker binds attempt_id to THIS aggregated message digest AND the
+    // canonical taproot root, so a valid aggregate over a different message or root
+    // cannot finalize this attempt id (and so cannot, via the Round2 completion gate,
+    // preempt an unrelated live attempt or root - the signature differs per tweak).
+    let aggregated_message_digest = hash_hex(signing_package.message().as_slice());
+    let aggregated_marker = interactive_aggregated_marker(
+        &attempt_id,
+        &aggregated_message_digest,
+        taproot_merkle_root.as_ref(),
+    );
 
     let mut guard = state()?
         .lock()
@@ -758,6 +776,7 @@ pub fn interactive_aggregate(
             &session.aggregated_interactive_attempt_markers,
             &attempt_id,
             &aggregated_message_digest,
+            taproot_merkle_root.as_ref(),
         ) {
             return Err(EngineError::InteractiveAttemptAlreadyAggregated {
                 session_id: request.session_id.clone(),
@@ -870,6 +889,7 @@ pub fn interactive_aggregate(
         &session.aggregated_interactive_attempt_markers,
         &attempt_id,
         &aggregated_message_digest,
+        taproot_merkle_root.as_ref(),
     ) {
         return Err(EngineError::InteractiveAttemptAlreadyAggregated {
             session_id: request.session_id.clone(),

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

@@ -11993,6 +11993,100 @@ fn interactive_honors_legacy_bare_aggregate_completion_marker() {
     );
 }
 
+#[test]
+fn interactive_round2_completion_marker_binds_taproot_root() {
+    // The completion marker binds the taproot root: a completion recorded for one
+    // root must NOT finalize the same attempt/message for a member opened with a
+    // different root (the signature differs per tweak), else Round2 for the live root
+    // is wrongly preempted.
+    let _guard = lock_test_state();
+    reset_for_tests();
+
+    let key_packages = interactive_test_key_packages();
+    let session_id = "interactive-taproot-root-binding";
+    let key_group = "interactive-test-key-group";
+    let message = [0x44u8; 32];
+    let included = [1u16, 2];
+
+    // Member 1 opens key-path (no taproot root).
+    let opened = open_interactive_for_test(session_id, key_group, &message, &included, 1, 1, 2)
+        .expect("member 1 opens key-path");
+    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");
+
+    let digest = hash_hex(&message);
+    let package = interactive_package_for_test(
+        &message,
+        vec![NativeFrostCommitment {
+            identifier: key_packages[&1].identifier.clone(),
+            data_hex: c1.commitments_hex.clone(),
+        }],
+    );
+
+    // A completion recorded for a DIFFERENT taproot root must not finalize this
+    // member's key-path attempt: Round2 gets past the completion gate (and then fails
+    // on the deliberately sub-threshold package, not as already-aggregated).
+    {
+        let mut guard = state().expect("state").lock().expect("lock");
+        guard
+            .sessions
+            .get_mut(session_id)
+            .expect("session")
+            .aggregated_interactive_attempt_markers
+            .insert(interactive_aggregated_marker(
+                &opened.attempt_id,
+                &digest,
+                Some(&[0x22u8; 32]),
+            ));
+    }
+    let not_preempted = interactive_round2(InteractiveRound2Request {
+        session_id: session_id.to_string(),
+        attempt_id: opened.attempt_id.clone(),
+        member_identifier: 1,
+        signing_package_hex: package.clone(),
+    });
+    assert!(
+        !matches!(
+            not_preempted,
+            Err(EngineError::InteractiveAttemptAlreadyAggregated { .. })
+        ),
+        "a different-root completion must not finalize this attempt: {not_preempted:?}"
+    );
+
+    // A completion for THIS member's root (key-path) does finalize it.
+    {
+        let mut guard = state().expect("state").lock().expect("lock");
+        guard
+            .sessions
+            .get_mut(session_id)
+            .expect("session")
+            .aggregated_interactive_attempt_markers
+            .insert(interactive_aggregated_marker(
+                &opened.attempt_id,
+                &digest,
+                None,
+            ));
+    }
+    let preempted = 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 same-root completion finalizes the attempt");
+    assert!(
+        matches!(
+            preempted,
+            EngineError::InteractiveAttemptAlreadyAggregated { .. }
+        ),
+        "unexpected error: {preempted:?}"
+    );
+}
+
 #[test]
 fn interactive_round1_is_idempotent_until_consumed() {
     let _guard = lock_test_state();