Fixed Clippy Complaints & Some Failing Tests (#3791)

* Fixed Clippy Complaints & Some Failing Tests
* Update Dockerfile to Rust-1.65
* EF test file renamed
* Touch up comments based on feedback

Dockerfile

@@ -1,4 +1,4 @@
-FROM rust:1.62.1-bullseye AS builder
+FROM rust:1.65.0-bullseye AS builder
 RUN apt-get update && apt-get -y upgrade && apt-get install -y cmake libclang-dev protobuf-compiler
 COPY . lighthouse
 ARG FEATURES

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -2205,8 +2205,10 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
                 .verify_and_observe(bls_to_execution_change, &wall_clock_state, &self.spec)?)
         }
 
+        // TODO: remove this whole block once withdrawals-processing is removed
         #[cfg(not(feature = "withdrawals-processing"))]
         {
+            #[allow(clippy::drop_non_drop)]
             drop(bls_to_execution_change);
             Ok(ObservationOutcome::AlreadyKnown)
         }
@@ -4342,17 +4344,17 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
                 //   Might implement caching here in the future..
                 let prepare_state = self
                     .state_at_slot(prepare_slot, StateSkipConfig::WithoutStateRoots)
-                    .or_else(|e| {
+                    .map_err(|e| {
                         error!(self.log, "State advance for withdrawals failed"; "error" => ?e);
-                        Err(e)
+                        e
                     })?;
                 Some(get_expected_withdrawals(&prepare_state, &self.spec))
             }
         }
         .transpose()
-        .or_else(|e| {
+        .map_err(|e| {
             error!(self.log, "Error preparing beacon proposer"; "error" => ?e);
-            Err(e)
+            e
         })
         .map(|withdrawals_opt| withdrawals_opt.map(|w| w.into()))
         .map_err(Error::PrepareProposerFailed)?;

beacon_node/execution_layer/src/engine_api/json_structures.rs

@@ -176,7 +176,7 @@ impl<T: EthSpec> JsonExecutionPayload<T> {
                                 .collect::<Vec<_>>()
                                 .into()
                         })
-                        .ok_or(Error::BadConversion("Null withdrawal field converting JsonExecutionPayloadV2 -> ExecutionPayloadCapella".to_string()))?
+                        .ok_or_else(|| Error::BadConversion("Null withdrawal field converting JsonExecutionPayloadV2 -> ExecutionPayloadCapella".to_string()))?
                 })),
                 ForkName::Eip4844 => Ok(ExecutionPayload::Eip4844(ExecutionPayloadEip4844 {
                     parent_hash: v2.parent_hash,
@@ -191,7 +191,7 @@ impl<T: EthSpec> JsonExecutionPayload<T> {
                     timestamp: v2.timestamp,
                     extra_data: v2.extra_data,
                     base_fee_per_gas: v2.base_fee_per_gas,
-                    excess_data_gas: v2.excess_data_gas.ok_or(Error::BadConversion("Null `excess_data_gas` field converting JsonExecutionPayloadV2 -> ExecutionPayloadEip4844".to_string()))?,
+                    excess_data_gas: v2.excess_data_gas.ok_or_else(|| Error::BadConversion("Null `excess_data_gas` field converting JsonExecutionPayloadV2 -> ExecutionPayloadEip4844".to_string()))?,
                     block_hash: v2.block_hash,
                     transactions: v2.transactions,
                     #[cfg(feature = "withdrawals")]
@@ -204,7 +204,7 @@ impl<T: EthSpec> JsonExecutionPayload<T> {
                                 .collect::<Vec<_>>()
                                 .into()
                         })
-                        .ok_or(Error::BadConversion("Null withdrawal field converting JsonExecutionPayloadV2 -> ExecutionPayloadEip4844".to_string()))?
+                        .ok_or_else(|| Error::BadConversion("Null withdrawal field converting JsonExecutionPayloadV2 -> ExecutionPayloadEip4844".to_string()))?
                 })),
                 _ => Err(Error::UnsupportedForkVariant(format!("Unsupported conversion from JsonExecutionPayloadV2 for {}", fork_name))),
             }

beacon_node/execution_layer/src/engines.rs

@@ -342,7 +342,7 @@ impl Engine {
 impl PayloadIdCacheKey {
     fn new(head_block_hash: &ExecutionBlockHash, attributes: &PayloadAttributes) -> Self {
         Self {
-            head_block_hash: head_block_hash.clone(),
+            head_block_hash: *head_block_hash,
             payload_attributes: attributes.clone(),
         }
     }

beacon_node/execution_layer/src/lib.rs

@@ -1582,7 +1582,7 @@ impl<T: EthSpec> ExecutionLayer<T> {
         let transactions = VariableList::new(
             block
                 .transactions()
-                .into_iter()
+                .iter()
                 .map(|transaction| VariableList::new(transaction.rlp().to_vec()))
                 .collect::<Result<_, _>>()
                 .map_err(ApiError::DeserializeTransaction)?,

beacon_node/execution_layer/src/test_utils/handle_rpc.rs

@@ -74,11 +74,29 @@ pub async fn handle_rpc<T: EthSpec>(
                 .unwrap())
             }
         }
+        ENGINE_NEW_PAYLOAD_V1 | ENGINE_NEW_PAYLOAD_V2 => {
+            let request = match method {
                 ENGINE_NEW_PAYLOAD_V1 => {
-            let request: JsonExecutionPayload<T> = get_param(params, 0)?;
+                    JsonExecutionPayload::V1(get_param::<JsonExecutionPayloadV1<T>>(params, 0)?)
+                }
+                ENGINE_NEW_PAYLOAD_V2 => {
+                    JsonExecutionPayload::V2(get_param::<JsonExecutionPayloadV2<T>>(params, 0)?)
+                }
+                _ => unreachable!(),
+            };
+            let fork = match request {
+                JsonExecutionPayload::V1(_) => ForkName::Merge,
+                JsonExecutionPayload::V2(ref payload) => {
+                    if payload.withdrawals.is_none() {
+                        ForkName::Merge
+                    } else {
+                        ForkName::Capella
+                    }
+                }
+            };
 
             // Canned responses set by block hash take priority.
-            if let Some(status) = ctx.get_new_payload_status(&request.block_hash()) {
+            if let Some(status) = ctx.get_new_payload_status(request.block_hash()) {
                 return Ok(serde_json::to_value(JsonPayloadStatusV1::from(status)).unwrap());
             }
 
@@ -97,8 +115,7 @@ pub async fn handle_rpc<T: EthSpec>(
                 Some(
                     ctx.execution_block_generator
                         .write()
-                        // FIXME: should this worry about other forks?
+                        .new_payload(request.try_into_execution_payload(fork).unwrap()),
-                        .new_payload(request.try_into_execution_payload(ForkName::Merge).unwrap()),
                 )
             } else {
                 None

beacon_node/http_api/src/publish_blocks.rs

@@ -41,7 +41,7 @@ pub async fn publish_block<T: BeaconChainTypes>(
                 ))
             } else {
                 //TODO(pawan): return an empty sidecar instead
-                return Err(warp_utils::reject::broadcast_without_import(format!("")));
+                return Err(warp_utils::reject::broadcast_without_import(String::new()));
             }
         }
         _ => PubsubMessage::BeaconBlock(block.clone()),

beacon_node/lighthouse_network/src/types/pubsub.rs

@@ -224,12 +224,10 @@ impl<T: EthSpec> PubsubMessage<T> {
                                 | ForkName::Merge
                                 | ForkName::Capella,
                             )
-                            | None => {
+                            | None => Err(format!(
-                                return Err(format!(
                                 "beacon_blobs_and_sidecar topic invalid for given fork digest {:?}",
                                 gossip_topic.fork_digest
-                            ))
+                            )),
-                            }
                         }
                     }
                     GossipKind::VoluntaryExit => {

beacon_node/operation_pool/src/lib.rs

@@ -558,8 +558,10 @@ impl<T: EthSpec> OperationPool<T> {
             )
         }
 
+        // TODO: remove this whole block once withdrwals-processing is removed
         #[cfg(not(feature = "withdrawals-processing"))]
         {
+            #[allow(clippy::drop_copy)]
             drop((state, spec));
             vec![]
         }
@@ -597,8 +599,10 @@ impl<T: EthSpec> OperationPool<T> {
             );
         }
 
+        // TODO: remove this whole block once withdrwals-processing is removed
         #[cfg(not(feature = "withdrawals-processing"))]
         {
+            #[allow(clippy::drop_copy)]
             drop((head_block, head_state, spec));
         }
     }

consensus/types/src/payload.rs

@@ -261,7 +261,7 @@ impl<T: EthSpec> ExecPayload<T> for FullPayload<T> {
         })
     }
 
-    fn is_default_with_empty_roots<'a>(&'a self) -> bool {
+    fn is_default_with_empty_roots(&self) -> bool {
         // For full payloads the empty/zero distinction does not exist.
         self.is_default_with_zero_roots()
     }
@@ -536,7 +536,7 @@ impl<T: EthSpec> ExecPayload<T> for BlindedPayload<T> {
         }
     }
 
-    fn is_default_with_zero_roots<'a>(&'a self) -> bool {
+    fn is_default_with_zero_roots(&self) -> bool {
         self.to_ref().is_default_with_zero_roots()
     }
 
@@ -643,13 +643,13 @@ impl<'b, T: EthSpec> ExecPayload<T> for BlindedPayloadRef<'b, T> {
 }
 
 macro_rules! impl_exec_payload_common {
-    ($wrapper_type:ident,
+    ($wrapper_type:ident,           // BlindedPayloadMerge          |   FullPayloadMerge
-     $wrapped_type:ident,
+     $wrapped_type:ident,           // ExecutionPayloadHeaderMerge  |   ExecutionPayloadMerge
-     $wrapped_type_full:ident,
+     $wrapped_type_full:ident,      // ExecutionPayloadMerge        |   ExecutionPayloadMerge
-     $wrapped_type_header:ident,
+     $wrapped_type_header:ident,    // ExecutionPayloadHeaderMerge  |   ExecutionPayloadHeaderMerge
-     $wrapped_field:ident,
+     $wrapped_field:ident,          // execution_payload_header     |   execution_payload
-     $fork_variant:ident,
+     $fork_variant:ident,           // Merge                        |   Merge
-     $block_type_variant:ident,
+     $block_type_variant:ident,     // Blinded                      |   Full
      $f:block,
      $g:block) => {
         impl<T: EthSpec> ExecPayload<T> for $wrapper_type<T> {
@@ -696,8 +696,16 @@ macro_rules! impl_exec_payload_common {
             }
 
             fn is_default_with_empty_roots(&self) -> bool {
+                // FIXME: is there a better way than ignoring this lint?
+                // This is necessary because the first invocation of this macro might expand to:
+                //     self.execution_payload_header == ExecutionPayloadHeaderMerge::from(ExecutionPayloadMerge::default())
+                // but the second invocation might expand to:
+                //     self.execution_payload == ExecutionPayloadMerge::from(ExecutionPayloadMerge::default())
+                #[allow(clippy::cmp_owned)]
+                {
                     self.$wrapped_field == $wrapped_type::from($wrapped_type_full::default())
                 }
+            }
 
             fn transactions(&self) -> Option<&Transactions<T>> {
                 let f = $f;
@@ -720,16 +728,17 @@ macro_rules! impl_exec_payload_common {
 }
 
 macro_rules! impl_exec_payload_for_fork {
+    // BlindedPayloadMerge, FullPayloadMerge, ExecutionPayloadHeaderMerge, ExecutionPayloadMerge, Merge
     ($wrapper_type_header:ident, $wrapper_type_full:ident, $wrapped_type_header:ident, $wrapped_type_full:ident, $fork_variant:ident) => {
         //*************** Blinded payload implementations ******************//
 
         impl_exec_payload_common!(
-            $wrapper_type_header,
+            $wrapper_type_header, // BlindedPayloadMerge
-            $wrapped_type_header,
+            $wrapped_type_header, // ExecutionPayloadHeaderMerge
-            $wrapped_type_full,
+            $wrapped_type_full,   // ExecutionPayloadMerge
-            $wrapped_type_header,
+            $wrapped_type_header, // ExecutionPayloadHeaderMerge
             execution_payload_header,
-            $fork_variant,
+            $fork_variant, // Merge
             Blinded,
             { |_| { None } },
             {
@@ -794,12 +803,12 @@ macro_rules! impl_exec_payload_for_fork {
         //*************** Full payload implementations ******************//
 
         impl_exec_payload_common!(
-            $wrapper_type_full,
+            $wrapper_type_full,   // FullPayloadMerge
-            $wrapped_type_full,
+            $wrapped_type_full,   // ExecutionPayloadMerge
-            $wrapped_type_full,
+            $wrapped_type_full,   // ExecutionPayloadMerge
-            $wrapped_type_header,
+            $wrapped_type_header, // ExecutionPayloadHeaderMerge
             execution_payload,
-            $fork_variant,
+            $fork_variant, // Merge
             Full,
             {
                 let c: for<'a> fn(&'a $wrapper_type_full<T>) -> Option<&'a Transactions<T>> =

lcli/src/new_testnet.rs

@@ -87,9 +87,9 @@ pub fn run<T: EthSpec>(testnet_dir_path: PathBuf, matches: &ArgMatches) -> Resul
             execution_payload_header.as_ref()
         {
             let eth1_block_hash =
-                parse_optional(matches, "eth1-block-hash")?.unwrap_or(payload.block_hash());
+                parse_optional(matches, "eth1-block-hash")?.unwrap_or_else(|| payload.block_hash());
             let genesis_time =
-                parse_optional(matches, "genesis-time")?.unwrap_or(payload.timestamp());
+                parse_optional(matches, "genesis-time")?.unwrap_or_else(|| payload.timestamp());
             (eth1_block_hash, genesis_time)
         } else {
             let eth1_block_hash = parse_required(matches, "eth1-block-hash").map_err(|_| {

testing/ef_tests/src/cases/merkle_proof_validity.rs

@@ -29,7 +29,7 @@ pub struct MerkleProofValidity<E: EthSpec> {
 impl<E: EthSpec> LoadCase for MerkleProofValidity<E> {
     fn load_from_dir(path: &Path, fork_name: ForkName) -> Result<Self, Error> {
         let spec = &testing_spec::<E>(fork_name);
-        let state = ssz_decode_state(&path.join("state.ssz_snappy"), spec)?;
+        let state = ssz_decode_state(&path.join("object.ssz_snappy"), spec)?;
         let merkle_proof = yaml_decode_file(&path.join("proof.yaml"))?;
         // Metadata does not exist in these tests but it is left like this just in case.
         let meta_path = path.join("meta.yaml");

testing/ef_tests/src/cases/operations.rs

@@ -21,8 +21,6 @@ use state_processing::{
     ConsensusContext,
 };
 use std::fmt::Debug;
-#[cfg(not(all(feature = "withdrawals", feature = "withdrawals-processing")))]
-use std::marker::PhantomData;
 use std::path::Path;
 #[cfg(all(feature = "withdrawals", feature = "withdrawals-processing"))]
 use types::SignedBlsToExecutionChange;
@@ -44,12 +42,10 @@ struct ExecutionMetadata {
 }
 
 /// Newtype for testing withdrawals.
+#[cfg(all(feature = "withdrawals", feature = "withdrawals-processing"))]
 #[derive(Debug, Clone, Deserialize)]
 pub struct WithdrawalsPayload<T: EthSpec> {
-    #[cfg(all(feature = "withdrawals", feature = "withdrawals-processing"))]
     payload: FullPayload<T>,
-    #[cfg(not(all(feature = "withdrawals", feature = "withdrawals-processing")))]
-    _phantom_data: PhantomData<T>,
 }
 
 #[derive(Debug, Clone)]

testing/ef_tests/src/lib.rs

@@ -1,9 +1,11 @@
 pub use case_result::CaseResult;
+#[cfg(all(feature = "withdrawals", feature = "withdrawals-processing"))]
+pub use cases::WithdrawalsPayload;
 pub use cases::{
     Case, EffectiveBalanceUpdates, Eth1DataReset, HistoricalRootsUpdate, InactivityUpdates,
     JustificationAndFinalization, ParticipationFlagUpdates, ParticipationRecordUpdates,
     RandaoMixesReset, RegistryUpdates, RewardsAndPenalties, Slashings, SlashingsReset,
-    SyncCommitteeUpdates, WithdrawalsPayload,
+    SyncCommitteeUpdates,
 };
 pub use decode::log_file_access;
 pub use error::Error;

testing/ef_tests/tests/tests.rs

@@ -82,12 +82,14 @@ fn operations_execution_payload_blinded() {
     OperationsHandler::<MainnetEthSpec, BlindedPayload<_>>::default().run();
 }
 
+#[cfg(all(feature = "withdrawals", feature = "withdrawals-processing"))]
 #[test]
 fn operations_withdrawals() {
     OperationsHandler::<MinimalEthSpec, WithdrawalsPayload<_>>::default().run();
     OperationsHandler::<MainnetEthSpec, WithdrawalsPayload<_>>::default().run();
 }
 
+#[cfg(all(feature = "withdrawals", feature = "withdrawals-processing"))]
 #[test]
 fn operations_bls_to_execution_change() {
     OperationsHandler::<MinimalEthSpec, SignedBlsToExecutionChange>::default().run();