Fixes #10115 - 10 : storage snapshot timing #10176
Open
sagarkhandagre998 wants to merge 4 commits intobesu-eth:mainfrom
Open
Fixes #10115 - 10 : storage snapshot timing #10176sagarkhandagre998 wants to merge 4 commits intobesu-eth:mainfrom
sagarkhandagre998 wants to merge 4 commits intobesu-eth:mainfrom
Conversation
Per execution-apis spec (PR besu-eth#762), the storage field in debug trace struct logs must only be emitted for SLOAD and SSTORE opcodes, showing only the single slot touched by that operation. Previously, captureStorage() was called on every opcode and returned the full accumulated dirty-storage map from getUpdatedStorage(), so every frame after the first SSTORE would include all previously written slots regardless of opcode. Changes: - captureStorage() now returns Optional.empty() for all opcodes except SLOAD and SSTORE - SSTORE: uses frame.getMaybeUpdatedStorage() (set by SStoreOperation via frame.storageWasUpdated()) to return only the written slot - SLOAD: captures the slot key in tracePreExecution() before the opcode pops it off the stack, then reads the loaded value from the stack top in tracePostExecution() - Removed ModificationNotAllowedException import (no longer used) Tests: - shouldRecordStorageWhenEnabled -> split into three focused tests: shouldRecordStorageForSstoreWhenEnabled, shouldRecordStorageForSloadWhenEnabled, shouldNotRecordStorageForNonStorageOpcodeWhenEnabled - shouldCaptureFrameWhenExceptionalHaltOccurs: storage is now empty for a non-storage opcode halt (MUL), assertion updated accordingly Signed-off-by: Sagar Khandagre <sagar.khandagre998@gmail.com>
Per execution-apis spec (PR besu-eth#762), the storage field is only populated for SLOAD and SSTORE opcodes and contains a single-entry map for the slot touched. Update the getter and builder setter Javadoc accordingly. Signed-off-by: Sagar Khandagre <sagar.khandagre998@gmail.com>
…raceFrame Signed-off-by: Sagar Khandagre <sagar.khandagre998@gmail.com>
69de00e to
53f6dae
Compare
Author
|
@macfarla Kindly take a look at once. |
Signed-off-by: Sagar Khandagre <sagar.khandagre998@gmail.com>
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.
PR description
Problem
The
storagefield in debug trace struct logs was being emitted on every opcode, not just storage-touching ones. The root cause was inDebugOperationTracer.captureStorage():getUpdatedStorage()
returns "all storage slots written so far in the transaction" — it accumulates across opcodes. So once anySSTORE` executed, every subsequent frame (ADD, PUSH, JUMP, etc.) included the full dirty-storage map. This diverges from geth and the execution-apis spec, as highlighted in the ethPandaOps trace comparison report.Approach & Reasoning
The spec says: emit
storageonly forSLOADandSSTORE, showing only the single slot touched by that operation.For SSTORE this was straightforward —
SStoreOperationalready callsframe.storageWasUpdated(key, newValue)on success, which setsframe.getMaybeUpdatedStorage(). That's exactly the one slot we need. No extra tracking required.For SLOAD it was trickier.
SLoadOperationdoesn't callstorageWasUpdated(it's a read, not a write), sogetMaybeUpdatedStorage()is always empty for it. Two pieces of data are needed: the slot key and the loaded value.tracePreExecution()before the opcode runs — introducing thepreExecutionStorageKeyfield.frame.getStackItem(0)intracePostExecution()gives it directly.This approach deliberately avoids depending on
options.traceStack()being enabled — the key is captured independently of stack tracing, so storage capture works correctly regardless of what other trace flags the caller set.One edge case considered: if SLOAD halts (e.g. OOG), the value was never pushed, so the stack top would be wrong. This is guarded by checking
operationResult.getHaltReason() == nullbefore reading the stack — matching what geth does (no storage entry on a halted SLOAD).Tests
All 21 tests in
DebugOperationTracerTestpass, including the 3 new ones:shouldRecordStorageForSstoreWhenEnabled— verifies single-slot map with correct key/valueshouldRecordStorageForSloadWhenEnabled— verifies key captured pre-execution, value captured post-executionshouldNotRecordStorageForNonStorageOpcodeWhenEnabled— verifies MUL (and any non-storage opcode) emits empty storage even when `traceStorage=trueFixed Issue(s)
Fixes #10115
Thanks for sending a pull request! Have you done the following?
doc-change-requiredlabel to this PR if updates are required.Locally, you can run these tests to catch failures early:
./gradlew spotlessApply./gradlew build./gradlew acceptanceTest./gradlew integrationTest./gradlew ethereum:referenceTests:referenceTests