Skip to content

Commit

Permalink
fix(l1): fix engine api v3 hive test related to latest valid hash and…
Browse files Browse the repository at this point in the history
… errors on ForkchoiceState (#1233)

**Motivation**

Fix the `engine-cancun` suite tests that we are able to fix with our
current implementation (without v1, v2 or reorg/syncing working as
expected)

**Description**

After a couples of back and forths the actual solution for both errors
are pretty simple, in the case of the latest valid hash it was needed to
send the parent when the evm fails after the parent was checked. In the
case of the ForkchoiceState we were sending an incorrect code we needed
to use the RpcError for FokchoiceState that we already had implemented.

This pass **31** new tests. On regards the Engine Cancun step in the CI,
previously it ran 38 tests, now it runs 149.

**Summary of the outstanding failing test in engine-cancun**

- 1 test (Blob Transaction Ordering) expect 6 blobs but got 5
- 2 test (Invalid NewPayload, ReceiptsRoot) Expected INVALID but got
VALID
- 4 test (Sidechain Reorg|TransactionRe-Org|Re-Org back into canonical
Chain) fails on Reorg or latest valid hash
- 15 tests ({exec_method}V[3|2]) are related to Shanghai (V2)
compatibility (v2 method not found)
- 13 tests(Fork ID: Genesis|Pre-Merge) are also related to previous
versions, (v2|1 method not found)
- 2 test are related to pooled blobs and depend on DevP2PRequestPooled
- 24 tests (Invalid Missing Ancestors) are related to syncing and
timeout

Resolves #1235
  • Loading branch information
rodrigo-o authored Nov 23, 2024
1 parent b9ec164 commit a49590b
Show file tree
Hide file tree
Showing 3 changed files with 9 additions and 7 deletions.
6 changes: 3 additions & 3 deletions .github/workflows/hive.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,11 @@ jobs:
name: "Devp2p eth tests"
run_command: make run-hive-on-latest SIMULATION=devp2p TEST_PATTERN="eth/Status|GetBlockHeaders|SimultaneousRequests|SameRequestID|ZeroRequestID|GetBlockBodies|MaliciousHandshake|MaliciousStatus|Transaction"
- simulation: engine
name: "Engine tests"
run_command: make run-hive-on-latest SIMULATION=ethereum/engine TEST_PATTERN="/Blob Transactions On Block 1, Cancun Genesis|Blob Transactions On Block 1, Shanghai Genesis|Blob Transaction Ordering, Single Account, Single Blob|Blob Transaction Ordering, Single Account, Dual Blob|Blob Transaction Ordering, Multiple Accounts|Replace Blob Transactions|Parallel Blob Transactions|ForkchoiceUpdatedV3 Modifies Payload ID on Different Beacon Root|NewPayloadV3 After Cancun|NewPayloadV3 Versioned Hashes|ForkchoiceUpdated Version on Payload Request"
name: "Engine Auth and EC tests"
run_command: make run-hive-on-latest SIMULATION=ethereum/engine TEST_PATTERN="engine-(auth|exchange-capabilities)/"
- simulation: engine-cancun
name: "Cancun Engine tests"
run_command: make run-hive-on-latest SIMULATION=ethereum/engine HIVE_EXTRA_ARGS="--sim.parallelism 4" TEST_PATTERN="cancun/Unique Payload ID|ParentHash equals BlockHash on NewPayload|Re-Execute Payload|Payload Build after New Invalid Payload|RPC|Build Payload with Invalid ChainID|Invalid PayloadAttributes, Zero timestamp, Syncing=False|Invalid PayloadAttributes, Parent timestamp, Syncing=False|Invalid PayloadAttributes, Missing BeaconRoot, Syncing=False|Suggested Fee Recipient Test|PrevRandao Opcode Transactions Test|Invalid Missing Ancestor ReOrg, StateRoot"
run_command: make run-hive-on-latest SIMULATION=ethereum/engine HIVE_EXTRA_ARGS="--sim.parallelism 4" TEST_PATTERN="engine-cancun/Blob Transactions On Block 1|Blob Transaction Ordering, Single|Blob Transaction Ordering, Multiple Accounts|Replace Blob Transactions|Parallel Blob Transactions|ForkchoiceUpdatedV3 Modifies Payload ID on Different Beacon Root|NewPayloadV3 After Cancun|NewPayloadV3 Versioned Hashes|Incorrect BlobGasUsed|Bad Hash|ParentHash equals BlockHash|RPC:|in ForkchoiceState|Unknown|Invalid PayloadAttributes|Unique|ForkchoiceUpdated Version on Payload Request|Re-Execute Payload|In-Order Consecutive Payload|Multiple New Payloads|Valid NewPayload->|NewPayload with|Payload Build after|Build Payload with|Invalid Missing Ancestor ReOrg, StateRoot|Re-Org Back to|Re-org to Previously|Safe Re-Org to Side Chain|Transaction Re-Org, Re-Org Back In|Re-Org Back into Canonical Chain, Depth=5|Suggested Fee Recipient Test|PrevRandao Opcode|Invalid NewPayload, [^R][^e]|Fork ID Genesis=0, Cancun=0|Fork ID Genesis=0, Cancun=1|Fork ID Genesis=1, Cancun=0|Fork ID Genesis=1, Cancun=2, Shanghai=2"
steps:
- name: Download artifacts
uses: actions/download-artifact@v4
Expand Down
4 changes: 1 addition & 3 deletions crates/networking/rpc/engine/fork_choice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,7 @@ impl RpcHandler for ForkChoiceUpdatedV3 {
InvalidForkChoice::Syncing => ForkChoiceResponse::from(PayloadStatus::syncing()),
reason => {
warn!("Invalid fork choice state. Reason: {:#?}", reason);
ForkChoiceResponse::from(PayloadStatus::invalid_with_err(
reason.to_string().as_str(),
))
return Err(RpcErr::InvalidForkChoiceState(reason.to_string()));
}
};

Expand Down
6 changes: 5 additions & 1 deletion crates/networking/rpc/engine/payload.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ impl RpcHandler for NewPayloadV3Request {

fn handle(&self, context: RpcApiContext) -> Result<Value, RpcErr> {
let storage = &context.storage;

let block_hash = self.payload.block_hash;
info!("Received new payload with block hash: {block_hash:#x}");

Expand Down Expand Up @@ -136,7 +137,10 @@ impl RpcHandler for NewPayloadV3Request {
}
Err(ChainError::EvmError(error)) => {
warn!("Error executing block: {error}");
Ok(PayloadStatus::invalid_with_err(&error.to_string()))
Ok(PayloadStatus::invalid_with(
block.header.parent_hash,
error.to_string(),
))
}
Err(ChainError::StoreError(error)) => {
warn!("Error storing block: {error}");
Expand Down

0 comments on commit a49590b

Please sign in to comment.