execution/execmodule: allow reorgs on canonical chain up to finalised hash#20825
execution/execmodule: allow reorgs on canonical chain up to finalised hash#20825
Conversation
yperbasis
left a comment
There was a problem hiding this comment.
Major
- Nil-pointer panic when finalizedHash is unknown to the EL — execution/execmodule/forkchoice.go:298-302 blockNum, err := e.blockReader.HeaderNumber(ctx, tx, finalizedHash) if err != nil { return sendForkchoiceErrorWithoutWaiting(...) } finalisedBlockNum = *blockNum
BlockReader.HeaderNumber (db/snapshotsync/freezeblocks/block_reader.go:572) returns (nil, nil) when the block is absent — the existing FCU code at forkchoice.go:545 explicitly guards
with if headNumber != nil. Here the bare *blockNum will panic if the CL ever sends a finalized hash the EL hasn't seen via newPayload. Recommend either treating blockNum == nil as the
"no known finalized" branch, or returning InvalidForkchoiceState.
Minor
2. Spec-compliance gap when finalizedHash is zero — same block
The spec wording is "if there is a known finalizedBlockHash …". The implementation falls back to finishProgressBefore (execution-stage progress) when finalizedHash == 0x0, which is a
different proxy than "ancestor of finalized". Pragmatic for fresh devnets where finality hasn't kicked in yet, but worth either:
- adding a comment explaining why this fallback is used, or
- not allowing the no-op skip at all when finalized is unset (strictly spec-conforming, slightly less efficient at startup).
The current inline comment "is at or behind last finalized hash, treat as no-op" is also a little misleading on the fallback branch — the fallback is "behind execution progress", not
"behind finalized".
- Stale log message — forkchoice.go:369
e.logger.Warn("reorg requested too low, capping to the minimum unwindable block", ...)
return sendForkchoiceResultWithoutWaiting(outcomeCh, ForkChoiceResult{Status: ExecutionStatusReorgTooDeep}, false)
The warning still says "capping to the minimum unwindable block", but the code no longer caps — it returns ReorgTooDeep. I confirmed via the test run that the test trace still shows
this misleading WARN line. Suggest something like "reorg target below minimum unwindable block, returning ReorgTooDeep".
- Test asserts Error, not the specific code — engine_api_reorg_test.go:371
TestFcuReturnsReorgTooDeepCode38006 ends with require.Error(t, err) — given the test name explicitly calls out code 38006, it would be stronger to assert on the message/code, e.g.
require.ErrorContains(t, err, "Too deep reorg") or unwrap the JSON-RPC error and check Code == -38006. Right now any error along the way would make the test pass.
- Proto enum vs native enum drift — node/interfaces/execution/execution.proto:10
ExecutionStatusReorgTooDeep = 6 is added to the native enum, but the proto enum still stops at Busy = 5. The native enum's comment says "The numeric values intentionally match the
proto constants for easy conversion." That invariant is now broken. The proto isn't actually used for status conversion in this code path today, so it's not a runtime issue, but for
consistency consider either adding ReorgTooDeep = 6; to the proto or dropping the comment.
- convertGrpcStatusToEngineStatus doesn't cover the new status — engine_server.go:999
The early-return at engine_server.go:1135-1137 means we never reach the switch with ExecutionStatusReorgTooDeep, so the panic("unhandled execution status") is unreachable today. This
matches the existing pattern for InvalidForkchoice, so it's consistent — just flagging since a future caller of convertGrpcStatusToEngineStatus could trip it.
implements ethereum/execution-apis#786
which is needed for glamsterdam-devnet-0