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

Add EIP-7623 implementation #934

Open
wants to merge 35 commits into
base: forks/prague
Choose a base branch
from

Conversation

nerolation
Copy link

Reopened for #897

@nerolation nerolation mentioned this pull request Apr 19, 2024
@SamWilsn
Copy link
Collaborator

SamWilsn commented May 6, 2024

This is meant for Prague right? We have our Prague branch at forks/prague.

Feel free to move this over there (both changing the base branch and editing the src/ethereum/prague directory instead), or I can probably edit the pull request if you'd like.

Fix: wrong tx type(0x03) and link to EIP4844
@nerolation
Copy link
Author

This is meant for Prague right? We have our Prague branch at forks/prague.

Feel free to move this over there (both changing the base branch and editing the src/ethereum/prague directory instead), or I can probably edit the pull request if you'd like.

How "set" are the changes in src/ethereum/prague?
We haven't had full consensus on the inclusion of 7623 into prague so I'd probably wait until the next ACDE for confirmation.

@SamWilsn
Copy link
Collaborator

SamWilsn commented May 7, 2024

We don't need to merge the pull request until it's ready according to ACDE. I'm suggesting that you make the base branch of your pull request forks/prague instead of master.

@nerolation nerolation changed the base branch from master to forks/prague May 8, 2024 06:29
@SamWilsn SamWilsn closed this May 27, 2024
@SamWilsn SamWilsn reopened this May 27, 2024
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.

Looks like you're still modifying the shanghai fork files, and not prague's

@@ -635,7 +636,13 @@ def process_transaction(

output = process_message_call(message, env)

gas_used = tx.gas - output.gas_left
floor = tokens_in_calldata * TOTAL_COST_FLOOR_PER_TOKEN + TX_BASE_COST
if gas_used + tokens_in_calldata * STANDARD_TOKEN_COST < floor:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be more readable if gas_used + tokens_in_calldata * STANDARD_TOKEN_COST was stored in an intermediate variable?

@@ -699,7 +706,7 @@ def validate_transaction(tx: Transaction) -> bool:
verified : `bool`
True if the transaction can be executed, or False otherwise.
"""
if calculate_intrinsic_cost(tx) > tx.gas:
if calculate_intrinsic_cost(tx)[0] > tx.gas:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if calculate_intrinsic_cost(tx)[0] > tx.gas:
intrinsic_gas = calculate_intrinsic_cost(tx)[0]
if intrinsic_gas > tx.gas:

Comment on lines +741 to +742
verified : `ethereum.base_types.Uint`
The calldata tokens used.
Copy link
Collaborator

Choose a reason for hiding this comment

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

You'll probably want to use better names for the tuple elements.

Comment on lines +765 to +767
return Uint(TX_BASE_COST +
data_cost +
create_cost + access_list_cost), tokens_in_calldata
Copy link
Collaborator

Choose a reason for hiding this comment

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

You'll need to update the return type of the function to Tuple[Uint, Uint] I think.

Comment on lines +23 to +24
TOTAL_COST_FLOOR_PER_TOKEN = 12
STANDARD_TOKEN_COST = 4
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
TOTAL_COST_FLOOR_PER_TOKEN = 12
STANDARD_TOKEN_COST = 4
TOTAL_COST_FLOOR_PER_CALLDATA_TOKEN = 12
CALLDATA_TOKEN_COST = 4

Unless there's a compelling reason to say "STANDARD" here, I'd prefer not including it.

How do you feel about making it explicit these are calldata related constants?

@nerolation
Copy link
Author

I've merged them here, @SamWilsn :

https://github.com/ethereum/execution-specs/pull/959/files

I'm not sure why the changelog includes other files/changes that I didn't touch.

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.

None yet

8 participants