ethereum / tests

Common tests for all Ethereum implementations
MIT License
558 stars 319 forks source link

Suspicious EIP-4844 blob test #1343

Closed jangko closed 8 months ago

jangko commented 8 months ago

https://github.com/ethereum/tests/blob/5bf1fff257c0662bc526c27d7546d470025b3239/src/GeneralStateTestsFiller/Pyspecs/cancun/eip4844_blobs/test_blob_txs.py#L656

https://github.com/ethereum/tests/blob/5bf1fff257c0662bc526c27d7546d470025b3239/src/GeneralStateTestsFiller/Pyspecs/cancun/eip4844_blobs/test_blob_txs.py#L658

The above test looks suspicious compared to the EIP-4844 spec.

            # ensure that the user was willing to at least pay the current blob base fee
            assert tx.max_fee_per_blob_gas >= get_blob_base_fee(block.header)

Both RHS and LHS of the test is 1, so it should not become invalid according to the spec?

winsvega commented 8 months ago

@marioevz

spencer-tb commented 8 months ago

Just checked over this!

We use excessBlobGas=2359296 within the parameterized test case, and using that within:

def get_blob_base_fee(header: Header) -> int:
    return fake_exponential(
        MIN_BLOB_BASE_FEE,
        header.excess_blob_gas,
        BLOB_BASE_FEE_UPDATE_FRACTION
    )

We get a result of 2, hence for:

assert tx.max_fee_per_blob_gas >= get_blob_base_fee(block.header)

We end up with assert 1 >= 2.

I think we should add more detail to the comments here.

jangko commented 8 months ago

The problematic test is this file: https://github.com/ethereum/tests/blob/develop/GeneralStateTests/Pyspecs/cancun/eip4844_blobs/invalid_tx_max_fee_per_blob_gas_state.json


header.excess_blob_gas = 0
tx.max_fee_per_blob_gas = 1
get_blob_base_fee(block.header) = 1

The test will produce assert 1 >= 1 which is still valid according to the spec. But the test expect it to be invalid.

spencer-tb commented 8 months ago

From that file I read test 1: test_invalid_tx_max_fee_per_blob_gas_state[fork_Cancun-state_test-insufficient_max_fee_per_blob_gas with:

header.excess_blob_gas = 0x240000
tx.max_fee_per_blob_gas = 0x1
get_blob_base_fee(block.header) = 2
assert 1 >= 2

And then test 2, test_invalid_tx_max_fee_per_blob_gas_state[fork_Cancun-state_test-invalid_max_fee_per_blob_gas] with:

header.excess_blob_gas = 0x0
tx.max_fee_per_blob_gas = 0x0
get_blob_base_fee(block.header) = 1
assert 0 >= 1
jangko commented 8 months ago

Argh, turn out there is a subtle bug in my json parser. Thanks for the help @spencer-tb