Skip to content
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.

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

Implement EIP-4844 #867

Merged
merged 4 commits into from
Feb 8, 2024
Merged

Implement EIP-4844 #867

merged 4 commits into from
Feb 8, 2024

Conversation

gurukamath
Copy link
Collaborator

What was wrong?

EIP-4844 was not implemented in the specs

Related to Issue #848

How was it fixed?

Implemented EIP-4844. Updated the evm tools to account for the changes.

Cute Animal Picture

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

Copy link
Collaborator

@SamWilsn SamWilsn left a comment

Choose a reason for hiding this comment

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

I haven't really reviewed for correctness, just for readability and pythonic-ness.

src/ethereum/cancun/fork.py Outdated Show resolved Hide resolved
@@ -420,7 +438,8 @@ def apply_body(
chain_id: U64,
withdrawals: Tuple[Withdrawal, ...],
parent_beacon_block_root: Root,
) -> Tuple[Uint, Root, Root, Bloom, State, Root]:
excess_blob_gas: U64,
) -> Tuple[Uint, Root, Root, Bloom, State, Root, Uint]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we make a dataclass for this? It's getting really unwieldy.

src/ethereum/cancun/fork.py Show resolved Hide resolved
set_account_balance(env.state, sender, sender_balance_after_gas_fee)

preaccessed_addresses = set()
preaccessed_storage_keys = set()
preaccessed_addresses.add(env.coinbase)
if isinstance(tx, (AccessListTransaction, FeeMarketTransaction)):
if isinstance(
tx, (AccessListTransaction, FeeMarketTransaction, BlobTransaction)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we make (and backport) a has_access_list function? Not specifically for this pull request, just in general.

Copy link
Collaborator Author

@gurukamath gurukamath Feb 5, 2024

Choose a reason for hiding this comment

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

Would probably make the maintaining of these properties easier.

Edit: Apparently, mypy is unable to infer the narrowing of types if done inside another function.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You need to use User-Defined Type Guards.

src/ethereum/cancun/fork.py Outdated Show resolved Hide resolved
src/ethereum/cancun/vm/gas.py Outdated Show resolved Hide resolved
src/ethereum/utils/numeric.py Outdated Show resolved Hide resolved
src/ethereum/cancun/vm/gas.py Outdated Show resolved Hide resolved
src/ethereum/utils/numeric.py Outdated Show resolved Hide resolved
@gurukamath
Copy link
Collaborator Author

The currently failing CI is because of missing py.typed file in the consensus-specs that we use for the point evaluation pre-compile. Have already raised a PR

@gurukamath gurukamath merged commit 5e500d0 into ethereum:forks/cancun Feb 8, 2024
5 checks passed
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.

3 participants