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

@@ -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

Cargo.lock

@@ -3888,6 +3888,7 @@ name = "network"
 version = "0.2.0"
 dependencies = [
  "beacon_chain",
+ "derivative",
  "environment",
  "error-chain",
  "eth2_ssz",

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<T: BeaconChainTypes>(
 
 /// 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<T: BeaconChainTypes> {
     pub block: SignedBeaconBlock<T::EthSpec>,
     pub block_root: Hash256,

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"

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<T: BeaconChainTypes> {
     drop_during_sync: bool,
     work: Work<T>,
 }
 
-impl<T: BeaconChainTypes> fmt::Debug for WorkEvent<T> {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        write!(f, "{:?}", self)
-    }
-}
-
 impl<T: BeaconChainTypes> WorkEvent<T> {
     /// Create a new `Work` event for some unaggregated attestation.
     pub fn unaggregated_attestation(
@@ -615,7 +611,8 @@ impl<T: BeaconChainTypes> std::convert::From<ReadyWork<T>> for WorkEvent<T> {
 }
 
 /// A consensus message (or multiple) from the network that requires processing.
-#[derive(Debug)]
+#[derive(Derivative)]
+#[derivative(Debug(bound = "T: BeaconChainTypes"))]
 pub enum Work<T: BeaconChainTypes> {
     GossipAttestation {
         message_id: MessageId,

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<KeepChain, RemoveChain>;
 
 /// Reasons for removing a chain

testing/simulator/src/local_network.rs

@@ -107,15 +107,16 @@ impl<E: EthSpec> LocalNetwork<E> {
             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(())
     }
 

validator_client/src/preparation_service.rs

@@ -199,7 +199,8 @@ impl<T: SlotClock + 'static, E: EthSpec> PreparationService<T, E> {
                     .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<T: SlotClock + 'static, E: EthSpec> PreparationService<T, E> {
         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
                 }
             })

validator_client/src/validator_store.rs

@@ -171,6 +171,8 @@ impl<T: SlotClock + 'static, E: EthSpec> ValidatorStore<T, E> {
     /// - 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,