DebugOperationTracer #10115 - 10 : storage snapshot timing

CHANGELOG.md

@@ -19,6 +19,7 @@
 
 ### Bug fixes
 - Fix `engine_forkchoiceUpdatedV1` now returns `-38003 INVALID_PAYLOAD_ATTRIBUTES` for invalid payload attribute timestamps (zero or not greater than head). [#10353](https://github.com/besu-eth/besu/pull/10353)
+- Fix `debug_trace*` `storage` field to emit only for SLOAD/SSTORE opcodes showing the single slot touched, matching the execution-apis spec and geth behaviour [#10176](https://github.com/besu-eth/besu/pull/10176)
 
 ### Additions and Improvements
 - Add `eth_getStorageValues` JSON-RPC method for batched reads of multiple storage slots across multiple accounts in a single call [#10259](https://github.com/besu-eth/besu/pull/10259)

ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/vm/TraceTransactionIntegrationTest.java

@@ -158,21 +158,21 @@ public void shouldTraceSStoreOperation() {
 
     assertThat(result.isSuccessful()).isTrue();
 
-    // No storage changes before the SSTORE call.
+    // Non-storage opcodes produce no storage entry (per execution-apis spec).
     TraceFrame frame = tracer.getTraceFrames().get(170);
     assertThat(frame.getOpcode()).isEqualTo("DUP6");
+    assertThat(frame.getStorage()).isEmpty();
 
-    // Storage changes show up in the SSTORE frame.
+    // Storage is emitted only for the SSTORE frame, showing the single slot touched.
     frame = tracer.getTraceFrames().get(171);
     assertThat(frame.getOpcode()).isEqualTo("SSTORE");
     assertStorageContainsExactly(
         frame, entry("0x01", "0x6261720000000000000000000000000000000000000000000000000000000006"));
 
-    // And storage changes are still present in future frames.
+    // After SSTORE, non-storage opcodes produce no storage entry (per execution-apis spec).
     frame = tracer.getTraceFrames().get(172);
     assertThat(frame.getOpcode()).isEqualTo("PUSH2");
-    assertStorageContainsExactly(
-        frame, entry("0x01", "0x6261720000000000000000000000000000000000000000000000000000000006"));
+    assertThat(frame.getStorage()).isEmpty();
   }
 
   @Test

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/vm/DebugOperationTracer.java

@@ -17,7 +17,6 @@
 import org.hyperledger.besu.datatypes.Address;
 import org.hyperledger.besu.datatypes.Wei;
 import org.hyperledger.besu.evm.Code;
-import org.hyperledger.besu.evm.ModificationNotAllowedException;
 import org.hyperledger.besu.evm.frame.ExceptionalHaltReason;
 import org.hyperledger.besu.evm.frame.MessageFrame;
 import org.hyperledger.besu.evm.operation.AbstractCreateOperation;
@@ -42,6 +41,7 @@ public class DebugOperationTracer extends AbstractDebugOperationTracer {
   private List<TraceFrame> traceFrames = new ArrayList<>();
   private TraceFrame lastFrame;
 
+  private Optional<UInt256> preExecutionStorageKey = Optional.empty();
   private Bytes inputData;
   private int stepCount;
   private boolean limitReached;
@@ -57,6 +57,21 @@ public DebugOperationTracer(final OpCodeTracerConfig options, final boolean reco
     super(options, recordChildCallGas);
   }
 
+  @Override
+  public void tracePreExecution(final MessageFrame frame) {
+    super.tracePreExecution(frame);
+    final Operation currentOperation = frame.getCurrentOperation();
+    final String operationName = currentOperation != null ? currentOperation.getName() : null;
+    if (traceOpcode
+        && options.traceStorage()
+        && "SLOAD".equals(operationName)
+        && frame.stackSize() > 0) {
+      preExecutionStorageKey = Optional.of(UInt256.fromBytes(frame.getStackItem(0)));
+    } else {
+      preExecutionStorageKey = Optional.empty();
+    }
+  }
+
   @Override
   protected void capturePreExecutionState(final MessageFrame frame) {
     if (options.limit() > 0 && stepCount >= options.limit()) {
@@ -99,7 +114,8 @@ public void tracePostExecution(final MessageFrame frame, final OperationResult o
       return;
     }
 
-    final Optional<Map<UInt256, UInt256>> storage = captureStorage(frame);
+    final Optional<Map<UInt256, UInt256>> storage =
+        captureStorage(frame, currentOperation, operationResult);
     final Optional<Map<Address, Wei>> maybeRefunds =
         frame.getRefunds().isEmpty() ? Optional.empty() : Optional.of(frame.getRefunds());
     final long thisGasCost = computeGasCost(currentOperation, operationResult, frame);
@@ -246,20 +262,43 @@ private Optional<Bytes> captureReturnData(final MessageFrame frame) {
         : Optional.of(returnData);
   }
 
-  private Optional<Map<UInt256, UInt256>> captureStorage(final MessageFrame frame) {
+  private Optional<Map<UInt256, UInt256>> captureStorage(
+      final MessageFrame frame,
+      final Operation currentOperation,
+      final OperationResult operationResult) {
     if (!options.traceStorage()) {
       return Optional.empty();
     }
-    try {
-      Map<UInt256, UInt256> updatedStorage =
-          frame.getWorldUpdater().getAccount(frame.getRecipientAddress()).getUpdatedStorage();
-      if (updatedStorage.isEmpty()) return Optional.empty();
-      final Map<UInt256, UInt256> storageContents = new TreeMap<>(updatedStorage);
-
-      return Optional.of(storageContents);
-    } catch (final ModificationNotAllowedException e) {
-      return Optional.of(new TreeMap<>());
+    // Per execution-apis spec, the storage field is only emitted for SLOAD and SSTORE opcodes,
+    // showing only the single slot touched by that specific operation.
+    final String opName = currentOperation.getName();
+    if ("SSTORE".equals(opName)) {
+      // SStoreOperation calls frame.storageWasUpdated(key, newValue) on success;
+      // empty if SSTORE halted (e.g. insufficient gas), which is correct.
+      return frame
+          .getMaybeUpdatedStorage()
+          .map(
+              entry -> {
+                final Map<UInt256, UInt256> map = new TreeMap<>();
+                map.put(entry.getOffset(), UInt256.fromBytes(entry.getValue()));
+                return map;
+              });
+    }
+    if ("SLOAD".equals(opName) && operationResult.getHaltReason() == null) {
+      // preExecutionStorageKey holds the slot key captured before the opcode ran;
+      // after SLOAD executes the loaded value sits at the top of the stack.
+      return preExecutionStorageKey.flatMap(
+          key -> {
+            if (frame.stackSize() == 0) {
+              return Optional.empty();
+            }
+            final UInt256 loadedValue = UInt256.fromBytes(frame.getStackItem(0));
+            final Map<UInt256, UInt256> map = new TreeMap<>();
+            map.put(key, loadedValue);
+            return Optional.of(map);
+          });
     }
+    return Optional.empty();
   }
 
   private Optional<Bytes[]> captureMemory(final MessageFrame frame) {
@@ -297,6 +336,7 @@ public void reset() {
     lastFrame = null;
     stepCount = 0;
     limitReached = false;
+    preExecutionStorageKey = Optional.empty();
   }
 
   public List<TraceFrame> copyTraceFrames() {

ethereum/core/src/test/java/org/hyperledger/besu/ethereum/vm/DebugOperationTracerTest.java

@@ -15,8 +15,6 @@
 package org.hyperledger.besu.ethereum.vm;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.when;
 
 import org.hyperledger.besu.datatypes.Wei;
 import org.hyperledger.besu.ethereum.core.BlockHeader;
@@ -25,7 +23,6 @@
 import org.hyperledger.besu.ethereum.core.MessageFrameTestFixture;
 import org.hyperledger.besu.ethereum.referencetests.ReferenceTestBlockchain;
 import org.hyperledger.besu.evm.EVM;
-import org.hyperledger.besu.evm.account.MutableAccount;
 import org.hyperledger.besu.evm.frame.ExceptionalHaltReason;
 import org.hyperledger.besu.evm.frame.MessageFrame;
 import org.hyperledger.besu.evm.gascalculator.CancunGasCalculator;
@@ -39,9 +36,7 @@
 import org.hyperledger.besu.evm.worldstate.WorldUpdater;
 
 import java.util.List;
-import java.util.Map;
 import java.util.OptionalLong;
-import java.util.TreeMap;
 
 import org.apache.tuweni.bytes.Bytes;
 import org.apache.tuweni.bytes.Bytes32;
@@ -83,6 +78,30 @@ public OperationResult execute(final MessageFrame frame, final EVM evm) {
 
   private final CallOperation callOperation = new CallOperation(new CancunGasCalculator());
 
+  private static final UInt256 SLOAD_RETURNED_VALUE =
+      UInt256.fromHexString("0xcccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc");
+
+  private final Operation SSTORE_OPERATION =
+      new AbstractOperation(0x55, "SSTORE", 2, 0, null) {
+        @Override
+        public OperationResult execute(final MessageFrame frame, final EVM evm) {
+          final UInt256 key = UInt256.fromBytes(frame.popStackItem());
+          final UInt256 value = UInt256.fromBytes(frame.popStackItem());
+          frame.storageWasUpdated(key, value);
+          return new OperationResult(5000L, null);
+        }
+      };
+
+  private final Operation SLOAD_OPERATION =
+      new AbstractOperation(0x54, "SLOAD", 1, 1, null) {
+        @Override
+        public OperationResult execute(final MessageFrame frame, final EVM evm) {
+          frame.popStackItem(); // consume the key
+          frame.pushStackItem(SLOAD_RETURNED_VALUE); // push the loaded value
+          return new OperationResult(2100L, null);
+        }
+      };
+
   @Test
   void shouldRecordProgramCounter() {
     final MessageFrame frame = validMessageFrame();
@@ -192,20 +211,60 @@ void shouldNotRecordMemoryWhenDisabled() {
   }
 
   @Test
-  void shouldRecordStorageWhenEnabled() {
+  void shouldRecordStorageForSstoreWhenEnabled() {
+    final MessageFrame frame = validMessageFrame();
+    final UInt256 storageKey = UInt256.fromHexString("0x01");
+    final UInt256 storageValue = UInt256.fromHexString("0xdeadbeef");
+    frame.pushStackItem(storageValue); // value (popped second by SSTORE)
+    frame.pushStackItem(storageKey); // key   (popped first  by SSTORE)
+    final DebugOperationTracer tracer =
+        new DebugOperationTracer(
+            OpCodeTracerConfigBuilder.createFrom(OpCodeTracerConfig.DEFAULT)
+                .traceStorage(true)
+                .traceMemory(false)
+                .traceStack(false)
+                .build(),
+            false);
+    traceFrame(frame, tracer, SSTORE_OPERATION);
+    final TraceFrame traceFrame = getOnlyTraceFrame(tracer);
+    assertThat(traceFrame.getStorage()).isPresent();
+    assertThat(traceFrame.getStorage().get()).hasSize(1);
+    assertThat(traceFrame.getStorage().get()).containsEntry(storageKey, storageValue);
+  }
+
+  @Test
+  void shouldRecordStorageForSloadWhenEnabled() {
     final MessageFrame frame = validMessageFrame();
-    final Map<UInt256, UInt256> updatedStorage = setupStorageForCapture(frame);
+    final UInt256 storageKey = UInt256.fromHexString("0x02");
+    frame.pushStackItem(storageKey); // key (popped by SLOAD)
+    final DebugOperationTracer tracer =
+        new DebugOperationTracer(
+            OpCodeTracerConfigBuilder.createFrom(OpCodeTracerConfig.DEFAULT)
+                .traceStorage(true)
+                .traceMemory(false)
+                .traceStack(false)
+                .build(),
+            false);
+    traceFrame(frame, tracer, SLOAD_OPERATION);
+    final TraceFrame traceFrame = getOnlyTraceFrame(tracer);
+    assertThat(traceFrame.getStorage()).isPresent();
+    assertThat(traceFrame.getStorage().get()).hasSize(1);
+    assertThat(traceFrame.getStorage().get()).containsEntry(storageKey, SLOAD_RETURNED_VALUE);
+  }
+
+  @Test
+  void shouldNotRecordStorageForNonStorageOpcodeWhenEnabled() {
+    // storage is only emitted for SLOAD/SSTORE; non-storage opcodes produce empty storage
     final TraceFrame traceFrame =
         traceFrame(
-            frame,
+            validMessageFrame(),
             OpCodeTracerConfigBuilder.createFrom(OpCodeTracerConfig.DEFAULT)
                 .traceStorage(true)
                 .traceMemory(false)
                 .traceStack(false)
                 .build(),
             false);
-    assertThat(traceFrame.getStorage()).isPresent();
-    assertThat(traceFrame.getStorage()).contains(updatedStorage);
+    assertThat(traceFrame.getStorage()).isEmpty();
   }
 
   @Test
@@ -323,7 +382,6 @@ void childGasFlagDoesNotMatterForNonCallOperations() {
   @Test
   void shouldCaptureFrameWhenExceptionalHaltOccurs() {
     final MessageFrame frame = validMessageFrame();
-    final Map<UInt256, UInt256> updatedStorage = setupStorageForCapture(frame);
 
     final DebugOperationTracer tracer =
         new DebugOperationTracer(
@@ -340,7 +398,7 @@ void shouldCaptureFrameWhenExceptionalHaltOccurs() {
     final TraceFrame traceFrame = getOnlyTraceFrame(tracer);
     assertThat(traceFrame.getExceptionalHaltReason())
         .contains(ExceptionalHaltReason.INSUFFICIENT_GAS);
-    assertThat(traceFrame.getStorage()).contains(updatedStorage);
+    assertThat(traceFrame.getStorage()).isEmpty();
   }
 
   @Test
@@ -589,23 +647,6 @@ private MessageFrame validMessageFrameWithInitiatedMemory(final Bytes32 initiate
     return frame;
   }
 
-  private Map<UInt256, UInt256> setupStorageForCapture(final MessageFrame frame) {
-    final MutableAccount account = mock(MutableAccount.class);
-    when(worldUpdater.getAccount(frame.getRecipientAddress())).thenReturn(account);
-
-    final Map<UInt256, UInt256> updatedStorage = new TreeMap<>();
-    updatedStorage.put(UInt256.ZERO, UInt256.valueOf(233));
-    updatedStorage.put(UInt256.ONE, UInt256.valueOf(2424));
-    when(account.getUpdatedStorage()).thenReturn(updatedStorage);
-    final Bytes32 word1 = Bytes32.fromHexString("0x01");
-    final Bytes32 word2 = Bytes32.fromHexString("0x02");
-    final Bytes32 word3 = Bytes32.fromHexString("0x03");
-    frame.writeMemory(0, 32, word1);
-    frame.writeMemory(32, 32, word2);
-    frame.writeMemory(64, 32, word3);
-    return updatedStorage;
-  }
-
   private static DebugOperationTracer createDebugOperationTracerWithMemory() {
     final OpCodeTracerConfig config =
         OpCodeTracerConfigBuilder.createFrom(OpCodeTracerConfig.DEFAULT)
@@ -615,4 +656,13 @@ private static DebugOperationTracer createDebugOperationTracerWithMemory() {
             .build();
     return new DebugOperationTracer(config, false);
   }
+
+  private void setupStorageForCapture(final MessageFrame frame) {
+    final Bytes32 word1 = Bytes32.fromHexString("0x01");
+    final Bytes32 word2 = Bytes32.fromHexString("0x02");
+    final Bytes32 word3 = Bytes32.fromHexString("0x03");
+    frame.writeMemory(0, 32, word1);
+    frame.writeMemory(32, 32, word2);
+    frame.writeMemory(64, 32, word3);
+  }
 }

evm/src/main/java/org/hyperledger/besu/evm/tracing/TraceFrame.java

@@ -373,9 +373,13 @@ public Builder setMemory(final Optional<Bytes[]> memory) {
     }
 
     /**
-     * Sets the storage for this operation.
+     * Sets the storage entry touched by this operation.
      *
-     * @param storage the storage as an optional map of UInt256 keys and values
+     * <p>Per the execution-apis spec, this field is only populated for SLOAD and SSTORE opcodes and
+     * contains solely the single slot accessed or written by that operation.
+     *
+     * @param storage the storage slot touched, as an optional single-entry map of slot key to
+     *     value; empty for all opcodes other than SLOAD and SSTORE
      * @return this builder instance for method chaining
      */
     public Builder setStorage(final Optional<Map<UInt256, UInt256>> storage) {
@@ -727,9 +731,13 @@ public Optional<Bytes[]> getMemory() {
   }
 
   /**
-   * data storage slots and values
+   * Returns the storage slot touched by this operation.
+   *
+   * <p>Per the execution-apis spec, this field is only populated for SLOAD and SSTORE opcodes and
+   * contains solely the single slot accessed or written by that operation. Empty for all other
+   * opcodes.
    *
-   * @return data storage slots and values
+   * @return an optional single-entry map of slot key to value, present only for SLOAD/SSTORE
    */
   public Optional<Map<UInt256, UInt256>> getStorage() {
     return storage;