From 1f089d423e130724bff87b68fd996523b919162c Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Fri, 12 Oct 2018 20:41:18 +1100 Subject: [PATCH] Ensure attestation val. check parent.slot As per comments by Danny Ryan on PR#33 --- .../validation/src/attestation_validation.rs | 20 +++++++++++-------- .../validation/src/block_validation.rs | 5 +++-- .../tests/attestation_validation/helpers.rs | 2 ++ .../tests/attestation_validation/tests.rs | 20 ++++++++++++++----- 4 files changed, 32 insertions(+), 15 deletions(-) diff --git a/beacon_chain/validation/src/attestation_validation.rs b/beacon_chain/validation/src/attestation_validation.rs index 9090558581..bb458ebc72 100644 --- a/beacon_chain/validation/src/attestation_validation.rs +++ b/beacon_chain/validation/src/attestation_validation.rs @@ -28,8 +28,9 @@ use super::signature_verification::{ #[derive(Debug,PartialEq)] pub enum AttestationValidationError { - SlotTooHigh, - SlotTooLow, + ParentSlotTooHigh, + BlockSlotTooHigh, + BlockSlotTooLow, JustifiedSlotIncorrect, InvalidJustifiedBlockHash, TooManyObliqueHashes, @@ -54,6 +55,8 @@ pub struct AttestationValidationContext { /// The slot as determined by the system time. pub block_slot: u64, + /// The slot of the parent of the block that contained this attestation. + pub parent_block_slot: u64, /// The cycle_length as determined by the chain configuration. pub cycle_length: u8, /// The last justified slot as per the client's view of the canonical chain. @@ -82,10 +85,11 @@ impl AttestationValidationContext -> Result, AttestationValidationError> { /* - * The attesation slot must not be higher than the block that contained it. + * The attesation slot must be less than or equal to the parent of the slot of the block + * that contained the attestation. */ - if a.slot > self.block_slot { - return Err(AttestationValidationError::SlotTooHigh); + if a.slot > self.parent_block_slot { + return Err(AttestationValidationError::ParentSlotTooHigh); } /* @@ -94,7 +98,7 @@ impl AttestationValidationContext */ if a.slot < self.block_slot .saturating_sub(u64::from(self.cycle_length).saturating_add(1)) { - return Err(AttestationValidationError::SlotTooLow); + return Err(AttestationValidationError::BlockSlotTooLow); } /* @@ -210,9 +214,9 @@ impl From for AttestationValidationError { ParentHashesError::BadObliqueHashes => AttestationValidationError::BadObliqueHashes, ParentHashesError::SlotTooLow - => AttestationValidationError::SlotTooLow, + => AttestationValidationError::BlockSlotTooLow, ParentHashesError::SlotTooHigh - => AttestationValidationError::SlotTooHigh, + => AttestationValidationError::BlockSlotTooHigh, ParentHashesError::IntWrapping => AttestationValidationError::IntWrapping } diff --git a/beacon_chain/validation/src/block_validation.rs b/beacon_chain/validation/src/block_validation.rs index c21a2fe4fe..3c3b828ed0 100644 --- a/beacon_chain/validation/src/block_validation.rs +++ b/beacon_chain/validation/src/block_validation.rs @@ -196,7 +196,7 @@ impl BlockValidationContext * Also, read the slot from the parent block for later use. */ let parent_hash = b.parent_hash(); - let parent_slot = match self.block_store.get_serialized_block(&parent_hash)? { + let parent_block_slot = match self.block_store.get_serialized_block(&parent_hash)? { None => return Err(SszBlockValidationError::UnknownParentHash), Some(ssz) => { let parent_block = SszBlock::from_slice(&ssz[..])?; @@ -209,6 +209,7 @@ impl BlockValidationContext */ let attestation_validation_context = Arc::new(AttestationValidationContext { block_slot, + parent_block_slot, cycle_length: self.cycle_length, last_justified_slot: self.last_justified_slot, parent_hashes: self.parent_hashes.clone(), @@ -230,7 +231,7 @@ impl BlockValidationContext * If the signature of proposer for the parent slot was not present in the first (0'th) * attestation of this block, reject the block. */ - let parent_block_proposer = self.proposer_map.get(&parent_slot) + let parent_block_proposer = self.proposer_map.get(&parent_block_slot) .ok_or(SszBlockValidationError::BadProposerMap)?; if !attestation_voters.contains(&parent_block_proposer) { return Err(SszBlockValidationError::NoProposerSignature); diff --git a/beacon_chain/validation/tests/attestation_validation/helpers.rs b/beacon_chain/validation/tests/attestation_validation/helpers.rs index e768638db3..df2bfdcc7a 100644 --- a/beacon_chain/validation/tests/attestation_validation/helpers.rs +++ b/beacon_chain/validation/tests/attestation_validation/helpers.rs @@ -169,6 +169,7 @@ pub fn setup_attestation_validation_test(shard_id: u16, attester_count: usize) .map(|i| Hash256::from(i as u64)) .collect(); let attestation_slot = block_slot - 1; + let parent_block_slot = attestation_slot; 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()); @@ -204,6 +205,7 @@ pub fn setup_attestation_validation_test(shard_id: u16, attester_count: usize) let context: AttestationValidationContext = AttestationValidationContext { block_slot, + parent_block_slot, cycle_length, last_justified_slot, parent_hashes: parent_hashes.clone(), diff --git a/beacon_chain/validation/tests/attestation_validation/tests.rs b/beacon_chain/validation/tests/attestation_validation/tests.rs index 06b598e934..f09da33136 100644 --- a/beacon_chain/validation/tests/attestation_validation/tests.rs +++ b/beacon_chain/validation/tests/attestation_validation/tests.rs @@ -33,22 +33,32 @@ fn test_attestation_validation_valid() { } #[test] -fn test_attestation_validation_invalid_slot_too_high() { +fn test_attestation_validation_invalid_parent_slot_too_high() { let mut rig = generic_rig(); - rig.attestation.slot = rig.context.block_slot + 1; + rig.context.parent_block_slot = rig.attestation.slot - 1; let result = rig.context.validate_attestation(&rig.attestation); - assert_eq!(result, Err(AttestationValidationError::SlotTooHigh)); + assert_eq!(result, Err(AttestationValidationError::ParentSlotTooHigh)); } #[test] -fn test_attestation_validation_invalid_slot_too_low() { +fn test_attestation_validation_invalid_block_slot_too_high() { + let mut rig = generic_rig(); + + rig.context.block_slot = rig.attestation.slot - 1; + + let result = rig.context.validate_attestation(&rig.attestation); + assert_eq!(result, Err(AttestationValidationError::BlockSlotTooHigh)); +} + +#[test] +fn test_attestation_validation_invalid_block_slot_too_low() { let mut rig = generic_rig(); rig.attestation.slot = rig.context.block_slot - u64::from(rig.context.cycle_length) - 2; let result = rig.context.validate_attestation(&rig.attestation); - assert_eq!(result, Err(AttestationValidationError::SlotTooLow)); + assert_eq!(result, Err(AttestationValidationError::BlockSlotTooLow)); } #[test]