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.

Sign up for GitHub

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

Jump to bottom

Implement EIP-7623 "Increase calldata cost" #1095

Merged
merged 3 commits into from
Jan 20, 2025
Merged

Implement EIP-7623 "Increase calldata cost" #1095

merged 3 commits into from
Jan 20, 2025

Conversation

chfast
Copy link
Member

@chfast chfast commented Jan 7, 2025 •
edited by rodiazet
Loading

This implements EIP-7623.
In effect, during transaction validation we compute minimal transaction
gas cost. After transaction execution the amount of gas used is
increased to this value if lower.

Requires #1107 #1108. Merged

@chfast chfast added the Prague Changes for Prague upgrade label Jan 7, 2025
@chfast chfast force-pushed the eip7623_calldata_cost branch 2 times, most recently from 40bb192 to 2ca9f63 Compare January 9, 2025 12:50
@codecov Codecov
Copy link

codecov bot commented Jan 9, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.84%. Comparing base (34ce775) to head (9899329).
Report is 4 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1095      +/-   ##
==========================================
- Coverage   94.31%   93.84%   -0.47%     
==========================================
  Files         159      159              
  Lines       17298    17330      +32     
==========================================
- Hits        16314    16263      -51     
- Misses        984     1067      +83
Flag Coverage Δ
eof_execution_spec_tests 18.61% <24.32%> (-5.20%) ⬇️
ethereum_tests 26.85% <24.32%> (-0.03%) ⬇️
ethereum_tests_silkpre 18.93% <27.58%> (-0.02%) ⬇️
execution_spec_tests 20.06% <29.72%> (-1.23%) ⬇️
unittests 88.99% <94.59%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
test/state/state.cpp 98.44% <100.00%> (+0.01%) ⬆️
test/state/transaction.hpp 100.00% <ø> (ø)
test/statetest/statetest_runner.cpp 97.14% <100.00%> (+0.26%) ⬆️
test/unittests/state_transition_tx_test.cpp 100.00% <100.00%> (ø)
test/unittests/state_tx_test.cpp 99.10% <100.00%> (+0.03%) ⬆️

... and 3 files with indirect coverage changes

@chfast chfast force-pushed the eip7623_calldata_cost branch from 2ca9f63 to e93ca40 Compare January 13, 2025 15:41
@chfast chfast self-assigned this Jan 16, 2025
@chfast chfast force-pushed the eip7623_calldata_cost branch from 95cc90a to d09ea2f Compare January 16, 2025 17:55
@chfast chfast changed the title EIP-7623: Increase calldata cost Implement EIP-7623 "Increase calldata cost" Jan 16, 2025
@chfast chfast requested review from gumb0 and pdobacz January 16, 2025 17:56
@chfast chfast marked this pull request as ready for review January 16, 2025 17:56
@chfast chfast force-pushed the eip7623_calldata_cost branch 2 times, most recently from e4267d7 to 23014df Compare January 16, 2025 19:12
rodiazet
rodiazet reviewed Jan 17, 2025
View reviewed changes
test/state/transaction.hpp Show resolved Hide resolved
test/unittests/state_tx_test.cpp Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add one more test which tests two different case:

  1. Case in which tx gas cost is the intrinsic. So the ratio of calldata gas and execution gas is below threshold.
  2. Case where we are above threshold and we have tx cost calculated calculated based on TOTAL_COST_FLOOR_PER_TOKEN
    It should be possible just by extending call data size until we get into second case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added 3 execution test cases. Please check if these are what you requested.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. Thanks.

@chfast chfast force-pushed the eip7623_calldata_cost branch from 23014df to 5c58a1b Compare January 17, 2025 13:47
gumb0
gumb0 reviewed Jan 17, 2025
View reviewed changes
test/state/state.cpp
@@ -497,6 +510,9 @@ TransactionReceipt transition(const StateView& state_view, const BlockInfo& bloc
gas_used -= refund;
assert(gas_used > 0);

// EIP-7623: The gas used by the transaction must be at least the min_gas_cost.
gas_used = std::max(gas_used, tx_props.min_gas_cost);
Copy link
Member

Choose a reason for hiding this comment

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

So in the end gas used may be higher than transaction gas limit? wild

Copy link
Member

Choose a reason for hiding this comment

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

Ah no, I guess not, because it's checked in transaction validity

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct. The tx gas limit must be big enough during validation to cover the minimal gas cost.

Copy link
Member

@gumb0 gumb0 Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hm in EIP max operation left side is

STANDARD_TOKEN_COST * tokens_in_calldata
+ execution_gas_used
+ isContractCreation * (32000 + INITCODE_WORD_COST * words(calldata))

but here it is, I think

21000
+ STANDARD_TOKEN_COST * tokens_in_calldata
+ execution_gas_used
+ isContractCreation * (32000 + INITCODE_WORD_COST * words(calldata))

because 21000 was included in intrinsic_cost?

Copy link
Member Author

Choose a reason for hiding this comment

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

By the Yellow Paper and the names we used so far the intrinsic gas includes the base cost of 21000. The EIP extracts the base cost outside of the max function: 21000 + max(intrinsic_without_base, floor_gas).
Because also the EIP don't name any of these values, I decided to include the base cost in the min_gas_cost. It is easier to use.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see

chfast added 3 commits January 17, 2025 15:10
@chfast
Implement EIP-7623 "Increase calldata cost"
d9d21bc 
This implements [EIP-7623](https://eips.ethereum.org/EIPS/eip-7623).
In effect, during transaction validation we compute minimal transaction
gas cost. After transaction execution the amount of gas used is
increased to this value if lower.
@chfast
statetests: Skip tests with EIP-7702 transactions
57911b5 
Skip state tests having EIP-7702 (type 4) transactions. These are not
supported yet what prevents clearly testing other new features.
@chfast
ci: Adjust EEST tests for pectra devnet 4/5
9899329
@chfast chfast force-pushed the eip7623_calldata_cost branch from 5c58a1b to 9899329 Compare January 17, 2025 14:10
@chfast chfast merged commit 7e658c9 into master Jan 20, 2025
24 of 25 checks passed
@chfast chfast deleted the eip7623_calldata_cost branch January 20, 2025 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gumb0 gumb0 gumb0 left review comments

@rodiazet rodiazet rodiazet approved these changes

@pdobacz pdobacz pdobacz approved these changes

Assignees

@chfast chfast

Labels
Prague Changes for Prague upgrade
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@chfast @gumb0 @pdobacz @rodiazet