Skip to content

Commit 8bab564

Browse files
edg-ledg-l
authored
fix(l1): suppress payload build on FCU with finalized-ancestor head (#6676)
## Summary execution-apis PR [#786](ethereum/execution-apis#786) revised paris.md: when `forkchoiceState.headBlockHash` references a VALID ancestor of the latest known finalized block, the response MUST be `{payloadStatus: VALID, payloadId: null}` and the client MUST NOT begin a payload build process. ethrex already had the correct `InvalidForkChoice::NewHeadAlreadyCanonical` detection path in `crates/blockchain/fork_choice.rs` (gate: `head.number <= stored_finalized && head_is_canonical`; the `<=` matches geth's `block.NumberU64() <= finalized.Number.Uint64()` in `eth/catalyst/api.go`, see geth PR [#34767](ethereum/go-ethereum#34767)). The bug was in `crates/networking/rpc/engine/fork_choice.rs`: the `NewHeadAlreadyCanonical` arm returned `Some(head_header)`, which caused `ForkChoiceUpdatedV3::handle` / `ForkChoiceUpdatedV4::handle` to call `build_payload` and set a non-null `payloadId` when `payloadAttributes` was present. Fix: return `None` for the head header so the V3/V4 dispatch's `if let (Some(_), Some(_))` guard short-circuits `build_payload`. The `VALID + latestValidHash = head` response is preserved, matching geth's `valid(nil)` return. ## Test plan - `cargo check -p ethrex-rpc` passes - `cargo clippy -p ethrex-rpc` passes - `cargo test -p ethrex-rpc` passes
1 parent c3755c1 commit 8bab564

4 files changed

Lines changed: 140 additions & 10 deletions

File tree

‎crates/blockchain/fork_choice.rs‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,9 +76,14 @@ pub async fn apply_fork_choice(
7676
let head_is_canonical = is_canonical(store, head.number, head_hash).await?;
7777

7878
// execution-apis PR 786: the no-reorg skip is only allowed when there is a known
79-
// finalized block and the head references a VALID ancestor of it. Skipping for
79+
// finalized block and the head is at or below it on the canonical chain. Skipping for
8080
// unfinalized canonical ancestors is no longer permitted - those must trigger a reorg.
81+
//
82+
// `head.number < latest` is the strict-ancestor check; equality (head IS the current
83+
// canonical head) falls through to normal FCU so the CL can still build a payload on
84+
// top, mirroring geth's `head == current_head` carve-out in `eth/catalyst/api.go`.
8185
if let Some(stored_finalized) = store.get_finalized_block_number().await?
86+
&& head.number < latest
8287
&& head.number <= stored_finalized
8388
&& head_is_canonical
8489
{

‎crates/networking/rpc/engine/fork_choice.rs‎

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -319,17 +319,14 @@ async fn handle_forkchoice(
319319
Err(forkchoice_error) => {
320320
let forkchoice_response = match forkchoice_error {
321321
InvalidForkChoice::NewHeadAlreadyCanonical => {
322-
// The fork-choice was effectively accepted: head is canonical and
323-
// points to a known block. Treat it like the Ok(head) branch:
324-
// - mark the node synced so eth_syncing reports `false`,
325-
// - return the head header so the caller can build a payload
326-
// when payloadAttributes is non-null (engine API spec).
322+
// execution-apis PR 786: when head references a VALID ancestor of
323+
// the latest known finalized block, return VALID + null payloadId
324+
// and MUST NOT begin a payload build process. We return `None` for
325+
// the head header so the V3/V4 dispatch short-circuits the
326+
// build_payload call.
327327
context.blockchain.set_synced();
328-
let head_block = context
329-
.storage
330-
.get_block_header_by_hash(fork_choice_state.head_block_hash)?;
331328
return Ok((
332-
head_block,
329+
None,
333330
ForkChoiceResponse::from(PayloadStatus::valid_with_hash(
334331
fork_choice_state.head_block_hash,
335332
)),

‎test/tests/rpc/fork_choice_tests.rs‎

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
use std::{fs::File, io::BufReader, path::PathBuf};
2+
3+
use bytes::Bytes;
4+
use ethrex_blockchain::{
5+
Blockchain,
6+
fork_choice::apply_fork_choice,
7+
payload::{BuildPayloadArgs, create_payload},
8+
};
9+
use ethrex_common::{
10+
H160, H256,
11+
types::{Block, BlockHeader, DEFAULT_BUILDER_GAS_CEIL, ELASTICITY_MULTIPLIER},
12+
};
13+
use ethrex_rpc::engine::fork_choice::ForkChoiceUpdatedV3;
14+
use ethrex_rpc::rpc::RpcHandler;
15+
use ethrex_rpc::test_utils::default_context_with_storage;
16+
use ethrex_rpc::utils::RpcRequest;
17+
use ethrex_storage::{EngineType, Store};
18+
19+
fn workspace_root() -> PathBuf {
20+
PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("..")
21+
}
22+
23+
async fn test_store() -> Store {
24+
let file = File::open(workspace_root().join("fixtures/genesis/execution-api.json"))
25+
.expect("Failed to open genesis file");
26+
let reader = BufReader::new(file);
27+
let genesis = serde_json::from_reader(reader).expect("Failed to deserialize genesis file");
28+
let mut store =
29+
Store::new("store.db", EngineType::InMemory).expect("Failed to build DB for testing");
30+
store
31+
.add_initial_state(genesis)
32+
.await
33+
.expect("Failed to add genesis state");
34+
store
35+
}
36+
37+
async fn new_block(store: &Store, parent: &BlockHeader) -> Block {
38+
let args = BuildPayloadArgs {
39+
parent: parent.hash(),
40+
timestamp: parent.timestamp + 12,
41+
fee_recipient: H160::random(),
42+
random: H256::random(),
43+
withdrawals: Some(Vec::new()),
44+
beacon_root: Some(H256::random()),
45+
slot_number: None,
46+
version: 1,
47+
elasticity_multiplier: ELASTICITY_MULTIPLIER,
48+
gas_ceil: DEFAULT_BUILDER_GAS_CEIL,
49+
};
50+
let blockchain = Blockchain::default_with_store(store.clone());
51+
let block = create_payload(&args, store, Bytes::new()).unwrap();
52+
blockchain.build_payload(block).unwrap().payload
53+
}
54+
55+
// Regression test for execution-apis PR #786: when engine_forkchoiceUpdatedV3
56+
// receives a head that is a VALID canonical ancestor of the latest known
57+
// finalized block, the response MUST be {payloadStatus: VALID, payloadId: null}
58+
// and the client MUST NOT begin a payload build process — even when
59+
// payloadAttributes is non-null.
60+
#[tokio::test]
61+
async fn test_fcu_v3_finalized_ancestor_returns_valid_with_null_payload_id() {
62+
let store = test_store().await;
63+
let genesis_header = store.get_block_header(0).unwrap().unwrap();
64+
let blockchain = Blockchain::default_with_store(store.clone());
65+
66+
let block_1 = new_block(&store, &genesis_header).await;
67+
let hash_1 = block_1.hash();
68+
blockchain.add_block(block_1.clone()).unwrap();
69+
70+
let block_2 = new_block(&store, &block_1.header).await;
71+
let hash_2 = block_2.hash();
72+
blockchain.add_block(block_2.clone()).unwrap();
73+
74+
// head = block_2 (latest tip), safe = finalized = block_1.
75+
// After this, block_1 is canonical, finalized number == 1, latest == 2.
76+
apply_fork_choice(&store, hash_2, hash_1, hash_1)
77+
.await
78+
.expect("apply_fork_choice failed");
79+
80+
// Now drive engine_forkchoiceUpdatedV3 with head = block_1 (finalized ancestor)
81+
// and non-null payloadAttributes. The guard in apply_fork_choice should
82+
// return InvalidForkChoice::NewHeadAlreadyCanonical, which the RPC layer
83+
// must translate into VALID + null payloadId without calling build_payload.
84+
let attrs_timestamp = block_1.header.timestamp + 12;
85+
let body = format!(
86+
r#"{{
87+
"jsonrpc": "2.0",
88+
"method": "engine_forkchoiceUpdatedV3",
89+
"params": [
90+
{{
91+
"headBlockHash": "{hash_1:#x}",
92+
"safeBlockHash": "{hash_1:#x}",
93+
"finalizedBlockHash": "{hash_1:#x}"
94+
}},
95+
{{
96+
"timestamp": "{attrs_timestamp:#x}",
97+
"prevRandao": "0x0000000000000000000000000000000000000000000000000000000000000001",
98+
"suggestedFeeRecipient": "0x0000000000000000000000000000000000000000",
99+
"withdrawals": [],
100+
"parentBeaconBlockRoot": "0x0000000000000000000000000000000000000000000000000000000000000002"
101+
}}
102+
],
103+
"id": 1
104+
}}"#
105+
);
106+
let request: RpcRequest = serde_json::from_str(&body).expect("valid FCU request");
107+
108+
let context = default_context_with_storage(store).await;
109+
let response = ForkChoiceUpdatedV3::call(&request, context)
110+
.await
111+
.expect("FCU V3 call should succeed");
112+
113+
assert_eq!(
114+
response["payloadStatus"]["status"], "VALID",
115+
"payloadStatus.status must be VALID per execution-apis PR #786"
116+
);
117+
assert_eq!(
118+
response["payloadStatus"]["latestValidHash"],
119+
format!("{hash_1:#x}"),
120+
"latestValidHash must echo the head hash"
121+
);
122+
assert!(
123+
response["payloadId"].is_null(),
124+
"payloadId must be null when head is a finalized ancestor; got {:?}",
125+
response["payloadId"]
126+
);
127+
}

‎test/tests/rpc/mod.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
mod authrpc_batch_tests;
22
mod block_access_list_tests;
33
mod client_version_tests;
4+
mod fork_choice_tests;
45
mod http_batch_tests;
56
mod subscription_manager_tests;

0 commit comments

Comments
 (0)