fix(specs): EIP-7928 larger refactoring moving the state tracker and introduce check gas and charge gas sequence

[nerolation]

🗒️ Description

Related to #1722, we should charge for account access costs before actually marking the account accessed.
If there is not enough gas for the access, we should never expend the resources to access the account.
Also, this addresses re-architecting things incl the state tracker that moved into the block env (cc @SamWilsn ).

Test cases for all of these boundaries should be added as well.

These changes should likely be introduced into all forks if we decide this is the correct approach.

✅ Checklist

• All: Ran fast tox checks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:

  uvx tox -e static

• All: PR title adheres to the repo standard — it will be used as the squash commit message and should start with type(scope):.

• All: Considered updating the online docs in the ./docs/ directory.

• All: Set appropriate labels for the changes (only maintainers can apply labels).

Cute Animal Picture

[SamWilsn]

Two things before I dive into this comment. First, as far an interim implementation goes for generating tests, go wild and do whatever unblocks you. Second, I've only just started looking at this, so my suggestions are going to be rapid fire and poorly thought out.

But basically: can we express the state tracker as a stack of "frames" that have defined merging rules?

Something like:

1. Create block state tracking frame
   1. Create transaction state tracking frame
      1. Create call frame state tracking frame
      2. Merge call frame state tracking frame into transaction/parent frame
      3. Repeat until all bytecode is executed
   2. Merge transaction state tracking frame into block
   3. Repeat until all transactions are executed
2. Finalize block state tracking frame

Anyway, structurally, I think we want something like this:

In state_tracker.py

@dataclass
class StateChanges:
    storage_changes
    storage_reads
    balance_changes
    nonce_changes
    code_changes

def incorporate_state_changes_on_success(parent: StateChanges, child: StateChanges) -> None:
    """
    Conceptually this would take all the recorded changes from `child` and append them to
    the recorded changes in `parent`.
    """

def incorporate_state_changes_on_failure(parent: StateChanges, child: StateChanges) -> None:
    """
    Conceptually this would take all the recorded changes from `child` and append them to
    the recorded changes in `parent`, but without including writes.
    """

def track_address_access(changes: StateChanges, address: Address) -> None:
    ...

def track_storage_read(changes: StateChanges, address: Address, key: Bytes32) -> None:
    ...

def track_storage_write(changes: StateChanges, address: Address, key: Bytes32, new_value: U256) -> None:
    ...

def track_balance_change(changes: StateChanges, address: Address, new_balance: U256) -> None:
    ...

def track_nonce_change(changes: StateChanges, address: Address, new_nonce: Uint) -> None:
    ...

def track_code_change(changes: StateChanges, address: Address, new_code: Bytes) -> None:
    ...

In vm/__init__.py

class Evm:
    ...
    state_changes: StateChanges  # Add this
    ...

def incorporate_child_on_success(evm: Evm, child_evm: Evm):
    ...
    incorporate_state_changes_on_success(evm.state_changes, child_evm.state_changes) # Add this
    ...

def incorporate_child_on_error(evm: Evm, child_evm: Evm):
    ...
    incorporate_state_changes_on_error(evm.state_changes, child_evm.state_changes) # Add this
    ...

In vm/interpreter.py

def process_message_call(message: Message) -> MessageCallOutput:
    ...
    # Do pre- / post- transaction book keeping, merging the `Evm`'s `StateChanges` into the block's `StateChanges`.
    ...

[fselmo]

This isn't optional post-Amsterdam right? Should this be an arg and not a kwarg set to None here? We are missing passing the block_env in many places at the moment and 99% of BAL tests fail. I think this is indicative that we require this arg and we would've caught this even in the lint checks because it would be missing something that is required and not optional.

[fselmo]

I made this change and updated passing block_env where we need to in a PR to this branch here. This is not taking into account the larger changes @SamWilsn suggested at the moment. I can confirm all BAL tests fill with this PR and my updates.

---

edit: I linked to a separate PR for the refactoring mentioned in this review as well. Both approaches pass all BAL tests.

[fselmo]

But basically: can we express the state tracker as a stack of "frames" that have defined merging rules?

@SamWilsn I took today to iterate on a first pass of this and just finished a rough initial implementation. I'm sure it will need some iteration but it passes all EIP-7928 tests at the moment. I am OOO from tomorrow until 11/11, so @nerolation feel free to amend my branch (if we decide to go this route) and push any changes requested there.

I made the PR to this branch since this was already headed in the right direction. PR here: nerolation#4.