From e4cbdc1c77d94e935ab838b2c2b1d5c4d7bf4018 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Sat, 15 Oct 2022 22:25:52 +0000 Subject: [PATCH] Optimistic sync spec tests (v1.2.0) (#3564) ## Issue Addressed Implements new optimistic sync test format from https://github.com/ethereum/consensus-specs/pull/2982. ## Proposed Changes - Add parsing and runner support for the new test format. - Extend the mock EL with a set of canned responses keyed by block hash. Although this doubles up on some of the existing functionality I think it's really nice to use compared to the `preloaded_responses` or static responses. I think we could write novel new opt sync tests using these primtives much more easily than the previous ones. Forks are natively supported, and different responses to `forkchoiceUpdated` and `newPayload` are also straight-forward. ## Additional Info Blocked on merge of the spec PR and release of new test vectors. --- Cargo.lock | 1 + .../src/engine_api/json_structures.rs | 4 +- .../src/test_utils/handle_rpc.rs | 14 ++++ .../execution_layer/src/test_utils/mod.rs | 46 +++++++++++++ testing/ef_tests/Cargo.toml | 1 + testing/ef_tests/Makefile | 2 +- .../ef_tests/src/cases/bls_aggregate_sigs.rs | 17 +++-- testing/ef_tests/src/cases/fork_choice.rs | 69 ++++++++++++++++--- testing/ef_tests/src/cases/operations.rs | 5 ++ testing/ef_tests/src/handler.rs | 31 +++++++++ testing/ef_tests/tests/tests.rs | 6 ++ 11 files changed, 177 insertions(+), 19 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d0e3622e7..34c932307 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1500,6 +1500,7 @@ dependencies = [ "eth2_ssz", "eth2_ssz_derive", "ethereum-types 0.12.1", + "execution_layer", "fork_choice", "fs2", "hex", diff --git a/beacon_node/execution_layer/src/engine_api/json_structures.rs b/beacon_node/execution_layer/src/engine_api/json_structures.rs index 31aa79f05..2b0c3a4c9 100644 --- a/beacon_node/execution_layer/src/engine_api/json_structures.rs +++ b/beacon_node/execution_layer/src/engine_api/json_structures.rs @@ -1,5 +1,6 @@ use super::*; use serde::{Deserialize, Serialize}; +use strum::EnumString; use types::{EthSpec, ExecutionBlockHash, FixedVector, Transaction, Unsigned, VariableList}; #[derive(Debug, PartialEq, Serialize, Deserialize)] @@ -311,8 +312,9 @@ impl From for ForkChoiceState { } } -#[derive(Debug, PartialEq, Serialize, Deserialize)] +#[derive(Debug, Clone, Copy, PartialEq, Serialize, Deserialize, EnumString)] #[serde(rename_all = "SCREAMING_SNAKE_CASE")] +#[strum(serialize_all = "SCREAMING_SNAKE_CASE")] pub enum JsonPayloadStatusV1Status { Valid, Invalid, 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 975f09fa5..ac677bf33 100644 --- a/beacon_node/execution_layer/src/test_utils/handle_rpc.rs +++ b/beacon_node/execution_layer/src/test_utils/handle_rpc.rs @@ -77,6 +77,11 @@ pub async fn handle_rpc( ENGINE_NEW_PAYLOAD_V1 => { let request: JsonExecutionPayloadV1 = get_param(params, 0)?; + // Canned responses set by block hash take priority. + if let Some(status) = ctx.get_new_payload_status(&request.block_hash) { + return Ok(serde_json::to_value(JsonPayloadStatusV1::from(status)).unwrap()); + } + let (static_response, should_import) = if let Some(mut response) = ctx.static_new_payload_response.lock().clone() { if response.status.status == PayloadStatusV1Status::Valid { @@ -120,6 +125,15 @@ pub async fn handle_rpc( let head_block_hash = forkchoice_state.head_block_hash; + // Canned responses set by block hash take priority. + if let Some(status) = ctx.get_fcu_payload_status(&head_block_hash) { + let response = JsonForkchoiceUpdatedV1Response { + payload_status: JsonPayloadStatusV1::from(status), + payload_id: None, + }; + return Ok(serde_json::to_value(response).unwrap()); + } + let mut response = ctx .execution_block_generator .write() diff --git a/beacon_node/execution_layer/src/test_utils/mod.rs b/beacon_node/execution_layer/src/test_utils/mod.rs index aaeea8aa5..f5066879a 100644 --- a/beacon_node/execution_layer/src/test_utils/mod.rs +++ b/beacon_node/execution_layer/src/test_utils/mod.rs @@ -12,6 +12,7 @@ use parking_lot::{Mutex, RwLock, RwLockWriteGuard}; use serde::{Deserialize, Serialize}; use serde_json::json; use slog::{info, Logger}; +use std::collections::HashMap; use std::convert::Infallible; use std::future::Future; use std::marker::PhantomData; @@ -98,6 +99,8 @@ impl MockServer { static_new_payload_response: <_>::default(), static_forkchoice_updated_response: <_>::default(), static_get_block_by_hash_response: <_>::default(), + new_payload_statuses: <_>::default(), + fcu_payload_statuses: <_>::default(), _phantom: PhantomData, }); @@ -370,6 +373,25 @@ impl MockServer { pub fn drop_all_blocks(&self) { self.ctx.execution_block_generator.write().drop_all_blocks() } + + pub fn set_payload_statuses(&self, block_hash: ExecutionBlockHash, status: PayloadStatusV1) { + self.set_new_payload_status(block_hash, status.clone()); + self.set_fcu_payload_status(block_hash, status); + } + + pub fn set_new_payload_status(&self, block_hash: ExecutionBlockHash, status: PayloadStatusV1) { + self.ctx + .new_payload_statuses + .lock() + .insert(block_hash, status); + } + + pub fn set_fcu_payload_status(&self, block_hash: ExecutionBlockHash, status: PayloadStatusV1) { + self.ctx + .fcu_payload_statuses + .lock() + .insert(block_hash, status); + } } #[derive(Debug)] @@ -419,9 +441,33 @@ pub struct Context { pub static_new_payload_response: Arc>>, pub static_forkchoice_updated_response: Arc>>, pub static_get_block_by_hash_response: Arc>>>, + + // Canned responses by block hash. + // + // This is a more flexible and less stateful alternative to `static_new_payload_response` + // and `preloaded_responses`. + pub new_payload_statuses: Arc>>, + pub fcu_payload_statuses: Arc>>, + pub _phantom: PhantomData, } +impl Context { + pub fn get_new_payload_status( + &self, + block_hash: &ExecutionBlockHash, + ) -> Option { + self.new_payload_statuses.lock().get(block_hash).cloned() + } + + pub fn get_fcu_payload_status( + &self, + block_hash: &ExecutionBlockHash, + ) -> Option { + self.fcu_payload_statuses.lock().get(block_hash).cloned() + } +} + /// Configuration for the HTTP server. #[derive(PartialEq, Debug, Clone, Serialize, Deserialize)] pub struct Config { diff --git a/testing/ef_tests/Cargo.toml b/testing/ef_tests/Cargo.toml index e04d67139..04a222c7a 100644 --- a/testing/ef_tests/Cargo.toml +++ b/testing/ef_tests/Cargo.toml @@ -35,3 +35,4 @@ fs2 = "0.4.3" beacon_chain = { path = "../../beacon_node/beacon_chain" } store = { path = "../../beacon_node/store" } fork_choice = { path = "../../consensus/fork_choice" } +execution_layer = { path = "../../beacon_node/execution_layer" } diff --git a/testing/ef_tests/Makefile b/testing/ef_tests/Makefile index fac1ab905..e05ef0b06 100644 --- a/testing/ef_tests/Makefile +++ b/testing/ef_tests/Makefile @@ -1,4 +1,4 @@ -TESTS_TAG := v1.2.0-rc.3 +TESTS_TAG := v1.2.0 TESTS = general minimal mainnet TARBALLS = $(patsubst %,%-$(TESTS_TAG).tar.gz,$(TESTS)) diff --git a/testing/ef_tests/src/cases/bls_aggregate_sigs.rs b/testing/ef_tests/src/cases/bls_aggregate_sigs.rs index 81e186a66..53387ee4d 100644 --- a/testing/ef_tests/src/cases/bls_aggregate_sigs.rs +++ b/testing/ef_tests/src/cases/bls_aggregate_sigs.rs @@ -7,7 +7,7 @@ use serde_derive::Deserialize; #[derive(Debug, Clone, Deserialize)] pub struct BlsAggregateSigs { pub input: Vec, - pub output: String, + pub output: Option, } impl_bls_load_case!(BlsAggregateSigs); @@ -25,14 +25,13 @@ impl Case for BlsAggregateSigs { aggregate_signature.add_assign(&sig); } - // Check for YAML null value, indicating invalid input. This is a bit of a hack, - // as our mutating `aggregate_signature.add` API doesn't play nicely with aggregating 0 - // inputs. - let output_bytes = if self.output == "~" { - AggregateSignature::infinity().serialize().to_vec() - } else { - hex::decode(&self.output[2..]) - .map_err(|e| Error::FailedToParseTest(format!("{:?}", e)))? + let output_bytes = match self.output.as_deref() { + // Check for YAML null value, indicating invalid input. This is a bit of a hack, + // as our mutating `aggregate_signature.add` API doesn't play nicely with aggregating 0 + // inputs. + Some("~") | None => AggregateSignature::infinity().serialize().to_vec(), + Some(output) => hex::decode(&output[2..]) + .map_err(|e| Error::FailedToParseTest(format!("{:?}", e)))?, }; let aggregate_signature = Ok(aggregate_signature.serialize().to_vec()); diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index 0e1bb2ace..8faf4db82 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -9,7 +9,8 @@ use beacon_chain::{ test_utils::{BeaconChainHarness, EphemeralHarnessType}, BeaconChainTypes, CachedHead, CountUnrealized, }; -use serde_derive::Deserialize; +use execution_layer::{json_structures::JsonPayloadStatusV1Status, PayloadStatusV1}; +use serde::Deserialize; use ssz_derive::Decode; use state_processing::state_advance::complete_state_advance; use std::future::Future; @@ -50,16 +51,53 @@ pub struct Checks { proposer_boost_root: Option, } +#[derive(Debug, Clone, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct PayloadStatus { + status: JsonPayloadStatusV1Status, + latest_valid_hash: Option, + validation_error: Option, +} + +impl From for PayloadStatusV1 { + fn from(status: PayloadStatus) -> Self { + PayloadStatusV1 { + status: status.status.into(), + latest_valid_hash: status.latest_valid_hash, + validation_error: status.validation_error, + } + } +} + #[derive(Debug, Clone, Deserialize)] #[serde(untagged, deny_unknown_fields)] pub enum Step { - Tick { tick: u64 }, - ValidBlock { block: B }, - MaybeValidBlock { block: B, valid: bool }, - Attestation { attestation: A }, - AttesterSlashing { attester_slashing: AS }, - PowBlock { pow_block: P }, - Checks { checks: Box }, + Tick { + tick: u64, + }, + ValidBlock { + block: B, + }, + MaybeValidBlock { + block: B, + valid: bool, + }, + Attestation { + attestation: A, + }, + AttesterSlashing { + attester_slashing: AS, + }, + PowBlock { + pow_block: P, + }, + OnPayloadInfo { + block_hash: ExecutionBlockHash, + payload_status: PayloadStatus, + }, + Checks { + checks: Box, + }, } #[derive(Debug, Clone, Deserialize)] @@ -119,6 +157,13 @@ impl LoadCase for ForkChoiceTest { ssz_decode_file(&path.join(format!("{}.ssz_snappy", pow_block))) .map(|pow_block| Step::PowBlock { pow_block }) } + Step::OnPayloadInfo { + block_hash, + payload_status, + } => Ok(Step::OnPayloadInfo { + block_hash, + payload_status, + }), Step::Checks { checks } => Ok(Step::Checks { checks }), }) .collect::>()?; @@ -168,6 +213,14 @@ impl Case for ForkChoiceTest { tester.process_attester_slashing(attester_slashing) } Step::PowBlock { pow_block } => tester.process_pow_block(pow_block), + Step::OnPayloadInfo { + block_hash, + payload_status, + } => { + let el = tester.harness.mock_execution_layer.as_ref().unwrap(); + el.server + .set_payload_statuses(*block_hash, payload_status.clone().into()); + } Step::Checks { checks } => { let Checks { head, diff --git a/testing/ef_tests/src/cases/operations.rs b/testing/ef_tests/src/cases/operations.rs index 798dae083..54195cc23 100644 --- a/testing/ef_tests/src/cases/operations.rs +++ b/testing/ef_tests/src/cases/operations.rs @@ -117,6 +117,11 @@ impl Operation for Deposit { ssz_decode_file(path) } + fn is_enabled_for_fork(_: ForkName) -> bool { + // Some deposit tests require signature verification but are not marked as such. + cfg!(not(feature = "fake_crypto")) + } + fn apply_to( &self, state: &mut BeaconState, diff --git a/testing/ef_tests/src/handler.rs b/testing/ef_tests/src/handler.rs index 92d5db7fd..dd5ed82da 100644 --- a/testing/ef_tests/src/handler.rs +++ b/testing/ef_tests/src/handler.rs @@ -546,6 +546,37 @@ impl Handler for ForkChoiceHandler { } } +#[derive(Derivative)] +#[derivative(Default(bound = ""))] +pub struct OptimisticSyncHandler(PhantomData); + +impl Handler for OptimisticSyncHandler { + type Case = cases::ForkChoiceTest; + + fn config_name() -> &'static str { + E::name() + } + + fn runner_name() -> &'static str { + "sync" + } + + fn handler_name(&self) -> String { + "optimistic".into() + } + + fn use_rayon() -> bool { + // The opt sync tests use `block_on` which can cause panics with rayon. + false + } + + fn is_enabled_for_fork(&self, fork_name: ForkName) -> bool { + fork_name != ForkName::Base + && fork_name != ForkName::Altair + && cfg!(not(feature = "fake_crypto")) + } +} + #[derive(Derivative)] #[derivative(Default(bound = ""))] pub struct GenesisValidityHandler(PhantomData); diff --git a/testing/ef_tests/tests/tests.rs b/testing/ef_tests/tests/tests.rs index 2c8b9d223..28c57028c 100644 --- a/testing/ef_tests/tests/tests.rs +++ b/testing/ef_tests/tests/tests.rs @@ -448,6 +448,12 @@ fn fork_choice_ex_ante() { ForkChoiceHandler::::new("ex_ante").run(); } +#[test] +fn optimistic_sync() { + OptimisticSyncHandler::::default().run(); + OptimisticSyncHandler::::default().run(); +} + #[test] fn genesis_initialization() { GenesisInitializationHandler::::default().run();