Skip to content

fix withdrawal bal index tracking #28

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
nerolation wants to merge 1 commit into from
Closed

fix withdrawal bal index tracking #28

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/ethereum/forks/amsterdam/fork.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -792,11 +792,12 @@ def apply_body(
data=block_env.block_hashes[-1], # The parent hash
)

for i, tx in enumerate(map(decode_transaction, transactions)):
decoded_txs = list(map(decode_transaction, transactions))
Copy link
Collaborator

@raxhvl raxhvl Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mapping an array doesn’t change its length each element produces exactly one result, even if decoding fails.

Copy link
Collaborator Author

@nerolation nerolation Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, but I cannot see the issue here.

Copy link
Collaborator

@raxhvl raxhvl Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I placed an exit(1) after this code and the code path is not getting hit, so the bug is elsewhere. I noticed that amsterdam doesn't call self.fork.process_unchecked_system_transaction like prague and cancun, maybe that has something to do with this?

execution-specs/src/ethereum_spec_tools/evm_tools/t8n/__init__.py

Lines 255 to 271 in 5898499

if self.fork.is_after_fork("amsterdam"):
self.fork.set_block_access_index(
block_env.state.change_tracker, Uint(0)
)
if self.fork.is_after_fork("prague"):
self.fork.process_unchecked_system_transaction(
block_env=block_env,
target_address=self.fork.HISTORY_STORAGE_ADDRESS,
data=block_env.block_hashes[-1], # The parent hash
)
if self.fork.is_after_fork("cancun"):
self.fork.process_unchecked_system_transaction(
block_env=block_env,
target_address=self.fork.BEACON_ROOTS_ADDRESS,
data=block_env.parent_beacon_block_root,
)

Copy link
Owner

@fselmo fselmo Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that amsterdam doesn't call self.fork.process_unchecked_system_transaction like prague and cancun, maybe that has something to do with this?

Amsterdam is after both Prague and Cancun, all of these lines should be called by Amsterdam.

Copy link
Owner

@fselmo fselmo Oct 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The state of things here should be:

  • Amsterdam first sets the block access index before any of the system txs
  • Prague line should be hit processing history storage contract call
  • Cancun line is hit processing beacon roots contract call

Copy link
Owner

@fselmo fselmo Oct 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this being discussed? This could be an issue with the rebase after the weld, tbh. I will try filling and see if I notice anything strange but please provide more context here if you can.

Copy link
Owner

@fselmo fselmo Oct 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, #26 👍🏼

Copy link
Owner

@fselmo fselmo Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we're missing something else, an exit(1) is not hitting this line though:

execution-specs/src/ethereum/forks/amsterdam/fork.py

Line 799 in 5898499

post_execution_index = ulen(transactions) + Uint(1)

If it's not hitting that line then the transaction is failing, no? Without looking much into it if we never process_transaction appropriately is the only case we wouldn't hit that line? 👀

Copy link
Collaborator

@raxhvl raxhvl Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah super weird the transaction goes through and we get a BAL mismatch error:

E   execution_testing.test_types.block_access_list.exceptions.BlockAccessListValidationError: Balance change (2, 10000000000) not found or not in correct order. Actual changes: [(1, 10000000000)]

Copy link
Owner

@fselmo fselmo Oct 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found the culprit. I fixed it in the tests PR #23 @nerolation @raxhvl here: 2617d30. Sorry, seemed easier since I was in there already running the tests and now we can modify both spec and tests in the same repo / PR 🔥

Super quirky / tricky to find haha

for i, tx in enumerate(decoded_txs):
process_transaction(block_env, block_output, tx, Uint(i))

# EIP-7928: Post-execution uses block_access_index len(transactions) + 1
post_execution_index = ulen(transactions) + Uint(1)
post_execution_index = Uint(len(decoded_txs)) + Uint(1)
set_block_access_index(
block_env.state.change_tracker, post_execution_index
)
Expand Down
Loading