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

PrecompileHandle trait (subcall support) + precompile testing utils #1487

Merged
merged 39 commits into from
May 24, 2022

Conversation

nanocryk
Copy link
Contributor

@nanocryk nanocryk commented May 10, 2022

What does it do?

Update to changes suggested in this PR: rust-ethereum/evm#122

Adds support for precompiles to do subcalls throught an handle. This handle also allow to record gas and logs which are no longer part of PrecompileOutput.

Since it changed the signature of PrecompileSet::execute all the tests broke. While fixing them I wrote a util to help write precompile tests and hide the implementation details, which will hopefully allow them to not break when new changes to the EVM are introduced.

What important points reviewers should know?

Currently use custom branches until PRs are merged in their respective repos such as our frontier repo:
moonbeam-foundation/frontier#63

Is there something left for follow-up PRs?

Implementation of batch precompile: #1498
Some precompile performing subcall are needed to properly test tracing. A new tracing event has already been added to properly handle precompile subcalls.

The new design of precompiles moves all parameters inside the handle. A new Precompile trait following this design is added in the utils, and all precompiles implementing pallet_evm::Precompile automatically implement this new trait at the cost of cloning the input. Thus, a following PR could migrate our precompiles to directly implement this new trait to avoid input cloning and use new features.

What alternative implementations were considered?

Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?

What value does it bring to the blockchain users?

@nanocryk nanocryk added A0-pleasereview Pull request needs code review. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. labels May 10, 2022
@nanocryk nanocryk requested a review from tgmichel May 10, 2022 13:20
@nanocryk nanocryk changed the title PrecompileHandle trait + precompile testing utils PrecompileHandle trait (subcall support) + precompile testing utils May 10, 2022
Copy link
Contributor

@tgmichel tgmichel left a comment

Choose a reason for hiding this comment

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

Looks awesome, after reading it a couple of times cannot really find anything fundamentally wrong with it.

To help me (and others) understand the changes, a quick summary:

  • PrecompileHandle trait is introduced to rust-blockchain/evm.

  • This trait is implemented for StackExecutorHandle, which is a defensive private struct which sole purpose is to wrap a mutable reference to the StackExecutor.

  • The purpose of the new handle is to:

    1. enables the WIP subcall functionality. That is, entering in a new nested EVM call on behalf of a precompile-provided, higher-level context caller without the need of delegate call op.
    2. allows to record gasometer cost and logs in-place a.k.a. without the need of returning them as part of the precompile result and perform the gasometer+logs later on. Thus logs can be recorded in the right order. This also removes the need of passing around the gasometer which is great.
  • A new functional precompile testing api that simplifies assertions and mocking.

@nanocryk nanocryk mentioned this pull request May 12, 2022
@nanocryk nanocryk added the A10-evmtracing Pull request includes evm tracing functionality label May 13, 2022
@nanocryk nanocryk requested a review from tgmichel May 19, 2022 08:17
@@ -11,6 +11,5 @@ git clone --depth 1 -b master-without-wasm https://github.com/PureStake/moonbeam

cd build/moonbeam-runtime-overrides
./scripts/import-tracing-runtime.sh local ${1:-"$LOCAL_GIT_BRANCH"}
cd tracing/local && cargo update -p evm && cd ../..
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seemed to be necessary at some point but no longer is, and instead doesn't seem to like the git dependency.

Copy link
Contributor

@notlesh notlesh left a comment

Choose a reason for hiding this comment

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

I read through +6,228 −10,648 lines changed and only had a problem with the very last one 😆

@girazoki
Copy link
Collaborator

Are we gonna include this PR with the modified branch in frontier?

@nanocryk
Copy link
Contributor Author

nanocryk commented May 23, 2022

Are we gonna include this PR with the modified branch in frontier?

We should merge the PRed branch into our usual versionned branch.
I'm migrating both branches to substrate 0.9.20 now.

@nanocryk
Copy link
Contributor Author

Blocked until #1528 is merged (0.9.20 upgrade with frontier bump)

Copy link
Contributor

@tgmichel tgmichel left a comment

Choose a reason for hiding this comment

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

Awesome

@nanocryk nanocryk merged commit 533d7d7 into master May 24, 2022
@nanocryk nanocryk deleted the jeremy-precompile-handle branch May 24, 2022 14:00
@notlesh notlesh added D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A0-pleasereview Pull request needs code review. A10-evmtracing Pull request includes evm tracing functionality B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants