search

ethereum / execution-specs

Specification for the Execution Layer. Tracking network upgrades.
Creative Commons Zero v1.0 Universal
831 stars 234 forks source link

Refactor includability check in forks #890

Open richardgreg opened 7 months ago

richardgreg commented 7 months ago

What was wrong?

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

Related to Issue # Ref #799

How was it fixed?

Cute Animal Picture

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

richardgreg commented 7 months ago

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

richardgreg commented 6 months ago

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

richardgreg commented 5 months ago

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

SamWilsn commented 5 months ago

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

gurukamath commented 5 months ago

@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 commented 5 months ago

Awesome

richardgreg commented 5 months ago

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 commented 4 months ago

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 commented 4 months ago

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

SamWilsn commented 4 months ago

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.