Skip to content

perf(bal): cache wire BAL hash on ReadOnlyBlockAccessList#11668

Merged
LukaszRozmej merged 243 commits into
masterfrom
perf/bal-cached-wire-hash
May 20, 2026
Merged

perf(bal): cache wire BAL hash on ReadOnlyBlockAccessList#11668
LukaszRozmej merged 243 commits into
masterfrom
perf/bal-cached-wire-hash

Conversation

@LukaszRozmej

Copy link
Copy Markdown
Member

Summary

BlockValidator.ValidateBlockLevelAccessListHashMatches currently calls ValueKeccak.Compute(block.EncodedBlockAccessList!) for every block, re-hashing bytes the decoder just consumed. This PR moves the hash to the canonical place: the decoder. The keccak is computed once over the BAL's wire RLP slice and cached on ReadOnlyBlockAccessList.WireHash; the validator then does a single Hash256 equality check.

Changes

  • ReadOnlyBlockAccessList: new Hash256? WireHash property and a 3-arg constructor that takes the precomputed hash. The existing 2-arg ctor delegates to it with null, so synthesised BALs (test fixtures, in-process construction) keep working unchanged.
  • BlockAccessListDecoder.Decode(ref Rlp.ValueDecoderContext, ...): captures the BAL's RLP slice via ctx.Position before/after DecodeArray, hashes only that slice (so envelope-embedded BALs still hash only their own bytes), and passes the hash to the new constructor.
  • BlockValidator.ValidateBlockLevelAccessListHashMatches: prefers bal.WireHash and falls back to ValueKeccak.Compute(block.EncodedBlockAccessList!) when the cache is absent.

Tests

  • New regression tests in BlockAccessListDecoderTests:
    • Decode_caches_wire_hash_matching_full_rlp_keccak — caches a hash byte-for-byte equal to a fresh ValueKeccak.Compute over the same RLP bytes.
    • Decode_handles_envelope_with_trailing_bytes_and_hashes_only_the_bal_slice — embeds a BAL in an envelope with trailing bytes; the cached hash must match only the BAL's own RLP slice.
    • Synthesised_bal_has_no_wire_hashWireHash is null on the parameterless ctor (fallback path stays exercised).

Test plan

  • dotnet build src/Nethermind/Nethermind.slnx -c release
  • dotnet test --project src/Nethermind/Nethermind.Core.Test/Nethermind.Core.Test.csproj -c release (5449 passed, 105 skipped, 0 failed)
  • dotnet test --project src/Nethermind/Nethermind.Consensus.Test/Nethermind.Consensus.Test.csproj -c release (only a pre-existing flake in PreWarmCaches_WhenPoolEvicts_EvictedEnvsAreDisposed, unrelated; passes in isolation)
  • dotnet test --project src/Nethermind/Nethermind.Merge.Plugin.Test/Nethermind.Merge.Plugin.Test.csproj -c release (1006 passed)
  • dotnet format whitespace src/Nethermind/ --folder --verify-no-changes

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.
Marchhill and others added 2 commits May 18, 2026 16:03
M1: ReadOnlySlotChanges.Equals used Changes.SequenceEqual(...) with
    using System.Linq in scope. Since Changes is StorageChange[], that
    bound to Enumerable.SequenceEqual<T> (allocates an iterator) rather
    than MemoryExtensions.SequenceEqual<T> on ReadOnlySpan<T>. Same
    pattern that was fixed in ReadOnlyAccountChanges.Equals (M1 round 1);
    missed it on the slot-changes file. Force the span overload via
    (ReadOnlySpan<StorageChange>)Changes and drop the now-unused
    System.Linq import.

L1: Drop the "// Otherwise, push the account's code hash." noise
    comment from InstructionExtCodeHash — it restates the next line.
    Came back when EXTCODEHASH was reverted to master's body; cleaner
    to remove on both sides per coding-style.md ("Comments that merely
    restate the code are noise — don't add them, and remove them when
    you encounter them"). The comment above it stays — that one earns
    its place via the EIP-1052 spec reference.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The decoder already has the BAL's RLP bytes in hand, so compute the
keccak there and store it on the decoded instance. BlockValidator's
hash check then becomes a single Hash256 equality instead of a
per-block ValueKeccak.Compute over the same bytes.

Synthesised BALs (test fixtures, generated/in-process construction)
have no WireHash and fall back to the existing hash-on-demand path.
@claude

claude Bot commented May 19, 2026

Copy link
Copy Markdown
Contributor

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


Review

  • Gather context and read changed files
  • Review ReadOnlyBlockAccessList.cs changes
  • Review BlockAccessListDecoder.cs changes
  • Review BlockValidator.cs changes
  • Review test changes in BlockAccessListDecoderTests.cs
  • Check adherence to repo rules and coding guidelines
  • Post findings

Summary: The optimization is correct and well-structured. The hash is computed once during decoding from the exact wire bytes, cached on the immutable ReadOnlyBlockAccessList, and consumed in the validator as a single reference comparison. Two low-severity findings noted below; no blockers.


Correctness

Position tracking (BlockAccessListDecoder.cs:34-36)
startPosition captures ctx.Position before ctx.DecodeArray(...). DecodeArray reads the outer list prefix + all account elements, advancing ctx.Position to the end of the BAL. The resulting slice:

ctx.Data.Slice(startPosition, ctx.Position - startPosition)

captures the complete BAL wire encoding (outer list prefix + content), which is exactly the bytes hashed for the block header. This is correct.

Hash computed after validation loop
The ValueKeccak.Compute call is placed after the sort-order validation loop (line 59). If ThrowAccountChangesOutOfOrder is raised, the keccak is never computed and no Hash256 is allocated — ideal ordering.

new Hash256(ValueKeccak.Compute(...)) constructor path
ValueKeccak.Compute returns a ValueHash256. Hash256 has a Hash256(in ValueHash256) constructor that copies the 32 bytes directly — no intermediate byte-array allocation. The old validator fallback used .Bytes (array allocation); the new decoder path and the updated fallback both use the in ValueHash256 overload, which is strictly better.

Hash256? on a reference type — no boxing
Hash256 is a sealed class, so Hash256? is a nullable reference annotation, not a Nullable<T> wrapper. The null check in the validator (bal.WireHash ?? ...) is a plain null reference test — no overhead.


Findings

Low — Misleading comment in the envelope test

BlockAccessListDecoderTests.cs:50-51:

// Embed a BAL inside an envelope so its bytes are not at the start of the input. The
// decoder must hash only the BAL's own RLP, not the surrounding bytes.

The BAL bytes are at the start (offset 0) of the envelope buffer; the trailing 0xdeadbeef bytes follow. The test correctly verifies that trailing bytes are excluded from the hash, but "not at the start of the input" is inaccurate and may confuse future readers. A clearer description would be: "Append trailing bytes after a BAL's RLP; the decoder must hash only the BAL's own slice, not the junk bytes."

Fix this →

Low — Fallback path not exercised through the validator in new tests

Synthesised_bal_has_no_wire_hash confirms WireHash == null for the parameterless ctor, but no new test calls ValidateBlockLevelAccessListHashMatches with a synthesised BAL (i.e., exercises the ?? new Hash256(ValueKeccak.Compute(block.EncodedBlockAccessList!)) branch). Existing Nethermind.Consensus.Test coverage likely handles this, so not a blocker, but an explicit regression test in BlockAccessListDecoderTests or the validator tests would add confidence.


Minor Style Observation

BlockAccessListDecoder.cs:31-33: The 3-line block comment explaining position tracking is justified (non-obvious performance invariant), but could be trimmed to one or two lines per the project's conciseness preference. Not required.


Verdict: Mergeable. Both findings are low severity. No correctness, security, or performance issues.

Comment thread src/Nethermind/Nethermind.Core.Test/Encoding/BlockAccessListDecoderTests.cs Outdated
…e-hash

# Conflicts:
#	src/Nethermind/Nethermind.Consensus.Test/BlockAccessListValidationIndexTests.cs
#	src/Nethermind/Nethermind.Consensus/Processing/BlockAccessListManager.StateChanges.cs
#	src/Nethermind/Nethermind.Consensus/Processing/BlockAccessListManager.TxProcessorPool.cs
#	src/Nethermind/Nethermind.Consensus/Processing/BlockAccessListManager.Validation.cs
#	src/Nethermind/Nethermind.Consensus/Processing/BlockAccessListManager.cs
#	src/Nethermind/Nethermind.Consensus/Processing/BlockAccessListValidationIndex.cs
#	src/Nethermind/Nethermind.Core.Test/BlockAccessLists/BlockAccessListItemCountTests.cs
#	src/Nethermind/Nethermind.Core.Test/BlockAccessLists/BlockAccessListJournalTests.cs
#	src/Nethermind/Nethermind.Core.Test/BlockAccessLists/ReadOnlyAccountChangesLookupTests.cs
#	src/Nethermind/Nethermind.Core/BlockAccessLists/BlockAccessListAtIndex.cs
#	src/Nethermind/Nethermind.Core/BlockAccessLists/GeneratedAccountChanges.cs
#	src/Nethermind/Nethermind.Core/BlockAccessLists/GeneratedBlockAccessList.cs
#	src/Nethermind/Nethermind.Core/BlockAccessLists/ReadOnlyAccountChanges.cs
#	src/Nethermind/Nethermind.Core/BlockAccessLists/ReadOnlyBlockAccessList.cs
#	src/Nethermind/Nethermind.Core/BlockAccessLists/StorageChangesByIndexConverter.cs
#	src/Nethermind/Nethermind.Evm.Test/Eip7928Tests.cs
#	src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs
#	src/Nethermind/Nethermind.Serialization.Rlp/Eip7928/BlockAccessListDecoder.cs
#	src/Nethermind/Nethermind.Specs/ChainSpecStyle/GethGenesisLoader.cs
#	src/Nethermind/Nethermind.State.Test/BlockAccessListBasedWorldStateTests.cs
#	src/Nethermind/Nethermind.State.Test/TracedAccessWorldStateTests.cs
#	src/Nethermind/Nethermind.State/TracedAccessWorldState.cs
Drop blocks that restate the code or test name; keep the why-notes
where they actually add context.
Co-authored-by: Lukasz Rozmej <lukasz.rozmej@gmail.com>
@LukaszRozmej LukaszRozmej marked this pull request as ready for review May 20, 2026 11:49
@claude

claude Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

Claude finished @LukaszRozmej's task in 2m 8s —— View job


Review

  • Gather context and read changed files
  • Review ReadOnlyBlockAccessList.cs changes
  • Review BlockAccessListDecoder.cs changes
  • Review BlockValidator.cs changes
  • Review test changes in BlockAccessListDecoderTests.cs
  • Check for updates since prior review
  • Post findings

Summary: The optimization is correct and well-structured. Both findings from the prior review session have been addressed. No new issues found.


What changed since the last review

Two follow-up commits landed since the previous review:

  • 076891a5 trimmed redundant comments in the decoder, validator, and test file — including removing the misleading comment that the prior review flagged.
  • 6c37c60c applied LukaszRozmej's own suggestions, removing two code-narrating comments from unrelated test files.

Correctness

Position tracking (BlockAccessListDecoder.cs:33-35)
startPosition captures ctx.Position before DecodeArray. After DecodeArray the context sits at the end of the outer BAL list. The slice ctx.Data.Slice(startPosition, ctx.Position - startPosition) covers the outer list prefix plus all content — exactly the bytes the block header hashes. The envelope test (Decode_handles_envelope_with_trailing_bytes_and_hashes_only_the_bal_slice) confirms trailing bytes are excluded. Correct.

Hash computed after validation loop (BlockAccessListDecoder.cs:58)
ValueKeccak.Compute is called only after all address-order checks pass. If ThrowAccountChangesOutOfOrder fires, no Hash256 is allocated. Ideal ordering.

Allocation profile
ctx.Data.Slice(...) produces a ReadOnlySpan<byte> with no heap allocation. ValueKeccak.Compute accepts a ReadOnlySpan<byte> directly. new Hash256(ValueHash256) copies 32 bytes via the in ValueHash256 constructor, not via a byte array. The hot path on the validator side reduces to a single reference comparison — nothing hashed at all.

Hash256? is a nullable reference type annotation
Hash256 is a sealed class, so Hash256? carries no boxing or Nullable<T> overhead. The ?? new Hash256(...) fallback in ValidateBlockLevelAccessListHashMatches is a plain null reference test. Correct.

block.EncodedBlockAccessList! in the fallback
The null-forgiving operator was already present before this PR. The fallback is only reached when bal.WireHash is null (synthesised BAL), and the caller already guards on block.BlockAccessList is not null. The pre-existing contract assumption is unchanged by this PR.


Addressed since prior review

Low — Misleading comment in envelope test (prior BlockAccessListDecoderTests.cs:50-51)
Fixed: 076891a5 removed the comment entirely. The test name (Decode_handles_envelope_with_trailing_bytes_and_hashes_only_the_bal_slice) is self-documenting.

Low — Fallback path not exercised through the validator
Not added as a new test, but the prior review noted this is covered by existing Nethermind.Consensus.Test integration tests and is not a blocker. No change in status.


Verdict: Mergeable. No Critical, High, or Medium findings. All prior Low findings are resolved or acknowledged.

public ReadOnlyBlockAccessList(ReadOnlyAccountChanges[] orderedAccounts, int itemCount)
: this(orderedAccounts, itemCount, wireHash: null) { }

/// <inheritdoc cref="ReadOnlyBlockAccessList(ReadOnlyAccountChanges[], int)"/>

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.

comment needed?

}

[Test]
public void Synthesised_bal_has_no_wire_hash()

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.

test needed?

Per Marc's review on #11668: the <inheritdoc/> on the 3-arg ctor was
restating the 2-arg ctor's doc; the synthesised-bal test only asserted
a constructor default. Both add noise without context.
Comment thread src/Nethermind/Nethermind.Consensus/Validators/BlockValidator.cs
@LukaszRozmej LukaszRozmej merged commit c2cc430 into master May 20, 2026
548 checks passed
@LukaszRozmej LukaszRozmej deleted the perf/bal-cached-wire-hash branch May 20, 2026 15:32
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