From 0810b73ee310747f7b3a62821c98cb3e5bb8d03a Mon Sep 17 00:00:00 2001 From: Danno Ferrin Date: Mon, 13 Jan 2025 08:51:11 -0700 Subject: [PATCH] Refactor BlockHashOperation block height check into Operation (#8091) * Refactor BlockHashOperation block height check into Operation Refactor the 4 different places block height is checked across Besu into the BlockHashOperation. Add support to make the lookback range configurable in the BlockHashLookup interface, but default it to 256. Signed-off-by: Danno Ferrin * spotless Signed-off-by: Danno Ferrin * javadoc Signed-off-by: Danno Ferrin * Update Unit Tests to use the operation. Signed-off-by: Danno Ferrin * Update Unit Tests to use the operation. Signed-off-by: Danno Ferrin --------- Signed-off-by: Danno Ferrin --- .../vm/BlockchainBasedBlockHashLookup.java | 12 ------- .../ethereum/vm/Eip7709BlockHashLookup.java | 12 +++---- .../BlockchainBasedBlockHashLookupTest.java | 19 +++++++++++- .../vm/Eip7709BlockHashLookupTest.java | 31 ++++++++++++------- .../besu/evmtool/StateTestSubCommand.java | 4 +-- .../hyperledger/besu/evmtool/T8nExecutor.java | 9 ++---- .../besu/evm/blockhash/BlockHashLookup.java | 12 ++++++- .../evm/operation/BlockHashOperation.java | 18 +++++++++-- 8 files changed, 72 insertions(+), 45 deletions(-) diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/vm/BlockchainBasedBlockHashLookup.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/vm/BlockchainBasedBlockHashLookup.java index 9c09956ab..248a4216f 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/vm/BlockchainBasedBlockHashLookup.java +++ b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/vm/BlockchainBasedBlockHashLookup.java @@ -35,16 +35,12 @@ import java.util.Map; * all transactions within that block. */ public class BlockchainBasedBlockHashLookup implements BlockHashLookup { - private static final int MAX_RELATIVE_BLOCK = 256; - - private final long currentBlockNumber; private ProcessableBlockHeader searchStartHeader; private final Blockchain blockchain; private final Map hashByNumber = new HashMap<>(); public BlockchainBasedBlockHashLookup( final ProcessableBlockHeader currentBlock, final Blockchain blockchain) { - this.currentBlockNumber = currentBlock.getNumber(); this.searchStartHeader = currentBlock; this.blockchain = blockchain; hashByNumber.put(currentBlock.getNumber() - 1, currentBlock.getParentHash()); @@ -52,14 +48,6 @@ public class BlockchainBasedBlockHashLookup implements BlockHashLookup { @Override public Hash apply(final MessageFrame frame, final Long blockNumber) { - // If the current block is the genesis block or the sought block is - // not within the last 256 completed blocks, zero is returned. - if (currentBlockNumber == 0 - || blockNumber >= currentBlockNumber - || blockNumber < (currentBlockNumber - MAX_RELATIVE_BLOCK)) { - return ZERO; - } - final Hash cachedHash = hashByNumber.get(blockNumber); if (cachedHash != null) { return cachedHash; diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/vm/Eip7709BlockHashLookup.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/vm/Eip7709BlockHashLookup.java index 0627a4161..1b9fca3bf 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/vm/Eip7709BlockHashLookup.java +++ b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/vm/Eip7709BlockHashLookup.java @@ -74,13 +74,6 @@ public class Eip7709BlockHashLookup implements BlockHashLookup { @Override public Hash apply(final MessageFrame frame, final Long blockNumber) { - final long currentBlockNumber = frame.getBlockValues().getNumber(); - final long minBlockServe = Math.max(0, currentBlockNumber - blockHashServeWindow); - if (blockNumber >= currentBlockNumber || blockNumber < minBlockServe) { - LOG.trace("failed to read hash from system account for block {}", blockNumber); - return ZERO; - } - final Hash cachedHash = hashByNumber.get(blockNumber); if (cachedHash != null) { return cachedHash; @@ -105,4 +98,9 @@ public class Eip7709BlockHashLookup implements BlockHashLookup { hashByNumber.put(blockNumber, blockHash); return blockHash; } + + @Override + public long getLookback() { + return blockHashServeWindow; + } } diff --git a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/vm/BlockchainBasedBlockHashLookupTest.java b/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/vm/BlockchainBasedBlockHashLookupTest.java index 8504d6139..b9e70ee99 100644 --- a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/vm/BlockchainBasedBlockHashLookupTest.java +++ b/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/vm/BlockchainBasedBlockHashLookupTest.java @@ -16,6 +16,7 @@ package org.hyperledger.besu.ethereum.vm; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.Mockito.clearInvocations; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; @@ -27,10 +28,14 @@ import org.hyperledger.besu.ethereum.chain.Blockchain; import org.hyperledger.besu.ethereum.core.BlockHeader; import org.hyperledger.besu.ethereum.core.BlockHeaderTestFixture; import org.hyperledger.besu.evm.blockhash.BlockHashLookup; +import org.hyperledger.besu.evm.frame.BlockValues; import org.hyperledger.besu.evm.frame.MessageFrame; +import org.hyperledger.besu.evm.gascalculator.CancunGasCalculator; +import org.hyperledger.besu.evm.operation.BlockHashOperation; import java.util.Optional; +import org.apache.tuweni.bytes.Bytes; import org.assertj.core.api.Assertions; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; @@ -47,6 +52,7 @@ class BlockchainBasedBlockHashLookupTest { private BlockHeader[] headers; private BlockHashLookup lookup; private final MessageFrame messageFrameMock = mock(MessageFrame.class); + private final BlockValues blockValuesMock = mock(BlockValues.class); @BeforeEach void setUp() { @@ -138,7 +144,18 @@ class BlockchainBasedBlockHashLookupTest { } private void assertHashForBlockNumber(final int blockNumber, final Hash hash) { - Assertions.assertThat(lookup.apply(messageFrameMock, (long) blockNumber)).isEqualTo(hash); + clearInvocations(messageFrameMock, blockValuesMock); + + BlockHashOperation op = new BlockHashOperation(new CancunGasCalculator()); + when(messageFrameMock.getRemainingGas()).thenReturn(10_000_000L); + when(messageFrameMock.popStackItem()).thenReturn(Bytes.ofUnsignedInt(blockNumber)); + when(messageFrameMock.getBlockValues()).thenReturn(blockValuesMock); + when(messageFrameMock.getBlockHashLookup()).thenReturn(lookup); + when(blockValuesMock.getNumber()).thenReturn((long) CURRENT_BLOCK_NUMBER); + + op.execute(messageFrameMock, null); + + verify(messageFrameMock).pushStackItem(hash); } private BlockHeader createHeader(final int blockNumber, final BlockHeader parentHeader) { diff --git a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/vm/Eip7709BlockHashLookupTest.java b/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/vm/Eip7709BlockHashLookupTest.java index d9cf91d78..163b76e0f 100644 --- a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/vm/Eip7709BlockHashLookupTest.java +++ b/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/vm/Eip7709BlockHashLookupTest.java @@ -15,7 +15,6 @@ package org.hyperledger.besu.ethereum.vm; import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.clearInvocations; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.spy; @@ -35,17 +34,19 @@ import org.hyperledger.besu.evm.fluent.SimpleAccount; import org.hyperledger.besu.evm.fluent.SimpleWorld; import org.hyperledger.besu.evm.frame.BlockValues; import org.hyperledger.besu.evm.frame.MessageFrame; +import org.hyperledger.besu.evm.gascalculator.CancunGasCalculator; +import org.hyperledger.besu.evm.operation.BlockHashOperation; import org.hyperledger.besu.evm.worldstate.WorldUpdater; import java.util.ArrayList; import java.util.List; +import org.apache.tuweni.bytes.Bytes; import org.apache.tuweni.units.bigints.UInt256; -import org.assertj.core.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -public class Eip7709BlockHashLookupTest { +class Eip7709BlockHashLookupTest { private static final long BLOCKHASH_SERVE_WINDOW = 160; private static final Address STORAGE_ADDRESS = Address.fromHexString("0x0"); private static final long HISTORY_SERVE_WINDOW = 200L; @@ -58,9 +59,9 @@ public class Eip7709BlockHashLookupTest { @BeforeEach void setUp() { headers = new ArrayList<>(); - frame = createMessageFrame(CURRENT_BLOCK_NUMBER, createWorldUpdater(0, CURRENT_BLOCK_NUMBER)); lookup = new Eip7709BlockHashLookup(STORAGE_ADDRESS, HISTORY_SERVE_WINDOW, BLOCKHASH_SERVE_WINDOW); + frame = createMessageFrame(CURRENT_BLOCK_NUMBER, createWorldUpdater(0, CURRENT_BLOCK_NUMBER)); } private WorldUpdater createWorldUpdater(final int fromBlockNumber, final int toBlockNumber) { @@ -84,6 +85,8 @@ public class Eip7709BlockHashLookupTest { final BlockValues blockValues = mock(BlockValues.class); when(blockValues.getNumber()).thenReturn(currentBlockNumber); when(messageFrame.getBlockValues()).thenReturn(blockValues); + when(messageFrame.getBlockHashLookup()).thenReturn(lookup); + when(messageFrame.getRemainingGas()).thenReturn(10_000_000L); when(messageFrame.getWorldUpdater()).thenReturn(worldUpdater); return messageFrame; } @@ -100,7 +103,7 @@ public class Eip7709BlockHashLookupTest { @Test void shouldReturnEmptyHashWhenRequestedBlockHigherThanHead() { - assertThat(lookup.apply(frame, CURRENT_BLOCK_NUMBER + 20L)).isEqualTo(Hash.ZERO); + assertHashForBlockNumber(CURRENT_BLOCK_NUMBER + 20, Hash.ZERO); } @Test @@ -138,12 +141,9 @@ public class Eip7709BlockHashLookupTest { assertHashForBlockNumber(blockNumber3); assertHashForBlockNumber(blockNumber3); - verify(account, times(1)) - .getStorageValue(eq(UInt256.valueOf(blockNumber1 % HISTORY_SERVE_WINDOW))); - verify(account, times(1)) - .getStorageValue(eq(UInt256.valueOf(blockNumber2 % HISTORY_SERVE_WINDOW))); - verify(account, times(1)) - .getStorageValue(eq(UInt256.valueOf(blockNumber3 % HISTORY_SERVE_WINDOW))); + verify(account, times(1)).getStorageValue(UInt256.valueOf(blockNumber1 % HISTORY_SERVE_WINDOW)); + verify(account, times(1)).getStorageValue(UInt256.valueOf(blockNumber2 % HISTORY_SERVE_WINDOW)); + verify(account, times(1)).getStorageValue(UInt256.valueOf(blockNumber3 % HISTORY_SERVE_WINDOW)); verifyNoMoreInteractions(account); } @@ -177,7 +177,14 @@ public class Eip7709BlockHashLookupTest { } private void assertHashForBlockNumber(final int blockNumber, final Hash hash) { - Assertions.assertThat(lookup.apply(frame, (long) blockNumber)).isEqualTo(hash); + clearInvocations(frame); + + BlockHashOperation op = new BlockHashOperation(new CancunGasCalculator()); + when(frame.popStackItem()).thenReturn(Bytes.ofUnsignedInt(blockNumber)); + + op.execute(frame, null); + + verify(frame).pushStackItem(hash); } private BlockHeader createHeader(final long blockNumber, final BlockHeader parentHeader) { diff --git a/ethereum/evmtool/src/main/java/org/hyperledger/besu/evmtool/StateTestSubCommand.java b/ethereum/evmtool/src/main/java/org/hyperledger/besu/evmtool/StateTestSubCommand.java index b15aec6b9..07dbf8722 100644 --- a/ethereum/evmtool/src/main/java/org/hyperledger/besu/evmtool/StateTestSubCommand.java +++ b/ethereum/evmtool/src/main/java/org/hyperledger/besu/evmtool/StateTestSubCommand.java @@ -271,9 +271,7 @@ public class StateTestSubCommand implements Runnable { transaction, blockHeader.getCoinbase(), (__, blockNumber) -> - blockNumber >= blockHeader.getNumber() - ? Hash.ZERO - : Hash.hash(Bytes.wrap(Long.toString(blockNumber).getBytes(UTF_8))), + Hash.hash(Bytes.wrap(Long.toString(blockNumber).getBytes(UTF_8))), false, TransactionValidationParams.processingBlock(), tracer, diff --git a/ethereum/evmtool/src/main/java/org/hyperledger/besu/evmtool/T8nExecutor.java b/ethereum/evmtool/src/main/java/org/hyperledger/besu/evmtool/T8nExecutor.java index 14f1cce97..7a5650792 100644 --- a/ethereum/evmtool/src/main/java/org/hyperledger/besu/evmtool/T8nExecutor.java +++ b/ethereum/evmtool/src/main/java/org/hyperledger/besu/evmtool/T8nExecutor.java @@ -395,13 +395,8 @@ public class T8nExecutor { // from them so need to // add in a manual BlockHashLookup blockHashLookup = - (__, blockNumber) -> { - if (referenceTestEnv.getNumber() - blockNumber > 256L - || blockNumber >= referenceTestEnv.getNumber()) { - return Hash.ZERO; - } - return referenceTestEnv.getBlockhashByNumber(blockNumber).orElse(Hash.ZERO); - }; + (__, blockNumber) -> + referenceTestEnv.getBlockhashByNumber(blockNumber).orElse(Hash.ZERO); } result = processor.processTransaction( diff --git a/evm/src/main/java/org/hyperledger/besu/evm/blockhash/BlockHashLookup.java b/evm/src/main/java/org/hyperledger/besu/evm/blockhash/BlockHashLookup.java index 47f9414a4..cd27843de 100644 --- a/evm/src/main/java/org/hyperledger/besu/evm/blockhash/BlockHashLookup.java +++ b/evm/src/main/java/org/hyperledger/besu/evm/blockhash/BlockHashLookup.java @@ -25,4 +25,14 @@ import java.util.function.BiFunction; *

Arg is the current executing message frame. The Result is the Hash, which may be zero based on * lookup rules. */ -public interface BlockHashLookup extends BiFunction {} +public interface BlockHashLookup extends BiFunction { + + /** + * How far back from the current block are hash queries valid for? Default is 256. + * + * @return The number of blocks before the current that should return a hash value. + */ + default long getLookback() { + return 256L; + } +} diff --git a/evm/src/main/java/org/hyperledger/besu/evm/operation/BlockHashOperation.java b/evm/src/main/java/org/hyperledger/besu/evm/operation/BlockHashOperation.java index c88dc1370..60a16229e 100644 --- a/evm/src/main/java/org/hyperledger/besu/evm/operation/BlockHashOperation.java +++ b/evm/src/main/java/org/hyperledger/besu/evm/operation/BlockHashOperation.java @@ -17,11 +17,13 @@ package org.hyperledger.besu.evm.operation; import org.hyperledger.besu.datatypes.Hash; import org.hyperledger.besu.evm.EVM; import org.hyperledger.besu.evm.blockhash.BlockHashLookup; +import org.hyperledger.besu.evm.frame.BlockValues; import org.hyperledger.besu.evm.frame.ExceptionalHaltReason; import org.hyperledger.besu.evm.frame.MessageFrame; import org.hyperledger.besu.evm.gascalculator.GasCalculator; import org.apache.tuweni.bytes.Bytes; +import org.apache.tuweni.bytes.Bytes32; /** The Block hash operation. */ public class BlockHashOperation extends AbstractOperation { @@ -50,9 +52,21 @@ public class BlockHashOperation extends AbstractOperation { return new OperationResult(cost, null); } + final long soughtBlock = blockArg.toLong(); + final BlockValues blockValues = frame.getBlockValues(); + final long currentBlockNumber = blockValues.getNumber(); final BlockHashLookup blockHashLookup = frame.getBlockHashLookup(); - final Hash blockHash = blockHashLookup.apply(frame, blockArg.toLong()); - frame.pushStackItem(blockHash); + + // If the current block is the genesis block or the sought block is + // not within the lookback window, zero is returned. + if (currentBlockNumber == 0 + || soughtBlock >= currentBlockNumber + || soughtBlock < (currentBlockNumber - blockHashLookup.getLookback())) { + frame.pushStackItem(Bytes32.ZERO); + } else { + final Hash blockHash = blockHashLookup.apply(frame, soughtBlock); + frame.pushStackItem(blockHash); + } return new OperationResult(cost, null); }