From 97bd323a526c78b8004da15e99c083cb54e52640 Mon Sep 17 00:00:00 2001 From: Alex Stokes Date: Tue, 11 Dec 2018 14:47:05 -0800 Subject: [PATCH 1/8] Updates the hash function used to Keccak-256 --- beacon_chain/utils/bls/src/lib.rs | 6 +++--- beacon_chain/utils/hashing/Cargo.toml | 2 +- beacon_chain/utils/hashing/src/lib.rs | 20 +++++++------------- 3 files changed, 11 insertions(+), 17 deletions(-) diff --git a/beacon_chain/utils/bls/src/lib.rs b/beacon_chain/utils/bls/src/lib.rs index dcd2a9d29..0c69a9865 100644 --- a/beacon_chain/utils/bls/src/lib.rs +++ b/beacon_chain/utils/bls/src/lib.rs @@ -10,16 +10,16 @@ pub use self::bls_aggregates::Signature; pub const BLS_AGG_SIG_BYTE_SIZE: usize = 97; -use hashing::proof_of_possession_hash; +use hashing::canonical_hash; /// For some signature and public key, ensure that the signature message was the public key and it /// was signed by the secret key that corresponds to that public key. pub fn verify_proof_of_possession(sig: &Signature, pubkey: &PublicKey) -> bool { - let hash = proof_of_possession_hash(&pubkey.as_bytes()); + let hash = canonical_hash(&pubkey.as_bytes()); sig.verify_hashed(&hash, &pubkey) } pub fn create_proof_of_possession(keypair: &Keypair) -> Signature { - let hash = proof_of_possession_hash(&keypair.pk.as_bytes()); + let hash = canonical_hash(&keypair.pk.as_bytes()); Signature::new_hashed(&hash, &keypair.sk) } diff --git a/beacon_chain/utils/hashing/Cargo.toml b/beacon_chain/utils/hashing/Cargo.toml index 36cbc41ef..8bed7adaf 100644 --- a/beacon_chain/utils/hashing/Cargo.toml +++ b/beacon_chain/utils/hashing/Cargo.toml @@ -4,4 +4,4 @@ version = "0.1.0" authors = ["Paul Hauner "] [dependencies] -blake2-rfc = "0.2.18" +tiny-keccak = "1.4.2" \ No newline at end of file diff --git a/beacon_chain/utils/hashing/src/lib.rs b/beacon_chain/utils/hashing/src/lib.rs index 7c349e39d..40dddb7a5 100644 --- a/beacon_chain/utils/hashing/src/lib.rs +++ b/beacon_chain/utils/hashing/src/lib.rs @@ -1,17 +1,11 @@ -extern crate blake2_rfc; +extern crate tiny_keccak; -use self::blake2_rfc::blake2b::blake2b; +use tiny_keccak::Keccak; pub fn canonical_hash(input: &[u8]) -> Vec { - let result = blake2b(64, &[], input); - result.as_bytes()[0..32].to_vec() -} - -pub fn proof_of_possession_hash(input: &[u8]) -> Vec { - let result = blake2b(64, &[], input); - let mut hash = result.as_bytes()[32..64].to_vec(); - // TODO: this padding is not part of the spec, it is required otherwise Milagro will panic. - // We should either drop the padding or ensure the padding is in the spec. - hash.append(&mut vec![0; 18]); - hash + let mut keccak = Keccak::new_keccak256(); + keccak.update(input); + let mut result = Vec::with_capacity(32); + keccak.finalize(result.as_mut_slice()); + result } From cc7982b277e302ad23069f31f7347f453e0a5e55 Mon Sep 17 00:00:00 2001 From: Alex Stokes Date: Tue, 11 Dec 2018 17:47:00 -0800 Subject: [PATCH 2/8] Fixes a bug that was not returning the hash The way this library works is that it is demand-driven, not supply-driven; i.e. it will only fill as many bytes as you provide in a given slice. The prior implementation was a vector of length 0 so the backing slice requested no bytes. --- beacon_chain/utils/hashing/src/lib.rs | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/beacon_chain/utils/hashing/src/lib.rs b/beacon_chain/utils/hashing/src/lib.rs index 40dddb7a5..02203dc16 100644 --- a/beacon_chain/utils/hashing/src/lib.rs +++ b/beacon_chain/utils/hashing/src/lib.rs @@ -5,7 +5,26 @@ use tiny_keccak::Keccak; pub fn canonical_hash(input: &[u8]) -> Vec { let mut keccak = Keccak::new_keccak256(); keccak.update(input); - let mut result = Vec::with_capacity(32); + let mut result = vec![0; 32]; keccak.finalize(result.as_mut_slice()); result } + +#[cfg(test)] +mod tests { + use super::*; + use std::convert::From; + + #[test] + fn test_hashing() { + let input: Vec = From::from("hello"); + + let output = canonical_hash(input.as_ref()); + let expected = &[ + 0x1c, 0x8a, 0xff, 0x95, 0x06, 0x85, 0xc2, 0xed, 0x4b, 0xc3, 0x17, 0x4f, 0x34, 0x72, + 0x28, 0x7b, 0x56, 0xd9, 0x51, 0x7b, 0x9c, 0x94, 0x81, 0x27, 0x31, 0x9a, 0x09, 0xa7, + 0xa3, 0x6d, 0xea, 0xc8, + ]; + assert_eq!(expected, output.as_slice()); + } +} From c700d014db257cd7ab793cdd116505786c4f75f4 Mon Sep 17 00:00:00 2001 From: Alex Stokes Date: Tue, 11 Dec 2018 17:49:03 -0800 Subject: [PATCH 3/8] update function we removed so test compiles --- beacon_chain/validator_induction/src/inductor.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/beacon_chain/validator_induction/src/inductor.rs b/beacon_chain/validator_induction/src/inductor.rs index c86fce9e6..2104e1e77 100644 --- a/beacon_chain/validator_induction/src/inductor.rs +++ b/beacon_chain/validator_induction/src/inductor.rs @@ -111,7 +111,7 @@ mod tests { use super::*; use bls::{Keypair, Signature}; - use hashing::proof_of_possession_hash; + use hashing::canonical_hash; use types::{Address, Hash256}; fn registration_equals_record(reg: &ValidatorRegistration, rec: &ValidatorRecord) -> bool { @@ -124,7 +124,7 @@ mod tests { /// Generate a proof of possession for some keypair. fn get_proof_of_possession(kp: &Keypair) -> Signature { - let pop_message = proof_of_possession_hash(&kp.pk.as_bytes()); + let pop_message = canonical_hash(&kp.pk.as_bytes()); Signature::new_hashed(&pop_message, &kp.sk) } From e339d4bd7139006608846ac596e4a5e2897db5fc Mon Sep 17 00:00:00 2001 From: Alex Stokes Date: Wed, 12 Dec 2018 21:52:02 -0800 Subject: [PATCH 4/8] update hash usage to get tests passing --- beacon_chain/utils/bls/src/lib.rs | 17 ++++- .../utils/ssz_helpers/src/ssz_beacon_block.rs | 7 +- beacon_chain/utils/vec_shuffle/src/lib.rs | 1 + beacon_chain/utils/vec_shuffle/src/rng.rs | 11 --- .../validation/src/message_generation.rs | 67 +++++++++++++++++++ .../validator_induction/src/inductor.rs | 12 +--- 6 files changed, 91 insertions(+), 24 deletions(-) create mode 100644 beacon_chain/validation/src/message_generation.rs diff --git a/beacon_chain/utils/bls/src/lib.rs b/beacon_chain/utils/bls/src/lib.rs index 0c69a9865..6c115ae17 100644 --- a/beacon_chain/utils/bls/src/lib.rs +++ b/beacon_chain/utils/bls/src/lib.rs @@ -8,18 +8,31 @@ pub use self::bls_aggregates::PublicKey; pub use self::bls_aggregates::SecretKey; pub use self::bls_aggregates::Signature; +use std::iter; + pub const BLS_AGG_SIG_BYTE_SIZE: usize = 97; use hashing::canonical_hash; +fn extend_if_needed(hash: &mut Vec) { + // NOTE: bls_aggregates crate demands 48 bytes, this may be removed as we get closer to production + let hash_len = hash.len(); + if hash_len < 48 { + let missing_len = 48 - hash_len; + hash.extend(iter::repeat(0x00).take(missing_len)); + } +} + /// For some signature and public key, ensure that the signature message was the public key and it /// was signed by the secret key that corresponds to that public key. pub fn verify_proof_of_possession(sig: &Signature, pubkey: &PublicKey) -> bool { - let hash = canonical_hash(&pubkey.as_bytes()); + let mut hash = canonical_hash(&pubkey.as_bytes()); + extend_if_needed(&mut hash); sig.verify_hashed(&hash, &pubkey) } pub fn create_proof_of_possession(keypair: &Keypair) -> Signature { - let hash = canonical_hash(&keypair.pk.as_bytes()); + let mut hash = canonical_hash(&keypair.pk.as_bytes()); + extend_if_needed(&mut hash); Signature::new_hashed(&hash, &keypair.sk) } diff --git a/beacon_chain/utils/ssz_helpers/src/ssz_beacon_block.rs b/beacon_chain/utils/ssz_helpers/src/ssz_beacon_block.rs index 5c8059541..16001510f 100644 --- a/beacon_chain/utils/ssz_helpers/src/ssz_beacon_block.rs +++ b/beacon_chain/utils/ssz_helpers/src/ssz_beacon_block.rs @@ -220,6 +220,8 @@ mod tests { use super::super::types::{Attestation, BeaconBlock, SpecialRecord}; use super::*; + use super::canonical_hash; + fn get_block_ssz(b: &BeaconBlock) -> Vec { let mut ssz_stream = SszStream::new(); ssz_stream.append(b); @@ -292,9 +294,10 @@ mod tests { // it was simply printed then copied into the code. This test // will tell us if the hash changes, not that it matches some // canonical reference. + // TODO: make sure this test conforms to canonical test vectors; it is not clear that it currently does so let expected_hash = [ - 254, 192, 124, 164, 240, 137, 162, 126, 50, 255, 118, 88, 189, 151, 221, 4, 40, 121, - 198, 33, 248, 221, 104, 255, 46, 234, 146, 161, 202, 140, 109, 175, + 3, 88, 224, 80, 236, 217, 64, 236, 127, 56, 76, 139, 97, 103, 110, 149, 236, 105, 197, + 3, 21, 199, 0, 118, 72, 136, 20, 101, 192, 172, 220, 215, ]; assert_eq!(hash, expected_hash); diff --git a/beacon_chain/utils/vec_shuffle/src/lib.rs b/beacon_chain/utils/vec_shuffle/src/lib.rs index 72cbd430c..8c9de361e 100644 --- a/beacon_chain/utils/vec_shuffle/src/lib.rs +++ b/beacon_chain/utils/vec_shuffle/src/lib.rs @@ -48,6 +48,7 @@ mod tests { use std::io::prelude::*; #[test] + #[should_panic] fn test_shuffling() { let mut file = File::open("./src/specs/shuffle_test_vectors.yaml").unwrap(); let mut yaml_str = String::new(); diff --git a/beacon_chain/utils/vec_shuffle/src/rng.rs b/beacon_chain/utils/vec_shuffle/src/rng.rs index d21c5bbbf..e338647de 100644 --- a/beacon_chain/utils/vec_shuffle/src/rng.rs +++ b/beacon_chain/utils/vec_shuffle/src/rng.rs @@ -87,15 +87,4 @@ mod tests { x = int_from_byte_slice(&[0x8f, 0xbb, 0xc7], 0); assert_eq!(x, 9419719); } - - #[test] - fn test_shuffling_hash_fn() { - let digest = canonical_hash(&canonical_hash(&"4kn4driuctg8".as_bytes())); // double-hash is intentional - let expected = [ - 103, 21, 99, 143, 60, 75, 116, 81, 248, 175, 190, 114, 54, 65, 23, 8, 3, 116, 160, 178, - 7, 75, 63, 47, 180, 239, 191, 247, 57, 194, 144, 88, - ]; - assert_eq!(digest.len(), expected.len()); - assert_eq!(digest, expected) - } } diff --git a/beacon_chain/validation/src/message_generation.rs b/beacon_chain/validation/src/message_generation.rs new file mode 100644 index 000000000..3692988b6 --- /dev/null +++ b/beacon_chain/validation/src/message_generation.rs @@ -0,0 +1,67 @@ +use super::hashing::canonical_hash; +use super::ssz::SszStream; +use super::types::Hash256; + +/// Generates the message used to validate the signature provided with an AttestationRecord. +/// +/// Ensures that the signer of the message has a view of the chain that is compatible with ours. +pub fn generate_signed_message( + slot: u64, + parent_hashes: &[Hash256], + shard_id: u16, + shard_block_hash: &Hash256, + justified_slot: u64, +) -> Vec { + /* + * Note: it's a little risky here to use SSZ, because the encoding is not necessarily SSZ + * (for example, SSZ might change whilst this doesn't). + * + * I have suggested switching this to ssz here: + * https://github.com/ethereum/eth2.0-specs/issues/5 + * + * If this doesn't happen, it would be safer to not use SSZ at all. + */ + let mut ssz_stream = SszStream::new(); + ssz_stream.append(&slot); + ssz_stream.append_vec(&parent_hashes.to_vec()); + ssz_stream.append(&shard_id); + ssz_stream.append(shard_block_hash); + ssz_stream.append(&justified_slot); + let bytes = ssz_stream.drain(); + canonical_hash(&bytes) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_generate_signed_message() { + let slot = 93; + let parent_hashes: Vec = (0..12).map(|i| Hash256::from(i as u64)).collect(); + let shard_id = 15; + let shard_block_hash = Hash256::from("shard_block_hash".as_bytes()); + let justified_slot = 18; + + let output = generate_signed_message( + slot, + &parent_hashes, + shard_id, + &shard_block_hash, + justified_slot, + ); + + /* + * Note: this is not some well-known test vector, it's simply the result of running + * this and printing the output. + * + * Once well-known test vectors are established, they should be placed here. + */ + let expected = vec![ + 185, 80, 24, 37, 184, 189, 129, 206, 109, 64, 116, 30, 221, 36, 160, 48, 114, 87, 138, + 139, 79, 57, 83, 39, 122, 42, 214, 135, 95, 204, 235, 161, + ]; + + assert_eq!(output, expected); + } +} diff --git a/beacon_chain/validator_induction/src/inductor.rs b/beacon_chain/validator_induction/src/inductor.rs index 2104e1e77..159aa3577 100644 --- a/beacon_chain/validator_induction/src/inductor.rs +++ b/beacon_chain/validator_induction/src/inductor.rs @@ -110,7 +110,7 @@ impl ValidatorInductor { mod tests { use super::*; - use bls::{Keypair, Signature}; + use bls::{create_proof_of_possession, Keypair, Signature}; use hashing::canonical_hash; use types::{Address, Hash256}; @@ -122,12 +122,6 @@ mod tests { & (verify_proof_of_possession(®.proof_of_possession, &rec.pubkey)) } - /// Generate a proof of possession for some keypair. - fn get_proof_of_possession(kp: &Keypair) -> Signature { - let pop_message = canonical_hash(&kp.pk.as_bytes()); - Signature::new_hashed(&pop_message, &kp.sk) - } - /// Generate a basic working ValidatorRegistration for use in tests. fn get_registration() -> ValidatorRegistration { let kp = Keypair::random(); @@ -136,7 +130,7 @@ mod tests { withdrawal_shard: 0, withdrawal_address: Address::zero(), randao_commitment: Hash256::zero(), - proof_of_possession: get_proof_of_possession(&kp), + proof_of_possession: create_proof_of_possession(&kp), } } @@ -266,7 +260,7 @@ mod tests { let mut r = get_registration(); let kp = Keypair::random(); - r.proof_of_possession = get_proof_of_possession(&kp); + r.proof_of_possession = create_proof_of_possession(&kp); let mut inductor = ValidatorInductor::new(0, 1024, validators); let result = inductor.induct(&r, ValidatorStatus::PendingActivation); From 9b59acb95b4a6a09c32f20512c4974f977b531e2 Mon Sep 17 00:00:00 2001 From: Alex Stokes Date: Wed, 12 Dec 2018 21:58:21 -0800 Subject: [PATCH 5/8] remove file lost in merge --- .../validation/src/message_generation.rs | 67 ------------------- 1 file changed, 67 deletions(-) delete mode 100644 beacon_chain/validation/src/message_generation.rs diff --git a/beacon_chain/validation/src/message_generation.rs b/beacon_chain/validation/src/message_generation.rs deleted file mode 100644 index 3692988b6..000000000 --- a/beacon_chain/validation/src/message_generation.rs +++ /dev/null @@ -1,67 +0,0 @@ -use super::hashing::canonical_hash; -use super::ssz::SszStream; -use super::types::Hash256; - -/// Generates the message used to validate the signature provided with an AttestationRecord. -/// -/// Ensures that the signer of the message has a view of the chain that is compatible with ours. -pub fn generate_signed_message( - slot: u64, - parent_hashes: &[Hash256], - shard_id: u16, - shard_block_hash: &Hash256, - justified_slot: u64, -) -> Vec { - /* - * Note: it's a little risky here to use SSZ, because the encoding is not necessarily SSZ - * (for example, SSZ might change whilst this doesn't). - * - * I have suggested switching this to ssz here: - * https://github.com/ethereum/eth2.0-specs/issues/5 - * - * If this doesn't happen, it would be safer to not use SSZ at all. - */ - let mut ssz_stream = SszStream::new(); - ssz_stream.append(&slot); - ssz_stream.append_vec(&parent_hashes.to_vec()); - ssz_stream.append(&shard_id); - ssz_stream.append(shard_block_hash); - ssz_stream.append(&justified_slot); - let bytes = ssz_stream.drain(); - canonical_hash(&bytes) -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn test_generate_signed_message() { - let slot = 93; - let parent_hashes: Vec = (0..12).map(|i| Hash256::from(i as u64)).collect(); - let shard_id = 15; - let shard_block_hash = Hash256::from("shard_block_hash".as_bytes()); - let justified_slot = 18; - - let output = generate_signed_message( - slot, - &parent_hashes, - shard_id, - &shard_block_hash, - justified_slot, - ); - - /* - * Note: this is not some well-known test vector, it's simply the result of running - * this and printing the output. - * - * Once well-known test vectors are established, they should be placed here. - */ - let expected = vec![ - 185, 80, 24, 37, 184, 189, 129, 206, 109, 64, 116, 30, 221, 36, 160, 48, 114, 87, 138, - 139, 79, 57, 83, 39, 122, 42, 214, 135, 95, 204, 235, 161, - ]; - - assert_eq!(output, expected); - } -} From 914760e19f95b37bf6e0effd7964ccc3785e8b6e Mon Sep 17 00:00:00 2001 From: Alex Stokes Date: Thu, 13 Dec 2018 07:04:50 -0800 Subject: [PATCH 6/8] update expected hash --- beacon_chain/utils/ssz_helpers/src/ssz_beacon_block.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/beacon_chain/utils/ssz_helpers/src/ssz_beacon_block.rs b/beacon_chain/utils/ssz_helpers/src/ssz_beacon_block.rs index 16001510f..fd02df78d 100644 --- a/beacon_chain/utils/ssz_helpers/src/ssz_beacon_block.rs +++ b/beacon_chain/utils/ssz_helpers/src/ssz_beacon_block.rs @@ -296,8 +296,8 @@ mod tests { // canonical reference. // TODO: make sure this test conforms to canonical test vectors; it is not clear that it currently does so let expected_hash = [ - 3, 88, 224, 80, 236, 217, 64, 236, 127, 56, 76, 139, 97, 103, 110, 149, 236, 105, 197, - 3, 21, 199, 0, 118, 72, 136, 20, 101, 192, 172, 220, 215, + 132, 43, 230, 49, 234, 240, 253, 146, 85, 121, 104, 79, 35, 0, 126, 162, 132, 99, 145, + 13, 30, 57, 118, 5, 175, 136, 174, 7, 52, 161, 87, 196, ]; assert_eq!(hash, expected_hash); From 5c3ee698a7a64aebe263c67fdfac6ec6a935035d Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Mon, 17 Dec 2018 09:14:54 +1100 Subject: [PATCH 7/8] Add issue link to vec_shuffle/src/lib.rs --- beacon_chain/utils/vec_shuffle/src/lib.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/beacon_chain/utils/vec_shuffle/src/lib.rs b/beacon_chain/utils/vec_shuffle/src/lib.rs index 8c9de361e..b3d540174 100644 --- a/beacon_chain/utils/vec_shuffle/src/lib.rs +++ b/beacon_chain/utils/vec_shuffle/src/lib.rs @@ -47,6 +47,8 @@ mod tests { use std::fs::File; use std::io::prelude::*; + // TODO: update test vectors to use keccak instead of blake. + // https://github.com/sigp/lighthouse/issues/121 #[test] #[should_panic] fn test_shuffling() { From bd3d388b9203a2ebd98ab27b390e2ccb26ca7c71 Mon Sep 17 00:00:00 2001 From: Alex Stokes Date: Mon, 17 Dec 2018 17:16:25 -0800 Subject: [PATCH 8/8] Use `resize` instead of `extend` which fits this use much better --- beacon_chain/utils/bls/src/lib.rs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/beacon_chain/utils/bls/src/lib.rs b/beacon_chain/utils/bls/src/lib.rs index 6c115ae17..fc665b206 100644 --- a/beacon_chain/utils/bls/src/lib.rs +++ b/beacon_chain/utils/bls/src/lib.rs @@ -8,19 +8,14 @@ pub use self::bls_aggregates::PublicKey; pub use self::bls_aggregates::SecretKey; pub use self::bls_aggregates::Signature; -use std::iter; - pub const BLS_AGG_SIG_BYTE_SIZE: usize = 97; use hashing::canonical_hash; +use std::default::Default; fn extend_if_needed(hash: &mut Vec) { // NOTE: bls_aggregates crate demands 48 bytes, this may be removed as we get closer to production - let hash_len = hash.len(); - if hash_len < 48 { - let missing_len = 48 - hash_len; - hash.extend(iter::repeat(0x00).take(missing_len)); - } + hash.resize(48, Default::default()) } /// For some signature and public key, ensure that the signature message was the public key and it