Merge main into verkle

[matkt]

PR description

Fixed Issue(s)

Thanks for sending a pull request! Have you done the following?

• Checked out our contribution guidelines?
• Considered documentation and added the doc-change-required label to this PR if updates are required.
• Considered the changelog and included an update if required.
• For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

• spotless: ./gradlew spotlessApply
• unit tests: ./gradlew build
• acceptance tests: ./gradlew acceptanceTest
• integration tests: ./gradlew integrationTest
• reference tests: ./gradlew ethereum:referenceTests:referenceTests

[matkt]

Tried and this PR is syncing the current devnet

[lu-pinto]

Removal of comments, intentional?

[matkt]

it's an old comment we added before your refactoring of the gascost . it seems to be useless to keep it now

[lu-pinto]

why is this if check present? it does not exist on my merge

[matkt]

it's a temporary code we have on verkle to trigger a rollback and rollfoward for each block when we are syncing and testing that everything is working correctly

[lu-pinto]

what is this method used for?

[matkt]

yes seems to be useless I removed it

[lu-pinto]

doesn't seem correct, we are using 0xff...fe

[matkt]

it will not be a problem because PragueBlockHashProcessor still use the correct with my modification . Now Eip7709BlockHashProcessor.java will have it's own address value and will not override the one from Prague

[lu-pinto]

doesn't seem correct, we are using 0xff...fe

[matkt]

it will not be a problem because PragueBlockHashProcessor still use the correct with my modification . Now Eip7709BlockHashProcessor.java will have it's own address value and will not override the one from Prague

[lu-pinto]

again not 0xfffffffffffffffffffffffffffffffffffffffe ?

[matkt]

it will not be a problem because PragueBlockHashProcessor still use the correct with my modification . Now Eip7709BlockHashProcessor.java will have it's own address value and will not override the one from Prague

[lu-pinto]

again not 0xfffffffffffffffffffffffffffffffffffffffe ?

[matkt]

it will not be a problem because PragueBlockHashProcessor still use the correct with my modification . Now Eip7709BlockHashProcessor.java will have it's own address value and will not override the one from Prague

[lu-pinto]

again not 0xfffffffffffffffffffffffffffffffffffffffe ?

[matkt]

it will not be a problem because PragueBlockHashProcessor still use the correct with my modification . Now Eip7709BlockHashProcessor.java will have it's own address value and will not override the one from Prague

[lu-pinto]

again not 0xfffffffffffffffffffffffffffffffffffffffe ?

[matkt]

it will not be a problem because PragueBlockHashProcessor still use the correct with my modification . Now Eip7709BlockHashProcessor.java will have it's own address value and will not override the one from Prague

[lu-pinto]

nit: extra new line

[matkt]

fixed

[lu-pinto]

nit: spacing

[lu-pinto]

if you do return, why do you need the else?
copy pasting my version:

    // If the sought block is negative, a future block, the current block, or not in the
    // lookback window, zero is returned.
    if (soughtBlock < 0
        || soughtBlock >= currentBlockNumber
        || soughtBlock < (currentBlockNumber - blockHashLookup.getLookback())) {
      frame.pushStackItem(Bytes32.ZERO);
      return new OperationResult(cost, null);
    }

    final long remainingGas = frame.getRemainingGas();
    final Hash blockHash = blockHashLookup.apply(frame, soughtBlock);
    final long lookupCost = remainingGas - frame.getRemainingGas();
    // give lookupCost back as it will be taken after
    frame.incrementRemainingGas(lookupCost);
    if (blockHash == null) {
      return new OperationResult(cost + lookupCost, ExceptionalHaltReason.INSUFFICIENT_GAS);
    }
    frame.pushStackItem(blockHash);

    return new OperationResult(cost + lookupCost, null);

[matkt]

yes we can remove it

[lu-pinto]

sorry you need to put this back before the check for OOG. I remembered the reason I did it this way - if you fail on OOG you want to be able to report exactly by how much have you failed on, so you need to decrement the gas cost here from the frame so you can get it correctly on the other side upon returning the failure.

[matkt]

I did a rollback of this modification

[lu-pinto]

I would prefer the version that was there in the verkle branch, reason: I reworked these tests while implementing the gas costs so the version on the verkle branch is more up to date:

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;
  private static final int CURRENT_BLOCK_NUMBER =
      Math.toIntExact(HISTORY_SERVE_WINDOW + BLOCKHASH_SERVE_WINDOW / 2);
  private List<BlockHeader> headers;
  private BlockHashLookup lookup;
  private MessageFrame frame;
  private WorldUpdater worldUpdater;

  @BeforeEach
  void setUp() {
    headers = new ArrayList<>();
    worldUpdater = createWorldUpdater(0, CURRENT_BLOCK_NUMBER);
    lookup =
        new Eip7709BlockHashLookup(STORAGE_ADDRESS, HISTORY_SERVE_WINDOW, BLOCKHASH_SERVE_WINDOW);
    frame = createMessageFrame(CURRENT_BLOCK_NUMBER, worldUpdater, Long.MAX_VALUE);
  }

  private WorldUpdater createWorldUpdater(final int fromBlockNumber, final int toBlockNumber) {
    WorldUpdater worldUpdaterMock = mock(WorldUpdater.class);
    SimpleAccount contractAccount = spy(new SimpleAccount(STORAGE_ADDRESS, 0, Wei.ZERO));
    when(worldUpdaterMock.get(STORAGE_ADDRESS)).thenReturn(contractAccount);
    BlockHeader parentHeader = null;
    for (int i = fromBlockNumber; i < toBlockNumber; i++) {
      final BlockHeader header = createHeader(i, parentHeader);
      headers.add(header);
      contractAccount.setStorageValue(
          UInt256.valueOf(i % HISTORY_SERVE_WINDOW), UInt256.fromBytes(header.getHash()));
      parentHeader = header;
    }
    return worldUpdaterMock;
  }

  private MessageFrame createMessageFrame(
      final long currentBlockNumber, final WorldUpdater worldUpdater, final long remainingGas) {
    final MessageFrame messageFrame = mock(MessageFrame.class);
    final BlockValues blockValues = mock(BlockValues.class);
    when(blockValues.getNumber()).thenReturn(currentBlockNumber);
    when(messageFrame.getBlockValues()).thenReturn(blockValues);
    when(messageFrame.getBlockHashLookup()).thenReturn(lookup);
    when(messageFrame.getWorldUpdater()).thenReturn(worldUpdater);
    when(messageFrame.getRemainingGas()).thenReturn(remainingGas);
    when(messageFrame.getAccessWitness()).thenReturn(NoopAccessWitness.get());
    return messageFrame;
  }

  @Test
  void shouldGetHashOfImmediateParent() {
    assertHashForBlockNumber(CURRENT_BLOCK_NUMBER - 1);
  }

  @Test
  void shouldGetHashOfMaxBlocksBehind() {
    assertHashForBlockNumber(Math.toIntExact(CURRENT_BLOCK_NUMBER - BLOCKHASH_SERVE_WINDOW));
  }

  @Test
  void shouldReturnZeroHashWhenSystemContractNotExists() {
    worldUpdater = new SimpleWorld();
    frame = createMessageFrame(CURRENT_BLOCK_NUMBER, worldUpdater, Long.MAX_VALUE);
    assertThat(lookup.apply(frame, CURRENT_BLOCK_NUMBER - 1L)).isEqualTo(Hash.ZERO);
  }

  @Test
  @SuppressWarnings("ReturnValueIgnored")
  void shouldDecrementRemainingGasFromFrame() {
    AccessWitness accessWitness = mock(AccessWitness.class);
    when(accessWitness.touchAndChargeStorageLoad(any(), any())).thenReturn(100L);
    frame = createMessageFrame(CURRENT_BLOCK_NUMBER, worldUpdater, 200L);
    when(frame.getAccessWitness()).thenReturn(accessWitness);
    lookup.apply(frame, CURRENT_BLOCK_NUMBER - 1L);
    verify(frame).decrementRemainingGas(eq(100L));
    verify(frame).getAccessWitness();
    verify(frame).getRemainingGas();
    verify(frame).getWorldUpdater();
    verifyNoMoreInteractions(frame);
  }

  @Test
  void insufficientGasReturnsNullBlockHash() {
    worldUpdater = new SimpleWorld();
    AccessWitness accessWitness = mock(AccessWitness.class);
    when(accessWitness.touchAndChargeStorageLoad(any(), any())).thenReturn(100L);
    frame = createMessageFrame(CURRENT_BLOCK_NUMBER, worldUpdater, 1L);
    when(frame.getAccessWitness()).thenReturn(accessWitness);
    final Hash blockHash = lookup.apply(frame, CURRENT_BLOCK_NUMBER - 1L);
    assertThat(blockHash).isNull();
  }

  @Test
  void shouldReturnZeroHashWhenParentBlockNotInContract() {
    worldUpdater = createWorldUpdater(CURRENT_BLOCK_NUMBER - 10, CURRENT_BLOCK_NUMBER);
    frame = createMessageFrame(CURRENT_BLOCK_NUMBER, worldUpdater, Long.MAX_VALUE);
    lookup =
        new Eip7709BlockHashLookup(STORAGE_ADDRESS, HISTORY_SERVE_WINDOW, BLOCKHASH_SERVE_WINDOW);
    assertHashForBlockNumber(CURRENT_BLOCK_NUMBER - 20, Hash.ZERO);
  }

  @Test
  void shouldCacheBlockHashes() {
    worldUpdater = createWorldUpdater(0, CURRENT_BLOCK_NUMBER);
    frame = createMessageFrame(CURRENT_BLOCK_NUMBER, worldUpdater, Long.MAX_VALUE);
    final Account account = worldUpdater.get(STORAGE_ADDRESS);
    clearInvocations(account);

    int blockNumber1 = CURRENT_BLOCK_NUMBER - 1;
    int blockNumber2 = CURRENT_BLOCK_NUMBER - 4;
    int blockNumber3 = CURRENT_BLOCK_NUMBER - 5;
    assertHashForBlockNumber(blockNumber1);
    assertHashForBlockNumber(blockNumber1);
    assertHashForBlockNumber(blockNumber2);
    assertHashForBlockNumber(blockNumber3);
    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)));
    verifyNoMoreInteractions(account);
  }

  @Test
  void shouldGetHashWhenParentIsGenesis() {
    worldUpdater = createWorldUpdater(0, 1);
    frame = createMessageFrame(1, worldUpdater, Long.MAX_VALUE);
    lookup =
        new Eip7709BlockHashLookup(STORAGE_ADDRESS, HISTORY_SERVE_WINDOW, BLOCKHASH_SERVE_WINDOW);
    assertHashForBlockNumber(0);
  }

  @Test
  void shouldReturnZeroWhenRequestedBlockEqualToImportingBlock() {
    assertHashForBlockNumber(CURRENT_BLOCK_NUMBER, Hash.ZERO);
  }

  @Test
  void shouldReturnZeroWhenRequestedBlockAheadOfCurrent() {
    assertHashForBlockNumber(CURRENT_BLOCK_NUMBER + 1, Hash.ZERO);
    assertHashForBlockNumber(CURRENT_BLOCK_NUMBER + 20, Hash.ZERO);
  }

  @Test
  void shouldReturnZeroWhenRequestedBlockTooFarBehindCurrent() {
    assertHashForBlockNumber(
        Math.toIntExact(CURRENT_BLOCK_NUMBER - BLOCKHASH_SERVE_WINDOW - 1), Hash.ZERO);
    assertHashForBlockNumber(2, Hash.ZERO);
  }

  private void assertHashForBlockNumber(final int blockNumber) {
    assertHashForBlockNumber(blockNumber, headers.get(blockNumber).getHash());
  }

  private void assertHashForBlockNumber(final int blockNumber, final Hash 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) {
    return new BlockHeaderTestFixture()
        .number(blockNumber)
        .parentHash(parentHeader != null ? parentHeader.getHash() : Hash.EMPTY)
        .buildHeader();
  }
}