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

@@ -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,
+};

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<Self, BlockValidatorError>
+        -> Result<Self, SszBlockError>
     {
         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)
         );
     }
 

lighthouse/state/block/validation/mod.rs

@@ -7,6 +7,7 @@ mod benches;
 use super::attestation_record;
 use super::{
     SszBlock,
+    SszBlockError,
     Block,
 };
 use super::db;

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_serialized_block(justified_block_hash.as_ref(), &vec![42]).unwrap();
-    stores.block.put_block(parent_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<u8> {
 pub fn run_block_validation_scenario<F>(
     validation_slot: u64,
     validation_last_justified_slot: u64,
+    validation_last_finalized_slot: u64,
     params: &TestParams,
     mutator_func: F)
     -> Result<(BlockStatus, Option<Block>), SszBlockValidationError>
@@ -234,6 +244,7 @@ pub fn run_block_validation_scenario<F>(
         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,
         &params,
         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,
         &params,
         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,
         &params,
         mutator);
 

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,
+    NewBlock,
-    NewBlockInForkChain,
     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<T>
     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<Vec<Hash256>>,
     /// A map of slots to a block proposer validation index.
@@ -110,8 +115,16 @@ impl<T> BlockValidationContext<T>
     {
 
         /*
-         * If the block slot corresponds to a slot in the future (according to the local time),
+         * If this block is already known, return immediately and indicate the the block is
-         * drop it.
+         * 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<T> BlockValidationContext<T>
         }
 
         /*
-         * 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 block_slot <= self.last_finalized_slot {
-        if self.block_store.block_exists(&block_hash)? {
+            return Err(SszBlockValidationError::SlotAlreadyFinalized);
-            return Ok((BlockStatus::KnownBlock, None));
         }
 
-
         /*
          * 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,
+         * Note: it is not clear what a "known" PoW chain ref is. Likely it means the block hash is
-         * it means "sufficienty deep in the canonical PoW chain".
+         * "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<T> BlockValidationContext<T>
         }
 
         /*
-         * 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<T> BlockValidationContext<T>
             &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
+         * The presence of oblique hashes in the first attestation would indicate that the proposer
-         * must have signed the 0th attestation record.
+         * 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<T> BlockValidationContext<T>
                    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)
+        let parent_hash = b.parent_hash();
-            .ok_or(SszBlockValidationError::BadProposerMap)?;
+        match self.block_store.get_serialized_block(&parent_hash)? {
-
+            None => return Err(SszBlockValidationError::UnknownParentHash),
-        /*
+            Some(ssz) => {
-         * If the proposer for this slot was not a voter, reject the block.
+                let parent_block = SszBlock::from_slice(&ssz[..])?;
-         */
+                let proposer = self.proposer_map.get(&parent_block.slot_number())
-        if !attestation_voters.contains(&proposer) {
+                    .ok_or(SszBlockValidationError::BadProposerMap)?;
-            return Err(SszBlockValidationError::NoProposerSignature);
+                if !attestation_voters.contains(&proposer) {
+                    return Err(SszBlockValidationError::NoProposerSignature);
+                }
+            }
         }
 
         /*
@@ -210,7 +247,7 @@ impl<T> BlockValidationContext<T>
          * is found.
          */
         let failure: RwLock<Option<SszBlockValidationError>> = RwLock::new(None);
-        let deserialized_attestations: Vec<AttestationRecord> = other_attestations
+        let mut deserialized_attestations: Vec<AttestationRecord> = other_attestations
             .par_iter()
             .filter_map(|attestation_ssz| {
                 /*
@@ -284,21 +321,16 @@ impl<T> BlockValidationContext<T>
             }
         }
 
+        /*
+         * 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<T> BlockValidationContext<T>
             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<DBError> for SszBlockValidationError {
     fn from(e: DBError) -> Self {
-        SszBlockValidationError::DatabaseError(e.message)
+        SszBlockValidationError::DBError(e.message)
     }
 }
 
@@ -327,6 +359,17 @@ impl From<AttestationSplitError> for SszBlockValidationError {
     }
 }
 
+impl From<SszBlockError> 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<DecodeError> for SszBlockValidationError {
     fn from(e: DecodeError) -> Self {
         match e {