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

Fix eip2929 implementation #280

Merged
merged 9 commits into from
Jun 4, 2024

Conversation

RomarQ
Copy link
Contributor

@RomarQ RomarQ commented May 8, 2024

Includes a fix related to EIP-2929, where some addresses were incorrectly considered cold and consumed extra gas. I identified this issue when running the tests located at ethtests/GeneralStateTests/Cancun/stEIP1153-transientStorage, which were failing because the gas costs did not match.

@RomarQ
Copy link
Contributor Author

RomarQ commented May 13, 2024

@sorpaas can you have a look at these changes?

I splitted these changes from the #278 to make the review easier.

@koushiro
Copy link
Contributor

koushiro commented Jun 4, 2024

@RomarQ could you split this PR into three PRs?

  • PR1: fix eip1559
  • PR2: fix eip2929 (and update jsontests in CI)
  • PR3: add cancun config

@RomarQ RomarQ changed the title Adds Cancun configuration and fixes issues related to eip1559 and eip2929 Fixes issues related to eip1559 and eip2929 Jun 4, 2024
@RomarQ RomarQ changed the title Fixes issues related to eip1559 and eip2929 Fixe eip2929 implementation Jun 4, 2024
@RomarQ RomarQ changed the title Fixe eip2929 implementation Fix eip2929 implementation Jun 4, 2024
@koushiro
Copy link
Contributor

koushiro commented Jun 4, 2024

Could you add ethtests/GeneralStateTests/Cancun/stEIP1153-transientStorage into jsontests in CI

@RomarQ
Copy link
Contributor Author

RomarQ commented Jun 4, 2024

@RomarQ could you split this PR into three PRs?

  • PR1: fix eip1559
  • PR2: fix eip2929 (and update jsontests in CI)
  • PR3: add cancun config

Done

@RomarQ
Copy link
Contributor Author

RomarQ commented Jun 4, 2024

Could you add ethtests/GeneralStateTests/Cancun/stEIP1153-transientStorage into jsontests in CI

This depends on #288.

We can test it this way:

cargo run --release --verbose -p jsontests -- jsontests/res/ethtests/GeneralStateTests/Cancun/stEIP1153-transientStorage --debug

@RomarQ
Copy link
Contributor Author

RomarQ commented Jun 4, 2024

Once we enable tests for Cancun fork here:

Fork::Berlin => Config::berlin(),

if we run all the tests, some will fail, mainly because there are still a few incompatibilities after berlin (gas related). I only fixed a few, there are more.

@koushiro
Copy link
Contributor

koushiro commented Jun 4, 2024

Once we enable tests for Cancun fork here:

Fork::Berlin => Config::berlin(),

if we run all the tests, some will fail, mainly because there are still a few incompatibilities after berlin (gas related). I only fixed a few, there are more.

We could fix those incompatibilities in future PRs, let's merge this PR first

@koushiro koushiro merged commit a5a79c4 into rust-ethereum:master Jun 4, 2024
3 checks passed

// https://eips.ethereum.org/EIPS/eip-2929
let target_is_cold = handler.is_cold(target, None);
handler.mark_hot(target, None);
Copy link
Member

Choose a reason for hiding this comment

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

I wonder is this move (from interpreter to gasometer), related to the fix?

The reason why it was put in interpreter because the code is supposed to support custom gasometers. Otherwise with this change, any custom meters will not properly mark hot/cold.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, in some circumstances, the addresses were not being considered as warm when they should. If the objective is to have the code as customisable as possible, probably having it in the gasometer is not that bad.

Kolsky added a commit to BarterLab/evm that referenced this pull request Aug 26, 2024
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