chore(signer): resolve TODOs

- simplify internals of `EcdsaPublicKey`: no need to store the actual abstract object, just its compressed encoding - we construct it when needed
- extract verification logic into a separate trait `Verifier`, not in `SecretKey`
- remove some (already) obsolete utility functions from `signature.rs`

Author: David-Petrov

crates/common/src/commit/request.rs

@@ -54,8 +54,6 @@ impl fmt::Display for SignedProxyDelegation {
 // generalisation) and avoid the is_proxy flag
 #[derive(Debug, Clone, Serialize, Deserialize)]
 pub struct SignRequest {
-    // TODO(David): Vec<u8> might not be the most memory inefficient, think about something on the
-    // stack
     pub pubkey: Vec<u8>,
     pub is_proxy: bool,
     pub object_root: [u8; 32],

crates/common/src/signature.rs

@@ -1,61 +1,35 @@
-use alloy::rpc::types::beacon::{constants::BLS_DST_SIG, BlsPublicKey, BlsSignature};
-use blst::{
-    min_pk::{PublicKey, SecretKey as BlsSecretKey, Signature},
-    BLST_ERROR,
-};
+use alloy::rpc::types::beacon::{BlsPublicKey, BlsSignature};
 use ssz_derive::{Decode, Encode};
 use tree_hash::TreeHash;
 use tree_hash_derive::TreeHash;
 
 use crate::{
     constants::{APPLICATION_BUILDER_DOMAIN, GENESIS_VALIDATORS_ROOT},
     error::BlstErrorWrapper,
-    signer::SecretKey,
+    signer::{SecretKey, Verifier},
     types::Chain,
-    utils::{alloy_pubkey_to_blst, alloy_sig_to_blst},
 };
 
-// TODO(David): Think about removing
-pub fn verify_signature(
-    pubkey: &BlsPublicKey,
-    msg: &[u8],
-    signature: &BlsSignature,
-) -> Result<(), BlstErrorWrapper> {
-    let pubkey: PublicKey = alloy_pubkey_to_blst(pubkey)?;
-    let signature: Signature = alloy_sig_to_blst(signature)?;
-
-    let res = signature.verify(true, msg, BLS_DST_SIG, &[], &pubkey, true);
-    if res == BLST_ERROR::BLST_SUCCESS {
-        Ok(())
-    } else {
-        Err(res.into())
+pub fn compute_signing_root(object_root: [u8; 32], signing_domain: [u8; 32]) -> [u8; 32] {
+    #[derive(Default, Debug, Encode, Decode, TreeHash)]
+    struct SigningData {
+        object_root: [u8; 32],
+        signing_domain: [u8; 32],
     }
-}
-
-pub fn sign_message(secret_key: &BlsSecretKey, msg: &[u8]) -> BlsSignature {
-    let signature = secret_key.sign(msg, BLS_DST_SIG, &[]).to_bytes();
-    BlsSignature::from_slice(&signature)
-}
 
-#[derive(Default, Debug, Encode, Decode, TreeHash)]
-struct SigningData {
-    object_root: [u8; 32],
-    signing_domain: [u8; 32],
-}
-
-pub fn compute_signing_root(object_root: [u8; 32], signing_domain: [u8; 32]) -> [u8; 32] {
     let signing_data = SigningData { object_root, signing_domain };
     signing_data.tree_hash_root().0
 }
 
-#[derive(Debug, Encode, Decode, TreeHash)]
-struct ForkData {
-    fork_version: [u8; 4],
-    genesis_validators_root: [u8; 32],
-}
-
 #[allow(dead_code)]
 fn compute_builder_domain(chain: Chain) -> [u8; 32] {
+
+    #[derive(Debug, Encode, Decode, TreeHash)]
+    struct ForkData {
+        fork_version: [u8; 4],
+        genesis_validators_root: [u8; 32],
+    }
+
     let mut domain = [0u8; 32];
     domain[..4].copy_from_slice(&APPLICATION_BUILDER_DOMAIN);
 
@@ -77,7 +51,7 @@ pub fn verify_signed_builder_message<T: TreeHash>(
     let domain = chain.builder_domain();
     let signing_root = compute_signing_root(msg.tree_hash_root().0, domain);
 
-    verify_signature(pubkey, &signing_root, signature)
+    pubkey.verify_signature(&signing_root, signature)
 }
 
 pub fn sign_builder_message<T: SecretKey>(
@@ -88,7 +62,6 @@ pub fn sign_builder_message<T: SecretKey>(
     sign_builder_root(chain, secret_key, msg.tree_hash_root().0)
 }
 
-// TODO(David): This utility function seems unnecessary
 pub fn sign_builder_root<T: SecretKey>(
     chain: Chain,
     secret_key: &T,
@@ -97,7 +70,6 @@ pub fn sign_builder_root<T: SecretKey>(
     let domain = chain.builder_domain();
     let signing_root = compute_signing_root(object_root, domain);
     secret_key.sign(&signing_root)
-    // sign_message(secret_key, &signing_root)
 }
 
 #[cfg(test)]

crates/common/src/signer/mod.rs

@@ -3,7 +3,7 @@ use std::{
     fmt::{self, LowerHex},
 };
 
-use eyre::{Context, Result};
+use eyre::Context;
 use serde::{Deserialize, Serialize};
 use ssz_derive::{Decode, Encode};
 use tree_hash::TreeHash;
@@ -17,24 +17,30 @@ pub use signers::{GenericProxySigner, ProxySigner, Signer};
 pub type PubKey<T> = <T as SecretKey>::PubKey;
 
 pub trait SecretKey {
-    type PubKey: AsRef<[u8]> + Clone;
+    type PubKey: AsRef<[u8]> + Clone + Verifier<Self>;
     type Signature: AsRef<[u8]> + Clone;
-    type VerificationError: Error;
 
     fn new_random() -> Self;
-    fn new_from_bytes(bytes: &[u8]) -> Result<Self>
+    fn new_from_bytes(bytes: &[u8]) -> eyre::Result<Self>
     where
         Self: Sized;
     fn pubkey(&self) -> Self::PubKey;
-    fn sign(&self, msg: &[u8; 32]) -> Self::Signature;
+    fn sign(&self, msg: &[u8]) -> Self::Signature;
     fn sign_msg(&self, msg: &impl TreeHash) -> Self::Signature {
         self.sign(&msg.tree_hash_root().0)
     }
+}
+
+pub trait Verifier<T: SecretKey>
+where
+    T: ?Sized,
+{
+    type VerificationError: Error;
 
     fn verify_signature(
-        pubkey: &Self::PubKey,
+        &self,
         msg: &[u8],
-        signature: &Self::Signature,
+        signature: &T::Signature,
     ) -> Result<(), Self::VerificationError>;
 }
 
@@ -49,14 +55,13 @@ pub enum GenericPubkey {
 impl GenericPubkey {
     pub fn verify_signature(&self, msg: &[u8], signature: &[u8]) -> eyre::Result<()> {
         match self {
-            GenericPubkey::Bls(bls_pubkey) => Ok(<BlsSecretKey as SecretKey>::verify_signature(
-                bls_pubkey,
-                msg,
-                signature.try_into().context("Invalid signature length for BLS.")?,
-            )?),
+            GenericPubkey::Bls(bls_pubkey) => {
+                let sig = signature.try_into().context("Invalid signature length for BLS.")?;
+                Ok(bls_pubkey.verify_signature(msg, &sig)?)
+            }
             GenericPubkey::Ecdsa(ecdsa_pubkey) => {
                 let sig = signature.try_into().context("Invalid signature for ECDSA.")?;
-                Ok(<EcdsaSecretKey as SecretKey>::verify_signature(ecdsa_pubkey, msg, &sig)?)
+                Ok(ecdsa_pubkey.verify_signature(msg, &sig)?)
             }
         }
     }

crates/common/src/signer/schemes/bls.rs

@@ -1,9 +1,9 @@
-use alloy::rpc::types::beacon::{BlsPublicKey, BlsSignature};
+use alloy::rpc::types::beacon::{constants::BLS_DST_SIG, BlsPublicKey, BlsSignature};
+use blst::BLST_ERROR;
 
 use crate::{
     error::BlstErrorWrapper,
-    signature::{sign_message, verify_signature},
-    signer::{GenericPubkey, SecretKey},
+    signer::{GenericPubkey, PubKey, SecretKey, Verifier},
     utils::blst_pubkey_to_alloy,
 };
 
@@ -12,7 +12,6 @@ pub type BlsSecretKey = blst::min_pk::SecretKey;
 impl SecretKey for BlsSecretKey {
     type PubKey = BlsPublicKey;
     type Signature = BlsSignature;
-    type VerificationError = BlstErrorWrapper;
 
     fn new_random() -> Self {
         use rand::RngCore;
@@ -36,16 +35,31 @@ impl SecretKey for BlsSecretKey {
         blst_pubkey_to_alloy(&self.sk_to_pk())
     }
 
-    fn sign(&self, msg: &[u8; 32]) -> Self::Signature {
-        sign_message(self, msg)
+    fn sign(&self, msg: &[u8]) -> Self::Signature {
+        let signature = self.sign(msg, BLS_DST_SIG, &[]).to_bytes();
+        BlsSignature::from_slice(&signature)
     }
+}
+
+impl Verifier<BlsSecretKey> for PubKey<BlsSecretKey> {
+    type VerificationError = BlstErrorWrapper;
 
     fn verify_signature(
-        pubkey: &Self::PubKey,
+        &self,
         msg: &[u8],
-        signature: &Self::Signature,
+        signature: &<BlsSecretKey as SecretKey>::Signature,
     ) -> Result<(), Self::VerificationError> {
-        verify_signature(pubkey, msg, signature)
+        use crate::utils::{alloy_pubkey_to_blst, alloy_sig_to_blst};
+
+        let pubkey = alloy_pubkey_to_blst(self)?;
+        let signature = alloy_sig_to_blst(signature)?;
+
+        let res = signature.verify(true, msg, BLS_DST_SIG, &[], &pubkey, true);
+        if res == BLST_ERROR::BLST_SUCCESS {
+            Ok(())
+        } else {
+            Err(res.into())
+        }
     }
 }
 

crates/common/src/signer/schemes/ecdsa.rs

@@ -3,56 +3,26 @@ use std::hash::Hash;
 use derive_more::derive::Into;
 use eyre::Result;
 use generic_array::GenericArray;
-use k256::{
-    ecdsa::{Signature as EcdsaSignatureInner, VerifyingKey as EcdsaPublicKeyInner},
-    AffinePoint,
-};
+use k256::ecdsa::{Signature as EcdsaSignatureInner, VerifyingKey as EcdsaPublicKeyInner};
 use serde::{Deserialize, Serialize};
 use ssz_types::{typenum::U33, FixedVector};
 use tree_hash::TreeHash;
 
-use crate::signer::{GenericPubkey, PubKey, SecretKey};
+use crate::signer::{GenericPubkey, PubKey, SecretKey, Verifier};
 
 pub type EcdsaSecretKey = k256::ecdsa::SigningKey;
-type EcdsaCompressedKey = GenericArray<u8, U33>;
+
+type CompressedPublicKey = GenericArray<u8, U33>;
 
 #[derive(Debug, Clone, Copy, Into, Serialize, Deserialize, PartialEq, Eq)]
-#[serde(from = "JSONEcdsaPublicKey")]
 pub struct EcdsaPublicKey {
-    #[serde(skip_serializing)]
-    inner: EcdsaPublicKeyInner,
-    encoded: EcdsaCompressedKey,
+    encoded: CompressedPublicKey,
 }
 
 impl EcdsaPublicKey {
     /// Size of the public key in bytes. We store the SEC1 encoded affine point
     /// compressed, thus 33 bytes.
     const SIZE: usize = 33;
-
-    pub fn new(inner: EcdsaPublicKeyInner) -> Self {
-        use elliptic_curve::sec1::ToEncodedPoint;
-
-        let affine: &AffinePoint = inner.as_ref();
-        let encoded: [u8; Self::SIZE] =
-            affine.to_encoded_point(true).as_bytes().try_into().unwrap();
-
-        let encoded = GenericArray::from_array(encoded);
-
-        EcdsaPublicKey { inner, encoded }
-    }
-
-    pub fn new_from_bytes(encoded: Vec<u8>) -> Result<Self, k256::ecdsa::Error> {
-        let inner = EcdsaPublicKeyInner::from_sec1_bytes(&encoded)?;
-        let encoded = GenericArray::from_array::<{ Self::SIZE }>(encoded.try_into().unwrap());
-        Ok(Self { inner, encoded })
-    }
-
-    fn new_from_bytes_infallible(encoded: [u8; Self::SIZE]) -> Self {
-        Self {
-            inner: EcdsaPublicKeyInner::from_sec1_bytes(&encoded).unwrap(),
-            encoded: GenericArray::from_array(encoded),
-        }
-    }
 }
 
 impl Hash for EcdsaPublicKey {
@@ -131,38 +101,21 @@ impl ssz::Decode for EcdsaPublicKey {
         let mut fixed_array = [0_u8; Self::SIZE];
         fixed_array.copy_from_slice(bytes);
 
-        Ok(EcdsaPublicKey::new_from_bytes_infallible(fixed_array))
+        Ok(EcdsaPublicKey { encoded: fixed_array.into() })
     }
 }
 
-impl TryFrom<Vec<u8>> for EcdsaPublicKey {
-    type Error = k256::ecdsa::Error;
-
-    fn try_from(value: Vec<u8>) -> std::result::Result<Self, Self::Error> {
-        Self::new_from_bytes(value)
-    }
-}
-
-impl From<EcdsaCompressedKey> for EcdsaPublicKey {
-    fn from(value: EcdsaCompressedKey) -> Self {
-        Self::new_from_bytes_infallible(value.into())
-    }
-}
-
-#[derive(Deserialize)]
-struct JSONEcdsaPublicKey {
-    encoded: EcdsaCompressedKey,
-}
-
-impl From<JSONEcdsaPublicKey> for EcdsaPublicKey {
-    fn from(value: JSONEcdsaPublicKey) -> Self {
-        Self::from(value.encoded)
+impl From<CompressedPublicKey> for EcdsaPublicKey {
+    fn from(value: CompressedPublicKey) -> Self {
+        Self { encoded: value }
     }
 }
 
 impl From<EcdsaPublicKeyInner> for EcdsaPublicKey {
     fn from(value: EcdsaPublicKeyInner) -> Self {
-        EcdsaPublicKey::new(value)
+        let encoded: [u8; Self::SIZE] = value.to_encoded_point(true).as_bytes().try_into().unwrap();
+
+        EcdsaPublicKey { encoded: encoded.into() }
     }
 }
 
@@ -210,8 +163,6 @@ impl SecretKey for EcdsaSecretKey {
 
     type Signature = EcdsaSignature;
 
-    type VerificationError = k256::ecdsa::Error;
-
     fn new_random() -> Self {
         EcdsaSecretKey::random(&mut rand::thread_rng())
     }
@@ -227,17 +178,22 @@ impl SecretKey for EcdsaSecretKey {
         EcdsaPublicKeyInner::from(self).into()
     }
 
-    fn sign(&self, msg: &[u8; 32]) -> Self::Signature {
+    fn sign(&self, msg: &[u8]) -> Self::Signature {
         k256::ecdsa::signature::Signer::<EcdsaSignatureInner>::sign(self, msg).into()
     }
+}
+
+impl Verifier<EcdsaSecretKey> for PubKey<EcdsaSecretKey> {
+    type VerificationError = k256::ecdsa::Error;
 
     fn verify_signature(
-        pubkey: &Self::PubKey,
+        &self,
         msg: &[u8],
-        signature: &Self::Signature,
+        signature: &<EcdsaSecretKey as SecretKey>::Signature,
     ) -> Result<(), Self::VerificationError> {
         use k256::ecdsa::signature::Verifier;
-        pubkey.inner.verify(msg, &signature.inner)
+        let ecdsa_pubkey = EcdsaPublicKeyInner::from_sec1_bytes(&self.encoded)?;
+        ecdsa_pubkey.verify(msg, &signature.inner)
     }
 }
 

crates/common/src/signer/signers.rs

@@ -38,17 +38,6 @@ impl<T: SecretKey> Signer<T> {
     pub async fn sign_msg(&self, chain: Chain, msg: &impl TreeHash) -> T::Signature {
         self.sign(chain, msg.tree_hash_root().0).await
     }
-
-    // TODO(David):
-    // This doesn't need to be here. A separate `Verifier` might make sense.
-    // Left here for now (for convenience).
-    pub async fn verify_signature(
-        pubkey: &T::PubKey,
-        msg: &[u8],
-        signature: &T::Signature,
-    ) -> Result<(), T::VerificationError> {
-        T::verify_signature(pubkey, msg, signature)
-    }
 }
 
 // For extra safety and to avoid risking signing malicious messages, use a proxy