From 6ef4268d6d160d30348ca44fa4ec1111245c71a2 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Fri, 12 Oct 2018 00:41:47 +1100 Subject: [PATCH] Verify attestation justified_block_hash. Previously there was not a check that the hash was in the chain, just that it was known (in the database in any chain) --- .../validation/src/attestation_validation.rs | 66 +++++++++++++------ .../validation/src/block_validation.rs | 2 +- .../tests/attestation_validation/helpers.rs | 64 +++++++++++++++--- .../tests/attestation_validation/tests.rs | 47 +++++++++++-- .../tests/block_validation/helpers.rs | 22 ++++++- .../validation/tests/block_validation/mod.rs | 2 +- 6 files changed, 167 insertions(+), 36 deletions(-) diff --git a/beacon_chain/validation/src/attestation_validation.rs b/beacon_chain/validation/src/attestation_validation.rs index 2ba81fd1d..5d492719f 100644 --- a/beacon_chain/validation/src/attestation_validation.rs +++ b/beacon_chain/validation/src/attestation_validation.rs @@ -12,7 +12,11 @@ use super::db::{ ClientDB, DBError }; -use super::db::stores::ValidatorStore; +use super::db::stores::{ + BlockStore, + BlockAtSlotError, + ValidatorStore, +}; use super::types::{ Hash256, }; @@ -27,7 +31,7 @@ pub enum AttestationValidationError { SlotTooHigh, SlotTooLow, JustifiedSlotIncorrect, - UnknownJustifiedBlock, + InvalidJustifiedBlockHash, TooManyObliqueHashes, BadCurrentHashes, BadObliqueHashes, @@ -54,10 +58,10 @@ pub struct AttestationValidationContext pub cycle_length: u8, /// The last justified slot as per the client's view of the canonical chain. pub last_justified_slot: u64, - /// The last justified block hash as per the client's view of the canonical chain. - pub last_justified_block_hash: Hash256, /// A vec of the hashes of the blocks preceeding the present slot. pub parent_hashes: Arc>, + /// The store containing block information. + pub block_store: Arc>, /// The store containing validator information. pub validator_store: Arc>, /// A map of (slot, shard_id) to the attestation set of validation indices. @@ -97,18 +101,10 @@ impl AttestationValidationContext * The attestation must indicate that its last justified slot is the same as the last justified * slot known to us. */ - if a.justified_slot != self.last_justified_slot { + if a.justified_slot > self.last_justified_slot { return Err(AttestationValidationError::JustifiedSlotIncorrect); } - /* - * The specified justified block hash supplied in the attestation must match our knowledge - * of the last justified block this chain. - */ - if a.justified_block_hash != self.last_justified_block_hash { - return Err(AttestationValidationError::UnknownJustifiedBlock) - } - /* * There is no need to include more oblique parents hashes than there are blocks * in a cycle. @@ -147,13 +143,35 @@ impl AttestationValidationContext return Err(AttestationValidationError::InvalidBitfieldEndBits) } + /* + * Generate the parent hashes for this attestation + */ + let parent_hashes = attestation_parent_hashes( + self.cycle_length, + self.block_slot, + a.slot, + &self.parent_hashes, + &a.oblique_parent_hashes)?; + + /* + * The specified justified block hash supplied in the attestation must be in the chain at + * the given slot number. + * + * First, we find the latest parent hash from the parent_hashes array. Then, using the + * block store (database) we iterate back through the blocks until we find (or fail to + * find) the justified block hash referenced in the attestation record. + */ + let latest_parent_hash = parent_hashes.last() + .ok_or(AttestationValidationError::BadCurrentHashes)?; + match self.block_store.block_at_slot(&latest_parent_hash, a.justified_slot)? { + Some((ref hash, _)) if *hash == a.justified_block_hash.to_vec() => (), + _ => return Err(AttestationValidationError::InvalidJustifiedBlockHash) + }; + + /* + * Generate the message that this attestation aggregate signature must sign across. + */ let signed_message = { - let parent_hashes = attestation_parent_hashes( - self.cycle_length, - self.block_slot, - a.slot, - &self.parent_hashes, - &a.oblique_parent_hashes)?; generate_signed_message( a.slot, &parent_hashes, @@ -201,6 +219,16 @@ impl From for AttestationValidationError { } } +impl From for AttestationValidationError { + fn from(e: BlockAtSlotError) -> Self { + match e { + BlockAtSlotError::DBError(s) => AttestationValidationError::DBError(s), + _ => AttestationValidationError::InvalidJustifiedBlockHash + + } + } +} + impl From for AttestationValidationError { fn from(e: DBError) -> Self { AttestationValidationError::DBError(e.message) diff --git a/beacon_chain/validation/src/block_validation.rs b/beacon_chain/validation/src/block_validation.rs index 51db94059..a663fb9a0 100644 --- a/beacon_chain/validation/src/block_validation.rs +++ b/beacon_chain/validation/src/block_validation.rs @@ -207,8 +207,8 @@ impl BlockValidationContext block_slot, cycle_length: self.cycle_length, last_justified_slot: self.last_justified_slot, - last_justified_block_hash: self.last_justified_block_hash, parent_hashes: self.parent_hashes.clone(), + block_store: self.block_store.clone(), validator_store: self.validator_store.clone(), attester_map: self.attester_map.clone(), }); diff --git a/beacon_chain/validation/tests/attestation_validation/helpers.rs b/beacon_chain/validation/tests/attestation_validation/helpers.rs index f263fe06e..e768638db 100644 --- a/beacon_chain/validation/tests/attestation_validation/helpers.rs +++ b/beacon_chain/validation/tests/attestation_validation/helpers.rs @@ -3,11 +3,15 @@ use std::sync::Arc; use super::db::{ MemoryDB, }; -use super::db::stores::ValidatorStore; +use super::db::stores::{ + ValidatorStore, + BlockStore, +}; use super::types::{ AttestationRecord, AttesterMap, Bitfield, + Block, Hash256, }; use super::validation::attestation_validation::{ @@ -28,15 +32,18 @@ use super::hashing::{ pub struct TestStore { pub db: Arc, pub validator: Arc>, + pub block: Arc>, } impl TestStore { pub fn new() -> Self { let db = Arc::new(MemoryDB::open()); let validator = Arc::new(ValidatorStore::new(db.clone())); + let block = Arc::new(BlockStore::new(db.clone())); Self { db, validator, + block, } } } @@ -73,7 +80,8 @@ pub fn generate_attestation(shard_id: u16, justified_block_hash: &Hash256, cycle_length: u8, parent_hashes: &[Hash256], - signing_keys: &[Option]) + signing_keys: &[Option], + block_store: &BlockStore) -> AttestationRecord { let mut attester_bitfield = Bitfield::new(); @@ -86,6 +94,11 @@ pub fn generate_attestation(shard_id: u16, &parent_hashes[first..last] }; + /* + * Create a justified block at the correct slot and store it in the db. + */ + create_block_at_slot(&block_store, &justified_block_hash, justified_slot); + /* * Generate the message that will be signed across for this attr record. */ @@ -120,6 +133,31 @@ pub fn generate_attestation(shard_id: u16, } } +/// Create a minimum viable block at some slot. +/// +/// Allows the validation function to read the block and verify its slot. +pub fn create_block_at_slot(block_store: &BlockStore, hash: &Hash256, slot: u64) { + let mut justified_block = Block::zero(); + justified_block.attestations.push(AttestationRecord::zero()); + justified_block.slot_number = slot; + let mut s = SszStream::new(); + s.append(&justified_block); + let justified_block_ssz = s.drain(); + block_store.put_serialized_block(&hash.to_vec(), &justified_block_ssz).unwrap(); +} + +/// Inserts a justified_block_hash in a position that will be referenced by an attestation record. +pub fn insert_justified_block_hash( + parent_hashes: &mut Vec, + justified_block_hash: &Hash256, + block_slot: u64, + attestation_slot: u64) +{ + let attestation_parent_hash_index = parent_hashes.len() - 1 - + (block_slot as usize - attestation_slot as usize); + parent_hashes[attestation_parent_hash_index] = justified_block_hash.clone(); +} + pub fn setup_attestation_validation_test(shard_id: u16, attester_count: usize) -> TestRig { @@ -127,15 +165,24 @@ pub fn setup_attestation_validation_test(shard_id: u16, attester_count: usize) let block_slot = 10000; let cycle_length: u8 = 64; - let last_justified_slot = block_slot - u64::from(cycle_length); - let parent_hashes: Vec = (0..(cycle_length * 2)) + let mut parent_hashes: Vec = (0..(cycle_length * 2)) .map(|i| Hash256::from(i as u64)) .collect(); - let parent_hashes = Arc::new(parent_hashes); + let attestation_slot = block_slot - 1; + let last_justified_slot = attestation_slot - 1; let justified_block_hash = Hash256::from("justified_block".as_bytes()); let shard_block_hash = Hash256::from("shard_block".as_bytes()); - let attestation_slot = block_slot - 1; + /* + * Insert the required justified_block_hash into parent_hashes + */ + insert_justified_block_hash( + &mut parent_hashes, + &justified_block_hash, + block_slot, + attestation_slot); + + let parent_hashes = Arc::new(parent_hashes); let mut keypairs = vec![]; let mut signing_keys = vec![]; @@ -159,8 +206,8 @@ pub fn setup_attestation_validation_test(shard_id: u16, attester_count: usize) block_slot, cycle_length, last_justified_slot, - last_justified_block_hash: justified_block_hash, parent_hashes: parent_hashes.clone(), + block_store: stores.block.clone(), validator_store: stores.validator.clone(), attester_map: Arc::new(attester_map), }; @@ -173,7 +220,8 @@ pub fn setup_attestation_validation_test(shard_id: u16, attester_count: usize) &justified_block_hash, cycle_length, &parent_hashes.clone(), - &signing_keys); + &signing_keys, + &stores.block); TestRig { attestation, diff --git a/beacon_chain/validation/tests/attestation_validation/tests.rs b/beacon_chain/validation/tests/attestation_validation/tests.rs index 50ccf32ef..06b598e93 100644 --- a/beacon_chain/validation/tests/attestation_validation/tests.rs +++ b/beacon_chain/validation/tests/attestation_validation/tests.rs @@ -3,6 +3,7 @@ use std::sync::Arc; use super::helpers::{ TestRig, setup_attestation_validation_test, + create_block_at_slot, }; use super::validation::attestation_validation::{ AttestationValidationError, @@ -56,12 +57,25 @@ fn test_attestation_validation_invalid_justified_slot_incorrect() { let original = rig.attestation.justified_slot; rig.attestation.justified_slot = original - 1; + // Ensures we don't get a bad justified block error instead. + create_block_at_slot( + &rig.stores.block, + &rig.attestation.justified_block_hash, + rig.attestation.justified_slot); let result = rig.context.validate_attestation(&rig.attestation); - assert_eq!(result, Err(AttestationValidationError::JustifiedSlotIncorrect)); + assert_eq!(result, Err(AttestationValidationError::BadAggregateSignature)); rig.attestation.justified_slot = original + 1; + // Ensures we don't get a bad justified block error instead. + create_block_at_slot( + &rig.stores.block, + &rig.attestation.justified_block_hash, + rig.attestation.justified_slot); + // Ensures we don't get an error that the last justified slot is ahead of the context justified + // slot. + rig.context.last_justified_slot = rig.attestation.justified_slot; let result = rig.context.validate_attestation(&rig.attestation); - assert_eq!(result, Err(AttestationValidationError::JustifiedSlotIncorrect)); + assert_eq!(result, Err(AttestationValidationError::BadAggregateSignature)); } #[test] @@ -139,13 +153,38 @@ fn test_attestation_validation_invalid_invalid_bitfield_end_bit_with_irreguar_bi } #[test] -fn test_attestation_validation_invalid_unknown_justfied_block_hash() { +fn test_attestation_validation_invalid_unknown_justified_block_hash() { let mut rig = generic_rig(); rig.attestation.justified_block_hash = Hash256::from("unknown block hash".as_bytes()); let result = rig.context.validate_attestation(&rig.attestation); - assert_eq!(result, Err(AttestationValidationError::UnknownJustifiedBlock)); + assert_eq!(result, Err(AttestationValidationError::InvalidJustifiedBlockHash)); +} + +#[test] +fn test_attestation_validation_invalid_unknown_justified_block_hash_wrong_slot() { + let rig = generic_rig(); + + /* + * justified_block_hash points to a block with a slot that is too high. + */ + create_block_at_slot( + &rig.stores.block, + &rig.attestation.justified_block_hash, + rig.attestation.justified_slot + 1); + let result = rig.context.validate_attestation(&rig.attestation); + assert_eq!(result, Err(AttestationValidationError::InvalidJustifiedBlockHash)); + + /* + * justified_block_hash points to a block with a slot that is too low. + */ + create_block_at_slot( + &rig.stores.block, + &rig.attestation.justified_block_hash, + rig.attestation.justified_slot - 1); + let result = rig.context.validate_attestation(&rig.attestation); + assert_eq!(result, Err(AttestationValidationError::InvalidJustifiedBlockHash)); } #[test] diff --git a/beacon_chain/validation/tests/block_validation/helpers.rs b/beacon_chain/validation/tests/block_validation/helpers.rs index a79e3d97c..83c806af9 100644 --- a/beacon_chain/validation/tests/block_validation/helpers.rs +++ b/beacon_chain/validation/tests/block_validation/helpers.rs @@ -1,6 +1,9 @@ use std::sync::Arc; -use super::generate_attestation; +use super::attestation_validation::helpers::{ + generate_attestation, + insert_justified_block_hash, +}; use super::bls::{ Keypair, }; @@ -82,7 +85,7 @@ pub fn setup_block_validation_scenario(params: &BlockTestParams) let block_slot = params.block_slot; let attestations_justified_slot = params.attestations_justified_slot; - let parent_hashes: Vec = (0..(cycle_length * 2)) + let mut parent_hashes: Vec = (0..(cycle_length * 2)) .map(|i| Hash256::from(i as u64)) .collect(); let parent_hash = Hash256::from("parent_hash".as_bytes()); @@ -93,6 +96,9 @@ pub fn setup_block_validation_scenario(params: &BlockTestParams) let crystallized_state_root = Hash256::from("cry_state".as_bytes()); let shard_block_hash = Hash256::from("shard_block_hash".as_bytes()); + /* + * Store a valid PoW chain ref + */ stores.pow_chain.put_block_hash(pow_chain_ref.as_ref()).unwrap(); /* @@ -117,6 +123,15 @@ pub fn setup_block_validation_scenario(params: &BlockTestParams) let mut attester_map = AttesterMap::new(); let mut attestations = vec![]; let mut keypairs = vec![]; + + /* + * Insert the required justified_block_hash into parent_hashes + */ + insert_justified_block_hash( + &mut parent_hashes, + &justified_block_hash, + block_slot, + attestation_slot); /* * For each shard in this slot, generate an attestation. */ @@ -146,7 +161,8 @@ pub fn setup_block_validation_scenario(params: &BlockTestParams) &justified_block_hash, cycle_length, &parent_hashes, - &signing_keys[..]); + &signing_keys[..], + &stores.block); attestations.push(attestation); } (attester_map, attestations, keypairs) diff --git a/beacon_chain/validation/tests/block_validation/mod.rs b/beacon_chain/validation/tests/block_validation/mod.rs index 83431606c..0ec0f17ee 100644 --- a/beacon_chain/validation/tests/block_validation/mod.rs +++ b/beacon_chain/validation/tests/block_validation/mod.rs @@ -9,4 +9,4 @@ use super::ssz_helpers; use super::types; use super::validation; -use super::attestation_validation::helpers::generate_attestation; +use super::attestation_validation;