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-7623 #897

Closed
wants to merge 0 commits into from
Closed

Implement EIP-7623 #897

wants to merge 0 commits into from

Conversation

nerolation
Copy link

I gave implementing EIP-7623 a try and got this first draft ready.

The EIP can be found here.
There's a side-doc with some additional analysis on the EIP here.

Cute Animal Picture

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

Copy link
Member

@marioevz marioevz left a comment

Choose a reason for hiding this comment

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

Some minor fixes I had to make when writing the tests.

Another thing is that validate_transaction function now fails because calculate_intrinsic_cost returns two values, so line:

if calculate_intrinsic_cost(tx) > tx.gas:

has to be rewritten as:

if calculate_intrinsic_cost(tx)[0] > tx.gas:

src/ethereum/shanghai/fork.py Outdated Show resolved Hide resolved
@@ -613,6 +614,14 @@ 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
Copy link
Member

Choose a reason for hiding this comment

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

STANDARD_TOKEN_COST and TOTAL_COST_FLOOR_PER_TOKEN need to be imported from .fork_types.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks!
I replaced it for the imports

 TX_DATA_COST_PER_NON_ZERO,
 TX_DATA_COST_PER_ZERO,

src/ethereum/shanghai/fork.py Outdated Show resolved Hide resolved
floor = tokens_in_calldata * TOTAL_COST_FLOOR_PER_TOKEN + TX_BASE_COST
if gas_used < floor:
if floor > tx.gas:
raise
Copy link
Collaborator

Choose a reason for hiding this comment

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

We generally use ensure(<condition>, <exception>) to check for invalid conditions.


I'd also recommend choosing the exception type to raise. We have a few in exceptions.py (Maybe InvalidBlock?)

"""
data_cost = 0

zb = 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

We generally prefer that identifiers be fully spelled out. What does zb represent?

Copy link
Author

Choose a reason for hiding this comment

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

I was refering to zero-bytes. Agree that it makes sense to spell it out, fixed it.

@@ -617,8 +617,7 @@ def process_transaction(

floor = tokens_in_calldata * TOTAL_COST_FLOOR_PER_TOKEN + TX_BASE_COST
if gas_used < floor:
if floor > tx.gas:
raise
ensure(floor > tx.gas, InvalidBlock)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This may be backwards? It is now saying floor must be greater than tx.gas or else it'll raise InvalidBlock.

Copy link
Author

Choose a reason for hiding this comment

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

Right. Fixed it - thanks!

@nerolation
Copy link
Author

nerolation commented Apr 18, 2024

I believe I made a mistake here:
The fact if a transactions is valid should never be decided after the execution and I'm not deducting the floor price at the intrinsic gas calculation.

Edit: This explains why you raised that comment in the last ACDE, @marioevz. So good catch. It should have been "deducting the high price" initially and then, in the case of not hitting the floor, increase the refund.


tokens_in_calldata = zero_bytes + (len(tx.data) - zero_bytes) * 4

data_cost = tokens_in_calldata * TOTAL_COST_FLOOR_PER_TOKEN
Copy link

@rjnrohit rjnrohit Apr 19, 2024

Choose a reason for hiding this comment

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

It doesn't make sense to do

if gas_used < floor:

If the data_cost = tokens_in_calldata * TOTAL_COST_FLOOR_PER_TOKEN

the gas_used is always gonna be greater than floor

explaination:

floor = tokens_in_calldata * TOTAL_COST_FLOOR_PER_TOKEN + TX_BASE_COST
i.e
floor = data_cost + TX_BASE_COST

And intrinsic_cost = TX_BASE_COST + data_cost + something
Or intrinsic_cost = floor + something
Or intrinsic_cost >= floor

And gas_used = intrinsic_cost + something
Or gas_used >= intrinsic_cost
Or gas_used >= floor always

Copy link
Author

Choose a reason for hiding this comment

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

Accidentally closed. will reopen and fix that

@nerolation
Copy link
Author

Reopended at #934

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.

4 participants