erigontech / erigontech/erigon

parallel-exec: validateReadImpl recursive cross-validate probes are inert no-ops

Open
#21,318 2 comments 0 reactions 1 assignee Claimed by @mh0lt View on GitHub
tech debt reduction
Dominant language
Go
Stars
3.6k
Forks
1.5k
Avg merge
1d 16h
Merged PRs (30d)
455

Description

## Context

Raised from a Copilot review comment on #21294 (`execution/state/versionmap.go`, `validateReadImpl`).

## The soft spot

In `validateReadImpl`'s `MVReadResultDone` handling:

```go
if recursive && readVal == nil {
// Synthetic probe — outer entry's own validation covers it.
}
```

The `MVReadResultNone` branch cross-validates an account-field read by recursively calling `validateReadImpl` for `AddressPath` and `SelfDestructPath` with `readVal=nil, recursive=true`. When such a recursive probe finds a `Done` entry, the no-op above makes it return `VersionValid` — so the recursive cross-validate probes are effectively **inert**: they cannot, on their own, invalidate a read on the basis of a prior account create / self-destruct.

The `recursive` gate was added (commit `b23da1a90f`) because the un-gated recursive probe produced false *positives*; a naive un-gate over-fires on every in-block-created account (each gets a `SelfDestructPath=false` Done entry).

## Why this is currently sound (not a live bug)

Account-lifecycle detection does not actually depend on the recursive probe:

- **Recording side** — a per-account-field read of a self-destructed account records an explicit `SelfDestructPath` dependency (rather than a bare `StorageRead`), which is validated directly. (Strengthened by the follow-up to #21294.)
- The `path == AddressPath` branch keeps an explicit, ungated `IncarnationPath` Done check — `IncarnationPath` is the precise create/destruct signal.
- The `MVReadResultDone` SD-staleness check (revival-aware) catches later self-destructs.

Empirically, the SD/account-lifecycle-heavy suites are green (EEST devnet/Amsterdam EIP-7928, stable, `-race`).

## Ask

Revisit `validateReadImpl`'s cross-validation as part of the parallel-exec validator / `skipCheck` cleanup: either make the recursive probes carry meaningful (revival-aware) lifecycle detection, or remove them as dead code and rely explicitly on the recording-side dependencies + `IncarnationPath` + SD-staleness checks. The recursive probe should not silently look like a safety net while being a no-op.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.