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

EVM + Weight v2 support #1039

Merged

Conversation

tgmichel
Copy link
Contributor

@tgmichel tgmichel commented Apr 13, 2023

Depends on #893 (cherry picking some of the changes here to make it work)
Drafting EVM changes in https://github.com/PureStake/evm/tree/tgm-record-external-cost (pinning this branch here temporarily to make the CI pass)

The problem we are trying to solve

Current parachain block validation design is resource restrictive, and one of this restrictions is proof size: the amount of parachain state data a relay validator needs to proof the state transition for a block is valid.

The evm gasometer only records and has capacity for one metric - gas -, so we need the ability of recording additional metrics - in our case proof_size - so we can exit the evm when the proof size capacity limit has been reached for the current block.

What we should avoid

Modifying the evm gasometer.

The changes we are introducing are not part of the evm design - and will never be. There is plans in Ethereum to adapt the gasometer architecture to support witness data recording, so a solution for a similar problem will eventually be part of the standard specification. Changing the gasometer logic - or abstracting away the Gas for example - will only lead to complex foreign code in the evm that really gives nothing in return.

In our case we have a Substrate-only problem, and the solution should happen completely (or almost) in Substrate.

StackState external cost recorder

This PR proposes adding two methods to the StackState trait:

  • record_external_opcode_cost: which is called from pre_validate in the executor (that is per each evm runtime step).
  • record_external_cost: meant to be called from precompiles, and more specifically, substrate-implemented precompiles, where there is the need of recording a foreign (not opcode based) Weight v2 cost. This method is behing a with-substrate feature gate.

Every time the evm steps into an opcode, if it has a proof_size associated to it - reads or writes from storage - this proof size will be, if the storage is cold, cheaply calculated on the fly and recorded. The recorder will OutOfGas if the metric capacity is reached.

Note: the reasons to not abstract away record_external_cost and use a feature flag are two:

  • The concept of dispatching - and the need of an external cost - from precompiles is purely a substrate one. At least afaik.
  • Adding generics to the PrecompileHandle will cascade into changes across the evm, and thus goes against the main motivation of this proposal: keep changes outside the evm whenevr possible.

Differences between Gas and External cost

Gas is part of the evm design, and has its own target capacity per subcall. This is not the case for external costs: they have one transaction-wide usage and capacity because, again, they are a foreign metric, not part of evm design.

Ref time support

https://forum.polkadot.network/t/frontier-support-for-evm-weight-v2/2470/5 does a great job describing why ref_time recording should also be supported.

The solution proposed in this PR supports it (once we benchmark Opcodes in substrate) and also supports a preliminary version without it, where we still use gas to weight conversion and rely on the native gasometer.


cc
@sorpaas
@librelois
@nanocryk
@crystalin

frame/evm/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@sorpaas sorpaas left a comment

Choose a reason for hiding this comment

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

Design LGTM. Need to fix conflicts.

Also, let's remove the evm-with-weight-limit feature and the evm/with-substrate feature. It's really unnecessary to have two code paths where one will be seldom used.

@tgmichel
Copy link
Contributor Author

Design LGTM. Need to fix conflicts.

Also, let's remove the evm-with-weight-limit feature and the evm/with-substrate feature. It's really unnecessary to have two code paths where one will be seldom used.

Just to confirm, does that mean you suggest we include this functionality for standalone chains using frontier and even non-substrate-based chains using sputnikvm? Including it comes with residual overhead.

tgmichel added 4 commits June 19, 2023 16:56
# Conflicts:
#	frame/evm/src/lib.rs
#	frame/evm/src/runner/stack.rs
frame/evm/src/runner/stack.rs Outdated Show resolved Hide resolved
primitives/evm/src/lib.rs Show resolved Hide resolved
@sorpaas
Copy link
Member

sorpaas commented Jun 21, 2023

@tgmichel

Including it comes with residual overhead.

The weightv2 interface in Substrate explicitly has proof_size as a parameter. So I think this overhead would be justified even for solo chains as they need to account for it as well. We might be able to later reduce the standard gas (because we use ref_time for compute gas, and proof_size for storage gas, where Ethereum always has this combined, thus double calculating the cost).

Plus, the extra overhead can mostly be ignored. We're not adding any loops, just some simple additions and subtractions. I didn't do any benchmarks but I would expect that the result would not really be noticeable.

@tgmichel
Copy link
Contributor Author

Including it comes with residual overhead.

The weightv2 interface in Substrate explicitly has proof_size as a parameter. So I think this overhead would be justified even for solo chains as they need to account for it as well. We might be able to later reduce the standard gas (because we use ref_time for compute gas, and proof_size for storage gas, where Ethereum always has this combined, thus double calculating the cost).

Plus, the extra overhead can mostly be ignored. We're not adding any loops, just some simple additions and subtractions. I didn't do any benchmarks but I would expect that the result would not really be noticeable.

Thank you @sorpaas, sounds reasonable let me work on it

Cargo.toml Outdated Show resolved Hide resolved
@tgmichel
Copy link
Contributor Author

@sorpaas CI is timing out please re-run (verified locally the test pass).

@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/frontier-support-for-evm-weight-v2/2470/6

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.

9 participants