Fix light client merkle proofs (#7007)

Fix a regression introduced in this PR:

- https://github.com/sigp/lighthouse/pull/6361

We were indexing into the `MerkleTree` with raw generalized indices, which was incorrect and triggering `debug_assert` failures, as described here:

- https://github.com/sigp/lighthouse/issues/7005


  - Convert `generalized_index` to the correct leaf index prior to proof generation.
- Add sanity checks on indices used in `BeaconState::generate_proof`.
- Remove debug asserts from `MerkleTree::generate_proof` in favour of actual errors. This would have caught the bug earlier.
- Refactor the EF tests so that the merkle validity tests are actually run. They were misconfigured in a way that resulted in them running silently with 0 test cases, and the `check_all_files_accessed.py` script still had an ignore that covered the test files, so this omission wasn't detected.

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);
+        if proof.len() != depth {
-        debug_assert!(current_node.is_leaf());
+            // 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();

consensus/types/src/beacon_block_body.rs

@@ -277,9 +277,9 @@ impl<'a, E: EthSpec, Payload: AbstractExecPayload<E>> 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();

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<E: EthSpec> BeaconState<E> {
         // 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<E: EthSpec> BeaconState<E> {
         // 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<E: EthSpec> BeaconState<E> {
     pub fn compute_finalized_root_proof(&self) -> Result<Vec<Hash256>, 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() {
+        let checkpoint_root_gindex = if self.fork_name_unchecked().electra_enabled() {
-            // Index should be 169/2 - 64 = 20 which matches the position
-            // of `finalized_checkpoint` in `BeaconState`
             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<E: EthSpec> BeaconState<E> {
         field_index: usize,
         leaves: &[Hash256],
     ) -> Result<Vec<Hash256>, 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)?;

testing/ef_tests/check_all_files_accessed.py

@@ -27,10 +27,8 @@ excluded_paths = [
     "tests/.*/.*/ssz_static/PowBlock/",
     # We no longer implement merge logic.
     "tests/.*/bellatrix/fork_choice/on_merge_block",
-    # light_client
+    # Light client sync is not implemented
-    "tests/.*/.*/light_client/single_merkle_proof",
     "tests/.*/.*/light_client/sync",
-    "tests/.*/electra/light_client/update_ranking",
     # LightClientStore
     "tests/.*/.*/ssz_static/LightClientStore",
     # LightClientSnapshot

testing/ef_tests/src/cases/merkle_proof_validity.rs

@@ -20,6 +20,12 @@ pub struct MerkleProof {
     pub branch: Vec<Hash256>,
 }
 
+#[derive(Debug)]
+pub enum GenericMerkleProofValidity<E: EthSpec> {
+    BeaconState(BeaconStateMerkleProofValidity<E>),
+    BeaconBlockBody(Box<BeaconBlockBodyMerkleProofValidity<E>>),
+}
+
 #[derive(Debug, Clone, Deserialize)]
 #[serde(bound = "E: EthSpec")]
 pub struct BeaconStateMerkleProofValidity<E: EthSpec> {
@@ -28,6 +34,39 @@ pub struct BeaconStateMerkleProofValidity<E: EthSpec> {
     pub merkle_proof: MerkleProof,
 }
 
+impl<E: EthSpec> LoadCase for GenericMerkleProofValidity<E> {
+    fn load_from_dir(path: &Path, fork_name: ForkName) -> Result<Self, Error> {
+        let path_components = path.iter().collect::<Vec<_>>();
+
+        // 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<E: EthSpec> Case for GenericMerkleProofValidity<E> {
+    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<E: EthSpec> LoadCase for BeaconStateMerkleProofValidity<E> {
     fn load_from_dir(path: &Path, fork_name: ForkName) -> Result<Self, Error> {
         let spec = &testing_spec::<E>(fork_name);
@@ -72,11 +111,9 @@ impl<E: EthSpec> Case for BeaconStateMerkleProofValidity<E> {
             }
         };
 
-        let Ok(proof) = proof else {
+        let proof = proof.map_err(|e| {
-            return Err(Error::FailedToParseTest(
+            Error::FailedToParseTest(format!("Could not retrieve merkle proof: {e:?}"))
-                "Could not retrieve merkle proof".to_string(),
+        })?;
-            ));
-        };
         let proof_len = proof.len();
         let branch_len = self.merkle_proof.branch.len();
         if proof_len != branch_len {
@@ -273,11 +310,11 @@ impl<E: EthSpec> Case for BeaconBlockBodyMerkleProofValidity<E> {
     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 {
+        let proof = block_body
-            return Err(Error::FailedToParseTest(
+            .block_body_merkle_proof(self.merkle_proof.leaf_index)
-                "Could not retrieve merkle proof".to_string(),
+            .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 {

testing/ef_tests/src/handler.rs

@@ -1000,30 +1000,6 @@ impl<E: EthSpec> Handler for KZGRecoverCellsAndKZGProofHandler<E> {
     }
 }
 
-#[derive(Derivative)]
-#[derivative(Default(bound = ""))]
-pub struct BeaconStateMerkleProofValidityHandler<E>(PhantomData<E>);
-
-impl<E: EthSpec + TypeName> Handler for BeaconStateMerkleProofValidityHandler<E> {
-    type Case = cases::BeaconStateMerkleProofValidity<E>;
-
-    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<E>(PhantomData<E>);
@@ -1054,10 +1030,10 @@ impl<E: EthSpec + TypeName> Handler for KzgInclusionMerkleProofValidityHandler<E
 
 #[derive(Derivative)]
 #[derivative(Default(bound = ""))]
-pub struct BeaconBlockBodyMerkleProofValidityHandler<E>(PhantomData<E>);
+pub struct MerkleProofValidityHandler<E>(PhantomData<E>);
 
-impl<E: EthSpec + TypeName> Handler for BeaconBlockBodyMerkleProofValidityHandler<E> {
+impl<E: EthSpec + TypeName> Handler for MerkleProofValidityHandler<E> {
-    type Case = cases::BeaconBlockBodyMerkleProofValidity<E>;
+    type Case = cases::GenericMerkleProofValidity<E>;
 
     fn config_name() -> &'static str {
         E::name()
@@ -1068,11 +1044,11 @@ impl<E: EthSpec + TypeName> 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()
     }
 }
 

testing/ef_tests/tests/tests.rs

@@ -955,13 +955,9 @@ fn kzg_recover_cells_and_proofs() {
 }
 
 #[test]
-fn beacon_state_merkle_proof_validity() {
+fn light_client_merkle_proof_validity() {
-    BeaconStateMerkleProofValidityHandler::<MainnetEthSpec>::default().run();
+    MerkleProofValidityHandler::<MinimalEthSpec>::default().run();
-}
+    MerkleProofValidityHandler::<MainnetEthSpec>::default().run();
-
-#[test]
-fn beacon_block_body_merkle_proof_validity() {
-    BeaconBlockBodyMerkleProofValidityHandler::<MainnetEthSpec>::default().run();
 }
 
 #[test]