Skip to content

Instrument the SSTORE opcode #484

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
Merged
merged 10 commits into from
Jul 11, 2025
Merged

Instrument the SSTORE opcode #484

merged 10 commits into from
Jul 11, 2025

Conversation

MishkaRogachev
Copy link
Contributor

@MishkaRogachev MishkaRogachev commented Jul 4, 2025 •
edited
Loading

For NIT-3484

Done:

  • Modify gasFunc api to return multi-dimensional gas
  • Add test scenario for SSTORE for multi-dimensional gas calculation
  • replace NewEmptyMultiGas with ZeroGas after Add more methods to MultiGas struct #483 merged
  • Instrument modern makeGasSStoreFunc
  • Instrument gasSStore4762
  • Instrument legacy gasSStore
  • Instrument gasSStoreEIP2200

Сheat sheet when which methods are used

Function Activation Condition EVM Phase Related EIPs
gasSStore Before Istanbul (!IsIstanbul) Legacy (pre-EIP-1283) —
gasSStoreEIP2200 Istanbul hard fork (IsIstanbul == true) EIP-1283 + reentrancy fix EIP-1283, EIP-2200
makeGasSStoreFunc Berlin hard fork (IsBerlin == true) Access list / modern EIP-2200, EIP-2929
gasSStore4762 IsEIP4762 == true Verkle/stateless mode EIP-4762

@MishkaRogachev MishkaRogachev requested review from eljobe and gligneul July 4, 2025 14:41
@MishkaRogachev MishkaRogachev force-pushed the adapt-gas-func-use-multi-gas branch 3 times, most recently from a198204 to d04692d Compare July 7, 2025 18:12
@MishkaRogachev MishkaRogachev force-pushed the adapt-gas-func-use-multi-gas branch from d04692d to 791eefd Compare July 7, 2025 18:49
@MishkaRogachev MishkaRogachev changed the title Use MultiGas struct for gas calculations Instrument the SSTORE opcode Jul 7, 2025
@MishkaRogachev MishkaRogachev force-pushed the adapt-gas-func-use-multi-gas branch from 791eefd to 418c0fe Compare July 7, 2025 19:07
@MishkaRogachev MishkaRogachev marked this pull request as ready for review July 8, 2025 09:15
@MishkaRogachev MishkaRogachev force-pushed the adapt-gas-func-use-multi-gas branch from e9933f4 to d25c432 Compare July 8, 2025 09:24
@cla-bot cla-bot
Copy link

cla-bot bot commented Jul 8, 2025

We require contributors to sign our Contributor License Agreement. In order for us to review and merge your code, please sign the linked documents below to get yourself added. https://na3.docusign.net/Member/PowerFormSigning.aspx?PowerFormId=b15c81cc-b5ea-42a6-9107-3992526f2898&env=na3&acct=6e152afc-6284-44af-a4c1-d8ef291db402&v=2

eljobe
eljobe requested changes Jul 8, 2025
View reviewed changes
@eljobe eljobe assigned MishkaRogachev and unassigned eljobe Jul 8, 2025
@hkalodner
Copy link
Contributor

@cla-bot check

@cla-bot cla-bot bot added the s CLA signed label Jul 8, 2025
@cla-bot cla-bot
Copy link

cla-bot bot commented Jul 8, 2025

The cla-bot has been summoned, and re-checked this pull request!

eljobe
eljobe reviewed Jul 8, 2025
View reviewed changes
@eljobe eljobe assigned MishkaRogachev and unassigned eljobe Jul 8, 2025
@MishkaRogachev MishkaRogachev force-pushed the adapt-gas-func-use-multi-gas branch from c7dba1c to 1d9a02b Compare July 8, 2025 13:14
gligneul
gligneul reviewed Jul 8, 2025
View reviewed changes
eljobe
eljobe requested changes Jul 9, 2025
View reviewed changes
core/vm/gas_table.go Outdated
return params.SstoreClearGas, nil
multiGas := multigas.StorageAccessGas(params.SstoreClearGas)
singleGas, overflow := multiGas.SingleGas()
if overflow {
Copy link
Member

Choose a reason for hiding this comment

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

I thought we decided NOT to check the overflow when calculating singleGas from multiGas. No?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@eljobe eljobe assigned MishkaRogachev and unassigned eljobe Jul 9, 2025
@MishkaRogachev MishkaRogachev force-pushed the adapt-gas-func-use-multi-gas branch from 32dcaff to d055606 Compare July 10, 2025 12:22
gligneul
gligneul previously approved these changes Jul 10, 2025
View reviewed changes
Copy link
Contributor

@gligneul gligneul left a comment

Choose a reason for hiding this comment

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

LGTM, just a minor comment.

@MishkaRogachev MishkaRogachev force-pushed the adapt-gas-func-use-multi-gas branch from d055606 to 7b73188 Compare July 10, 2025 14:25
@MishkaRogachev MishkaRogachev requested a review from eljobe July 10, 2025 14:32
eljobe
eljobe previously approved these changes Jul 11, 2025
View reviewed changes
Copy link
Member

@eljobe eljobe left a comment

Choose a reason for hiding this comment

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

I'm approving this, but it there is one more tiny nit of a comment. Please address that, and then we can merge it.

@eljobe eljobe assigned MishkaRogachev and unassigned eljobe Jul 11, 2025
@eljobe eljobe merged commit f3f9e27 into master Jul 11, 2025
14 checks passed
@eljobe eljobe deleted the adapt-gas-func-use-multi-gas branch July 11, 2025 09:59
eljobe added a commit that referenced this pull request Jul 18, 2025
@eljobe
Revert "Merge pull request #484 from OffchainLabs/adapt-gas-func-use-…
671d12f 
…multi-gas"

This reverts commit f3f9e27, reversing
changes made to db018f8.
@eljobe eljobe mentioned this pull request Jul 18, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eljobe eljobe eljobe approved these changes

@gligneul gligneul gligneul left review comments

Assignees

@eljobe eljobe

Labels

design-approved s CLA signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@MishkaRogachev @hkalodner @eljobe @gligneul