evmonly: parallelize OCC validation and merge behind the serial acceptance barrier - #4261
Conversation
…tance barrier Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
PR SummaryHigh Risk Overview Validation indexes all speculative writes up front into a 64-way address-sharded write index that tracks first/last tx index spans per key. Conflict checks move from “any write at or after Merge shards Docs, shard/unit tests, a large mixed-block OCC vs sequential parity test, and an ERC-20 single-contract load benchmark case exercise the new paths. Reviewed by Cursor Bugbot for commit 592e38f. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## giga-1 #4261 +/- ##
===========================================
+ Coverage 65.55% 84.69% +19.14%
===========================================
Files 2081 27 -2054
Lines 157460 4358 -153102
===========================================
- Hits 103222 3691 -99531
+ Misses 54097 667 -53430
+ Partials 141 0 -141
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
The parallel validation/merge redesign is correct as far as I can verify: the [sourcePrefix, txIndex) span check is equivalent to the old accepted-only index (since sourcePrefix is only ever 0 or txIndex), firstUnacceptedResult genuinely returns the minimum stop index, and shard ownership is a disjoint partition with no races. The findings are performance regressions for small and storage-heavy blocks, plus a test-coverage gap around the new helpers.
Findings: 0 blocking | 4 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] Sharding by
addr[0]gives no parallelism for the case the merge is most likely to be slow on: a block dominated by one hot contract's storage. All of that contract'sstorageentries land in a single shard, sochangeSetIntoParalleldegenerates to serial for the storage portion (which also carries the sort and the per-slotsource.GetStatereads) while still paying the fan-out and fragment allocations. The benchmark in the description (BenchmarkExecuteTransferBlock) is the opposite shape — many distinct EOAs — so it doesn't exercise this. Worth benchmarking a contract-heavy block before rollout; if it regresses, storage could be sub-sharded by slot range within an address, which preserves the canonical-order-by-concatenation property. - [suggestion] None of the new helpers in
occ_shards.gohave a direct unit test:conflictsWithin/writtenWithinbounds,occShardOfmonotonicity across shard boundaries,cumulativeGasFrom's overflow truncation,firstUnacceptedResultreturning the true minimum under concurrency, and theserialUntilbackoff are all covered only transitively byTestExecutorOCCLargeMixedBlockMatchesSequential. Notably, the three existing conflict tests were updated by passingmath.MaxIntastxIndex, which disables the upper bound this PR introduces — so the newhisemantics have no unit-level coverage at all. A small table test asserting that a write at an index >= txIndex does not conflict, and that one in[sourcePrefix, txIndex)does, would pin the central semantic change of the PR. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
… threshold; shard helper tests Each incarnation records the shards its writes, credits and changes touch, so index/apply workers skip results that hold nothing of theirs instead of scanning every result's maps. The changeset merge reuses pooled per-shard fragments and stays on the calling goroutine when the prefix holds fewer than 256 keys. Adds direct tests for conflictsWithin bounds, occShardOf, cumulativeGasFrom, firstUnacceptedResult and touchedShards, and an ERC-20 single-contract case to the block benchmark. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
@seidroid review |
There was a problem hiding this comment.
The parallel validation/merge redesign holds up: the [sourcePrefix, txIndex) span check is exact for the only two sourcePrefix values that occur, firstUnacceptedResult provably returns the minimum stop index, shard ownership is a disjoint partition with block-order preserved per shard, and the previous round's small-block and fragment-allocation findings are genuinely fixed. Two non-blocking items: a readability/structure point in the new backoff loop and a no-op line that voids one test assertion.
Findings: 0 blocking | 2 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…the shard-3 runner; privval untouched here) Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Describe your changes and provide context
On giga-testnet-2,
occ_validate+occ_mergeare ~34% of main-loop wall time and ran entirely on the calling goroutine, while conflicts are ~0.85 per ~1,800-tx block. Nearly all of that time is bookkeeping for transactions that are accepted as they stand. This PR keeps acceptance strictly in block order (the same frontier, same rerun/incarnation/fallback rules) but moves the work behind it onto the existingoccWorkerPool.Validation (
validateBlockSTM):stateAccessIndex) is now sharded by address (64 contiguous address ranges) and records atxIndexSpan{first,last}per key instead of a single index. All speculative results are indexed up front in parallel, each worker filling the shards it owns.conflictsWithAfter(key, sourcePrefix)toconflictsWithin(key, sourcePrefix, txIndex): a write invalidates a result only if it sits in[sourcePrefix, txIndex). With every result indexed, this is the same question the serial frontier asked, because when the frontier is atievery write belowiis an accepted one. A rerun's writes are added; the previous incarnation's stay. Both the span and the stale entries can only produce false positives (an unnecessary rerun, which then validates trivially against its exact prefix), never a missed conflict.acceptValidatedPrefixruns a parallel pass over[nextToValidate, n): workers evaluatestmFrontierAccepts(error, gas, read/write conflicts against the precomputed cumulative gas) and lower a sharedstopto the first index that would not be accepted. Everything beforestopis folded into the prefix withapplyRange(each worker applies the shards it owns; per-address state lives entirely in one shard, so per-shard order equals block order). The serial frontier then handles only the transaction atstop— recording conflicts, scheduling the rerun, or surfacing the gas/execution error exactly as before.occTxExecution.shards, computed on the worker that produced it), so index/apply workers skip whole results that hold nothing in their shards instead of scanning every result's maps.serialUntil), so worst-case blocks don't pay pool wake-ups per transaction. Blocks under 64 pending results skip the pool entirely.Merge (
mergeOCCResults):blockSTMStateis sharded the same way;changeSetIntoParallelcomputes each shard's changeset (with its ownbaseAccountsrow cache, so each account is still read once) on the pool into pooled per-shard fragments and concatenates shards in order, which is canonical address order — output is byte-identical to the serialChangeSetInto. A prefix underoccMinParallelMergeKeys(256) keys is merged serially straight into the pooled changeset. The tx/receipt loop (cumulative gas, log indexes) stays serial and unchanged.prefetchBaseAccountspass is subsumed: base rows are now read inside the per-shard merge.errOCCWorkerPoolClosedsites.What does not change: transaction order, cumulative gas / block gas-limit / overflow checks,
occMaxTxIncarnations, only-earliest-invalid-rerun, sequential fallback reasons, final changeset semantics. Conflict counts are still deterministic (a pure function of the block); they can be slightly higher than before due to span false positives.Known shape: a hot contract's storage lands in one shard, so that shard's merge/apply runs on one worker.
BenchmarkExecuteTransferBlockgained anerc20_single_contractcase (1,000 transfers on one ERC-20) to keep that measured: 19.9 → 18.6 ms/op vsgiga-1. Whole-block local benchmarks (8 cores, incl. parse — parse is ~50% of it):conflict_free15.1 → 12.9 ms/op,hot_recipientandsame_sender_nonce_chainunchanged within noise, allocations flat. The rollout check isocc_validate+occ_mergeshare of the main loop on testnet-2 and executed tx/s.Testing performed to validate your change
TestExecutorOCCLargeMixedBlockMatchesSequential: ~330-tx block (independent transfers, same-sender nonce chains, hot recipient, a late reader of the hot balance that must be rerun after the parallel pass) — assertsTxs,Receipts,GasUsedand the fullChangeSetequal the sequential executor's, and that a rerun actually happened.TestParallelMergeResolvesEveryTouchedAccountOnce(replaces the prefetch tests): parallel merge output equals serial, canonical order, one row read per account.occ_shards_test.go:conflictsWithinlower/upper bounds and span over-approximation,occShardOfmonotonic and covering,occShardsOwnedBypartitioning,cumulativeGasFromoverflow truncation,firstUnacceptedResultreturning the lowest rejection under concurrency (error, gas limit, conflict, source-prefix),touchedShards, and the serial-merge guard against a closed pool.txIndexparameter.go test -race ./giga/evmonly/...and./sei-tendermint/internal/evmonlyapp/...green;make fmtcheckclean;golangci-lint run ./giga/evmonly/...0 issues (v2.13.2, Go 1.27.1).Link to Devin session: https://app.devin.ai/sessions/ff612badcded4aa5914ea408dbb41888
Open in Devin Desktop: https://app.devin.ai/desktop/session/ff612badcded4aa5914ea408dbb41888?variant=devin
Requested by: @bdchatham