Conversation
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
PR SummaryMedium Risk Overview The app keeps a single prepared slot keyed by
Tests cover prepared vs unprepared parity, wrong-hash ignores, concurrent prepare/finalize, and router proposer checks on delivered blocks. Reviewed by Cursor Bugbot for commit 453eb65. 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-2-pending-overlay #4272 +/- ##
============================================================================
- Coverage 90.90% 83.02% -7.89%
============================================================================
Files 28 30 +2
Lines 4091 2992 -1099
============================================================================
- Hits 3719 2484 -1235
- Misses 372 507 +135
- Partials 0 1 +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 two-stage fetch/prepare pipeline and the preparedBlock slot are correct — (height, hash) guarding means a wrong or stale preparation only falls back to inline decoding, and Executor.PrepareBlock is genuinely stateless so running it beside FinalizeBlock is safe. The main concern is that the handoff between the fetcher and the execute loop is unsynchronized, so the optimization can silently not apply.
Findings: 0 blocking | 4 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] No test pins that the pipeline actually produces prepared hits.
TestEVMOnlyApplicationPreparesTheNextBlockWhileFinalizingTheCurrentOneasserts determinism but never readsevmonly_finalize_prepared_blocks_total, and in that testPrepareBlock(n+1)racesFinalizeBlock(n)exactly as in production — so its own hit/miss split is nondeterministic and a regression to always-miss would stay green. The p2p router test (CheckBlocks) likewise only checks heights and proposers. Consider asserting the hit counter somewhere the handoff is deterministic. - [suggestion] The PR description does not match the implementation: it describes fetching n+1 with a non-blocking
TryBlockover the contiguous prefix, butrunExecutespawns a fetcher that blocks inr.data.GlobalBlock(n); it says FinalizeBlock matches "height and tx hashes" when the code matches height andreq.Hash; and it names{hit|miss}labels where the counter usesprepared=true|false. Worth updating so the description describes the mechanism that landed. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| return fmt.Errorf("r.data.GlobalBlock(%v): %w", n, err) | ||
| } | ||
| req := r.finalizeRequest(b) | ||
| if err := app.PrepareBlock(ctx, req); err != nil { |
There was a problem hiding this comment.
[suggestion] The comment above claims the fetcher "holds block n+1, already prepared, while the loop below executes block n", but the app keeps a single prepared slot and nothing synchronizes the fetcher's write to it against the execute loop's read.
Ordering is: prepare(n) → Send(n) → executor receives and starts executeBlock(n) → fetcher immediately does GlobalBlock(n+1) + PrepareBlock(n+1), which overwrites the slot. FinalizeBlock(n) only reaches takePrepared(n, hash) after proposerAddress(), parseFinalizeRequest, the executor lock and beginBlock. If the fetcher gets there first — likely when n+1 is empty or tiny and already local, i.e. during catch-up — block n misses and pays the full RLP decode + ecrecover inline, which is exactly the cost this PR removes. The loss is bounded (n+1 then hits) and correctness is unaffected, but it is silent: only the prepared counter shows it.
Either carry the preparation with the fetched block through the channel instead of a shared slot, or let the app hold the last two prepared blocks (dropping any at or below the last finalized height) so preparing n+1 cannot evict n.
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. The PR description now describes the shared slot and the blocking fetcher as they are, including this miss window.
| Txs: req.Txs, | ||
| Senders: senders, | ||
| }) | ||
| prepared, hit := a.takePrepared(block.height, block.blockHash) |
There was a problem hiding this comment.
[suggestion] Two things follow from keying the prepared block on (height, blockHash) alone.
First, on a hit req.Txs is never executed — result, and therefore TxResults, come entirely from prepared. That is safe only because the autobahn header hash commits to payloadHash, so (height, req.Hash) pins the tx set. Nothing in the app states that requirement, and Proxy.PrepareBlock is reachable for any ABCI application; a caller whose req.Hash does not commit to req.Txs (as TestEVMOnlyApplicationIgnoresAPreparedBlockForAnotherHash constructs by hand) would silently execute a different tx list than the one it passed.
Second, forgetSenders(req.Txs) on line 741 re-runs hashRawTxs over every raw transaction in the block on the critical path, purely to evict entries peekSenders already looked up during preparation.
One change covers both: have PrepareBlock keep the tx hashes it computed in preparedBlock, then verify them against the block's txs on the hit path and use them to drop the cached senders without hashing again.
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.
120c763 to
1b530d3
Compare
1b530d3 to
453eb65
Compare
Re-opens #4260 on top of #4271 (stack 3/4) for review; the code is the merged change, unmodified.
Block preparation — RLP decoding and
ecrecoverfor the sendersCheckTxdid not see — sat on the FinalizeBlock critical path. This PR takes it off: the evmonly application gains an ABCI-sidePrepareBlockthat decodes a block and parks it in a singlepreparedBlockslot keyed by(height, req.Hash);FinalizeBlocktakes the slot when both match (takePrepared) and otherwise prepares inline as before. Matching on the header hash is enough because the autobahn header commits to the payload hash, so the tx set is pinned. Hits and misses are counted byevmonly_finalize_prepared_blocks_total{prepared=true|false}, and theevmonly_preparetimer (#4267) wrapsPrepareBlock.The autobahn execute loop in
gigaRouterCommon.runExecutebecomes a two-stage pipeline underutils/scope: a fetcher goroutine blocks inr.data.GlobalBlock(n), callsapp.PrepareBlockon it, and hands the block over an unbuffered channel, so block n+1 is fetched and prepared while the loop executes block n. The proxy forwardsPrepareBlockwhen the application implements it. Storage-tail phases (#4263) are unchanged;evmonly_finalize'spreparephase now mostly measures the slot hit.Determinism is unaffected: preparation is pure decoding, and a miss (wrong or stale preparation) only costs the inline decode. What a reviewer should weigh is the unsynchronized handoff — the fetcher can prepare n+1 before FinalizeBlock(n) has taken its slot, which is a silent miss rather than an error. Validated with
go test -raceonsei-tendermint/internal/{evmonlyapp,p2p,proxy}(prepared/unprepared parity, wrong-hash, concurrent prepare/finalize) plusmake lint/fmtcheck.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