From cbd22011640fbc881440ed49d7f371bdfcf0fc06 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Mon, 15 Nov 2021 11:26:42 +1100 Subject: [PATCH] Fixes after rebasing Kintsugi onto unstable (#2799) * Fix fork choice after rebase * Remove paulhauner warp dep * Fix fork choice test compile errors * Assume fork choice payloads are valid * Add comment * Ignore new tests * Fix error in test skipping --- beacon_node/beacon_chain/src/beacon_chain.rs | 1 + beacon_node/beacon_chain/src/fork_revert.rs | 1 + beacon_node/beacon_chain/src/lib.rs | 2 +- beacon_node/beacon_chain/src/test_utils.rs | 11 +++++++++++ beacon_node/execution_layer/Cargo.toml | 2 +- .../execution_layer/src/test_utils/handle_rpc.rs | 11 ++++++++--- beacon_node/execution_layer/src/test_utils/mod.rs | 7 +++++++ consensus/fork_choice/src/fork_choice.rs | 5 +++-- consensus/fork_choice/src/lib.rs | 2 +- consensus/fork_choice/tests/tests.rs | 5 +++-- testing/ef_tests/Cargo.toml | 1 + testing/ef_tests/check_all_files_accessed.py | 11 +++++++++-- testing/ef_tests/src/cases.rs | 2 +- testing/ef_tests/src/cases/fork_choice.rs | 9 +++++++-- 14 files changed, 55 insertions(+), 15 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 8b0600969d..3bcbda84f8 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -2448,6 +2448,7 @@ impl BeaconChain { block_root, &state, payload_verification_status, + &self.spec, ) .map_err(|e| BlockError::BeaconChainError(e.into()))?; } diff --git a/beacon_node/beacon_chain/src/fork_revert.rs b/beacon_node/beacon_chain/src/fork_revert.rs index a1ca120418..610e27eb9e 100644 --- a/beacon_node/beacon_chain/src/fork_revert.rs +++ b/beacon_node/beacon_chain/src/fork_revert.rs @@ -178,6 +178,7 @@ pub fn reset_fork_choice_to_finalization, Cold: It block.canonical_root(), &state, payload_verification_status, + spec, ) .map_err(|e| format!("Error applying replayed block to fork choice: {:?}", e))?; } diff --git a/beacon_node/beacon_chain/src/lib.rs b/beacon_node/beacon_chain/src/lib.rs index 717af99b4c..cc0c6f9e12 100644 --- a/beacon_node/beacon_chain/src/lib.rs +++ b/beacon_node/beacon_chain/src/lib.rs @@ -36,7 +36,7 @@ mod validator_pubkey_cache; pub use self::beacon_chain::{ AttestationProcessingOutcome, BeaconChain, BeaconChainTypes, BeaconStore, ChainSegmentResult, - ForkChoiceError, HeadSafetyStatus, StateSkipConfig, WhenSlotSkipped, HeadInfo + ForkChoiceError, HeadInfo, HeadSafetyStatus, StateSkipConfig, WhenSlotSkipped, MAXIMUM_GOSSIP_CLOCK_DISPARITY, }; pub use self::beacon_snapshot::BeaconSnapshot; diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index 0dd99b8985..f53054ff2c 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -370,6 +370,17 @@ where self } + /// Instruct the mock execution engine to always return a "valid" response to any payload it is + /// asked to execute. + pub fn mock_execution_layer_all_payloads_valid(self) -> Self { + self.mock_execution_layer + .as_ref() + .expect("requires mock execution layer") + .server + .all_payloads_valid(); + self + } + pub fn build(self) -> BeaconChainHarness> { let (shutdown_tx, shutdown_receiver) = futures::channel::mpsc::channel(1); diff --git a/beacon_node/execution_layer/Cargo.toml b/beacon_node/execution_layer/Cargo.toml index aeeaab67ae..2fc6fffe85 100644 --- a/beacon_node/execution_layer/Cargo.toml +++ b/beacon_node/execution_layer/Cargo.toml @@ -17,7 +17,7 @@ eth2_serde_utils = { path = "../../consensus/serde_utils" } serde_json = "1.0.58" serde = { version = "1.0.116", features = ["derive"] } eth1 = { path = "../eth1" } -warp = { git = "https://github.com/paulhauner/warp ", branch = "cors-wildcard" } +warp = { git = "https://github.com/macladson/warp", rev ="dfa259e", features = ["tls"] } environment = { path = "../../lighthouse/environment" } bytes = "1.1.0" task_executor = { path = "../../common/task_executor" } diff --git a/beacon_node/execution_layer/src/test_utils/handle_rpc.rs b/beacon_node/execution_layer/src/test_utils/handle_rpc.rs index 0501263e7e..5523bef8e0 100644 --- a/beacon_node/execution_layer/src/test_utils/handle_rpc.rs +++ b/beacon_node/execution_layer/src/test_utils/handle_rpc.rs @@ -64,10 +64,15 @@ pub async fn handle_rpc( } ENGINE_EXECUTE_PAYLOAD => { let request: JsonExecutionPayload = get_param_0(params)?; + let status = ctx - .execution_block_generator - .write() - .execute_payload(request.into()); + .static_execute_payload_response + .lock() + .unwrap_or_else(|| { + ctx.execution_block_generator + .write() + .execute_payload(request.into()) + }); Ok(serde_json::to_value(ExecutePayloadResponseWrapper { status }).unwrap()) } diff --git a/beacon_node/execution_layer/src/test_utils/mod.rs b/beacon_node/execution_layer/src/test_utils/mod.rs index 87490042b7..5ba5c8f032 100644 --- a/beacon_node/execution_layer/src/test_utils/mod.rs +++ b/beacon_node/execution_layer/src/test_utils/mod.rs @@ -1,6 +1,7 @@ //! Provides a mock execution engine HTTP JSON-RPC API for use in testing. use crate::engine_api::http::JSONRPC_VERSION; +use crate::engine_api::ExecutePayloadResponse; use bytes::Bytes; use environment::null_logger; use handle_rpc::handle_rpc; @@ -60,6 +61,7 @@ impl MockServer { last_echo_request: last_echo_request.clone(), execution_block_generator: RwLock::new(execution_block_generator), preloaded_responses, + static_execute_payload_response: <_>::default(), _phantom: PhantomData, }); @@ -112,6 +114,10 @@ impl MockServer { pub fn push_preloaded_response(&self, response: serde_json::Value) { self.ctx.preloaded_responses.lock().push(response) } + + pub fn all_payloads_valid(&self) { + *self.ctx.static_execute_payload_response.lock() = Some(ExecutePayloadResponse::Valid) + } } #[derive(Debug)] @@ -146,6 +152,7 @@ pub struct Context { pub last_echo_request: Arc>>, pub execution_block_generator: RwLock>, pub preloaded_responses: Arc>>, + pub static_execute_payload_response: Arc>>, pub _phantom: PhantomData, } diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index a683ed8ad6..93ed1c3bae 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -3,8 +3,8 @@ use std::marker::PhantomData; use proto_array::{Block as ProtoBlock, ExecutionStatus, ProtoArrayForkChoice}; use ssz_derive::{Decode, Encode}; use types::{ - AttestationShufflingId, BeaconBlock, BeaconState, BeaconStateError, Checkpoint, Epoch, EthSpec, - Hash256, IndexedAttestation, RelativeEpoch, SignedBeaconBlock, Slot, + AttestationShufflingId, BeaconBlock, BeaconState, BeaconStateError, ChainSpec, Checkpoint, + Epoch, EthSpec, Hash256, IndexedAttestation, RelativeEpoch, SignedBeaconBlock, Slot, }; use crate::ForkChoiceStore; @@ -469,6 +469,7 @@ where block_root: Hash256, state: &BeaconState, payload_verification_status: PayloadVerificationStatus, + spec: &ChainSpec, ) -> Result<(), Error> { let current_slot = self.update_time(current_slot)?; diff --git a/consensus/fork_choice/src/lib.rs b/consensus/fork_choice/src/lib.rs index b829cd6d9b..7dd80b7982 100644 --- a/consensus/fork_choice/src/lib.rs +++ b/consensus/fork_choice/src/lib.rs @@ -3,7 +3,7 @@ mod fork_choice_store; pub use crate::fork_choice::{ Error, ForkChoice, InvalidAttestation, InvalidBlock, PayloadVerificationStatus, - PersistedForkChoice, QueuedAttestation, SAFE_SLOTS_TO_UPDATE_JUSTIFIED, + PersistedForkChoice, QueuedAttestation, }; pub use fork_choice_store::ForkChoiceStore; pub use proto_array::Block as ProtoBlock; diff --git a/consensus/fork_choice/tests/tests.rs b/consensus/fork_choice/tests/tests.rs index 5f451cf120..129b79c399 100644 --- a/consensus/fork_choice/tests/tests.rs +++ b/consensus/fork_choice/tests/tests.rs @@ -11,8 +11,7 @@ use beacon_chain::{ StateSkipConfig, WhenSlotSkipped, }; use fork_choice::{ - ForkChoiceStore, InvalidAttestation, InvalidBlock, PayloadVerificationStatus, - QueuedAttestation, SAFE_SLOTS_TO_UPDATE_JUSTIFIED, + ForkChoiceStore, InvalidAttestation, InvalidBlock, PayloadVerificationStatus, QueuedAttestation, }; use store::MemoryStore; use types::{ @@ -277,6 +276,7 @@ impl ForkChoiceTest { block.canonical_root(), &state, PayloadVerificationStatus::Verified, + &self.harness.chain.spec, ) .unwrap(); self @@ -318,6 +318,7 @@ impl ForkChoiceTest { block.canonical_root(), &state, PayloadVerificationStatus::Verified, + &self.harness.chain.spec, ) .err() .expect("on_block did not return an error"); diff --git a/testing/ef_tests/Cargo.toml b/testing/ef_tests/Cargo.toml index e1668a9b49..9bebff279a 100644 --- a/testing/ef_tests/Cargo.toml +++ b/testing/ef_tests/Cargo.toml @@ -34,3 +34,4 @@ snap = "1.0.1" fs2 = "0.4.3" beacon_chain = { path = "../../beacon_node/beacon_chain" } store = { path = "../../beacon_node/store" } +fork_choice = { path = "../../consensus/fork_choice" } diff --git a/testing/ef_tests/check_all_files_accessed.py b/testing/ef_tests/check_all_files_accessed.py index 806a08e68e..cd70533f14 100755 --- a/testing/ef_tests/check_all_files_accessed.py +++ b/testing/ef_tests/check_all_files_accessed.py @@ -32,9 +32,16 @@ excluded_paths = [ # LightClientSnapshot "tests/minimal/altair/ssz_static/LightClientSnapshot", "tests/mainnet/altair/ssz_static/LightClientSnapshot", + "tests/minimal/merge/ssz_static/LightClientSnapshot", + "tests/mainnet/merge/ssz_static/LightClientSnapshot", # Merkle-proof tests for light clients - "tests/mainnet/altair/merkle/single_proof/pyspec_tests/", - "tests/minimal/altair/merkle/single_proof/pyspec_tests/" + "tests/mainnet/altair/merkle/single_proof", + "tests/minimal/altair/merkle/single_proof", + "tests/mainnet/merge/merkle/single_proof", + "tests/minimal/merge/merkle/single_proof", + # Fork choice tests featuring PoW blocks + "tests/minimal/merge/fork_choice/on_merge_block/", + "tests/mainnet/merge/fork_choice/on_merge_block/" ] def normalize_path(path): diff --git a/testing/ef_tests/src/cases.rs b/testing/ef_tests/src/cases.rs index e290421762..ac9ca8993c 100644 --- a/testing/ef_tests/src/cases.rs +++ b/testing/ef_tests/src/cases.rs @@ -26,6 +26,7 @@ mod ssz_generic; mod ssz_static; mod transition; +pub use self::fork_choice::*; pub use bls_aggregate_sigs::*; pub use bls_aggregate_verify::*; pub use bls_eth_aggregate_pubkeys::*; @@ -36,7 +37,6 @@ pub use bls_verify_msg::*; pub use common::SszStaticType; pub use epoch_processing::*; pub use fork::ForkTest; -pub use fork_choice::*; pub use genesis_initialization::*; pub use genesis_validity::*; pub use operations::*; diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index 7d7b21da13..4bbcdc1978 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -1,5 +1,6 @@ use super::*; use crate::decode::{ssz_decode_file, ssz_decode_file_with, ssz_decode_state, yaml_decode_file}; +use ::fork_choice::PayloadVerificationStatus; use beacon_chain::{ attestation_verification::{ obtain_indexed_attestation_and_committees_per_slot, VerifiedAttestation, @@ -218,6 +219,8 @@ impl Tester { .spec(spec.clone()) .keypairs(vec![]) .genesis_state_ephemeral_store(case.anchor_state.clone()) + .mock_execution_layer() + .mock_execution_layer_all_payloads_valid() .build(); if harness.chain.genesis_block_root != case.anchor_block.canonical_root() { @@ -283,10 +286,11 @@ impl Tester { let block_root = block.canonical_root(); if result.is_ok() != valid { return Err(Error::DidntFail(format!( - "block with root {} was valid={} whilst test expects valid={}", + "block with root {} was valid={} whilst test expects valid={}. result: {:?}", block_root, result.is_ok(), - valid + valid, + result ))); } @@ -319,6 +323,7 @@ impl Tester { &block, block_root, &state, + PayloadVerificationStatus::Irrelevant, &self.harness.chain.spec, );