Ethereum tests failures when upgrading from revm 9 to 10

[ufoscout]

When upgrading our project from revm 9 to 14, the following tests from the Ethereum suite fail with invalid state root:

ethereum_tests/12.2/BlockchainTests/GeneralStateTests/stRevertTest/RevertPrefoundEmptyCallOOG.json
ethereum_tests/12.2/BlockchainTests/GeneralStateTests/stRevertTest/RevertPrefoundEmptyOOG.json
ethereum_tests/12.2/BlockchainTests/GeneralStateTests/stRevertTest/TouchToEmptyAccountRevert.json
ethereum_tests/12.2/BlockchainTests/GeneralStateTests/stSStoreTest/InitCollision.json
ethereum_tests/12.2/BlockchainTests/GeneralStateTests/stStaticCall/static_call_OOG_additionalGasCosts2.json
ethereum_tests/12.2/BlockchainTests/GeneralStateTests/stZeroCallsRevert/ZeroValue_CALLCODE_ToEmpty_OOGRevert.json
ethereum_tests/12.2/BlockchainTests/GeneralStateTests/stZeroCallsRevert/ZeroValue_CALLCODE_ToOneStorageKey_OOGRevert.json
ethereum_tests/12.2/BlockchainTests/GeneralStateTests/stZeroCallsRevert/ZeroValue_CALL_ToEmpty_OOGRevert.json
ethereum_tests/12.2/BlockchainTests/GeneralStateTests/stZeroCallsRevert/ZeroValue_CALL_ToOneStorageKey_OOGRevert.json
ethereum_tests/12.2/BlockchainTests/GeneralStateTests/stZeroCallsRevert/ZeroValue_DELEGATECALL_ToEmpty_OOGRevert.json
ethereum_tests/12.2/BlockchainTests/GeneralStateTests/stZeroCallsRevert/ZeroValue_DELEGATECALL_ToOneStorageKey_OOGRevert.json
ethereum_tests/12.2/BlockchainTests/GeneralStateTests/stZeroCallsRevert/ZeroValue_SUICIDE_ToEmpty_OOGRevert.json
ethereum_tests/12.2/BlockchainTests/GeneralStateTests/stZeroCallsRevert/ZeroValue_SUICIDE_ToOneStorageKey_OOGRevert.json

This is the function that executes the transactions:

pub(crate) fn transact_commit_with_inspect<DB>(
    env: Env,
    db: DB,
) -> ExecutionResult
where
    DB: Database + DatabaseCommit,
{
    let mut evm = revm::Evm::builder()
        .with_db(db)
        .with_env_with_handler_cfg(EnvWithHandlerCfg::new_with_spec_id(
            env.into(),
            FORK_SPEC_ID, // Paris fork
        ))
        .append_handler_register(revm::inspector_handle_register)
        .build();

    evm.transact_commit().unwrap() // error handling omitted
}

revm dependency is declared as:

# Using "9.0" works as expected
revm = { version = "14.0", default-features = false, features = [
    "optional_eip3607",
    "serde",
] }

Would you happen to have any clue? Is there anything we should look into? It seems that all failing tests are somehow related to a tx revert

[mattsse]

could be a feature thing, blst, c-kzg

[ufoscout]

Same issue with:

revm = { version = "14.0", default-features = true, features = [
    "serde",
    "arbitrary",
    "asm-keccak",
    "dev",
    "kzg-rs",
] }

[rakita]

There is EIP (https://eips.ethereum.org/EIPS/eip-7610) that changed how an Account is considered empty, in essence, it requires storage to be empty.

In Revm we don't have that check as it is close to impossible to trigger on mainnet as there are only few empty accounts with storage and you would need to get a hash collision to trigger it.

Statetest are handmade with some storage, there is list in Revm here:

revm/bins/revme/src/cmd/statetest/runner.rs

Lines 115 to 125 in e477c7f

	// Test with some storage check.
	| "RevertInCreateInInit_Paris.json"
	| "RevertInCreateInInit.json"
	| "dynamicAccountOverwriteEmpty.json"
	| "dynamicAccountOverwriteEmpty_Paris.json"
	| "RevertInCreateInInitCreate2Paris.json"
	| "create2collisionStorage.json"
	| "RevertInCreateInInitCreate2.json"
	| "create2collisionStorageParis.json"
	| "InitCollision.json"
	| "InitCollisionParis.json"

[ufoscout]

@rakita
We have investigated more and we can reproduce the error with every REVM version >= 10.

Even by ignoring the InitialCollision.json test as shown in your code, there are still 12 other tests that fail with revm 10 and work as expected with revm 9:

GeneralStateTests/stRevertTest/RevertPrefoundEmptyCallOOG.json
GeneralStateTests/stRevertTest/RevertPrefoundEmptyOOG.json
GeneralStateTests/stRevertTest/TouchToEmptyAccountRevert.json
GeneralStateTests/stSStoreTest/InitCollision.json
GeneralStateTests/stStaticCall/static_call_OOG_additionalGasCosts2.json
GeneralStateTests/stZeroCallsRevert/ZeroValue_CALLCODE_ToEmpty_OOGRevert.json
GeneralStateTests/stZeroCallsRevert/ZeroValue_CALLCODE_ToOneStorageKey_OOGRevert.json
GeneralStateTests/stZeroCallsRevert/ZeroValue_CALL_ToEmpty_OOGRevert.json
GeneralStateTests/stZeroCallsRevert/ZeroValue_CALL_ToOneStorageKey_OOGRevert.json
GeneralStateTests/stZeroCallsRevert/ZeroValue_DELEGATECALL_ToEmpty_OOGRevert.json
GeneralStateTests/stZeroCallsRevert/ZeroValue_DELEGATECALL_ToOneStorageKey_OOGRevert.json
GeneralStateTests/stZeroCallsRevert/ZeroValue_SUICIDE_ToEmpty_OOGRevert.json
GeneralStateTests/stZeroCallsRevert/ZeroValue_SUICIDE_ToOneStorageKey_OOGRevert.json

Any idea?

[ufoscout]

@rakita Our tests are broken by this commit: 9955d9f

After that commit the DatabaseCommit::commit fn is called with a different set of changes. For example, in the case of the BlockchainTests/GeneralStateTests/stRevertTest/RevertPrefoundEmptyOOG.json test, the set of changes before the commit is:

{
    0x2adc25665018aa1fe0e6bc666dac8fc2697ff9ba: Account {
        info: AccountInfo {
            balance: 0,
            nonce: 0,
            code_hash: 0xc5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470,
            code: None,
        },
        storage: {},
        status: AccountStatus(
            Touched,
        ),
    },
    0xa94f5374fce5edbc8e2a8697c15331677e6ebf0b: Account {
        info: AccountInfo {
            balance: 999990700000,
            nonce: 1,
            code_hash: 0xc5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470,
            code: None,
        },
        storage: {},
        status: AccountStatus(
            Touched,
        ),
    },
    0xa000000000000000000000000000000000000000: Account {
        info: AccountInfo {
            balance: 1,
            nonce: 0,
            code_hash: 0x7b5b62ce77e883bfb4a35d856b28fe6d3d38b2d689920b7f6ff96a1d8df3cbea,
            code: Some(
                LegacyRaw(
                    0x602060006000f0600055622fffff60002000,
                ),
            ),
        },
        storage: {},
        status: AccountStatus(
            0x0,
        ),
    },
}

but after that commit the changes are:

{
    0xa94f5374fce5edbc8e2a8697c15331677e6ebf0b: Account {
        info: AccountInfo {
            balance: 999990700000,
            nonce: 1,
            code_hash: 0xc5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470,
            code: None,
        },
        storage: {},
        status: AccountStatus(
            Touched,
        ),
    },
    0xa000000000000000000000000000000000000000: Account {
        info: AccountInfo {
            balance: 1,
            nonce: 0,
            code_hash: 0x7b5b62ce77e883bfb4a35d856b28fe6d3d38b2d689920b7f6ff96a1d8df3cbea,
            code: Some(
                LegacyRaw(
                    0x602060006000f0600055622fffff60002000,
                ),
            ),
        },
        storage: {
            0: EvmStorageSlot {
                original_value: 0,
                present_value: 0,
                is_cold: true,
            },
        },
        status: AccountStatus(
            0x0,
        ),
    },
    0x7db299e0885c85039f56fa504a13dd8ce8a56aa7: Account {
        info: AccountInfo {
            balance: 0,
            nonce: 0,
            code_hash: 0xc5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470,
            code: None,
        },
        storage: {},
        status: AccountStatus(
            Cold,
        ),
    },
    0x2adc25665018aa1fe0e6bc666dac8fc2697ff9ba: Account {
        info: AccountInfo {
            balance: 0,
            nonce: 0,
            code_hash: 0xc5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470,
            code: None,
        },
        storage: {},
        status: AccountStatus(
            Touched,
        ),
    },
}

Should we ignore everything marked as cold?

[rakita]

Should we ignore everything marked as cold?

This is the solution. It is more performant if I don't remove the item from the hashmap, and it is helpful (a nice feature) to see all loaded accounts.

For info, I am doing a lot of refactoring in the current main branch (PR #1865). I am not sure about your timeline, but first version of this change should be made in a next few weeks. This is part of making the Revm an Evm Framework.

[ufoscout]

Should we ignore everything marked as cold?

This is the solution.

Great! Should we also ignore storage slots with is_cold: true?