cerc-io/lighthouse
16
0
Fork 0
Code Issues 1 Pull Requests Packages Projects Releases Wiki Activity

Permit a null LVH from an INVALID response to newPayload (#4037)

Browse Source 
## Issue Addressed

NA

## Proposed Changes

As discovered in #4034, Lighthouse is not accepting `latest_valid_hash == None` in an `INVALID` response to `newPayload`. The `null`/`None` response *was* illegal at one point, however it was added in https://github.com/ethereum/execution-apis/pull/254.

This PR brings Lighthouse in line with the standard and should fix the root cause of what #4034 patched around.

## Additional Info

NA
This commit is contained in:
Paul Hauner 2023-03-03 04:12:50 +00:00
parent 2b6348781a
commit cac3a66be4
5 changed files with 81 additions and 54 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

25
beacon_node/beacon_chain/src/beacon_chain.rs
View File

@ -5016,18 +5016,34 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
"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() {
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?;
} else {
}
// 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.
Some(latest_valid_hash) => {
self.process_invalid_execution_payload(
&InvalidationOperation::InvalidateMany {
head_block_root,
@ -5037,6 +5053,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
)
.await?;
}
}
Err(BeaconChainError::ExecutionForkChoiceUpdateInvalid { status })
}

23
beacon_node/beacon_chain/src/execution_payload.rs
View File

@ -172,14 +172,26 @@ 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,
@ -187,6 +199,7 @@ async fn notify_new_payload<'a, T: BeaconChainTypes>(
latest_valid_ancestor: latest_valid_hash,
})
.await?;
}
Err(ExecutionPayloadError::RejectedByExecutionEngine { status }.into())
}

24
beacon_node/execution_layer/src/payload_status.rs
View File

@ -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<ExecutionBlockHash>,
validation_error: Option<String>,
},
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.

6
testing/execution_engine_integration/src/geth.rs
View File

@ -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

11
testing/execution_engine_integration/src/test_rig.rs
View File

@ -427,7 +427,16 @@ impl<E: GenericExecutionEngine> TestRig<E> {
.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: