Repository navigation
perf(evmonlyapp): hash a block's transactions before taking the sender lock - #4235
Conversation
PR SummaryLow Risk Overview
Adds Reviewed by Cursor Bugbot for commit 23e892a. 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 #4235 +/- ##
===========================================
+ Coverage 65.55% 78.70% +13.14%
===========================================
Files 2081 7 -2074
Lines 157460 756 -156704
===========================================
- Hits 103222 595 -102627
+ Misses 54097 161 -53936
+ 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 keccak pass out of the checkedSenders lock and across the pool is correct and behaviour-preserving: hashes stay index-aligned, lookups still run in order so the duplicate-transaction case is unchanged, workers write disjoint contiguous ranges of hashes, and TestHashRawTxsMatchesKeccak256Hash pins the Reset/Write/Read hasher reuse to crypto.Keccak256Hash (the equality that would otherwise silently drop every sender). Two non-blocking notes: the chunking degenerates to one goroutine per transaction on blocks smaller than GOMAXPROCS, and the new godoc paragraphs carry rationale that AGENTS.md asks be kept inline.
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.
851ae16 to
d0397fd
Compare
…r lock
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
the 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 over 1,848 transactions on a 10-core machine:
keccak on the loop 791 us (428 ns/tx)
across the pool 231 us
That is 560 us off a 28.83 ms loop, and a validator at GOMAXPROCS 32 has more
of the pool to spread it over. It costs one 59 KB slice of hashes per block.
The lookups still run in index order, so a block carrying the same transaction
twice resolves the first and leaves the second unknown, as before. One hasher is
reset per worker rather than allocated per transaction, and
TestHashRawTxsMatchesKeccak256Hash pins the result to crypto.Keccak256Hash: a
hash that differs by a byte would silently lose every sender and make the
executor recover them all again.
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>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
9c4915a to
23e892a
Compare
…r lock (sei-protocol#4364) Backports sei-protocol#4235 from `giga-1` to `main` (PLT-1303, part of PLT-1297). `takeSenders` keccaks every raw transaction while holding the `checkedSenders` lock on the execute loop, and `CheckTx` writes 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). `takeSenders` now computes all hashes first with `hashRawTxs`, which splits the block across up to `GOMAXPROCS` workers (at least `minTxsPerHashWorker` = 64 txs each, so small blocks stay on one goroutine), and the lock covers only the `senderCache.take` lookups. Main has since replaced the plain map with the two-generation `senderCache`, so the port keeps that structure and only moves the hashing out of the lock. Unlike the `giga-1` version, the fan-out uses `scope.Parallel` instead of a bare `sync.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. `TestHashRawTxsMatchesKeccak256Hash` pins `hashRawTxs` to `crypto.Keccak256Hash` across the single-worker and multi-worker paths; the evmonlyapp package tests pass under `-race`, and `make lint` / `make fmtcheck` are 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 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>
One of four independent PRs from the serial-budget survey (#4234, #4235, #4236, #4237); each targets
giga-1on its own.takeSenderskeccaked every raw transaction inside thecheckedSenderslock, on the execute loop. The hashing is the whole cost of the pass, andCheckTxwrites 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
Measured locally with a throwaway
BenchmarkHashRawTxs(not included in this PR). 1,848 transactions, 10-core machine:560 µs off a 28.83 ms loop, ~1.9%. A validator at
GOMAXPROCS32 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.
Correctness
TestHashRawTxsMatchesKeccak256HashpinshashRawTxstocrypto.Keccak256Hashover 0, 1, 2, 17 and 1,848 inputs. This matters more than it looks: the map is keyed bytx.Hash(), so a hash that differs by one byte would silently lose every sender and make the executor recover all 1,848 signatures instead.-raceclean 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