Skip to content

core, blockstm, state: add BlockSTM v2 parallel transaction execution#2210

Open
cffls wants to merge 7 commits intodevelopfrom
blockstm_redesign
Open

core, blockstm, state: add BlockSTM v2 parallel transaction execution#2210
cffls wants to merge 7 commits intodevelopfrom
blockstm_redesign

Conversation

@cffls
Copy link
Copy Markdown
Contributor

@cffls cffls commented May 5, 2026

BlockSTM v2

Introduces BlockSTM v2 — a from-scratch redesign of Bor's parallel
transaction execution engine. V2 speculatively executes block transactions
concurrently, validates each tx's reads against a multi-version store, and
re-executes any whose reads turned stale. On the 241-block mainnet witness
benchmark V2/4w delivers ~1.6× throughput over serial (570 mgas/s vs
350 mgas/s, AMD Ryzen 7 5800H, all-in-memory).

Architecture

V2 runs three coordinated goroutine groups around a per-tx PDB:

  V2StateProcessor.Process    (core/parallel_state_processor.go)
        │
  ExecuteV2BlockSTM           (core/blockstm/v2_executor.go)
        │
  ┌─────┼──────────────┬──────────────┐
  │     │              │              │
  Workers (N)     Validator (1)   Settlement (1)
  ParallelStateDB StoreReads      finalDB
                  BalReads        IntermediateRoot

Backed by:

  • SafeBase — thread-safe base reads (sync.Map caches over a bounded
    pool of StateDB.Copy() with concurrent-reads mode on the trieReader).
  • MVStore — sharded multi-version per-key store with a lock-free bloom
    filter for cold-key reads.
  • MVBalanceStore — sharded commutative balance delta store (per-tx
    Add/Sub; reads sum prior entries).

Execution flow

  1. Task building. Block transactions become V2Tasks. Same-sender
    chains get pre-computed nonces (SenderNonces) so nonce reads on a
    chain are skipped during validation.
  2. Parallel execution. N worker goroutines pull tasks from a buffered
    dispatcher (window numWorkers * InFlightTaskMultiplier). Each tx runs
    in its own ParallelStateDB; reads come from SafeBase + MVStore +
    MVBalanceStore and are recorded in StoreReads / BalReads. Writes
    accumulate locally (DeferMVWrites) and flush to MVStore at end-of-tx
    so concurrent readers only ever see FINAL values — never mid-tx
    reentrancy-guard writes.
  3. Sequential validation. A single goroutine validates txs in tx-index
    order. Each recorded read is re-checked against MVStore; match by
    writer/incarnation OR by value-equal fallback (handles idempotent writes
    such as reentrancy-guard SSTOREs that flip back). Mismatch →
    MarkEstimate the failed tx's writes and dispatch a re-execution
    goroutine. Per-key pipelining: readers that hit an ESTIMATE entry
    under Incarnation > 0 block on WaitForFinal until the upstream
    writer is finalized.
  4. Pipelined settlement. As txs finalize, a settlement goroutine drains
    chSettle in tx-index order and applies each tx's writes to finalDB
    (the real, single-threaded *state.StateDB) through a *Direct setter
    family that bypasses the journal, then asks finalDB for the
    IntermediateRoot.

Testing

V2 is gated on a layered test surface. From cheapest to most expensive,
and what each layer is meant to catch:

1. Compile-time conformance + drift detection

The PDB shadows StateDB's interface and behaviour, so any upstream
go-ethereum merge that adds or changes a StateDB method would silently
bypass V2. A handful of go test-time checks fail CI before any logic
runs:

Check What it pins
core/vm/statedb_impl_test.go PDB satisfies vm.StateDB via a static assertion
TestPDBMethodParity every StateDB method has a PDB mapping or is in pdbExemptMethods
TestV2DependencyCompileCheck every StateDB method V2 settle calls remains present
TestV2JournalEntryCoverage every journal entry kind has a parallelJournalEntry mapping
TestV2TracingHookParity every tracing.Hooks field is classified as fired-or-skipped
TestV2ForkParity every params.ChainConfig.IsX fork rule is classified V1/V2

2. Per-method unit tests (~210 tests across ~25 files)

Cover individual PDB getters/setters, MVStore / MVBalanceStore
primitives, V2 executor channel mesh, and SettleTo helpers. Highlights:

File Coverage
core/state/parallel_statedb_test.go 76 tests; PDB behaviour + the Tier-1 mutation kill suite (see layer 5)
core/state/parallel_statedb_coverage_test.go 42 tests; branch coverage
core/state/parallel_statedb_getter_table_test.go every PDB getter records its read with the right WriterIdx across Committed / ESTIMATE / NoEntry / AtTxZero
core/state/safe_base_test.go sync.Map cache + pool semantics
core/blockstm/mvstore_test.go, core/blockstm/mvbalance_store_test.go versioned store primitives
core/blockstm/v2_executor_wait_test.go waitForTx / waitForFinal + cancellation

3. Direct-setter parity tests

The *Direct setter family bypasses StateDB's journal at settle time.
core/state/v2_direct_setter_parity_test.go (7 tests) pins that
SetXDirect produces a byte-identical state root to journaled
SetX + Finalise. Catches divergence the moment a future change to either
path breaks the parity.

4. Differential tests vs serial StateDB

Hand-written + table-driven scenarios that exercise the PDB against a
parallel-mirror serial StateDB and assert byte-identical output. Catches
behaviour drift the parity-table tests can't express:

  • core/state/v2_differential_test.go — PDB-only diff
  • core/state/v2_executor_differential_test.go — synthetic-env executor diff
  • core/v1_differential_test.go — V1 vs serial parity for the legacy
    in-tree path

5. Mutation testing (Tier-1 kill tests)

diffguard runs mutation testing against V2's critical paths. Every
survivor flagged by a sample run has a corresponding targeted test inline
in core/state/parallel_statedb_test.go under the "Tier-1 mutation kill
tests" divider — boundary, negation, and return-value mutations on
storeReadMatches, journal revert, settleTo helpers, applyFeeData,
Reset, etc. Tier-1 logic kill-rate ≥99% on the latest run.

6. Fuzz targets

Randomized inputs against either a serial mirror or a hand-built reference:

  • core/state/v2_fuzz_test.go — random PDB op sequences vs StateDB
  • core/state/v2_executor_fuzz_test.go — executor-level fuzz on
    synthetic env
  • core/v2_serial_parity_fuzz_test.go (FuzzV2ExecutorVsSerial) — random
    tx batches through ExecuteV2BlockSTM vs an ApplyMessage loop

The race-detected fuzz under -race caught the shared-trie-reader race
that the non-race fuzz missed; worth keeping on the nightly.

7. End-to-end consistency + benchmark on real mainnet blocks

core/mainnet_witness_benchmark_test.go bundles 241 real Polygon mainnet
blocks (under core/blockstm/testdata/) with their pre-block witnesses.
Two harnesses share the corpus:

  • TestV2BlockSTMAllBlocks (gated on BOR_BLOCKSTM_TEST=1) replays each
    block through both serial and V2 and asserts byte-identical state roots
    and receipt roots.
  • BenchmarkV2AllBlocks runs serial + V2 across worker counts (4 / 8 / 16)
    and witness-on/off variants on the same corpus. Backs the throughput
    numbers referenced at the top of this commit.

8. Runtime invariants under -tags=invariants

Build-tag-gated runtime assertions inside the executor and the PDB. Off in
production builds (zero-cost), on in CI:

  • assertSettleOrder — validation walk's induction
  • assertReexecVisitedExactlyOnce — drain loop doesn't lose a tx
  • assertSettleNotPanicked — panicked PDBs never settle

A tiny set of "panic if invariant breaks" tests under
//go:build invariants verifies the assertions actually fire on crafted
violations (core/blockstm/v2_executor_invariants_panic_test.go,
core/state/parallel_statedb_invariants_panic_test.go).

9. Race detector

All of layers 2–8 are runnable under go test -race. CI runs the full
state + blockstm packages in race mode; the TestV2BlockSTMAllBlocks
gated test is also race-clean on the 241-block corpus.

10. Production soak — >1 million Polygon mainnet blocks

Beyond the unit / parity / fuzz layers above, this branch has been used to
sync more than 1,000,000 mainnet blocks end-to-end on a real node with V2
as the primary processor (with serial disabled). Zero state-root
divergences, zero panics requiring fallback, no consensus-affecting issues
observed. This is the most stringent layer: real on-chain workload, real
database backend, real prefetcher contention.

Bundled additions

  • intermediateRootTimer metric (chain/intermediateroot) — measures the
    post-execution trie computation in block_validator.go.

Major changes

The code surface is ~5.1k lines across 39 production .go files, plus
~11.7k lines across 37 test files. The remaining 484 file entries in the
diff are block + witness fixtures under core/blockstm/testdata used by
TestV2BlockSTMAllBlocks and the benchmark harness — read-only data, no
review needed.

Shapes of change a reviewer should expect:

  • New per-tx state. ParallelStateDB shadows *state.StateDB but
    reads from SafeBase + MVStore + MVBalanceStore and tracks reads for
    validation. Implements vm.StateDB. Has its own journal layer
    (parallelJournalEntry) parallel to StateDB's journal.go.
  • New concurrent stores. MVStore (sharded multi-version per-key store
    with bloom filter) and MVBalanceStore (sharded commutative balance
    deltas) — both new, both load-bearing.
  • New executor. ExecuteV2BlockSTM owns the worker pool + in-order
    validator + pipelined settle goroutine and the chSettle /
    completionCh / execDone channel mesh between them.
  • Concurrent-safe base reads. SafeBase is a thread-safe wrapper
    around a *state.StateDB with sync.Map caches + a bounded pool of
    db.Copy() instances; the pool copies share the underlying reader, so
    the V2 entry point flips trieReader into its concurrent-reads mode
    (sync.Map node-resolve cache instead of in-place mutation) — this
    required surgery in state/database.go, state/reader.go,
    state/trie_prefetcher.go, trie/trie.go, trie/secure_trie.go,
    triedb/pathdb/reader.go, and triedb/pathdb/biased_fastcache.go.
  • *Direct setter family on StateDB. Bypasses the journal at settle
    time so V2 can replay per-tx PDB writes onto finalDB
    deterministically. Pinned byte-equal to journaled SetX + Finalise by
    TestDirectSetterParity_*.
  • Production fallback. BlockChain wires V2 as the primary processor
    and falls back to serial on panics, ApplyMessage consensus errors,
    ctx cancellation, and witness requests.

Files that warrant the most reviewer attention

Tier 1 — load-bearing executor + per-tx state

File Notes
core/blockstm/v2_executor.go +631 new
core/parallel_state_processor.go +925; V2StateProcessor, settle-fn closure, env
core/state/parallel_statedb.go +1147 new
core/state/parallel_statedb_validate.go +223 new
core/state/parallel_statedb_settle.go +195 new
core/state/parallel_statedb_journal.go +127 new
core/state/safe_base.go +207 new

Tier 2 — concurrent stores

File Notes
core/blockstm/mvstore.go +186 new
core/blockstm/mvbalance_store.go +175 new

Tier 3 — modified upstream files (highest merge-conflict risk)

File Notes
core/state/statedb.go *Direct setters, skipTimers, concurrent-reads enabler
core/state/state_object.go concurrent-safe getters
core/state/database.go concurrent reader
core/state/reader.go cache attribution
core/state/trie_prefetcher.go concurrent prefetch
trie/trie.go, trie/secure_trie.go concurrent-reads mode
triedb/pathdb/reader.go sync.Map node-resolve cache for concurrent reads; small lock changes
triedb/pathdb/biased_fastcache.go lock semantics
core/vm/evm.go, jumpdests.go, instructions.go, interface.go, interpreter.go jumpdest cache sharing, precompile-cache, StateDB iface adds
core/blockchain.go V2 wiring + fallback
core/state_transition.go interrupt plumbing

Tier 4 — drift-detection tests

Read these to understand the parity contract V2 must hold against
StateDB:

File What it pins
core/state/v2_method_parity_test.go every StateDB method has a PDB mapping
core/state/v2_journal_entry_coverage_test.go every journal kind has a parallel mapping
core/state/v2_direct_setter_parity_test.go SetXDirect ↔ journaled
core/state/parallel_statedb_getter_table_test.go every PDB getter records its read
core/parallel_state_processor_hooks_parity_test.go tracing.Hooks fire-or-skip classification
core/parallel_state_processor_fork_parity_test.go params.IsX classification
core/v2_serial_parity_fuzz_test.go real-tx executor fuzz vs serial
core/mainnet_witness_benchmark_test.go gated 241-block end-to-end consistency + benchmark harness

See docs/blockstm-v2.md for full architectural
detail, the list of correctness bug classes V2 prevents, and
ongoing-improvement notes.

Introduces BlockSTM v2 — a from-scratch redesign of Bor's parallel
transaction execution engine. V2 speculatively executes block
transactions concurrently, validates each tx's reads against a
multi-version store, and re-executes any whose reads turned stale.
On the 241-block mainnet witness benchmark V2/4w delivers ~1.6×
throughput over serial (570 mgas/s vs 350 mgas/s, AMD Ryzen 7 5800H,
all-in-memory).

V2 runs three coordinated goroutine groups around a per-tx PDB:

  V2StateProcessor.Process    (core/parallel_state_processor.go)
        │
  ExecuteV2BlockSTM           (core/blockstm/v2_executor.go)
        │
  ┌─────┼──────────────┬──────────────┐
  │     │              │              │
  Workers (N)     Validator (1)   Settlement (1)
  ParallelStateDB StoreReads      finalDB
                  BalReads        IntermediateRoot

  Backed by:
    SafeBase       Thread-safe base reads (sync.Map caches over a
                   bounded pool of StateDB.Copy() with concurrent-
                   reads mode on the trieReader)
    MVStore        Sharded multi-version per-key store with a
                   lock-free bloom filter for cold-key reads
    MVBalanceStore Sharded commutative balance delta store
                   (per-tx Add/Sub; reads sum prior entries)

1. Task building. Block transactions become V2Tasks. Same-sender
   chains get pre-computed nonces (SenderNonces) so nonce reads
   on a chain are skipped during validation.

2. Parallel execution. N worker goroutines pull tasks from a
   buffered dispatcher (window numWorkers * InFlightTaskMultiplier).
   Each tx runs in its own ParallelStateDB; reads come from
   SafeBase + MVStore + MVBalanceStore and are recorded in
   StoreReads / BalReads. Writes accumulate locally (DeferMVWrites)
   and flush to MVStore at end-of-tx so concurrent readers only
   ever see FINAL values — never mid-tx reentrancy-guard writes.

3. Sequential validation. A single goroutine validates txs in
   tx-index order. Each recorded read is re-checked against MVStore;
   match by writer/incarnation OR by value-equal fallback (handles
   idempotent writes such as reentrancy-guard SSTOREs that flip
   back). Mismatch → MarkEstimate the failed tx's writes and
   dispatch a re-execution goroutine. Per-key pipelining: readers
   that hit an ESTIMATE entry under Incarnation > 0 block on
   WaitForFinal until the upstream writer is finalized.

4. Pipelined settlement. As txs finalize, a settlement goroutine
   drains chSettle in tx-index order and applies each tx's writes
   to finalDB (the real, single-threaded *state.StateDB) through a
   *Direct setter family that bypasses the journal, then asks
   finalDB for the IntermediateRoot.

V2 is gated on a layered test surface. From cheapest to most
expensive, and what each layer is meant to catch:

1. Compile-time conformance + drift detection
   The PDB shadows StateDB's interface and behaviour, so any
   upstream go-ethereum merge that adds or changes a StateDB
   method would silently bypass V2. A handful of `go test`-time
   checks fail CI before any logic runs:
     - core/vm/statedb_impl_test.go     (PDB satisfies vm.StateDB
                                         via a static assertion)
     - TestPDBMethodParity              (every StateDB method has
                                         a PDB mapping or is in
                                         pdbExemptMethods)
     - TestV2DependencyCompileCheck     (every StateDB method V2
                                         settle calls remains present)
     - TestV2JournalEntryCoverage       (every journal entry kind has
                                         a parallelJournalEntry mapping)
     - TestV2TracingHookParity          (every tracing.Hooks field is
                                         classified as fired-or-skipped)
     - TestV2ForkParity                 (every params.ChainConfig.IsX
                                         fork rule is classified V1/V2)

2. Per-method unit tests (~210 tests across ~25 files)
   Cover individual PDB getters/setters, MVStore / MVBalanceStore
   primitives, V2 executor channel mesh, and SettleTo helpers.
   Highlights:
     - core/state/parallel_statedb_test.go         (76 tests; PDB
                                                    behaviour + the
                                                    Tier-1 mutation
                                                    kill suite — see
                                                    layer 5 below)
     - core/state/parallel_statedb_coverage_test.go (42 tests;
                                                    branch coverage)
     - core/state/parallel_statedb_getter_table_test.go (every PDB
                                                    getter records
                                                    its read with
                                                    the right WriterIdx
                                                    across Committed /
                                                    ESTIMATE / NoEntry /
                                                    AtTxZero)
     - core/state/safe_base_test.go                (sync.Map cache +
                                                    pool semantics)
     - core/blockstm/mvstore_test.go,
       core/blockstm/mvbalance_store_test.go       (versioned store
                                                    primitives)
     - core/blockstm/v2_executor_wait_test.go      (waitForTx /
                                                    waitForFinal +
                                                    cancellation)

3. Direct-setter parity tests
   The *Direct setter family bypasses StateDB's journal at settle
   time. core/state/v2_direct_setter_parity_test.go (7 tests) pins
   that SetXDirect produces a byte-identical state root to journaled
   SetX + Finalise. Catches divergence the moment a future change
   to either path breaks the parity.

4. Differential tests vs serial StateDB
   Hand-written + table-driven scenarios that exercise the PDB
   against a parallel-mirror serial StateDB and assert byte-identical
   output. Catches behaviour drift the parity-table tests can't
   express:
     - core/state/v2_differential_test.go          (PDB-only diff)
     - core/state/v2_executor_differential_test.go (synthetic-env
                                                    executor diff)
     - core/v1_differential_test.go                (V1 vs serial
                                                    parity for the
                                                    legacy in-tree path)

5. Mutation testing (Tier-1 kill tests)
   diffguard runs mutation testing against V2's critical paths.
   Every survivor flagged by a sample run has a corresponding
   targeted test inline in core/state/parallel_statedb_test.go
   under the "Tier-1 mutation kill tests" divider — boundary,
   negation, and return-value mutations on storeReadMatches,
   journal revert, settleTo helpers, applyFeeData, Reset, etc.
   Tier-1 logic kill-rate ≥ 99% on the latest run.

6. Fuzz targets
   Randomized inputs against either a serial mirror or a hand-built
   reference:
     - core/state/v2_fuzz_test.go                (random PDB op
                                                  sequences vs StateDB)
     - core/state/v2_executor_fuzz_test.go       (executor-level fuzz
                                                  on synthetic env)
     - core/v2_serial_parity_fuzz_test.go        (FuzzV2ExecutorVsSerial:
                                                  random tx batches
                                                  through ExecuteV2BlockSTM
                                                  vs an ApplyMessage loop)
   The race-detected fuzz under `-race` caught the shared-trie-reader
   race that the non-race fuzz missed; worth keeping on the nightly.

7. End-to-end consistency + benchmark on real mainnet blocks
   core/mainnet_witness_benchmark_test.go bundles 241 real Polygon
   mainnet blocks (under core/blockstm/testdata/) with their pre-
   block witnesses. Two harnesses share the corpus:
     - TestV2BlockSTMAllBlocks (gated on BOR_BLOCKSTM_TEST=1)
       replays each block through both serial and V2 and asserts
       byte-identical state roots and receipt roots.
     - BenchmarkV2AllBlocks runs serial + V2 across worker counts
       (4 / 8 / 16) and witness-on/off variants on the same corpus.
       Backs the throughput numbers referenced at the top of this
       commit.

8. Runtime invariants under -tags=invariants
   Build-tag-gated runtime assertions inside the executor and the
   PDB. Off in production builds (zero-cost), on in CI:
     - assertSettleOrder              (validation walk's induction)
     - assertReexecVisitedExactlyOnce (drain loop doesn't lose a tx)
     - assertSettleNotPanicked        (panicked PDBs never settle)
   A tiny set of "panic if invariant breaks" tests under
   //go:build invariants verifies the assertions actually fire on
   crafted violations (core/blockstm/v2_executor_invariants_panic_test.go,
   core/state/parallel_statedb_invariants_panic_test.go).

9. Race detector
   All of layers 2-8 are runnable under `go test -race`. CI runs
   the full state + blockstm packages in race mode; the
   TestV2BlockSTMAllBlocks gated test is also race-clean on the
   241-block corpus.

10. Production soak — >1 million Polygon mainnet blocks
    Beyond the unit / parity / fuzz layers above, this branch has
    been used to sync more than 1,000,000 mainnet blocks end-to-end
    on a real node with V2 as the primary processor (with serial
    disabled). Zero state-root divergences, zero panics
    requiring fallback, no consensus-affecting issues observed.
    This is the most stringent layer: real on-chain workload,
    real database backend, real prefetcher contention.

  - intermediateRootTimer metric (chain/intermediateroot) — measures
    the post-execution trie computation in block_validator.go.

The code surface is ~5.1k lines across 39 production .go files,
plus ~11.7k lines across 37 test files. The remaining 484 file
entries in the diff are block + witness fixtures under
core/blockstm/testdata used by TestV2BlockSTMAllBlocks and the
benchmark harness — read-only data, no review needed.

Shapes of change a reviewer should expect:

  - New per-tx state. ParallelStateDB shadows *state.StateDB but
    reads from SafeBase + MVStore + MVBalanceStore and tracks reads
    for validation. Implements vm.StateDB. Has its own journal
    layer (parallelJournalEntry) parallel to StateDB's journal.go.

  - New concurrent stores. MVStore (sharded multi-version per-key
    store with bloom filter) and MVBalanceStore (sharded
    commutative balance deltas) — both new, both load-bearing.

  - New executor. ExecuteV2BlockSTM owns the worker pool +
    in-order validator + pipelined settle goroutine and the
    chSettle / completionCh / execDone channel mesh between them.

  - Concurrent-safe base reads. SafeBase is a thread-safe wrapper
    around a *state.StateDB with sync.Map caches + a bounded pool
    of db.Copy() instances; the pool copies share the underlying
    reader, so the V2 entry point flips trieReader into its
    concurrent-reads mode (sync.Map node-resolve cache instead of
    in-place mutation) — this required surgery in state/database.go,
    state/reader.go, state/trie_prefetcher.go, trie/trie.go,
    trie/secure_trie.go, triedb/pathdb/reader.go, and
    triedb/pathdb/biased_fastcache.go.

  - *Direct setter family on StateDB. Bypass the journal at
    settle time so V2 can replay per-tx PDB writes onto finalDB
    deterministically. Pinned byte-equal to journaled SetX +
    Finalise by TestDirectSetterParity_*.

  - Production fallback. BlockChain wires V2 as the primary
    processor and falls back to serial on panics, ApplyMessage
    consensus errors, ctx cancellation, and witness requests.

Tier 1 — load-bearing executor + per-tx state:

  core/blockstm/v2_executor.go              (+631 new)
  core/parallel_state_processor.go          (+925 V2StateProcessor,
                                             settle-fn closure, env)
  core/state/parallel_statedb.go            (+1147 new)
  core/state/parallel_statedb_validate.go   (+223 new)
  core/state/parallel_statedb_settle.go     (+195 new)
  core/state/parallel_statedb_journal.go    (+127 new)
  core/state/safe_base.go                   (+207 new)

Tier 2 — concurrent stores:

  core/blockstm/mvstore.go                  (+186 new)
  core/blockstm/mvbalance_store.go          (+175 new)

Tier 3 — modified upstream files (highest merge-conflict risk):

  core/state/statedb.go                     (Direct setters,
                                             skipTimers, concurrent
                                             reads enabler)
  core/state/state_object.go                (concurrent-safe getters)
  core/state/database.go                    (concurrent reader)
  core/state/reader.go                      (cache attribution)
  core/state/trie_prefetcher.go             (concurrent prefetch)
  trie/trie.go, trie/secure_trie.go         (concurrent-reads mode)
  triedb/pathdb/reader.go                   (sync.Map node-resolve
                                             cache for concurrent
                                             reads; small lock changes)
  triedb/pathdb/biased_fastcache.go         (lock semantics)
  core/vm/evm.go, jumpdests.go,             (jumpdest cache sharing,
       instructions.go, interface.go,        precompile-cache,
       interpreter.go                        StateDB iface adds)
  core/blockchain.go                        (V2 wiring + fallback)
  core/state_transition.go                  (interrupt plumbing)

Tier 4 — drift-detection tests (read these to understand the
parity contract V2 must hold against StateDB):

  core/state/v2_method_parity_test.go       (every StateDB method
                                             has a PDB mapping)
  core/state/v2_journal_entry_coverage_test.go
                                            (every journal kind has
                                             a parallel mapping)
  core/state/v2_direct_setter_parity_test.go
                                            (SetXDirect ↔ journaled)
  core/state/parallel_statedb_getter_table_test.go
                                            (every PDB getter
                                             records its read)
  core/parallel_state_processor_hooks_parity_test.go
                                            (tracing.Hooks fire-or-
                                             skip classification)
  core/parallel_state_processor_fork_parity_test.go
                                            (params.IsX classification)
  core/v2_serial_parity_fuzz_test.go        (real-tx executor fuzz
                                             vs serial)
  core/mainnet_witness_benchmark_test.go    (gated 241-block end-
                                             to-end consistency +
                                             benchmark harness)

See docs/blockstm-v2.md for full architectural detail, the list of
correctness bug classes V2 prevents, and ongoing-improvement notes.
Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

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

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

Tip: disable this comment in your organization's Code Review settings.

@claude
Copy link
Copy Markdown

claude Bot commented May 5, 2026

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

@marcello33 marcello33 requested a review from Copilot May 5, 2026 08:06
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

@marcello33
Copy link
Copy Markdown
Collaborator

@claude review

@claude
Copy link
Copy Markdown

claude Bot commented May 5, 2026

Claude finished @marcello33's task in 4m 15s —— View job


Code Review: BlockSTM v2 Parallel Transaction Execution

  • Read Tier 1 files — executor, parallel state processor, ParallelStateDB
  • Read Tier 2 files — MVStore, MVBalanceStore
  • Read Tier 3 files — modified upstream files (statedb, state_object, database, trie, blockchain)
  • Read supporting files — SafeBase, journal, validation, settlement
  • Read VM interface changes
  • Security review (concurrency, consensus, fund safety)
  • Post final review

This is a thorough, well-engineered redesign of Bor's parallel execution engine. The architecture — workers + single-goroutine validator + pipelined settlement — is sound and avoids most classic concurrency bugs by design. The test surface (10 layers including mutation testing, differential fuzz, and 241-block mainnet corpus) is exceptional for a change of this complexity.

Below are the findings, organized by severity.


Potential Issues

1. SubBalanceDirect has no underflow guard — uint256.Int.Sub wraps silently

Severity: MEDIUM | core/state/statedb.go:2191

func (s *StateDB) SubBalanceDirect(addr common.Address, amount *uint256.Int) {
    // ...
    obj.setBalance(new(uint256.Int).Sub(obj.Balance(), amount))
}

uint256.Int.Sub wraps on underflow (it performs modular arithmetic). The regular SubBalance path goes through stateObject.SubBalance which also wraps, so this is consistent with serial behavior. However, the regular path journals the old balance for revert — SubBalanceDirect skips the journal entirely. If a bug elsewhere produces a wrong amount, the balance silently wraps to a huge number with no revert possible during settlement. Since V2 settlement is irrevocable by design, this is worth noting. The EVM's own transfer checks prevent this in practice, but a defensive if amount.Gt(obj.Balance()) { panic(...) } guard in the Direct setter would be cheap insurance.

2. SenderInitBalance nil check uses inline function — fragile

Severity: LOW | core/state_transition.go:666

SenderInitBalance: func() *big.Int { if input1 != nil { return input1.ToBig() }; return nil }(),

input1 is assigned from st.state.GetBalance(st.msg.From) at line 468, which never returns nil (returns uint256.NewInt(0) for missing accounts). The nil guard is dead code and the inline IIFE obscures this. A straight input1.ToBig() is clearer and matches the idiom used everywhere else in this function.

3. ecrecoverCache key is a fixed [128]byte — input < 128 bytes is right-padded

Severity: LOW | core/vm/evm.go:79-81

var key [128]byte
copy(key[:], common.RightPadBytes(input, 128))

This allocates a 128-byte temporary via RightPadBytes on every cache miss AND every cache hit check. Since copy already zero-pads the destination (the key array is zero-initialized), the RightPadBytes call is redundant — copy(key[:], input) achieves the same result without the allocation.

4. keccak256Cache stores common.Hash (value type) but loads with type assertion

Severity: LOW | core/vm/instructions.go:270-275

if cached, ok := evm.Config.Keccak256Cache.Load(key); ok {
    size.SetBytes(cached.(common.Hash).Bytes())
    return nil, nil
}

The .Bytes() call allocates. size.SetBytes32(cached.(common.Hash)[:]) would avoid the allocation on the hot path, though the compiler may optimize this away.


Design Observations (Not Bugs)

5. ProcessBlock now waits synchronously for the losing processor

Observation | core/blockchain.go:936-938

The old code drained the losing processor asynchronously (go func() { ... }). The new code blocks:

if processorCount == 2 {
    second_result := <-resultChan
    second_result.statedb.StopPrefetcher()
}

This is actually correct and important — the old async drain was a latent bug because the caller could commit the block (advancing pathdb layers) while the loser's prefetcher still held trie references. The comment explains this well. Worth keeping.

6. Ablation flags are exported struct fields with no config plumbing

Observation | core/blockchain.go:417-420

AblationSkipFlush    bool
AblationSkipSettle   bool
AblationSkipFinalise bool
AblationSkipMVRead   bool

These appear to be performance-experiment knobs. They're exported but I don't see them wired to any CLI flag or config struct, which means they're only settable from test code. If they're not intended for production, consider keeping them unexported or behind a build tag to prevent accidental activation.

7. Bloom filter false-positive rate at high key counts

Observation | core/blockstm/mvhashmap.go:25-30

The bloom filter is 32Kbit (4KB). The PR description notes ~5% false-positive rate at 5000 unique keys. For large blocks this means ~5% of cold-key MVStore reads will unnecessarily acquire a shard read lock. The shard lock is an RWMutex with an empty map lookup, so the overhead is small — but worth monitoring with the Estimates counter if real-world blocks push past 5K unique keys.

8. Re-execution goroutine uses workerID = x.numWorkers (out of normal worker range)

Observation | core/blockstm/v2_executor.go:512

x.execute(idx, x.numWorkers)

Re-execution uses a synthetic worker ID equal to numWorkers (one past the last real worker). This means re-execution and initial execution can overlap on different worker IDs for the same tx. Since execute only writes x.states[idx] and per-idx MVStore entries, this is safe — but worth a comment noting the sentinel value.


What Looks Correct

Concurrency model: The separation of concerns — workers produce PDBs, single validator thread validates in order, single settler thread applies to finalDB — is the right architecture. The only shared mutable state is the MVStore/MVBalanceStore, which are properly sharded with per-shard mutexes.

Channel-based synchronization: execDone (closed once per tx by the initial worker), completionCh (closed once per tx when finalized), and chSettle (buffered, drained in order) form a correct coordination mesh. The cancellation via ctx.Done() is properly wired through waitForTx, waitForFinal, startTaskDispatcher, and runValidationLoop.

ESTIMATE handling: The spin-wait-on-ESTIMATE-then-retry pattern in readStoreWait / handleEstimate correctly implements per-key pipelining. The ReadVersionFull atomic read of (value, estimate flag) prevents the TOCTOU race mentioned in the comment.

Journal correctness: The parallel journal (parallelJournalEntry) correctly reverts all state kinds. The revertBalance path subtracts from localBalAdd/localBalSub (not from MVBalanceStore), which is correct because balance deltas are only flushed at end-of-tx.

Settlement ordering: The settleBalanceOpsAndLogs function replays balance operations in recording order, interleaving transfer logs at the correct positions. This is a subtle but correct approach to matching the serial path's log ordering.

Fallback path: V2 propagates panics, consensus errors (ExecErr), and context cancellation correctly to the caller, which falls back to serial execution. This defense-in-depth is important for a production deployment.

Trie concurrent reads: The getConcurrent path in trie/trie.go correctly uses a sync.Map resolve cache instead of mutating the tree in place. The prevalueTracer.Put call inside the concurrent path is noted as having its own lock — this is correct.

pathdb reader hash-mismatch retry: The cache-eviction-and-retry logic in triedb/pathdb/reader.go handles the race between the biased preloader's Has→Set and a flusher's Set(newer). This is a real bug fix, not just V2 work.


Test Coverage Assessment

The 10-layer test pyramid described in the PR is well-structured:

  • Compile-time conformance (method parity, journal coverage, fork parity) catches upstream drift automatically — this is the most valuable layer for long-term maintenance.
  • Differential tests against serial StateDB and the 241-block mainnet corpus provide strong confidence in correctness.
  • Mutation testing at ≥99% kill rate on Tier-1 logic is impressive.
  • Race-detected fuzz catching the shared-trie-reader race validates the -race investment.

The 1M+ mainnet block soak mentioned in the PR description provides the strongest signal — no state-root divergence or panics over real production workload.


Summary

This is a high-quality, well-tested PR. The architecture is sound, concurrency primitives are used correctly, and the fallback to serial execution provides a safety net. The issues found are minor (no CRITICAL or HIGH severity). The most actionable item is the defensive underflow guard in SubBalanceDirect (finding #1). The remaining items are cosmetic or informational.

Comment thread core/blockchain.go Outdated
Comment thread core/blockchain.go Outdated
Comment thread core/blockstm/mvhashmap.go Outdated
@adamdossa adamdossa requested a review from pratikspatil024 May 5, 2026 15:20
cffls and others added 4 commits May 5, 2026 11:14
Cosmetic / micro-perf fixes from the claude[bot] review of #2210.
No behaviour change.

* state/statedb.go SubBalanceDirect — add a comment noting the
  uint256.Sub wrap matches the journaled SubBalance path
  (statedb.go:922) and that TestDirectSetterParity_SubBalance pins
  byte-equality between the two. A defensive panic was suggested
  but would diverge from the journaled path and break the parity
  test, so we keep the documentation-only.

* state_transition.go SenderInitBalance — drop the inline IIFE nil
  check; input1 is GetBalance(...) which returns a value type,
  never nil. Straight input1.ToBig() matches the idiom used
  elsewhere in the function.

* vm/evm.go runEcrecoverWithCache — drop the redundant
  RightPadBytes(input, 128) allocation. The [128]byte key is
  zero-initialised, so copy(key[:], input) achieves the same
  result without the extra heap allocation. Caller already
  guarantees len(input) <= 128.

* vm/instructions.go opKeccak256 — replace size.SetBytes(cached.
  (common.Hash).Bytes()) with size.SetBytes32(h[:]) to skip the
  per-cache-hit Bytes() allocation on the SHA3 fast path.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The 241-block witness fixtures under core/blockstm/testdata/ are
managed via Git LFS (~1.6 GB total). On a fresh clone or a CI
runner that hasn't run `git lfs pull`, the .block / .witness.gz
files are LFS pointer text stubs rather than the real data, and
gzip.NewReader fails with "gzip: invalid header" — exactly what
the unit-tests CI workflow has been hitting.

Detect LFS pointers in readFileMaybeGz via the canonical
"version https://git-lfs.github.com/spec/" prefix and surface a
sentinel errLFSPointer error. loadEmbeddedBlocks and
loadBlocksFromDir then call t.Skipf instead of t.Fatalf when the
fixtures aren't materialized — the harness skips cleanly with a
helpful message ("run `git lfs pull` to materialize testdata")
instead of producing confusing gzip errors.

This is the same prerequisite called out in
docs/blockstm-v2.md → "Test data (Git LFS)".

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The lint workflow on #2210 flagged 15+ issues across V2 files. Fix
each so `make lint` is clean. No behaviour change in production
paths.

* goimports — formatting on ~12 files (idiomatic import grouping
  + blank-line alignment that the in-tree gofmt missed).
* unused — drop dead code:
    - executeWithParallelStateDBV2 + ValidatingParallelStateDB.checkBalance
      in core/mainnet_witness_benchmark_test.go (debug shims, never
      called).
    - timedMockV2State.execDelay + timedMockV2Env.fails in
      core/blockstm/v2_executor_test.go (vestigial fields).
    - ParallelStateDB.priorDestructed convenience wrapper (callers
      use priorDestructedAt).
    - opSubRefund / opWarmAddress diff ops in
      core/state/v2_differential_test.go (no scenario references them).
* copyloopvar — drop the redundant `x := x` loop-variable copies
  across 8 test files (Go 1.22+ no longer needs them).
* unconvert — drop the `time.Duration(result.Phase1)` cast (Phase1
  is already time.Duration) and the `JumpDestCache(newMapJumpDests())`
  cast (already satisfies the interface).
* durationcheck — fix `timeAfter(seconds time.Duration)` in
  core/blockstm/mvbalance_store_test.go: callers passed an int and
  the multiplication `seconds * time.Second` is a duration*duration
  bug. Make the parameter `int` and cast inside.
* copylocks — `*statedb = *backupStateDB` in V1's
  maybeRerunWithoutFeeDelay copies a struct holding atomic.Int64.
  This is single-threaded V1 rerun-from-snapshot; tag with
  `//nolint:govet` and a comment.
* whitespace — drop a leading blank line in v2Env.Execute that
  golangci-lint flagged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three issues flagged in the inline PR review of #2210.

* core/blockchain.go: V2-failure fallback recovery was broken.
  cancel() ran BEFORE the `if result.parallel && result.err != nil`
  block, so when V2 finished first with an error (panic, ApplyMessage
  consensus error) the still-running serial processor was interrupted
  at its next tx boundary and the fallback `result = <-resultChan`
  received context.Canceled instead of a usable serial result.
  Documented as the recovery contract in the PR description.

  Move cancel() and followupInterrupt.Store(true) to AFTER the
  fallback block. The fallback (when V2 errors with V1 also running)
  gets a real serial result. Once we have the result we plan to
  return, cancel the loser so it stops at its next tx boundary
  before commit advances the pathdb layer (the original intent).

  See review comment r3187036031.

* core/blockchain.go: drop unused AblationSkip* fields from
  BlockChain. Four exported boolean fields (AblationSkipFlush /
  AblationSkipSettle / AblationSkipFinalise / AblationSkipMVRead)
  were declared but never read or written anywhere — repo-wide grep
  confirms zero references outside the declaration site. The
  intended bridge from these BlockChain-level toggles to the per-
  block MVHashMap.Skip* fields (which ARE wired) was never threaded
  through, so flipping the BlockChain field was a silent no-op.
  Exported fields enter the API surface, so keeping them locks us
  into either a SemVer-breaking removal or maintainer confusion;
  drop them now and re-introduce as wired knobs in a separate
  change if/when the ablation experiments need a runtime entry
  point.

  See review comment r3187036037.

* core/blockstm/mvhashmap.go: bloom h2 dimension was constant zero
  for the hottest key class. h2 read bytes [20:24] of Key, which are
  populated only for state keys; NewAddressKey leaves [20:52] zero
  and NewSubpathKey leaves [20:51] zero. h3 also half-degraded for
  those classes ([28:32] zero). Result: address-only and subpath
  reads collapsed the bloom from 3-of-3 to ~2-of-3, which doubles
  the false-positive rate at typical block sizes (~0.07% → ~0.35%
  at 1k unique keys). No correctness impact, just hot-path
  selectivity.

  Re-derive h2 from address bytes [16:20] (always populated) and
  fold the subpath/type bytes [52][53] into h3 so all three hashes
  draw from non-constant ranges for every key class. Updated the
  comment to reflect the new layout.

  See review comment r3187036040.

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

codecov Bot commented May 5, 2026

Codecov Report

❌ Patch coverage is 91.44737% with 39 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.74%. Comparing base (1bf990d) to head (0c26b08).

Files with missing lines Patch % Lines
core/blockchain.go 79.64% 17 Missing and 6 partials ⚠️
core/blockstm/mvhashmap.go 90.97% 9 Missing and 4 partials ⚠️
core/blockstm/mvstore.go 97.97% 2 Missing ⚠️
core/block_validator.go 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2210      +/-   ##
===========================================
+ Coverage    52.29%   52.74%   +0.44%     
===========================================
  Files          884      894      +10     
  Lines       155571   158193    +2622     
===========================================
+ Hits         81355    83434    +2079     
- Misses       68989    69457     +468     
- Partials      5227     5302      +75     
Files with missing lines Coverage Δ
core/blockstm/invariants_off.go 100.00% <100.00%> (ø)
core/blockstm/mvbalance_store.go 100.00% <100.00%> (ø)
core/block_validator.go 49.55% <75.00%> (+1.37%) ⬆️
core/blockstm/mvstore.go 97.97% <97.97%> (ø)
core/blockstm/mvhashmap.go 91.16% <90.97%> (+11.29%) ⬆️
core/blockchain.go 62.67% <79.64%> (+0.46%) ⬆️

... and 55 files with indirect coverage changes

Files with missing lines Coverage Δ
core/blockstm/invariants_off.go 100.00% <100.00%> (ø)
core/blockstm/mvbalance_store.go 100.00% <100.00%> (ø)
core/block_validator.go 49.55% <75.00%> (+1.37%) ⬆️
core/blockstm/mvstore.go 97.97% <97.97%> (ø)
core/blockstm/mvhashmap.go 91.16% <90.97%> (+11.29%) ⬆️
core/blockchain.go 62.67% <79.64%> (+0.46%) ⬆️

... and 55 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread core/blockchain.go
Comment thread core/blockstm/mvstore.go
Comment thread core/blockchain.go Outdated
cffls and others added 2 commits May 5, 2026 13:49
Three more issues flagged in the inline PR review of #2210.

* core/parallel_state_processor.go: V2 was silently dropping the
  stateless-witness pointer. ProcessBlock wires the witness via
  parallelStatedb.StartPrefetcher("chain", witness, nil), but inside
  V2.Process the prefetcher is restarted with a hard-coded nil for
  the witness slot — and StateDB.StartPrefetcher unconditionally
  overwrites s.witness, so every s.witness != nil-gated collection
  point (CollectStateWitness, CollectCodeWitness, settle-phase trie
  walks) became a no-op for the rest of execution. On
  StatelessSelfValidation and single-block makeWitness paths the
  produced witness landed empty with no error.

  Fix: stash finalDB.Witness() before StopPrefetcher and pass it
  through to the v2-settle prefetcher restart, so the wired pointer
  survives the swap.

  See review comment r3191282978.

* core/state/parallel_statedb.go: SelfDestruct skipped recordWrite
  for the SuicidePath key. FlushToMVStore writes
  (SuicidePath_addr, txIdx, inc, true) for every entry in
  s.destructed, but the key was never appended to s.WriteKeys, so
  MVStore.MarkEstimate / CleanupEstimate could not reach it on
  re-execution. Cross-incarnation invalidation was broken: a stale
  SuicidePath entry from incarnation N survived into N+1's view, and
  a downstream tx that observed it via priorDestructedAt could pass
  validation against state that no longer exists — a state-root
  divergence path. Every other MVStore-targeting writer (SetNonce,
  SetCode, SetState, CreateAccount) already calls recordWrite for
  the same reason; only the destruct path was missed.

  Fix: call s.recordWrite(NewSubpathKey(addr, SuicidePath)) inside
  the !s.destructed[addr] guard, matching the journal-entry guard so
  repeated SelfDestruct in the same tx doesn't append a duplicate.
  Pinned by TestPDB_SelfDestruct_RecordsSuicidePathWrite.

  See review comment r3191282996.

* core/blockchain.go: V2 reader cache hit/miss stats were silently
  dropped. setupBlockReaders called ReadersWithCacheStatsTriple to
  create three independent ReaderWithStats wrappers (prefetch /
  process / parallel) and wired the parallel one into
  parallelStatedb, but reportReaderStats only consumed prefetch and
  process. V2's reads accumulated in the parallel wrapper's atomic
  counters and were discarded each block — and since V2 is the
  primary processor in production, the chain/state/account /
  storage/cache/{hit,miss} meters were essentially empty on the hot
  path.

  Fix: thread parallel through setupBlockReaders' return signature
  and into reportReaderStats. process and parallel both carry the
  roleProcess label and share the same underlying cache, so merge
  their counters into the same meter set rather than introducing a
  new "process_parallel" series.

  See review comment r3191283003.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The diffguard run on commit 4c688e4 flagged 35 surviving mutations
(score 77.7%, T1 logic 84.5%). Triage classified seven as
HIGH-severity — branch / boolean / conditional mutations on V2's
per-tx correctness paths. Add five targeted Tier-1 kill tests for
the five that are deterministically killable:

* TestPDB_EnableReadTracking_InitializesBalAddrs
  Pins the `s.BalAddrs == nil` guard at parallel_statedb.go:335.
  Flipping == to != silently skips the make() on a fresh PDB,
  leaving cap=0 instead of the documented 8 — every recordBalWrite
  reallocates. Test asserts cap >= 8 after EnableReadTracking.

* TestPDB_PriorDestructedAt_RecordsAbsenceRead
  Pins the else-if branch at parallel_statedb.go:531. Removing the
  body drops the absence read recordStoreRead(suicideKey, -1, 0,
  nil); without it, validation can't catch a concurrent prior tx
  destructing addr. Test asserts the absence read appears in
  StoreReads AND that subsequent MVStore writes flip validation
  to invalid.

* TestPDB_Exist_DestructedInBaseReturnsFalse
  Pins the `if suicideIdx >= 0 { ... }` branch at
  parallel_statedb.go:576. Removing the body lets a destructed
  addr fall through to base.Exist and incorrectly return true
  when the account was set up in base. The test seeds the base
  StateDB with code on addr (so the fallthrough path is
  observable) and asserts Exist returns false after a SuicidePath
  write.

* TestPDB_CreateAccount_WritesTrueValue
  Pins the literal `true` at parallel_statedb.go:1014
  (CreateAccount → store.WriteInc). Flipping it to false would
  publish (CreatePath_addr, txIdx, inc, false), defeating the
  value-based fallback in storeReadMatches. Test reads the MVStore
  entry and asserts the value is true.

* TestPDB_DiagnoseBalanceRead_MatchReturnsFalse
  Pins the `false` literal at parallel_statedb_validate.go:215.
  Flipping to true would have a matching balance read produce a
  phantom diagnostic with zero-valued fields; DiagnoseValidation
  would aggregate these as empty "" -category diags. Test asserts
  len(diags) == 0 on a matching read.

The other two HIGH survivors are documented as unkillable in
their current form:

* parallel_statedb.go:751 GetCodeHash `if len(code) == 0` —
  removing the early-return falls through to
  crypto.Keccak256Hash(empty), which equals types.EmptyCodeHash by
  spec. Behaviourally equivalent; can't be killed without locking
  in an internal performance signal.

* v2_executor.go:586 runValidationLoop `cancelled = true` after
  ctx-cancel — the mutation's observable effect (drain runs on
  cancel) is timing-dependent because reexec goroutines exit
  promptly via ctx.Done() in waitForTx/waitForFinal regardless,
  so the post-loop drain completes either way. A deterministic
  kill needs a redesign of the cancel handling.

Each new test was verified to:
  - PASS on unmutated code,
  - FAIL on the corresponding mutated code (sed/python
    in-place mutation, run, restore).

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

sonarqubecloud Bot commented May 5, 2026

Comment thread core/blockstm/mvstore.go
Comment thread core/blockchain.go
Comment thread core/blockstm/mvhashmap.go
@claude
Copy link
Copy Markdown

claude Bot commented May 7, 2026

Code review

Unable to complete review — this PR exceeds GitHub's 300-file API diff limit (500+ files, mostly testdata fixtures under core/blockstm/testdata/), and the CI sandbox restricts the gh api / git fetch commands needed to retrieve the diff locally.

Previous review (2026-05-05): No issues found. Checked for bugs and CLAUDE.md compliance.

Since that review, 6 follow-up commits were pushed:

  • a7c0b00 — address PR review feedback on V2 follow-up
  • 7e99f1f — skip witness-corpus tests cleanly when LFS testdata is missing
  • df8a4bb — fix lint failures
  • 4c688e4 — address inline PR review comments
  • e8e367a — address second round of inline review comments
  • 0c26b08 — kill diffguard Tier-1 survivors on V2 PDB hot paths

These commits address reviewer feedback and fix lint/test issues. A fresh automated review of these changes was not possible due to the access limitations described above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants