From 5c1d0dcea51e26e12e4caeacda4d7cc0388a9992 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Wed, 13 Feb 2019 07:32:31 +1100 Subject: [PATCH] Fix various clippy lints --- .../test_harness/src/beacon_chain_harness.rs | 5 +- .../validator_harness/direct_beacon_node.rs | 2 +- .../src/validator_harness/direct_duties.rs | 2 +- beacon_node/src/rpc/beacon_block.rs | 2 +- .../src/test_utils/simulated_beacon_node.rs | 1 - .../state_processing/src/block_processable.rs | 2 +- .../state_processing/src/epoch_processable.rs | 31 ++++------ eth2/state_processing/src/slot_processable.rs | 2 - eth2/types/src/attestation.rs | 1 - eth2/types/src/beacon_state.rs | 16 ++---- eth2/types/src/slot_epoch.rs | 26 +++++++-- eth2/types/src/validator.rs | 4 +- .../beacon_block_grpc_client.rs | 5 +- .../block_producer_service.rs | 53 ----------------- .../src/block_producer_service/mod.rs | 57 ++++++++++++++++++- validator_client/src/duties/epoch_duties.rs | 2 +- validator_client/src/duties/mod.rs | 4 +- 17 files changed, 105 insertions(+), 110 deletions(-) delete mode 100644 validator_client/src/block_producer_service/block_producer_service.rs diff --git a/beacon_node/beacon_chain/test_harness/src/beacon_chain_harness.rs b/beacon_node/beacon_chain/test_harness/src/beacon_chain_harness.rs index 09080c60d..f551c94c2 100644 --- a/beacon_node/beacon_chain/test_harness/src/beacon_chain_harness.rs +++ b/beacon_node/beacon_chain/test_harness/src/beacon_chain_harness.rs @@ -61,8 +61,7 @@ impl BeaconChainHarness { debug!("Creating validator deposits..."); - let mut initial_validator_deposits = Vec::with_capacity(validator_count); - initial_validator_deposits = keypairs + let initial_validator_deposits = keypairs .par_iter() .map(|keypair| Deposit { branch: vec![], // branch verification is not specified. @@ -235,7 +234,7 @@ impl BeaconChainHarness { } /// Write the output of `chain_dump` to a JSON file. - pub fn dump_to_file(&self, filename: String, chain_dump: &Vec) { + pub fn dump_to_file(&self, filename: String, chain_dump: &[CheckPoint]) { let json = serde_json::to_string(chain_dump).unwrap(); let mut file = File::create(filename).unwrap(); file.write_all(json.as_bytes()) diff --git a/beacon_node/beacon_chain/test_harness/src/validator_harness/direct_beacon_node.rs b/beacon_node/beacon_chain/test_harness/src/validator_harness/direct_beacon_node.rs index 79d2cbc83..7c822ced0 100644 --- a/beacon_node/beacon_chain/test_harness/src/validator_harness/direct_beacon_node.rs +++ b/beacon_node/beacon_chain/test_harness/src/validator_harness/direct_beacon_node.rs @@ -80,7 +80,7 @@ impl BeaconBlockNode for DirectBeaconNode { .beacon_chain .produce_block(randao_reveal.clone()) .ok_or_else(|| { - BeaconBlockNodeError::RemoteFailure(format!("Did not produce block.")) + BeaconBlockNodeError::RemoteFailure("Did not produce block.".to_string()) })?; if block.slot == slot { diff --git a/beacon_node/beacon_chain/test_harness/src/validator_harness/direct_duties.rs b/beacon_node/beacon_chain/test_harness/src/validator_harness/direct_duties.rs index eac56679b..cabbbd8a7 100644 --- a/beacon_node/beacon_chain/test_harness/src/validator_harness/direct_duties.rs +++ b/beacon_node/beacon_chain/test_harness/src/validator_harness/direct_duties.rs @@ -60,7 +60,7 @@ impl AttesterDutiesReader for DirectDuties { } Ok(Some(_)) => Ok(None), Ok(None) => Err(AttesterDutiesReaderError::UnknownEpoch), - Err(_) => panic!("Error when getting validator attestation shard."), + Err(_) => unreachable!("Error when getting validator attestation shard."), } } else { Err(AttesterDutiesReaderError::UnknownValidator) diff --git a/beacon_node/src/rpc/beacon_block.rs b/beacon_node/src/rpc/beacon_block.rs index a047365ef..96f64e0dd 100644 --- a/beacon_node/src/rpc/beacon_block.rs +++ b/beacon_node/src/rpc/beacon_block.rs @@ -25,7 +25,7 @@ impl BeaconBlockService for BeaconBlockServiceInstance { // TODO: build a legit block. let mut block = BeaconBlockProto::new(); block.set_slot(req.get_slot()); - block.set_block_root("cats".as_bytes().to_vec()); + block.set_block_root(b"cats".to_vec()); let mut resp = ProduceBeaconBlockResponse::new(); resp.set_block(block); diff --git a/eth2/block_producer/src/test_utils/simulated_beacon_node.rs b/eth2/block_producer/src/test_utils/simulated_beacon_node.rs index ec2695d56..c0a12c1ac 100644 --- a/eth2/block_producer/src/test_utils/simulated_beacon_node.rs +++ b/eth2/block_producer/src/test_utils/simulated_beacon_node.rs @@ -2,7 +2,6 @@ use crate::traits::{BeaconNode, BeaconNodeError, PublishOutcome}; use std::sync::RwLock; use types::{BeaconBlock, Signature, Slot}; -type NonceResult = Result; type ProduceResult = Result, BeaconNodeError>; type PublishResult = Result; diff --git a/eth2/state_processing/src/block_processable.rs b/eth2/state_processing/src/block_processable.rs index 3750529cd..76c87698a 100644 --- a/eth2/state_processing/src/block_processable.rs +++ b/eth2/state_processing/src/block_processable.rs @@ -333,7 +333,7 @@ fn validate_attestation_signature_optional( ensure!( (attestation.data.latest_crosslink == state.latest_crosslinks[attestation.data.shard as usize]) - || (attestation.data.latest_crosslink + | (attestation.data.latest_crosslink == state.latest_crosslinks[attestation.data.shard as usize]), AttestationValidationError::BadLatestCrosslinkRoot ); diff --git a/eth2/state_processing/src/epoch_processable.rs b/eth2/state_processing/src/epoch_processable.rs index 3bc0d7cb9..4afa725ce 100644 --- a/eth2/state_processing/src/epoch_processable.rs +++ b/eth2/state_processing/src/epoch_processable.rs @@ -226,7 +226,7 @@ impl EpochProcessable for BeaconState { */ let mut new_justified_epoch = self.justified_epoch; - self.justification_bitfield = self.justification_bitfield << 1; + self.justification_bitfield <<= 1; // If > 2/3 of the total balance attested to the previous epoch boundary // @@ -277,8 +277,7 @@ impl EpochProcessable for BeaconState { // - The presently justified epoch was two epochs ago. // // Then, set the finalized epoch to two epochs ago. - if ((self.justification_bitfield >> 0) % 8 == 0b111) - & (self.justified_epoch == previous_epoch - 1) + if (self.justification_bitfield % 8 == 0b111) & (self.justified_epoch == previous_epoch - 1) { self.finalized_epoch = self.justified_epoch; trace!("epoch - 2 was finalized (3rd condition)."); @@ -289,9 +288,7 @@ impl EpochProcessable for BeaconState { // - Set the previous epoch to be justified. // // Then, set the finalized epoch to be the previous epoch. - if ((self.justification_bitfield >> 0) % 4 == 0b11) - & (self.justified_epoch == previous_epoch) - { + if (self.justification_bitfield % 4 == 0b11) & (self.justified_epoch == previous_epoch) { self.finalized_epoch = self.justified_epoch; trace!("epoch - 1 was finalized (4th condition)."); } @@ -386,10 +383,8 @@ impl EpochProcessable for BeaconState { base_reward * previous_epoch_justified_attesting_balance / previous_total_balance ); - } else { - if active_validator_indices_hashset.contains(&index) { - safe_sub_assign!(self.validator_balances[index], base_reward); - } + } else if active_validator_indices_hashset.contains(&index) { + safe_sub_assign!(self.validator_balances[index], base_reward); } if previous_epoch_boundary_attester_indices_hashset.contains(&index) { @@ -398,10 +393,8 @@ impl EpochProcessable for BeaconState { base_reward * previous_epoch_boundary_attesting_balance / previous_total_balance ); - } else { - if active_validator_indices_hashset.contains(&index) { - safe_sub_assign!(self.validator_balances[index], base_reward); - } + } else if active_validator_indices_hashset.contains(&index) { + safe_sub_assign!(self.validator_balances[index], base_reward); } if previous_epoch_head_attester_indices_hashset.contains(&index) { @@ -410,10 +403,8 @@ impl EpochProcessable for BeaconState { base_reward * previous_epoch_head_attesting_balance / previous_total_balance ); - } else { - if active_validator_indices_hashset.contains(&index) { - safe_sub_assign!(self.validator_balances[index], base_reward); - } + } else if active_validator_indices_hashset.contains(&index) { + safe_sub_assign!(self.validator_balances[index], base_reward); } } @@ -505,7 +496,7 @@ impl EpochProcessable for BeaconState { if let Some(Ok(winning_root)) = winning_root_for_shards.get(&shard) { // TODO: remove the map. let attesting_validator_indices: HashSet = HashSet::from_iter( - winning_root.attesting_validator_indices.iter().map(|i| *i), + winning_root.attesting_validator_indices.iter().cloned(), ); for index in 0..self.validator_balances.len() { @@ -664,7 +655,7 @@ fn winning_root( } let candidate_root = WinningRoot { - shard_block_root: shard_block_root.clone(), + shard_block_root: *shard_block_root, attesting_validator_indices, total_attesting_balance, total_balance, diff --git a/eth2/state_processing/src/slot_processable.rs b/eth2/state_processing/src/slot_processable.rs index f0e776f57..7726c5071 100644 --- a/eth2/state_processing/src/slot_processable.rs +++ b/eth2/state_processing/src/slot_processable.rs @@ -30,8 +30,6 @@ where self.slot += 1; - let block_proposer = self.get_beacon_proposer_index(self.slot, spec)?; - self.latest_randao_mixes[self.slot.as_usize() % spec.latest_randao_mixes_length] = self.latest_randao_mixes[(self.slot.as_usize() - 1) % spec.latest_randao_mixes_length]; diff --git a/eth2/types/src/attestation.rs b/eth2/types/src/attestation.rs index afffc2e2f..eb375d490 100644 --- a/eth2/types/src/attestation.rs +++ b/eth2/types/src/attestation.rs @@ -1,6 +1,5 @@ use super::{AggregatePublicKey, AggregateSignature, AttestationData, Bitfield, Hash256}; use crate::test_utils::TestRandom; -use bls::bls_verify_aggregate; use rand::RngCore; use serde_derive::Serialize; use ssz::{hash, Decodable, DecodeError, Encodable, SszStream, TreeHash}; diff --git a/eth2/types/src/beacon_state.rs b/eth2/types/src/beacon_state.rs index e11b43e80..ed53bfea9 100644 --- a/eth2/types/src/beacon_state.rs +++ b/eth2/types/src/beacon_state.rs @@ -283,7 +283,7 @@ impl BeaconState { shuffled_active_validator_indices .honey_badger_split(committees_per_epoch as usize) - .filter_map(|slice: &[usize]| Some(slice.to_vec())) + .map(|slice: &[usize]| slice.to_vec()) .collect() } @@ -522,11 +522,9 @@ impl BeaconState { .filter(|i| eligible(*i)) .collect(); eligable_indices.sort_by_key(|i| self.validator_registry[*i].exit_epoch); - let mut withdrawn_so_far = 0; - for index in eligable_indices { - self.prepare_validator_for_withdrawal(index); - withdrawn_so_far += 1; - if withdrawn_so_far >= spec.max_withdrawals_per_epoch { + for (withdrawn_so_far, index) in eligable_indices.iter().enumerate() { + self.prepare_validator_for_withdrawal(*index); + if withdrawn_so_far as u64 >= spec.max_withdrawals_per_epoch { break; } } @@ -796,11 +794,7 @@ impl BeaconState { for (i, a) in attestations.iter().enumerate() { let participants = self.get_attestation_participants(&a.data, &a.aggregation_bitfield, spec)?; - if participants - .iter() - .find(|i| **i == validator_index) - .is_some() - { + if participants.iter().any(|i| *i == validator_index) { included_attestations.push(i); } } diff --git a/eth2/types/src/slot_epoch.rs b/eth2/types/src/slot_epoch.rs index a810bb102..fb4f8d942 100644 --- a/eth2/types/src/slot_epoch.rs +++ b/eth2/types/src/slot_epoch.rs @@ -13,9 +13,10 @@ use crate::test_utils::TestRandom; use rand::RngCore; use serde_derive::Serialize; use slog; -use ssz::{hash, Decodable, DecodeError, Encodable, SszStream, TreeHash}; +use ssz::{hash, ssz_encode, Decodable, DecodeError, Encodable, SszStream, TreeHash}; use std::cmp::{Ord, Ordering}; use std::fmt; +use std::hash::{Hash, Hasher}; use std::iter::Iterator; use std::ops::{Add, AddAssign, Div, DivAssign, Mul, MulAssign, Rem, Sub, SubAssign}; @@ -243,6 +244,18 @@ macro_rules! impl_ssz { }; } +macro_rules! impl_hash { + ($type: ident) => { + // Implemented to stop clippy lint: + // https://rust-lang.github.io/rust-clippy/master/index.html#derive_hash_xor_eq + impl Hash for $type { + fn hash(&self, state: &mut H) { + ssz_encode(self).hash(state) + } + } + }; +} + macro_rules! impl_common { ($type: ident) => { impl_from_into_u64!($type); @@ -252,13 +265,14 @@ macro_rules! impl_common { impl_math!($type); impl_display!($type); impl_ssz!($type); + impl_hash!($type); }; } -#[derive(Eq, Debug, Clone, Copy, Default, Serialize, Hash)] +#[derive(Eq, Debug, Clone, Copy, Default, Serialize)] pub struct Slot(u64); -#[derive(Eq, Debug, Clone, Copy, Default, Serialize, Hash)] +#[derive(Eq, Debug, Clone, Copy, Default, Serialize)] pub struct Epoch(u64); impl_common!(Slot); @@ -269,7 +283,7 @@ impl Slot { Slot(slot) } - pub fn epoch(&self, epoch_length: u64) -> Epoch { + pub fn epoch(self, epoch_length: u64) -> Epoch { Epoch::from(self.0 / epoch_length) } @@ -287,11 +301,11 @@ impl Epoch { Epoch(u64::max_value()) } - pub fn start_slot(&self, epoch_length: u64) -> Slot { + pub fn start_slot(self, epoch_length: u64) -> Slot { Slot::from(self.0.saturating_mul(epoch_length)) } - pub fn end_slot(&self, epoch_length: u64) -> Slot { + pub fn end_slot(self, epoch_length: u64) -> Slot { Slot::from( self.0 .saturating_add(1) diff --git a/eth2/types/src/validator.rs b/eth2/types/src/validator.rs index 50eb65486..047817a86 100644 --- a/eth2/types/src/validator.rs +++ b/eth2/types/src/validator.rs @@ -78,7 +78,7 @@ impl Default for Validator { impl TestRandom for StatusFlags { fn random_for_test(rng: &mut T) -> Self { let options = vec![StatusFlags::InitiatedExit, StatusFlags::Withdrawable]; - options[(rng.next_u32() as usize) % options.len()].clone() + options[(rng.next_u32() as usize) % options.len()] } } @@ -130,7 +130,7 @@ impl TreeHash for Validator { result.append(&mut self.exit_epoch.hash_tree_root()); result.append(&mut self.withdrawal_epoch.hash_tree_root()); result.append(&mut self.penalized_epoch.hash_tree_root()); - result.append(&mut (status_flag_to_byte(self.status_flags) as u64).hash_tree_root()); + result.append(&mut u64::from(status_flag_to_byte(self.status_flags)).hash_tree_root()); hash(&result) } } diff --git a/validator_client/src/block_producer_service/beacon_block_grpc_client.rs b/validator_client/src/block_producer_service/beacon_block_grpc_client.rs index 6e038ae8f..39ef7fcdc 100644 --- a/validator_client/src/block_producer_service/beacon_block_grpc_client.rs +++ b/validator_client/src/block_producer_service/beacon_block_grpc_client.rs @@ -5,7 +5,7 @@ use protos::services::{ use protos::services_grpc::BeaconBlockServiceClient; use ssz::{ssz_encode, Decodable}; use std::sync::Arc; -use types::{BeaconBlock, BeaconBlockBody, Eth1Data, Hash256, PublicKey, Signature, Slot}; +use types::{BeaconBlock, BeaconBlockBody, Eth1Data, Hash256, Signature, Slot}; /// A newtype designed to wrap the gRPC-generated service so the `BeaconNode` trait may be /// implemented upon it. @@ -27,7 +27,8 @@ impl BeaconNode for BeaconBlockGrpcClient { fn produce_beacon_block( &self, slot: Slot, - randao_reveal: &Signature, + // TODO: use randao_reveal, when proto APIs have been updated. + _randao_reveal: &Signature, ) -> Result, BeaconNodeError> { let mut req = ProduceBeaconBlockRequest::new(); req.set_slot(slot.as_u64()); diff --git a/validator_client/src/block_producer_service/block_producer_service.rs b/validator_client/src/block_producer_service/block_producer_service.rs deleted file mode 100644 index 5e335e383..000000000 --- a/validator_client/src/block_producer_service/block_producer_service.rs +++ /dev/null @@ -1,53 +0,0 @@ -use block_producer::{ - BeaconNode, BlockProducer, DutiesReader, PollOutcome as BlockProducerPollOutcome, Signer, -}; -use slog::{error, info, warn, Logger}; -use slot_clock::SlotClock; -use std::time::Duration; - -pub struct BlockProducerService { - pub block_producer: BlockProducer, - pub poll_interval_millis: u64, - pub log: Logger, -} - -impl BlockProducerService { - /// Run a loop which polls the block producer each `poll_interval_millis` millseconds. - /// - /// Logs the results of the polls. - pub fn run(&mut self) { - loop { - match self.block_producer.poll() { - Err(error) => { - error!(self.log, "Block producer poll error"; "error" => format!("{:?}", error)) - } - Ok(BlockProducerPollOutcome::BlockProduced(slot)) => { - info!(self.log, "Produced block"; "slot" => slot) - } - Ok(BlockProducerPollOutcome::SlashableBlockNotProduced(slot)) => { - warn!(self.log, "Slashable block was not signed"; "slot" => slot) - } - Ok(BlockProducerPollOutcome::BlockProductionNotRequired(slot)) => { - info!(self.log, "Block production not required"; "slot" => slot) - } - Ok(BlockProducerPollOutcome::ProducerDutiesUnknown(slot)) => { - error!(self.log, "Block production duties unknown"; "slot" => slot) - } - Ok(BlockProducerPollOutcome::SlotAlreadyProcessed(slot)) => { - warn!(self.log, "Attempted to re-process slot"; "slot" => slot) - } - Ok(BlockProducerPollOutcome::BeaconNodeUnableToProduceBlock(slot)) => { - error!(self.log, "Beacon node unable to produce block"; "slot" => slot) - } - Ok(BlockProducerPollOutcome::SignerRejection(slot)) => { - error!(self.log, "The cryptographic signer refused to sign the block"; "slot" => slot) - } - Ok(BlockProducerPollOutcome::ValidatorIsUnknown(slot)) => { - error!(self.log, "The Beacon Node does not recognise the validator"; "slot" => slot) - } - }; - - std::thread::sleep(Duration::from_millis(self.poll_interval_millis)); - } - } -} diff --git a/validator_client/src/block_producer_service/mod.rs b/validator_client/src/block_producer_service/mod.rs index 52aac688b..82c3f2537 100644 --- a/validator_client/src/block_producer_service/mod.rs +++ b/validator_client/src/block_producer_service/mod.rs @@ -1,5 +1,58 @@ mod beacon_block_grpc_client; -mod block_producer_service; +// mod block_producer_service; + +use block_producer::{ + BeaconNode, BlockProducer, DutiesReader, PollOutcome as BlockProducerPollOutcome, Signer, +}; +use slog::{error, info, warn, Logger}; +use slot_clock::SlotClock; +use std::time::Duration; pub use self::beacon_block_grpc_client::BeaconBlockGrpcClient; -pub use self::block_producer_service::BlockProducerService; + +pub struct BlockProducerService { + pub block_producer: BlockProducer, + pub poll_interval_millis: u64, + pub log: Logger, +} + +impl BlockProducerService { + /// Run a loop which polls the block producer each `poll_interval_millis` millseconds. + /// + /// Logs the results of the polls. + pub fn run(&mut self) { + loop { + match self.block_producer.poll() { + Err(error) => { + error!(self.log, "Block producer poll error"; "error" => format!("{:?}", error)) + } + Ok(BlockProducerPollOutcome::BlockProduced(slot)) => { + info!(self.log, "Produced block"; "slot" => slot) + } + Ok(BlockProducerPollOutcome::SlashableBlockNotProduced(slot)) => { + warn!(self.log, "Slashable block was not signed"; "slot" => slot) + } + Ok(BlockProducerPollOutcome::BlockProductionNotRequired(slot)) => { + info!(self.log, "Block production not required"; "slot" => slot) + } + Ok(BlockProducerPollOutcome::ProducerDutiesUnknown(slot)) => { + error!(self.log, "Block production duties unknown"; "slot" => slot) + } + Ok(BlockProducerPollOutcome::SlotAlreadyProcessed(slot)) => { + warn!(self.log, "Attempted to re-process slot"; "slot" => slot) + } + Ok(BlockProducerPollOutcome::BeaconNodeUnableToProduceBlock(slot)) => { + error!(self.log, "Beacon node unable to produce block"; "slot" => slot) + } + Ok(BlockProducerPollOutcome::SignerRejection(slot)) => { + error!(self.log, "The cryptographic signer refused to sign the block"; "slot" => slot) + } + Ok(BlockProducerPollOutcome::ValidatorIsUnknown(slot)) => { + error!(self.log, "The Beacon Node does not recognise the validator"; "slot" => slot) + } + }; + + std::thread::sleep(Duration::from_millis(self.poll_interval_millis)); + } + } +} diff --git a/validator_client/src/duties/epoch_duties.rs b/validator_client/src/duties/epoch_duties.rs index d93b8ac2f..b555eee28 100644 --- a/validator_client/src/duties/epoch_duties.rs +++ b/validator_client/src/duties/epoch_duties.rs @@ -47,7 +47,7 @@ impl EpochDutiesMap { pub fn get(&self, epoch: Epoch) -> Result, EpochDutiesMapError> { let map = self.map.read().map_err(|_| EpochDutiesMapError::Poisoned)?; match map.get(&epoch) { - Some(duties) => Ok(Some(duties.clone())), + Some(duties) => Ok(Some(*duties)), None => Ok(None), } } diff --git a/validator_client/src/duties/mod.rs b/validator_client/src/duties/mod.rs index 936034aca..febab4755 100644 --- a/validator_client/src/duties/mod.rs +++ b/validator_client/src/duties/mod.rs @@ -12,7 +12,7 @@ use self::traits::{BeaconNode, BeaconNodeError}; use bls::PublicKey; use slot_clock::SlotClock; use std::sync::Arc; -use types::{ChainSpec, Epoch, Slot}; +use types::{ChainSpec, Epoch}; #[derive(Debug, PartialEq, Clone, Copy)] pub enum PollOutcome { @@ -33,7 +33,6 @@ pub enum Error { SlotClockError, SlotUnknowable, EpochMapPoisoned, - EpochLengthIsZero, BeaconNodeError(BeaconNodeError), } @@ -103,6 +102,7 @@ mod tests { use super::*; use bls::Keypair; use slot_clock::TestingSlotClock; + use types::Slot; // TODO: implement more thorough testing. // https://github.com/sigp/lighthouse/issues/160