Repository navigation
perf(evmonlyapp): hash a block's transactions before taking the sender lock - #4364
Conversation
…r lock (#4235) One of four independent PRs from the serial-budget survey (#4234, #4235, `takeSenders` keccaked every raw transaction **inside** the `checkedSenders` lock, on the execute loop. The hashing is the whole cost of the pass, and `CheckTx` writes that same map about 64,500 times a second — so admission stalled for the length of a block's worth of hashes. The hashes are now taken across the pool first, and the lock covers only the map lookups. Measured locally with a throwaway `BenchmarkHashRawTxs` (not included in this PR). 1,848 transactions, 10-core machine: | | time | |---|---| | keccak on the loop (what it did) | 791 µs — 428 ns/tx | | across the pool (what it does) | 231 µs | **560 µs off a 28.83 ms loop, ~1.9%.** A validator at `GOMAXPROCS` 32 has more of the pool to spread it over, so this is the floor. It costs one 59 KB slice of hashes per block. Two things the survey did not have: the keccak is 428 ns/tx, slightly above its 300–420 ns estimate; and the lock was held across the whole pass, which it did not mention and which is the part that touches admission latency rather than throughput. - Lookups still run in index order, so a block carrying the same transaction twice resolves the first and leaves the second unknown — unchanged. - `TestHashRawTxsMatchesKeccak256Hash` pins `hashRawTxs` to `crypto.Keccak256Hash` over 0, 1, 2, 17 and 1,848 inputs. This matters more than it looks: the map is keyed by `tx.Hash()`, so a hash that differs by one byte would silently lose every sender and make the executor recover all 1,848 signatures instead. - One hasher is reset per worker rather than allocated per transaction. - Ranges are contiguous, not strided, so workers do not share cache lines while writing. - `-race` clean on the package. Link to Devin session: https://app.devin.ai/sessions/0952787e9df84a7994fda23fea5db400 Open in Devin Desktop: https://app.devin.ai/desktop/session/0952787e9df84a7994fda23fea5db400?variant=devin Requested by: @bdchatham --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> (cherry picked from commit abeab64)
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
PR SummaryLow Risk Overview
Adds Reviewed by Cursor Bugbot for commit 48978dd. 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 @@
## main #4364 +/- ##
==========================================
- Coverage 67.73% 66.56% -1.18%
==========================================
Files 2172 2046 -126
Lines 168523 156057 -12466
==========================================
- Hits 114144 103874 -10270
+ Misses 54370 52174 -2196
Partials 9 9
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
I found nothing that needs fixing. The PR moves transaction hashing out of the checkedSenders lock and runs it in parallel with scope.Parallel. Lookups still happen in block order under the lock, so duplicate handling stays the same, and a new test checks that the hashes match crypto.Keccak256Hash for block sizes both below and above the parallel threshold.
Findings: 0 blocking | 0 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
…1303-hash-before-sender-lock Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> # Conflicts: # sei-tendermint/internal/evmonlyapp/app_test.go
…1303-hash-before-sender-lock Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> # Conflicts: # sei-tendermint/internal/evmonlyapp/app.go
2 participants
Backports #4235 from
giga-1tomain(PLT-1303, part of PLT-1297).takeSenderskeccaks every raw transaction while holding thecheckedSenderslock on the execute loop, andCheckTxwrites that same cache constantly, so admission stalls for the length of a block's worth of hashes (~790 µs for 1,848 txs on the original measurement).takeSendersnow computes all hashes first withhashRawTxs, which splits the block across up toGOMAXPROCSworkers (at leastminTxsPerHashWorker= 64 txs each, so small blocks stay on one goroutine), and the lock covers only thesenderCache.takelookups. Main has since replaced the plain map with the two-generationsenderCache, so the port keeps that structure and only moves the hashing out of the lock. Unlike thegiga-1version, the fan-out usesscope.Parallelinstead of a baresync.WaitGroup+go func, per the sei-tendermint conventions.No consensus or state impact: lookups still run in block index order, so a duplicate tx in a block still resolves only the first occurrence.
TestHashRawTxsMatchesKeccak256HashpinshashRawTxstocrypto.Keccak256Hashacross the single-worker and multi-worker paths; the evmonlyapp package tests pass under-race, andmake lint/make fmtcheckare clean.Link to Devin session: https://app.devin.ai/sessions/d808db90106341278344623de10b92c0
Open in Devin Desktop: https://app.devin.ai/desktop/session/d808db90106341278344623de10b92c0?variant=devin
Requested by: @bdchatham