diff --git a/beacon_node/network/src/message_handler.rs b/beacon_node/network/src/message_handler.rs index 95692d4e84..758a3f1b29 100644 --- a/beacon_node/network/src/message_handler.rs +++ b/beacon_node/network/src/message_handler.rs @@ -201,13 +201,10 @@ impl MessageHandler { match gossip_message { PubsubMessage::Block(message) => match self.decode_gossip_block(message) { Ok(block) => { - let should_forward_on = self - .message_processor - .on_block_gossip(peer_id.clone(), block); - // TODO: Apply more sophisticated validation and decoding logic - 524 verify block - if should_forward_on { + if self.message_processor.should_forward_block(block.clone()) { self.propagate_message(id, peer_id.clone()); } + self.message_processor.on_block_gossip(peer_id.clone(), block); } Err(e) => { debug!(self.log, "Invalid gossiped beacon block"; "peer_id" => format!("{}", peer_id), "Error" => format!("{:?}", e)); @@ -215,10 +212,8 @@ impl MessageHandler { }, PubsubMessage::Attestation(message) => match self.decode_gossip_attestation(message) { Ok(attestation) => { - // TODO: Apply more sophisticated validation and decoding logic - 524 verify attestation - let should_forward_on = self.message_processor - .on_attestation_gossip(peer_id.clone(), attestation); - if should_forward_on { + self.message_processor.on_attestation_gossip(peer_id.clone(), attestation.clone()); + if self.message_processor.should_forward_attestation(attestation) { self.propagate_message(id, peer_id); } } diff --git a/beacon_node/network/src/sync/simple_sync.rs b/beacon_node/network/src/sync/simple_sync.rs index 0e0a1c6519..065c6429c9 100644 --- a/beacon_node/network/src/sync/simple_sync.rs +++ b/beacon_node/network/src/sync/simple_sync.rs @@ -14,7 +14,7 @@ use tokio::sync::{mpsc, oneshot}; use tree_hash::SignedRoot; use types::{Attestation, BeaconBlock, Epoch, EthSpec, Hash256, Slot, BeaconState, RelativeEpoch, Domain}; use bls::SignatureSet; -use state_processing::per_slot_processing; +use state_processing::{per_slot_processing, common::get_indexed_attestation, per_block_processing::signature_sets::indexed_attestation_signature_set}; //TODO: Put a maximum limit on the number of block that can be requested. //TODO: Rate limit requests @@ -23,9 +23,6 @@ use state_processing::per_slot_processing; /// Otherwise we queue it. pub(crate) const FUTURE_SLOT_TOLERANCE: u64 = 1; -const SHOULD_FORWARD_GOSSIP_BLOCK: bool = true; -const SHOULD_NOT_FORWARD_GOSSIP_BLOCK: bool = false; - /// Keeps track of syncing information for known connected peers. #[derive(Clone, Copy, Debug)] pub struct PeerSyncInfo { @@ -399,30 +396,19 @@ impl MessageProcessor { /// If we don't know the block's parent, we don't know who is supposed to sign it, therefore we can /// not validate the signature. /// 1.) We can try to look up the block's parent using the syncing thread. (it's expensive not sure how to reprop from sync thread) - pub fn on_block_gossip(&mut self, peer_id: PeerId, block: BeaconBlock) -> bool { + pub fn on_block_gossip(&mut self, peer_id: PeerId, block: BeaconBlock) { match self.chain.process_block(block.clone()) { Ok(outcome) => match outcome { BlockProcessingOutcome::Processed { .. } => { trace!(self.log, "Gossipsub block processed"; "peer_id" => format!("{:?}",peer_id)); - SHOULD_FORWARD_GOSSIP_BLOCK } BlockProcessingOutcome::ParentUnknown { parent: _ } => { // Inform the sync manager to find parents for this block trace!(self.log, "Block with unknown parent received"; "peer_id" => format!("{:?}",peer_id)); self.send_to_sync(SyncMessage::UnknownBlock(peer_id, block.clone())); - SHOULD_NOT_FORWARD_GOSSIP_BLOCK } - BlockProcessingOutcome::FutureSlot { - present_slot, - block_slot, - } if present_slot + FUTURE_SLOT_TOLERANCE >= block_slot => { - self.should_forward_block(block) - } - BlockProcessingOutcome::BlockIsAlreadyKnown => { - self.should_forward_block(block) - }, other => { warn!( self.log, @@ -436,7 +422,6 @@ impl MessageProcessor { "Invalid gossip beacon block ssz"; "ssz" => format!("0x{}", hex::encode(block.as_ssz_bytes())), ); - SHOULD_NOT_FORWARD_GOSSIP_BLOCK //TODO: Decide if we want to forward these } }, Err(e) => { @@ -451,84 +436,115 @@ impl MessageProcessor { "Erroneous gossip beacon block ssz"; "ssz" => format!("0x{}", hex::encode(block.as_ssz_bytes())), ); - SHOULD_NOT_FORWARD_GOSSIP_BLOCK } } } - fn should_forward_block(&mut self, block: BeaconBlock) -> bool { - // Retrieve the parent block, if it exists. - if let Ok(Some(parent_block)) = self + pub fn should_forward_block(&mut self, block: BeaconBlock) -> bool { + // Retrieve the parent block used to generate the signature. + // This will eventually return false if this operation fails or returns an empty option. + let parent_block_opt = if let Ok(Some(parent_block)) = self .chain .store - .get::>(&block.parent_root) - { - // Check if the parent block's state is equal to the current state, if it is, then - // we can validate the block using the current chain. - let mut state = - match self.chain.head().beacon_state_root == parent_block.state_root { - true => self.chain.head().beacon_state.clone(), - false => self - .chain - .store - .get::>(&parent_block.state_root) - .unwrap().unwrap() // should handle better - }; + .get::>(&block.parent_root) { + // Check if the parent block's state root is equal to the current state, if it is, then + // we can validate the block using the state in our chain head. This saves us from + // having to make an unecessary database read. + let state_res = if self.chain.head().beacon_state_root == parent_block.state_root { + Ok(Some(self.chain.head().beacon_state.clone())) + } else { + self.chain + .store + .get::>(&parent_block.state_root) + }; + + // If we are unable to find a state for the block, we eventually return false. This + // should never be the case though. + match state_res { + Ok(Some(state)) => Some((parent_block, state)), + _ => None + } + } else { + None + }; + + // If we found a parent block and state to validate the signature with, we enter this + // section, otherwise, we return false. + if let Some((parent_block, mut state)) = parent_block_opt { // Determine the epochal relationship between the state used to validate the block and // the block itself. - let relative_epoch = RelativeEpoch::from_slot( + let relative_epoch = if let Ok(relative_epoch) = RelativeEpoch::from_slot( parent_block.slot, block.slot, T::EthSpec::slots_per_epoch() - ).map_err(|_| -> Result { + ) { + relative_epoch + } else { + // This section is entered if the block being verified is to far from the parent to + // have a RelativeEpoch. + + // We make sure the block being verified follows the parent's slot. + if state.slot.as_u64() > block.slot.as_u64() { + return false; + } + // If the block is more than one epoch in the future, we must fast-forward to // the state and compute the committee. for _ in state.slot.as_u64()..block.slot.as_u64() { - per_slot_processing(&mut state, &self.chain.spec); + if per_slot_processing(&mut state, &self.chain.spec).is_err() { + // Return false if something goes wrong. + return false; + } } - // Note: this is expensive. - state.build_committee_cache(RelativeEpoch::Current, &self.chain.spec); + // Compute the committee cache so we can check the proposer. + // TODO: Downvote peer + if state.build_committee_cache(RelativeEpoch::Current, &self.chain.spec).is_err() { + return false; + } - Ok(RelativeEpoch::Current) - }).unwrap(); + // The relative epoch for the state is now Current. + RelativeEpoch::Current + }; // Retrieve the block's designated proposer. - let proposer_index = state + let proposer_result = state .get_beacon_proposer_index( block.slot, relative_epoch, &self.chain.spec - ).unwrap(); - let proposer = state.validators.get(proposer_index).unwrap(); + ).map( |i| state.validators.get(i)); - // Create a SignatureSet and validate it. + // Generate the domain that should have been used to create the signature. let domain = self.chain.spec.get_domain( block.slot.epoch(T::EthSpec::slots_per_epoch()), Domain::BeaconProposer, &state.fork ); - let set = SignatureSet::single( - &block.signature, - &proposer.pubkey, - block.signed_root(), - domain - ); + // Verify the signature if we were able to get a proposer, otherwise, we eventually + // return false. + if let Ok(Some(proposer)) = proposer_result { + let signature = SignatureSet::single( + &block.signature, + &proposer.pubkey, + block.signed_root(), + domain + ); - return set.is_valid(); + // TODO: downvote if the signature is invalid. + return signature.is_valid(); + } } - // The signature can not be verified without the parent's state, so in this case, we simply - // do not forward. false } /// Process a gossip message declaring a new attestation. /// /// Not currently implemented. - pub fn on_attestation_gossip(&mut self, _peer_id: PeerId, msg: Attestation) -> bool { + pub fn on_attestation_gossip(&mut self, _peer_id: PeerId, msg: Attestation) { match self.chain.process_attestation(msg.clone()) { Ok(outcome) => { info!( @@ -555,12 +571,58 @@ impl MessageProcessor { error!(self.log, "Invalid gossip attestation"; "error" => format!("{:?}", e)); } }; - - true } - fn should_forward_attestation(&self, attestation: Attestation) -> bool { - true + pub fn should_forward_attestation(&self, attestation: Attestation) -> bool { + // Attempt to validate the attestation's signature against the head state. + // In this case, we do not read anything from the database, which should be fast and will + // work for most attestations getting passed around the network. + let head_state = &self.chain.head().beacon_state; + + // Convert the attestation to an indexed attestation. + if let Ok(indexed_attestation) = get_indexed_attestation(&head_state, &attestation) { + // Validate the signature and return true if it is valid. Otherwise, we move on and read + // the database to make certain we have the correct state. + if let Ok(signature) = indexed_attestation_signature_set( + &head_state, + &indexed_attestation.signature, + &indexed_attestation, + &self.chain.spec + ) { + // An invalid signature here does not necessarily mean the attestation is invalid. + // It could be the case that our head state does not have the same registry. + if signature.is_valid() { + return true; + } + } + } + + // Retrieve the block being attested to. + if let Ok(Some(block)) = self + .chain + .store + .get::>(&attestation.data.beacon_block_root) + { + if let Ok(Some(state)) = self.chain.store.get::>(&block.state_root) { + // Convert the attestation to an indexed attestation. + if let Ok(indexed_attestation) = get_indexed_attestation(&state, &attestation) { + // Validate the indexed attestation against the state we retrieved using the + // attestation's LMD Ghost vote. + if let Ok(signature) = indexed_attestation_signature_set( + &state, + &indexed_attestation.signature, + &indexed_attestation, + &self.chain.spec + ) { + // TODO: Downvote peer if the signature is invalid. + // Also maybe downvote the peer for getting here in the first place. + return signature.is_valid(); + } + } + } + } + + false } } diff --git a/eth2/state_processing/src/per_block_processing.rs b/eth2/state_processing/src/per_block_processing.rs index fec16c5b95..04b42e9ac5 100644 --- a/eth2/state_processing/src/per_block_processing.rs +++ b/eth2/state_processing/src/per_block_processing.rs @@ -29,7 +29,7 @@ pub mod block_processing_builder; mod block_signature_verifier; pub mod errors; mod is_valid_indexed_attestation; -mod signature_sets; +pub mod signature_sets; pub mod tests; mod verify_attestation; mod verify_attester_slashing;