evmonly: start the receipt write as soon as execution returns; the state commit waits for it - #4266
Conversation
…ate commit waits for it 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 SummaryMedium Risk Overview The refactor splits Reviewed by Cursor Bugbot for commit 1644b9b. 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## giga-1 #4266 +/- ##
===========================================
+ Coverage 65.55% 91.59% +26.03%
===========================================
Files 2081 20 -2061
Lines 157460 3081 -154379
===========================================
- Hits 103222 2822 -100400
+ Misses 54097 259 -53838
+ 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.
Moving the receipt write off the block tail is correctly ordered: writes chain on the previous block's done, startPipelineCommit still gates the state commit on receipts.done, and the phase-timer handoff (Reset before close(done)) keeps the new receiptPhases single-threaded. Two non-blocking notes: the new timer reuses the evmonly_pipeline metric name while now recording concurrently with it, and the new failure path this PR introduces (block fails after the receipt handoff) has no test.
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.
…lose draining a failed block's receipt write Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
@seidroid review |
There was a problem hiding this comment.
Hoisting the receipt write ahead of the block tail is correctly ordered: receipts stay in block order via the previous.done chain, the state commit still gates on receipts.done, the phase timer handoff (Reset before close(done)) keeps receiptPhases single-threaded, and Close now drains a write with no commit behind it before occPool.Close(). Three non-blocking notes: an untested new chained-failure branch, a godoc that understates how long the pooled result is retained, and an existing evmonly_pipeline phase name whose meaning changes silently.
Findings: 0 blocking | 3 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] The
evmonly_pipelinephase nameawait_receiptsis reused with a different meaning: before this PR it was the litt-landing wait insidepersistReceipts(nowawait_store, understage="receipts"), and it now names the state-commit goroutine's wait for the receipt write (stage="state"). Any existing panel keyed onphase="await_receipts"keeps rendering but silently plots a different quantity, since the old series has nostagelabel to distinguish it from the new one. The PR body documents the change, but a distinct name (e.g.await_receipt_write) would make the break visible as a gap in the panel instead of a shifted line. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
…e; test the write after a failed one; state the result's retention window Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
On the phase-name note: renamed the state commit's wait to |
…ner) Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
… skips the receipt write Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Describe your changes and provide context
Since #4264 the receipt encode+write for block N is kicked off at the very end of
ExecutePreparedBlock(start_commit), andCommitthen waits for it to land before advancing the RPC-visible head. On testnet-2 that wait shows up asapp_commit≈ 0.16 s/s: the litt writer's ~3.5 ms/block used to trail off-loop and now sits on the critical path, because nothing overlaps it.This PR starts the receipt write the moment execution returns, so it runs under the rest of the block's tail (block-changeset encoder,
await_commitfor block N-1,start_commit) and the router's tail (vault commit, PushAppHash) instead of after them. The head-advance invariant is unchanged:Commitstill callsAwaitReceiptsand the state commit still waits for the block's receipts before committing, so the store never holds a block whose receipts cannot be read.startReceiptWrite:LatestVersionkeeps meaning "every block up to here"; a failed previous write fails this one too (the pipeline is already latched at that point).pipelineReceiptsimmediately, soAwaitReceiptsfinds it even if the block later fails on the loop (block encoder error, ctx cancel).Closenow also waits on it, since such a block has no commit to drain. Receipts for a block that failed after execution can therefore be persisted ahead of state — same class as the existing "state failure leaves receipts behind" case, healed by re-execution on restart; README updated.startPipelineCommitno longer takes a ctx or retains the result; the receipt goroutine owns the retention.Metrics: receipt phases move to their own
PhaseTimer, built from the sameevmonly_pipelinefactory with astage="receipts"label (the state commit's timer getsstage="state"): the receipt write now overlaps block N-1'scommit_state, so one timer is not goroutine-safe (the race detector caught this on the first cut) and the two record overlapping intervals — filter onstagefor a share-of-clock panel. Phase names:encode_receipts/write_receiptsunchanged; the litt-landing wait is nowawait_store; the oldawait_receiptsis retired;await_receipt_write(stage="state") is the state-commit goroutine's wait for the receipt write (i.e. the part of the receipt write that did not overlap). Newstart_receiptsblock phase (handoff only).Independent of #4265 (receipt store optional); the two touch
startPipelineCommitso whichever lands second gets a small rebase.Expected effect: recovers the part of the 0.16 s/s
app_commitwait that the ~2 ms loop+router tail can hide; RPC nodes benefit regardless of whether validators turn receipts off.Testing performed to validate your change
TestReceiptWriteStartsBeforeThePreviousCommitLands;TestCloseWaitsForTheReceiptsOfABlockThatFailedAfterHandingThemOffpins thatClosedrains the receipt write of a block that failed on the loop after the handoff (no state commit exists to drain it through): with a store-reading block encoder (loop must wait for N-1's commit) and N-1's commit held, block N's receipts are written while N'sExecutePreparedBlockis still blocked; N returns only after release; commits land in order.AwaitReceiptsbefore commit lands, encoder sees own changes,ReadLatestAccountretiring cases.go test -race -count=5 ./giga/evmonly/,go test -race ./giga/evmonly/... ./sei-tendermint/internal/evmonlyapp/...green.make fmtcheck,golangci-lint run ./giga/evmonly/...(0 issues),go build ./....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