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

feat: precompiled contract poc #1

Merged
merged 4 commits into from
Sep 11, 2024

Conversation

dudong2
Copy link

@dudong2 dudong2 commented Aug 23, 2024

this changes are based on crypto-org-chain#12

@dudong2 dudong2 changed the title feat: app level precompile feat: precompiled contract Aug 23, 2024
@dudong2 dudong2 changed the title feat: precompiled contract feat: precompiled contract poc Aug 23, 2024
@dudong2 dudong2 changed the base branch from basechain/v1.10.19 to basechain/abci1.0/delevelop August 31, 2024 05:30
@dudong2 dudong2 marked this pull request as ready for review August 31, 2024 05:31
@dudong2 dudong2 requested a review from jasonsong0 September 2, 2024 02:29
Copy link

@jasonsong0 jasonsong0 left a comment

Choose a reason for hiding this comment

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

  1. It's better to mention the cherry-picked commits for follow-up.
    It seems to be based on the commit

  2. There isn't much information on what is resolved by this fix, but it needs to be checked.

  3. Please check if the code should be changed to vm.DefaultActivePrecompiles

    cfg.State.Prepare(rules, cfg.Origin, cfg.Coinbase, &address, vm.ActivePrecompiles(rules), nil)

    cfg.State.Prepare(rules, cfg.Origin, cfg.Coinbase, nil, vm.ActivePrecompiles(rules), nil)

    statedb.Prepare(rules, cfg.Origin, cfg.Coinbase, &address, vm.ActivePrecompiles(rules), nil)

@dudong2 dudong2 self-assigned this Sep 9, 2024
@dudong2
Copy link
Author

dudong2 commented Sep 9, 2024

  1. It's better to mention the cherry-picked commits for follow-up.
    It seems to be based on the commit

this pr was done with reference to crypto-org-chain#12. i mentioned url in description.

  1. There isn't much information on what is resolved by this fix, but it needs to be checked.

for big2, there is a history of being added and removed.
no additional information exists, but i think it was an fault.

@dudong2
Copy link
Author

dudong2 commented Sep 9, 2024

  1. Please check if the code should be changed to vm.DefaultActivePrecompiles

    cfg.State.Prepare(rules, cfg.Origin, cfg.Coinbase, &address, vm.ActivePrecompiles(rules), nil)

    cfg.State.Prepare(rules, cfg.Origin, cfg.Coinbase, nil, vm.ActivePrecompiles(rules), nil)

    statedb.Prepare(rules, cfg.Origin, cfg.Coinbase, &address, vm.ActivePrecompiles(rules), nil)

these have been changed to vmevm.ActivePrecompiles(rules) and also fixed the part of the test code that was causing the build to fail.

@dudong2 dudong2 added the enhancement New feature or request label Sep 9, 2024
@dudong2 dudong2 changed the base branch from basechain/abci1.0/delevelop to basechain/abci1.0/develop September 9, 2024 09:00
@dongsam dongsam requested a review from cloudgray September 11, 2024 02:31
@jasonsong0
Copy link

LGTM

cloudgray

This comment was marked as duplicate.

@cloudgray cloudgray self-requested a review September 11, 2024 06:17
@cloudgray
Copy link
Collaborator

cloudgray commented Sep 11, 2024

  1. Please check if the code should be changed to vm.DefaultActivePrecompiles

    cfg.State.Prepare(rules, cfg.Origin, cfg.Coinbase, &address, vm.ActivePrecompiles(rules), nil)

    cfg.State.Prepare(rules, cfg.Origin, cfg.Coinbase, nil, vm.ActivePrecompiles(rules), nil)

    statedb.Prepare(rules, cfg.Origin, cfg.Coinbase, &address, vm.ActivePrecompiles(rules), nil)

these have been changed to vmevm.ActivePrecompiles(rules) and also fixed the part of the test code that was causing the build to fail.

I checked fix test build commit, and I think these three parts are not related to build failure.

[Suggestion]
When I change vmenv.ActivePrecompiles to vm.DefaultActivePrecompiles of these three parts and build, it works.
Of course it is extension of vm.DefaultActivePrecompiles and it has not impact to ethermint and basechain.
But it can impact independent working of this go-etherum fork.
It is not required but suggesion that we use vm.DefaultActivePrecompiles to these 3 parts.

@@ -1405,7 +1405,7 @@ func AccessList(ctx context.Context, b Backend, blockNrOrHash rpc.BlockNumberOrH
}
isPostMerge := header.Difficulty.Cmp(common.Big0) == 0
// Retrieve the precompiles since they don't need to be added to the access list
precompiles := vm.ActivePrecompiles(b.ChainConfig().Rules(header.Number, isPostMerge))
precompiles := vm.DefaultActivePrecompiles(b.ChainConfig().Rules(header.Number, isPostMerge))
Copy link
Collaborator

Choose a reason for hiding this comment

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

[Optional] If we use vmenv.ActivePrecompiles instead of vm.DefaultActivePrecompiles in go-ethereum/core/vm/runtime/runtime.go, I think same change should be applied to this part.

Copy link
Author

@dudong2 dudong2 Sep 11, 2024

Choose a reason for hiding this comment

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

I reverted changes in runtime.go.
Execute, Create, Call functions are performed via evm binary, which is the evm executor.
The point at which the precompiled contract address is added to the active precompiles list is when ethermint executes an evm tx, so there is no need to change to ActivePrecompiles in parts that do not have this access route.

Copy link
Collaborator

@cloudgray cloudgray left a comment

Choose a reason for hiding this comment

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

I have some suggestions but entirely, LGTM.

@dudong2 dudong2 merged commit ceb46ab into basechain/abci1.0/develop Sep 11, 2024
@dudong2 dudong2 deleted the feat/precompiled-bank-contract branch September 11, 2024 10:43
dudong2 added a commit that referenced this pull request Sep 25, 2024
dudong2 added a commit that referenced this pull request Nov 4, 2024
* feat: app level precompile

* chore: Fix test build

* chore: Remove usused var big2

* chore: Revert ActivePrecompiles to DefaultActivePrecompilestest: Fix gofumpt, gci
dudong2 added a commit that referenced this pull request Nov 6, 2024
* feat: precompiled contract poc (#1)

* feat: app level precompile

* chore: Fix test build

* chore: Remove usused var big2

* chore: Revert ActivePrecompiles to DefaultActivePrecompilestest: Fix gofumpt, gci

* fix resolve

* Merge pull request ethereum#18 from yihuang/release/v1.11.x

go.mod: bump pebble db to official release

---------

Co-authored-by: mmsqe <[email protected]>
Co-authored-by: yihuang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants