Backport giga-1: Make Receipt and SS write fully async - #4187
Conversation
## 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
`SetReceipts` used to write the receipt bodies, the `eth_getLogs` index
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.
- Setting it to `<= 0` keeps 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_ss` looked 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 with `DisableInternalWAL` and the commit-path write is gone.
The composite (non-giga) path is untouched and keeps its changelog,
which it does need: `ss/composite` rollback replays it to reach versions
above a snapshot.
### Interface cleanup
`SetLatestVersion` / `SetEarliestVersion` are no longer on the
`ReceiptStore` interface. No production code called them — the write
path carries the markers, and every external caller was test or
benchmark scaffolding. cryptosim's redundant `SetLatestVersion` after
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:
```go
if db.streamHandler != nil {
close(db.pendingChanges)
db.asyncWriteWG.Wait()
...
}
```
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.Once` so `Close` stays idempotent.
### Dashboard
`receipt_write_queue_depth` now 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 while `write_receipts` was 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/...` and `x/evm/keeper` all pass.
- The receipt package passes three repeats under `-race`.
- `make dblint` reports 0 issues; `go vet ./...` is clean.
New tests:
- `TestLittIdxSynchronousWriteBuffer` — with the buffer off, a block is
queryable the moment `SetReceipts` returns.
- `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
`LatestVersion` instead. 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. The `littidx` helper waits
on both.
(cherry picked from commit c45517d)
PR SummaryHigh Risk Overview Receipt store ( Giga Gigasim/cryptosim pre-marshal receipts in the block generator, stop bumping receipt version separately, and drop the old “encode vs store” receipt-write phase metrics. EVM RPC test harnesses pin receipt query windows via Reviewed by Cursor Bugbot for commit 31e4530. Bugbot is set up for automated code reviews on this repo. Configure here. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## giga-1 #4187 +/- ##
==========================================
- Coverage 65.55% 65.46% -0.10%
==========================================
Files 2081 2078 -3
Lines 157460 157286 -174
==========================================
- Hits 103222 102960 -262
- Misses 54097 54185 +88
Partials 141 141
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
![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 (bodies, log index and version marker) onto a bounded background queue and turns off the pebble mvcc changelog for giga's EVM state store, with matching test, config and dashboard updates. The concurrency design (admission RWMutex, latched write failure, drain-on-Close) holds up and recovery is correctly integrated via findTargetRecoveryHeight/PruneAfter; the notes below are non-blocking.
Findings: 0 blocking | 7 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] Recovery cost after an unclean exit changes shape: before this PR the receipt head was written synchronously ahead of the state commit, so it was never the lowest head in
findTargetRecoveryHeight. It now routinely trails by up toAsyncWriteBufferblocks, which means essentially every crash restart triggers a state rollback (SC/SS snapshot restore plus WAL replay) that previously did not happen. Worth confirming the deployed SC/SS snapshot interval and state-WAL retention always span that window, sincediscardStateAbove/requireReplayablerefuses to open rather than degrade if they do not. - [suggestion]
receipt-store.async-write-bufferchanges meaning rather than gaining a new key, so the new default of 10 only reaches nodes with no app.toml entry. Any app.toml rendered fromsei-db/config/toml.goalready carriesasync-write-buffer = 100, and those nodes get a 100-block receipt lag on the RPC head and a 100-block recovery setback with no signal that the key now means something different. Consider clamping the littidx queue independently of the key, or calling it out in release notes. - [suggestion] With
DisableInternalWALset,streamHandlerstays nil, so thechangelogdirectory an earlier giga build left under the EVM state store directory is now never written, never read and never pruned (TruncateBefore/the changelog-range helpers both return early on a nil handler). It is bounded by whateverchangelogKeepRecentleft there, but it is dead disk on every upgraded node; deleting it on open (or documenting that operators may remove it) would close that out. - 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.goOpenDB: thewal.NewChangelogWALerror path returns without closing the already-opened pebble DB, leaking the handle and its directory lock. Present on the base branch; this PR only moves the block under the newif !config.DisableInternalWALguard.
Inline comments (could not post inline; listed here)
sei-db/ledger_db/receipt/litt_receipt_store.go:344(RIGHT) -- [suggestion] The synchronous path returns before the newclosingguard, so theErrStoreClosedcontract only holds whenAsyncWriteBuffer > 0. WithAsyncWriteBuffer <= 0(a documented operator setting — "Set <= 0 for synchronous writes"), aSetReceiptsthat arrives afterCloserunsapplyReceiptsagainst an already-closed litt DB and pebble index, which surfaces as a pebble "closed" panic rather than a clean error.TestWriteAfterCloseIsRefusedonly exercises the async store, so this gap is untested.
Taking the admission read lock and checking closing before dispatching on s.writes == nil would put the guard on the single path every write passes through, and would let the two refusal tests cover both configurations.
docker/monitornode/dashboards/gigasim-dashboard.json:10330(RIGHT) -- [suggestion] These two panels (panel-131here andpanel-132below) still describereceipt_store_write_phase_duration_seconds_totalas "what the execution loop's write_receipts phase is made of", and their titles are indented as children of that phase. After this changewrite_receiptson the execution loop is just the enqueue;probe_part_index/stage_tag_keys/commit_indexare recorded on the background writer, so these bands no longer subdivide the parent and no longer sum to it. The "committed inline" wording is also now misleading — the index commit is inline within the writer, not within the caller.panel-23's description got exactly this treatment in the same diff; these two look like they were missed.sei-db/state_db/giga/state_db_replay_test.go:27(RIGHT) -- [suggestion] Both sub-tests build theStateDBby hand and applystateStoreConfigForthemselves, so together withTestStateStoreConfigForDisablesTheInternalWALthey pin that the helper sets the flag and that a store opened with the flag keeps no changelog — but nothing pins thatNewStateDBandNewStateDBWithRollbackactually route their config through the helper. Dropping either call site (the regression the comment says recovery rests on) leaves this file green.
Driving at least the "opened to commit" case through NewStateDB, or asserting NewStateDBWithRollback's resulting ssCfg.DisableInternalWAL, would cover the constructors the comment is about.
|
@seidroid review |
There was a problem hiding this comment.
Backport of #4159: moves the littidx receipt write onto a bounded background queue, drops the version markers from the ReceiptStore interface in favour of SetReceipts/PinVersions, and turns off pebble's mvcc changelog for giga's EVM state store. The concurrency design (admission RWMutex, latched write failure, drain-on-Close) and the stateStoreConfigFor choke point hold up; the notes below are non-blocking, and four of them are earlier findings that the current head still carries.
Findings: 0 blocking | 9 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion]
sei-db/config/toml.go:204-208still renders the old guidance forasync-write-buffer: "Applies only when rs-backend = "pebbledb"" and "defaults to 100". Both are now wrong — littidx honours the key as of this PR, and the default is 10 — and the rendered block is the only version an operator reads, so the cost the Go doc comment now spells out (recovery setback, RPC head lag) never reaches them. - [suggestion]
receipt-store.async-write-bufferchanges meaning rather than gaining a new key, so the new default of 10 only reaches nodes with no app.toml entry. Any app.toml rendered fromsei-db/config/toml.goalready carriesasync-write-buffer = 100, and on those nodes the key now sizes a queue that holds up to 100 blocks of receipt records in memory, caps the RPC safe-latest watermark (evmrpc/watermark_manager.go:76) 100 blocks back under load, and sets recovery back 100 blocks after an unclean exit. Clamping the littidx queue independently of the key, or calling the reinterpretation out in release notes, would close that. - [suggestion] Recovery cost after an unclean exit changes shape: the receipt head was previously written synchronously ahead of the state commit, so it was never the lowest head in
findTargetRecoveryHeight. It now routinely trails, so essentially every crash restart triggers a state rollback (SC/SS snapshot restore plus WAL replay) that previously did not happen. Worth confirming the deployed SC/SS snapshot interval and state-WAL retention always span the queue depth, sincediscardStateAbove/requireReplayablerefuses to open rather than degrade when they do not. - [suggestion] With
DisableInternalWALset,streamHandlerstays nil, so thechangelogdirectory an earlier giga build left under the EVM state store directory is now never written, never read and never pruned (PruneWALBeforeVersionandWALVersionsAfterboth return early on a nil handler). It is bounded by whateverchangelogKeepRecentleft there, but it is dead disk on every upgraded node; deleting it on open, or documenting that operators may remove it, would close that out. - 4 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.goOpenDB: thewal.NewChangelogWALerror path returns without closing the already-opened pebble DB, leaking the handle and its directory lock. Present on the base branch; this PR only moves the block under the newif !config.DisableInternalWALguard.
Inline comments (could not post inline; listed here)
sei-db/ledger_db/receipt/litt_receipt_store.go:344(RIGHT) -- [suggestion] The synchronous path returns before the newclosingguard, so theErrStoreClosedcontract only holds whenAsyncWriteBuffer > 0. WithAsyncWriteBuffer <= 0(a documented operator setting — "Set <= 0 for synchronous writes"), aSetReceiptsthat arrives afterCloserunsapplyReceiptsagainst an already-closed litt DB and pebble index, which surfaces as a pebble "closed" panic rather than a clean error.TestWriteAfterCloseIsRefusedandTestWriteAfterCloseIsRefusedWithAFullQueueonly exercise the async store, so this gap is untested.
Taking the admission read lock and checking closing before dispatching on s.writes == nil would put the guard on the single path every write passes through — the choke point rather than one of two branches — and would let the two refusal tests cover both configurations.
(Raised on the previous review and still open; Codex flags the same line, classifying the underlying panic as pre-existing. The panic is, but the guard that now fails to cover it is new here.)
sei-db/ledger_db/receipt/litt_receipt_store.go:350(RIGHT) -- [suggestion] A head of 0 now has two meanings, and recovery reads only one of them.recoveryTarget(sei-db/bootstrap/recovery.go:154) deliberately leaves a receipt head of 0 out of the minimum, so that receipts newly enabled on a node with history start filling at the target. Before this PR the receipt head was stamped synchronously, so an enabled store could not read as 0 once any block had committed; now a brand-new store can have its first up-to-AsyncWriteBufferblocks queued and unapplied while the state and block heads have advanced. An unclean exit in that window comes back withreceiptHeight == 0, the exemption fires,targetis the state head, no rollback runs, and those blocks keep their state with no receipts and no path to regenerate them —eth_getTransactionReceiptandeth_getLogsserve a silent hole under a watermark that says the range is complete.
Narrow (fresh chain or freshly wiped receipt store, plus a crash in the first few blocks), which is why this is not marked blocking, but distinguishing an enabled-but-never-written store from an intentionally absent one would close it. Codex flags the same gap.
docker/monitornode/dashboards/gigasim-dashboard.json:10330(RIGHT) -- [suggestion] This description (panel-131 here, and panel-132 at line 10415 with the same text) still saysreceipt_store_write_phase_duration_seconds_totalis "what the execution loop's write_receipts phase is made of", and both titles are indented as children of that phase. After this changewrite_receiptson the execution loop is just the enqueue:probe_part_index/stage_tag_keys/commit_indexare recorded fromapplyReceiptson the background writer, so these bands no longer subdivide the parent and no longer sum to it. "committed inline" is misleading for the same reason — the index commit is inline within the writer, not within the caller. Panel-23's description got exactly this treatment in the same diff (line 372); these two look like they were missed.
(Raised on the previous review and still open.)
sei-db/state_db/giga/state_db_replay_test.go:27(RIGHT) -- [suggestion] Both sub-tests build theStateDBliteral by hand and applystateStoreConfigForthemselves, so together withTestStateStoreConfigForDisablesTheInternalWALthey pin that the helper sets the flag and that a store opened with the flag keeps no changelog — but nothing pins thatNewStateDBandNewStateDBWithRollbackactually route their config through the helper. Dropping either call site (state_db.go:80orstate_db.go:142) — the regression the comment above says recovery rests on — leaves this file green.
Driving at least the "opened to commit" case through NewStateDB, or asserting the resulting ssCfg.DisableInternalWAL on a StateDB returned by NewStateDBWithRollback, would cover the constructors the comment is about.
(Raised on the previous review and still open.)
3 participants
Backport of #4159 to
giga-1.