diff --git a/consensus/merkle_proof/src/lib.rs b/consensus/merkle_proof/src/lib.rs index b01f3f4429f..271e676df1c 100644 --- a/consensus/merkle_proof/src/lib.rs +++ b/consensus/merkle_proof/src/lib.rs @@ -34,6 +34,8 @@ pub enum MerkleTree { pub enum MerkleTreeError { // Trying to push in a leaf LeafReached, + // Trying to generate a proof for a non-leaf node + NonLeafProof, // No more space in the MerkleTree MerkleTreeFull, // MerkleTree is invalid @@ -313,8 +315,17 @@ impl MerkleTree { current_depth -= 1; } - debug_assert_eq!(proof.len(), depth); - debug_assert!(current_node.is_leaf()); + if proof.len() != depth { + // This should be unreachable regardless of how the method is called, because we push + // one proof element for each layer of `depth`. + return Err(MerkleTreeError::PleaseNotifyTheDevs); + } + + // Generating a proof for a non-leaf node is invalid and indicates an error on the part of + // the caller. + if !current_node.is_leaf() { + return Err(MerkleTreeError::NonLeafProof); + } // Put proof in bottom-up order. proof.reverse(); diff --git a/consensus/types/src/beacon_block_body.rs b/consensus/types/src/beacon_block_body.rs index 3f75790a357..410374c18f3 100644 --- a/consensus/types/src/beacon_block_body.rs +++ b/consensus/types/src/beacon_block_body.rs @@ -277,9 +277,9 @@ impl<'a, E: EthSpec, Payload: AbstractExecPayload> BeaconBlockBodyRef<'a, E, // https://github.com/ethereum/consensus-specs/blob/dev/specs/deneb/beacon-chain.md#beaconblockbody generalized_index .checked_sub(NUM_BEACON_BLOCK_BODY_HASH_TREE_ROOT_LEAVES) - .ok_or(Error::IndexNotSupported(generalized_index))? + .ok_or(Error::GeneralizedIndexNotSupported(generalized_index))? } - _ => return Err(Error::IndexNotSupported(generalized_index)), + _ => return Err(Error::GeneralizedIndexNotSupported(generalized_index)), }; let leaves = self.body_merkle_leaves(); diff --git a/consensus/types/src/beacon_state.rs b/consensus/types/src/beacon_state.rs index 157271b2272..4aed79898d3 100644 --- a/consensus/types/src/beacon_state.rs +++ b/consensus/types/src/beacon_state.rs @@ -157,6 +157,7 @@ pub enum Error { current_fork: ForkName, }, TotalActiveBalanceDiffUninitialized, + GeneralizedIndexNotSupported(usize), IndexNotSupported(usize), InvalidFlagIndex(usize), MerkleTreeError(merkle_proof::MerkleTreeError), @@ -2580,11 +2581,12 @@ impl BeaconState { // for the internal nodes. Result should be 22 or 23, the field offset of the committee // in the `BeaconState`: // https://github.com/ethereum/consensus-specs/blob/dev/specs/altair/beacon-chain.md#beaconstate - let field_index = if self.fork_name_unchecked().electra_enabled() { + let field_gindex = if self.fork_name_unchecked().electra_enabled() { light_client_update::CURRENT_SYNC_COMMITTEE_INDEX_ELECTRA } else { light_client_update::CURRENT_SYNC_COMMITTEE_INDEX }; + let field_index = field_gindex.safe_sub(self.num_fields_pow2())?; let leaves = self.get_beacon_state_leaves(); self.generate_proof(field_index, &leaves) } @@ -2594,11 +2596,12 @@ impl BeaconState { // for the internal nodes. Result should be 22 or 23, the field offset of the committee // in the `BeaconState`: // https://github.com/ethereum/consensus-specs/blob/dev/specs/altair/beacon-chain.md#beaconstate - let field_index = if self.fork_name_unchecked().electra_enabled() { + let field_gindex = if self.fork_name_unchecked().electra_enabled() { light_client_update::NEXT_SYNC_COMMITTEE_INDEX_ELECTRA } else { light_client_update::NEXT_SYNC_COMMITTEE_INDEX }; + let field_index = field_gindex.safe_sub(self.num_fields_pow2())?; let leaves = self.get_beacon_state_leaves(); self.generate_proof(field_index, &leaves) } @@ -2606,17 +2609,24 @@ impl BeaconState { pub fn compute_finalized_root_proof(&self) -> Result, Error> { // Finalized root is the right child of `finalized_checkpoint`, divide by two to get // the generalized index of `state.finalized_checkpoint`. - let field_index = if self.fork_name_unchecked().electra_enabled() { - // Index should be 169/2 - 64 = 20 which matches the position - // of `finalized_checkpoint` in `BeaconState` + let checkpoint_root_gindex = if self.fork_name_unchecked().electra_enabled() { light_client_update::FINALIZED_ROOT_INDEX_ELECTRA } else { - // Index should be 105/2 - 32 = 20 which matches the position - // of `finalized_checkpoint` in `BeaconState` light_client_update::FINALIZED_ROOT_INDEX }; + let checkpoint_gindex = checkpoint_root_gindex / 2; + + // Convert gindex to index by subtracting 2**depth (gindex = 2**depth + index). + // + // After Electra, the index should be 169/2 - 64 = 20 which matches the position + // of `finalized_checkpoint` in `BeaconState`. + // + // Prior to Electra, the index should be 105/2 - 32 = 20 which matches the position + // of `finalized_checkpoint` in `BeaconState`. + let checkpoint_index = checkpoint_gindex.safe_sub(self.num_fields_pow2())?; + let leaves = self.get_beacon_state_leaves(); - let mut proof = self.generate_proof(field_index, &leaves)?; + let mut proof = self.generate_proof(checkpoint_index, &leaves)?; proof.insert(0, self.finalized_checkpoint().epoch.tree_hash_root()); Ok(proof) } @@ -2626,6 +2636,10 @@ impl BeaconState { field_index: usize, leaves: &[Hash256], ) -> Result, Error> { + if field_index >= leaves.len() { + return Err(Error::IndexNotSupported(field_index)); + } + let depth = self.num_fields_pow2().ilog2() as usize; let tree = merkle_proof::MerkleTree::create(leaves, depth); let (_, proof) = tree.generate_proof(field_index, depth)?; diff --git a/testing/ef_tests/check_all_files_accessed.py b/testing/ef_tests/check_all_files_accessed.py index 8a662b72e35..4e744b797a5 100755 --- a/testing/ef_tests/check_all_files_accessed.py +++ b/testing/ef_tests/check_all_files_accessed.py @@ -27,10 +27,8 @@ "tests/.*/.*/ssz_static/PowBlock/", # We no longer implement merge logic. "tests/.*/bellatrix/fork_choice/on_merge_block", - # light_client - "tests/.*/.*/light_client/single_merkle_proof", + # Light client sync is not implemented "tests/.*/.*/light_client/sync", - "tests/.*/electra/light_client/update_ranking", # LightClientStore "tests/.*/.*/ssz_static/LightClientStore", # LightClientSnapshot diff --git a/testing/ef_tests/src/cases/merkle_proof_validity.rs b/testing/ef_tests/src/cases/merkle_proof_validity.rs index 109d2cc7969..711974dd43f 100644 --- a/testing/ef_tests/src/cases/merkle_proof_validity.rs +++ b/testing/ef_tests/src/cases/merkle_proof_validity.rs @@ -20,6 +20,12 @@ pub struct MerkleProof { pub branch: Vec, } +#[derive(Debug)] +pub enum GenericMerkleProofValidity { + BeaconState(BeaconStateMerkleProofValidity), + BeaconBlockBody(Box>), +} + #[derive(Debug, Clone, Deserialize)] #[serde(bound = "E: EthSpec")] pub struct BeaconStateMerkleProofValidity { @@ -28,6 +34,39 @@ pub struct BeaconStateMerkleProofValidity { pub merkle_proof: MerkleProof, } +impl LoadCase for GenericMerkleProofValidity { + fn load_from_dir(path: &Path, fork_name: ForkName) -> Result { + let path_components = path.iter().collect::>(); + + // The "suite" name is the 2nd last directory in the path. + assert!( + path_components.len() >= 2, + "path should have at least 2 components" + ); + let suite_name = path_components[path_components.len() - 2]; + + if suite_name == "BeaconState" { + BeaconStateMerkleProofValidity::load_from_dir(path, fork_name) + .map(GenericMerkleProofValidity::BeaconState) + } else if suite_name == "BeaconBlockBody" { + BeaconBlockBodyMerkleProofValidity::load_from_dir(path, fork_name) + .map(Box::new) + .map(GenericMerkleProofValidity::BeaconBlockBody) + } else { + panic!("unsupported type for merkle proof test: {:?}", suite_name) + } + } +} + +impl Case for GenericMerkleProofValidity { + fn result(&self, case_index: usize, fork_name: ForkName) -> Result<(), Error> { + match self { + Self::BeaconState(test) => test.result(case_index, fork_name), + Self::BeaconBlockBody(test) => test.result(case_index, fork_name), + } + } +} + impl LoadCase for BeaconStateMerkleProofValidity { fn load_from_dir(path: &Path, fork_name: ForkName) -> Result { let spec = &testing_spec::(fork_name); @@ -72,11 +111,9 @@ impl Case for BeaconStateMerkleProofValidity { } }; - let Ok(proof) = proof else { - return Err(Error::FailedToParseTest( - "Could not retrieve merkle proof".to_string(), - )); - }; + let proof = proof.map_err(|e| { + Error::FailedToParseTest(format!("Could not retrieve merkle proof: {e:?}")) + })?; let proof_len = proof.len(); let branch_len = self.merkle_proof.branch.len(); if proof_len != branch_len { @@ -273,11 +310,11 @@ impl Case for BeaconBlockBodyMerkleProofValidity { fn result(&self, _case_index: usize, _fork_name: ForkName) -> Result<(), Error> { let binding = self.block_body.clone(); let block_body = binding.to_ref(); - let Ok(proof) = block_body.block_body_merkle_proof(self.merkle_proof.leaf_index) else { - return Err(Error::FailedToParseTest( - "Could not retrieve merkle proof".to_string(), - )); - }; + let proof = block_body + .block_body_merkle_proof(self.merkle_proof.leaf_index) + .map_err(|e| { + Error::FailedToParseTest(format!("Could not retrieve merkle proof: {e:?}")) + })?; let proof_len = proof.len(); let branch_len = self.merkle_proof.branch.len(); if proof_len != branch_len { diff --git a/testing/ef_tests/src/handler.rs b/testing/ef_tests/src/handler.rs index 481c9b2169f..a3754982396 100644 --- a/testing/ef_tests/src/handler.rs +++ b/testing/ef_tests/src/handler.rs @@ -1000,30 +1000,6 @@ impl Handler for KZGRecoverCellsAndKZGProofHandler { } } -#[derive(Derivative)] -#[derivative(Default(bound = ""))] -pub struct BeaconStateMerkleProofValidityHandler(PhantomData); - -impl Handler for BeaconStateMerkleProofValidityHandler { - type Case = cases::BeaconStateMerkleProofValidity; - - fn config_name() -> &'static str { - E::name() - } - - fn runner_name() -> &'static str { - "light_client" - } - - fn handler_name(&self) -> String { - "single_merkle_proof/BeaconState".into() - } - - fn is_enabled_for_fork(&self, fork_name: ForkName) -> bool { - fork_name.altair_enabled() - } -} - #[derive(Derivative)] #[derivative(Default(bound = ""))] pub struct KzgInclusionMerkleProofValidityHandler(PhantomData); @@ -1054,10 +1030,10 @@ impl Handler for KzgInclusionMerkleProofValidityHandler(PhantomData); +pub struct MerkleProofValidityHandler(PhantomData); -impl Handler for BeaconBlockBodyMerkleProofValidityHandler { - type Case = cases::BeaconBlockBodyMerkleProofValidity; +impl Handler for MerkleProofValidityHandler { + type Case = cases::GenericMerkleProofValidity; fn config_name() -> &'static str { E::name() @@ -1068,11 +1044,11 @@ impl Handler for BeaconBlockBodyMerkleProofValidityHandle } fn handler_name(&self) -> String { - "single_merkle_proof/BeaconBlockBody".into() + "single_merkle_proof".into() } fn is_enabled_for_fork(&self, fork_name: ForkName) -> bool { - fork_name.capella_enabled() + fork_name.altair_enabled() } } diff --git a/testing/ef_tests/tests/tests.rs b/testing/ef_tests/tests/tests.rs index 1f5a7dd9974..3948708edf5 100644 --- a/testing/ef_tests/tests/tests.rs +++ b/testing/ef_tests/tests/tests.rs @@ -955,13 +955,9 @@ fn kzg_recover_cells_and_proofs() { } #[test] -fn beacon_state_merkle_proof_validity() { - BeaconStateMerkleProofValidityHandler::::default().run(); -} - -#[test] -fn beacon_block_body_merkle_proof_validity() { - BeaconBlockBodyMerkleProofValidityHandler::::default().run(); +fn light_client_merkle_proof_validity() { + MerkleProofValidityHandler::::default().run(); + MerkleProofValidityHandler::::default().run(); } #[test]