Make Receipt and SS write fully async - #4159
Conversation
PR SummaryMedium Risk Overview Giga’s EVM state store opens with Gigasim pre-marshals Tests and RPC harnesses Reviewed by Cursor Bugbot for commit 58eed66. 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).
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4159 +/- ##
==========================================
- Coverage 66.70% 65.39% -1.31%
==========================================
Files 2204 2081 -123
Lines 169840 157648 -12192
==========================================
- Hits 113285 103093 -10192
+ Misses 56414 54414 -2000
Partials 141 141
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Moves the littidx receipt write onto a background writer goroutine bounded by AsyncWriteBuffer, with LatestVersion as the publish watermark and the version markers folded into SetReceipts. The mechanism is clean, but a failed queued write still lets the head advance past the missing block, and the new crash-loss window interacts with startup recovery in a way the diff does not address.
Findings: 1 blocking | 6 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion] No test covers the async writer's failure path — that a queued write's error reaches a later
SetReceiptscaller (andClose), and what the head does afterwards. The two new tests (TestLittIdxSynchronousWriteBuffer,TestLittIdxWriteBufferBoundsLag) only exercise the happy path. Given the head-advance issue above, this is the case most worth pinning. - [suggestion] Narrowing
ReceiptStoreby droppingSetLatestVersion/SetEarliestVersiontraded a compile-time check for a runtime one, and the same anonymous-interface assertion is now copied into four places (evmrpc/setup_test.go,evmrpc/tests/utils.go,sei-db/ledger_db/receipt/receipt_store_test.go,sei-db/bootstrap/recovery_test.go), two of them panicking rather than failing.evmrpc/setup_test.goalready has the right shape inpinReceiptVersions; consider exporting one such helper (e.g. from thereceiptpackage as a test-only export) and having the other three call it, rather than three hand-rolled copies. - [suggestion]
evmrpc/watermark_manager.go:76mins the RPClatestagainstreceiptStore.LatestVersion(), so with the write now queued the advertised chain head trails the real one by the queue depth under write pressure instead of the write back-pressuring the block pipeline. That looks like the intent of the PR, but it is a user-visible change toeth_blockNumber/latestfreshness that neither the PR description nor the config doc mentions; worth stating explicitly somewhere an operator will read it. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
yzang2019
changed the title
|
@seidroid review |
There was a problem hiding this comment.
The async receipt writer is in good shape — the failure latch, the closed-store refusal and the new litt_write_failure_internal_test.go cases address every inline finding from the previous review. What blocks is an unrelated line riding along: DefaultGigaStorageConfig now flips SeparateEVMSubDBs to true, changing giga's on-disk EVM state-store layout with no migration, no detection, and no mention in the PR description.
Findings: 1 blocking | 5 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion]
sei-db/config/toml.go:204-208still documents the receipt store'sasync-write-bufferas "Applies only when rs-backend = "pebbledb"" and "defaults to 100". This PR makes the setting the back-pressure and crash-loss bound for littidx too, and lowers the default to 10, so the template an operator actually reads is now wrong on both counts. Worth carrying over the recovery-cost and RPC-head-staleness wording that the Go doc inreceipt_config.gogained — especially since an existingapp.tomlrendered from the old template still carries a literal100, so the lowered default only reaches fresh configs. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
sei-db/db_engine/pebbledb/mvcc/db.go— whenwal.NewChangelogWALfails,OpenDBreturns the error without closing the Pebble handle it already opened (every other error path in the function does_ = db.Close()first). Leaks the open database on a rare startup failure. Pre-existing; the PR only moved the block inside the!DisableInternalWALguard.
|
@seidroid review |
There was a problem hiding this comment.
The async receipt writer is now in good shape — the admission lock, the failure latch and the new race/close tests close every unresolved finding from the previous reviews, and PinVersions/VersionPinner cleans up the interface narrowing. What blocks is an unrelated set of Pebble tuning constants riding along in mvcc/db.go (4× memtable memory per DB, 4× compaction concurrency) with no mention in the PR description and a stale // 64 MB comment proving the edit was incidental.
Findings: 1 blocking | 5 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion]
sei-db/config/toml.go:204-208still tells operators thatreceipt-store.async-write-buffer"Applies only when rs-backend = "pebbledb"" and "defaults to 100". This PR makes the setting the back-pressure, crash-loss and RPC-head-staleness bound for littidx too, and lowers the default to 10, so the template an operator actually reads is wrong on both counts.sei-db/config/toml_test.go:141actively pins the stale sentence (require.Contains(t, output, "Applies only when rs-backend = \"pebbledb\"")), so the template and that assertion have to move together. Worth carrying over the recovery-cost and watermark wording the Go doc inreceipt_config.gojust gained. This was raised in the previous review and is still unaddressed. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
sei-db/db_engine/pebbledb/mvcc/db.go— whenwal.NewChangelogWALfails,OpenDBreturns the error without closing the Pebble handle it already opened (every other error path in the function does_ = db.Close()first), leaking the open database on a rare startup failure. Pre-existing; this PR only moved the block inside the new!DisableInternalWALguard.
|
@seidroid review |
There was a problem hiding this comment.
The async receipt writer and the giga SS changelog removal are now in good shape: the unrelated Pebble tuning constants that blocked the last revision are gone, the writeLitBlock barrier now waits on the log index rather than just the body, and stateStoreConfigFor puts the changelog guard at the single point both the open and rollback paths pass through. What remains are two non-blocking carry-overs — the orphaned SS changelog/ directory on a giga upgrade, and the receipt async-write-buffer documentation in toml.go that is now wrong on both its backend scope and its default.
Findings: 0 blocking | 5 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion]
sei-db/config/toml.go:204-208still tells operators thatreceipt-store.async-write-buffer"Applies only when rs-backend = "pebbledb"" and "defaults to 100". This PR makes the setting the back-pressure, crash-loss and RPC-head-staleness bound for littidx too, and lowers the default to 10, so the template an operator actually reads is wrong on both counts.sei-db/config/toml_test.go:141pins the stale sentence, so the template and that assertion have to move together. Worth carrying over the recovery-cost and watermark wording the Go doc inreceipt_config.gogained. (The PR description also still says "default 100".) Raised in the two previous reviews and still unaddressed; also raised by Codex. - [suggestion] The gigasim bloom change (
sei-db/bench/gigasim/receipt.go) — replacing keccak-derived bloom bits with an FNV-1a mix, plus the receipt-field cache — is unrelated to making the receipt and SS writes async and is not mentioned in the PR description or its testing section. It is confined to the benchmark harness so nothing in production is affected, but it does change what the benchmark stores: the receipts'LogsBloombytes are no longer what a real log bloom would produce, which is a fidelity trade the description should state alongside the generator speedup it buys. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
sei-db/db_engine/pebbledb/mvcc/db.go:261-262— whenwal.NewChangelogWALfails,OpenDBreturns the error without closing the Pebble handle it already opened (every other error path in the function does_ = db.Close()first), leaking the open database on a rare startup failure. Pre-existing; this PR only moved the block inside the new!DisableInternalWALguard.
| return nil, err | ||
| // An owner that logs every block replays it into this store, leaving the changelog here written | ||
| // and never read. | ||
| if !config.DisableInternalWAL { |
There was a problem hiding this comment.
[suggestion] For a giga node upgrading onto this build, the changelog/ directory already written under the EVM SS directory becomes orphaned: with DisableInternalWAL set, streamHandler stays nil, so PruneWALBeforeVersion (line 317) returns immediately and nothing else touches the directory. Per the comment just below, that changelog is sized at "roughly one snapshot interval of changelog per retained snapshot", so the disk it holds is never reclaimed and never re-read. Note the rollback path does not clean it either — RewindClosedStoreTo/ResetClosedStore only move snapshot files.
Worth either deleting the directory once when the flag is set, or noting in the release notes that operators should remove it by hand after the upgrade.
(Still open from the previous review; also raised by Codex.)
| keccakSet += countBloomBits(keccak) | ||
| } | ||
| // Three bits per value either way, less whatever collides; the collision rates have to agree. | ||
| require.InDelta(t, keccakSet, mixedSet, float64(keccakSet)*0.01, |
There was a problem hiding this comment.
[suggestion] This assertion cannot fail for any bit-picking function, so it does not pin the property its name claims. Each iteration sets three bits into a fresh ethtypes.Bloom, so countBloomBits returns 3 unless two of the three picks collide — for both sides. Over 2048 values that is ~6144 bits each way, with a per-value collision probability around 0.3%, and the tolerance is 1% of the total. A bloomBitsFor that returned, say, bits[i] = uint(i) for every input would pass.
To actually compare density (and therefore how the corpus compresses, which is what the comment says is at stake), accumulate every value into one shared mixed bloom and one shared keccak bloom and compare the saturation of those two — that is where a clustered bit-picker diverges from keccak.
Superseded: latest AI review found no blocking issues.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 58eed66. Configure here.
| return ErrStoreClosed | ||
| } | ||
| seidbmetrics.Send(s.writeQueue, s.writes, write) | ||
| return nil |
There was a problem hiding this comment.
Async receipt write aliases caller data
Medium Severity
SetReceipts now queues the caller’s receipts slice and returns, but the writer later reads those ReceiptRecords (including Receipt pointers and ReceiptBytes) without taking its own copy. Callers that reuse, pool, or Release the source after a successful SetReceipts can have the background write persist mutated or recycled data. The giga commit path encodes records from result, queues them, then returns result to the caller while the store may still be several blocks behind.
Reviewed by Cursor Bugbot for commit 58eed66. Configure here.
|
Successfully created backport PR for |
2 participants
Describe your changes and provide context
Both the receipt write and the EVM state store (SS) write were doing slow synchronous work on the block commit path. This makes both of them fully async.
Receipt store
SetReceiptsused to write the receipt bodies, theeth_getLogsindex and the version marker inline, and the index commit alone was ~74% of the call. It now hands the block to a background writer and returns.Measured at 2,000 receipts per block, the commit path went from 4.0 ms to 11 µs. The work still costs the same; it just happens on the writer, where it overlaps with execution instead of serializing against it.
receipt-store.async-write-buffer(default 100) bounds how many blocks the store may fall behind. A full queue blocks the caller — that is the back-pressure.<= 0keeps writes synchronous, which is the escape hatch if strict read-after-write is wanted.LatestVersion()only advances once a write has actually been applied, so it never advertises a receipt that is not yet readable. It is the watermark a reader follows.EVM state store
enqueue_sslooked async but was dominated by a synchronous changelog WAL write sitting in front of the queue. That is also why its queue depth always read 0: queue depth only reveals a slow consumer, and here the producer was the slow side.Under giga that changelog is written every block and never read — crash recovery replays giga's own state WAL via
catchUpTo, and rollback rewinds SS from its snapshots against that same WAL. So giga now opens SS withDisableInternalWALand the commit-path write is gone.The composite (non-giga) path is untouched and keeps its changelog, which it does need:
ss/compositerollback replays it to reach versions above a snapshot.Interface cleanup
SetLatestVersion/SetEarliestVersionare no longer on theReceiptStoreinterface. No production code called them — the write path carries the markers, and every external caller was test or benchmark scaffolding. cryptosim's redundantSetLatestVersionafter each block is deleted for the same reason.Bug fixed along the way
Draining the pebble async writer on close was nested inside the changelog check:
With the changelog off, that drain would never run, silently dropping queued blocks on every clean shutdown. The drain is now unconditional, behind a
sync.OncesoClosestays idempotent.Dashboard
receipt_write_queue_depthnow covers the whole receipt write. The old "ReceiptDB Queue Depth" panel tracked only litt's table queue, which is ~7% of the call, which is why it read 0 whilewrite_receiptswas a large share of the execution loop.Testing performed to validate your change
sei-db/ledger_db/...,sei-db/state_db/...,sei-db/bootstrap,sei-db/config,sei-db/db_engine/pebbledb/...,giga/evmonly/...,evmrpc/...andx/evm/keeperall pass.-race.make dblintreports 0 issues;go vet ./...is clean.New tests:
TestLittIdxSynchronousWriteBuffer— with the buffer off, a block is queryable the momentSetReceiptsreturns.TestLittIdxWriteBufferBoundsLag— the buffer is the back-pressure point; the store cannot trail further than it allows.TestOpenSSKeepsNoChangelogOfItsOwn— pins the absence of the SS changelog under giga rather than trusting the config. Verified non-vacuous by re-enabling the flag and watching it fail.Tests that previously relied on read-after-write now wait on
LatestVersioninstead. Worth noting for reviewers: that watermark is necessary but not sufficient as a "my write landed" signal — the bodies land just before the version marker commits, and a block written in parts advances the marker on its first part. Thelittidxhelper waits on both.