From 6eaeaa542fcb3ad7204b0631de69023a9d621fcc Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Fri, 20 May 2022 05:02:13 +0000 Subject: [PATCH] Fix Rust 1.61 clippy lints (#3192) ## Issue Addressed This fixes the low-hanging Clippy lints introduced in Rust 1.61 (due any hour now). It _ignores_ one lint, because fixing it requires a structural refactor of the validator client that needs to be done delicately. I've started on that refactor and will create another PR that can be reviewed in more depth in the coming days. I think we should merge this PR in the meantime to unblock CI. --- .github/workflows/test-suite.yml | 2 +- Cargo.lock | 1 + .../beacon_chain/src/block_verification.rs | 4 +- beacon_node/network/Cargo.toml | 1 + .../network/src/beacon_processor/mod.rs | 13 ++-- .../network/src/sync/range_sync/chain.rs | 5 +- testing/simulator/src/local_network.rs | 9 +-- validator_client/src/preparation_service.rs | 70 +++++++++---------- validator_client/src/validator_store.rs | 2 + 9 files changed, 54 insertions(+), 53 deletions(-) diff --git a/.github/workflows/test-suite.yml b/.github/workflows/test-suite.yml index 57ccbdaa1..da0bcb385 100644 --- a/.github/workflows/test-suite.yml +++ b/.github/workflows/test-suite.yml @@ -12,7 +12,7 @@ env: # Deny warnings in CI RUSTFLAGS: "-D warnings" # The Nightly version used for cargo-udeps, might need updating from time to time. - PINNED_NIGHTLY: nightly-2021-12-01 + PINNED_NIGHTLY: nightly-2022-05-20 jobs: target-branch-check: name: target-branch-check diff --git a/Cargo.lock b/Cargo.lock index a35902c19..efb2c9fa3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3888,6 +3888,7 @@ name = "network" version = "0.2.0" dependencies = [ "beacon_chain", + "derivative", "environment", "error-chain", "eth2_ssz", diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 70ec48cd1..afdbaf13e 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -53,6 +53,7 @@ use crate::{ }, metrics, BeaconChain, BeaconChainError, BeaconChainTypes, }; +use derivative::Derivative; use eth2::types::EventKind; use execution_layer::PayloadStatus; use fork_choice::{ForkChoice, ForkChoiceStore, PayloadVerificationStatus}; @@ -537,7 +538,8 @@ pub fn signature_verify_chain_segment( /// A wrapper around a `SignedBeaconBlock` that indicates it has been approved for re-gossiping on /// the p2p network. -#[derive(Debug)] +#[derive(Derivative)] +#[derivative(Debug(bound = "T: BeaconChainTypes"))] pub struct GossipVerifiedBlock { pub block: SignedBeaconBlock, pub block_root: Hash256, diff --git a/beacon_node/network/Cargo.toml b/beacon_node/network/Cargo.toml index 3688baf34..5aae8652e 100644 --- a/beacon_node/network/Cargo.toml +++ b/beacon_node/network/Cargo.toml @@ -43,3 +43,4 @@ lru_cache = { path = "../../common/lru_cache" } if-addrs = "0.6.4" strum = "0.24.0" tokio-util = { version = "0.6.3", features = ["time"] } +derivative = "2.2.0" diff --git a/beacon_node/network/src/beacon_processor/mod.rs b/beacon_node/network/src/beacon_processor/mod.rs index 513949bcc..3e25bd144 100644 --- a/beacon_node/network/src/beacon_processor/mod.rs +++ b/beacon_node/network/src/beacon_processor/mod.rs @@ -42,6 +42,7 @@ use crate::sync::manager::BlockProcessType; use crate::{metrics, service::NetworkMessage, sync::SyncMessage}; use beacon_chain::parking_lot::Mutex; use beacon_chain::{BeaconChain, BeaconChainTypes, GossipVerifiedBlock}; +use derivative::Derivative; use futures::stream::{Stream, StreamExt}; use futures::task::Poll; use lighthouse_network::{ @@ -51,7 +52,6 @@ use lighthouse_network::{ use logging::TimeLatch; use slog::{crit, debug, error, trace, warn, Logger}; use std::collections::VecDeque; -use std::fmt; use std::pin::Pin; use std::sync::{Arc, Weak}; use std::task::Context; @@ -331,17 +331,13 @@ impl DuplicateCache { } /// An event to be processed by the manager task. +#[derive(Derivative)] +#[derivative(Debug(bound = "T: BeaconChainTypes"))] pub struct WorkEvent { drop_during_sync: bool, work: Work, } -impl fmt::Debug for WorkEvent { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{:?}", self) - } -} - impl WorkEvent { /// Create a new `Work` event for some unaggregated attestation. pub fn unaggregated_attestation( @@ -615,7 +611,8 @@ impl std::convert::From> for WorkEvent { } /// A consensus message (or multiple) from the network that requires processing. -#[derive(Debug)] +#[derive(Derivative)] +#[derivative(Debug(bound = "T: BeaconChainTypes"))] pub enum Work { GossipAttestation { message_id: MessageId, diff --git a/beacon_node/network/src/sync/range_sync/chain.rs b/beacon_node/network/src/sync/range_sync/chain.rs index 3f8164721..9f4142dd6 100644 --- a/beacon_node/network/src/sync/range_sync/chain.rs +++ b/beacon_node/network/src/sync/range_sync/chain.rs @@ -26,8 +26,9 @@ const BATCH_BUFFER_SIZE: u8 = 5; /// A return type for functions that act on a `Chain` which informs the caller whether the chain /// has been completed and should be removed or to be kept if further processing is /// required. -#[must_use = "Should be checked, since a failed chain must be removed. A chain that requested - being removed and continued is now in an inconsistent state"] +/// +/// Should be checked, since a failed chain must be removed. A chain that requested being removed +/// and continued is now in an inconsistent state. pub type ProcessingResult = Result; /// Reasons for removing a chain diff --git a/testing/simulator/src/local_network.rs b/testing/simulator/src/local_network.rs index 3668cf006..6cfc3e6db 100644 --- a/testing/simulator/src/local_network.rs +++ b/testing/simulator/src/local_network.rs @@ -107,15 +107,16 @@ impl LocalNetwork { beacon_config.network.discv5_config.table_filter = |_| true; } - let mut write_lock = self_1.beacon_nodes.write(); - let index = write_lock.len(); - + // We create the beacon node without holding the lock, so that the lock isn't held + // across the await. This is only correct if this function never runs in parallel + // with itself (which at the time of writing, it does not). + let index = self_1.beacon_nodes.read().len(); let beacon_node = LocalBeaconNode::production( self.context.service_context(format!("node_{}", index)), beacon_config, ) .await?; - write_lock.push(beacon_node); + self_1.beacon_nodes.write().push(beacon_node); Ok(()) } diff --git a/validator_client/src/preparation_service.rs b/validator_client/src/preparation_service.rs index ad04717cc..b4b6caa05 100644 --- a/validator_client/src/preparation_service.rs +++ b/validator_client/src/preparation_service.rs @@ -199,7 +199,8 @@ impl PreparationService { .map_err(|e| { error!( log, - "{}", format!("Error loading fee-recipient file: {:?}", e); + "Error loading fee-recipient file"; + "error" => ?e ); }) .unwrap_or(()); @@ -213,44 +214,39 @@ impl PreparationService { all_pubkeys .into_iter() .filter_map(|pubkey| { - let validator_index = self.validator_store.validator_index(&pubkey); - if let Some(validator_index) = validator_index { - let fee_recipient = if let Some(from_validator_defs) = - self.validator_store.suggested_fee_recipient(&pubkey) - { - // If there is a `suggested_fee_recipient` in the validator definitions yaml - // file, use that value. - Some(from_validator_defs) - } else { - // If there's nothing in the validator defs file, check the fee recipient - // file. - fee_recipient_file - .as_ref() - .and_then(|f| match f.get_fee_recipient(&pubkey) { - Ok(f) => f, - Err(_e) => None, - }) - // If there's nothing in the file, try the process-level default value. - .or(self.fee_recipient) - }; + // Ignore fee recipients for keys without indices, they are inactive. + let validator_index = self.validator_store.validator_index(&pubkey)?; - if let Some(fee_recipient) = fee_recipient { - Some(ProposerPreparationData { - validator_index, - fee_recipient, - }) - } else { - if spec.bellatrix_fork_epoch.is_some() { - error!( - log, - "Validator is missing fee recipient"; - "msg" => "update validator_definitions.yml", - "pubkey" => ?pubkey - ); - } - None - } + // If there is a `suggested_fee_recipient` in the validator definitions yaml + // file, use that value. + let fee_recipient = self + .validator_store + .suggested_fee_recipient(&pubkey) + .or_else(|| { + // If there's nothing in the validator defs file, check the fee + // recipient file. + fee_recipient_file + .as_ref()? + .get_fee_recipient(&pubkey) + .ok()? + }) + // If there's nothing in the file, try the process-level default value. + .or(self.fee_recipient); + + if let Some(fee_recipient) = fee_recipient { + Some(ProposerPreparationData { + validator_index, + fee_recipient, + }) } else { + if spec.bellatrix_fork_epoch.is_some() { + error!( + log, + "Validator is missing fee recipient"; + "msg" => "update validator_definitions.yml", + "pubkey" => ?pubkey + ); + } None } }) diff --git a/validator_client/src/validator_store.rs b/validator_client/src/validator_store.rs index 5abe37f43..b39ef9ef8 100644 --- a/validator_client/src/validator_store.rs +++ b/validator_client/src/validator_store.rs @@ -171,6 +171,8 @@ impl ValidatorStore { /// - Adding the validator definition to the YAML file, saving it to the filesystem. /// - Enabling the validator with the slashing protection database. /// - If `enable == true`, starting to perform duties for the validator. + // FIXME: ignore this clippy lint until the validator store is refactored to use async locks + #[allow(clippy::await_holding_lock)] pub async fn add_validator( &self, validator_def: ValidatorDefinition,