diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index b4776d1b88..6d9d290bd0 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -5016,26 +5016,43 @@ impl BeaconChain { "status" => ?status ); - // This implies that the terminal block was invalid. We are being explicit in - // invalidating only the head block in this case. - if latest_valid_hash == ExecutionBlockHash::zero() { - self.process_invalid_execution_payload( - &InvalidationOperation::InvalidateOne { - block_root: head_block_root, - }, - ) - .await?; - } else { + match latest_valid_hash { + // The `latest_valid_hash` is set to `None` when the EE + // "cannot determine the ancestor of the invalid + // payload". In such a scenario we should only + // invalidate the head block and nothing else. + None => { + self.process_invalid_execution_payload( + &InvalidationOperation::InvalidateOne { + block_root: head_block_root, + }, + ) + .await?; + } + // An all-zeros execution block hash implies that + // the terminal block was invalid. We are being + // explicit in invalidating only the head block in + // this case. + Some(hash) if hash == ExecutionBlockHash::zero() => { + self.process_invalid_execution_payload( + &InvalidationOperation::InvalidateOne { + block_root: head_block_root, + }, + ) + .await?; + } // The execution engine has stated that all blocks between the // `head_execution_block_hash` and `latest_valid_hash` are invalid. - self.process_invalid_execution_payload( - &InvalidationOperation::InvalidateMany { - head_block_root, - always_invalidate_head: true, - latest_valid_ancestor: latest_valid_hash, - }, - ) - .await?; + Some(latest_valid_hash) => { + self.process_invalid_execution_payload( + &InvalidationOperation::InvalidateMany { + head_block_root, + always_invalidate_head: true, + latest_valid_ancestor: latest_valid_hash, + }, + ) + .await?; + } } Err(BeaconChainError::ExecutionForkChoiceUpdateInvalid { status }) diff --git a/beacon_node/beacon_chain/src/execution_payload.rs b/beacon_node/beacon_chain/src/execution_payload.rs index 3182058173..5cc8ee2d28 100644 --- a/beacon_node/beacon_chain/src/execution_payload.rs +++ b/beacon_node/beacon_chain/src/execution_payload.rs @@ -172,21 +172,34 @@ async fn notify_new_payload<'a, T: BeaconChainTypes>( "method" => "new_payload", ); - // latest_valid_hash == 0 implies that this was the terminal block - // Hence, we don't need to run `BeaconChain::process_invalid_execution_payload`. - if latest_valid_hash == ExecutionBlockHash::zero() { - return Err(ExecutionPayloadError::RejectedByExecutionEngine { status }.into()); + // Only trigger payload invalidation in fork choice if the + // `latest_valid_hash` is `Some` and non-zero. + // + // A `None` latest valid hash indicates that the EE was unable + // to determine the most recent valid ancestor. Since `block` + // has not yet been applied to fork choice, there's nothing to + // invalidate. + // + // An all-zeros payload indicates that an EIP-3675 check has + // failed regarding the validity of the terminal block. Rather + // than iterating back in the chain to find the terminal block + // and invalidating that, we simply reject this block without + // invalidating anything else. + if let Some(latest_valid_hash) = + latest_valid_hash.filter(|hash| *hash != ExecutionBlockHash::zero()) + { + // This block has not yet been applied to fork choice, so the latest block that was + // imported to fork choice was the parent. + let latest_root = block.parent_root(); + + chain + .process_invalid_execution_payload(&InvalidationOperation::InvalidateMany { + head_block_root: latest_root, + always_invalidate_head: false, + latest_valid_ancestor: latest_valid_hash, + }) + .await?; } - // This block has not yet been applied to fork choice, so the latest block that was - // imported to fork choice was the parent. - let latest_root = block.parent_root(); - chain - .process_invalid_execution_payload(&InvalidationOperation::InvalidateMany { - head_block_root: latest_root, - always_invalidate_head: false, - latest_valid_ancestor: latest_valid_hash, - }) - .await?; Err(ExecutionPayloadError::RejectedByExecutionEngine { status }.into()) } diff --git a/beacon_node/execution_layer/src/payload_status.rs b/beacon_node/execution_layer/src/payload_status.rs index 7db8e234d1..5405fd7009 100644 --- a/beacon_node/execution_layer/src/payload_status.rs +++ b/beacon_node/execution_layer/src/payload_status.rs @@ -10,7 +10,9 @@ use types::ExecutionBlockHash; pub enum PayloadStatus { Valid, Invalid { - latest_valid_hash: ExecutionBlockHash, + /// The EE will provide a `None` LVH when it is unable to determine the + /// latest valid ancestor. + latest_valid_hash: Option, validation_error: Option, }, Syncing, @@ -55,22 +57,10 @@ pub fn process_payload_status( }) } } - PayloadStatusV1Status::Invalid => { - if let Some(latest_valid_hash) = response.latest_valid_hash { - // The response is only valid if `latest_valid_hash` is not `null`. - Ok(PayloadStatus::Invalid { - latest_valid_hash, - validation_error: response.validation_error.clone(), - }) - } else { - Err(EngineError::Api { - error: ApiError::BadResponse( - "new_payload: response.status = INVALID but null latest_valid_hash" - .to_string(), - ), - }) - } - } + PayloadStatusV1Status::Invalid => Ok(PayloadStatus::Invalid { + latest_valid_hash: response.latest_valid_hash, + validation_error: response.validation_error, + }), PayloadStatusV1Status::InvalidBlockHash => { // In the interests of being liberal with what we accept, only raise a // warning here. diff --git a/testing/execution_engine_integration/src/geth.rs b/testing/execution_engine_integration/src/geth.rs index 5a1a5d4f53..1b96fa9f3f 100644 --- a/testing/execution_engine_integration/src/geth.rs +++ b/testing/execution_engine_integration/src/geth.rs @@ -7,7 +7,7 @@ use std::{env, fs::File}; use tempfile::TempDir; use unused_port::unused_tcp_port; -// const GETH_BRANCH: &str = "master"; +const GETH_BRANCH: &str = "master"; const GETH_REPO_URL: &str = "https://github.com/ethereum/go-ethereum"; pub fn build_result(repo_dir: &Path) -> Output { @@ -27,9 +27,7 @@ pub fn build(execution_clients_dir: &Path) { } // Get the latest tag on the branch - // TODO: Update when version is corrected - // let last_release = build_utils::get_latest_release(&repo_dir, GETH_BRANCH).unwrap(); - let last_release = "v1.11.1"; + let last_release = build_utils::get_latest_release(&repo_dir, GETH_BRANCH).unwrap(); build_utils::checkout(&repo_dir, dbg!(&last_release)).unwrap(); // Build geth diff --git a/testing/execution_engine_integration/src/test_rig.rs b/testing/execution_engine_integration/src/test_rig.rs index bb416634e5..15e9f26018 100644 --- a/testing/execution_engine_integration/src/test_rig.rs +++ b/testing/execution_engine_integration/src/test_rig.rs @@ -427,7 +427,16 @@ impl TestRig { .notify_new_payload(&invalid_payload) .await .unwrap(); - assert!(matches!(status, PayloadStatus::InvalidBlockHash { .. })); + assert!(matches!( + status, + PayloadStatus::InvalidBlockHash { .. } + // Geth is returning `INVALID` with a `null` LVH to indicate it + // does not know the invalid ancestor. + | PayloadStatus::Invalid { + latest_valid_hash: None, + .. + } + )); /* * Execution Engine A: