Repository navigation
Conversation
The execute loop fetches the next global block on its own goroutine and calls PrepareBlock on it, so the evmonly app decodes its transactions and recovers their senders while the current block executes. FinalizeBlock uses the prepared transactions only for the same height and hash, and decodes the block itself otherwise. The proposer is still read on the execute loop, after the previous block's Commit. Backport of #4272 (giga-1 #4260). Adapted to main: the two-generation sender cache gains peek, FinalizeBlock keeps main's PrevRandao and phase constants, the prepare stage lives in prepare.go, and the app's package constants move to the top of the file. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@seidroid review |
PR SummaryMedium Risk Overview The ABCI proxy exposes optional OTel counters and phase timers track prepare hits vs misses. Extensive tests assert identical app hashes and tx results with and without preparation, including concurrent prepare/finalize and router catch-up. Reviewed by Cursor Bugbot for commit d3868c0. 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 @@
## main #4459 +/- ##
==========================================
+ Coverage 56.77% 56.82% +0.04%
==========================================
Files 2126 2128 +2
Lines 166876 167121 +245
==========================================
+ Hits 94737 94959 +222
- Misses 72134 72157 +23
Partials 5 5
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 review |
There was a problem hiding this comment.
The execute loop now fetches block n+1 and decodes it with PrepareBlock while block n executes. FinalizeBlock uses the prepared transactions only when the height and hash match, and decodes the block itself otherwise. I approve because a prepared block decodes to the same transactions and senders as an unprepared one (parsePreparedTx is deterministic for a given Number/Time signer), so preparing cannot change results; the one issue that holds affects performance only. I kept codex's single-slot overwrite finding as a suggestion because it costs repeated decoding, not correctness; no other reading was provided. I could not run the tests because there is no Go toolchain in the sandbox.
Non-blocking
1 finding on the changed lines, as inline comments.
- No test runs the evmonly app's PrepareBlock through the router's fetch-ahead loop. The router tests use
testApp, which does not implementblockPreparer. The app tests call PrepareBlock directly, so nothing checks that the pipeline actually produces hits, and the slot-overwrite case above goes undetected.
1 nit, not posted on the code
sei-tendermint/internal/evmonlyapp/app.go:791— On a hit,forgetSendershashes every transaction again on the execute loop just to drop cache entries, althoughpeekSendersalready computed those hashes in PrepareBlock. Storing the hashes inpreparedBlockwould take this work off the critical path that the change is trying to shorten.
seidroid review · decision approve · session 0c7b4d3668f34384acee423f608fc47d · turn resp_claude_ee77d3c47fb71b7b410dccc1699e05e0 · item d990d26818ab5ccb8e9b9c6cf1b09e7a
Findings: 0 blocking | 2 non-blocking | 1 posted inline
PrepareBlock keeps up to two prepared blocks, one per height, until FinalizeBlock takes its block or the height falls below the next one to execute. During catch-up the fetcher can prepare block n+1 before block n reaches FinalizeBlock, and a single slot then dropped block n. A prepared block also keeps its transaction hashes, so a hit drops the CheckTx senders without hashing the block again on the execute loop. A router test drives the fetch-ahead loop over a filled BlockStore with an app that implements PrepareBlock, and checks every block arrives prepared. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@seidroid review |
There was a problem hiding this comment.
Since my last review, d1818b4 replaced the single prepared slot with a height-ordered queue of at most two blocks, stored the transaction hashes so a prepared block no longer rehashes on the execute loop, and added a router test that runs the real fetch-ahead loop; the overwrite race, the rehash nit and the missing router test I raised are all fixed as the reply claims. Nothing blocks, because the fetcher cannot get more than one block ahead and the queue keeps the next committed height; of codex's single reading I kept the eviction-tail point as a nit, since it holds at most one stale block and is not a growing leak.
1 nit, not posted on the code
sei-tendermint/internal/evmonlyapp/prepare.go:50— (codex, confirmed) Reslicingkept[:min(len(kept), maxPreparedBlocks)]leaves the evicted block in the backing array, so its txs stay reachable until a laterappendoverwrites that slot. It is bounded to one block and the router never fills the queue past two, but clearing the tail before truncating (clear(kept[maxPreparedBlocks:])) would release it.
seidroid review · decision approve · session 0c7b4d3668f34384acee423f608fc47d · turn resp_claude_f2b7f22048fa176fa3437a0a526324ed · item 5ce82dc82b3750f2be7454174ffdbb3a
Findings: 0 blocking | 0 non-blocking | 0 posted inline
Clear the queue's tail before truncating, so an evicted block's transactions stop being reachable through the backing array. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@seidroid review |
There was a problem hiding this comment.
This PR has the execute loop prepare block n+1 while block n runs, keeping up to two prepared blocks in a height-ordered queue. The one change since my last review is that put now clears the evicted tail before truncating, with a test that checks it, so my eviction-retention nit (first raised by codex) is fixed. My earlier slot-overwrite finding was already fixed, and codex found nothing new, which I confirmed, so nothing blocks.
seidroid review · decision approve · session 0c7b4d3668f34384acee423f608fc47d · turn resp_claude_92e61fae57b4a1848cc8ba5b226bfb1d · item 98c1b3f240845b9ea2bd3c020a69826f
Findings: 0 blocking | 0 non-blocking | 0 posted inline
This is a backport to main of #4272 (original giga-1 PR #4260) from the giga-1 PR stack #4270–#4273. That stack targeted stacked giga-1 branches and never reached main, except #4270, which landed as #4366. @codchen approved #4272 at 453eb65. Since that commit, FinalizeBlock keeps main's PrevRandao and phase constants, main's two-generation sender cache gains
peek, the prepare stage moves toprepare.go, andapp.go's package constants move to the top. It does not depend on the #4271 backport. Tracked in PLT-1378.The execute loop fetches the next global block on its own goroutine and calls
PrepareBlockon it. The EVM-only app decodes that block's transactions and recovers their senders while the current block executes. FinalizeBlock uses the prepared transactions only for the same height and hash, and decodes the block itself otherwise. The proposer is still read on the execute loop after the previous Commit.This is not app-hash-breaking. A prepared block decodes to the same transactions and senders, and tests compare app hashes and results with and without preparation, including preparation running at the same time as FinalizeBlock. It does not lower main-loop cost on a CPU-bound host: in a local run the prepare phase of about 10 ms left FinalizeBlock, but the execute phase grew by about the same amount because both stages use every core, which matches giga-testnet-2 (19.44 ms per block before the revert, 19.43 ms after). A prepared block is kept until its height executes, at most two at a time, so the fetcher can run a block ahead during catch-up without dropping the block before it; a router test checks that every block reaches FinalizeBlock prepared. A
GlobalBlockerror for n+1 now cancels block n mid-FinalizeBlock instead of one block later; the node errors out either way and n re-executes on restart. It merges cleanly with #4452 and the #4271 backport in any order.