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: vm.JumpTable override #30

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
83002ea
feat: draft `vm.JumpTable` override
ARR4N Sep 13, 2024
7105d78
Merge branch 'libevm' into arr4n/jumptable-override
ARR4N Sep 17, 2024
482edd9
doc: comments on all exported identifiers
ARR4N Sep 17, 2024
ce10b13
test: improved strategy and coverage
ARR4N Sep 17, 2024
55ef2a4
chore: `gci`
ARR4N Sep 17, 2024
6a5de75
refactor: replace `NewOperation` with `OperationBuilder`
ARR4N Sep 18, 2024
7d054b9
refactor: allow constant and dynamic gas simultaneously
ARR4N Sep 18, 2024
58ded5b
feat: `vm.OperationEnvironment` for custom instructions
ARR4N Sep 18, 2024
a0d8ef9
refactor: move hooks logic to its own file
ARR4N Sep 18, 2024
263a9cb
chore: include changes forgotten in last commit
ARR4N Sep 18, 2024
792eb29
feat: `OperationEnvironment` read-only state access
ARR4N Sep 18, 2024
0473ed8
chore: remove unnecessary `RulesHooks.OverrideJumpTable() bool` #yagni
ARR4N Sep 19, 2024
59cf9d7
doc: update due to removals in last commit
ARR4N Sep 19, 2024
9d1bd9b
Merge branch 'libevm' into arr4n/jumptable-override
ARR4N Sep 19, 2024
7e62c28
refactor: combine `vm.{Precompile,Operation}Environment`
ARR4N Sep 18, 2024
966d148
chore: fix remnant references to `PrecompileEnvironment`
ARR4N Sep 18, 2024
524167e
refactor: `*environment.readOnly bool` instead of `func() bool`
ARR4N Sep 18, 2024
a312417
Merge branch 'libevm' into arr4n/jumptable-override
ARR4N Sep 25, 2024
d80b492
chore: copyright headers
ARR4N Sep 25, 2024
f122da5
Merge branch 'libevm' into arr4n/jumptable-override
ARR4N Sep 30, 2024
e2e9330
Merge branch 'libevm' into arr4n/jumptable-override
ARR4N Oct 7, 2024
789efec
Merge branch 'main' into arr4n/jumptable-override
ARR4N Oct 20, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 2 additions & 17 deletions core/vm/contracts.libevm.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,10 @@ package vm

import (
"fmt"
"math/big"

"github.com/holiman/uint256"

"github.com/ava-labs/libevm/common"
"github.com/ava-labs/libevm/core/types"
"github.com/ava-labs/libevm/libevm"
"github.com/ava-labs/libevm/params"
)

// evmCallArgs mirrors the parameters of the [EVM] methods Call(), CallCode(),
Expand Down Expand Up @@ -124,20 +120,9 @@ func (p statefulPrecompile) Run([]byte) ([]byte, error) {
// precompiled contract is being run; and (b) a means of calling other
// contracts.
type PrecompileEnvironment interface {
ChainConfig() *params.ChainConfig
Rules() params.Rules
ReadOnly() bool
// StateDB will be non-nil i.f.f !ReadOnly().
StateDB() StateDB
// ReadOnlyState will always be non-nil.
ReadOnlyState() libevm.StateReader
Addresses() *libevm.AddressContext
IncomingCallType() CallType

BlockHeader() (types.Header, error)
BlockNumber() *big.Int
BlockTime() uint64
Environment

IncomingCallType() CallType
// Call is equivalent to [EVM.Call] except that the `caller` argument is
// removed and automatically determined according to the type of call that
// invoked the precompile.
Expand Down
2 changes: 1 addition & 1 deletion core/vm/contracts.libevm_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ func TestNewStatefulPrecompile(t *testing.T) {

run := func(env vm.PrecompileEnvironment, input []byte, suppliedGas uint64) ([]byte, uint64, error) {
if got, want := env.StateDB() != nil, !env.ReadOnly(); got != want {
return nil, 0, fmt.Errorf("PrecompileEnvironment().StateDB() must be non-nil i.f.f. not read-only; got non-nil? %t; want %t", got, want)
return nil, 0, fmt.Errorf("Environment().StateDB() must be non-nil i.f.f. not read-only; got non-nil? %t; want %t", got, want)
}
hdr, err := env.BlockHeader()
if err != nil {
Expand Down
17 changes: 17 additions & 0 deletions core/vm/environment.libevm.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,23 @@ import (
"github.com/ava-labs/libevm/params"
)

// An Environment provides information about the context in which an instruction
// is being executed.
type Environment interface {
ChainConfig() *params.ChainConfig
Rules() params.Rules
ReadOnly() bool
// StateDB will be non-nil i.f.f !ReadOnly().
StateDB() StateDB
// ReadOnlyState will always be non-nil.
ReadOnlyState() libevm.StateReader
Addresses() *libevm.AddressContext

BlockHeader() (types.Header, error)
BlockNumber() *big.Int
BlockTime() uint64
}

var _ PrecompileEnvironment = (*environment)(nil)

type environment struct {
Expand Down
2 changes: 2 additions & 0 deletions core/vm/evm.libevm_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ func (o *evmArgOverrider) OverrideNewEVMArgs(args *NewEVMArgs) *NewEVMArgs {
return args
}

func (evmArgOverrider) OverrideJumpTable(_ params.Rules, jt *JumpTable) *JumpTable { return jt }

func (o *evmArgOverrider) OverrideEVMResetArgs(r params.Rules, _ *EVMResetArgs) *EVMResetArgs {
o.gotResetChainID = r.ChainID
return &EVMResetArgs{
Expand Down
8 changes: 8 additions & 0 deletions core/vm/hooks.libevm.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ var libevmHooks Hooks
type Hooks interface {
OverrideNewEVMArgs(*NewEVMArgs) *NewEVMArgs
OverrideEVMResetArgs(params.Rules, *EVMResetArgs) *EVMResetArgs
OverrideJumpTable(params.Rules, *JumpTable) *JumpTable
}

// NewEVMArgs are the arguments received by [NewEVM], available for override
Expand Down Expand Up @@ -74,3 +75,10 @@ func (evm *EVM) overrideEVMResetArgs(txCtx TxContext, statedb StateDB) (TxContex
args := libevmHooks.OverrideEVMResetArgs(evm.chainRules, &EVMResetArgs{txCtx, statedb})
return args.TxContext, args.StateDB
}

func overrideJumpTable(r params.Rules, jt *JumpTable) *JumpTable {
if libevmHooks == nil {
return jt
}
Comment on lines +79 to +82
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm thinking, if a user calls overrideJumpTable, maybe we should panic if the libevmHooks is nil? It sounds a bit strange to pass in rules which just gets silently discarded by this function if the hooks are nil 🤔 We could handle the libevmHooks nil check at the calling layer instead?

return libevmHooks.OverrideJumpTable(r, jt)
}
1 change: 1 addition & 0 deletions core/vm/interpreter.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ func NewEVMInterpreter(evm *EVM) *EVMInterpreter {
}
}
evm.Config.ExtraEips = extraEips
table = overrideJumpTable(evm.chainRules, table)
return &EVMInterpreter{evm: evm, table: table}
}

Expand Down
60 changes: 60 additions & 0 deletions core/vm/jump_table.libevm.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
// Copyright 2024 the libevm authors.
//
// The libevm additions to go-ethereum are free software: you can redistribute
// them and/or modify them under the terms of the GNU Lesser General Public License
// as published by the Free Software Foundation, either version 3 of the License,
// or (at your option) any later version.
//
// The libevm additions are distributed in the hope that they will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU Lesser
// General Public License for more details.
//
// You should have received a copy of the GNU Lesser General Public License
// along with the go-ethereum library. If not, see
// <http://www.gnu.org/licenses/>.

package vm

// An OperationBuilder is a factory for a new operations to include in a
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// An OperationBuilder is a factory for a new operations to include in a
// An OperationBuilder is a factory for new operations to include in a

// [JumpTable].
type OperationBuilder struct {
Execute OperationFunc
ConstantGas uint64
DynamicGas func(_ *EVM, _ *Contract, _ *Stack, _ *Memory, requestedMemorySize uint64) (uint64, error)
MinStack, MaxStack int
MemorySize func(s *Stack) (size uint64, overflow bool)
}

// Build constructs the operation.
func (b OperationBuilder) Build() *operation {
o := &operation{
execute: b.Execute.internal(),
constantGas: b.ConstantGas,
dynamicGas: b.DynamicGas,
minStack: b.MinStack,
maxStack: b.MaxStack,
memorySize: b.MemorySize,
}
return o
}

// An OperationFunc is the execution function of a custom instruction.
type OperationFunc func(_ Environment, pc *uint64, _ *EVMInterpreter, _ *ScopeContext) ([]byte, error)

// internal converts an exported [OperationFunc] into an un-exported
// [executionFunc] as required to build an [operation].
func (fn OperationFunc) internal() executionFunc {
return func(pc *uint64, interpreter *EVMInterpreter, scope *ScopeContext) ([]byte, error) {
env := &environment{
evm: interpreter.evm,
self: scope.Contract,
// The CallType isn't exposed by an instruction's [Environment] and,
// although [UnknownCallType] is the default value, it's explicitly
// set to avoid future accidental setting without proper
// justification.
callType: UnknownCallType,
}
return fn(env, pc, interpreter, scope)
}
}
110 changes: 110 additions & 0 deletions core/vm/jump_table.libevm_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
// Copyright 2024 the libevm authors.
//
// The libevm additions to go-ethereum are free software: you can redistribute
// them and/or modify them under the terms of the GNU Lesser General Public License
// as published by the Free Software Foundation, either version 3 of the License,
// or (at your option) any later version.
//
// The libevm additions are distributed in the hope that they will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU Lesser
// General Public License for more details.
//
// You should have received a copy of the GNU Lesser General Public License
// along with the go-ethereum library. If not, see
// <http://www.gnu.org/licenses/>.

package vm_test

import (
"fmt"
"reflect"
"testing"

"github.com/holiman/uint256"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/ava-labs/libevm/common"
"github.com/ava-labs/libevm/core/vm"
"github.com/ava-labs/libevm/libevm/ethtest"
"github.com/ava-labs/libevm/params"
)

type vmHooksStub struct {
replacement *vm.JumpTable
overridden bool
}

var _ vm.Hooks = (*vmHooksStub)(nil)

// OverrideJumpTable overrides all non-nil operations from s.replacement .
func (s *vmHooksStub) OverrideJumpTable(_ params.Rules, jt *vm.JumpTable) *vm.JumpTable {
s.overridden = true
for op, instr := range s.replacement {
if instr != nil {
fmt.Println(op, instr)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove print I think?

Suggested change
fmt.Println(op, instr)

jt[op] = instr
}
}
return jt
}

func (*vmHooksStub) OverrideNewEVMArgs(a *vm.NewEVMArgs) *vm.NewEVMArgs { return a }

func (*vmHooksStub) OverrideEVMResetArgs(r params.Rules, a *vm.EVMResetArgs) *vm.EVMResetArgs {
return a
}

// An opRecorder is an instruction that records its inputs.
type opRecorder struct {
stateVal common.Hash
}

func (op *opRecorder) execute(env vm.Environment, pc *uint64, interpreter *vm.EVMInterpreter, scope *vm.ScopeContext) ([]byte, error) {
op.stateVal = env.StateDB().GetState(scope.Contract.Address(), common.Hash{})
return nil, nil
}

func TestOverrideJumpTable(t *testing.T) {
const (
opcode = 1
gasLimit uint64 = 1e6
)
rng := ethtest.NewPseudoRand(142857)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any particular reason to use 142857? If not, maybe just use 0? 🤔

gasCost := 1 + rng.Uint64n(gasLimit)
spy := &opRecorder{}

vmHooks := &vmHooksStub{
replacement: &vm.JumpTable{
opcode: vm.OperationBuilder{
Execute: spy.execute,
ConstantGas: gasCost,
MemorySize: func(s *vm.Stack) (size uint64, overflow bool) {
return 0, false
},
}.Build(),
},
}
vm.RegisterHooks(vmHooks)

state, evm := ethtest.NewZeroEVM(t)

contract := rng.Address()
state.CreateAccount(contract)
state.SetCode(contract, []byte{opcode})
value := rng.Hash()
state.SetState(contract, common.Hash{}, value)

_, gasRemaining, err := evm.Call(vm.AccountRef(rng.Address()), contract, []byte{}, gasLimit, uint256.NewInt(0))
require.NoError(t, err, "evm.Call([contract with overridden opcode])")
assert.Equal(t, gasLimit-gasCost, gasRemaining, "gas remaining")
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit I don't think there is a need to have an extra message, since assert.Equal would show the log line and it's relatively easy to understand what's going on without the gas remaining message I think

Suggested change
assert.Equal(t, gasLimit-gasCost, gasRemaining, "gas remaining")
assert.Equal(t, gasLimit-gasCost, gasRemaining)

assert.Equal(t, spy.stateVal, value, "StateDB propagated")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be

Suggested change
assert.Equal(t, spy.stateVal, value, "StateDB propagated")
assert.Equal(t, spy.stateVal, value, "StateDB did not propagate")

}

func TestOperationFieldCount(t *testing.T) {
// The libevm OperationBuilder assumes that the 6 struct fields are the only
// ones.
op := vm.OperationBuilder{}.Build()
require.Equalf(t, 6, reflect.TypeOf(*op).NumField(), "number of fields in %T struct", *op)
}
Comment on lines +105 to +110
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test is to make sure we update the Build method to set all fields in the future right?

7 changes: 6 additions & 1 deletion core/vm/jump_table_export.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,12 @@ import (

// LookupInstructionSet returns the instruction set for the fork configured by
// the rules.
func LookupInstructionSet(rules params.Rules) (JumpTable, error) {
func LookupInstructionSet(rules params.Rules) (jt JumpTable, err error) {
defer func() {
if err == nil { // NOTE `err ==` NOT !=
jt = *overrideJumpTable(rules, &jt)
}
}()
Comment on lines +27 to +32
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice defer to lower diffs with base repo 😉 !

switch {
case rules.IsVerkle:
return newCancunInstructionSet(), errors.New("verkle-fork not defined yet")
Expand Down