Use SignatureBytes and PublicKeyBytes for deposits (#472)

* Replace deposit signatures with SignatureBytes, a struct which lazyly parsers signatures only on demand.

* check byte length when parsing SignatureBytes

* add comment to struct

* distinguish BadSignature and BadSignatureBytes in verify_deposit_signature

* add test for valid signature

* Implements TryInto<Signature> for &SignatureBytes and From<Signature> for &SignatureBytes

* add and use PublicKeyBytes + fix formatting

* fix compiler warning + docs for macro generated structs

* adds tests to ensure correct byte lengths

* small style improvement as suggested by michaelsproul

eth2/state_processing/src/per_block_processing.rs

@@ -2,6 +2,7 @@ use crate::common::{initiate_validator_exit, slash_validator};
 use errors::{BlockInvalid as Invalid, BlockProcessingError as Error, IntoWithIndex};
 use rayon::prelude::*;
 use std::collections::HashSet;
+use std::convert::TryInto;
 use std::iter::FromIterator;
 use tree_hash::{SignedRoot, TreeHash};
 use types::*;
@@ -396,9 +397,13 @@ pub fn process_deposit<T: EthSpec>(
     // depositing validator already exists in the registry.
     state.update_pubkey_cache()?;
 
+    let pubkey: PublicKey = match (&deposit.data.pubkey).try_into() {
+        Err(_) => return Ok(()), //bad public key => return early
+        Ok(k) => k,
+    };
     // Get an `Option<u64>` where `u64` is the validator index if this deposit public key
     // already exists in the beacon_state.
-    let validator_index = get_existing_validator_index(state, deposit)
+    let validator_index = get_existing_validator_index(state, &pubkey)
         .map_err(|e| e.into_with_index(deposit_index))?;
 
     let amount = deposit.data.amount;
@@ -409,13 +414,13 @@ pub fn process_deposit<T: EthSpec>(
     } else {
         // The signature should be checked for new validators. Return early for a bad
         // signature.
-        if verify_deposit_signature(state, deposit, spec).is_err() {
+        if verify_deposit_signature(state, deposit, spec, &pubkey).is_err() {
             return Ok(());
         }
 
         // Create a new validator.
         let validator = Validator {
-            pubkey: deposit.data.pubkey.clone(),
+            pubkey,
             withdrawal_credentials: deposit.data.withdrawal_credentials,
             activation_eligibility_epoch: spec.far_future_epoch,
             activation_epoch: spec.far_future_epoch,

eth2/state_processing/src/per_block_processing/errors.rs

@@ -349,6 +349,8 @@ pub enum DepositInvalid {
     BadIndex { state: u64, deposit: u64 },
     /// The signature (proof-of-possession) does not match the given pubkey.
     BadSignature,
+    /// The signature does not represent a valid BLS signature.
+    BadSignatureBytes,
     /// The specified `branch` and `index` did not form a valid proof that the deposit is included
     /// in the eth1 deposit root.
     BadMerkleProof,

eth2/state_processing/src/per_block_processing/verify_deposit.rs

@@ -1,5 +1,6 @@
 use super::errors::{DepositInvalid as Invalid, DepositValidationError as Error};
 use merkle_proof::verify_merkle_proof;
+use std::convert::TryInto;
 use tree_hash::{SignedRoot, TreeHash};
 use types::*;
 
@@ -10,15 +11,17 @@ pub fn verify_deposit_signature<T: EthSpec>(
     state: &BeaconState<T>,
     deposit: &Deposit,
     spec: &ChainSpec,
+    pubkey: &PublicKey,
 ) -> Result<(), Error> {
     // Note: Deposits are valid across forks, thus the deposit domain is computed
     // with the fork zeroed.
     let domain = spec.get_domain(state.current_epoch(), Domain::Deposit, &Fork::default());
+    let signature: Signature = (&deposit.data.signature)
+        .try_into()
+        .map_err(|_| Error::Invalid(Invalid::BadSignatureBytes))?;
+
     verify!(
-        deposit
+        signature.verify(&deposit.data.signed_root(), domain, pubkey),
-            .data
-            .signature
-            .verify(&deposit.data.signed_root(), domain, &deposit.data.pubkey,),
         Invalid::BadSignature
     );
 
@@ -33,9 +36,9 @@ pub fn verify_deposit_signature<T: EthSpec>(
 /// Errors if the state's `pubkey_cache` is not current.
 pub fn get_existing_validator_index<T: EthSpec>(
     state: &BeaconState<T>,
-    deposit: &Deposit,
+    pub_key: &PublicKey,
 ) -> Result<Option<u64>, Error> {
-    let validator_index = state.get_validator_index(&deposit.data.pubkey)?;
+    let validator_index = state.get_validator_index(pub_key)?;
     Ok(validator_index.map(|idx| idx as u64))
 }
 

eth2/types/src/deposit_data.rs

@@ -1,6 +1,7 @@
 use crate::test_utils::TestRandom;
 use crate::*;
-use bls::{PublicKey, Signature};
+use bls::{PublicKeyBytes, SignatureBytes};
+use std::convert::From;
 
 use serde_derive::{Deserialize, Serialize};
 use ssz_derive::{Decode, Encode};
@@ -25,11 +26,11 @@ use tree_hash_derive::{CachedTreeHash, SignedRoot, TreeHash};
     TestRandom,
 )]
 pub struct DepositData {
-    pub pubkey: PublicKey,
+    pub pubkey: PublicKeyBytes,
     pub withdrawal_credentials: Hash256,
     pub amount: u64,
     #[signed_root(skip_hashing)]
-    pub signature: Signature,
+    pub signature: SignatureBytes,
 }
 
 impl DepositData {
@@ -42,11 +43,11 @@ impl DepositData {
         epoch: Epoch,
         fork: &Fork,
         spec: &ChainSpec,
-    ) -> Signature {
+    ) -> SignatureBytes {
         let msg = self.signed_root();
         let domain = spec.get_domain(epoch, Domain::Deposit, fork);
 
-        Signature::new(msg.as_slice(), domain, secret_key)
+        SignatureBytes::from(Signature::new(msg.as_slice(), domain, secret_key))
     }
 }
 

eth2/types/src/test_utils/builders/testing_deposit_builder.rs

@@ -1,5 +1,5 @@
 use crate::*;
-use bls::get_withdrawal_credentials;
+use bls::{get_withdrawal_credentials, PublicKeyBytes, SignatureBytes};
 
 /// Builds an deposit to be used for testing purposes.
 ///
@@ -14,10 +14,10 @@ impl TestingDepositBuilder {
         let deposit = Deposit {
             proof: vec![].into(),
             data: DepositData {
-                pubkey,
+                pubkey: PublicKeyBytes::from(pubkey),
                 withdrawal_credentials: Hash256::zero(),
                 amount,
-                signature: Signature::empty_signature(),
+                signature: SignatureBytes::empty(),
             },
         };
 
@@ -34,7 +34,7 @@ impl TestingDepositBuilder {
             &get_withdrawal_credentials(&keypair.pk, spec.bls_withdrawal_prefix_byte)[..],
         );
 
-        self.deposit.data.pubkey = keypair.pk.clone();
+        self.deposit.data.pubkey = PublicKeyBytes::from(keypair.pk.clone());
         self.deposit.data.withdrawal_credentials = withdrawal_credentials;
 
         self.deposit.data.signature =

eth2/types/src/test_utils/test_random.rs

@@ -7,8 +7,10 @@ mod aggregate_signature;
 mod bitfield;
 mod hash256;
 mod public_key;
+mod public_key_bytes;
 mod secret_key;
 mod signature;
+mod signature_bytes;
 
 pub trait TestRandom {
     fn random_for_test(rng: &mut impl RngCore) -> Self;
@@ -99,3 +101,5 @@ macro_rules! impl_test_random_for_u8_array {
 
 impl_test_random_for_u8_array!(4);
 impl_test_random_for_u8_array!(32);
+impl_test_random_for_u8_array!(48);
+impl_test_random_for_u8_array!(96);

eth2/types/src/test_utils/test_random/public_key_bytes.rs

@@ -0,0 +1,19 @@
+use std::convert::From;
+
+use bls::{PublicKeyBytes, BLS_PUBLIC_KEY_BYTE_SIZE};
+
+use super::*;
+
+impl TestRandom for PublicKeyBytes {
+    fn random_for_test(rng: &mut impl RngCore) -> Self {
+        //50-50 chance for signature to be "valid" or invalid
+        if bool::random_for_test(rng) {
+            //valid signature
+            PublicKeyBytes::from(PublicKey::random_for_test(rng))
+        } else {
+            //invalid signature, just random bytes
+            PublicKeyBytes::from_bytes(&<[u8; BLS_PUBLIC_KEY_BYTE_SIZE]>::random_for_test(rng))
+                .unwrap()
+        }
+    }
+}

eth2/types/src/test_utils/test_random/signature_bytes.rs

@@ -0,0 +1,17 @@
+use bls::{SignatureBytes, BLS_SIG_BYTE_SIZE};
+
+use super::*;
+use std::convert::From;
+
+impl TestRandom for SignatureBytes {
+    fn random_for_test(rng: &mut impl RngCore) -> Self {
+        //50-50 chance for signature to be "valid" or invalid
+        if bool::random_for_test(rng) {
+            //valid signature
+            SignatureBytes::from(Signature::random_for_test(rng))
+        } else {
+            //invalid signature, just random bytes
+            SignatureBytes::from_bytes(&<[u8; BLS_SIG_BYTE_SIZE]>::random_for_test(rng)).unwrap()
+        }
+    }
+}

eth2/utils/bls/src/lib.rs

@@ -4,10 +4,14 @@ extern crate ssz;
 #[macro_use]
 mod macros;
 mod keypair;
+mod public_key_bytes;
 mod secret_key;
+mod signature_bytes;
 
 pub use crate::keypair::Keypair;
+pub use crate::public_key_bytes::PublicKeyBytes;
 pub use crate::secret_key::SecretKey;
+pub use crate::signature_bytes::SignatureBytes;
 pub use milagro_bls::{compress_g2, hash_on_g2};
 
 #[cfg(feature = "fake_crypto")]

eth2/utils/bls/src/macros.rs

@@ -84,3 +84,111 @@ macro_rules! impl_cached_tree_hash {
         }
     };
 }
+
+macro_rules! bytes_struct {
+    ($name: ident, $type: ty, $byte_size: expr, $small_name: expr, $ssz_type_size: ident,
+     $type_str: expr, $byte_size_str: expr) => {
+        #[doc = "Stores `"]
+        #[doc = $byte_size_str]
+        #[doc = "` bytes which may or may not represent a valid BLS "]
+        #[doc = $small_name]
+        #[doc = ".\n\nThe `"]
+        #[doc = $type_str]
+        #[doc = "` struct performs validation when it is instantiated, where as this struct does \
+                 not. This struct is suitable where we may wish to store bytes that are \
+                 potentially not a valid "]
+        #[doc = $small_name]
+        #[doc = " (e.g., from the deposit contract)."]
+        #[derive(Clone)]
+        pub struct $name([u8; $byte_size]);
+    };
+    ($name: ident, $type: ty, $byte_size: expr, $small_name: expr, $ssz_type_size: ident) => {
+        bytes_struct!($name, $type, $byte_size, $small_name, $ssz_type_size, stringify!($type),
+        stringify!($byte_size));
+
+        impl $name {
+            pub fn from_bytes(bytes: &[u8]) -> Result<Self, ssz::DecodeError> {
+                Ok(Self(Self::get_bytes(bytes)?))
+            }
+
+            pub fn empty() -> Self {
+                Self([0; $byte_size])
+            }
+
+            pub fn as_bytes(&self) -> Vec<u8> {
+                self.0.to_vec()
+            }
+
+            fn get_bytes(bytes: &[u8]) -> Result<[u8; $byte_size], ssz::DecodeError> {
+                let mut result = [0; $byte_size];
+                if bytes.len() != $byte_size {
+                    Err(ssz::DecodeError::InvalidByteLength {
+                        len: bytes.len(),
+                        expected: $byte_size,
+                    })
+                } else {
+                    result[..].copy_from_slice(bytes);
+                    Ok(result)
+                }
+            }
+        }
+
+        impl std::fmt::Debug for $name {
+            fn fmt(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result {
+                self.0[..].fmt(formatter)
+            }
+        }
+
+        impl PartialEq for $name {
+            fn eq(&self, other: &Self) -> bool {
+                &self.0[..] == &other.0[..]
+            }
+        }
+
+        impl Eq for $name {}
+
+        impl std::convert::TryInto<$type> for &$name {
+            type Error = ssz::DecodeError;
+
+            fn try_into(self) -> Result<$type, Self::Error> {
+                <$type>::from_bytes(&self.0[..])
+            }
+        }
+
+        impl std::convert::From<$type> for $name {
+            fn from(obj: $type) -> Self {
+                // We know that obj.as_bytes() always has exactly $byte_size many bytes.
+                Self::from_bytes(obj.as_ssz_bytes().as_slice()).unwrap()
+            }
+        }
+
+        impl_ssz!($name, $byte_size, "$type");
+
+        impl_tree_hash!($name, $ssz_type_size);
+
+        impl_cached_tree_hash!($name, $ssz_type_size);
+
+        impl serde::ser::Serialize for $name {
+            /// Serde serialization is compliant the Ethereum YAML test format.
+            fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+            where
+                S: serde::ser::Serializer,
+            {
+                serializer.serialize_str(&hex::encode(ssz::ssz_encode(self)))
+            }
+        }
+
+        impl<'de> serde::de::Deserialize<'de> for $name {
+            /// Serde serialization is compliant the Ethereum YAML test format.
+            fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+            where
+                D: serde::de::Deserializer<'de>,
+            {
+                let bytes = deserializer.deserialize_str(serde_hex::HexVisitor)?;
+                let signature = Self::from_ssz_bytes(&bytes[..])
+                    .map_err(|e| serde::de::Error::custom(format!("invalid ssz ({:?})", e)))?;
+                Ok(signature)
+            }
+        }
+    };
+}

eth2/utils/bls/src/public_key.rs

@@ -150,6 +150,15 @@ mod tests {
         assert_eq!(original, decoded);
     }
 
+    #[test]
+    pub fn test_byte_size() {
+        let sk = SecretKey::random();
+        let original = PublicKey::from_secret_key(&sk);
+
+        let bytes = ssz_encode(&original);
+        assert_eq!(bytes.len(), BLS_PUBLIC_KEY_BYTE_SIZE);
+    }
+
     #[test]
     // TODO: once `CachedTreeHash` is fixed, this test should _not_ panic.
     #[should_panic]

eth2/utils/bls/src/public_key_bytes.rs

@@ -0,0 +1,43 @@
+use ssz::{Decode, DecodeError, Encode};
+
+use super::{PublicKey, BLS_PUBLIC_KEY_BYTE_SIZE};
+
+bytes_struct!(
+    PublicKeyBytes,
+    PublicKey,
+    BLS_PUBLIC_KEY_BYTE_SIZE,
+    "public key",
+    U48
+);
+
+#[cfg(test)]
+mod tests {
+    use std::convert::TryInto;
+
+    use ssz::ssz_encode;
+
+    use super::super::Keypair;
+    use super::*;
+
+    #[test]
+    pub fn test_valid_public_key() {
+        let keypair = Keypair::random();
+
+        let bytes = ssz_encode(&keypair.pk);
+        let public_key_bytes = PublicKeyBytes::from_bytes(&bytes).unwrap();
+        let public_key: Result<PublicKey, _> = (&public_key_bytes).try_into();
+        assert!(public_key.is_ok());
+        assert_eq!(keypair.pk, public_key.unwrap());
+    }
+
+    #[test]
+    pub fn test_invalid_public_key() {
+        let mut public_key_bytes = [0; BLS_PUBLIC_KEY_BYTE_SIZE];
+        public_key_bytes[0] = 255; //a_flag1 == b_flag1 == c_flag1 == 1 and x1 = 0 shouldn't be allowed
+        let public_key_bytes = PublicKeyBytes::from_bytes(&public_key_bytes[..]);
+        assert!(public_key_bytes.is_ok());
+
+        let public_key: Result<PublicKey, _> = public_key_bytes.as_ref().unwrap().try_into();
+        assert!(public_key.is_err());
+    }
+}

eth2/utils/bls/src/signature.rs

@@ -155,6 +155,15 @@ mod tests {
         assert_eq!(original, decoded);
     }
 
+    #[test]
+    pub fn test_byte_size() {
+        let keypair = Keypair::random();
+
+        let signature = Signature::new(&[42, 42], 0, &keypair.sk);
+        let bytes = ssz_encode(&signature);
+        assert_eq!(bytes.len(), BLS_SIG_BYTE_SIZE);
+    }
+
     #[test]
     // TODO: once `CachedTreeHash` is fixed, this test should _not_ panic.
     #[should_panic]

eth2/utils/bls/src/signature_bytes.rs

@@ -0,0 +1,44 @@
+use ssz::{Decode, DecodeError, Encode};
+
+use super::{Signature, BLS_SIG_BYTE_SIZE};
+
+bytes_struct!(
+    SignatureBytes,
+    Signature,
+    BLS_SIG_BYTE_SIZE,
+    "signature",
+    U96
+);
+
+#[cfg(test)]
+mod tests {
+    use std::convert::TryInto;
+
+    use ssz::ssz_encode;
+
+    use super::super::Keypair;
+    use super::*;
+
+    #[test]
+    pub fn test_valid_signature() {
+        let keypair = Keypair::random();
+        let original = Signature::new(&[42, 42], 0, &keypair.sk);
+
+        let bytes = ssz_encode(&original);
+        let signature_bytes = SignatureBytes::from_bytes(&bytes).unwrap();
+        let signature: Result<Signature, _> = (&signature_bytes).try_into();
+        assert!(signature.is_ok());
+        assert_eq!(original, signature.unwrap());
+    }
+
+    #[test]
+    pub fn test_invalid_signature() {
+        let mut signature_bytes = [0; BLS_SIG_BYTE_SIZE];
+        signature_bytes[0] = 255; //a_flag1 == b_flag1 == c_flag1 == 1 and x1 = 0 shouldn't be allowed
+        let signature_bytes = SignatureBytes::from_bytes(&signature_bytes[..]);
+        assert!(signature_bytes.is_ok());
+
+        let signature: Result<Signature, _> = signature_bytes.as_ref().unwrap().try_into();
+        assert!(signature.is_err());
+    }
+}