From 0345b2473f349e229af05d663b340da24741b121 Mon Sep 17 00:00:00 2001 From: delehef Date: Fri, 27 Oct 2023 00:33:10 +0200 Subject: [PATCH] Trigger contextEnter/Exit for all frames, including root (#6074) * Trigger contextEnter/Exit for all frames, including root * Differentiate between context entry and re-entry in `OperationTracer` * Update evm/src/test/java/org/hyperledger/besu/evm/processor/AbstractMessageProcessorTest.java Co-authored-by: Sally MacFarlane Signed-off-by: delehef --------- Signed-off-by: Franklin Delehelle Signed-off-by: delehef Signed-off-by: Sally MacFarlane Co-authored-by: Sally MacFarlane --- .../processor/AbstractMessageProcessor.java | 12 +++-- .../besu/evm/tracing/OperationTracer.java | 7 +++ .../AbstractMessageProcessorTest.java | 51 +++++++------------ 3 files changed, 34 insertions(+), 36 deletions(-) diff --git a/evm/src/main/java/org/hyperledger/besu/evm/processor/AbstractMessageProcessor.java b/evm/src/main/java/org/hyperledger/besu/evm/processor/AbstractMessageProcessor.java index 0e910068258..2bc98b58efc 100644 --- a/evm/src/main/java/org/hyperledger/besu/evm/processor/AbstractMessageProcessor.java +++ b/evm/src/main/java/org/hyperledger/besu/evm/processor/AbstractMessageProcessor.java @@ -184,8 +184,12 @@ private void codeExecute(final MessageFrame frame, final OperationTracer operati * @param operationTracer the operation tracer */ public void process(final MessageFrame frame, final OperationTracer operationTracer) { - if (operationTracer != null && frame.getMessageStackSize() > 1) { - operationTracer.traceContextEnter(frame); + if (operationTracer != null) { + if (frame.getState() == MessageFrame.State.NOT_STARTED) { + operationTracer.traceContextEnter(frame); + } else { + operationTracer.traceContextReEnter(frame); + } } if (frame.getState() == MessageFrame.State.NOT_STARTED) { @@ -213,13 +217,13 @@ public void process(final MessageFrame frame, final OperationTracer operationTra } if (frame.getState() == MessageFrame.State.COMPLETED_SUCCESS) { - if (operationTracer != null && frame.getMessageStackSize() > 1) { + if (operationTracer != null) { operationTracer.traceContextExit(frame); } completedSuccess(frame); } if (frame.getState() == MessageFrame.State.COMPLETED_FAILED) { - if (operationTracer != null && frame.getMessageStackSize() > 1) { + if (operationTracer != null) { operationTracer.traceContextExit(frame); } completedFailed(frame); diff --git a/evm/src/main/java/org/hyperledger/besu/evm/tracing/OperationTracer.java b/evm/src/main/java/org/hyperledger/besu/evm/tracing/OperationTracer.java index 455911c6ca1..edaed8e2416 100644 --- a/evm/src/main/java/org/hyperledger/besu/evm/tracing/OperationTracer.java +++ b/evm/src/main/java/org/hyperledger/besu/evm/tracing/OperationTracer.java @@ -102,6 +102,13 @@ default void traceEndTransaction( */ default void traceContextEnter(final MessageFrame frame) {} + /** + * Trace the re-entry in a context from a child context + * + * @param frame the frame + */ + default void traceContextReEnter(final MessageFrame frame) {} + /** * Trace the exiting of a context * diff --git a/evm/src/test/java/org/hyperledger/besu/evm/processor/AbstractMessageProcessorTest.java b/evm/src/test/java/org/hyperledger/besu/evm/processor/AbstractMessageProcessorTest.java index 598758fb752..016f3f6da37 100644 --- a/evm/src/test/java/org/hyperledger/besu/evm/processor/AbstractMessageProcessorTest.java +++ b/evm/src/test/java/org/hyperledger/besu/evm/processor/AbstractMessageProcessorTest.java @@ -17,9 +17,9 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.hyperledger.besu.evm.processor.AbstractMessageProcessorTest.ContextTracer.TRACE_TYPE.CONTEXT_ENTER; import static org.hyperledger.besu.evm.processor.AbstractMessageProcessorTest.ContextTracer.TRACE_TYPE.CONTEXT_EXIT; +import static org.hyperledger.besu.evm.processor.AbstractMessageProcessorTest.ContextTracer.TRACE_TYPE.CONTEXT_RE_ENTER; import static org.hyperledger.besu.evm.processor.AbstractMessageProcessorTest.ContextTracer.TRACE_TYPE.POST_EXECUTION; import static org.hyperledger.besu.evm.processor.AbstractMessageProcessorTest.ContextTracer.TRACE_TYPE.PRE_EXECUTION; -import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -30,8 +30,8 @@ import org.hyperledger.besu.evm.fluent.EVMExecutor; import org.hyperledger.besu.evm.frame.MessageFrame; import org.hyperledger.besu.evm.operation.Operation; +import org.hyperledger.besu.evm.toy.ToyWorld; import org.hyperledger.besu.evm.tracing.OperationTracer; -import org.hyperledger.besu.evm.worldstate.WorldUpdater; import java.util.ArrayList; import java.util.Arrays; @@ -41,59 +41,37 @@ import org.apache.tuweni.bytes.Bytes; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; -import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.ValueSource; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; @ExtendWith(MockitoExtension.class) abstract class AbstractMessageProcessorTest { - @Mock MessageFrame messageFrame; @Mock OperationTracer operationTracer; @Mock Deque messageFrameStack; - @Mock WorldUpdater worldUpdater; protected abstract T getAbstractMessageProcessor(); - @ParameterizedTest - @ValueSource(ints = {0, 1}) - void shouldNotTraceContextIfStackSizeIsZero(final int stackSize) { - when(messageFrame.getMessageStackSize()).thenReturn(stackSize); - when(messageFrame.getState()) - .thenReturn(MessageFrame.State.COMPLETED_SUCCESS, MessageFrame.State.COMPLETED_FAILED); - when(messageFrame.getMessageFrameStack()).thenReturn(messageFrameStack); - - getAbstractMessageProcessor().process(messageFrame, operationTracer); - - verify(operationTracer, never()).traceContextEnter(messageFrame); - verify(operationTracer, never()).traceContextExit(messageFrame); - } - - @ParameterizedTest - @ValueSource(ints = {2, 3, 5, 15, Integer.MAX_VALUE}) - void shouldTraceContextIfStackSizeIsGreaterZeroAndSuccess(final int stackSize) { - when(messageFrame.getMessageStackSize()).thenReturn(stackSize); + @Test + void shouldTraceExitForContext() { + when(messageFrame.getWorldUpdater()).thenReturn(new ToyWorld()); when(messageFrame.getState()).thenReturn(MessageFrame.State.COMPLETED_SUCCESS); when(messageFrame.getMessageFrameStack()).thenReturn(messageFrameStack); - when(messageFrame.getWorldUpdater()).thenReturn(worldUpdater); getAbstractMessageProcessor().process(messageFrame, operationTracer); - verify(operationTracer, times(1)).traceContextEnter(messageFrame); + // As the only MessageFrame state will be COMPLETED_SUCCESS, only a contextExit is expected verify(operationTracer, times(1)).traceContextExit(messageFrame); } - @ParameterizedTest - @ValueSource(ints = {2, 3, 5, 15, Integer.MAX_VALUE}) - void shouldTraceContextIfStackSizeIsGreaterZeroAndFailure(final int stackSize) { - when(messageFrame.getMessageStackSize()).thenReturn(stackSize); + @Test + void shouldTraceExitEvenIfContextFailed() { when(messageFrame.getState()).thenReturn(MessageFrame.State.COMPLETED_FAILED); when(messageFrame.getMessageFrameStack()).thenReturn(messageFrameStack); getAbstractMessageProcessor().process(messageFrame, operationTracer); - verify(operationTracer, times(1)).traceContextEnter(messageFrame); + // As the only MessageFrame state will be COMPLETED_FAILED, only a contextExit is expected verify(operationTracer, times(1)).traceContextExit(messageFrame); } @@ -133,6 +111,7 @@ void shouldTraceContextEnterExitForEip3155Test() { final List expectedTraces = Arrays.asList( + CONTEXT_ENTER, // Entry in root context PRE_EXECUTION, // PUSH1 POST_EXECUTION, // PUSH1 PRE_EXECUTION, // DUP1 @@ -161,10 +140,12 @@ void shouldTraceContextEnterExitForEip3155Test() { POST_EXECUTION, // STATICCALL CONTEXT_ENTER, // STATICCALL CONTEXT_EXIT, // STATICCALL + CONTEXT_RE_ENTER, // Re-entry in root context post-STATICALL PRE_EXECUTION, // PUSH1 POST_EXECUTION, // PUSH1 PRE_EXECUTION, // RETURN - POST_EXECUTION // RETURN + POST_EXECUTION, // RETURN + CONTEXT_EXIT // Exiting root context ); assertThat(contextTracer.traceHistory()).isEqualTo(expectedTraces); @@ -175,6 +156,7 @@ enum TRACE_TYPE { PRE_EXECUTION, POST_EXECUTION, CONTEXT_ENTER, + CONTEXT_RE_ENTER, CONTEXT_EXIT } @@ -196,6 +178,11 @@ public void traceContextEnter(final MessageFrame frame) { traceHistory.add(TRACE_TYPE.CONTEXT_ENTER); } + @Override + public void traceContextReEnter(final MessageFrame frame) { + traceHistory.add(CONTEXT_RE_ENTER); + } + @Override public void traceContextExit(final MessageFrame frame) { traceHistory.add(TRACE_TYPE.CONTEXT_EXIT);