refactor(spec-specs): Refactor state changes and frame hierarchy #1841

fselmo · 2025-12-03T19:48:33Z

🗒️ Description

Refactor state tracking frames to be part of BlockEnvironment, TransactionEnvironment, and Message. This removes much of the unnecessary complexity related to parent frame tracking.
Remove state_changes from State functions, opt to capture changes before and after (from comments here)
General cleanup in spec code - Note: builder and rlp_utils have not been cleaned up here

🔗 Related Issues or PRs

#1836

✅ 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):.
All: Considered adding an entry to CHANGELOG.md.
All: Set appropriate labels for the changes (only maintainers can apply labels).

Cute Animal Picture

codecov · 2025-12-04T01:48:11Z

Codecov Report

❌ Patch coverage is 12.29050% with 157 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.45%. Comparing base (4a3a7e0) to head (2478c13).
⚠️ Report is 5 commits behind head on eips/amsterdam/eip-7928.

Files with missing lines	Patch %	Lines
src/ethereum/forks/amsterdam/state_tracker.py	10.14%	62 Missing ⚠️
src/ethereum/forks/amsterdam/vm/interpreter.py	4.00%	24 Missing ⚠️
...ethereum/forks/amsterdam/vm/instructions/system.py	8.00%	23 Missing ⚠️
src/ethereum/forks/amsterdam/fork.py	0.00%	22 Missing ⚠️
...thereum/forks/amsterdam/vm/instructions/storage.py	5.88%	16 Missing ⚠️
src/ethereum/forks/amsterdam/vm/eoa_delegation.py	10.00%	9 Missing ⚠️
src/ethereum/forks/amsterdam/utils/message.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@                     Coverage Diff                     @@
##           eips/amsterdam/eip-7928    #1841      +/-   ##
===========================================================
- Coverage                    82.46%   82.45%   -0.01%     
===========================================================
  Files                          797      797              
  Lines                        47987    47990       +3     
  Branches                      4341     4331      -10     
===========================================================
- Hits                         39574    39572       -2     
- Misses                        7927     7932       +5     
  Partials                       486      486

Flag	Coverage Δ
unittests	`82.45% <12.29%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

gurukamath · 2025-12-04T02:32:55Z

src/ethereum/forks/amsterdam/vm/__init__.py

    disable_precompiles: bool
    parent_evm: Optional["Evm"]
-    transaction_state_changes: StateChanges
+    is_create: bool = False


I think in case of Message and TransactionEnvironment, I'd prefer not to rely on default values of the dataclass. I think the specs are more readable when we explicitly define all the attributes of these objects at the time of instantiation. Same for BlockEnvironment

I fixed this for is_create. Do you think the same for state_changes or do you think this is fine to instantiate with a blank slate there?

gurukamath · 2025-12-04T08:52:20Z

src/ethereum/forks/amsterdam/state_tracker.py

 def create_child_frame(parent: StateChanges) -> StateChanges:
    """
-    Create a child frame for nested execution.
+    Create a child frame linked to the given parent.


The other thing we need to consider here is if we are ok with the _block_access_index defaulting to 0 for every child frame. I think every child frame should get the same _block_access_index as it parent.

Agreed. I also think this can have the underscore removed? I will include this here.

gurukamath · 2025-12-04T08:58:09Z

src/ethereum/forks/amsterdam/vm/interpreter.py

-    parent_frame = get_parent_frame(message)
-    create_frame = create_child_frame(parent_frame)
+    block_access_index = get_block_access_index(
+        message.block_env.block_state_changes


Maybe rename this to block_env.state_changes to stay consistent with the tx_env.state_changes and message.state_changes

gurukamath · 2025-12-04T08:59:42Z

src/ethereum/forks/amsterdam/vm/interpreter.py

+    increment_nonce(state, message.current_target)
+    nonce_after = get_account(state, message.current_target).nonce
+    track_nonce_change(
+        message.state_changes,


If we make the _block_access_index directly available in every child frame, we would not need to pass it as a function parameter

gurukamath · 2025-12-04T09:08:12Z

src/ethereum/forks/amsterdam/vm/interpreter.py

-            set_code(
-                state, message.current_target, contract_code, create_frame
+            # Track code change for contract creation
+            pre_code = get_account(state, message.current_target).code


I think this will always be b`` since we check if the target has code or nonce before going ahead with contract creation

gurukamath · 2025-12-04T09:13:26Z

src/ethereum/forks/amsterdam/fork.py

-        BlockAccessIndex(
-            get_block_access_index(block_env.block_state_changes)
-        ),
+        block_access_index,


If we intend to call this function beyond a transaction processing, might be best to rename it to normalize_balance_changes instead

I think we can normalize all net-zero filtering here once we remove that from merge_on_success as I mentioned before. And yeah, agreed, this is called on withdrawals too so the naming should be better here.

gurukamath · 2025-12-04T09:16:23Z

src/ethereum/forks/amsterdam/state.py

    recipient_address: Address,
    amount: U256,
-    state_changes: StateChanges,
 ) -> None:


Note to self: Discuss the reasoning behind separating this out.

gurukamath · 2025-12-04T09:47:35Z

src/ethereum/forks/amsterdam/state_tracker.py

-        if (addr, key) in child_frame.pre_storage:
-            if child_frame.pre_storage[(addr, key)] != value:
+        if (addr, key) in tx_frame.pre_storage:
+            if tx_frame.pre_storage[(addr, key)] != value:


I am thinking of the following scenario. tx_frame.pre_storage is 1. The parent_frame of the current frame changes it to 2 and the current frame changes it back to 1. Would the changes made by the current frame be ignored because the value is the same as in the tx_frame but the changes in the parent_frame be captured. Thus leading us to believe the overall change was 1->2 while in reality it was 1->2->1

Yes! I think this is definitely broken and I wrote a test to make sure here :). I think this should be simplified quite a bit. The children should just merge all their changes to the parent's changes to be honest, to update the state of the parent after processing the child frame, and the net-zero filtering should only be done at the commit_transaction_frame level. That's the reason that has its own method anyhow since the only difference between the top-level transaction frame committing and merge_on_success for child frames is that net-zero changes need to be filtered out.

I did check this before and after and it was broken but now passes those nested tests. Good tests to have 👍🏼

fselmo added A-spec-specs Area: Specification—The Ethereum specification itself (eg. `src/ethereum/*`) C-refactor Category: refactor labels Dec 3, 2025

refactor(spec-specs): Refactor state changes and their frames

06d9476

fselmo force-pushed the refactor/state-changes-and-frames branch from 7e81fd1 to af5d4ef Compare December 3, 2025 20:14

chore(spec-specs): cleanup BAL logic; organize gas check for SSTORE

4569117

fselmo force-pushed the refactor/state-changes-and-frames branch from af5d4ef to 4569117 Compare December 3, 2025 20:15

fselmo marked this pull request as ready for review December 3, 2025 20:51

fselmo requested a review from gurukamath December 3, 2025 20:51

fselmo mentioned this pull request Dec 3, 2025

refactor(bal): Align BAL state_changes with the specs Env hierarchy #1836

Open

2 tasks

gurukamath reviewed Dec 4, 2025

View reviewed changes

refactor(spec-specs): Changes from comments on PR ethereum#1841

2478c13

refactor(spec-specs): Refactor state changes and frame hierarchy #1841

Are you sure you want to change the base?

refactor(spec-specs): Refactor state changes and frame hierarchy #1841

Uh oh!

Conversation

fselmo commented Dec 3, 2025

🗒️ Description

🔗 Related Issues or PRs

✅ Checklist

Cute Animal Picture

Uh oh!

codecov bot commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Dec 4, 2025 •

edited

Loading