execution/state: make the past-transaction log check unconditional - #23087
Merged
Conversation
resetLogs re-walked every retained log buffer of the block on every call. The executor resets before each transaction, so a block cost one full walk of its own log arena per transaction. Keep running totals of what the buffers retain and walk only the slots written since the last reset; the full walk stays as trimLogs, for when the totals leave the budget.
The budget bookkeeping put five fields on IntraBlockState for what is one concern. logBuffer owns the groups, the reuse budget and the block-wide log index, so IntraBlockState holds one field where it held two plus the four this branch added, and journal reverts through the buffer instead of reaching into its slices. AllocLog passes the journal down so it stays a forwarding call the compiler can inline; without that the extra call costs 9% on the emit path.
The type is an arena: it owns the entries, hands out pointers into its own storage and reuses them under a budget. The codebase already spells that prefixArena. Its groups field is the logs of one transaction, indexed by txIndex+1, so byTx says what the index means. reset walked every entry of what the transaction wrote to find Data that outgrew the per-entry cap. Only alloc grows Data, so alloc now flags it and reset scans only when the flag is set — which no realistic block sets. The hot path is left with the truncation.
CopyLogGroups was the only place calling the per-transaction slices groups, and it had one real caller outside tests — Bor state-sync and two debug-RPC paths, none of them hot. The batched copy is worth keeping where it runs per transaction, so it moves into Logs.Copy itself and IntraBlockState.Logs appends the per-tx copies. types is left with Log.Copy and Logs.Copy, and RlpHashLogs takes byTx.
alloc is the only place an entry outgrows the per-entry cap, and it knows where: record the position instead of a flag, so reset frees exactly those and never walks entries. It frees the Data only — the entry and its Topics are small enough to keep, and the next block reuses them. Reuse across blocks is what the cap protects: within a block an entry comes back only after a revert in the same tx, so dropping at the reset that follows costs nothing and holds the worker's watermark down. alloc also loads a.byTx once instead of on every use, which lets prove drop the repeated index checks and the pair on entries[logIdx]; reset iterates the filled range by index. Together -7.9% on the 200-tx emit benchmark.
Freeing a large Data on sight means a block of large logs allocates them all again next block. Let them stay inside the existing byte budget instead, and make them the first thing the arena gives back when that budget is exceeded: one of them holds as many bytes as hundreds of ordinary entries. The ceiling is unchanged, so this costs no watermark — it only decides who may use it. Eviction goes newest first. Transactions take their positions in order, so freeing the oldest frees exactly what the next block asks for first: measured that way the queue never reused anything and the case got 44% slower. A revert still frees an oversized Data on the spot — behind the length it is reachable again only if the transaction re-emits that many logs. BenchmarkLogEmitLargeDataPerTx, new, one log per tx past the cap: txs=16 50.69µs -> 12.60µs, 16 allocs -> 0 (fits the budget) txs=100 291.5µs -> 191.0µs, 103 allocs -> 45 (evicts the tail) Small-log emit costs 2.3% for the added branch.
Both budget paths recount, so assert the totals still match the buffers after an eviction and after a trim — neither was checked past the budget. Also pin that a buffer is queued for eviction once, when it grows: reusing it must not queue it again. alloc keeps one slice value across the growth branch, which drops a store on the common path; the index check survives it, the compiler does not join the fact across the phi.
The two budgets were far apart against real traffic. Sampling 50 mainnet blocks near tip: p99 is 1706 logs and 109KB of log data per block, so the byte budget carried 37x headroom while entries had 2.4x — and entries needs more than one block's log count, since it sums the high-water at each tx position. Both are ceilings, not reservations: an arena holds what its blocks produced, so a higher cap costs nothing until blocks emit that much. At ~304 bytes per retained entry, 16384 is ~4.75MB, the same order as the byte budget, and still far under the protocol's 120k entries per block.
The log tests and benchmarks lived in two files named after IntraBlockState; they belong with the type they exercise, so they move to log_arena_test.go. The budget comment also carried 45M gas limit figures — mainnet is at 60M — and said nothing about the per-transaction cap, which is the ceiling that matters when the arena resets once per transaction.
Retention was keyed by tx index, so the arena kept a slot for every shape it had seen: a benchmark cycling four block shapes held 1340 entries and 125KB where a reset-per-tx caller needs only its largest transaction. Reset is the ownership boundary — GetLogs has copied out by then — so entries can come back to a pool that any transaction may draw from. That drops the eviction machinery with it: no positions to hunt for later, no order to evict in, no periodic recount. The per-entry size cap stays, as an admission rule — one large buffer would otherwise take the whole budget and starve the small entries real traffic is made of — and a cap on slots per transaction replaces what the entry budget used to bound. Budgets are now per transaction, which is what a caller needs live. EIP-7825 caps one at MaxTxnGasLimit: 44k entries or 2MB of data. shifting shapes 1340 entries -> 0 retained, 25 -> 5 allocs, -87% B/op 100 large logs 152µs -> 68µs, 3.26MB -> 411B per block 200 small txs 19.5µs -> 18.4µs, -89% B/op GetLogs now asserts under ERIGON_ASSERT that the transaction's entries have not been reclaimed: the old answer was an empty set, which a receipt records as "emitted no logs".
- writtenGen is indexed by txIndex+1 like byTx; say so. - revertLast on an empty transaction hit an index panic before reaching its own message; fold the check into the guard. - The budget rationale sat above the const block. Put the arithmetic on the constant it bounds instead.
The arithmetic was in the comments; put it in the code. The ceilings come from params, the pool takes a tenth of the entry ceiling rounded to a power of two, and the rest follows from that. 4096 rather than 1024: the assembler and the tooling reset once a block is built, so a whole block's entries reach the pool at once, and 1024 dropped 576 of a p99 block's 1706 logs. Callers that reset per transaction never come close either way.
The ceiling was in the code but nothing read it. A constant expression now enforces the policy the comment describes: raising the pool past a tenth of what one transaction can emit fails to compile.
… slots reset dropped an oversized tx group before pooling anything from it, so a transaction that spends its whole gas on logs left the pool empty and the next one started from scratch. Pool first, then decide about the slots. New BenchmarkLogEmitWorstCaseTx covers that shape — MaxTxnGasLimit spent entirely on logs, 44739 of them: 89.5k allocs -> 85.2k, 9.9MB -> 9.2MB per transaction, and 4473 entries survive for the next one.
BenchmarkLogEmitWorstCaseTx parsed a hex address on every one of its 44739 logs, so 40% of what it measured was encoding/hex. With it hoisted the transaction costs 1.30ms rather than 2.18ms, and pooling an outlier's entries is worth -9.94% allocs, -8.12% B/op and -1.53% on it.
…s apart The RPC integration tests caught it: erigon_getLatestLogs executes a transaction that emits no logs and reads them back, and the position still carried a stamp from the block whose transaction at that index did emit. The assert called that a reclaim and crashed the handler. An empty group is the honest answer to both "emitted nothing" and "was reclaimed", and positions are reused across blocks, so the arena cannot distinguish them. Clearing the stamp on reclaim would only stop it from ever firing.
The assembler and the tooling reset once a block is built, so the whole block's entries reach the pool at once — the case that decides how small the pool may be, and the one shape no benchmark covered. It brackets the constant from below: at 512 a p99 block costs 102.53µs and 3872 allocs against 34.01µs and 21, because a dropped entry takes its Topics and Data with it and costs three allocations to rebuild.
byTx was the last of the positional retention: a group per transaction index, each keeping its pointer array for good. Nothing bounded them in aggregate, so 2000 logs at each of 500 positions left 9MB of pointers per arena — 51MB at a block's worth of positions, for ~2.1 Ggas spread over blocks. Every GetLogs caller asks for the transaction it just ran, and the block-level ones want the block, not random access, so the grouping buys nothing: the logs now live in one run in block order. A transaction's own are the tail of it, found by walking back while TxIndex matches, which also retires txStart. Retention is one array, capped: the same 500 positions now leave 17KB. Logs() becomes a single batched copy and RlpHashLogs is no longer needed. The assert is sound in this shape. Entries are appended in transaction order, so a request newer than the tail is a transaction that emitted nothing — how most of them end — while one older is a caller reading what it has left.
LogsRlpHash went through types.RlpHash when the run went flat, which encodes each entry through its own buffer: 50KB and 601 allocations to hash 600 logs. RlpHashLogs takes the run instead of groups now, and hashes it through one shared buffer — 171µs and 25 bytes. GetLogs also asserts again, in a shape this arena can support: entries are appended in transaction order, so a request newer than the tail is a transaction that emitted nothing, and one older is a caller reading what the arena has already moved past.
- alloc's nil check was dead: reset and revertLast empty a slot before shrinking past it, so a non-nil slot would be one two transactions share. Always take, and a broken invariant surfaces as lost reuse rather than an aliased consensus value. - maxRetainedLogSlots borrowed a per-transaction budget for a block-scale array. Bound it by the memory it costs instead: 32KB of pointers. - AllocLog's docstring said unwritten bytes are the previous block's; through the pool they can belong to a transaction of this one. - Drop an unrelated exec3_parallel.go hunk that an auto-save commit swept in.
P1 was a memory regression, and the benchmark that reports retention cannot fail a build. This asserts the invariant instead: mixed shapes — wide blocks, one deep transaction, Data past the per-entry cap, Data that fits alone but not together — and the arena still holds only its pool and one array. Each of the three guards was checked by disabling it: the array drop, the Data admission and the entry cap each make it fail. The first shapes I wrote crossed none of the thresholds and passed with all three disabled.
…om the tx-group arena
forTx's older-than-tail check was ERIGON_ASSERT-gated, so a release build handed a caller reading a past transaction an empty slice, which a receipt records as "emitted no logs". It runs once per receipt read, so ungating it costs nothing measurable and turns a silent wrong answer into a loud one. Every current caller reads the transaction it just executed, so nothing trips it today - it is a tripwire for a future one.
# Conflicts: # execution/state/log_arena.go # execution/state/log_arena_test.go
AskAlexSharov
marked this pull request as ready for review
August 12, 2026 09:00
Contributor
There was a problem hiding this comment.
Pull request overview
Makes past-transaction log checks unconditional, preventing release builds from silently returning incorrect empty logs.
Changes:
- Removes the assertion-build gate from
logArena.forTx. - Adds production-path panic coverage.
- Clarifies log-retention comments.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
execution/state/log_arena.go |
Enforces the past-transaction check in all builds. |
execution/state/log_arena_test.go |
Tests panic behavior with assertions enabled or disabled. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
yperbasis
approved these changes
Aug 13, 2026
yperbasis
left a comment
yperbasis
left a comment
Member
There was a problem hiding this comment.
LGTM, safe to ship.
What I checked: all 15 production GetLogs/GetRawLogs call sites read the just-executed transaction's index on a fresh or per-task-reset IntraBlockState (including bor_receipts_generator's GetLogs(0, ...), which runs on a fresh ComputeBlockContext ibs), so no current caller can trip the unconditional panic, and the RPC layer recovers panics per request. Tests are green under -race -shuffle=on with and without ERIGON_ASSERT=true; the serial flag-writing test cannot interleave with the parallel ones. Perf is neutral: forTx stays inlined and the benchmarks never reach it.
Nits inline on the two docstrings, plus one for a file outside the diff: the exported GetLogs/GetRawLogs (intra_block_state.go) now panic unconditionally, but their godocs don't say so. A one-line note like "Panics if txIndex is older than the newest logged transaction" would let the future per-tx-reset caller this tripwire targets find the contract at review time.
While auditing the callers I hit a pre-existing bug in the block-finalize syscall closures (each syscall re-appends the cumulative GetRawLogs run, duplicating earlier syscalls' logs on Gnosis/Polygon) — filed separately as #23255; nothing for this PR to do.
Comment on lines
+152
to
+153
| // in one run rather than grouped, and a transaction's own are the tail of it, so | ||
| // a transaction newer than the tail reads as empty and an older one panics. |
Member
There was a problem hiding this comment.
forTx compares only against the current run: after reset() a past-tx read answers empty again, and once the next block logs at the same index a stale reader gets that block's logs back — no panic in either case. Worth scoping the promise:
Suggested change
| // in one run rather than grouped, and a transaction's own are the tail of it, so | |
| // a transaction newer than the tail reads as empty and an older one panics. | |
| // in one run rather than grouped, and a transaction's own are the tail of it, so | |
| // a transaction newer than the tail reads as empty and an older one panics | |
| // while the run still holds a newer entry. |
Comment on lines
+675
to
+677
| // The past-transaction check guards production, not just assert builds: a caller | ||
| // that reads what the run has left must not be handed the tail's logs as if they | ||
| // were its own. Not parallel - it writes the global assert flag. |
Member
There was a problem hiding this comment.
Ungated forTx can't hand a past reader the tail's logs: entries are stamped with their TxIndex, so a strictly-older read matches nothing and the answer is empty — the "emitted no logs" failure the sibling test's docstring and the PR body describe. Suggest pinning the docstring to that mechanism:
Suggested change
| // The past-transaction check guards production, not just assert builds: a caller | |
| // that reads what the run has left must not be handed the tail's logs as if they | |
| // were its own. Not parallel - it writes the global assert flag. | |
| // The past-transaction check guards production, not just assert builds: a caller | |
| // that reads what the run has left would otherwise be answered empty, which a | |
| // receipt records as "emitted no logs". Not parallel - it writes the global | |
| // assert flag. |
| // The past-transaction check guards production, not just assert builds: a caller | ||
| // that reads what the run has left must not be handed the tail's logs as if they | ||
| // were its own. Not parallel - it writes the global assert flag. | ||
| func TestGetLogsOfPastTxPanicsWithoutAsserts(t *testing.T) { |
Member
There was a problem hiding this comment.
Optional: with dbg.AssertEnabled gone from the checked path, this and TestGetLogsOfPastTxPanics execute identical code and differ only in an ambient flag the code never reads. They could fold into one serial test that forces the flag false (deterministic under local ERIGON_ASSERT=true runs too). Keeping both as an explicit unconditionality pin is also fine — feel free to drop this one.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the non-blocking points from #23007 (review). Stacked on
alex/slow_reset_log_37, so review it after #23007.Ungate the older-than-tail check in
forTx. It ran behindERIGON_ASSERT, so a release build handed a caller reading a past transaction an empty slice - which a receipt records as "emitted no logs". Now it panics either way. Every current caller reads the transaction it just executed (ibs.TxnIndex()/txTask.TxIndexright after running it), so nothing trips it today; it is a tripwire for a future one.The check is one comparison on data the loop below it reads anyway.
BenchmarkLogEmitAndResetPerTxandBenchmarkAddLogare unchanged - and neither reachesforTx, since nothing in them reads logs back.Tests.
TestGetLogsOfPastTxAssertsbecomesTestGetLogsOfPastTxPanics: no global flag, runs parallel. AddedTestGetLogsOfPastTxPanicsWithoutAsserts, which pins the production path by turning asserts off explicitly - re-adding thedbg.AssertEnabled &&gate turns both red.Nit. "the p99 block of 1706 logs" → "~1700" in both const comments.