erigontech / erigontech/erigon
execution/execmodule: FCU to the already-canonical valid ancestor of a rejected block re-executes the rejected block
- Dominant language
- Go
- Stars
- 3.6k
- Forks
- 1.5k
- Avg merge
- 1d 16h
- Merged PRs (30d)
- 465
Description
Found on `main` (e44109d163) while writing integration tests for #21860 (see PR referencing this issue).
#### What happens
A CL applies devp2p-fetched (never `newPayload`-validated) blocks: valid block 11 and invalid block 12 (its header `gasUsed` doesn't match execution — the post-execution gas check rejects it). `UpdateForkChoice(12)` correctly returns `BadBlock`. But the CL's natural next move per the engine API — set head to the latest valid ancestor, block 11 — fails too:
```
updateForkChoice(block11.Header()) => status=BadBlock
validationErr="updateForkChoice: invalid block, block=12, invalid block, gas used by execution: 21000, in header: 42000, headerNum=12, 7e4bce90…"
```
The FCU to the *valid, already-canonical* block 11 re-executes the rejected block 12 and re-fails with block 12's error. Every subsequent FCU to 11 (or 10) does the same, so a node in this state cannot move its head to any already-canonical ancestor — the classic repeated `Cannot update chain head` symptom. Recovery only works via a *not yet canonical* target (e.g. the valid sibling of block 12), and in the test only after a `ValidateChain` call first.
#### Reproduction
`TestUpdateForkChoiceBadBlockMidBatchThenRecovery` in `execution/execmodule/exec_module_unwind_gap_test.go` pins the working recovery path (untampered sibling + validate + FCU). To reproduce this bug, replace its recovery block with a direct FCU to block 11:
```go
res, err = updateForkChoice(ctx, m.ExecModule, block11.Header())
require.NoError(t, err)
require.Equal(t, execmodule.ExecutionStatusSuccess, res.Status) // fails: BadBlock, re-reporting block 12
```
#### Analysis
In `updateForkChoice` (execution/execmodule/forkchoice.go) the new canonical markers are written *before* forward execution, and the FCU transaction commits even when execution then rejects the tip. After the failed FCU the DB therefore holds: canonical marker 12 → the bad block, and Headers/Bodies/Senders progress at 12.
On the next FCU to block 11, block 11 is *already canonical*, so `newCanonicals` is empty — and the stale-marker cleanup is guarded by it:
```go
if len(newCanonicals) > 0 {
...
if err := rawdb.TruncateCanonicalHash(tx, newCanonicals[0].number+1, false); err != nil {
```
The bad block's canonical marker at height 12 survives, and the pipeline's forward run (bounded by stage progress, not by the FCU height) executes to 12 again and fails. The engine-server bad-block LRU can short-circuit an FCU *to* a known-bad hash, but not an FCU to a good ancestor that merely sits below a stale bad marker — and it doesn't help at all on the devp2p/backward-sync path.
A plausible fix direction: truncate canonical markers above the FCU head (and roll the affected stage progress back) even when `newCanonicals` is empty, mirroring what `setHead` does — or bound the forward run by the FCU target.
Contributor guide
Assessment
This issue has not been assessed yet.