The previous follow-up note: per-tx + pre-tx + post-tx StateDBs each
have their own stateObjects, so summing CodeLoaded/CodeLoadBytes
over-counts contracts whose code body was fetched by multiple phases.
Fix: snapshot per-StateDB the {address: codeLen} map of contracts whose
s.code is populated, plumb through the existing aggregation pipeline,
dedupe by address in resultHandler/prepareExecResult. The merged map's
size and value-sum become CodeLoaded and CodeLoadBytes respectively,
overriding the per-tx-summed values at the wiring site.
Empirical: a 3-tx block touching the same set of system contracts now
reports code=4, code_bytes=1098 (matches single-tx baseline) instead
of code=12, code_bytes=3294 under the prior over-count.
Per-tx + pre-tx + post-tx StateDBs each have independent stateObjects
caches, so summing their AccountLoaded/StorageLoaded counts over-counts
addresses/slots touched by multiple phase StateDBs (compared to non-BAL
single-StateDB semantics where the cache deduplicates).
Override the read counts at the BAL stats wiring site using two new
helpers on bal.BlockAccessList. The BAL is the canonical block-level
deduplicated access record, so this restores cross-client comparable
"unique accounts/slots touched" semantics.
CodeLoaded/CodeLoadBytes still sum per-tx — the BAL doesn't track code-
fetch events distinctly. Slight over-count remains there, documented.
- Replace reader.go:553 line citation in GetStateStats with the function
name; line numbers rot.
- Note the BAL sum-of-CPU-time semantics on the read-time field group
in ExecuteStats so cross-client consumers don't read total ≤ TotalTime
as an invariant.
Replace the {Account, Storage, Code} time.Duration scalars threaded through
ProcessResultWithMetrics, txExecResult, processBlockPreTx and resultHandler
with a single ReadDurations struct + Add merge primitive. Same shape as
StateCounts. Adds (*StateDB).SnapshotReads() helper at the boundary.
Replace the cached AccountReadTime/StorageReadTime fields (which had a
snapshot-staleness bug fixed in 16e98f5d9 by re-calling Metrics()) with
a live ReadTimes() accessor. Metrics() now only returns commit/hash-phase
timings — it no longer touches atomics. blockchain.go reads atomics
directly via stateTransition.ReadTimes(), eliminating the refresh hack.
Also resolves the I1 fragility: Metrics() returning &s.metrics no longer
involves any writes inside the function, so concurrent callers can't race
on the read-time field updates.
Forward prefetchStateReader.Wait() through *reader.WaitPrefetch and call
it before reading the read-time atomics. Eliminates the edge-case where
prefetcher goroutines outlast block execution + commit. For slow blocks
(the metric's target audience) this is a no-op; for fast blocks it
ensures the metric is complete rather than slightly under.
The first Metrics() call inside calcAndVerifyRoot snapshots accountReadNS
and storageReadNS into the cached AccountReadTime/StorageReadTime fields.
But commitAccount (called from writeBlockWithState's CommitWithUpdate
path) increments storageReadNS *after* that snapshot, so reading
m.StorageReadTime later would silently drop those reads.
Re-call Metrics() before reading the read-time fields so the cache
reflects the post-commit atomics. Other metric fields (AccountUpdate,
AccountCommits, etc.) are written directly to s.metrics elsewhere and
remain untouched by Metrics().
Found via the metric-correctness audit.
The struct is 80 bytes (10 ints) — value semantics matches the type's
"snapshot, safe to pass by value" thesis stated in its doc comment, and
removes three unnecessary &-takings at call sites. No behavior change.
- Add a comment at the code-mutation gate explaining the deliberate
len(code) > 0 (vs code != nil) match against non-BAL semantics; in
devnet-3 BAL access lists, an empty []byte is non-nil but encodes
"no code install".
- Remove BALStateTransitionMetrics.OriginStorageLoadTime: declared but
never assigned anywhere in the tree. The actual state-transition
read time is captured by AccountReadTime/StorageReadTime added in
the prior commit.
Mirrors the nil-check already used in buildSlowBlockLog. The previous
unguarded access was safe today only because parallel_state_processor
short-circuits on error before the metrics path is reached, but the API
contract was fragile — a future caller could reach reportBALMetrics
without an established balTransitionStats and panic.
Without this, the inline interface assertion in processBlockWithAccessList
silently fell through (the prefetchReader returned by ReaderEIP7928 is a
*reader wrapper, not the inner *prefetchStateReader), causing the
prefetcher contribution to state_read_ms to drop to zero in production.
Mirrors the existing GetStateStats forwarding pattern. Adds a regression
test that asserts *reader exposes PrefetchReadTimes via the BAL chain,
plus a fallback test for non-prefetch readers.
Adds the StateCounts type that the BAL slow-block work depends on:
- core/state/state_counts.go: 10-field plain-int snapshot type with
Add merge primitive; isolates the live atomic mutation surface from
the value-typed aggregation pipeline.
- core/state/statedb.go: SnapshotCounts() method that converts the
StateDB's atomic counters to a plain StateCounts at the boundary.
- core/blockchain_stats.go: ExecuteStats embeds state.StateCounts;
adds ExecWall/PostProcess/Prefetch BAL extension fields, the
slowBlockBAL JSON struct + BAL field on slowBlockLog, and extracts
buildSlowBlockLog as a pure helper for direct testing.
Without this commit the bal-devnet-3 branch as committed in subsequent
commits would not build for a fresh clone (state.StateCounts undefined).
The BAL reader tracker captures access list reads at the reader level.
When statedb has an account cached the BAL tracker is not informed of
the access. This is ok during the lifetime of a transaction because you
only need to record the access the first time. It is also ok during the
lifetime of a block because BAL reads are block-level (same as statedb
caches).
Where I think the issue can rise is in the miner. Namely when building a
block, if the miner picks up a tx which fails, it drops it and picks up
another tx to include. There might be some edge case here where the
failed tx which is not included poisons the cache and a future block
which is included omits an account because it wasn't aware of the
access.
To check whether a transaction can be applied, we validate that
`blockGasLimit > txGasLimit + (cumulativeRegularGasUsed +
cumulativeStateGasUsed)`. However, the check should only be applied to
the bottleneck resource, i.e. `blockGasLimit >
max(txRegularGasUsed+cumulativeRegularGasUsed, txStateGasUsed+
cumulativeStateGasUsed)`.
The changes here break multiple tests. I am trying to determine why.
---------
Co-authored-by: qu0b <stefan@starflinger.eu>
* add method on StateReaderTracker to clear the accumulated reads
* don't factor the BAL size into the payload size during construction in the miner
* simplify miner code for constructing payloads-with-BALs via the use of aformentioned StateReaderTracker clear method
* clean up the configuration of the BAL execution mode based on the preset flag specified
CopyHeader copies all pointer-typed header fields (WithdrawalsHash,
RequestsHash, SlotNumber, etc.) but was missing the deep copy for
BlockAccessListHash added by EIP-7928. This caused the BAL hash
to be silently shared between the original and the copy, leading
to potential data races and incorrect nil-checks on copied headers.
