erigontech / erigontech/erigon

execution/state: follow-ups from the typed-vio refactor review (#21536)

Open
#22,259 2 comments 0 reactions 2 assignees Claimed by @mh0lt View on GitHub
performance
Dominant language
Go
Stars
3.6k
Forks
1.5k
Avg merge
1d 16h
Merged PRs (30d)
455

Description

Follow-up work items from the review of #21536. None are blockers there — the retype verified as faithful to main's logic. Distinct from the cache-stack work in #22154 and the main-side bug in #22204.

### Efficiency

- **Write-only `WriteCell` pools** (`versionmap.go`): the per-path cell pools only get cells back via the rare `Delete`/`DeleteAll` exception paths; each block builds a fresh `NewVersionMap` and drops the old map wholesale, so in steady state `putCell`'s `getCell()` hits an empty pool and allocates anyway — per-write pool bookkeeping without the recycling the comment ("a freed cell from block N is recycled into block N+1's first write") promises. Either add a block-end walk that releases cells, or drop the pools and allocate plainly as before.
- **`ApplyVersionedWrites` header materialize + re-probe** (`intra_block_state.go`): the apply loop copies every `WriteHeader` into a slice, sorts it, then re-probes the typed per-path maps (`GetStorage`/`GetBalance`/…) to recover each value it just iterated past. Collect (header, typed value) pairs in the same single map walk before sorting. Runs per finalized tx.
- **`AsBlockAccessList` per-tx `addrs()` map** (`versionedio.go`): `writes.addrs()` allocates a throwaway dedup map per transaction purely to feed the idempotent `ensureAccountState`; iterating `forEachAddr` directly is alloc-free and identical (the BAL is sorted afterwards). `addrs()` itself duplicates `forEachAddr`'s ten per-path loops and can be a 3-line wrapper over it — or deleted once the call site iterates directly.

### Structure

- **Empty-code→delete rule in one place**: the `len(code)==0 → DomainDel(CodeDomain)` rule is hand-rolled at three sites — `Writer.UpdateAccountCode`, `applyVersionedWrites`, and `BlockStateCache.Flush` (`bcOpPutCode`). This exact drift already happened once in the other direction (main had the rule only at the Flush site — the bug #21536 fixes). A single helper over `kv.TemporalPutDel` (`SharedDomains.AsPutDel` adapts the non-`Writer` sites) makes the next tweak land everywhere.
- **Enforce `accounts.Code`'s invariant structurally** (`execution/types/accounts/code.go`): `Hash == Keccak256(Bytes)` is comment-only — both fields are exported, and several sites in `execution/state` build `accounts.Code{...}` literals bypassing `NewCode`, while the parallel commitment calculator (`calc_state.go`) feeds `vw.Val.Hash` straight into the state root. Unexport the fields behind `NewCode` plus a trusted-pair constructor (which must canonicalize the empty-hash variants `EmptyCodeHash`/`ZeroCodeHash`/`NilCodeHash`). Related vestige: `applyVersionedWrites` erases the typed value to `[]byte` and re-keccaks at apply time (`accounts.NewCode(d.code).Hash`) even though every producer already carries a construction-time hash — keep the typed `Code` and use its hash.
- **Generic typed pool accessors**: twenty near-identical `getVW*`/`releaseVW*` (`versionedio.go`) and `getCell*`/`releaseCell*` (`versionmap.go`) pairs. A generic pool wrapper with an optional clear-func (same shape as `vwMapPool[T]`) collapses ~40 declarations and centralizes the easy-to-miss unpin-on-release special cases (Address, Code).
- **`AccountView` is a single-implementation interface** (`versionedio.go`): only `concreteAccountView` exists (the versionMap-cell backing is future work), and it forces the double `r.Val != nil && !r.Val.IsNil()` check at every consumer — forgetting either half at a future call site is a nil-deref or wrong-existence bug. `VersionedRead[*accounts.Account]` gives one nil-check with identical semantics; introduce the interface when the second backing lands.

### Minor

- `SetAccountFieldZero` is behavior-identical to `SetAccountFieldFromAccount(out, addr, path, ver, nil)` — delegate or inline.
- Several tests hand-roll `accounts.Code{Hash: accounts.InternCodeHash(crypto.HashData(x)), Bytes: x}` where `accounts.NewCode(x)` does exactly that (`state_test.go`, `parallel_fixes_test.go`, `exec3_finalize_test.go`).

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.