From 2d0a37afac3cf2bd67f57d305f1169d32bbd4037 Mon Sep 17 00:00:00 2001 From: maclane Date: Sun, 14 Jun 2026 23:04:18 -0400 Subject: [PATCH 1/2] feat(tbtc/signer): Phase 7.2b-3 candidate-culprit detection in InteractiveAggregate On a failed aggregate, InteractiveAggregate now names EVERY member whose signature share did not verify, as CANDIDATE culprits, instead of an opaque error. This is the engine-side input to the Go host's envelope-bound blame adjudication (frozen Phase 7.2b spec, section 6). - Aggregate via frost_core::aggregate_custom(.., AllCheaters) instead of the frost-secp256k1-tr wrappers, which hardcode FirstCheater (one culprit). verification_key_package is exactly the (taproot-tweaked) public key package the wrappers derive, so the call is equivalent on the success path; cheater detection only runs after the aggregate signature fails to verify, so there is no happy-path cost. - New EngineError::AggregateShareVerificationFailed{session_id, attempt_id, candidate_culprits} (code aggregate_share_verification_failed, recoverable) carrying AggregateCulprit{member_identifier, reason}. Fail-closed: no signature. - Surfaced across the FFI: ErrorResponse gains an additive, skip-if-empty candidate_culprits field, so existing Go clients are unaffected. - CANDIDATE only: the engine verifies pure FROST shares against the group's own verifying material and never inspects operator-signed envelopes (frozen Q1 boundary). A coordinator that aggregated honest shares against a substituted package/root would make those honest shares appear here; authoritative blame is Go-side at an f+1 accuser quorum. Tests: real-crypto e2e (an invalid share names exactly the cheating member, not the honest one), AllCheaters multi-culprit mapping, and error code/message/recovery_class/accessor. Coarse (non-interactive) Aggregate is out of scope (no attempt context). Co-Authored-By: Claude Opus 4.8 --- pkg/tbtc/signer/Cargo.lock | 1 + pkg/tbtc/signer/Cargo.toml | 4 + pkg/tbtc/signer/src/api.rs | 26 +++++-- pkg/tbtc/signer/src/engine/codec.rs | 33 +++++++++ pkg/tbtc/signer/src/engine/interactive.rs | 64 ++++++++++------ pkg/tbtc/signer/src/engine/mod.rs | 2 +- pkg/tbtc/signer/src/engine/tests.rs | 59 +++++++++++++-- pkg/tbtc/signer/src/errors.rs | 90 ++++++++++++++++++++++- pkg/tbtc/signer/src/ffi.rs | 1 + 9 files changed, 242 insertions(+), 38 deletions(-) diff --git a/pkg/tbtc/signer/Cargo.lock b/pkg/tbtc/signer/Cargo.lock index e77fe4d2f5..6e25f344a8 100644 --- a/pkg/tbtc/signer/Cargo.lock +++ b/pkg/tbtc/signer/Cargo.lock @@ -1326,6 +1326,7 @@ dependencies = [ "bitcoin", "chacha20poly1305", "criterion", + "frost-core", "frost-secp256k1-tr", "hex", "libc", diff --git a/pkg/tbtc/signer/Cargo.toml b/pkg/tbtc/signer/Cargo.toml index 6790fb46a1..cbe97c1b03 100644 --- a/pkg/tbtc/signer/Cargo.toml +++ b/pkg/tbtc/signer/Cargo.toml @@ -20,6 +20,10 @@ sha2 = "0.10" hex = "0.4" thiserror = "2.0" frost-secp256k1-tr = "=3.0.0" +# Direct, version-matched access to aggregate_custom + CheaterDetection (the +# frost-secp256k1-tr aggregate wrappers hardcode FirstCheater). Already a +# transitive dependency via frost-secp256k1-tr, pinned to the same 3.0.0. +frost-core = { version = "=3.0.0", default-features = false } chacha20poly1305 = "0.10" rand_chacha = "0.3" libc = "0.2" diff --git a/pkg/tbtc/signer/src/api.rs b/pkg/tbtc/signer/src/api.rs index 86fc85b344..0b204cf994 100644 --- a/pkg/tbtc/signer/src/api.rs +++ b/pkg/tbtc/signer/src/api.rs @@ -1,5 +1,7 @@ use serde::{Deserialize, Serialize}; +use crate::errors::AggregateCulprit; + #[derive(Clone, Debug, Deserialize, PartialEq, Eq, Serialize)] pub struct DkgParticipant { pub identifier: u16, @@ -219,13 +221,15 @@ pub struct InteractiveAggregateRequest { /// The signing package the shares were produced over (carries the /// message and the chosen subset's commitments). pub signing_package_hex: String, - /// The collected signature shares from the responsive subset. Each - /// is verified against the member's verifying share (resolved from - /// the session's DKG public key package) before aggregation; an - /// invalid share fails the call closed with `validation_error` and - /// no signature. Per-member attributable blame (a culprit list) is - /// deferred to Phase 7.2b, where the signed-package envelopes bind - /// what each member signed and make the attribution unforgeable. + /// The collected signature shares from the responsive subset. Each is + /// verified against the member's verifying share (resolved from the + /// session's DKG public key package) before aggregation. If any share fails, + /// the call fails closed with no signature and the + /// `aggregate_share_verification_failed` error, which carries the CANDIDATE + /// culprits - every member whose share failed (Phase 7.2b-3). These are + /// pure-crypto candidates for the Go host's envelope-bound blame + /// adjudication (frozen Phase 7.2b spec, section 6); the engine never + /// inspects operator-signed envelopes itself. pub signature_shares: Vec, #[serde(default, skip_serializing_if = "Option::is_none")] pub taproot_merkle_root_hex: Option, @@ -669,6 +673,14 @@ pub struct ErrorResponse { pub code: String, pub message: String, pub recovery_class: String, + /// CANDIDATE culprits for an `aggregate_share_verification_failed` error: + /// the members whose FROST signature shares failed verification. Empty - and + /// omitted from the JSON via skip_serializing_if - for every other error, so + /// existing Go clients that do not read the field are unaffected. These are + /// pure-crypto candidates, not adjudicated blame; the Go host performs the + /// envelope-bound adjudication (frozen Phase 7.2b spec, section 6). + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub candidate_culprits: Vec, } /// Init-time signer configuration installed once by the host over FFI. diff --git a/pkg/tbtc/signer/src/engine/codec.rs b/pkg/tbtc/signer/src/engine/codec.rs index 83fc94a2d8..77eabd08ec 100644 --- a/pkg/tbtc/signer/src/engine/codec.rs +++ b/pkg/tbtc/signer/src/engine/codec.rs @@ -53,6 +53,39 @@ pub(crate) fn frost_identifier_to_go_string(identifier: frost::Identifier) -> St .expect("serializing hex identifier as JSON string cannot fail") } +/// Map a FROST aggregate error to the CANDIDATE culprits it identifies. +/// +/// Returns the participant identifiers FROST flagged for an invalid signature +/// share (`Error::InvalidSignatureShare`, populated with the full set when +/// aggregation uses `CheaterDetection::AllCheaters`). Every other error class - +/// malformed package, wrong share count, group/field errors - yields an empty +/// list: those are not per-member share attributions, so the caller surfaces +/// them as a generic validation failure instead. The identifiers are CANDIDATES +/// only - pure FROST verification verdicts, not adjudicated fault. +pub(crate) fn aggregate_candidate_culprits(error: &frost::Error) -> Vec { + match error { + frost_core::Error::InvalidSignatureShare { culprits } => { + candidate_culprits_from_identifiers(culprits) + } + _ => Vec::new(), + } +} + +/// Map FROST participant identifiers to CANDIDATE culprit records, tagging each +/// with the `invalid_signature_share` reason and the canonical Go-string member +/// identifier the rest of the engine uses. +pub(crate) fn candidate_culprits_from_identifiers( + identifiers: &[frost::Identifier], +) -> Vec { + identifiers + .iter() + .map(|identifier| AggregateCulprit { + member_identifier: frost_identifier_to_go_string(*identifier), + reason: "invalid_signature_share".to_string(), + }) + .collect() +} + pub(crate) fn parse_frost_identifier( operation: &str, field_name: &str, diff --git a/pkg/tbtc/signer/src/engine/interactive.rs b/pkg/tbtc/signer/src/engine/interactive.rs index ebe932cda5..a16065dcc3 100644 --- a/pkg/tbtc/signer/src/engine/interactive.rs +++ b/pkg/tbtc/signer/src/engine/interactive.rs @@ -653,34 +653,54 @@ pub fn interactive_aggregate( // step is each signer's Round2, where lifecycle/quarantine/firewall // were already enforced (including the full-subset quarantine check). // - // frost verifies every share and can name which failed, but this path - // does NOT surface those as attributable member blame: the engine - // cannot yet bind these public inputs (signing package, taproot root) - // to what each member actually signed at Round2, so a coordinator - // aggregating against a different package/root would make honest - // shares fail and frame their members. Attributable blame waits for - // the signed-package envelopes (Phase 7.2b, frozen spec section 6), - // which prove what each member signed. Until then a verification - // failure is a generic fail-closed error: no signature, no blame. + // frost verifies every share and names which failed. This path now surfaces + // those as CANDIDATE culprits (Phase 7.2b-3): the engine reports the members + // whose shares did not verify against the group's own verifying material, + // but it does NOT adjudicate fault. The engine cannot bind these public + // inputs (signing package, taproot root) to what each member signed at + // Round2, so a coordinator that aggregated honest shares against a + // substituted package/root would make those honest shares fail and appear + // here. Authoritative, envelope-bound blame is the Go host's job at an f+1 + // accuser quorum (frozen Phase 7.2b spec, section 6), using the signed + // signing-package envelopes; this candidate list is its input. Fail-closed + // either way: no signature leaves on a verification failure. let verification_key_package = match taproot_merkle_root.as_ref() { Some(root) => public_key_package.clone().tweak(Some(root.as_slice())), None => public_key_package.clone(), }; - let aggregate_result = match taproot_merkle_root.as_ref() { - Some(root) => frost::aggregate_with_tweak( - &signing_package, - &signature_shares, - &public_key_package, - Some(root.as_slice()), - ), - None => frost::aggregate(&signing_package, &signature_shares, &public_key_package), + // Aggregate with AllCheaters detection. The frost-secp256k1-tr + // aggregate/aggregate_with_tweak wrappers hardcode FirstCheater, so a + // failure would name only one member; AllCheaters names EVERY member whose + // share failed. verification_key_package is the (taproot-tweaked, when a + // root is set) public key package - exactly what aggregate_with_tweak + // derives internally - so this is equivalent to those wrappers on the + // success path. Cheater detection only runs after the aggregate signature + // itself fails to verify, so there is no happy-path cost. + let signature = match frost_core::aggregate_custom( + &signing_package, + &signature_shares, + &verification_key_package, + frost_core::CheaterDetection::AllCheaters, + ) { + Ok(signature) => signature, + Err(error) => { + let candidate_culprits = aggregate_candidate_culprits(&error); + if candidate_culprits.is_empty() { + // Not a per-member share attribution (malformed package, wrong + // share count, group/field error): fail closed with the generic + // validation error, no blame. + return Err(EngineError::Validation(format!( + "InteractiveAggregate: failed to aggregate: {error}" + ))); + } + return Err(EngineError::AggregateShareVerificationFailed { + session_id: request.session_id.clone(), + attempt_id, + candidate_culprits, + }); + } }; - let signature = aggregate_result.map_err(|error| { - EngineError::Validation(format!( - "InteractiveAggregate: failed to aggregate: {error}" - )) - })?; // Self-verify the aggregate against the (tweaked) group verifying // key before releasing it, matching the coarse finalize path. diff --git a/pkg/tbtc/signer/src/engine/mod.rs b/pkg/tbtc/signer/src/engine/mod.rs index ffa4d695f9..009deddd67 100644 --- a/pkg/tbtc/signer/src/engine/mod.rs +++ b/pkg/tbtc/signer/src/engine/mod.rs @@ -83,7 +83,7 @@ use crate::api::{ TranscriptAuditResult, TriggerEmergencyRekeyRequest, TriggerEmergencyRekeyResult, VerifyBlameProofRequest, }; -use crate::errors::EngineError; +use crate::errors::{AggregateCulprit, EngineError}; use crate::go_math_rand::select_coordinator_identifier; mod audit; diff --git a/pkg/tbtc/signer/src/engine/tests.rs b/pkg/tbtc/signer/src/engine/tests.rs index 60f8a2d2cb..7095aae9c1 100644 --- a/pkg/tbtc/signer/src/engine/tests.rs +++ b/pkg/tbtc/signer/src/engine/tests.rs @@ -13120,17 +13120,62 @@ fn interactive_aggregate_rejects_invalid_share_fail_closed() { taproot_merkle_root_hex: None, }) .expect_err("an invalid share must fail aggregation closed"); - // 7.2a fails closed without attributable member blame: the engine - // cannot yet bind the aggregate inputs to what each member signed - // (that needs the Phase 7.2b signed-package envelopes), so a - // verification failure is a generic error - no signature, and no - // culprit naming that a wrong-package/root coordinator could forge. + // 7.2b-3: the aggregate now fails closed WITH attributable CANDIDATE blame. + // Member 2 submitted a structurally valid share over a different package, so + // its share fails verification against the group's verifying material and is + // named a candidate culprit; member 1's honest share is not. The engine + // surfaces candidates only - envelope-bound adjudication is the Go host's + // job (frozen Phase 7.2b spec, section 6). + let candidate_culprits = match err { + EngineError::AggregateShareVerificationFailed { + ref candidate_culprits, + .. + } => candidate_culprits.clone(), + other => panic!("expected AggregateShareVerificationFailed, got {other:?}"), + }; assert!( - matches!(err, EngineError::Validation(ref m) if m.contains("failed to aggregate")), - "unexpected error: {err:?}" + candidate_culprits + .iter() + .any(|c| c.member_identifier == key_packages[&2].identifier), + "member 2 must be named a candidate culprit: {candidate_culprits:?}" + ); + assert!( + !candidate_culprits + .iter() + .any(|c| c.member_identifier == key_packages[&1].identifier), + "honest member 1 must not be blamed: {candidate_culprits:?}" + ); + assert!( + candidate_culprits + .iter() + .all(|c| c.reason == "invalid_signature_share"), + "{candidate_culprits:?}" ); } +#[test] +fn candidate_culprits_from_identifiers_maps_each_member() { + // The AllCheaters mapping must surface EVERY flagged member (not just the + // first), each tagged with the stable reason and the same canonical + // Go-string identifier the DKG path emits. + let id2 = participant_identifier_to_frost_identifier(2).expect("identifier 2"); + let id3 = participant_identifier_to_frost_identifier(3).expect("identifier 3"); + let culprits = candidate_culprits_from_identifiers(&[id2, id3]); + assert_eq!(culprits.len(), 2); + assert_eq!( + culprits[0].member_identifier, + frost_identifier_to_go_string(id2) + ); + assert_eq!( + culprits[1].member_identifier, + frost_identifier_to_go_string(id3) + ); + assert!(culprits + .iter() + .all(|c| c.reason == "invalid_signature_share")); + assert!(candidate_culprits_from_identifiers(&[]).is_empty()); +} + #[test] fn interactive_aggregate_sweeps_expired_sessions() { let _guard = lock_test_state(); diff --git a/pkg/tbtc/signer/src/errors.rs b/pkg/tbtc/signer/src/errors.rs index 34a0671ee7..9ac391b141 100644 --- a/pkg/tbtc/signer/src/errors.rs +++ b/pkg/tbtc/signer/src/errors.rs @@ -1,5 +1,25 @@ +use serde::{Deserialize, Serialize}; use thiserror::Error; +/// A single CANDIDATE culprit surfaced by InteractiveAggregate when a signature +/// share fails verification. `member_identifier` is the FROST participant +/// identifier in the engine's canonical Go-string form (see +/// `frost_identifier_to_go_string`); `reason` is a stable machine-readable code +/// for the failure class. +/// +/// CANDIDATE, not verdict: the engine verifies pure FROST shares against the +/// group's own verifying material and never inspects operator-signed envelopes +/// (frozen Q1 boundary). A coordinator that aggregated honest shares against a +/// substituted signing package or taproot root would make those honest shares +/// fail and appear here. Authoritative, envelope-bound blame is adjudicated by +/// the Go host at an f+1 accuser quorum (frozen Phase 7.2b spec, section 6); +/// this is its input, not its conclusion. +#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] +pub struct AggregateCulprit { + pub member_identifier: String, + pub reason: String, +} + #[derive(Debug, Error)] pub enum EngineError { #[error("validation failed: {0}")] @@ -94,6 +114,24 @@ pub enum EngineError { session_id: String, attempt_id: String, }, + /// Returned when InteractiveAggregate fails because one or more signature + /// shares did not verify against the (tweaked) group verifying material. + /// Unlike the generic `Validation` failure, this carries the FROST-identified + /// CANDIDATE culprits - every member whose share failed, via + /// `CheaterDetection::AllCheaters` - so the Go host can feed them into + /// envelope-bound blame adjudication. The engine never adjudicates fault + /// itself (see `AggregateCulprit`). Fail-closed: no signature is produced. + /// Distinct code so callers match on `aggregate_share_verification_failed` + /// rather than the message. + #[error( + "InteractiveAggregate: {} signature share(s) failed verification for attempt [{attempt_id}] in session [{session_id}]", + candidate_culprits.len() + )] + AggregateShareVerificationFailed { + session_id: String, + attempt_id: String, + candidate_culprits: Vec, + }, #[error("internal error: {0}")] Internal(String), } @@ -119,6 +157,7 @@ impl EngineError { Self::InteractiveAttemptAlreadyAggregated { .. } => { "interactive_attempt_already_aggregated" } + Self::AggregateShareVerificationFailed { .. } => "aggregate_share_verification_failed", Self::Internal(_) => "internal_error", } } @@ -147,16 +186,33 @@ impl EngineError { // is durably marked complete; a re-aggregation request is a benign // duplicate the caller should not retry, not an engine fault. Self::InteractiveAttemptAlreadyAggregated { .. } => "recoverable", + // A fresh attempt that excludes the candidate culprits can still + // produce a signature, so this is recoverable: the caller mints a + // new attempt after the Go host adjudicates blame. + Self::AggregateShareVerificationFailed { .. } => "recoverable", Self::SessionFinalized { .. } => "terminal", Self::SessionNotFound { .. } => "terminal", Self::Internal(_) => "terminal", } } + + /// The CANDIDATE culprits carried by this error. Non-empty only for + /// `AggregateShareVerificationFailed`; empty for every other variant. The + /// FFI layer uses this to surface the list to the Go host without matching + /// the variant inline. + pub fn candidate_culprits(&self) -> &[AggregateCulprit] { + match self { + Self::AggregateShareVerificationFailed { + candidate_culprits, .. + } => candidate_culprits, + _ => &[], + } + } } #[cfg(test)] mod tests { - use super::EngineError; + use super::{AggregateCulprit, EngineError}; #[test] fn consumed_attempt_replay_has_stable_code_and_message_format() { @@ -245,4 +301,36 @@ mod tests { "terminal" ); } + + #[test] + fn aggregate_share_verification_failed_code_message_and_culprits() { + let err = EngineError::AggregateShareVerificationFailed { + session_id: "session-a".to_string(), + attempt_id: "attempt-1".to_string(), + candidate_culprits: vec![AggregateCulprit { + member_identifier: + "\"0200000000000000000000000000000000000000000000000000000000000000\"" + .to_string(), + reason: "invalid_signature_share".to_string(), + }], + }; + assert_eq!(err.code(), "aggregate_share_verification_failed"); + assert_eq!(err.recovery_class(), "recoverable"); + // The count is rendered; the culprit identifiers are not (they travel in + // the structured candidate_culprits list, not the message string). + assert_eq!( + err.to_string(), + "InteractiveAggregate: 1 signature share(s) failed verification for attempt [attempt-1] in session [session-a]", + ); + assert_eq!(err.candidate_culprits().len(), 1); + assert_eq!( + err.candidate_culprits()[0].reason, + "invalid_signature_share" + ); + + // Every non-aggregate error exposes no culprits. + assert!(EngineError::Validation("x".to_string()) + .candidate_culprits() + .is_empty()); + } } diff --git a/pkg/tbtc/signer/src/ffi.rs b/pkg/tbtc/signer/src/ffi.rs index e52a2e5723..95eb840cd8 100644 --- a/pkg/tbtc/signer/src/ffi.rs +++ b/pkg/tbtc/signer/src/ffi.rs @@ -72,6 +72,7 @@ fn error_result(error: EngineError) -> TbtcSignerResult { code: error.code().to_string(), message: error.to_string(), recovery_class: error.recovery_class().to_string(), + candidate_culprits: error.candidate_culprits().to_vec(), }; let bytes = serde_json::to_vec(&payload).unwrap_or_else(|_| { From b8407f6b3a6c8d6e67aeef0dbf0565a3d9b76930 Mon Sep 17 00:00:00 2001 From: maclane Date: Mon, 15 Jun 2026 10:07:12 -0400 Subject: [PATCH 2/2] fixup(tbtc/signer): align candidate culprits to u16 member ids + multi-culprit e2e Review folding for PR #4062: - Codex [P2]: surface culprits as u16 Go member identifiers - the same space as excluded_member_identifiers / included_participants the Go host already keys on - not FROST go-string identifiers (which the engine reserves for raw frost-protocol artifacts fed back into frost). Drop AggregateCulprit; the error variant and ErrorResponse now carry candidate_culprits: Vec, mirroring excluded_member_identifiers (skip_serializing_if). Add frost_identifier_to_u16, the inverse of participant_identifier_to_frost_identifier (big-endian scalar). Foreign (non-u16) identifiers are dropped: they cannot be real group members. (Codex's cited spec does not exist in-repo, but the u16 convention is dominant and correct - AttemptExclusionEvidence is the precedent.) - Gemini [P3]: add interactive_aggregate_names_all_invalid_share_culprits, a real-crypto e2e where both members of a threshold-2 subset cheat and the aggregate names BOTH - proving AllCheaters end to end, not just the mapping. - The per-culprit reason field is dropped: the error code (aggregate_share_verification_failed) already names the reason, matching the AttemptExclusionEvidence idiom (struct-level reason + Vec members). Co-Authored-By: Claude Opus 4.8 --- pkg/tbtc/signer/src/api.rs | 15 ++- pkg/tbtc/signer/src/engine/codec.rs | 54 ++++----- pkg/tbtc/signer/src/engine/mod.rs | 2 +- pkg/tbtc/signer/src/engine/tests.rs | 164 +++++++++++++++++++++------- pkg/tbtc/signer/src/errors.rs | 61 +++-------- 5 files changed, 178 insertions(+), 118 deletions(-) diff --git a/pkg/tbtc/signer/src/api.rs b/pkg/tbtc/signer/src/api.rs index 0b204cf994..4a87f7bfd4 100644 --- a/pkg/tbtc/signer/src/api.rs +++ b/pkg/tbtc/signer/src/api.rs @@ -1,7 +1,5 @@ use serde::{Deserialize, Serialize}; -use crate::errors::AggregateCulprit; - #[derive(Clone, Debug, Deserialize, PartialEq, Eq, Serialize)] pub struct DkgParticipant { pub identifier: u16, @@ -674,13 +672,14 @@ pub struct ErrorResponse { pub message: String, pub recovery_class: String, /// CANDIDATE culprits for an `aggregate_share_verification_failed` error: - /// the members whose FROST signature shares failed verification. Empty - and - /// omitted from the JSON via skip_serializing_if - for every other error, so - /// existing Go clients that do not read the field are unaffected. These are - /// pure-crypto candidates, not adjudicated blame; the Go host performs the - /// envelope-bound adjudication (frozen Phase 7.2b spec, section 6). + /// the u16 Go member identifiers whose FROST signature shares failed + /// verification (the same identifier space as `excluded_member_identifiers`). + /// Empty - and omitted from the JSON via skip_serializing_if - for every + /// other error, so existing Go clients are unaffected. These are pure-crypto + /// candidates, not adjudicated blame; the Go host performs the envelope-bound + /// adjudication (frozen Phase 7.2b spec, section 6). #[serde(default, skip_serializing_if = "Vec::is_empty")] - pub candidate_culprits: Vec, + pub candidate_culprits: Vec, } /// Init-time signer configuration installed once by the host over FFI. diff --git a/pkg/tbtc/signer/src/engine/codec.rs b/pkg/tbtc/signer/src/engine/codec.rs index 77eabd08ec..68d41f4685 100644 --- a/pkg/tbtc/signer/src/engine/codec.rs +++ b/pkg/tbtc/signer/src/engine/codec.rs @@ -53,37 +53,41 @@ pub(crate) fn frost_identifier_to_go_string(identifier: frost::Identifier) -> St .expect("serializing hex identifier as JSON string cannot fail") } -/// Map a FROST aggregate error to the CANDIDATE culprits it identifies. +/// Map a FROST aggregate error to the CANDIDATE culprits it identifies, as u16 +/// Go member identifiers (the same identifier space as +/// `excluded_member_identifiers`, so the Go host consumes them directly). /// -/// Returns the participant identifiers FROST flagged for an invalid signature -/// share (`Error::InvalidSignatureShare`, populated with the full set when -/// aggregation uses `CheaterDetection::AllCheaters`). Every other error class - -/// malformed package, wrong share count, group/field errors - yields an empty -/// list: those are not per-member share attributions, so the caller surfaces -/// them as a generic validation failure instead. The identifiers are CANDIDATES -/// only - pure FROST verification verdicts, not adjudicated fault. -pub(crate) fn aggregate_candidate_culprits(error: &frost::Error) -> Vec { +/// Returns the members FROST flagged for an invalid signature share +/// (`Error::InvalidSignatureShare`, the full set under +/// `CheaterDetection::AllCheaters`). Every other error class - malformed +/// package, wrong share count, group/field errors - yields an empty list: those +/// are not per-member share attributions, so the caller surfaces them as a +/// generic validation failure instead. Identifiers that do not map to a u16 are +/// dropped: they cannot belong to a real group member (every submitted share +/// carries a u16-derived identifier), so they are foreign to the Go host's +/// member set. CANDIDATES only - pure FROST verdicts, not adjudicated fault. +pub(crate) fn aggregate_candidate_culprits(error: &frost::Error) -> Vec { match error { - frost_core::Error::InvalidSignatureShare { culprits } => { - candidate_culprits_from_identifiers(culprits) - } + frost_core::Error::InvalidSignatureShare { culprits } => culprits + .iter() + .filter_map(|identifier| frost_identifier_to_u16(*identifier)) + .collect(), _ => Vec::new(), } } -/// Map FROST participant identifiers to CANDIDATE culprit records, tagging each -/// with the `invalid_signature_share` reason and the canonical Go-string member -/// identifier the rest of the engine uses. -pub(crate) fn candidate_culprits_from_identifiers( - identifiers: &[frost::Identifier], -) -> Vec { - identifiers - .iter() - .map(|identifier| AggregateCulprit { - member_identifier: frost_identifier_to_go_string(*identifier), - reason: "invalid_signature_share".to_string(), - }) - .collect() +/// Recover the u16 Go member identifier from a FROST participant identifier - +/// the inverse of `participant_identifier_to_frost_identifier`. FROST(secp256k1) +/// serializes the scalar big-endian, so this requires every byte above the low +/// two to be zero and reads the trailing two big-endian. Returns None for an +/// identifier that does not fit a u16. +pub(crate) fn frost_identifier_to_u16(identifier: frost::Identifier) -> Option { + let bytes = identifier.serialize(); + let split = bytes.len().checked_sub(2)?; + if bytes[..split].iter().any(|&b| b != 0) { + return None; + } + Some(u16::from_be_bytes([bytes[split], bytes[split + 1]])) } pub(crate) fn parse_frost_identifier( diff --git a/pkg/tbtc/signer/src/engine/mod.rs b/pkg/tbtc/signer/src/engine/mod.rs index 009deddd67..ffa4d695f9 100644 --- a/pkg/tbtc/signer/src/engine/mod.rs +++ b/pkg/tbtc/signer/src/engine/mod.rs @@ -83,7 +83,7 @@ use crate::api::{ TranscriptAuditResult, TriggerEmergencyRekeyRequest, TriggerEmergencyRekeyResult, VerifyBlameProofRequest, }; -use crate::errors::{AggregateCulprit, EngineError}; +use crate::errors::EngineError; use crate::go_math_rand::select_coordinator_identifier; mod audit; diff --git a/pkg/tbtc/signer/src/engine/tests.rs b/pkg/tbtc/signer/src/engine/tests.rs index 7095aae9c1..4c05783a82 100644 --- a/pkg/tbtc/signer/src/engine/tests.rs +++ b/pkg/tbtc/signer/src/engine/tests.rs @@ -13123,57 +13123,139 @@ fn interactive_aggregate_rejects_invalid_share_fail_closed() { // 7.2b-3: the aggregate now fails closed WITH attributable CANDIDATE blame. // Member 2 submitted a structurally valid share over a different package, so // its share fails verification against the group's verifying material and is - // named a candidate culprit; member 1's honest share is not. The engine - // surfaces candidates only - envelope-bound adjudication is the Go host's - // job (frozen Phase 7.2b spec, section 6). + // named a candidate culprit (its u16 Go member id); member 1's honest share + // is not. The engine surfaces candidates only - envelope-bound adjudication + // is the Go host's job (frozen Phase 7.2b spec, section 6). let candidate_culprits = match err { EngineError::AggregateShareVerificationFailed { - ref candidate_culprits, - .. - } => candidate_culprits.clone(), + candidate_culprits, .. + } => candidate_culprits, other => panic!("expected AggregateShareVerificationFailed, got {other:?}"), }; - assert!( - candidate_culprits - .iter() - .any(|c| c.member_identifier == key_packages[&2].identifier), - "member 2 must be named a candidate culprit: {candidate_culprits:?}" - ); - assert!( - !candidate_culprits - .iter() - .any(|c| c.member_identifier == key_packages[&1].identifier), - "honest member 1 must not be blamed: {candidate_culprits:?}" - ); - assert!( - candidate_culprits - .iter() - .all(|c| c.reason == "invalid_signature_share"), - "{candidate_culprits:?}" + assert_eq!( + candidate_culprits, + vec![2], + "only the cheating member 2 must be named: {candidate_culprits:?}" ); } #[test] -fn candidate_culprits_from_identifiers_maps_each_member() { - // The AllCheaters mapping must surface EVERY flagged member (not just the - // first), each tagged with the stable reason and the same canonical - // Go-string identifier the DKG path emits. - let id2 = participant_identifier_to_frost_identifier(2).expect("identifier 2"); - let id3 = participant_identifier_to_frost_identifier(3).expect("identifier 3"); - let culprits = candidate_culprits_from_identifiers(&[id2, id3]); - assert_eq!(culprits.len(), 2); - assert_eq!( - culprits[0].member_identifier, - frost_identifier_to_go_string(id2) +fn frost_identifier_to_u16_inverts_participant_mapping() { + // The culprit list reports u16 Go member identifiers, so the inverse of + // participant_identifier_to_frost_identifier must round-trip - including + // across the low/high byte boundary (255 -> 256). + for id in [1u16, 2, 3, 255, 256, 65535] { + let identifier = participant_identifier_to_frost_identifier(id).expect("identifier"); + assert_eq!(frost_identifier_to_u16(identifier), Some(id), "id {id}"); + } +} + +#[test] +fn interactive_aggregate_names_all_invalid_share_culprits() { + let _guard = lock_test_state(); + reset_for_tests(); + + let session_id = "interactive-aggregate-multi-blame"; + let key_group = "interactive-test-key-group"; + let message = [0x5au8; 32]; + let included = [1u16, 2]; + let key_packages = ensure_interactive_dkg_session(session_id, key_group); + + // Both members of the threshold-2 signing subset cheat: each signs a + // DIFFERENT package, so both shares fail verification against the + // authoritative package. Aggregation must name BOTH (AllCheaters), not just + // the first cheater. (The signing package carries exactly `threshold` + // commitments, so a multi-culprit case needs every subset member to cheat.) + let opened = open_interactive_for_test(session_id, key_group, &message, &included, 1, 1, 2) + .expect("opens"); + + let real1 = generate_nonces_and_commitments(GenerateNoncesAndCommitmentsRequest { + key_package_identifier: key_packages[&1].identifier.clone(), + key_package_hex: key_packages[&1].data_hex.clone(), + }) + .expect("member 1 nonces"); + let real2 = 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![real1.commitment.clone(), real2.commitment.clone()], ); - assert_eq!( - culprits[1].member_identifier, - frost_identifier_to_go_string(id3) + + // Each member signs a different (2-party) package over another message, so + // both shares fail verification against the authoritative package. + let other_message = [0x5bu8; 32]; + let bogus1 = generate_nonces_and_commitments(GenerateNoncesAndCommitmentsRequest { + key_package_identifier: key_packages[&1].identifier.clone(), + key_package_hex: key_packages[&1].data_hex.clone(), + }) + .expect("bogus member 1 nonces"); + let bogus1_package = interactive_package_for_test( + &other_message, + vec![ + NativeFrostCommitment { + identifier: key_packages[&1].identifier.clone(), + data_hex: bogus1.commitment.data_hex.clone(), + }, + NativeFrostCommitment { + identifier: key_packages[&2].identifier.clone(), + data_hex: bogus1.commitment.data_hex.clone(), + }, + ], ); - assert!(culprits - .iter() - .all(|c| c.reason == "invalid_signature_share")); - assert!(candidate_culprits_from_identifiers(&[]).is_empty()); + let bogus1_share = sign_share(SignShareRequest { + signing_package_hex: bogus1_package, + nonces_hex: bogus1.nonces_hex, + key_package_identifier: key_packages[&1].identifier.clone(), + key_package_hex: key_packages[&1].data_hex.clone(), + }) + .expect("bogus member 1 share"); + let bogus2 = generate_nonces_and_commitments(GenerateNoncesAndCommitmentsRequest { + key_package_identifier: key_packages[&2].identifier.clone(), + key_package_hex: key_packages[&2].data_hex.clone(), + }) + .expect("bogus member 2 nonces"); + let bogus2_package = interactive_package_for_test( + &other_message, + vec![ + NativeFrostCommitment { + identifier: key_packages[&2].identifier.clone(), + data_hex: bogus2.commitment.data_hex.clone(), + }, + NativeFrostCommitment { + identifier: key_packages[&1].identifier.clone(), + data_hex: bogus2.commitment.data_hex.clone(), + }, + ], + ); + let bogus2_share = sign_share(SignShareRequest { + signing_package_hex: bogus2_package, + nonces_hex: bogus2.nonces_hex, + key_package_identifier: key_packages[&2].identifier.clone(), + key_package_hex: key_packages[&2].data_hex.clone(), + }) + .expect("bogus member 2 share"); + + let err = interactive_aggregate(InteractiveAggregateRequest { + session_id: session_id.to_string(), + attempt_id: opened.attempt_id.clone(), + signing_package_hex, + signature_shares: vec![bogus1_share.signature_share, bogus2_share.signature_share], + taproot_merkle_root_hex: None, + }) + .expect_err("two invalid shares must fail aggregation closed"); + + let mut candidate_culprits = match err { + EngineError::AggregateShareVerificationFailed { + candidate_culprits, .. + } => candidate_culprits, + other => panic!("expected AggregateShareVerificationFailed, got {other:?}"), + }; + candidate_culprits.sort_unstable(); + // AllCheaters, not FirstCheater: BOTH cheating members are named. + assert_eq!(candidate_culprits, vec![1, 2], "{candidate_culprits:?}"); } #[test] diff --git a/pkg/tbtc/signer/src/errors.rs b/pkg/tbtc/signer/src/errors.rs index 9ac391b141..8cfe342ac1 100644 --- a/pkg/tbtc/signer/src/errors.rs +++ b/pkg/tbtc/signer/src/errors.rs @@ -1,25 +1,5 @@ -use serde::{Deserialize, Serialize}; use thiserror::Error; -/// A single CANDIDATE culprit surfaced by InteractiveAggregate when a signature -/// share fails verification. `member_identifier` is the FROST participant -/// identifier in the engine's canonical Go-string form (see -/// `frost_identifier_to_go_string`); `reason` is a stable machine-readable code -/// for the failure class. -/// -/// CANDIDATE, not verdict: the engine verifies pure FROST shares against the -/// group's own verifying material and never inspects operator-signed envelopes -/// (frozen Q1 boundary). A coordinator that aggregated honest shares against a -/// substituted signing package or taproot root would make those honest shares -/// fail and appear here. Authoritative, envelope-bound blame is adjudicated by -/// the Go host at an f+1 accuser quorum (frozen Phase 7.2b spec, section 6); -/// this is its input, not its conclusion. -#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] -pub struct AggregateCulprit { - pub member_identifier: String, - pub reason: String, -} - #[derive(Debug, Error)] pub enum EngineError { #[error("validation failed: {0}")] @@ -117,12 +97,16 @@ pub enum EngineError { /// Returned when InteractiveAggregate fails because one or more signature /// shares did not verify against the (tweaked) group verifying material. /// Unlike the generic `Validation` failure, this carries the FROST-identified - /// CANDIDATE culprits - every member whose share failed, via - /// `CheaterDetection::AllCheaters` - so the Go host can feed them into - /// envelope-bound blame adjudication. The engine never adjudicates fault - /// itself (see `AggregateCulprit`). Fail-closed: no signature is produced. - /// Distinct code so callers match on `aggregate_share_verification_failed` - /// rather than the message. + /// CANDIDATE culprits as u16 Go member identifiers - every member whose share + /// failed, via `CheaterDetection::AllCheaters` - so the Go host can feed them + /// into envelope-bound blame adjudication. CANDIDATES, not a verdict: the + /// engine verifies pure FROST shares against the group's own verifying + /// material and never inspects operator-signed envelopes (frozen Q1 + /// boundary); a coordinator that aggregated honest shares against a + /// substituted package or root would make those honest shares appear here. + /// Authoritative blame is the Go host's at an f+1 accuser quorum. Fail-closed: + /// no signature is produced. Distinct code so callers match on + /// `aggregate_share_verification_failed` rather than the message. #[error( "InteractiveAggregate: {} signature share(s) failed verification for attempt [{attempt_id}] in session [{session_id}]", candidate_culprits.len() @@ -130,7 +114,7 @@ pub enum EngineError { AggregateShareVerificationFailed { session_id: String, attempt_id: String, - candidate_culprits: Vec, + candidate_culprits: Vec, }, #[error("internal error: {0}")] Internal(String), @@ -200,7 +184,7 @@ impl EngineError { /// `AggregateShareVerificationFailed`; empty for every other variant. The /// FFI layer uses this to surface the list to the Go host without matching /// the variant inline. - pub fn candidate_culprits(&self) -> &[AggregateCulprit] { + pub fn candidate_culprits(&self) -> &[u16] { match self { Self::AggregateShareVerificationFailed { candidate_culprits, .. @@ -212,7 +196,7 @@ impl EngineError { #[cfg(test)] mod tests { - use super::{AggregateCulprit, EngineError}; + use super::EngineError; #[test] fn consumed_attempt_replay_has_stable_code_and_message_format() { @@ -307,26 +291,17 @@ mod tests { let err = EngineError::AggregateShareVerificationFailed { session_id: "session-a".to_string(), attempt_id: "attempt-1".to_string(), - candidate_culprits: vec![AggregateCulprit { - member_identifier: - "\"0200000000000000000000000000000000000000000000000000000000000000\"" - .to_string(), - reason: "invalid_signature_share".to_string(), - }], + candidate_culprits: vec![2, 3], }; assert_eq!(err.code(), "aggregate_share_verification_failed"); assert_eq!(err.recovery_class(), "recoverable"); - // The count is rendered; the culprit identifiers are not (they travel in - // the structured candidate_culprits list, not the message string). + // The count is rendered; the member identifiers travel in the structured + // candidate_culprits list, not the message string. assert_eq!( err.to_string(), - "InteractiveAggregate: 1 signature share(s) failed verification for attempt [attempt-1] in session [session-a]", - ); - assert_eq!(err.candidate_culprits().len(), 1); - assert_eq!( - err.candidate_culprits()[0].reason, - "invalid_signature_share" + "InteractiveAggregate: 2 signature share(s) failed verification for attempt [attempt-1] in session [session-a]", ); + assert_eq!(err.candidate_culprits(), &[2, 3]); // Every non-aggregate error exposes no culprits. assert!(EngineError::Validation("x".to_string())