Persist reverted account and storage slot lookups in JournaledState

[frisitano]

This PR modifies the logic of how JournaledState manages reverts for AccountLoaded and StorageChange journal entries.

When reverting, instead of removing Account and StorageSlot entries from State we mark the entries as cold. As such if they are later loaded they will already be in journaled state mitigating the requirement to load them again from the database. When they are requested again they will be marked as warm.

This is beneficial when finalizing the JournaledState object as it will include all accounts and storage slots which have been loaded during a transactions execution, regardless of whether they were reverted. This is a requirement for generating stateless witnesses. Currently the PrestateTracer does not include accounts and storage slots which have been accessed and then subsequently reverted.

We need stateless witnesses to use revm (and reth) as an execution engine for the type 1 prover which is under development at Polgyon and by association the shared sequencer we are developing at Fractal.

[frisitano]

Alternative implementation: #1438

[rakita]

I like this option better, and i think it would be good addition to revm.

In general there were few footguns where people asumed behavior from this PR, and get surprised when account gets reverted/deleted. And this gives more flexibiliy and align with others EVMs where access list is a different struct.

Will need to review this in detail, entry revert seems to be missing and I need to check what to do on create revert and sloads of reverted storage, a lot of edge cases there.

[frisitano]

In general there were few footguns where people asumed behavior from this PR, and get surprised when account gets reverted/deleted. And this gives more flexibiliy and align with others EVMs where access list is a different struct.

Could you elaborate on the point regarding access lists? I don't think this will have any implications in regards to how the access lists are managed with journaled state? From my understanding the access list is populated in journaled state via the initial_account_load method which is unchanged by this PR. I think the current mechanism is the correct approach.

Will need to review this in detail, entry revert seems to be missing and I need to check what to do on create revert and sloads of reverted storage, a lot of edge cases there.

Could you elaborate on what you are referring to with "entry revert" please?

With create the logic is unchanged, the account that is loaded remains marked as warm even if the create is reverted. I believe this aligns with the spec taken from EIP-2929:

In all cases, the gas cost is charged and the map is updated at the time that the opcode is being called. When a CREATE or CREATE2 opcode is called, immediately (ie. before checks are done to determine whether or not the address is unclaimed) add the address being created to accessed_addresses, but gas costs of CREATE and CREATE2 are unchanged. Clarification: If a CREATE/CREATE2 operation fails later on, e.g during the execution of initcode or has insufficient gas to store the code in the state, the address of the contract itself remains in access_addresses (but any additions made within the inner scope are reverted).

For reverted sloads and (sloads associated with sstores), the loaded slot will remain in the journaled state but will marked as cold. I think this is correct behaviour.

We certainly need to test edge cases thoroughly. How would you propose we go about this?

[frisitano]

I'm not sure on the specifics you are referring to around access list handling but If we wanted to add the ability to assess if an account was loaded due to an access list then we could extend the AccountStatus bitflag to add a marker for this:

// The `bitflags!` macro generates `struct`s that manage a set of flags.
bitflags! {
    #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
    #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
    #[cfg_attr(feature = "serde", serde(transparent))]
    pub struct AccountStatus: u8 {
        ...
        /// used to mark account as cold
        const Cold = 0b0010000;
        /// used to mark and access list account
        const AccessList = 0b0100000;
    }
}

We could also introduce bitflags on the SlotStorage to follow this pattern. However, I propose this is considered and implemented as a separate PR.

[rakita]

In general there were few footguns where people asumed behavior from this PR, and get surprised when account gets reverted/deleted. And this gives more flexibiliy and align with others EVMs where access list is a different struct.

Could you elaborate on the point regarding access lists? I don't think this will have any implications in regards to how the access lists are managed with journaled state? From my understanding the access list is populated in journaled state via the initial_account_load method which is unchanged by this PR. I think the current mechanism is the correct approach.

In other EVMs, the access list is a different HashMap and it is disconnected from the internal state, which another HashMap. I was just making zoomed out comment that this PR aligns more with that, and this is desirable.

Will need to review this in detail, entry revert seems to be missing and I need to check what to do on create revert and sloads of reverted storage, a lot of edge cases there.

Could you elaborate on what you are referring to with "entry revert" please?

With create the logic is unchanged, the account that is loaded remains marked as warm even if the create is reverted. I believe this aligns with the spec taken from EIP-2929:

I didn't see it, I meant about this: https://github.com/bluealloy/revm/pull/1437/files#diff-32ab64096b0ec0ba2da45b3ed2c625d583845fb31fce99db138683b0178fdd44R322
journal_revert function. Seems okay but will double check it.

In all cases, the gas cost is charged and the map is updated at the time that the opcode is being called. When a CREATE or CREATE2 opcode is called, immediately (ie. before checks are done to determine whether or not the address is unclaimed) add the address being created to accessed_addresses, but gas costs of CREATE and CREATE2 are unchanged. Clarification: If a CREATE/CREATE2 operation fails later on, e.g during the execution of initcode or has insufficient gas to store the code in the state, the address of the contract itself remains in access_addresses (but any additions made within the inner scope are reverted).

For reverted sloads and (sloads associated with sstores), the loaded slot will remain in the journaled state but will marked as cold. I think this is correct behaviour.

We certainly need to test edge cases thoroughly. How would you propose we go about this?

tldr: I like this change, but will need to check edge cases and how they behave in code.
Was last week in EF interop and coming back to usually work

[rakita]

Awesome change!

[rakita]

Have made additional change to StorageSlot structures, now it is localised only to internal Evm state so is_cold is not leaked outside.

[rakita]

PR is good to go, but will need to check how this will propagate to inspectors

[rakita]

It shouldn't be disruptive.

@frisitano thank you for PR.

[frisitano]

It shouldn't be disruptive.

@frisitano thank you for PR.

Most welcome. Thank you for the review and support 🙏