EIP-2935 get() fails to revert on out-of-bounds input

[daejunpark]

Problem

According to the EIP-2935 spec, the valid input range for the get() operation is [block.number - HISTORY_SERVE_WINDOW, block.number - 1]. Additionally, as per #19, the get() operation should revert if the input falls outside this valid range.

However, when get() is called with an input of block.number - HISTORY_SERVE_WINDOW - 1, which is outside the valid range, it currently does NOT revert; instead it returns the blockhash of block.number - 1, which I believe is incorrect behavior.

How to reproduce

The issue can be reproduced by adding the following test to test/ExecutionHash.t.sol.in:

    function testReadBadBlockNumbers2() public {                                                                 
        // Set reasonable block number.                                                                          
        vm.roll(21053500);                                                                                       

        // Store hash for the last block.
        vm.store(unit, hash_idx(), hash);

        // Read hash from a block older than the lower bound.
        (bool ret, bytes memory data) = unit.call(bytes.concat(bytes32(block.number - buflen - 1)));             
        assertFalse(ret);
        assertEq(data, hex"");
    }

Test output:
(Note that the returned data is 0x88e96d4537bea4d9c05d12549907b32561d3bf31f45aae734cdc119f13406cb6, which is the blockhash set earlier for the last block.)

$ ./build-wrapper test --mt testReadBadBlockNumbers2 -vvvv
[⠊] Compiling...
[⠑] Compiling 1 files with Solc 0.8.27
[⠘] Solc 0.8.27 finished in 684.84ms
Compiler run successful!

Ran 1 test for test/ExecutionHash.t.sol:ContractTest
[FAIL: assertion failed] testReadBadBlockNumbers2() (gas: 7362)
Traces:
  [8214] ContractTest::setUp()
    ├─ [0] VM::etch(0x000000000000000000000000000000000000aaaa, 0x3373fffffffffffffffffffffffffffffffffffffffe14604b57602036146024575f5ffd5b5f3560014303811160465780430361200010604657611fff9006545f5260205ff35b505f5ffd5b5f35611fff60014303065500)
    │   └─ ← [Return] 
    └─ ← [Stop] 

  [7362] ContractTest::testReadBadBlockNumbers2()
    ├─ [0] VM::roll(21053500 [2.105e7])
    │   └─ ← [Return] 
    ├─ [0] VM::store(0x000000000000000000000000000000000000aaaa, 0x0000000000000000000000000000000000000000000000000000000000000a45, 0x88e96d4537bea4d9c05d12549907b32561d3bf31f45aae734cdc119f13406cb6)
    │   └─ ← [Return] 
    ├─ [226] 0x000000000000000000000000000000000000aaaa::00000000(0000000000000000000000000000000000000000000000000141203c)
    │   └─ ← [Return] 0x88e96d4537bea4d9c05d12549907b32561d3bf31f45aae734cdc119f13406cb6
    ├─ [0] VM::assertFalse(true) [staticcall]
    │   └─ ← [Revert] assertion failed
    └─ ← [Revert] assertion failed

Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 325.29µs (65.13µs CPU time)

Ran 1 test suite in 224.65ms (325.29µs CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/ExecutionHash.t.sol:ContractTest
[FAIL: assertion failed] testReadBadBlockNumbers2() (gas: 7362)

Encountered a total of 1 failing tests, 0 tests succeeded

Suggested fix

The root cause of this issue is the following line:

sys-asm/src/execution_hash/main.eas

Line 70 in b5b9f33

push BUFLEN+1 ;; [buflen, number - input, input]

This issue can be fixed by changing it to push BUFLEN (removing +1).

Once this issue is fixed, the existing testHistoricalReads() test will also need to be adjusted, e.g., by adding vm.roll(buflen); before the second for loop.

Patch: daejunpark@2a11c29

[daejunpark]

Although not directly related to this issue, it's worth noting that the BUFLEN constant is currently set to 8191, which differs from the HISTORY_SERVE_WINDOW value (8192) in EIP-2935.

Patch: daejunpark@a6af555

[fjl]

The EIP says to return zero outside the range.

https://eips.ethereum.org/EIPS/eip-2935#block-hash-history-contract

For any output outside the range of [block.number-HISTORY_SERVE_WINDOW, block.number-1] return 0.

It seems @lightclient changed this in the code but didn't update the EIP.

[daejunpark]

The EIP says to return zero outside the range.

https://eips.ethereum.org/EIPS/eip-2935#block-hash-history-contract

For any output outside the range of [block.number-HISTORY_SERVE_WINDOW, block.number-1] return 0.

It seems @lightclient changed this in the code but didn't update the EIP.

I agree with your point, but please note that this issue is a different thing. The current implementation neither returns zero nor reverts for the specific out-of-range input, block.number - HISTORY_SERVE_WINDOW - 1; instead, it incorrectly returns the hash for block.number - 1.

(Additionally, this is unrelated to the edge case of block.number == 0.)

[fjl]

This was fixed in 6cae17b

[daejunpark]

This was fixed in 6cae17b

Thanks for fixing it!

Could you please also check the ring buffer size (#34 (comment))? It's set to 8191 in the code, while it's 8192 in the eip spec. Which one is correct?

[fjl]

@lightclient ^^

[fjl]

It seems he is busy and has forgotten about us 😢

But a recent PR against the EIP clarifies things: ethereum/EIPs#9144. Ring buffer size of 8191 is the correct one, and the PR proposes to change the EIP based on the code here. The size of 8191 was chosen because it is prime, see https://eips.ethereum.org/EIPS/eip-4788#size-of-ring-buffers.

[jsign]

The size of 8191 was chosen because it is prime, see https://eips.ethereum.org/EIPS/eip-4788#size-of-ring-buffers.

@fjl, chatted with @lightclient out of band, and for EIP-2935 using a prime number isn't required. For 4788 makes sense since the value that is taking the modulo is timestamp which increases in 12 steps (i.e: would only use even slots). In the case of 2935, the value is blocknumber which increases by +1. 1 is coprime with any modulo, so 8192 would be fine (i.e: no "holes", since block num increases by +1, every write is always in the next slot number (wrapped around), in any modulo).

After our chat, the argument used now for 8191 is mainly using the same size in a similar construction (4788). The rationale was updated with this argument removing the prime one.

[fjl]

Thanks. That's actually a good point about it not using timestamp!

[daejunpark]

@fjl @jsign thank you so much for the clarification!