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..4a87f7bfd4 100644 --- a/pkg/tbtc/signer/src/api.rs +++ b/pkg/tbtc/signer/src/api.rs @@ -219,13 +219,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 +671,15 @@ pub struct ErrorResponse { pub code: String, pub message: String, pub recovery_class: String, + /// CANDIDATE culprits for an `aggregate_share_verification_failed` error: + /// 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, } /// 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..68d41f4685 100644 --- a/pkg/tbtc/signer/src/engine/codec.rs +++ b/pkg/tbtc/signer/src/engine/codec.rs @@ -53,6 +53,43 @@ 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, as u16 +/// Go member identifiers (the same identifier space as +/// `excluded_member_identifiers`, so the Go host consumes them directly). +/// +/// 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 } => culprits + .iter() + .filter_map(|identifier| frost_identifier_to_u16(*identifier)) + .collect(), + _ => Vec::new(), + } +} + +/// 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( 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/tests.rs b/pkg/tbtc/signer/src/engine/tests.rs index 60f8a2d2cb..4c05783a82 100644 --- a/pkg/tbtc/signer/src/engine/tests.rs +++ b/pkg/tbtc/signer/src/engine/tests.rs @@ -13120,15 +13120,142 @@ 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. - assert!( - matches!(err, EngineError::Validation(ref m) if m.contains("failed to aggregate")), - "unexpected error: {err:?}" + // 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 (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 { + candidate_culprits, .. + } => candidate_culprits, + other => panic!("expected AggregateShareVerificationFailed, got {other:?}"), + }; + assert_eq!( + candidate_culprits, + vec![2], + "only the cheating member 2 must be named: {candidate_culprits:?}" + ); +} + +#[test] +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()], + ); + + // 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(), + }, + ], + ); + 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 34a0671ee7..8cfe342ac1 100644 --- a/pkg/tbtc/signer/src/errors.rs +++ b/pkg/tbtc/signer/src/errors.rs @@ -94,6 +94,28 @@ 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 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() + )] + AggregateShareVerificationFailed { + session_id: String, + attempt_id: String, + candidate_culprits: Vec, + }, #[error("internal error: {0}")] Internal(String), } @@ -119,6 +141,7 @@ impl EngineError { Self::InteractiveAttemptAlreadyAggregated { .. } => { "interactive_attempt_already_aggregated" } + Self::AggregateShareVerificationFailed { .. } => "aggregate_share_verification_failed", Self::Internal(_) => "internal_error", } } @@ -147,11 +170,28 @@ 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) -> &[u16] { + match self { + Self::AggregateShareVerificationFailed { + candidate_culprits, .. + } => candidate_culprits, + _ => &[], + } + } } #[cfg(test)] @@ -245,4 +285,27 @@ 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![2, 3], + }; + assert_eq!(err.code(), "aggregate_share_verification_failed"); + assert_eq!(err.recovery_class(), "recoverable"); + // The count is rendered; the member identifiers travel in the structured + // candidate_culprits list, not the message string. + assert_eq!( + err.to_string(), + "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()) + .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(|_| {