refactor: simplification signature logic

core-client/src/decrypt.rs

@@ -2177,10 +2177,16 @@ fn verify_public_decrypt_responses(
     kms_addrs: &[alloy_primitives::Address],
     num_expected_responses: usize,
 ) -> anyhow::Result<()> {
-    // Resolve the verification material into (domain, external handles, extra_data) plus the
-    // optional original request used for the internal request-binding check.
-    let (domain, external_handles, extra_data, request) = match verification {
+    // Resolve the verification material into (domain, external handles, extra_data).
+    let (domain, external_handles, extra_data) = match verification {
         PubDecVerificationMaterial::Request(decryption_request) => {
+            // With the request at hand, the responses are also validated against it:
+            // bound to it, and accepted by majority.
+            internal_client.process_decryption_resp(
+                &decryption_request,
+                num_expected_responses as u32,
+                resp_response_vec,
+            )?;
             let domain_msg = decryption_request
                 .domain
                 .as_ref()
@@ -2192,19 +2198,13 @@ fn verify_public_decrypt_responses(
                 .iter()
                 .map(|ct| ct.external_handle.clone())
                 .collect();
-            let extra_data = decryption_request.extra_data.clone();
-            (
-                domain,
-                external_handles,
-                extra_data,
-                Some(decryption_request),
-            )
+            (domain, external_handles, decryption_request.extra_data)
         }
         PubDecVerificationMaterial::External {
             domain,
             external_handles,
             extra_data,
-        } => (domain, external_handles, extra_data, None),
+        } => (domain, external_handles, extra_data),
     };
 
     // If an expected answer is provided, use it; otherwise consider the first answer.
@@ -2222,15 +2222,8 @@ fn verify_public_decrypt_responses(
             .clone(),
     };
 
-    // check the internal signatures (verifies responses are signed by the trusted KMS keys;
-    // request-binding only applies for the `Request` variant)
-    internal_client.process_decryption_resp(
-        request,
-        num_expected_responses as u32,
-        resp_response_vec,
-    )?;
-
-    // check the per-scheme signatures
+    // Check every response's signatures, signer and plaintext. For `External` material this
+    // is the only check, since without the request there is nothing to bind the responses to.
     check_external_decryption_signature(
         resp_response_vec,
         ptxt,

core/service/src/client/client_non_wasm.rs

@@ -155,7 +155,7 @@ impl Client {
                 dsep,
                 internal_bytes: &[],
                 payload,
-                eip712_hash: Some(sol_type.eip712_signing_hash(domain)),
+                eip712_hash: sol_type.eip712_signing_hash(domain),
             },
             &self.signing_schemes,
             &ExpectedSigner::Discover {
@@ -274,7 +274,7 @@ mod tests {
         payload: &CrsSignedPayload,
     ) -> Vec<TypedSignature> {
         let eip712_hash = sol_type().eip712_signing_hash(&dummy_domain());
-        sign_result_entries(identity, schemes, DSEP, eip712_hash.as_slice(), payload)
+        sign_result_entries(identity, schemes, DSEP, &eip712_hash, payload)
             .unwrap()
             .iter()
             .map(TypedSignature::from)
@@ -326,25 +326,6 @@ mod tests {
         );
     }
 
-    /// The rolling-upgrade case: a node from a release before `signatures` answers with
-    /// an empty list and the legacy ECDSA signature alone, and that still authenticates
-    /// the result for a client asking only for ECDSA.
-    #[test]
-    fn an_empty_list_falls_back_to_the_legacy_signature() {
-        let identity = seeded_identity(12);
-        let client = client_for(&identity, &[]);
-
-        let (party_id, address) = verify_with_legacy(
-            &client,
-            &[],
-            &legacy_external_signature(&identity),
-            &payload(),
-        )
-        .unwrap();
-        assert_eq!(party_id, PARTY);
-        assert_eq!(address, identity.verf_key().address());
-    }
-
     /// The fallback counts for ECDSA only. An old node cannot produce a post-quantum
     /// signature, so a request that named one is not satisfied by its legacy signature —
     /// otherwise any server could drop a requested scheme and still be accepted.
@@ -371,81 +352,28 @@ mod tests {
         );
     }
 
-    /// The fallback is a real signature check, not a waiver for an empty list.
-    #[test]
-    fn the_legacy_fallback_rejects_a_signature_of_another_party() {
-        let identity = seeded_identity(14);
-        let client = client_for(&identity, &[]);
-        let stranger = legacy_external_signature(&seeded_identity(15));
-
-        assert!(verify_with_legacy(&client, &[], &stranger, &payload()).is_err());
-        assert!(verify_with_legacy(&client, &[], &[0u8; 65], &payload()).is_err());
-    }
-
-    /// The legacy signature is checked *alongside* the list, not instead of it. A result
-    /// whose list verifies but whose deprecated field does not is still rejected: the two
-    /// are independent statements about the same result, and they have to agree.
+    /// Each non-ECDSA entry verifies for, and is attributed to, the party that signed it,
+    /// and covers the payload it was signed over and nothing else.
     #[test]
-    fn a_bad_legacy_signature_is_rejected_even_when_the_list_verifies() {
-        let identity = seeded_identity(16);
-        let client = client_for(&identity, &[]);
-        let signatures = signatures_for(&identity, &[SigningSchemeType::Ecdsa256k1], &payload());
-
-        // Both copies present and agreeing is the honest case.
-        let (party_id, _address) = verify_with_legacy(
-            &client,
-            &signatures,
-            &legacy_external_signature(&identity),
-            &payload(),
-        )
-        .unwrap();
-        assert_eq!(party_id, PARTY);
-
-        // A garbage legacy signature is a rejection, and so is one of another party.
-        assert!(verify_with_legacy(&client, &signatures, &[0xAA; 65], &payload()).is_err());
-        assert!(
-            verify_with_legacy(
-                &client,
-                &signatures,
-                &legacy_external_signature(&seeded_identity(17)),
-                &payload(),
-            )
-            .is_err()
-        );
-    }
-
-    /// A MlDsa65 signature is attributed to the correct signing party, provided
-    /// MlDsa65 is what the client asked for.
-    #[test]
-    fn a_result_without_an_ecdsa_entry_is_attributed() {
+    fn each_non_ecdsa_entry_is_attributed_and_bound_to_its_payload() {
         let identity = seeded_identity(3);
-        let client = client_requesting(&identity, true, &[SigningSchemeType::MlDsa65]);
-        let signatures = signatures_for(&identity, &[SigningSchemeType::MlDsa65], &payload());
-
-        let (party_id, _address) = verify(&client, &signatures, &payload()).unwrap();
-        assert_eq!(party_id, PARTY);
-    }
-
-    /// Each entry covers the payload it was signed over, and nothing else.
-    #[test]
-    fn a_tampered_payload_is_rejected() {
-        let identity = seeded_identity(4);
 
         for scheme in SigningSchemeType::iter().filter(|s| *s != SigningSchemeType::Ecdsa256k1) {
-            // Ask for exactly the scheme under test, so the rejection can only come
-            // from the signature check and not from a scheme left unverified.
+            // Ask for exactly the scheme under test, so a rejection can only come from
+            // the signature check and not from a scheme left unverified.
             let client = client_requesting(&identity, true, &[scheme]);
             let signatures = signatures_for(&identity, &[scheme], &payload());
+            assert_eq!(
+                verify(&client, &signatures, &payload()).unwrap().0,
+                PARTY,
+                "{scheme}"
+            );
+            let tampered = CrsSignedPayload {
+                crs_digest: vec![8u8; 32],
+                ..payload()
+            };
             assert!(
-                verify(
-                    &client,
-                    &signatures,
-                    &CrsSignedPayload {
-                        crs_digest: vec![8u8; 32],
-                        ..payload()
-                    }
-                )
-                .is_err(),
+                verify(&client, &signatures, &tampered).is_err(),
                 "the {scheme} entry verified a payload it does not cover"
             );
         }
@@ -478,44 +406,6 @@ mod tests {
         assert_eq!(client.signing_schemes(), &composite);
     }
 
-    /// An entry of a scheme this release does not know is passed over, so a newer node
-    /// can add a scheme without breaking a verifier still on this release.
-    #[test]
-    fn an_entry_of_an_unknown_scheme_is_skipped() {
-        let identity = seeded_identity(20);
-        let client = client_for(&identity, &[]);
-        let mut signatures =
-            signatures_for(&identity, &[SigningSchemeType::Ecdsa256k1], &payload());
-        signatures.push(TypedSignature {
-            scheme: i32::MAX,
-            signature: vec![0xEE; 64],
-        });
-
-        assert_eq!(
-            verify(&client, &signatures, &payload()).unwrap(),
-            (PARTY, identity.verf_key().address())
-        );
-        // On its own the unknown entry authenticates nothing.
-        assert!(verify(&client, &signatures[1..], &payload()).is_err());
-    }
-
-    /// Another party's signatures are not accepted as this party's.
-    #[test]
-    fn another_partys_signatures_are_rejected() {
-        let identity = seeded_identity(5);
-        let client = client_for(&identity, &[]);
-        let signatures = signatures_for(
-            &seeded_identity(6),
-            &[SigningSchemeType::Ecdsa256k1],
-            &payload(),
-        );
-
-        let err = verify(&client, &signatures, &payload())
-            .unwrap_err()
-            .to_string();
-        assert!(err.contains("belongs to no known party"), "{err}");
-    }
-
     /// A signature that cannot be checked for want of a key must not pass for one
     /// that was checked.
     #[test]
@@ -536,8 +426,8 @@ mod tests {
             .unwrap_err()
             .to_string();
         assert!(
-            err.contains("party 1 signed under Ed25519")
-                && err.contains("no Ed25519 verification key is known"),
+            err.contains("party 1")
+                && err.contains("no party published a Ed25519 verification key"),
             "the error does not name the missing key: {err}"
         );
     }
@@ -608,7 +498,9 @@ mod tests {
         }
     }
 
-    /// ECDSA signatures must parse and authenticate the expected signer and message.
+    /// ECDSA signatures must parse and authenticate the expected signer and message, both
+    /// as the list entry and, for a node from before the list, as the legacy field. A
+    /// legacy field beside a valid list entry is not checked at all.
     #[test]
     fn invalid_ecdsa_signatures_are_rejected() {
         let identity = seeded_identity(18);
@@ -626,20 +518,19 @@ mod tests {
             ),
             ("wrong message", wrong_message),
         ];
+        let list = |signature: Vec<u8>| {
+            vec![TypedSignature {
+                scheme: SigningSchemeType::Ecdsa256k1.as_wire(),
+                signature,
+            }]
+        };
 
         for legacy in [false, true] {
             let check = |signature: Vec<u8>| {
                 if legacy {
                     verify_with_legacy(&client, &[], &signature, &payload())
                 } else {
-                    verify(
-                        &client,
-                        &[TypedSignature {
-                            scheme: SigningSchemeType::Ecdsa256k1.as_wire(),
-                            signature,
-                        }],
-                        &payload(),
-                    )
+                    verify(&client, &list(signature), &payload())
                 }
             };
             assert_eq!(
@@ -650,5 +541,21 @@ mod tests {
                 assert!(check(signature.clone()).is_err(), "{case}, legacy={legacy}");
             }
         }
+
+        // Another party's entry is attributed to no known party.
+        let err = verify(&client, &list(invalid[2].1.clone()), &payload())
+            .unwrap_err()
+            .to_string();
+        assert!(err.contains("belongs to no known party"), "{err}");
+
+        for (case, signature) in &invalid {
+            assert_eq!(
+                verify_with_legacy(&client, &list(valid.clone()), signature, &payload())
+                    .unwrap()
+                    .0,
+                PARTY,
+                "a bad {case} legacy field beside a valid list entry"
+            );
+        }
     }
 }

core/service/src/client/public_decryption.rs

@@ -6,7 +6,7 @@ use alloy_sol_types::Eip712Domain;
 use kms_grpc::identifiers::ContextId;
 use kms_grpc::kms::v1::TypedPlaintext;
 use kms_grpc::kms::v1::{PublicDecryptionRequest, PublicDecryptionResponse, TypedCiphertext};
-use kms_grpc::rpc_types::{alloy_to_protobuf_domain, optional_protobuf_to_alloy_domain};
+use kms_grpc::rpc_types::alloy_to_protobuf_domain;
 use kms_grpc::{EpochId, RequestId};
 
 impl Client {
@@ -47,26 +47,16 @@ impl Client {
     }
 
     /// Validates the aggregated decryption response `agg_resp` against the
-    /// original `DecryptionRequest` `request`, and returns the decrypted
-    /// plaintext if valid and at least `min_agree_count` agree on the result.
-    ///
-    /// __NOTE__: If the original request is not provided, we can __not__ check
-    /// that the response correctly contains the digest of the request.
+    /// original `request`, and returns the decrypted plaintext if valid and at
+    /// least `min_agree_count` agree on the result.
     ///
     /// # Arguments
     ///
     /// All arguments except `agg_resp` are **trusted** (client-side state):
     ///
     /// * `request` — The original public decryption request constructed by this
-    ///   client. Used to verify that the server responses match the request
-    ///   (digest, ciphertext handles, domain).
-    ///
-    ///   Passing `None` skips the request-level checks, and with them the EIP-712
-    ///   domain, so neither `external_signature` nor the ECDSA entry of
-    ///   `signatures` can be checked. A response is then authenticated by the
-    ///   deprecated internal `signature`, which covers the serialized payload and
-    ///   needs no domain. That is enough for a caller that only wants to inspect a
-    ///   result.
+    ///   client. The responses are verified against its EIP-712 domain, ciphertext
+    ///   handles, extra data and signing schemes, and bound to it.
     /// * `min_agree_count` — Minimum number of server responses that must agree
     ///   on the same plaintext for the result to be accepted.
     ///
@@ -76,30 +66,14 @@ impl Client {
     ///   (signatures, digest matching, majority agreement) before use.
     pub fn process_decryption_resp(
         &self,
-        request: Option<PublicDecryptionRequest>,
+        request: &PublicDecryptionRequest,
         min_agree_count: u32,
         agg_resp: &[PublicDecryptionResponse],
     ) -> anyhow::Result<Vec<TypedPlaintext>> {
-        let eip712_domain = match &request {
-            Some(req) => Some(optional_protobuf_to_alloy_domain(req.domain.as_ref())?),
-            None => None,
-        };
-        let ext_handles_bytes: Vec<Vec<u8>> = match &request {
-            Some(req) => req
-                .ciphertexts
-                .iter()
-                .map(|c| c.external_handle.clone())
-                .collect(),
-            None => vec![],
-        };
-        let extra_data = request.as_ref().map(|req| req.extra_data.as_slice());
         let trusted_ctx = PublicDecTrustedValidationContext::new(
             self.get_server_pks()?,
             &self.scheme_verf_keys,
-            eip712_domain.as_ref(),
-            &ext_handles_bytes,
-            extra_data,
-            request.as_ref(),
+            request,
         )?;
 
         // Partition the untrusted responses and enforce the majority threshold. Partitioning is

core/service/src/client/tests/centralized/public_decryption_tests.rs

@@ -287,7 +287,7 @@ pub(crate) async fn run_decryption_centralized(
         assert_eq!(responses.len(), 1);
 
         let received_plaintexts = internal_client
-            .process_decryption_resp(Some(req.clone()), 1, &responses)
+            .process_decryption_resp(req, 1, &responses)
             .unwrap();
 
         // we need 1 plaintext for each ciphertext in the batch