evmonly: persist receipts and encode state changes behind the block; Commit advances the head once the receipts have landed - #4264
Conversation
…Commit waits for the receipts to land 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
Reviewed by Cursor Bugbot for commit c6d9b4e. 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 #4264 +/- ##
===========================================
+ Coverage 65.55% 86.47% +20.91%
===========================================
Files 2081 36 -2045
Lines 157460 4871 -152589
===========================================
- Hits 103222 4212 -99010
+ Misses 54097 658 -53439
+ Partials 141 1 -140
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.
Moving receipt persistence and state-changeset encoding behind the block loop is implemented coherently: the pipeline goroutine starts only after the previous block's commit landed (so the state encoder still sees "every earlier block and none of this one" without the old encodingReadsTheStore special case), the pooled result is refcounted across the loop and the background write, Close still drains the goroutine, and Commit gates head advance on AwaitReceipts. No blocking defects found; three non-blocking notes, mainly around the new WaitForPendingWrites contract and coverage of the new "did not land" branch.
Findings: 0 blocking | 3 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] The new "receipts did not land" branch in
persistReceipts(giga/evmonly/giga_store.go:379) is untested.TestFailedReceiptWriteFailsTheBlockBeforeItsStateCommitcovers aSetReceiptserror return, not the case this branch exists for — a write admitted to the litt queue and then skipped byapplyWriteafter a latched failure, wherelandedcloses butLatestVersionstays short of the block. That path is what makes a dropped write surface on this block rather than the next one, and it is the difference between a halted node and a silently missing receipt range. - [suggestion]
persistReceiptsverifies the landing only when the receipt store implementsseidbtypes.PendingWriteWaiter; for any other store it assumes thatSetReceiptsreturning means the receipts are readable. The pebble-backedreceiptStoreapplies viaApplyChangesetAsyncand does not implement the interface, so the PR's headline invariant (Commitreturns only once the block's receipts are readable) would silently not hold there. Giga hardcodeslittidxinDefaultGigaStorageConfig, so this is latent rather than live, but aWaitForPendingWritesonreceiptStoredelegating tos.db(which already implements it — seesei-db/ledger_db/receipt/receipt_store.go:337) would remove the silent-degradation mode. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
…s WaitForPendingWrites; test a write that never lands Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
@seidroid review |
There was a problem hiding this comment.
Moving receipt persistence and state-changeset encoding behind the block loop is implemented soundly: the pipeline goroutine is strictly serialized behind the previous block's done, refcounting on the pooled result and the landed/lastQueued markers in littReceiptStore are correctly ordered, and all three previous review notes are addressed. No blocking defects; two non-blocking notes about the new Commit wait landing inside the mempool-locked section and stale contract text in giga/evmonly/README.md.
Findings: 0 blocking | 3 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion]
giga/evmonly/README.mdnow contradicts the executor's behaviour. Lines 92–94 ("Encoding, state commit, or receipt-store failures release the block result and return an error without invokingResultSink") and line 99 ("ResultSinkruns only after both stores succeed") were written for the serial path. State encoding and receipt persistence now run in the background goroutine started bystartPipelineCommit, whichExecutePreparedBlockcalls beforesinkBlockResult— this PR's ownexecutor_test.gochange asserts the opposite of the README (require.Len(t, sink.results, 1)on a receipt-store failure, with the comment "the sink has already seen the result"). The surrounding sentences that are still accurate (receipts before the height-advancing state commit, a receipt failure leaving state unchanged) make the stale ones easy to trust. - 1 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]
giga/evmonly/README.md:84states "The snapshot stays open through the commit and is always closed afterward." Thedefer snapshot.Close()inexecutePreparedBlockWithStoreruns when the function returns, which since the state commit was backgrounded is beforeCommitStateChangescompletes. Already stale on the base branch, not introduced here.
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
README notes taken in c6d9b4e: |
Describe your changes and provide context
Third of the three execution-side PRs (after #4262, #4263). Takes the rest of the executor's storage tail off the block loop, the way #4254 did for the state commit, while keeping "a block RPC serves has readable receipts".
Before, after execution the loop did, serially: encode state changesets → block encoder → encode receipts →
SetReceipts→ await previous commit → start this block's commit in the background. On testnet-2 the two encodes were ~0.08 s/s of loop time.After, the loop does: [await previous commit] → block encoder (stays on the loop: what it stages is what the caller reports for the block) → await previous commit → start the pipeline → return. The pipeline goroutine then, per block, in order:
NamedChangeSetEncodernow runs after the previous block has landed and before this one starts, so an encoder that reads the store (the storage-clear expansion) sees every earlier block and none of this one; theencodingReadsTheStorespecial case that used to serialize such blocks is gone. It runs on a clone, so the pooled result is free as soon as receipts are encoded.Head advances after receipts land.
evmOnlyApplication.Commitcalls the newExecutor.AwaitReceipts()before advancing the cursor. The router publishes the block (and RPClatestfollows it) only afterCommit, solatestnever points at a block whose receiptseth_getTransactionReceipt/eth_getBlockReceiptscannot read. Previously the litt store's own async writer meant receipts could trail the visible head by up to its queue depth; that window is closed, not widened.To make "land" real rather than "enqueued",
littReceiptStorenow implements the existingseidbtypes.PendingWriteWaitercapability: each queued write carries alandedchannel the writer closes after applying it (or skipping it after a latched failure), andWaitForPendingWriteswaits on the last admitted one. The executor assertsLatestVersion() >= blockafterwards so a write dropped after a latched failure is reported for this block rather than by the nextSetReceipts.Failure semantics are unchanged in shape: a receipt or state persistence failure latches (
pipelineFailure),Commitreturns it if the receipt stage failed, and the nextFinalizeBlockreturns it otherwise (persist block: …). Once latched the executor refuses further blocks. Cancelling the request that ran the block no longer abandons its persistence (context.WithoutCancel):Closewaits for it, and the block's outcome is reported through the pipeline rather than by dropping it.Cost to watch after the roll.
Commitnow waits for this block's receipt encode + litt write, which overlap the router'svault_commit(#4263) but no longer with the next block's execution. Newevmonly_pipelinephase timer (encode_receipts,write_receipts,await_receipts,encode_changesets,commit_state) and the loop'sevmonly_blocktimer (encode_block_changesets,await_commit,start_commit) show where the time went; ifawait_receiptsis large the litt write itself is the next target.Testing performed to validate your change
giga/evmonly:TestAwaitReceiptsReturnsBeforeTheStateCommitLands(receipts readable and pool lease returned while the state commit is still gated),TestFailedReceiptWriteFailsTheBlockBeforeItsStateCommit(no state commit after a failed receipt write; failure surfaces),TestBackgroundEncoderSeesTheBlocksOwnChanges(encoder gets a clone, not the recycled pooled result); existing store/pipeline/overlay/determinism tests unchanged exceptTestExecutorReturnsReceiptStoreError, updated for the write now landing behind the block and latching.sei-db/ledger_db/receipt:TestLittIdxWaitForPendingWritesLandsEveryQueuedBlock.sei-tendermint/internal/evmonlyapp:TestEVMOnlyApplicationCommitReturnsWithTheBlocksReceiptsReadable(receipt for height N readable from the store the momentCommitreturns, N=1..3); existing failed-commit fallback and reopen tests unchanged.scripts/ramtest.sh ./giga/evmonly/... -race,./sei-db/ledger_db/receipt/ -race,go test -race -count=8 ./internal/evmonlyapp/,go vet,golangci-lint runon touched packages,make fmtcheck— all green.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