From 3bfe59722a3416807d31f83a96e7387b319837b6 Mon Sep 17 00:00:00 2001 From: Fabio Di Fabio Date: Thu, 7 Dec 2023 18:28:34 +0100 Subject: [PATCH] Fix and test that the BlockAwareOperationTracer methods are invoked the correct number of times (#6259) * Test that the BlockAwareOperationTracer are invoked the correct number of times * Remove redundant calls to traceEndBlock Signed-off-by: Fabio Di Fabio --- .../besu/services/TraceServiceImpl.java | 2 - .../besu/services/TraceServiceImplTest.java | 103 ++++++++++++++++-- 2 files changed, 94 insertions(+), 11 deletions(-) diff --git a/besu/src/main/java/org/hyperledger/besu/services/TraceServiceImpl.java b/besu/src/main/java/org/hyperledger/besu/services/TraceServiceImpl.java index 02fff10d2fb..3ab2961734f 100644 --- a/besu/src/main/java/org/hyperledger/besu/services/TraceServiceImpl.java +++ b/besu/src/main/java/org/hyperledger/besu/services/TraceServiceImpl.java @@ -162,7 +162,6 @@ public void trace( blocks.forEach( block -> { results.addAll(trace(blockchain, block, chainUpdater, tracer)); - tracer.traceEndBlock(block.getHeader(), block.getBody()); }); afterTracing.accept(chainUpdater.getNextUpdater()); return Optional.of(results); @@ -180,7 +179,6 @@ private Optional> trace( block.getHash(), traceableState -> Optional.of(trace(blockchain, block, new ChainUpdater(traceableState), tracer))); - tracer.traceEndBlock(block.getHeader(), block.getBody()); return results; } diff --git a/besu/src/test/java/org/hyperledger/besu/services/TraceServiceImplTest.java b/besu/src/test/java/org/hyperledger/besu/services/TraceServiceImplTest.java index 30d51cfa737..f7ae97cdf57 100644 --- a/besu/src/test/java/org/hyperledger/besu/services/TraceServiceImplTest.java +++ b/besu/src/test/java/org/hyperledger/besu/services/TraceServiceImplTest.java @@ -15,31 +15,46 @@ package org.hyperledger.besu.services; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.fail; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; import org.hyperledger.besu.datatypes.Address; +import org.hyperledger.besu.datatypes.Hash; import org.hyperledger.besu.datatypes.Transaction; import org.hyperledger.besu.datatypes.Wei; import org.hyperledger.besu.ethereum.api.query.BlockchainQueries; import org.hyperledger.besu.ethereum.chain.MutableBlockchain; +import org.hyperledger.besu.ethereum.core.Block; import org.hyperledger.besu.ethereum.core.BlockchainSetupUtil; import org.hyperledger.besu.ethereum.worldstate.DataStorageFormat; import org.hyperledger.besu.ethereum.worldstate.WorldStateArchive; import org.hyperledger.besu.evm.log.Log; import org.hyperledger.besu.evm.worldstate.WorldView; +import org.hyperledger.besu.plugin.data.BlockBody; +import org.hyperledger.besu.plugin.data.BlockHeader; import org.hyperledger.besu.plugin.data.BlockTraceResult; import org.hyperledger.besu.plugin.data.TransactionTraceResult; import org.hyperledger.besu.plugin.services.TraceService; import org.hyperledger.besu.plugin.services.tracer.BlockAwareOperationTracer; +import java.util.HashSet; import java.util.List; +import java.util.Optional; +import java.util.Set; +import java.util.stream.LongStream; import org.apache.tuweni.bytes.Bytes; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import org.junit.runner.RunWith; -import org.mockito.junit.MockitoJUnitRunner; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.junit.jupiter.MockitoExtension; -@RunWith(MockitoJUnitRunner.class) +@ExtendWith(MockitoExtension.class) class TraceServiceImplTest { TraceService traceService; @@ -73,19 +88,40 @@ void shouldRetrieveStateUpdatePostTracingForOneBlock() { final long persistedNonceForAccount = worldStateArchive.getMutable().get(addressToVerify).getNonce(); + final long blockNumber = 2; + + final BlockAwareOperationTracer opTracer = mock(BlockAwareOperationTracer.class); + traceService.trace( - 2, - 2, + blockNumber, + blockNumber, worldState -> { assertThat(worldState.get(addressToVerify).getNonce()).isEqualTo(1); }, worldState -> { assertThat(worldState.get(addressToVerify).getNonce()).isEqualTo(2); }, - BlockAwareOperationTracer.NO_TRACING); + opTracer); assertThat(worldStateArchive.getMutable().get(addressToVerify).getNonce()) .isEqualTo(persistedNonceForAccount); + + final Block tracedBlock = blockchain.getBlockByNumber(blockNumber).get(); + + verify(opTracer).traceStartBlock(tracedBlock.getHeader(), tracedBlock.getBody()); + + tracedBlock + .getBody() + .getTransactions() + .forEach( + tx -> { + verify(opTracer).traceStartTransaction(any(), eq(tx)); + verify(opTracer) + .traceEndTransaction( + any(), eq(tx), anyBoolean(), any(), any(), anyLong(), anyLong()); + }); + + verify(opTracer).traceEndBlock(tracedBlock.getHeader(), tracedBlock.getBody()); } @Test @@ -96,9 +132,13 @@ void shouldRetrieveStateUpdatePostTracingForAllBlocks() { final long persistedNonceForAccount = worldStateArchive.getMutable().get(addressToVerify).getNonce(); + final long startBlock = 1; + final long endBlock = 32; + final BlockAwareOperationTracer opTracer = mock(BlockAwareOperationTracer.class); + traceService.trace( - 0, - 32, + startBlock, + endBlock, worldState -> { assertThat(worldState.get(addressToVerify).getNonce()).isEqualTo(0); }, @@ -106,10 +146,30 @@ void shouldRetrieveStateUpdatePostTracingForAllBlocks() { assertThat(worldState.get(addressToVerify).getNonce()) .isEqualTo(persistedNonceForAccount); }, - BlockAwareOperationTracer.NO_TRACING); + opTracer); assertThat(worldStateArchive.getMutable().get(addressToVerify).getNonce()) .isEqualTo(persistedNonceForAccount); + + LongStream.rangeClosed(startBlock, endBlock) + .mapToObj(blockchain::getBlockByNumber) + .map(Optional::get) + .forEach( + tracedBlock -> { + verify(opTracer).traceStartBlock(tracedBlock.getHeader(), tracedBlock.getBody()); + tracedBlock + .getBody() + .getTransactions() + .forEach( + tx -> { + verify(opTracer).traceStartTransaction(any(), eq(tx)); + verify(opTracer) + .traceEndTransaction( + any(), eq(tx), anyBoolean(), any(), any(), anyLong(), anyLong()); + }); + + verify(opTracer).traceEndBlock(tracedBlock.getHeader(), tracedBlock.getBody()); + }); } @Test @@ -197,8 +257,16 @@ private static class TxStartEndTracer implements BlockAwareOperationTracer { public long txEndGasUsed; public Long txEndTimeNs; + private final Set traceStartTxCalled = new HashSet<>(); + private final Set traceEndTxCalled = new HashSet<>(); + private final Set traceStartBlockCalled = new HashSet<>(); + private final Set traceEndBlockCalled = new HashSet<>(); + @Override public void traceStartTransaction(final WorldView worldView, final Transaction transaction) { + if (!traceStartTxCalled.add(transaction)) { + fail("traceStartTransaction already called for tx " + transaction); + } txStartWorldView = worldView; txStartTransaction = transaction; } @@ -212,6 +280,9 @@ public void traceEndTransaction( final List logs, final long gasUsed, final long timeNs) { + if (!traceEndTxCalled.add(transaction)) { + fail("traceEndTransaction already called for tx " + transaction); + } txEndWorldView = worldView; txEndTransaction = transaction; txEndStatus = status; @@ -220,5 +291,19 @@ public void traceEndTransaction( txEndGasUsed = gasUsed; txEndTimeNs = timeNs; } + + @Override + public void traceStartBlock(final BlockHeader blockHeader, final BlockBody blockBody) { + if (!traceStartBlockCalled.add(blockHeader.getBlockHash())) { + fail("traceStartBlock already called for block " + blockHeader); + } + } + + @Override + public void traceEndBlock(final BlockHeader blockHeader, final BlockBody blockBody) { + if (!traceEndBlockCalled.add(blockHeader.getBlockHash())) { + fail("traceEndBlock already called for block " + blockHeader); + } + } } }