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: vmLog/feePayer/validateSender precompiled contract address mapping #912

Merged
merged 23 commits into from
Jun 2, 2021

Conversation

yoomee1313
Copy link
Contributor

@yoomee1313 yoomee1313 commented Mar 4, 2021

Proposed changes

  • vmLog/feePayer/validateSender precompiled contracts' address will be changed after the istanbulCompatible change. You can see how the address changes by looking at the yellow highlights in the table below.
    image

  • However, the vmLog/feePayer/validateSender calling functions that are already in use cannot be used after the change. By this PR, we can still use those functions by utilizing CodeFormat. It is used as an incompatible change version.

  • Originally, when a smart contract is released, CodeFormatEVM is assigned to that contract. By this PR, CodeFormatEVM2 is assigned to the new contracts which will be deployed after istanbulCompatible change.

  • The result below shows the success result of calling precompiled contract according to those conditions: node's incompatible change version (constantinople/istanbul), deployment time, and call time. before means a contract is deployed or called before the IstanbulCompatible change. after means a contract is deployed or called after the IstanbulCompatible change.
    image

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...

@yoomee1313 yoomee1313 self-assigned this Mar 4, 2021
@yoomee1313 yoomee1313 marked this pull request as ready for review March 4, 2021 23:48
blockchain/vm/evm.go Outdated Show resolved Hide resolved
* assign CodeFormatEVM2 to the new contracts which will be deployed after istanbulCompatible change
* keep old precompiled address when calling the contracts deployed before istanbulCompatible change
@aidan-kwon
Copy link
Member

@yoomee1313 I don't understand why Istanbul Address is callable when deployment time is Before and call time is After. I think calling Istanbul Address of the contact may not be intended by the contract author.

blockchain/vm/evm.go Outdated Show resolved Hide resolved
blockchain/vm/evm.go Outdated Show resolved Hide resolved
blockchain/vm/evm.go Outdated Show resolved Hide resolved
blockchain/vm/evm.go Outdated Show resolved Hide resolved
blockchain/vm/evm.go Outdated Show resolved Hide resolved
blockchain/vm/evm.go Outdated Show resolved Hide resolved
@yoomee1313
Copy link
Contributor Author

@aidan-kwon

I don't understand why Istanbul Address is callable when deployment time is Before and call time is After.

I never thought deeply about what I pointed out. Thank you. I checked how geth handled it, and the Istanbul address is callable when the contract is deployed before the istanbul change. Unfortunately, I couldn't find the reason or related documentation about this.

I think calling Istanbul Address of the contact may not be intended by the contract author.

Since this is a new item address, it will not be the case that the author did not intend.

KimKyungup
KimKyungup previously approved these changes Mar 12, 2021
joowon-byun
joowon-byun previously approved these changes May 25, 2021
Copy link
Contributor

@joowon-byun joowon-byun left a comment

Choose a reason for hiding this comment

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

LGTM

ehnuje
ehnuje previously approved these changes May 25, 2021
@yoomee1313 yoomee1313 dismissed stale reviews from ehnuje and joowon-byun via 22e0416 May 26, 2021 10:13
* refactor protocol_params.go
* refactor StateDB.GetCodeInfo to StateDB.GetVmVersion to minimize the explosure of codeInfo
@yoomee1313
Copy link
Contributor Author

@aidan-kwon CodeInfo part is refactored. Please take a look.

aidan-kwon
aidan-kwon previously approved these changes May 28, 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.

LGTM except for minors

yoomee1313 and others added 2 commits May 28, 2021 09:56
Co-authored-by: Donghwan Aidan Kwon <48199072+aidan-kwon@users.noreply.github.com>
@yoomee1313 yoomee1313 merged commit 0ffd9fd into klaytn:dev Jun 2, 2021
@yoomee1313 yoomee1313 deleted the precompiled-addressmapping branch June 2, 2021 00:59
@yoomee1313 yoomee1313 mentioned this pull request Nov 22, 2022
9 tasks
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