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 <[email protected]>

* spotless

Signed-off-by: Danno Ferrin <[email protected]>

* javadoc

Signed-off-by: Danno Ferrin <[email protected]>

* Update Unit Tests to use the operation.

Signed-off-by: Danno Ferrin <[email protected]>

* Update Unit Tests to use the operation.

Signed-off-by: Danno Ferrin <[email protected]>

---------

Signed-off-by: Danno Ferrin <[email protected]>

Author: shemnon

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

@@ -35,31 +35,19 @@
  * 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<Long, Hash> 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());
   }
 
   @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;

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

@@ -74,13 +74,6 @@ public Eip7709BlockHashLookup(final Address contractAddress, final long historyS
 
   @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 Hash apply(final MessageFrame frame, final Long blockNumber) {
     hashByNumber.put(blockNumber, blockHash);
     return blockHash;
   }
+
+  @Override
+  public long getLookback() {
+    return blockHashServeWindow;
+  }
 }

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

@@ -16,6 +16,7 @@
 
 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.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 @@ private void assertHashForBlockNumber(final int blockNumber) {
   }
 
   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) {

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.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 @@ private MessageFrame createMessageFrame(
     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 @@ void shouldGetHashOfMaxBlocksBehind() {
 
   @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 @@ void shouldCacheBlockHashes() {
     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 @@ private void assertHashForBlockNumber(final int blockNumber) {
   }
 
   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) {

ethereum/evmtool/src/main/java/org/hyperledger/besu/evmtool/StateTestSubCommand.java

@@ -271,9 +271,7 @@ private void traceTestSpecs(final String test, final List<GeneralStateTestCaseEi
                 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,

ethereum/evmtool/src/main/java/org/hyperledger/besu/evmtool/T8nExecutor.java

@@ -395,13 +395,8 @@ static T8nResult runTest(
           // 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(

evm/src/main/java/org/hyperledger/besu/evm/blockhash/BlockHashLookup.java

@@ -25,4 +25,14 @@
  * <p>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<MessageFrame, Long, Hash> {}
+public interface BlockHashLookup extends BiFunction<MessageFrame, Long, Hash> {
+
+  /**
+   * 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;
+  }
+}

evm/src/main/java/org/hyperledger/besu/evm/operation/BlockHashOperation.java

@@ -17,11 +17,13 @@
 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 OperationResult execute(final MessageFrame frame, final EVM evm) {
       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);
   }