From 1ef4f0ea12f76ccf775a41ff0012c2d14f1044d8 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Sun, 27 Sep 2020 20:59:40 +0000 Subject: [PATCH] Add gossip conditions from spec v0.12.3 (#1667) ## Issue Addressed NA ## Proposed Changes There are four new conditions introduced in v0.12.3: 1. _[REJECT]_ The attestation's epoch matches its target -- i.e. `attestation.data.target.epoch == compute_epoch_at_slot(attestation.data.slot)` 1. _[REJECT]_ The attestation's target block is an ancestor of the block named in the LMD vote -- i.e. `get_ancestor(store, attestation.data.beacon_block_root, compute_start_slot_at_epoch(attestation.data.target.epoch)) == attestation.data.target.root` 1. _[REJECT]_ The committee index is within the expected range -- i.e. `data.index < get_committee_count_per_slot(state, data.target.epoch)`. 1. _[REJECT]_ The number of aggregation bits matches the committee size -- i.e. `len(attestation.aggregation_bits) == len(get_beacon_committee(state, data.slot, data.index))`. This PR implements new logic to suit (1) and (2). Tests are added for (3) and (4), although they were already implicitly enforced. ## Additional Info - There's a bit of edge-case with target root verification that I raised here: https://github.com/ethereum/eth2.0-specs/pull/2001#issuecomment-699246659 - I've had to add an `--ignore` to `cargo audit` to get CI to pass. See https://github.com/sigp/lighthouse/issues/1669 --- .../src/attestation_verification.rs | 73 +++++++++++-- .../tests/attestation_verification.rs | 103 +++++++++++++++++- .../network/src/beacon_processor/worker.rs | 26 +++++ beacon_node/network/src/metrics.rs | 16 +++ .../src/proto_array_fork_choice.rs | 1 + 5 files changed, 208 insertions(+), 11 deletions(-) diff --git a/beacon_node/beacon_chain/src/attestation_verification.rs b/beacon_node/beacon_chain/src/attestation_verification.rs index b81daa4bdb8..64803304441 100644 --- a/beacon_node/beacon_chain/src/attestation_verification.rs +++ b/beacon_node/beacon_chain/src/attestation_verification.rs @@ -37,6 +37,7 @@ use crate::{ BeaconChain, BeaconChainError, BeaconChainTypes, }; use bls::verify_signature_sets; +use proto_array::Block as ProtoBlock; use slog::debug; use slot_clock::SlotClock; use state_processing::{ @@ -226,6 +227,21 @@ pub enum Error { head_block_slot: Slot, attestation_slot: Slot, }, + /// The attestation has an invalid target epoch. + /// + /// ## Peer scoring + /// + /// The peer has sent an invalid message. + InvalidTargetEpoch { slot: Slot, epoch: Epoch }, + /// The attestation references an invalid target block. + /// + /// ## Peer scoring + /// + /// The peer has sent an invalid message. + InvalidTargetRoot { + attestation: Hash256, + expected: Hash256, + }, /// There was an error whilst processing the attestation. It is not known if it is valid or invalid. /// /// ## Peer scoring @@ -425,6 +441,16 @@ impl VerifiedUnaggregatedAttestation { subnet_id: SubnetId, chain: &BeaconChain, ) -> Result { + let attestation_epoch = attestation.data.slot.epoch(T::EthSpec::slots_per_epoch()); + + // Check the attestation's epoch matches its target. + if attestation_epoch != attestation.data.target.epoch { + return Err(Error::InvalidTargetEpoch { + slot: attestation.data.slot, + epoch: attestation.data.target.epoch, + }); + } + // Ensure attestation is within the last ATTESTATION_PROPAGATION_SLOT_RANGE slots (within a // MAXIMUM_GOSSIP_CLOCK_DISPARITY allowance). // @@ -433,16 +459,49 @@ impl VerifiedUnaggregatedAttestation { // Check to ensure that the attestation is "unaggregated". I.e., it has exactly one // aggregation bit set. - let num_aggreagtion_bits = attestation.aggregation_bits.num_set_bits(); - if num_aggreagtion_bits != 1 { - return Err(Error::NotExactlyOneAggregationBitSet(num_aggreagtion_bits)); + let num_aggregation_bits = attestation.aggregation_bits.num_set_bits(); + if num_aggregation_bits != 1 { + return Err(Error::NotExactlyOneAggregationBitSet(num_aggregation_bits)); } // Attestations must be for a known block. If the block is unknown, we simply drop the // attestation and do not delay consideration for later. // // Enforce a maximum skip distance for unaggregated attestations. - verify_head_block_is_known(chain, &attestation, chain.config.import_max_skip_slots)?; + let head_block = + verify_head_block_is_known(chain, &attestation, chain.config.import_max_skip_slots)?; + + // Check the attestation target root. + let head_block_epoch = head_block.slot.epoch(T::EthSpec::slots_per_epoch()); + if head_block_epoch > attestation_epoch { + // The attestation points to a head block from an epoch later than the attestation. + // + // Whilst this seems clearly invalid in the "spirit of the protocol", there is nothing + // in the specification to prevent these messages from propagating. + // + // Reference: + // https://github.com/ethereum/eth2.0-specs/pull/2001#issuecomment-699246659 + } else { + let target_root = if head_block_epoch == attestation_epoch { + // If the block is in the same epoch as the attestation, then use the target root + // from the block. + head_block.target_root + } else { + // If the head block is from a previous epoch then skip slots will cause the head block + // root to become the target block root. + // + // We know the head block is from a previous epoch due to a previous check. + head_block.root + }; + + // Reject any attestation with an invalid target root. + if target_root != attestation.data.target.root { + return Err(Error::InvalidTargetRoot { + attestation: attestation.data.target.root, + expected: target_root, + }); + } + } let (indexed_attestation, committees_per_slot) = obtain_indexed_attestation_and_committees_per_slot(chain, &attestation)?; @@ -541,7 +600,7 @@ fn verify_head_block_is_known( chain: &BeaconChain, attestation: &Attestation, max_skip_slots: Option, -) -> Result<(), Error> { +) -> Result { if let Some(block) = chain .fork_choice .read() @@ -556,7 +615,7 @@ fn verify_head_block_is_known( }); } } - Ok(()) + Ok(block) } else { Err(Error::UnknownHeadBlock { beacon_block_root: attestation.data.beacon_block_root, @@ -718,7 +777,7 @@ pub fn obtain_indexed_attestation_and_committees_per_slot( map_attestation_committee(chain, attestation, |(committee, committees_per_slot)| { get_indexed_attestation(committee.committee, &attestation) .map(|attestation| (attestation, committees_per_slot)) - .map_err(|e| BeaconChainError::from(e).into()) + .map_err(Error::Invalid) }) } diff --git a/beacon_node/beacon_chain/tests/attestation_verification.rs b/beacon_node/beacon_chain/tests/attestation_verification.rs index ec2200b1d77..937850751c3 100644 --- a/beacon_node/beacon_chain/tests/attestation_verification.rs +++ b/beacon_node/beacon_chain/tests/attestation_verification.rs @@ -11,13 +11,15 @@ use beacon_chain::{ BeaconChain, BeaconChainTypes, }; use int_to_bytes::int_to_bytes32; -use state_processing::per_slot_processing; +use state_processing::{ + per_block_processing::errors::AttestationValidationError, per_slot_processing, +}; use store::config::StoreConfig; use tree_hash::TreeHash; use types::{ - test_utils::generate_deterministic_keypair, AggregateSignature, Attestation, EthSpec, Hash256, - Keypair, MainnetEthSpec, SecretKey, SelectionProof, SignedAggregateAndProof, SignedBeaconBlock, - SubnetId, Unsigned, + test_utils::generate_deterministic_keypair, AggregateSignature, Attestation, BeaconStateError, + BitList, EthSpec, Hash256, Keypair, MainnetEthSpec, SecretKey, SelectionProof, + SignedAggregateAndProof, SignedBeaconBlock, SubnetId, Unsigned, }; pub type E = MainnetEthSpec; @@ -582,6 +584,31 @@ fn unaggregated_gossip_verification() { }; } + /* + * The following test ensures: + * + * Spec v0.12.3 + * + * The committee index is within the expected range -- i.e. `data.index < + * get_committee_count_per_slot(state, data.target.epoch)`. + */ + assert_invalid!( + "attestation with invalid committee index", + { + let mut a = valid_attestation.clone(); + a.data.index = harness + .chain + .head() + .unwrap() + .beacon_state + .get_committee_count_at_slot(a.data.slot) + .unwrap(); + a + }, + subnet_id, + AttnError::NoCommitteeForSlotAndIndex { .. } + ); + /* * The following test ensures: * @@ -642,6 +669,7 @@ fn unaggregated_gossip_verification() { { let mut a = valid_attestation.clone(); a.data.slot = early_slot; + a.data.target.epoch = early_slot.epoch(E::slots_per_epoch()); a }, subnet_id, @@ -654,6 +682,27 @@ fn unaggregated_gossip_verification() { if attestation_slot == early_slot && earliest_permissible_slot == current_slot - E::slots_per_epoch() - 1 ); + /* + * The following test ensures: + * + * Spec v0.12.3 + * + * The attestation's epoch matches its target -- i.e. `attestation.data.target.epoch == + * compute_epoch_at_slot(attestation.data.slot)` + * + */ + + assert_invalid!( + "attestation with invalid target epoch", + { + let mut a = valid_attestation.clone(); + a.data.target.epoch += 1; + a + }, + subnet_id, + AttnError::InvalidTargetEpoch { .. } + ); + /* * The following two tests ensure: * @@ -694,6 +743,32 @@ fn unaggregated_gossip_verification() { AttnError::NotExactlyOneAggregationBitSet(2) ); + /* + * The following test ensures: + * + * Spec v0.12.3 + * + * The number of aggregation bits matches the committee size -- i.e. + * `len(attestation.aggregation_bits) == len(get_beacon_committee(state, data.slot, + * data.index))`. + */ + assert_invalid!( + "attestation with invalid bitfield", + { + let mut a = valid_attestation.clone(); + let bits = a.aggregation_bits.iter().collect::>(); + a.aggregation_bits = BitList::with_capacity(bits.len() + 1).unwrap(); + for (i, bit) in bits.into_iter().enumerate() { + a.aggregation_bits.set(i, bit).unwrap(); + } + a + }, + subnet_id, + AttnError::Invalid(AttestationValidationError::BeaconStateError( + BeaconStateError::InvalidBitfield + )) + ); + /* * The following test ensures that: * @@ -717,6 +792,26 @@ fn unaggregated_gossip_verification() { if beacon_block_root == unknown_root ); + /* + * The following test ensures that: + * + * Spec v0.12.3 + * + * The attestation's target block is an ancestor of the block named in the LMD vote + */ + + let unknown_root = Hash256::from_low_u64_le(424242); + assert_invalid!( + "attestation with invalid target root", + { + let mut a = valid_attestation.clone(); + a.data.target.root = unknown_root; + a + }, + subnet_id, + AttnError::InvalidTargetRoot { .. } + ); + /* * The following test ensures that: * diff --git a/beacon_node/network/src/beacon_processor/worker.rs b/beacon_node/network/src/beacon_processor/worker.rs index 79d5d53c5fd..653922dfe5e 100644 --- a/beacon_node/network/src/beacon_processor/worker.rs +++ b/beacon_node/network/src/beacon_processor/worker.rs @@ -847,6 +847,32 @@ impl Worker { ); self.penalize_peer(peer_id.clone(), PeerAction::LowToleranceError); } + AttnError::InvalidTargetEpoch { .. } => { + /* + * The attestation is malformed. + * + * The peer has published an invalid consensus message. + */ + self.propagate_validation_result( + message_id, + peer_id.clone(), + MessageAcceptance::Reject, + ); + self.penalize_peer(peer_id.clone(), PeerAction::LowToleranceError); + } + AttnError::InvalidTargetRoot { .. } => { + /* + * The attestation is malformed. + * + * The peer has published an invalid consensus message. + */ + self.propagate_validation_result( + message_id, + peer_id.clone(), + MessageAcceptance::Reject, + ); + self.penalize_peer(peer_id.clone(), PeerAction::LowToleranceError); + } AttnError::TooManySkippedSlots { head_block_slot, attestation_slot, diff --git a/beacon_node/network/src/metrics.rs b/beacon_node/network/src/metrics.rs index e5167a065fa..cbe7f550ad8 100644 --- a/beacon_node/network/src/metrics.rs +++ b/beacon_node/network/src/metrics.rs @@ -244,7 +244,9 @@ lazy_static! { "beacon_processor_aggregated_attestation_imported_total", "Total number of aggregated attestations imported to fork choice, etc." ); +} +lazy_static! { /* * Attestation Errors */ @@ -336,6 +338,14 @@ lazy_static! { "gossipsub_attestation_error_invalid_too_many_skipped_slots", "Count of a specific error type (see metric name)" ); + pub static ref GOSSIP_ATTESTATION_ERROR_INVALID_TARGET_ROOT: Result = try_create_int_counter( + "gossip_attestation_error_invalid_target_root", + "Count of a specific error type (see metric name)" + ); + pub static ref GOSSIP_ATTESTATION_ERROR_INVALID_TARGET_EPOCH: Result = try_create_int_counter( + "gossip_attestation_error_invalid_target_epoch", + "Count of a specific error type (see metric name)" + ); pub static ref GOSSIP_ATTESTATION_ERROR_BEACON_CHAIN_ERROR: Result = try_create_int_counter( "gossipsub_attestation_error_beacon_chain_error", "Count of a specific error type (see metric name)" @@ -393,6 +403,12 @@ pub fn register_attestation_error(error: &AttnError) { inc_counter(&GOSSIP_ATTESTATION_ERROR_INVALID_SUBNET_ID) } AttnError::Invalid(_) => inc_counter(&GOSSIP_ATTESTATION_ERROR_INVALID_STATE_PROCESSING), + AttnError::InvalidTargetRoot { .. } => { + inc_counter(&GOSSIP_ATTESTATION_ERROR_INVALID_TARGET_ROOT) + } + AttnError::InvalidTargetEpoch { .. } => { + inc_counter(&GOSSIP_ATTESTATION_ERROR_INVALID_TARGET_EPOCH) + } AttnError::TooManySkippedSlots { .. } => { inc_counter(&GOSSIP_ATTESTATION_ERROR_INVALID_TOO_MANY_SKIPPED_SLOTS) } diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index 05f6c5ec4de..451f3999313 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -18,6 +18,7 @@ pub struct VoteTracker { /// A block that is to be applied to the fork choice. /// /// A simplified version of `types::BeaconBlock`. +#[derive(Clone, Debug, PartialEq)] pub struct Block { pub slot: Slot, pub root: Hash256,