Skip to content

Comments

Refactor includability check in forks#890

Closed
richardgreg wants to merge 15 commits intoethereum:masterfrom
richardgreg:includability-check-refactor
Closed

Refactor includability check in forks#890
richardgreg wants to merge 15 commits intoethereum:masterfrom
richardgreg:includability-check-refactor

Conversation

@richardgreg
Copy link
Contributor

@richardgreg richardgreg commented Feb 22, 2024

What was wrong?

Includability checks should be moved from process_transaction() to check_transaction()

Related to Issue #
Ref #799

How was it fixed?

  • Moved the check code block from process_transaction() to check_transaction()
  • Added the necessary function argument to check_transaction

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@richardgreg
Copy link
Contributor Author

Some process_transaction functions have ensure(validate_transaction(tx), InvalidBlock). Thought about moving them to process transaction too. What are your thoughts?

@richardgreg richardgreg changed the title [WIP]Refactor includability check in forks Refactor includability check in forks Feb 28, 2024
@richardgreg
Copy link
Contributor Author

@petertdavies could you take a quick look and state if I'm on the right path?

@richardgreg
Copy link
Contributor Author

@SamWilsn could you review this if you have the time? 🙂

@SamWilsn
Copy link
Contributor

I'll do my best! Trying to get #925 merged before we start refactoring in earnest.

@gurukamath
Copy link
Contributor

gurukamath commented Apr 22, 2024

@richardgreg There might be a few more checks that we might have to move to check_transaction. I have raised a PR to make these changes in cancun (See PR). You could use a similar template to port the changes over to the other forks.

@richardgreg
Copy link
Contributor Author

Awesome

@richardgreg
Copy link
Contributor Author

Hi @gurukamath, I just dealt with some conflicts while performing a rebase. I will get around to adding the checks with your template as soon as possible, but if this PR is okay, I'll appreciate it if you merge it so I don't have to rebase again when I get back to the task 😅 🙏🏾

@richardgreg
Copy link
Contributor Author

Does anyone know if these changes are still needed? I noticed it was approved two weeks ago but was never merged and now has a lot of conflicts

@SamWilsn
Copy link
Contributor

@Bbjj88h is a spam bot as far as I can tell.

@SamWilsn
Copy link
Contributor

Ah, sorry about the conflicts. We've removed the ensure function because it was hiding lines from code coverage in #932 & #944. I've assigned this to @petertdavies to walk you through and get this merged.

Comment on lines +329 to 337
sender = env.origin
sender_account = get_account(env.state, sender)
gas_fee = tx.gas * tx.gas_price
ensure(sender_account.nonce == tx.nonce, InvalidBlock)
ensure(sender_account.balance >= gas_fee + tx.value, InvalidBlock)
ensure(sender_account.code == bytearray(), InvalidBlock)

ensure(tx.gas <= gas_available, InvalidBlock)
sender_address = recover_sender(tx)
Copy link
Contributor

@gurukamath gurukamath Jan 17, 2025

Choose a reason for hiding this comment

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

Suggested change
sender = env.origin
sender_account = get_account(env.state, sender)
gas_fee = tx.gas * tx.gas_price
ensure(sender_account.nonce == tx.nonce, InvalidBlock)
ensure(sender_account.balance >= gas_fee + tx.value, InvalidBlock)
ensure(sender_account.code == bytearray(), InvalidBlock)
ensure(tx.gas <= gas_available, InvalidBlock)
sender_address = recover_sender(tx)
ensure(tx.gas <= gas_available, InvalidBlock)
sender_address = recover_sender(tx)
sender_account = get_account(env.state, sender_address)
gas_fee = tx.gas * tx.gas_price
ensure(sender_account.nonce == tx.nonce, InvalidBlock)
ensure(sender_account.balance >= gas_fee + tx.value, InvalidBlock)
ensure(sender_account.code == bytearray(), InvalidBlock)

This way, there is no need for env to be a parameter in the check_transaction function.

@gurukamath
Copy link
Contributor

This has been handled in #1131

@richardgreg richardgreg closed this by deleting the head repository Mar 26, 2025
danceratopz pushed a commit to danceratopz/execution-specs that referenced this pull request Oct 22, 2025
…thereum#890)

* put EmptyOmmersRoot in env file instead of 0 by default

* fix(types): tests

---------

Co-authored-by: Mario Vega <marioevz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants