From 5dd2361d21702e52aa880d951b735f4b7f2d8be2 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Sun, 30 Sep 2018 11:26:36 +0930 Subject: [PATCH] Progress further with block validation - Rename errors returned from SszBlock. - Add parent_hash concept to block validation. - Add more comments to block validation. --- lighthouse/state/block/mod.rs | 5 +- lighthouse/state/block/ssz_block.rs | 16 +-- lighthouse/state/block/validation/mod.rs | 1 + lighthouse/state/block/validation/tests.rs | 33 +++-- .../block/validation/validate_ssz_block.rs | 123 ++++++++++++------ 5 files changed, 121 insertions(+), 57 deletions(-) diff --git a/lighthouse/state/block/mod.rs b/lighthouse/state/block/mod.rs index 1b8a19439..9a461568d 100644 --- a/lighthouse/state/block/mod.rs +++ b/lighthouse/state/block/mod.rs @@ -11,4 +11,7 @@ mod ssz_block; mod validation; pub use self::structs::Block; -pub use self::ssz_block::SszBlock; +pub use self::ssz_block::{ + SszBlock, + SszBlockError, +}; diff --git a/lighthouse/state/block/ssz_block.rs b/lighthouse/state/block/ssz_block.rs index 1d4fb1c72..e92291f80 100644 --- a/lighthouse/state/block/ssz_block.rs +++ b/lighthouse/state/block/ssz_block.rs @@ -10,7 +10,7 @@ use super::structs::{ use super::attestation_record::MIN_SSZ_ATTESTION_RECORD_LENGTH; #[derive(Debug, PartialEq)] -pub enum BlockValidatorError { +pub enum SszBlockError { TooShort, TooLong, } @@ -46,7 +46,7 @@ impl<'a> SszBlock<'a> { /// how many bytes were read from the slice. In the case of multiple, sequentually serialized /// blocks `len` can be used to assume the location of the next serialized block. pub fn from_slice(vec: &'a [u8]) - -> Result + -> Result { let untrimmed_ssz = &vec[..]; /* @@ -55,19 +55,19 @@ impl<'a> SszBlock<'a> { * attestation record). */ if vec.len() < MIN_SSZ_BLOCK_LENGTH + MIN_SSZ_ATTESTION_RECORD_LENGTH { - return Err(BlockValidatorError::TooShort); + return Err(SszBlockError::TooShort); } /* * Ensure the SSZ slice isn't longer than is possible for a block. */ if vec.len() > MAX_SSZ_BLOCK_LENGTH { - return Err(BlockValidatorError::TooLong); + return Err(SszBlockError::TooLong); } /* * Determine how many bytes are used to store attestation records. */ let attestation_len = decode_length(untrimmed_ssz, 72, LENGTH_BYTES) - .map_err(|_| BlockValidatorError::TooShort)?; + .map_err(|_| SszBlockError::TooShort)?; /* * The block only has one variable field, `attestations`, therefore * the size of the block must be the minimum size, plus the length @@ -77,7 +77,7 @@ impl<'a> SszBlock<'a> { MIN_SSZ_BLOCK_LENGTH + attestation_len }; if vec.len() < block_ssz_len { - return Err(BlockValidatorError::TooShort); + return Err(SszBlockError::TooShort); } Ok(Self{ ssz: &untrimmed_ssz[0..block_ssz_len], @@ -171,7 +171,7 @@ mod tests { assert_eq!( SszBlock::from_slice(&ssz[..]), - Err(BlockValidatorError::TooShort) + Err(SszBlockError::TooShort) ); } @@ -183,7 +183,7 @@ mod tests { assert_eq!( SszBlock::from_slice(&ssz[0..(ssz.len() - 1)]), - Err(BlockValidatorError::TooShort) + Err(SszBlockError::TooShort) ); } diff --git a/lighthouse/state/block/validation/mod.rs b/lighthouse/state/block/validation/mod.rs index e2382da65..29f94c347 100644 --- a/lighthouse/state/block/validation/mod.rs +++ b/lighthouse/state/block/validation/mod.rs @@ -7,6 +7,7 @@ mod benches; use super::attestation_record; use super::{ SszBlock, + SszBlockError, Block, }; use super::db; diff --git a/lighthouse/state/block/validation/tests.rs b/lighthouse/state/block/validation/tests.rs index a70a25ece..1dc4da2e5 100644 --- a/lighthouse/state/block/validation/tests.rs +++ b/lighthouse/state/block/validation/tests.rs @@ -92,13 +92,22 @@ pub fn setup_block_validation_scenario(params: &TestParams) 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_block(justified_block_hash.as_ref(), &vec![42]).unwrap(); - stores.block.put_block(parent_hash.as_ref(), &vec![42]).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. + */ + let mut parent_block = Block::zero(); + let parent_attestation = AttestationRecord::zero(); + parent_block.slot_number = block_slot - 1; + parent_block.attestations.push(parent_attestation); + let parent_block_ssz = serialize_block(&parent_block); + stores.block.put_serialized_block(parent_hash.as_ref(), &parent_block_ssz).unwrap(); let validator_index: usize = 0; let proposer_map = { let mut proposer_map = ProposerMap::new(); - proposer_map.insert(block_slot, validator_index); + proposer_map.insert(parent_block.slot_number, validator_index); proposer_map }; @@ -209,6 +218,7 @@ pub fn serialize_block(b: &Block) -> Vec { pub fn run_block_validation_scenario( validation_slot: u64, validation_last_justified_slot: u64, + validation_last_finalized_slot: u64, params: &TestParams, mutator_func: F) -> Result<(BlockStatus, Option), SszBlockValidationError> @@ -234,6 +244,7 @@ pub fn run_block_validation_scenario( present_slot: validation_slot, cycle_length: params.cycle_length, last_justified_slot: validation_last_justified_slot, + last_finalized_slot: validation_last_finalized_slot, parent_hashes: Arc::new(parent_hashes), proposer_map: Arc::new(proposer_map), attester_map: Arc::new(attester_map), @@ -265,11 +276,12 @@ fn get_simple_params() -> TestParams { } #[test] -fn test_block_validation_simple_scenario_valid_in_canonical_chain() { +fn test_block_validation_simple_scenario_valid() { let params = get_simple_params(); let validation_slot = params.block_slot; let validation_last_justified_slot = params.attestations_justified_slot; + let validation_last_finalized_slot = 0; let no_mutate = |block, attester_map, proposer_map, stores| { (block, attester_map, proposer_map, stores) @@ -278,31 +290,34 @@ fn test_block_validation_simple_scenario_valid_in_canonical_chain() { let status = run_block_validation_scenario( validation_slot, validation_last_justified_slot, + validation_last_finalized_slot, ¶ms, no_mutate); - assert_eq!(status.unwrap().0, BlockStatus::NewBlockInCanonicalChain); + assert_eq!(status.unwrap().0, BlockStatus::NewBlock); } #[test] -fn test_block_validation_simple_scenario_valid_not_in_canonical_chain() { +fn test_block_validation_simple_scenario_invalid_unknown_parent_block() { let params = get_simple_params(); let validation_slot = params.block_slot; let validation_last_justified_slot = params.attestations_justified_slot; + let validation_last_finalized_slot = 0; let no_mutate = |mut block: Block, attester_map, proposer_map, stores| { - block.parent_hash = Hash256::from("not in canonical chain".as_bytes()); + block.parent_hash = Hash256::from("unknown parent block".as_bytes()); (block, attester_map, proposer_map, stores) }; let status = run_block_validation_scenario( validation_slot, validation_last_justified_slot, + validation_last_finalized_slot, ¶ms, no_mutate); - assert_eq!(status.unwrap().0, BlockStatus::NewBlockInForkChain); + assert_eq!(status, Err(SszBlockValidationError::UnknownParentHash)); } #[test] @@ -311,6 +326,7 @@ fn test_block_validation_simple_scenario_invalid_2nd_attestation() { let validation_slot = params.block_slot; let validation_last_justified_slot = params.attestations_justified_slot; + let validation_last_finalized_slot = 0; let mutator = |mut block: Block, attester_map, proposer_map, stores| { /* @@ -323,6 +339,7 @@ fn test_block_validation_simple_scenario_invalid_2nd_attestation() { let status = run_block_validation_scenario( validation_slot, validation_last_justified_slot, + validation_last_finalized_slot, ¶ms, mutator); diff --git a/lighthouse/state/block/validation/validate_ssz_block.rs b/lighthouse/state/block/validation/validate_ssz_block.rs index 0ad688145..d2aa3c853 100644 --- a/lighthouse/state/block/validation/validate_ssz_block.rs +++ b/lighthouse/state/block/validation/validate_ssz_block.rs @@ -1,4 +1,5 @@ extern crate rayon; + use self::rayon::prelude::*; use std::sync::{ @@ -21,6 +22,7 @@ use super::{ }; use super::{ SszBlock, + SszBlockError, Block, }; use super::db::{ @@ -40,24 +42,25 @@ use super::utils::types::Hash256; #[derive(Debug, PartialEq)] pub enum BlockStatus { - NewBlockInCanonicalChain, - NewBlockInForkChain, + NewBlock, KnownBlock, } #[derive(Debug, PartialEq)] pub enum SszBlockValidationError { FutureSlot, + SlotAlreadyFinalized, UnknownPoWChainRef, UnknownParentHash, BadAttestationSsz, AttestationValidationError(AttestationValidationError), AttestationSignatureFailed, FirstAttestationSignatureFailed, + ProposerAttestationHasObliqueHashes, NoProposerSignature, BadProposerMap, RwLockPoisoned, - DatabaseError(String), + DBError(String), } /// The context against which a block should be validated. @@ -70,6 +73,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 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. pub parent_hashes: Arc>, /// A map of slots to a block proposer validation index. @@ -110,8 +115,16 @@ impl BlockValidationContext { /* - * If the block slot corresponds to a slot in the future (according to the local time), - * drop it. + * If this block is already known, return immediately and indicate the the block is + * known. Don't attempt to deserialize the block. + */ + let block_hash = &b.block_hash(); + if self.block_store.block_exists(&block_hash)? { + return Ok((BlockStatus::KnownBlock, None)); + } + + /* + * If the block slot corresponds to a slot in the future, drop it. */ let block_slot = b.slot_number(); if block_slot > self.present_slot { @@ -119,21 +132,23 @@ impl BlockValidationContext } /* - * If this block is already known, return immediately. + * If the block is unknown (assumed unknown because we checked the db earlier in this + * function) and it comes from a slot that is already finalized, drop the block. + * + * If a slot is finalized, there's no point in considering any other blocks for that slot. */ - let block_hash = &b.block_hash(); - if self.block_store.block_exists(&block_hash)? { - return Ok((BlockStatus::KnownBlock, None)); + if block_slot <= self.last_finalized_slot { + return Err(SszBlockValidationError::SlotAlreadyFinalized); } - /* * If the PoW chain hash is not known to us, drop it. * * We only accept blocks that reference a known PoW hash. * - * Note: it is not clear what a "known" PoW chain ref is. Likely, - * it means "sufficienty deep in the canonical PoW chain". + * Note: it is not clear what a "known" PoW chain ref is. Likely it means the block hash is + * "sufficienty deep in the canonical PoW chain". This should be clarified as the spec + * crystallizes. */ let pow_chain_ref = b.pow_chain_ref(); if !self.pow_store.block_hash_exists(b.pow_chain_ref())? { @@ -141,13 +156,16 @@ impl BlockValidationContext } /* - * Store a reference to the serialized attestations from the block. + * Store a slice of the serialized attestations from the block SSZ. */ let attestations_ssz = &b.attestations(); /* - * Get a slice of the first serialized attestation (the 0th) and decode it into + * Get a slice of the first serialized attestation (the 0'th) and decode it into * a full AttestationRecord object. + * + * The first attestation must be validated separately as it must contain a signature of the + * proposer of the previous block (this is checked later in this function). */ let (first_attestation_ssz, next_index) = split_one_attestation( &attestations_ssz, @@ -156,10 +174,17 @@ impl BlockValidationContext &first_attestation_ssz, 0)?; /* - * Validate this first attestation. + * The first attestation may not have oblique hashes. * - * It is a requirement that the block proposer for this slot - * must have signed the 0th attestation record. + * The presence of oblique hashes in the first attestation would indicate that the proposer + * of the previous block is attesting to some other block than the one they produced. + */ + if first_attestation.oblique_parent_hashes.len() > 0 { + return Err(SszBlockValidationError::ProposerAttestationHasObliqueHashes); + } + + /* + * Validate this first attestation. */ let attestation_voters = validate_attestation( &first_attestation, @@ -179,16 +204,28 @@ impl BlockValidationContext FirstAttestationSignatureFailed)?; /* - * Read the proposer from the map of slot -> validator index. + * Read the parent hash from the block we are validating then attempt to load + * the parent block ssz from the database. If that parent doesn't exist in + * the database, reject the block. + * + * If the parent does exist in the database, read the slot of that parent. Then, + * determine the proposer of that slot (the parent slot) by looking it up + * in the proposer map. + * + * If that proposer (the proposer of the parent block) was not present in the first (0'th) + * attestation of this block, reject the block. */ - let proposer = self.proposer_map.get(&block_slot) - .ok_or(SszBlockValidationError::BadProposerMap)?; - - /* - * If the proposer for this slot was not a voter, reject the block. - */ - if !attestation_voters.contains(&proposer) { - return Err(SszBlockValidationError::NoProposerSignature); + let parent_hash = b.parent_hash(); + match self.block_store.get_serialized_block(&parent_hash)? { + None => return Err(SszBlockValidationError::UnknownParentHash), + Some(ssz) => { + let parent_block = SszBlock::from_slice(&ssz[..])?; + let proposer = self.proposer_map.get(&parent_block.slot_number()) + .ok_or(SszBlockValidationError::BadProposerMap)?; + if !attestation_voters.contains(&proposer) { + return Err(SszBlockValidationError::NoProposerSignature); + } + } } /* @@ -210,7 +247,7 @@ impl BlockValidationContext * is found. */ let failure: RwLock> = RwLock::new(None); - let deserialized_attestations: Vec = other_attestations + let mut deserialized_attestations: Vec = other_attestations .par_iter() .filter_map(|attestation_ssz| { /* @@ -284,21 +321,16 @@ impl BlockValidationContext } } + /* + * Add the first attestation to the vec of deserialized attestations at + * index 0. + */ + deserialized_attestations.insert(0, first_attestation); + /* * If we have reached this point, the block is a new valid block that is worthy of * processing. */ - - /* - * If the block's parent_hash _is_ in the canonical chain, the block is a - * new block in the canonical chain. Otherwise, it's a new block in a fork chain. - */ - let parent_hash = b.parent_hash(); - let status = if self.block_store.block_exists_in_canonical_chain(&parent_hash)? { - BlockStatus::NewBlockInCanonicalChain - } else { - BlockStatus::NewBlockInForkChain - }; let block = Block { parent_hash: Hash256::from(parent_hash), slot_number: block_slot, @@ -308,13 +340,13 @@ impl BlockValidationContext active_state_root: Hash256::from(b.act_state_root()), crystallized_state_root: Hash256::from(b.cry_state_root()), }; - Ok((status, Some(block))) + Ok((BlockStatus::NewBlock, Some(block))) } } impl From for SszBlockValidationError { fn from(e: DBError) -> Self { - SszBlockValidationError::DatabaseError(e.message) + SszBlockValidationError::DBError(e.message) } } @@ -327,6 +359,17 @@ impl From for SszBlockValidationError { } } +impl From for SszBlockValidationError { + fn from(e: SszBlockError) -> Self { + match e { + SszBlockError::TooShort => + SszBlockValidationError::DBError("Bad parent block in db.".to_string()), + SszBlockError::TooLong => + SszBlockValidationError::DBError("Bad parent block in db.".to_string()), + } + } +} + impl From for SszBlockValidationError { fn from(e: DecodeError) -> Self { match e {