Align debug_trace* opcode (struct) tracer with execution-apis #762#12056
Align debug_trace* opcode (struct) tracer with execution-apis #762#12056manusw7 wants to merge 24 commits into
Conversation
…t error, storage at SLOAD/SSTORE, 0x-prefix memory/storage)
…iciency in Geth-style tracers
|
Claude finished @manusw7's task in 7m 54s —— View job PR Review: Align debug_trace* opcode (struct) tracer with execution-apis #762Todo List
SummaryThe PR correctly aligns both the streaming and in-memory Geth-style struct-log tracers with execution-apis #762. The core root-cause fix — changing The behavioral changes are all correct:
The FindingsMediumMissing SLOAD test — The gate change from Low
VerdictThe correctness of the implementation is sound. The single Medium finding is a test-coverage gap for the newly-fixed SLOAD path, not a bug. Given SLOAD was entirely broken before and the fix touches the EVM hot path, a targeted test is strongly recommended before merge. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| // If storage tracing is enabled, record the operation. | ||
| if (vm.TxTracer.IsTracingStorage) | ||
| if (vm.TxTracer.IsTracingOpLevelStorage) |
There was a problem hiding this comment.
Medium — SLOAD storage tracking: correct fix, missing test
The gate change from IsTracingStorage (state-diff flag from IStorageTracer) to IsTracingOpLevelStorage (per-opcode flag from ITxTracer) is the right fix. GethLikeTxTracer sets IsTracingOpLevelStorage = !options.DisableStorage but never set IsTracingStorage, so LoadOperationStorage was never being called for TLOAD (same issue fixed identically for TSTORE, SLOAD, SSTORE below).
However, there's no test that exercises SLOAD and verifies the storage field appears in struct logs for the SLOAD step. Given SLOAD was completely silent before this fix (wrong flag = always false), a targeted test would catch any regression in this code path. The SSTORE path is tested (see DebugRpcModuleTests.TraceTransaction.cs), but SLOAD is not.
| _storageByAddress[address] = contractStorage = new PooledDictionary<UInt256, UInt256>(InitialStorageMapCapacity); | ||
| } | ||
| contractStorage[storageIndex] = new UInt256(value, isBigEndian: true); | ||
| _pendingStorageMap = contractStorage; |
There was a problem hiding this comment.
Low — _pendingStorageMap semantics: single-address snapshot only
_pendingStorageMap is set to the cumulative map for the single address touched by this opcode. If a tx has both address X and address Y with storage ops at different steps, each SLOAD/SSTORE step shows only its own contract's cumulative storage — which is correct per execution-apis #762.
One subtle edge: _pendingStorageMap is a live reference into _storageByAddress[address]. Between RecordStorage being called and WriteStorageObjectIfPresent being called during FinalizePendingOpcode, no other storage callback can fire (EVM is single-threaded and operations are atomic), so there's no race. But it's worth knowing the reference is live rather than a snapshot at capture-time.
|
|
||
| CurrentTraceEntry.Storage = new Dictionary<string, string>(previousTraceEntry.Storage); | ||
| } | ||
| contractStorage[bigEndian.ToHexString(true)] = new ZeroPaddedSpan(value, 32 - value.Length, PadDirection.Left) |
There was a problem hiding this comment.
Low — ZeroPaddedSpan with no length guard
32 - value.Length is negative if value.Length > 32. EVM storage values are bounded at 32 bytes (word size), so this can't happen in practice — SLOAD returns at most 32 bytes, and SSTORE strips leading zeros so the value is ≤32 bytes. No change needed, just documenting the implicit invariant.
| memory[i] = bytes.ToHexString(); | ||
| memory[i] = bytes.ToHexString(true); | ||
| } | ||
| else if (sizeAvailable > 0) |
There was a problem hiding this comment.
Low — stackalloc inside loop body
paddedWord is declared outside the loop (good), so the stackalloc does not repeat. The paddedWord.Clear() before each use is correct — it zeroes the padding bytes before copying a partial word. ✓
One nit: the else if (sizeAvailable > 0) case (partial word) can only happen when _memory.Length is not word-aligned — which shouldn't occur in normal EVM operation since memory is grown in 32-byte words. The fix still makes correctness more robust for any edge case. ✓
|
@claude review again, I've added the SLOAD storage tracking missing test |
|
Claude finished @manusw7's task in 1m 43s —— View job Re-review: Align debug_trace* opcode (struct) tracer with execution-apis #762Todo List
SLOAD Test AssessmentThe new Scenario: SSTORE(slot 1 = 0x42) → SLOAD(slot 1) → STOP In-memory tracer (
Streaming tracer (
Cumulative snapshot correctness: After SSTORE, the address map has The test directly exercises the VerdictAll findings from the first review are now resolved:
The PR is mergeable. No Critical, High, or Medium findings remain. |
manusw7
requested review from
LukaszRozmej,
MarekM25,
benaadams and
flcl42
as code owners
|
Claude finished @manusw7's task in 6m 57s —— View job PR Review: Align debug_trace* opcode (struct) tracer with execution-apis #762Todo List
SummaryThe PR correctly aligns both the streaming and in-memory Geth-style struct-log tracers with execution-apis #762. All five behavioral changes are correct and well-tested. Core fix (
Storage semantics (both tracers): The old depth-stack ( Memory encoding ( File-tracer doubled-prefix ( Tests: Comprehensive. Both tracers pinned to identical output. SLOAD, SSTORE, cross-depth, cross-call accumulation all covered. FindingsLow —
|
| Severity | Finding | Status |
|---|---|---|
| Medium | Missing SLOAD storage test | ✅ Resolved — Storage_snapshot_is_emitted_on_SLOAD added |
| Low | _pendingStorageMap live reference |
✅ Safe — single-threaded EVM invariant |
| Low | ZeroPaddedSpan no guard for value.Length > 32 |
✅ Bounded by EVM word-size constraint |
| Low | TraceMemory.ToHexWordList() partial-word nit |
✅ Latent correctness fix, fine |
| Low | GethLikeTxTraceConverterTests model data not 0x-prefixed |
New — doc-quality only, no test failure |
| Low | Streaming tracer not tested in cross-depth scenario | New — address isolation by design, low risk |
| Low | TLOAD/TSTORE transient storage no-op | New — explicit follow-up (#12057), not a regression |
The PR is mergeable. No Critical, High, or Medium findings remain.
There was a problem hiding this comment.
The description says this pins the two tracers to identical output, but the test only compares the storage on SSTORE/SLOAD steps. Could you compare the full entry JSON here instead of just storage?
…rmat at serialize
…cts, format at serialize
…e streaming tracer
… Geth path uses ToWordArray
|
@claude review again. Please verify that the latest commits correctly address all memory allocation and dictionary pooling issues raised by @LukaszRozmej in his changes-requested review. |
|
Claude finished @manusw7's task in 4m 21s —— View job Re-review: Allocation & Pooling Issues from @LukaszRozmejTodo List
SummaryThe latest commits address all of @LukaszRozmej's allocation and pooling concerns substantively. Here is the point-by-point verification: @LukaszRozmej Finding Resolution
|
| Concern | Status |
|---|---|
AddressAsKey for storage dictionary key |
✅ Fixed |
PooledDictionary + pool for streaming tracer |
✅ Fixed |
Domain objects instead of strings in GethLikeTxMemoryTracer |
✅ Fixed |
| Domain objects instead of strings in memory/stack | ✅ Fixed (UInt256[] + deferred converters) |
| File tracer memory blob — zero-allocation | ✅ Fixed (ArrayPool) |
| File tracer stack — zero-allocation | ✅ Fixed (HexWriter) |
| File tracer gas/gasCost — zero-allocation | |
TraceMemory.ToHexWordList() not called in hot path |
✅ Fixed |
No Critical, High, or Medium findings. The single Low finding (gas/gasCost string interpolation in the file tracer) is a minor residual that doesn't block merge.
The PR is mergeable.
svlachakis
left a comment
svlachakis
left a comment
There was a problem hiding this comment.
@claude check the following review
GethLikeTxMemoryTracer.RecordStorageSnapshot
This PR makes a per-step O(N²) storage copy go live on the default debug_trace* path.
This PR correctly flips the EVM gate to IsTracingOpLevelStorage (EvmInstructions.Storage.cs), which means op-level storage capture now actually fires for the struct tracer — on master it never did (IsTracingStorage is a stored flag that defaults to false and the geth tracer never set it, so the snapshot code was dead). Good fix.
The side effect: RecordStorageSnapshot now runs, and it does
CurrentTraceEntry.Storage = new Dictionary<UInt256, UInt256>(contractStorage);i.e. it deep-copies the entire cumulative per-address map on every SLOAD/SSTORE. For a tx with N storage ops on one contract that's N copies of a map growing to N entries → O(N²) time and, worse, O(N²) retained peak memory (every entry holds its own full copy until the trace is serialized). With DisableStorage defaulting to false, this is the default debug_traceTransaction/debug_traceBlock behaviour on the in-memory (writer == null) path. That's a new OOM/CPU surface this PR introduces, not a pre-existing live cost.
The streaming tracer already does this the right way (one live PooledDictionary per address, written straight to the wire, O(N) memory, no per-step copy) — so the model exists in this PR; it's just the in-memory tracer that regresses.
Note the total output is irreducibly O(N²) bytes (geth emits the full cumulative map per storage step) — so the goal isn't to beat O(N²) work, it's to stop holding O(N²) in memory and stop allocating a dictionary per step.
Recommended fix (best perf/allocations, keeps the object model)
Don't snapshot per step. Keep one live cumulative map per address (exactly like the streaming tracer), record on each storage-touching entry only a cheap watermark into an append-only write-log, and materialize the snapshot lazily at serialization:
// tracer: single source of truth, O(1) zero-alloc capture
private readonly Dictionary<AddressAsKey, Dictionary<UInt256, UInt256>> _liveStorage = [];
private readonly List<(AddressAsKey addr, UInt256 key, UInt256 val)> _writeLog = [];
private void RecordStorageSnapshot(Address address, UInt256 index, ReadOnlySpan<byte> value)
{
if (CurrentTraceEntry is null) return;
UInt256 v = new(value, isBigEndian: true);
_writeLog.Add((address, index, v));
CurrentTraceEntry.SetStorageWatermark(address, _writeLog.Count); // address + int, no copy
}Then GethTxTraceEntry.Storage becomes computed on demand (replay _writeLog for that address up to the watermark into a reused dict during serialization, write, discard).
Result:
- Capture: O(1), zero allocation.
- Peak memory: O(N²) → O(N) retained (the write-log).
- Serialization: still O(N²) bytes (intrinsic), but the per-step dictionaries are transient and GC'd immediately instead of all held at once.
This also removes the divergence between the two tracers' storage logic — the in-memory path ends up using the same single-live-map idea the streaming path already proves.
|
@claude check my review above |
|
Claude finished @svlachakis's task in 2m 33s —— View job Review: @svlachakis's O(N²) Storage Copy FindingTodo List
Verdict: Finding is correct — High severity, blocks merge@svlachakis's analysis is accurate. Here is the verification: The O(N²) root cause confirmed
CurrentTraceEntry.Storage = new Dictionary<UInt256, UInt256>(contractStorage);For a transaction with N storage ops on one contract:
All N dictionary copies are held alive simultaneously in Concrete scale: A contract with 1,000 SSTOREs allocates ~500,500 This is a new regression, not pre-existingBefore this PR, the EVM gate checked The streaming tracer already has the correct model
Assessment of the recommended fix@svlachakis's write-log + watermark approach is correct: O(1) capture per step, O(N) retained, O(N²) transient bytes only during serialization. A simpler alternative that avoids restructuring Severity Summary
All previous findings (from prior review rounds) remain resolved. This High finding is new and was not flagged in earlier reviews. It should be addressed before merge. |
|
|
||
| CurrentTraceEntry.Storage = new Dictionary<string, string>(previousTraceEntry.Storage); | ||
| } | ||
| CurrentTraceEntry.Storage = new Dictionary<UInt256, UInt256>(contractStorage); |
There was a problem hiding this comment.
High — O(N²) allocations and peak memory on the default debug_trace* path
@svlachakis's finding is correct. For a transaction with N storage ops on one contract:
| Step | Dictionary copy allocated | Entries copied |
|---|---|---|
| 1 | size 1 | 1 |
| 2 | size 2 | 1+2 = 3 |
| … | … | … |
| N | size N | N(N+1)/2 |
All N copies are held alive simultaneously in Trace.Entries[*].Storage until the full trace is serialized — O(N²) peak retained memory. With DisableStorage defaulting to false, this is the default debug_traceTransaction/debug_traceBlock path.
Concrete scale example: a contract executing 1,000 SSTOREs → ~500K UInt256 pairs allocated → ~32 MB held in memory per transaction trace. A debug_traceBlock on a storage-heavy block multiplies this by transaction count. This is an OOM risk in production.
This is a new regression, not a pre-existing cost: before this PR the EVM gate checked IsTracingStorage (always false for GethLikeTxTracer), so RecordStorageSnapshot was effectively dead. The gate fix in EvmInstructions.Storage.cs correctly activates it, but this copy-on-every-step pattern now fires on the hot path.
The streaming tracer already demonstrates the correct model — one live PooledDictionary<UInt256, UInt256> per address, written straight to the wire in WriteStorageObjectIfPresent, O(N) peak memory. The same principle should apply here.
@svlachakis's recommended write-log + watermark fix is correct and aligns both tracers conceptually. A simpler alternative that achieves O(N) peak memory without restructuring GethTxTraceEntry is to store the per-entry delta (the single (storageIndex, value) pair that fired this step's snapshot) and reconstruct each entry's cumulative snapshot lazily during serialization by scanning forward through the entry list. Either approach avoids holding N separate dictionary copies simultaneously.
…forward-pass serializer During tracing each SLOAD/SSTORE records only a (address, key, value) delta on the entry — no dict copy. GethLikeTxTraceConverter.WriteEntriesWithStorageForwardPass maintains one running cumulative dict per address; at serialization it sets entry.Storage transiently for the duration of JsonSerializer.Serialize, then clears it. Only one dict per address is ever live at a time, reducing peak retained memory from O(N²) to O(N) for the in-memory tracer path.
… whitespaces in GethLikeTxTraceConverter
|
@claude please review again, check the solution for the O(N²) Storage Copy Finding |
|
Claude finished @manusw7's task in 3m 54s —— View job Re-review: O(N²) Storage Copy FixTodo List
O(N²) Fix — Verified CorrectThe High finding raised by @svlachakis is resolved. Here's the full analysis: What changed
// Before: deep-copy of growing cumulative map on every SLOAD/SSTORE → O(N²) retained memory
CurrentTraceEntry.Storage = new Dictionary<UInt256, UInt256>(contractStorage);
// After: O(1) capture, no allocation beyond the tuple
CurrentTraceEntry.StorageDelta = (address, storageIndex, new UInt256(value, isBigEndian: true));
Dictionary<AddressAsKey, Dictionary<UInt256, UInt256>> runningByAddress = [];
foreach (GethTxTraceEntry entry in entries)
{
if (entry.StorageDelta is { } delta)
{
// update the single live map per address
map[delta.Key] = delta.Value;
entry.Storage = map; // borrow the live map temporarily
JsonSerializer.Serialize(writer, entry, options);
entry.Storage = null; // release after writing
}
else { ... }
}Memory complexity — confirmed O(N) retained
This is exactly the approach @svlachakis recommended (write-log + lazy materialization), and it mirrors what the streaming tracer already did. Correctness checks
One nit (non-blocking)
Verdict
No Critical, High, or Medium findings remain. The PR is mergeable. |
…eTxTraceConverter.cs Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
Integrates PR #12081 (refund counter tracking) from master while preserving the O(N) forward-pass storage serialiser (Solution B). Key resolutions: - GethTxTraceEntry: add Refund property, keep UInt256[] Stack - GethLikeTxMemoryTracer: keep StorageDelta delta approach, add lean StartOperation override that captures Refund before each opcode - GethLikeTxDirectStreamingTracer: reset _refund/_refundCheckpoints on MarkAsSuccess/MarkAsFailed; write refund field; keep error-only-when- not-null for JSON parity with the in-memory tracer - GethLikeTxMemoryTracerTests: keep all tests from both branches
4 participants
Fixes #12040
Changes
errorfrom struct logs when no error occurred (was always"error": null— the only client doing so).SLOADandSSTORE(previously never captured: the EVM gated these callbacks onIsTracingStorageinstead ofIsTracingOpLevelStorage), and emitstorageonly on those steps as a per-address, whole-tx cumulative snapshot.0x-prefixmemorywords (zero-padded to 32 bytes) andstoragekeys/values (bytes32).GethLikeTxDirectStreamingTracer) and in-memory (GethLikeTxMemoryTracer) tracers; add a test pinning them to identical output.0xinmemorynow that words are individually prefixed.Aligns the default opcode (struct) tracer with ethereum/execution-apis#762 and ethereum/go-ethereum#34093.
refundand per-stepreturnDataare tracked as a follow-up in #12057.Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Updated golden JSON in the Evm/JsonRpc tracer tests; added cross-tracer equivalence and file-tracer regression tests. Full
Nethermind.Evm.Testand thedebug_trace*JsonRpc suites pass.Documentation
Requires documentation update
Requires explanation in Release Notes
debug_trace*struct-log output changes to conform to execution-apis #762:erroromitted when empty;memory/storage0x-prefixed;storageemitted only atSLOAD/SSTOREas a per-address cumulative snapshot.