From 811248de7778f9ded71f167886651b60b674f6de Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Sat, 23 Nov 2019 21:20:04 +1100 Subject: [PATCH] Add comments, more validity checks --- validator_client/src/block_service.rs | 4 +++ validator_client/src/duties_service.rs | 42 ++++++++++++++++++++++++-- validator_client/src/fork_service.rs | 6 ++++ 3 files changed, 50 insertions(+), 2 deletions(-) diff --git a/validator_client/src/block_service.rs b/validator_client/src/block_service.rs index 293c24f446..20939ee748 100644 --- a/validator_client/src/block_service.rs +++ b/validator_client/src/block_service.rs @@ -82,6 +82,7 @@ impl BlockServiceBuilder { } } +/// Helper to minimise `Arc` usage. pub struct Inner { duties_service: DutiesService, validator_store: ValidatorStore, @@ -90,6 +91,7 @@ pub struct Inner { context: RuntimeContext, } +/// Attempts to produce attestations for any block producer(s) at the start of the epoch. pub struct BlockService { inner: Arc>, } @@ -111,6 +113,7 @@ impl Deref for BlockService { } impl BlockService { + /// Starts the service that periodically attempts to produce blocks. pub fn start_update_service(&self, spec: &ChainSpec) -> Result { let log = self.context.log.clone(); @@ -153,6 +156,7 @@ impl BlockService { Ok(exit_signal) } + /// Attempt to produce a block for any block producers in the `ValidatorStore`. fn do_update(self) -> impl Future { let service = self.clone(); let log = self.context.log.clone(); diff --git a/validator_client/src/duties_service.rs b/validator_client/src/duties_service.rs index 50793c4e8c..3d63b80f47 100644 --- a/validator_client/src/duties_service.rs +++ b/validator_client/src/duties_service.rs @@ -22,9 +22,15 @@ const PRUNE_DEPTH: u64 = 4; type BaseHashMap = HashMap>; enum InsertOutcome { + /// The duties were previously unknown and have been stored. New, + /// The duties were identical to some already in the store. Identical, + /// There were duties for this validator and epoch in the store that were different to the ones + /// provided. The existing duties were replaced. Replaced, + /// The given duties were invalid. + Invalid, } #[derive(Default)] @@ -74,9 +80,13 @@ impl DutiesStore { .collect() } - fn insert(&self, epoch: Epoch, duties: ValidatorDuty) -> InsertOutcome { + fn insert(&self, epoch: Epoch, duties: ValidatorDuty, slots_per_epoch: u64) -> InsertOutcome { let mut store = self.store.write(); + if !duties_match_epoch(&duties, epoch, slots_per_epoch) { + return InsertOutcome::Invalid; + } + if store.contains_key(&duties.validator_pubkey) { let validator_map = store.get_mut(&duties.validator_pubkey).expect( "Store is exclusively locked and this path is guarded to ensure the key exists.", @@ -340,15 +350,26 @@ impl DutiesService { let mut new = 0; let mut identical = 0; let mut replaced = 0; + let mut invalid = 0; all_duties.into_iter().for_each(|duties| { - match service_2.store.insert(epoch, duties) { + match service_2.store.insert(epoch, duties, E::slots_per_epoch()) { InsertOutcome::New => new += 1, InsertOutcome::Identical => identical += 1, InsertOutcome::Replaced => replaced += 1, + InsertOutcome::Invalid => invalid += 1, }; }); + if invalid > 0 { + error!( + service_2.context.log, + "Received invalid duties from beacon node"; + "bad_duty_count" => invalid, + "info" => "Duties are from wrong epoch." + ) + } + trace!( service_2.context.log, "Performed duties update"; @@ -368,3 +389,20 @@ impl DutiesService { }) } } + +/// Returns `true` if the slots in the `duties` are from the given `epoch` +fn duties_match_epoch(duties: &ValidatorDuty, epoch: Epoch, slots_per_epoch: u64) -> bool { + if let Some(attestation_slot) = duties.attestation_slot { + if attestation_slot.epoch(slots_per_epoch) != epoch { + return false; + } + } + + if let Some(block_proposal_slot) = duties.block_proposal_slot { + if block_proposal_slot.epoch(slots_per_epoch) != epoch { + return false; + } + } + + true +} diff --git a/validator_client/src/fork_service.rs b/validator_client/src/fork_service.rs index d694cf2eec..4bc8fb355c 100644 --- a/validator_client/src/fork_service.rs +++ b/validator_client/src/fork_service.rs @@ -14,6 +14,7 @@ use types::{ChainSpec, EthSpec, Fork}; /// Delay this period of time after the slot starts. This allows the node to process the new slot. const TIME_DELAY_FROM_SLOT: Duration = Duration::from_millis(80); +/// Builds a `ForkService`. pub struct ForkServiceBuilder { fork: Option, slot_clock: Option, @@ -64,6 +65,7 @@ impl ForkServiceBuilder { } } +/// Helper to minimise `Arc` usage. pub struct Inner { fork: RwLock>, beacon_node: RemoteBeaconNode, @@ -71,6 +73,7 @@ pub struct Inner { slot_clock: T, } +/// Attempts to download the `Fork` struct from the beacon node at the start of each epoch. pub struct ForkService { inner: Arc>, } @@ -92,10 +95,12 @@ impl Deref for ForkService { } impl ForkService { + /// Returns the last fork downloaded from the beacon node, if any. pub fn fork(&self) -> Option { self.fork.read().clone() } + /// Starts the service that periodically polls for the `Fork`. pub fn start_update_service(&self, spec: &ChainSpec) -> Result { let log = self.context.log.clone(); @@ -141,6 +146,7 @@ impl ForkService { Ok(exit_signal) } + /// Attempts to download the `Fork` from the server. fn do_update(&self) -> impl Future { let service_1 = self.clone(); let log_1 = service_1.context.log.clone();