diff --git a/CHANGELOG.md b/CHANGELOG.md index ce3f5852e40..940cd3695b6 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/vm/TraceTransactionIntegrationTest.java b/ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/vm/TraceTransactionIntegrationTest.java index adf365b6705..968ca5a0945 100644 --- a/ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/vm/TraceTransactionIntegrationTest.java +++ b/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 diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/vm/DebugOperationTracer.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/vm/DebugOperationTracer.java index 44aef097f17..f502eafa6b0 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/vm/DebugOperationTracer.java +++ b/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 traceFrames = new ArrayList<>(); private TraceFrame lastFrame; + private Optional 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> storage = captureStorage(frame); + final Optional> storage = + captureStorage(frame, currentOperation, operationResult); final Optional> maybeRefunds = frame.getRefunds().isEmpty() ? Optional.empty() : Optional.of(frame.getRefunds()); final long thisGasCost = computeGasCost(currentOperation, operationResult, frame); @@ -246,20 +262,43 @@ private Optional captureReturnData(final MessageFrame frame) { : Optional.of(returnData); } - private Optional> captureStorage(final MessageFrame frame) { + private Optional> captureStorage( + final MessageFrame frame, + final Operation currentOperation, + final OperationResult operationResult) { if (!options.traceStorage()) { return Optional.empty(); } - try { - Map updatedStorage = - frame.getWorldUpdater().getAccount(frame.getRecipientAddress()).getUpdatedStorage(); - if (updatedStorage.isEmpty()) return Optional.empty(); - final Map 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 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 map = new TreeMap<>(); + map.put(key, loadedValue); + return Optional.of(map); + }); } + return Optional.empty(); } private Optional captureMemory(final MessageFrame frame) { @@ -297,6 +336,7 @@ public void reset() { lastFrame = null; stepCount = 0; limitReached = false; + preExecutionStorageKey = Optional.empty(); } public List copyTraceFrames() { diff --git a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/vm/DebugOperationTracerTest.java b/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/vm/DebugOperationTracerTest.java index ceaf374a4d7..96515cfa779 100644 --- a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/vm/DebugOperationTracerTest.java +++ b/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 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 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 setupStorageForCapture(final MessageFrame frame) { - final MutableAccount account = mock(MutableAccount.class); - when(worldUpdater.getAccount(frame.getRecipientAddress())).thenReturn(account); - - final Map 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); + } } diff --git a/evm/src/main/java/org/hyperledger/besu/evm/tracing/TraceFrame.java b/evm/src/main/java/org/hyperledger/besu/evm/tracing/TraceFrame.java index 8128912da49..cfecd1152df 100644 --- a/evm/src/main/java/org/hyperledger/besu/evm/tracing/TraceFrame.java +++ b/evm/src/main/java/org/hyperledger/besu/evm/tracing/TraceFrame.java @@ -373,9 +373,13 @@ public Builder setMemory(final Optional 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 + *

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> storage) { @@ -727,9 +731,13 @@ public Optional getMemory() { } /** - * data storage slots and values + * Returns the storage slot touched by this operation. + * + *

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> getStorage() { return storage;