Skip to content

perf(bal): verify-only fast path — skip end-of-block encode + Keccak#11659

Merged
LukaszRozmej merged 245 commits into
masterfrom
perf/bal-verify-only-source
May 19, 2026
Merged

perf(bal): verify-only fast path — skip end-of-block encode + Keccak#11659
LukaszRozmej merged 245 commits into
masterfrom
perf/bal-verify-only-source

Conversation

@LukaszRozmej

Copy link
Copy Markdown
Member

Stacks on #11573 (bal-devnet-7-benchmarks). Idea 9 from my prioritization comment on #11573.

Changes

On the parallel validation path, replaces the canonical-bytes hash compare in SetBlockAccessList (encode the generated BAL to ~70 KB of RLP, Keccak that, compare to header.BlockAccessListHash) with a structural-equivalence check against the suggested BAL.

This mirrors paradigmxyz/reth#24297 — Reth's BAL validation does rebuilt_bal.as_slice() != decoded_bal.as_bal().as_slice() (structural slice compare) and only computes the hash for the error message. EIP-7928 says implementations MAY validate by re-encoding + hashing — it does not mandate it.

How each mismatch class is now caught without re-encoding

Mismatch class Now covered by
Lane values: balance / nonce / code / storage at each tx index column-index per-tx ChangesEqual (existing)
Lane ordering within / across tx indices column-index per-tx ChangesEqual (existing)
Generated has read-only account suggested doesn't list _hasGeneratedRequiredReadAccountMismatch → slow-walk fallback (existing)
Suggested has surplus account (including spec-form-empty entries) NEW account-set count + per-account lookup
storage_reads contents differ per account (count matches but values don't) NEW sorted linear walk
Post-execution slot's lane data NEW explicit ValidateBlockAccessList(block, lastIndex) call (was only catch by the hash)

Implementation notes

  • BlockProcessor.PrepareBlockForProcessing now copies BlockAccessListHash from the suggested header (one new line). The verify-only fast path leaves the processed header's hash as the wire value; SetBlockAccessList still rewrites it on the building path and on the non-verify validation path.
  • ValidateStructuralEquivalence is a static helper on BlockAccessListManager — iterates suggested.AccountChanges, looks up each in generated, compares StorageReads arrays.
  • Verify-only mode kicks in when ParallelExecutionEnabled — same gating as the column-index validator that already runs incrementally per-tx.

Performance

Per-block:

Time (estimated)
Skipped: RLP encode of generated BAL (~70 KB output, walks all collections) ~3–10 ms
Skipped: Keccak compute over the encoded bytes ~0.5–1 ms
Added: post-execution-row column-index check ~negligible
Added: structural-equivalence walk (~1k dict lookups + ~10k sorted comparisons) ~1–3 ms
Net ~4–12 ms / block

On a fast-sync workload processing many blocks per second, this is real.

Types of changes

  • Optimization

Testing

Requires testing

  • Yes

If yes, did you write tests?

  • Yes — NewPayloadV5_rejects_invalid_BAL_after_processing updated to accept either the old InvalidBlockLevelAccessListHash error or the new, more specific InvalidBlockLevelAccessList: Account-set size mismatch / ... storage_reads ... errors. The surplus-empty-entry attack the test exercises is still rejected, just with a more diagnostic error message.

Notes on testing

Verified clean against:

Project Total Passed Skipped Failed
Nethermind.Core.Test 5551 5446 105 0
Nethermind.State.Test 792 789 3 0
Nethermind.Consensus.Test 64 64 0 0
Nethermind.Evm.Test 3662 3652 10 0
Nethermind.Blockchain.Test 1497 1482 15 0
Nethermind.Merge.Plugin.Test 1008 1006 2 0
Nethermind.BalRecorder.Test 17 17 0 0

Full solution build clean; dotnet format --verify-no-changes clean.

Documentation

Requires documentation update

  • No

flcl42 and others added 30 commits April 30, 2026 13:49
)

* fix(bal): align EIP-7928 BlockAccessIndex with uint32 spec, add validation

Widens BlockAccessIndex from uint16 to uint32 per EIP-7928 (commit 645099785a)
and geth bal-devnet-4. Hardens the BAL decoder so truncated/malformed wire
bytes produce a clean RlpException at engine_newPayloadV5 instead of crashing.
Adds the missing validation rules geth bal-devnet-4 enforces: empty
storage_changes per slot is rejected, and BlockAccessIndex is bounded by
[0, txCount + 1].

Decoder primitives:
- New Rlp.ValueDecoderContext.DecodeUInt() / RlpStream.Encode(uint) /
  Rlp.LengthOf(uint) helpers.
- Rlp.Decode<T> and Rlp.DecodeArrayPool<T> wrap IndexOutOfRangeException /
  ArgumentOutOfRangeException as RlpException so any truncated-RLP primitive
  read surfaces consistently.
- BlockAccessListDecoder rejects an empty AccountChanges entry (0xc0 inside
  the outer list) and SlotChangesDecoder rejects an empty StorageChange list
  for a slot — geth bal-devnet-4 "empty storage writes" parity.

Type widening:
- IIndexedChange.Index, BalanceChange/NonceChange/CodeChange/StorageChange.Index,
  BlockAccessList.Index and BlockAccessIndex on the Change journal record all
  become uint. SortedList<int, T> keys flip to SortedList<uint, T>; ushort
  query parameters on AccountChanges become uint.

Prestate sentinel preservation:
- Replaces the legacy -1 int sentinel with Eip7928Constants.PrestateIndex
  (uint.MaxValue). Adds PrestateAwareIndexComparer that orders the sentinel
  before all real indices, restoring the iteration semantics that
  AccountChanges.GetBalance/GetNonce/GetCode/AccountExists and
  ApplyStateChanges' [^1].Index check rely on.
- Decoder-built SortedLists also use the prestate-aware comparer so that
  LoadPreStateToSuggestedBlockAccessList grafting prestate onto the suggested
  BAL after decode keeps it sorted first.
- Loop predicates that compare change.Key directly against blockAccessIndex
  explicitly skip PrestateIndex so the raw uint comparison doesn't trigger
  on the huge sentinel value.

Validation:
- BlockValidator gains ValidateBlockLevelAccessListIndexBounds enforcing
  index <= txCount + 1 (mirrors geth's index < txCount + 2 check) with a new
  BlockLevelAccessListIndexOutOfRange error message.

Tests:
- New PrestateAwareIndexComparerTests, AccountChangesPrestateTests covering
  the comparer and prestate-fallback iteration semantics.
- BlockAccessListDecoderTests adds: empty-bytes / truncated-outer-list /
  inner-empty-list throw RlpException; empty storage_changes per slot throw;
  decoded SlotChanges accepts a later prestate graft as first; BalanceChange
  round-trips for indices 0x10_0000 and uint.MaxValue.
- BlockValidatorTests adds tx-index bound cases (0, 1 valid; 2,
  uint.MaxValue rejected for a 0-tx block).
- ExecutionPayloadV4Tests covers the engine-API decoding-error path for
  malformed BAL bytes.

* style: drop unused usings flagged by IDE0005

CI surfaced four IDE0005 warnings (treated as errors):
- BlockAccessListManager: drop `using Nethermind.Crypto;` —
  Keccak.OfAnEmptySequenceRlp comes from Nethermind.Core.Crypto, already imported.
- PrestateAwareIndexComparerTests / AccountChangesPrestateTests: drop
  `using Nethermind.Core;` — Eip7928Constants resolves via the test's parent
  namespace Nethermind.Core.Test.BlockAccessLists.
- Eip8037Tests: drop `using System;` — no System.* type referenced directly.

* fix(bal): record system pre/post-block SSTOREs as reads per EIP-7928 v5.7.0

EIP-7928 v5.7.0 specifies that SSTOREs performed during system pre-block calls
(EIP-2935 BlockHashHistory, EIP-4788 BeaconRootContract) and post-block calls
(EIP-7002 withdrawal requests, EIP-7251 consolidation requests) are recorded
in the BAL as storage reads on the touched slot — not as storage changes with
post-values. Same-value writes are skipped entirely. Without this, nethermind
generated a BAL whose Keccak256 differs from what eels-built fixtures expect,
so the BlockAccessListHash check fails for every Amsterdam block that touches
a system contract slot (most pyspec tests).

IWorldState gains an opt-in scope:

  IDisposable? BeginSystemPreBlockScope()

TracedAccessWorldState implements it via an int depth counter. While depth > 0,
Set(storageCell, value) reclassifies the recording: AddStorageRead when the
slot value would change, no-op when unchanged. The state mutation still
applies via the inner world state.

BlockAccessListManager wraps the three system contract entry points with the
scope: StoreBeaconRoot (EIP-4788 system tx), ApplyBlockhashStateChanges
(EIP-2935 fast-path Set), and ProcessExecutionRequests (EIP-7002 / EIP-7251
post-block system txs).

Parallel-mode state application:

In parallel processing, non-system slots wrap stateProvider with
BlockAccessListBasedWorldState whose Set is a no-op — actual state mutation
relies on ApplyStateChanges replaying the suggested BAL's storage_changes.
With the spec-correct BAL containing reads instead of changes for the system
slots, ApplyStateChanges has nothing to replay for them, so the canonical
state would diverge.

TxProcessorWithWorldState gains an `isSystemSlot` parameter. The
ParallelTxProcessorWithWorldStateManager passes `isSystemSlot: i == 0 || i ==
_len - 1` (pre-execution and post-execution slots). For those slots, the
TracedAccessWorldState wraps stateProvider directly, so system pre/post-block
SSTOREs mutate the canonical state regardless of BAL recording. Tx slots
(1..n) keep the BAL-backed wrapping unchanged.

Sequential pyspec tests (which is what the Ethereum.Blockchain.Pyspec.Test
suite runs) are unaffected by the parallel slot change but benefit from the
BAL recording fix; the BlockAccessListHash check now passes for blocks that
previously failed solely on system pre-block storage encoding.

Note: a residual InvalidStateRoot mismatch remains on a subset of Amsterdam
pyspec tests (~70/360 ecrecover_weird_v + a similar slice of stMemoryStress).
These were previously masked by the BAL hash error firing first. The
remaining state divergence appears unrelated to BAL recording and is left
for follow-up.

* test(jsonrpc): update Eth_get_block_access_list_by_* fixtures for EIP-7928 v5.7.0

The Eth_get_block_access_list_by_hash and _by_number tests had hardcoded the
pre-v5.7.0 shape, recording the EIP-2935 BlockHashHistory system pre-block
SSTORE as a storageChanges entry with the post-value. v5.7.0 records system
pre-block SSTOREs as storageReads (slot key only).

Updated both expected JSON strings to match the new spec-compliant output.

* Revert "fix(bal): record system pre/post-block SSTOREs as reads per EIP-7928 v5.7.0"

This reverts commit 364f294.

* Revert "test(jsonrpc): update Eth_get_block_access_list_by_* fixtures for EIP-7928 v5.7.0"

This reverts commit 937ca5d.

* review: address PR #11362 feedback

- Rlp.DecodeArrayPool<T>: dispose partially-allocated ArrayPoolList<T>
  before wrapping IndexOutOfRangeException/ArgumentOutOfRangeException
  as RlpException so the rented buffer is returned to the pool.
- Rlp.ValueDecoderContext.DecodeUInt(): collapse case 0 to a single
  `return RlpHelpers.ThrowNonCanonicalInteger(Position)` (DoesNotReturn,
  uint) to match DecodeInt and drop the dead `return default`.
- BlockAccessListManager.GetPostExecution(): use uint.MaxValue literal
  to match the uint? balIndex parameter.
- AccountChanges.SlotChangesAtIndex: build the returned SortedList with
  PrestateAwareIndexComparer.Instance so a later prestate graft sorts
  first, matching the rest of the codebase.
- PrestateAwareIndexComparer xmldoc: clarify that decoded BALs also use
  this comparer (so later prestate grafting preserves order).

* test(pyspec): skip EELS bal@v5.7.0 ported_static fixtures with legacy state-test conversion bug

EELS's `from_state_test` conversion for ported_static tests omits the
EIP-2935 / EIP-4788 system pre-block SSTORE entries from the suggested
BAL, while a real client (and Nethermind) executes them — so every such
fixture's BlockAccessListHash diverges from what we compute. 91 such
tests were the entire residual pyspec failure set on the bal-devnet-4a
branch.

Detect the conversion via the legacy difficulty value 0x20000 baked
into the post-merge mixHash field — real prevRandao would never be
exactly 0x...020000 — and Assert.Ignore those tests.

Track upstream EELS fix; remove the guard once bal@>v5.7.0 ships with
the system pre-block SSTOREs included in the BAL.

* fix(pyspec test): drop ?-annotation in non-nullable file

CS8632: PyspecTestFixture.cs is not under `#nullable enable`, so the
`string?` introduced in 6fb6527 broke the build of every pyspec
job. `string` works the same here — the value already gets a null
check on the next line.

* test(pyspec): also skip blockchain_test_engine_from_state_test variant

The first guard only walked test.Blocks, which is null for engine
fixtures. Engine fixtures keep the same legacy 0x...020000 sentinel,
just on the JSON `prevRandao` field of the engine_newPayload params.
Walk EngineNewPayloads too.

* Update tests

* fix(eip-8037): pin cost_per_state_byte at static 1174 for bal-devnet-4

bal-devnet-4 keeps cost_per_state_byte static at 1174 (carried over from
bal-devnet-3), removing the per-block-gas-limit scaling formula that an
earlier draft of EIP-8037 specified. snøbal-devnet-4 fixtures encode the
same gas usage (63574) at 1M / 5M / 10M / 30M block gas limits, confirming
the value is now invariant across blockGasLimit.

Reduce CalculateCostPerStateByte to a direct return of CostPerStateByte
and drop the dynamic-quantization helpers and BitOperations dependency.
The blockGasLimit parameter is retained on call sites in case a future
devnet revisits per-block scaling. Update the EIP-8037 unit test to pin
the static behaviour rather than asserting quantized values.
- Replace SortedDictionary with Dictionary in BlockAccessList for O(1)
  account lookups on the EVM hot path (was O(log n) with 20-byte span
  comparisons). Sorted enumeration preserved for encoding/validation.
- Merge TryGetDelegation + GetCachedCodeInfo into a single call in
  InstructionCall, eliminating a redundant GetCodeHash per CALL opcode.
- Inline IsDeadAccount in EXTCODEHASH to avoid a second GetCodeHash
  call for the same address.
commit 3a3078f
Author: Marc Harvey-Hill <10379486+Marchhill@users.noreply.github.com>
Date:   Tue Apr 28 18:23:03 2026 +0200

    delete old tests

commit 95de888
Merge: 244c2c6 31a673a
Author: MarekM25 <marekm2504@gmail.com>
Date:   Tue Apr 28 18:08:12 2026 +0200

    Merge branch 'master' into engine-api-glamsterdam-cleanup

commit 244c2c6
Author: MarekM25 <marekm2504@gmail.com>
Date:   Tue Apr 28 18:05:32 2026 +0200

    fix lint

commit 9194b8f
Author: MarekM25 <marekm2504@gmail.com>
Date:   Tue Apr 28 18:04:19 2026 +0200

    remove tests

commit eedfbb7
Author: MarekM25 <marekm2504@gmail.com>
Date:   Tue Apr 28 17:57:46 2026 +0200

    revert formatting changes and zero hash stuff

commit dc71aa5
Author: MarekM25 <marekm2504@gmail.com>
Date:   Tue Apr 28 17:38:42 2026 +0200

    cleanups

commit ec2fce3
Author: MarekM25 <marekm2504@gmail.com>
Date:   Tue Apr 28 17:33:11 2026 +0200

    fixing

commit 90a1da8
Author: MarekM25 <marekm2504@gmail.com>
Date:   Tue Apr 28 17:15:58 2026 +0200

    remove test

commit 4451656
Author: MarekM25 <marekm2504@gmail.com>
Date:   Tue Apr 28 17:12:32 2026 +0200

    Cleanup mess

commit 2425748
Merge: 4a90460 a36154c
Author: Stavros Vlachakis <89769224+svlachakis@users.noreply.github.com>
Date:   Mon Apr 27 14:33:36 2026 +0300

    Merge branch 'master' into engine-api-glamsterdam

commit 4a90460
Merge: 3549768 18d60a4
Author: Stavros Vlachakis <89769224+svlachakis@users.noreply.github.com>
Date:   Mon Apr 27 14:08:25 2026 +0300

    Merge branch 'master' into engine-api-glamsterdam

commit 3549768
Merge: 30fcc36 b71c352
Author: Stavros Vlachakis <89769224+svlachakis@users.noreply.github.com>
Date:   Mon Apr 27 14:07:39 2026 +0300

    Merge branch 'master' into engine-api-glamsterdam

commit 30fcc36
Merge: 540e4a2 ed6a354
Author: Stavros Vlachakis <89769224+svlachakis@users.noreply.github.com>
Date:   Mon Apr 27 13:09:48 2026 +0300

    Merge branch 'master' into engine-api-glamsterdam

commit 540e4a2
Author: stavrosvl7 <stavrosvl7@gmail.com>
Date:   Sun Apr 26 04:52:49 2026 +0300

    minor fixes

commit 010547b
Author: stavrosvl7 <stavrosvl7@gmail.com>
Date:   Sun Apr 26 03:22:39 2026 +0300

    test fixes

commit 6fcc136
Author: stavrosvl7 <stavrosvl7@gmail.com>
Date:   Sun Apr 26 02:11:12 2026 +0300

    fixes

commit 0bccb06
Author: stavrosvl7 <stavrosvl7@gmail.com>
Date:   Sun Apr 26 01:52:02 2026 +0300

    test fix

commit 830c578
Author: stavrosvl7 <stavrosvl7@gmail.com>
Date:   Sun Apr 26 01:38:55 2026 +0300

    fix tests

commit 1f5fd2c
Author: stavrosvl7 <stavrosvl7@gmail.com>
Date:   Sun Apr 26 01:35:17 2026 +0300

    claude review again

commit d87491f
Author: stavrosvl7 <stavrosvl7@gmail.com>
Date:   Sun Apr 26 00:57:57 2026 +0300

    fixes

commit dd431e2
Author: stavrosvl7 <stavrosvl7@gmail.com>
Date:   Sun Apr 26 00:51:52 2026 +0300

    fix tests

commit 71dc99f
Author: stavrosvl7 <stavrosvl7@gmail.com>
Date:   Sat Apr 25 22:40:50 2026 +0300

    fixes

commit 9c03d12
Author: stavrosvl7 <stavrosvl7@gmail.com>
Date:   Sat Apr 25 22:32:58 2026 +0300

    update per 786

commit a4c4f3c
Merge: fc49e7e 9cba44c
Author: stavrosvl7 <stavrosvl7@gmail.com>
Date:   Sat Apr 25 21:38:25 2026 +0300

    Merge remote-tracking branch 'origin/master' into engine-api-glamsterdam

commit fc49e7e
Merge: bca0c63 42c551f
Author: Stavros Vlachakis <89769224+svlachakis@users.noreply.github.com>
Date:   Mon Apr 20 16:14:33 2026 +0300

    Merge branch 'master' into engine-api-glamsterdam

commit bca0c63
Author: stavrosvl7 <stavrosvl7@gmail.com>
Date:   Mon Apr 20 16:14:10 2026 +0300

    remove unused import

commit 3d2c973
Author: stavrosvl7 <stavrosvl7@gmail.com>
Date:   Mon Apr 20 16:09:17 2026 +0300

    fix taiko tests

commit b30ef5c
Author: stavrosvl7 <stavrosvl7@gmail.com>
Date:   Mon Apr 20 15:34:35 2026 +0300

    fix tests

commit 6f131cd
Merge: a8c884a 0fe41f4
Author: Stavros Vlachakis <89769224+svlachakis@users.noreply.github.com>
Date:   Mon Apr 20 14:55:46 2026 +0300

    Merge branch 'master' into engine-api-glamsterdam

commit a8c884a
Author: stavrosvl7 <stavrosvl7@gmail.com>
Date:   Mon Apr 20 14:54:40 2026 +0300

    conflicts fix

commit 96eb912
Merge: 82a1f70 425a2a3
Author: stavrosvl7 <stavrosvl7@gmail.com>
Date:   Mon Apr 20 14:52:33 2026 +0300

    Merge remote-tracking branch 'origin/master' into engine-api-glamsterdam

    # Conflicts:
    #	src/Nethermind/Nethermind.Merge.Plugin/BlockTreeExtensions.cs
    #	src/Nethermind/Nethermind.Merge.Plugin/Handlers/ForkchoiceUpdatedHandler.cs
    #	src/Nethermind/Nethermind.Taiko/Rpc/TaikoForkchoiceUpdatedHandler.cs

commit 82a1f70
Merge: 7b6133f 436c65b
Author: stavrosvl7 <stavrosvl7@gmail.com>
Date:   Mon Apr 20 13:16:55 2026 +0300

    Merge remote-tracking branch 'origin/master' into engine-api-glamsterdam

commit 7b6133f
Merge: 2f068ec 02c2026
Author: Stavros Vlachakis <89769224+svlachakis@users.noreply.github.com>
Date:   Thu Apr 16 14:49:46 2026 +0300

    Merge branch 'master' into engine-api-glamsterdam

commit 2f068ec
Merge: bdd7ee5 5464c8b
Author: Stavros Vlachakis <89769224+svlachakis@users.noreply.github.com>
Date:   Thu Apr 16 14:10:02 2026 +0300

    Merge branch 'master' into engine-api-glamsterdam

commit bdd7ee5
Merge: f09cb41 bfaaeb0
Author: Stavros Vlachakis <89769224+svlachakis@users.noreply.github.com>
Date:   Wed Apr 15 16:45:19 2026 +0300

    Merge branch 'master' into engine-api-glamsterdam

commit f09cb41
Merge: 370acdf cf03d18
Author: stavrosvl7 <stavrosvl7@gmail.com>
Date:   Wed Apr 15 16:24:17 2026 +0300

    Merge remote-tracking branch 'origin/master' into engine-api-glamsterdam

commit 370acdf
Merge: d1d0370 6cd0ea4
Author: Stavros Vlachakis <89769224+svlachakis@users.noreply.github.com>
Date:   Mon Apr 13 14:28:21 2026 +0300

    Merge branch 'master' into engine-api-glamsterdam

commit d1d0370
Author: stavrosvl7 <stavrosvl7@gmail.com>
Date:   Fri Apr 10 17:30:54 2026 +0300

    more fixes

commit c99d6cc
Merge: 4d0f215 a52cb90
Author: Stavros Vlachakis <89769224+svlachakis@users.noreply.github.com>
Date:   Fri Apr 10 17:24:06 2026 +0300

    Merge branch 'master' into engine-api-glamsterdam

commit 4d0f215
Author: stavrosvl7 <stavrosvl7@gmail.com>
Date:   Fri Apr 10 17:23:42 2026 +0300

    claude review

commit cb6defe
Merge: af63142 0057bd8
Author: Stavros Vlachakis <89769224+svlachakis@users.noreply.github.com>
Date:   Fri Apr 10 12:34:37 2026 +0300

    Merge branch 'master' into engine-api-glamsterdam

commit af63142
Merge: 1b338f3 2f24891
Author: Stavros Vlachakis <89769224+svlachakis@users.noreply.github.com>
Date:   Fri Apr 10 12:08:36 2026 +0300

    Merge branch 'master' into engine-api-glamsterdam

commit 1b338f3
Author: stavrosvl7 <stavrosvl7@gmail.com>
Date:   Fri Apr 10 12:08:13 2026 +0300

    claude review

commit 619259b
Author: stavrosvl7 <stavrosvl7@gmail.com>
Date:   Thu Apr 9 18:32:59 2026 +0300

    taiko fix

commit d067793
Author: stavrosvl7 <stavrosvl7@gmail.com>
Date:   Thu Apr 9 18:19:14 2026 +0300

    cleaner design

commit 1e27be1
Author: stavrosvl7 <stavrosvl7@gmail.com>
Date:   Thu Apr 9 18:12:04 2026 +0300

    improve tests matching the specs

commit a77e182
Author: stavrosvl7 <stavrosvl7@gmail.com>
Date:   Thu Apr 9 18:04:58 2026 +0300

    improve test

commit 4ba069c
Author: stavrosvl7 <stavrosvl7@gmail.com>
Date:   Thu Apr 9 18:01:59 2026 +0300

    fix comment

commit fe6f9b0
Author: stavrosvl7 <stavrosvl7@gmail.com>
Date:   Thu Apr 9 17:53:57 2026 +0300

    engine api changes as per ethereum/execution-apis#770
                              ethereum/execution-apis#760
…halt

Top-level REVERT preserves gas_left, so the spilled portion of state_gas_used
(originally drawn from gas_left) is still in the user's pocket and must be
refunded to the reservoir alongside the reservoir-funded portion. Top-level
halt burns gas_left, so the spilled portion was paid out of gas the user can
no longer reclaim — only the reservoir-funded portion is restorable.

Splits RefundRevertedTopLevelStateGas into a revert path (full refund) and a
new RefundHaltedTopLevelStateGas (reservoir-only, spill discarded) and wires
the halt error sites to the latter.
The pyspec fixture archive ships three uncovered directories: a stray state-
tests transition fork, plus two test types we never wired in. Adds:

- CancunToPragueAtTime15kStateTests — fills a one-class gap in Tests.cs
- PyspecSyncBlockchainTestFixture + AmsterdamSyncBlockchainTests,
  OsakaSyncBlockchainTests — runs blockchain_tests_sync fixtures through the
  engine harness; the additional syncPayload field is left for a follow-up
- TestType.Transaction + TransactionTest/Json/Base + ConvertTransactionTests
  + PyspecTransactionTestFixture — decodes raw txbytes via Rlp.Decode and
  matches expected EEST exception tokens (TYPE_4_*) against the validator's
  ValidationResult.Error or RLP decode message; covers AmsterdamTxTests,
  OsakaTxTests, PragueTxTests fork directories

Bumps FlatDB pyspec chunking from 4 to 16 to match the regular workflow —
~860 tests/shard instead of ~3,437/shard, well under the 256/matrix cap and
the 20-minute job timeout.

@benaadams benaadams left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@claude What if the supplied BAL doesn't agree with the hash in header; how will that be caught?

LukaszRozmej and others added 2 commits May 18, 2026 23:28
* Comments trimmed in SetBlockAccessList / ValidateStructuralEquivalence.
* ReadOnlyBlockAccessList now exposes AccountChangesAsSpan (keeps the
  decoder's orderedAccounts array around) so the structural-equivalence
  walk skips the dictionary's enumerator.
* Two new unit tests in BlockProcessorTests covering the verify-only paths
  the existing engine-API tests don't exercise explicitly:
  - storage_reads content mismatch (count matches but values differ) —
    this is the one mismatch class that nothing else catches: the
    column-index check only tracks read counts via the gas-budget check,
    and the canonical-bytes hash compare used to catch it implicitly.
  - account presence mismatch with matching count (different addresses).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two new tests merged into one [TestCaseSource]-driven method; the case
description lives on the SetName. Comments on _verifyOnly and the
BlockAccessListHash carry-forward in PrepareBlockForProcessing trimmed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@LukaszRozmej LukaszRozmej requested a review from Marchhill May 18, 2026 21:34
@LukaszRozmej

Copy link
Copy Markdown
Member Author

@claude re-review

@claude

claude Bot commented May 18, 2026

Copy link
Copy Markdown
Contributor

Claude finished @LukaszRozmej's task in 6m 44s —— View job


Re-review

  • Read all relevant validation paths
  • Answer benaadams's question about BAL hash consistency
  • Re-assess previous findings
  • Post findings

Answering @benaadams's question

What if the supplied BAL doesn't agree with the hash in header; how will that be caught?

It is caught before ProcessOne is called — via two mechanisms:

1. ExecutionPayloadV4.TryGetBlock() (line 51)

block.Header.BlockAccessListHash = ... new(ValueKeccak.Compute(BlockAccessList).Bytes);

This recomputes BlockAccessListHash from the actual wire BAL bytes rather than trusting the wire hash field. So BlockAccessListHash is always hash(actual_BAL_bytes) by construction, before any processing begins.

2. NewPayloadHandler.Handle() (line 129)

if (!HeaderValidator.ValidateHash(block!.Header, out Hash256 actualHash))
    return NewPayloadV1Result.Invalid(null, $"Invalid block hash...");

ValidateHash computes CalculateHash() over all header fields (which now includes the newly computed BlockAccessListHash) and checks it matches the wire blockHash field. Since the entire header is hashed against the wire commitment, any BAL whose hash differs from what the block producer committed to will produce a different BlockAccessListHash, make CalculateHash() diverge from the wire blockHash, and be rejected here before block processing starts.

On top of this, ValidateSuggestedBlockValidateBlockLevelAccessListValidateBlockLevelAccessListHashMatches (called at line 193 for non-orphaned blocks) also explicitly checks hash(EncodedBlockAccessList) == header.BlockAccessListHash, though it is trivially guaranteed by construction from step 1.

The verify-only path is therefore sound: by the time _verifyOnly = true is relevant, hash(suggested_BAL) == header.BlockAccessListHash is already a proven invariant. ValidateStructuralEquivalence then proves generated ≡ suggested, completing the equivalence to the original hash(generated) == header.BlockAccessListHash check.


Re-assessment of previous findings

Still open — Low: EncodedBlockAccessList null overwrite on verify path

BlockProcessor.cs:104 still reads:

suggestedBlock.EncodedBlockAccessList = block.EncodedBlockAccessList;

On the verify-only path, block.EncodedBlockAccessList is null (not copied in PrepareBlockForProcessing, not set in SetBlockAccessList). This silently overwrites the wire-encoded bytes that ExecutionPayloadV4.TryGetBlock had populated on suggestedBlock. Persistence then falls back to re-encoding from the decoded form, partially eroding the 3–10 ms saving this PR aims to eliminate. Fix: ?? suggestedBlock.EncodedBlockAccessList as noted in the prior review.

Still open — Low: ValidateStructuralEquivalence doc

The summary at StateChanges.cs:118 still doesn't explain why StorageChanges (writes) are absent from the walk. The inline comment added by the recent commit adds the Reth cross-reference on the field, but the method doc gap remains. A future auditor will ask.

Partially addressed — Low: test specificity

NewPayloadV5_rejects_invalid_BAL_after_processing still uses the broad OR pattern (any InvalidBlockLevelAccessList: prefix accepted). The new InvalidBalEarlyTestCases / BalErrorKind.SurplusReads test covers the surplus-reads structural path well, and SurplusChange covers surplus accounts with lane data caught early. The post-execution structural path (ValidateStructuralEquivalence → account-set size mismatch for an account with no lane data) is still covered only by the broad OR test. Not a blocker, but a gap in regression coverage for the specific new code path.


Correctness is solid. The benaadams concern is addressed by the existing ValidateHash gate. Three Low findings remain, none blocking merge.

benaadams
benaadams previously approved these changes May 18, 2026
Base automatically changed from bal-devnet-7-benchmarks to master May 19, 2026 12:06
@LukaszRozmej LukaszRozmej dismissed benaadams’s stale review May 19, 2026 12:06

The base branch was changed.

Eip7843TransitionTimestamp = config.AmsterdamTime,
Eip7928TransitionTimestamp = config.AmsterdamTime,
Eip7954TransitionTimestamp = config.AmsterdamTime,
Eip7976TransitionTimestamp = config.AmsterdamTime,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert change?

public bool ParallelExecutionEnabled { get; private set; }

// Replaces the end-of-block encode + Keccak with a structural-equivalence check (account-set
// + storage_reads). See paradigmxyz/reth#24297 for the prior art.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

important to include this in comment?


// Replaces the end-of-block encode + Keccak with a structural-equivalence check (account-set
// + storage_reads). See paradigmxyz/reth#24297 for the prior art.
private bool _verifyOnly;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need new bool instead of just using ParallelExecutionEnabled?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it might be useful to have some dev flag to disable this optimisation in case we want to compare generated and suggested bal. In that case we could use this bool

/// <summary>
/// Catches what the canonical-bytes hash compare used to catch but the column-index per-row
/// check doesn't: account-set presence and the storage_reads contents per account.
/// </summary>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should probably be in BlockAccessListManager.Validation

* Drop the _verifyOnly bool — it was always equal to ParallelExecutionEnabled
  in this PR. Use the property directly. (#11661 reintroduces a meaningful
  variant of this flag via ForceMaterializeGeneratedBlockAccessList, where it
  actually differs from ParallelExecutionEnabled.)
* Move ValidateStructuralEquivalence from BlockAccessListManager.StateChanges
  to BlockAccessListManager.Validation — it belongs with the other validation
  helpers, not the state-mutation helpers.
* Document on ValidateStructuralEquivalence why StorageChanges (writes) are
  not compared: they're fully covered by the per-tx ChangesAtIndexEqual lane
  comparisons that the incremental validation already runs at every index.
* Revert the stylistic line reordering in GethGenesisLoader.cs — unrelated to
  this PR.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@LukaszRozmej

Copy link
Copy Markdown
Member Author

@claude re-review

@claude

claude Bot commented May 19, 2026

Copy link
Copy Markdown
Contributor

Claude finished @LukaszRozmej's task in 3m 20s —— View job


Re-review

  • Gather context from prior reviews and new reviewer comments
  • Read changed files
  • Re-assess previous findings
  • Assess Marchhill's inline comments
  • Post final findings

Commit aab004b8 addresses all four of @Marchhill's inline comments

Marchhill comment Status
GethGenesisLoader.cs — revert change? ✅ Reverted. Line ordering now matches origin/master.
BlockAccessListManager.cs — important to include this in comment? ✅ Reth cross-reference comment was on the _verifyOnly field; field was dropped. The method-level <remarks> on ValidateStructuralEquivalence now documents the design rationale.
BlockAccessListManager.cs — why do we need new bool instead of just using ParallelExecutionEnabled? _verifyOnly dropped. SetBlockAccessList now branches directly on ParallelExecutionEnabled. Commit note: a meaningful variant is deferred to #11661 via ForceMaterializeGeneratedBlockAccessList.
BlockAccessListManager.StateChanges.cs — this should probably be in BlockAccessListManager.Validation ValidateStructuralEquivalence moved to BlockAccessListManager.Validation.cs (lines 291–340).

The <remarks> block on ValidateStructuralEquivalence (Validation.cs:297–301) also closes the previous Low finding about explaining why StorageChanges (writes) are not compared.


Remaining findings

Low — EncodedBlockAccessList null overwrite on verify path (BlockProcessor.cs:104) — unchanged

This finding from the first review remains. On the verify-only path SetBlockAccessList returns early after ValidateStructuralEquivalence and never sets block.EncodedBlockAccessList. So block.EncodedBlockAccessList is null here, and line 104 silently overwrites the wire bytes that ExecutionPayloadV4.TryGetBlock had populated on suggestedBlock:

suggestedBlock.EncodedBlockAccessList = block.EncodedBlockAccessList;  // null on verify path

IBlockAccessListStore.InsertFromBlock and ExecutionPayloadV4.Create both fall back to re-encoding from the decoded form when this is null, partially eroding the ~3–10 ms saving the PR aims to achieve. The fix is a one-liner:

suggestedBlock.EncodedBlockAccessList = block.EncodedBlockAccessList ?? suggestedBlock.EncodedBlockAccessList;

Low — Test specificity (post-execution structural path) — unchanged

NewPayloadV5_rejects_invalid_BAL_after_processing still accepts any InvalidBlockLevelAccessList: prefix via a broad OR. The new InvalidBalEarlyTestCases/SurplusReads case covers the early-validation structural path well. The post-execution path (ValidateStructuralEquivalenceAccount-set size mismatch for a no-lane-data account) is still covered only by the OR fallback. Not a blocker, but a regression gap for the specific new code path.


Overall

All Marchhill feedback is addressed. benaadams and Marchhill have approved. The correctness argument remains sound (explained in detail in the prior re-review above). Two Low findings remain, neither blocking. Mergeable.

…t test

* BlockProcessor copies block.EncodedBlockAccessList back onto suggestedBlock
  after Process. On the verify-only path that field is null (encode+Keccak is
  skipped), and overwriting silently dropped the wire bytes the engine-API
  decoder had attached, forcing later consumers (BAL store insert, RPC payload
  export) to re-encode from the decoded form. Coalesce: ?? suggestedBlock.EncodedBlockAccessList.
* NewPayloadV5_rejects_invalid_BAL_after_processing replaces the generic
  'InvalidBlockLevelAccessList:' OR fallback with the specific 'Account-set
  size mismatch' substring that the verify-only structural path emits for
  this no-lane-data surplus account case.
@LukaszRozmej LukaszRozmej merged commit b1fa33d into master May 19, 2026
824 of 831 checks passed
@LukaszRozmej LukaszRozmej deleted the perf/bal-verify-only-source branch May 19, 2026 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants