Conversation
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
PR SummaryHigh Risk Overview The write index is built up front (per-tx, sharded by address), and periodic parallel validation passes scan pending results with Docs in Reviewed by Cursor Bugbot for commit 971643c. 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 @@
## devin/1789943071-stack-3-prepare-pipeline #4273 +/- ##
=============================================================================
+ Coverage 83.02% 83.81% +0.79%
=============================================================================
Files 30 32 +2
Lines 2992 4764 +1772
=============================================================================
+ Hits 2484 3993 +1509
- Misses 507 771 +264
+ Partials 1 0 -1
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 OCC validation and sharded merge look correct: the pre-built span index is equivalent to the old accept-as-you-go index for the only two sourcePrefix values that occur, stmFrontierAccepts reproduces the serial accept condition, shard ownership is disjoint and totally covering, and shard order preserves canonical address order. Only two non-blocking efficiency observations.
Findings: 0 blocking | 2 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion]
occShardOfkeys onaddr[0]only, which is required for order-preserving concatenation but means a single-hot-contract workload puts all of that contract's storage into one shard. Theerc20_single_contractbenchmark this PR adds is exactly that shape, so the sharded merge will parallelize little there — worth calling out in the README alongside the existing shard description so the next reader doesn't expect merge speedup on single-contract blocks. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| if len(results)-from < occMinParallelValidation { | ||
| return 0, nil | ||
| } | ||
| cumulative, to := cumulativeGasFrom(results, from, state.cumulativeGasUsed) |
There was a problem hiding this comment.
[suggestion] cumulativeGasFrom walks and allocates for the entire remaining block (len(results)-from+1 entries) on every parallel pass, on the calling goroutine — i.e. inside the serial barrier this PR is trying to shrink.
The scan in firstUnacceptedResult is self-limiting (workers bail once stop drops below their index), so a pass typically advances only a short distance, but the prefix sum is paid in full regardless. With one conflict roughly every occMinParallelValidation+ transactions the backoff never engages, so you get ~C passes each doing O(N) serial work: O(C·N) total. At N=100k with C≈1k that is ~100M writes and ~800MB of allocation churn per block, all serial.
Capping the pass window (to = min(to, from+window)) would bound both the prefix sum and the atomic stop.Load() contention, at the cost of an extra pass on wide conflict-free runs.
There was a problem hiding this comment.
Not changed here by design: this stack re-opens the already-merged code unmodified so the team can review what actually runs on giga-1 (the stack top equals current giga-1). Noted as a follow-up.
a004fe1 to
a96c76d
Compare
a96c76d to
971643c
Compare
Describe your changes and provide context
Re-opens #4261 on top of #4272 (stack 4/4). With this PR applied the tree is byte-identical to today's
giga-1.OCC acceptance stays a serial barrier in block order, but the work behind it is spread across the existing worker pool (
occ_shards.go):stateAccessIndex);conflictsWithin) and reports the first one the frontier would not accept (firstUnacceptedResult); the frontier handles only that tx on the calling goroutine, then reruns from there (with aserialBackoffto avoid thrashing on hot contracts);occShardOf: contiguous address ranges), each incarnation recording which shards it touched so workers skip results holding nothing of theirs;mergeOCCResultsemits the changeset one shard at a time on the pool and concatenates shards in canonical address order, so the output is identical to the serial merge; a prefix with few keys is merged on the calling goroutine.Determinism: acceptance decisions and the emitted changeset/receipts are the same as the serial path by construction (parity tests compare both).
Testing performed to validate your change
scripts/ramtest.sh -race ./giga/evmonly/...— parity vs sequential validation/merge,conflictsWithinbounds,occShardOf,cumulativeGasFrom,firstUnacceptedResult, boundary rejection atresults[0].make fmtcheck,make lint.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