refactor(spec-specs): refactor EIP-7928 to use nested context frames

[fselmo]

🗒️ Description

Related to comment here.

This is a rough first-pass implementation but one that is passing all EIP-7928 tests at the moment. If we decide to go with this approach, this is a good base to get the tests released and we can make tweaks to the design as future PRs on top of eips/amsterdam/eip-7928.

✅ 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 type(scope):.

Cute Animal Picture

[SamWilsn]

Again, sorry for the scattered and disorganized feedback!

block_access_lists/rlp_utils.py

rlp_encode_block_access_list

It looks like you've reimplemented the logic from ethereum-rlp (treat a dataclass as a list of its children). If the encoding algorithm is standard RLP, you can probably just use encode on the BlockAccessList itself. If it requires slight tweaking, you could look at With. If you've already investigated those options, please disregard!

validate_block_access_list_against_execution

Probably doesn't belong in this file, since it has little to do with RLP.

Also, seems unused?

block_access_lists/builder.py

build_block_access_list

You should use to_be_bytes32 to convert to Bytes32.

I haven't reached StateChanges yet, so this comment might be premature (and if it is, and you're reading this, I forgot to remove it), but what's different between a BlockAccessListBuilder and StateChanges? They seem to have very similar fields.

block_access_lists/rlp_types.py

These aren't specifically RLP types, though they do get encoded to RLP. I'd name the file something different.

I'm not sure adding the type aliases from EIP-7928 actually make anything more clear. If we do want to go down the road of making semantically meaningful types, we should do it more broadly and uniformly.

We can add U16 to ethereum-types if you'd like.

The constants here seem out of place.

fork.py

process_transaction

It seems like you're constructing a second stack of frames. Unless there's a reason not to, I'd rather add a member to TransactionEnvironment .

Is increment_index replaceable with TransactionEnvironment.index_in_block + 1 maybe? I'm not 100% sure how index_in_block interacts with system txs.

state.py

Seems a bit odd that we're passing the StateChanges into these functions. If we're trying to cleanly separate the "storage of state" from "tracking state changes", we'd want to avoid mixing the two.

state_tracker.py

Overall comment, we don't use methods (def x(self, ...)). Everything should be a free function.

StateChanges

Should we introduce some named tuples or something? There are a lot of one and two element tuples here, and I feel like that could get confusing without human readable names.

How do the pre-state captures interact with reverts and nested frames and such? I'm assuming that these are captured pre-transaction, so it might make more sense to put this in TransactionEnvironment?

get_block_access_index

Should this be set when constructing the StateChanges, or can its value change and therefore needs to be recomputed?

merge_on_success / merge_on_failure

Would it be better if these asserted self.parent is not None? Is there a case where we call it where we don't know where we are in the stack?

handle_in_transaction_selfdestruct

I'd probably just rename this to track_selfdestruct (or track_self_destruct, whichever is consistent with our naming). All self-destructs in recent forks are in the same tx, right?

---

And this is all I have time for today!

[SamWilsn]

eoa_delegation.py

check_delegation

Probably want to make some kind of structure with named fields for the return type (and backport that to previous forks eventually).

apply_delegation_tracking

Rename to track_delegation maybe? Seems more consistent with the other tracking functions.

access_delegation

Noting only for completeness, but if we're deprecating this, we'd want to actually remove it instead.

environment.py

• I'd swap is_cold_access for is_warm_access, so we can avoid the cognitive overhead of not.
• These changes'll need to be backported to the other forks:

  - code = get_account(evm.message.block_env.state, address).code
  + state = evm.message.block_env.state
  + code = get_account(state, address).code

system.py

generic_create

Is this a bug in the existing forks, and if so, should the ordering change be made to all previous forks as well?

interpreter.py

Can we avoid a parent reference in StateChanges itself? I'd really prefer to avoid creating a second representation of the stack, instead relying on Evm.

create_call_frame

I assume this is a leftover, or will there be more stuff in here eventually?

get_message_state_frame

The name of this function is misleading since calling it multiple times would create separate StateChanges instances.

genesis.py

add_genesis_block

The type of Block.block_access_list is BlockAccessList but the return type of rlp.encode is Bytes.

---

Miscellaneous

• Should we rename Evm.accessed_addresses to Evm.warmed_addresses to avoid creating confusion between two types of "accessed" addresses?
• Some comments describe the behaviour of distant code. If we ever update that distant code, we won't know to update these comments, leading to a possibly confusing situation. Not sure how common this is in our codebase, but now that I see it, maybe we should go through and look?