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

refactor: PrecompileEnvironment.{,Use,Refund}Gas() in lieu of args #73

Merged
merged 11 commits into from
Dec 18, 2024

Conversation

ARR4N
Copy link
Collaborator

@ARR4N ARR4N commented Nov 15, 2024

Why this should be merged

Aligns precompiled contracts with the pattern used in vm.EVMInterpreter for regular contracts and simplifies gas accounting by using existing mechanisms. The most notable simplification occurs when there are multiple error paths that return early and have to account for consumed gas (any local solution would likely just mirror this one).

This forms part of a broader, long-term direction of feature parity between precompiled and bytecode contracts, exposed via vm.PrecompileEnvironment.

The vm.Contract.Value() method is also exposed as a natural accompaniment to this change.

How this works

The original signature for vm.PrecompiledStatefulContract received the amount of gas available and then returned the amount remaining; this has been removed in lieu of exposing the existing vm.Contract.UseGas() method via the vm.PrecompileEnvironment. A new legacy package wraps the old signature and converts it to a new one so ava-labs/coreth doesn't need to be refactored.

func oldPrecompile(env vm.PrecompileEnvironment, input []byte, gas uint64) ([]byte, uint64, error) {
  // ...
  if err != nil {
    return nil, gas - gasCost, err // pattern susceptible to bugs; should it be `nil, gas, err` ?
  }
  // ...
  return output, gas - gasCost, nil  
}

func newPrecompile(env vm.PrecompileEnvironment, input []byte) ([]byte, error) {
  // The original `gas` argument is still available as `env.Gas()`
  // ...
  if !env.UseGas(gasCost) { // an explicit point at which gas is consumed
    return nil, vm.ErrOutOfGas
  }
  // ...
  if err != nil {
    return nil, err
  }
  // ...
  return output, nil
}

How this was tested

Existing unit test modified to use the legacy adaptor.

@ARR4N ARR4N marked this pull request as ready for review November 18, 2024 15:00
Copy link
Collaborator

@qdm12 qdm12 left a comment

Choose a reason for hiding this comment

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

Nicely done, just a few nits mostly about libevm new code errors wrapping

This forms part of a broader, long-term direction of feature parity between precompiled and bytecode contracts, exposed via vm.PrecompileEnvironment.

What's the context here, do we require this to sync coreth/subnet-evm with geth? Or is this more of a long term we can do after syncing thing? - not criticizing at all, just trying to better understand it 😉

core/vm/contracts.libevm.go Show resolved Hide resolved
libevm/legacy/legacy.go Outdated Show resolved Hide resolved
core/vm/contracts.libevm.go Outdated Show resolved Hide resolved
core/vm/contracts.libevm.go Outdated Show resolved Hide resolved
core/vm/environment.libevm.go Show resolved Hide resolved
core/vm/environment.libevm.go Show resolved Hide resolved
core/vm/environment.libevm.go Show resolved Hide resolved
core/vm/environment.libevm.go Show resolved Hide resolved
core/vm/environment.libevm.go Show resolved Hide resolved
core/vm/environment.libevm.go Outdated Show resolved Hide resolved
ARR4N and others added 2 commits December 17, 2024 17:00
Co-authored-by: Quentin McGaw <[email protected]>
Signed-off-by: Arran Schlosberg <[email protected]>
Co-authored-by: Quentin McGaw <[email protected]>
Signed-off-by: Arran Schlosberg <[email protected]>
@ARR4N
Copy link
Collaborator Author

ARR4N commented Dec 17, 2024

Nicely done, just a few nits mostly about libevm new code errors wrapping

This forms part of a broader, long-term direction of feature parity between precompiled and bytecode contracts, exposed via vm.PrecompileEnvironment.

What's the context here, do we require this to sync coreth/subnet-evm with geth? Or is this more of a long term we can do after syncing thing? - not criticizing at all, just trying to better understand it 😉

Not a current need at all. I noticed that we were edging closer and closer to parity between stateful precompiles and regular Solidity so wanted to close the gap.

@ARR4N ARR4N merged commit 44f23c8 into main Dec 18, 2024
4 checks passed
@ARR4N ARR4N deleted the arr4n/precompile-env-use-gas branch December 18, 2024 13:54
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.

2 participants