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

blockchain/vm: sstore gas cost change (eip2200) #996

Merged
merged 3 commits into from
Jul 20, 2021

Conversation

yoomee1313
Copy link
Contributor

@yoomee1313 yoomee1313 commented Jun 24, 2021

Proposed changes

  • This PR implements eip2200 istanbul hardfork item. It is a part of istanbul compatible change. The hardfork number is nil, so it is not activated yet.
  • I referenced eip2200 implementation and ethereum/go-ethereum@dcc7fc6. Only SubRefund function is from eip1283 implementation.
  • This PR is written above blockchain/state: utilize dirtyStorage more accurately #993. (for now, the PR is not merged, so it contains whole code including PR 993 part)
  • Main idea: sstore gas cost change
    • sstore has dynamic gas cost function (sometimes it is called gas metering function. Ethereum tried to upgrade sstore gas cost function at constantinople, but it was not successful. However, eip2200 did it successfully, and it is included in istanbul.
    • before this PR, sstore gas cost function works simple, and it wasn't accurate. By this PR, original value(the value of the state at beginning of the transaction execution) is utilized for accurate gas metering.

Types of changes

Please put an x in the boxes related to your change.

  • Bugfix
  • New feature or enhancement
  • Others

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I have read the CONTRIBUTING GUIDELINES doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes ($ make test)
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream modules

Related issues

  • Please leave the issue numbers or links related to this PR here.

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

jeongkyun-oh
jeongkyun-oh previously approved these changes Jul 14, 2021
@jeongkyun-oh
Copy link
Contributor

jeongkyun-oh commented Jul 14, 2021

@yoomee1313 If you reference the associated Ethereum PR, it would be nicer.

hyochan-brown
hyochan-brown previously approved these changes Jul 14, 2021
Copy link
Member

@aidan-kwon aidan-kwon left a comment

Choose a reason for hiding this comment

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

@yoomee1313 Geth added one more PR on this change ethereum/go-ethereum#21605 .
Will you apply it later or it is not needed for the next version?

@yoomee1313
Copy link
Contributor Author

yoomee1313 commented Jul 16, 2021

@aidan-kwon oh.. thanks for letting me know about 21605 PR. It is not needed for the next version.. However, in case of bringing berlin hardfork in future, i'll apply it.

blockchain/vm/eips.go Show resolved Hide resolved
blockchain/vm/interpreter.go Show resolved Hide resolved
@yoomee1313 yoomee1313 merged commit 040a923 into klaytn:dev Jul 20, 2021
@yoomee1313 yoomee1313 deleted the eip2200 branch July 20, 2021 06:18
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

5 participants