From cf9f8c1e8594363eaa14957c4601997a16ec11e5 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Tue, 9 Oct 2018 12:14:59 +1100 Subject: [PATCH] Fix issue with last_justified_block_hash Previously we were just checking it exists in the DB. This is incorrect because the last_justified_block_hash _must_ be in the chain referenced by the block. I.e., it's not OK for a block to reference a justified block in another chain. --- .../validation/src/attestation_validation.rs | 24 +++++++++---------- .../validation/src/block_validation.rs | 4 +++- .../tests/attestation_validation/helpers.rs | 12 ++-------- .../tests/block_validation/helpers.rs | 3 ++- .../tests/block_validation/tests.rs | 2 ++ 5 files changed, 20 insertions(+), 25 deletions(-) diff --git a/beacon_chain/validation/src/attestation_validation.rs b/beacon_chain/validation/src/attestation_validation.rs index c23b21b6e..2ba81fd1d 100644 --- a/beacon_chain/validation/src/attestation_validation.rs +++ b/beacon_chain/validation/src/attestation_validation.rs @@ -12,10 +12,7 @@ use super::db::{ ClientDB, DBError }; -use super::db::stores::{ - BlockStore, - ValidatorStore, -}; +use super::db::stores::ValidatorStore; use super::types::{ Hash256, }; @@ -57,10 +54,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. @@ -104,6 +101,14 @@ impl AttestationValidationContext 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. @@ -142,13 +147,6 @@ impl AttestationValidationContext return Err(AttestationValidationError::InvalidBitfieldEndBits) } - /* - * The specified justified block hash must be known to us - */ - if !self.block_store.block_exists(&a.justified_block_hash)? { - return Err(AttestationValidationError::UnknownJustifiedBlock) - } - let signed_message = { let parent_hashes = attestation_parent_hashes( self.cycle_length, diff --git a/beacon_chain/validation/src/block_validation.rs b/beacon_chain/validation/src/block_validation.rs index 459320f8c..51db94059 100644 --- a/beacon_chain/validation/src/block_validation.rs +++ b/beacon_chain/validation/src/block_validation.rs @@ -72,6 +72,8 @@ pub struct BlockValidationContext 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, /// The last finalized slot as per the client's view of the canonical chain. pub last_finalized_slot: u64, /// A vec of the hashes of the blocks preceeding the present slot. @@ -205,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 7c8ba2085..f263fe06e 100644 --- a/beacon_chain/validation/tests/attestation_validation/helpers.rs +++ b/beacon_chain/validation/tests/attestation_validation/helpers.rs @@ -3,10 +3,7 @@ use std::sync::Arc; use super::db::{ MemoryDB, }; -use super::db::stores::{ - BlockStore, - ValidatorStore, -}; +use super::db::stores::ValidatorStore; use super::types::{ AttestationRecord, AttesterMap, @@ -30,18 +27,15 @@ use super::hashing::{ pub struct TestStore { pub db: Arc, - pub block: Arc>, pub validator: Arc>, } impl TestStore { pub fn new() -> Self { let db = Arc::new(MemoryDB::open()); - let block = Arc::new(BlockStore::new(db.clone())); let validator = Arc::new(ValidatorStore::new(db.clone())); Self { db, - block, validator, } } @@ -141,8 +135,6 @@ pub fn setup_attestation_validation_test(shard_id: u16, attester_count: usize) let justified_block_hash = Hash256::from("justified_block".as_bytes()); let shard_block_hash = Hash256::from("shard_block".as_bytes()); - stores.block.put_serialized_block(&justified_block_hash.as_ref(), &[42]).unwrap(); - let attestation_slot = block_slot - 1; let mut keypairs = vec![]; @@ -167,8 +159,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), }; diff --git a/beacon_chain/validation/tests/block_validation/helpers.rs b/beacon_chain/validation/tests/block_validation/helpers.rs index e04e6fac8..a79e3d97c 100644 --- a/beacon_chain/validation/tests/block_validation/helpers.rs +++ b/beacon_chain/validation/tests/block_validation/helpers.rs @@ -41,6 +41,7 @@ pub struct BlockTestParams { pub parent_proposer_index: usize, pub validation_context_slot: u64, pub validation_context_justified_slot: u64, + pub validation_context_justified_block_hash: Hash256, pub validation_context_finalized_slot: u64, } @@ -93,7 +94,6 @@ pub fn setup_block_validation_scenario(params: &BlockTestParams) let shard_block_hash = Hash256::from("shard_block_hash".as_bytes()); stores.pow_chain.put_block_hash(pow_chain_ref.as_ref()).unwrap(); - stores.block.put_serialized_block(justified_block_hash.as_ref(), &vec![42]).unwrap(); /* * Generate a minimum viable parent block and store it in the database. @@ -205,6 +205,7 @@ pub fn run_block_validation_scenario( present_slot: params.validation_context_slot, cycle_length: params.cycle_length, last_justified_slot: params.validation_context_justified_slot, + last_justified_block_hash: params.validation_context_justified_block_hash, last_finalized_slot: params.validation_context_finalized_slot, parent_hashes: Arc::new(parent_hashes), proposer_map: Arc::new(proposer_map), diff --git a/beacon_chain/validation/tests/block_validation/tests.rs b/beacon_chain/validation/tests/block_validation/tests.rs index 95ddd4222..9e16756ec 100644 --- a/beacon_chain/validation/tests/block_validation/tests.rs +++ b/beacon_chain/validation/tests/block_validation/tests.rs @@ -34,6 +34,7 @@ fn get_simple_params() -> BlockTestParams { let validation_context_slot = block_slot; let validation_context_justified_slot = attestations_justified_slot; + let validation_context_justified_block_hash = Hash256::from("justified_hash".as_bytes()); let validation_context_finalized_slot = 0; BlockTestParams { @@ -47,6 +48,7 @@ fn get_simple_params() -> BlockTestParams { attestations_justified_slot, validation_context_slot, validation_context_justified_slot, + validation_context_justified_block_hash, validation_context_finalized_slot, } }