From c9747fb77f3ff1d8da765003b4988c20ce2d7cd8 Mon Sep 17 00:00:00 2001 From: Jimmy Chen Date: Mon, 13 Jan 2025 11:08:51 +1100 Subject: [PATCH] Refactor feature testing for spec tests (#6737) * Refactor spec testing for features and simplify usage. * Fix `SszStatic` tests for PeerDAS: exclude eip7594 test vectors when testing Electra types. * Merge branch 'unstable' into refactor-ef-tests-features --- testing/ef_tests/src/cases.rs | 36 +++++++++++++-- .../ef_tests/src/cases/get_custody_columns.rs | 9 ++++ .../src/cases/kzg_blob_to_kzg_commitment.rs | 4 -- .../src/cases/kzg_compute_blob_kzg_proof.rs | 4 -- .../cases/kzg_compute_cells_and_kzg_proofs.rs | 8 +++- .../src/cases/kzg_compute_kzg_proof.rs | 4 -- .../cases/kzg_recover_cells_and_kzg_proofs.rs | 8 +++- .../src/cases/kzg_verify_blob_kzg_proof.rs | 4 -- .../cases/kzg_verify_blob_kzg_proof_batch.rs | 4 -- .../cases/kzg_verify_cell_kzg_proof_batch.rs | 8 +++- .../src/cases/kzg_verify_kzg_proof.rs | 4 -- testing/ef_tests/src/handler.rs | 46 ++++++++++++++----- testing/ef_tests/tests/tests.rs | 20 ++++---- 13 files changed, 103 insertions(+), 56 deletions(-) diff --git a/testing/ef_tests/src/cases.rs b/testing/ef_tests/src/cases.rs index 63274ee0c03..8f5571d64a6 100644 --- a/testing/ef_tests/src/cases.rs +++ b/testing/ef_tests/src/cases.rs @@ -74,11 +74,37 @@ pub use ssz_generic::*; pub use ssz_static::*; pub use transition::TransitionTest; -#[derive(Debug, PartialEq)] +/// Used for running feature tests for future forks that have not yet been added to `ForkName`. +/// This runs tests in the directory named by the feature instead of the fork name. This has been +/// the pattern used in the `consensus-spec-tests` repository: +/// `consensus-spec-tests/tests/general/[feature_name]/[runner_name].` +/// e.g. consensus-spec-tests/tests/general/peerdas/ssz_static +/// +/// The feature tests can be run with one of the following methods: +/// 1. `handler.run_for_feature(feature_name)` for new tests that are not on existing fork, i.e. a +/// new handler. This will be temporary and the test will need to be updated to use +/// `handle.run()` once the feature is incorporated into a fork. +/// 2. `handler.run()` for tests that are already on existing forks, but with new test vectors for +/// the feature. In this case the `handler.is_enabled_for_feature` will need to be implemented +/// to return `true` for the feature in order for the feature test vector to be tested. +#[derive(Debug, PartialEq, Clone, Copy)] pub enum FeatureName { Eip7594, } +impl FeatureName { + pub fn list_all() -> Vec { + vec![FeatureName::Eip7594] + } + + /// `ForkName` to use when running the feature tests. + pub fn fork_name(&self) -> ForkName { + match self { + FeatureName::Eip7594 => ForkName::Deneb, + } + } +} + impl Display for FeatureName { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { match self { @@ -107,11 +133,13 @@ pub trait Case: Debug + Sync { true } - /// Whether or not this test exists for the given `feature_name`. + /// Whether or not this test exists for the given `feature_name`. This is intended to be used + /// for features that have not been added to a fork yet, and there is usually a separate folder + /// for the feature in the `consensus-spec-tests` repository. /// - /// Returns `true` by default. + /// Returns `false` by default. fn is_enabled_for_feature(_feature_name: FeatureName) -> bool { - true + false } /// Execute a test and return the result. diff --git a/testing/ef_tests/src/cases/get_custody_columns.rs b/testing/ef_tests/src/cases/get_custody_columns.rs index 9665f877300..71b17aeaa3f 100644 --- a/testing/ef_tests/src/cases/get_custody_columns.rs +++ b/testing/ef_tests/src/cases/get_custody_columns.rs @@ -21,6 +21,14 @@ impl LoadCase for GetCustodyColumns { } impl Case for GetCustodyColumns { + fn is_enabled_for_fork(_fork_name: ForkName) -> bool { + false + } + + fn is_enabled_for_feature(feature_name: FeatureName) -> bool { + feature_name == FeatureName::Eip7594 + } + fn result(&self, _case_index: usize, _fork_name: ForkName) -> Result<(), Error> { let spec = E::default_spec(); let node_id = U256::from_str_radix(&self.node_id, 10) @@ -33,6 +41,7 @@ impl Case for GetCustodyColumns { ) .expect("should compute custody columns") .collect::>(); + let expected = &self.result; if computed == *expected { Ok(()) diff --git a/testing/ef_tests/src/cases/kzg_blob_to_kzg_commitment.rs b/testing/ef_tests/src/cases/kzg_blob_to_kzg_commitment.rs index fa16a5fcb7a..feb9a4ff5c4 100644 --- a/testing/ef_tests/src/cases/kzg_blob_to_kzg_commitment.rs +++ b/testing/ef_tests/src/cases/kzg_blob_to_kzg_commitment.rs @@ -31,10 +31,6 @@ impl Case for KZGBlobToKZGCommitment { fork_name == ForkName::Deneb } - fn is_enabled_for_feature(feature_name: FeatureName) -> bool { - feature_name != FeatureName::Eip7594 - } - fn result(&self, _case_index: usize, _fork_name: ForkName) -> Result<(), Error> { let kzg = get_kzg(); let commitment = parse_blob::(&self.input.blob).and_then(|blob| { diff --git a/testing/ef_tests/src/cases/kzg_compute_blob_kzg_proof.rs b/testing/ef_tests/src/cases/kzg_compute_blob_kzg_proof.rs index 694013e2513..4aadc37af21 100644 --- a/testing/ef_tests/src/cases/kzg_compute_blob_kzg_proof.rs +++ b/testing/ef_tests/src/cases/kzg_compute_blob_kzg_proof.rs @@ -32,10 +32,6 @@ impl Case for KZGComputeBlobKZGProof { fork_name == ForkName::Deneb } - fn is_enabled_for_feature(feature_name: FeatureName) -> bool { - feature_name != FeatureName::Eip7594 - } - fn result(&self, _case_index: usize, _fork_name: ForkName) -> Result<(), Error> { let parse_input = |input: &KZGComputeBlobKZGProofInput| -> Result<_, Error> { let blob = parse_blob::(&input.blob)?; diff --git a/testing/ef_tests/src/cases/kzg_compute_cells_and_kzg_proofs.rs b/testing/ef_tests/src/cases/kzg_compute_cells_and_kzg_proofs.rs index 2a9f8ceeef3..a7219f0629a 100644 --- a/testing/ef_tests/src/cases/kzg_compute_cells_and_kzg_proofs.rs +++ b/testing/ef_tests/src/cases/kzg_compute_cells_and_kzg_proofs.rs @@ -26,8 +26,12 @@ impl LoadCase for KZGComputeCellsAndKZGProofs { } impl Case for KZGComputeCellsAndKZGProofs { - fn is_enabled_for_fork(fork_name: ForkName) -> bool { - fork_name == ForkName::Deneb + fn is_enabled_for_fork(_fork_name: ForkName) -> bool { + false + } + + fn is_enabled_for_feature(feature_name: FeatureName) -> bool { + feature_name == FeatureName::Eip7594 } fn result(&self, _case_index: usize, _fork_name: ForkName) -> Result<(), Error> { diff --git a/testing/ef_tests/src/cases/kzg_compute_kzg_proof.rs b/testing/ef_tests/src/cases/kzg_compute_kzg_proof.rs index 6f53038f28e..4a47fe35ebe 100644 --- a/testing/ef_tests/src/cases/kzg_compute_kzg_proof.rs +++ b/testing/ef_tests/src/cases/kzg_compute_kzg_proof.rs @@ -39,10 +39,6 @@ impl Case for KZGComputeKZGProof { fork_name == ForkName::Deneb } - fn is_enabled_for_feature(feature_name: FeatureName) -> bool { - feature_name != FeatureName::Eip7594 - } - fn result(&self, _case_index: usize, _fork_name: ForkName) -> Result<(), Error> { let parse_input = |input: &KZGComputeKZGProofInput| -> Result<_, Error> { let blob = parse_blob::(&input.blob)?; diff --git a/testing/ef_tests/src/cases/kzg_recover_cells_and_kzg_proofs.rs b/testing/ef_tests/src/cases/kzg_recover_cells_and_kzg_proofs.rs index 10cc866fbe0..b72b3a05cd6 100644 --- a/testing/ef_tests/src/cases/kzg_recover_cells_and_kzg_proofs.rs +++ b/testing/ef_tests/src/cases/kzg_recover_cells_and_kzg_proofs.rs @@ -27,8 +27,12 @@ impl LoadCase for KZGRecoverCellsAndKZGProofs { } impl Case for KZGRecoverCellsAndKZGProofs { - fn is_enabled_for_fork(fork_name: ForkName) -> bool { - fork_name == ForkName::Deneb + fn is_enabled_for_fork(_fork_name: ForkName) -> bool { + false + } + + fn is_enabled_for_feature(feature_name: FeatureName) -> bool { + feature_name == FeatureName::Eip7594 } fn result(&self, _case_index: usize, _fork_name: ForkName) -> Result<(), Error> { diff --git a/testing/ef_tests/src/cases/kzg_verify_blob_kzg_proof.rs b/testing/ef_tests/src/cases/kzg_verify_blob_kzg_proof.rs index 3dc955bdcc8..66f50d534b8 100644 --- a/testing/ef_tests/src/cases/kzg_verify_blob_kzg_proof.rs +++ b/testing/ef_tests/src/cases/kzg_verify_blob_kzg_proof.rs @@ -116,10 +116,6 @@ impl Case for KZGVerifyBlobKZGProof { fork_name == ForkName::Deneb } - fn is_enabled_for_feature(feature_name: FeatureName) -> bool { - feature_name != FeatureName::Eip7594 - } - fn result(&self, _case_index: usize, _fork_name: ForkName) -> Result<(), Error> { let parse_input = |input: &KZGVerifyBlobKZGProofInput| -> Result<(Blob, KzgCommitment, KzgProof), Error> { let blob = parse_blob::(&input.blob)?; diff --git a/testing/ef_tests/src/cases/kzg_verify_blob_kzg_proof_batch.rs b/testing/ef_tests/src/cases/kzg_verify_blob_kzg_proof_batch.rs index 80cd0a28496..efd41588069 100644 --- a/testing/ef_tests/src/cases/kzg_verify_blob_kzg_proof_batch.rs +++ b/testing/ef_tests/src/cases/kzg_verify_blob_kzg_proof_batch.rs @@ -33,10 +33,6 @@ impl Case for KZGVerifyBlobKZGProofBatch { fork_name == ForkName::Deneb } - fn is_enabled_for_feature(feature_name: FeatureName) -> bool { - feature_name != FeatureName::Eip7594 - } - fn result(&self, _case_index: usize, _fork_name: ForkName) -> Result<(), Error> { let parse_input = |input: &KZGVerifyBlobKZGProofBatchInput| -> Result<_, Error> { let blobs = input diff --git a/testing/ef_tests/src/cases/kzg_verify_cell_kzg_proof_batch.rs b/testing/ef_tests/src/cases/kzg_verify_cell_kzg_proof_batch.rs index 5887d764cae..815ad7a5bcf 100644 --- a/testing/ef_tests/src/cases/kzg_verify_cell_kzg_proof_batch.rs +++ b/testing/ef_tests/src/cases/kzg_verify_cell_kzg_proof_batch.rs @@ -29,8 +29,12 @@ impl LoadCase for KZGVerifyCellKZGProofBatch { } impl Case for KZGVerifyCellKZGProofBatch { - fn is_enabled_for_fork(fork_name: ForkName) -> bool { - fork_name == ForkName::Deneb + fn is_enabled_for_fork(_fork_name: ForkName) -> bool { + false + } + + fn is_enabled_for_feature(feature_name: FeatureName) -> bool { + feature_name == FeatureName::Eip7594 } fn result(&self, _case_index: usize, _fork_name: ForkName) -> Result<(), Error> { diff --git a/testing/ef_tests/src/cases/kzg_verify_kzg_proof.rs b/testing/ef_tests/src/cases/kzg_verify_kzg_proof.rs index ed7583dbd0a..07df05a6ac1 100644 --- a/testing/ef_tests/src/cases/kzg_verify_kzg_proof.rs +++ b/testing/ef_tests/src/cases/kzg_verify_kzg_proof.rs @@ -33,10 +33,6 @@ impl Case for KZGVerifyKZGProof { fork_name == ForkName::Deneb } - fn is_enabled_for_feature(feature_name: FeatureName) -> bool { - feature_name != FeatureName::Eip7594 - } - fn result(&self, _case_index: usize, _fork_name: ForkName) -> Result<(), Error> { let parse_input = |input: &KZGVerifyKZGProofInput| -> Result<_, Error> { let commitment = parse_commitment(&input.commitment)?; diff --git a/testing/ef_tests/src/handler.rs b/testing/ef_tests/src/handler.rs index e7c148645c4..d8fe061061a 100644 --- a/testing/ef_tests/src/handler.rs +++ b/testing/ef_tests/src/handler.rs @@ -7,9 +7,6 @@ use std::marker::PhantomData; use std::path::PathBuf; use types::{BeaconState, EthSpec, ForkName}; -const EIP7594_FORK: ForkName = ForkName::Deneb; -const EIP7594_TESTS: [&str; 4] = ["ssz_static", "merkle_proof", "networking", "kzg"]; - pub trait Handler { type Case: Case + LoadCase; @@ -39,13 +36,12 @@ pub trait Handler { for fork_name in ForkName::list_all() { if !self.disabled_forks().contains(&fork_name) && self.is_enabled_for_fork(fork_name) { self.run_for_fork(fork_name); + } + } - if fork_name == EIP7594_FORK - && EIP7594_TESTS.contains(&Self::runner_name()) - && self.is_enabled_for_feature(FeatureName::Eip7594) - { - self.run_for_feature(EIP7594_FORK, FeatureName::Eip7594); - } + for feature_name in FeatureName::list_all() { + if self.is_enabled_for_feature(feature_name) { + self.run_for_feature(feature_name); } } } @@ -96,8 +92,9 @@ pub trait Handler { crate::results::assert_tests_pass(&name, &handler_path, &results); } - fn run_for_feature(&self, fork_name: ForkName, feature_name: FeatureName) { + fn run_for_feature(&self, feature_name: FeatureName) { let feature_name_str = feature_name.to_string(); + let fork_name = feature_name.fork_name(); let handler_path = PathBuf::from(env!("CARGO_MANIFEST_DIR")) .join("consensus-spec-tests") @@ -352,6 +349,22 @@ where fn is_enabled_for_fork(&self, fork_name: ForkName) -> bool { self.supported_forks.contains(&fork_name) } + + fn is_enabled_for_feature(&self, feature_name: FeatureName) -> bool { + // This ensures we only run the tests **once** for `Eip7594`, using the types matching the + // correct fork, e.g. `Eip7594` uses SSZ types from `Deneb` as of spec test version + // `v1.5.0-alpha.8`, therefore the `Eip7594` tests should get included when testing Deneb types. + // + // e.g. Eip7594 test vectors are executed in the first line below, but excluded in the 2nd + // line when testing the type `AttestationElectra`: + // + // ``` + // SszStaticHandler::, MainnetEthSpec>::pre_electra().run(); + // SszStaticHandler::, MainnetEthSpec>::electra_only().run(); + // ``` + feature_name == FeatureName::Eip7594 + && self.supported_forks.contains(&feature_name.fork_name()) + } } impl Handler for SszStaticTHCHandler, E> @@ -371,6 +384,10 @@ where fn handler_name(&self) -> String { BeaconState::::name().into() } + + fn is_enabled_for_feature(&self, feature_name: FeatureName) -> bool { + feature_name == FeatureName::Eip7594 + } } impl Handler for SszStaticWithSpecHandler @@ -392,6 +409,10 @@ where fn handler_name(&self) -> String { T::name().into() } + + fn is_enabled_for_feature(&self, feature_name: FeatureName) -> bool { + feature_name == FeatureName::Eip7594 + } } #[derive(Derivative)] @@ -971,9 +992,12 @@ impl Handler for KzgInclusionMerkleProofValidityHandler bool { - // Enabled in Deneb fork_name.deneb_enabled() } + + fn is_enabled_for_feature(&self, feature_name: FeatureName) -> bool { + feature_name == FeatureName::Eip7594 + } } #[derive(Derivative)] diff --git a/testing/ef_tests/tests/tests.rs b/testing/ef_tests/tests/tests.rs index 292625a371a..691d27951ad 100644 --- a/testing/ef_tests/tests/tests.rs +++ b/testing/ef_tests/tests/tests.rs @@ -627,17 +627,17 @@ mod ssz_static { #[test] fn data_column_sidecar() { SszStaticHandler::, MinimalEthSpec>::deneb_only() - .run_for_feature(ForkName::Deneb, FeatureName::Eip7594); + .run_for_feature(FeatureName::Eip7594); SszStaticHandler::, MainnetEthSpec>::deneb_only() - .run_for_feature(ForkName::Deneb, FeatureName::Eip7594); + .run_for_feature(FeatureName::Eip7594); } #[test] fn data_column_identifier() { SszStaticHandler::::deneb_only() - .run_for_feature(ForkName::Deneb, FeatureName::Eip7594); + .run_for_feature(FeatureName::Eip7594); SszStaticHandler::::deneb_only() - .run_for_feature(ForkName::Deneb, FeatureName::Eip7594); + .run_for_feature(FeatureName::Eip7594); } #[test] @@ -902,19 +902,19 @@ fn kzg_verify_kzg_proof() { #[test] fn kzg_compute_cells_and_proofs() { KZGComputeCellsAndKZGProofHandler::::default() - .run_for_feature(ForkName::Deneb, FeatureName::Eip7594); + .run_for_feature(FeatureName::Eip7594); } #[test] fn kzg_verify_cell_proof_batch() { KZGVerifyCellKZGProofBatchHandler::::default() - .run_for_feature(ForkName::Deneb, FeatureName::Eip7594); + .run_for_feature(FeatureName::Eip7594); } #[test] fn kzg_recover_cells_and_proofs() { KZGRecoverCellsAndKZGProofHandler::::default() - .run_for_feature(ForkName::Deneb, FeatureName::Eip7594); + .run_for_feature(FeatureName::Eip7594); } #[test] @@ -949,8 +949,6 @@ fn rewards() { #[test] fn get_custody_columns() { - GetCustodyColumnsHandler::::default() - .run_for_feature(ForkName::Deneb, FeatureName::Eip7594); - GetCustodyColumnsHandler::::default() - .run_for_feature(ForkName::Deneb, FeatureName::Eip7594); + GetCustodyColumnsHandler::::default().run_for_feature(FeatureName::Eip7594); + GetCustodyColumnsHandler::::default().run_for_feature(FeatureName::Eip7594); }