feat: vm.JumpTable override
#30
Conversation
ARR4N
requested review from
a team,
darioush,
ceyonur and
michaelkaplan13
and removed request for
a team
|
|
||
| package vm | ||
|
|
||
| // An OperationBuilder is a factory for a new operations to include in a |
There was a problem hiding this comment.
| // 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 |
| func LookupInstructionSet(rules params.Rules) (jt JumpTable, err error) { | ||
| defer func() { | ||
| if err == nil { // NOTE `err ==` NOT != | ||
| jt = *overrideJumpTable(rules, &jt) | ||
| } | ||
| }() |
There was a problem hiding this comment.
Nice defer to lower diffs with base repo 😉 !
| func overrideJumpTable(r params.Rules, jt *JumpTable) *JumpTable { | ||
| if libevmHooks == nil { | ||
| return jt | ||
| } |
There was a problem hiding this comment.
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?
| s.overridden = true | ||
| for op, instr := range s.replacement { | ||
| if instr != nil { | ||
| fmt.Println(op, instr) |
There was a problem hiding this comment.
Remove print I think?
| fmt.Println(op, instr) |
| 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) | ||
| } |
There was a problem hiding this comment.
This test is to make sure we update the Build method to set all fields in the future right?
|
|
||
| _, 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") |
There was a problem hiding this comment.
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
| assert.Equal(t, gasLimit-gasCost, gasRemaining, "gas remaining") | |
| assert.Equal(t, gasLimit-gasCost, gasRemaining) |
| _, 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") | ||
| assert.Equal(t, spy.stateVal, value, "StateDB propagated") |
There was a problem hiding this comment.
Shouldn't this be
| assert.Equal(t, spy.stateVal, value, "StateDB propagated") | |
| assert.Equal(t, spy.stateVal, value, "StateDB did not propagate") |
| opcode = 1 | ||
| gasLimit uint64 = 1e6 | ||
| ) | ||
| rng := ethtest.NewPseudoRand(142857) |
There was a problem hiding this comment.
Any particular reason to use 142857? If not, maybe just use 0? 🤔
Why this should be merged
Allows for creation of unexported
vm.operations and their injection intovm.JumpTables through a registered hook.First step to full support of #22.
How this works
Introduces
vm.Hooksthat can be registered viavm.RegisterHooks(). These are far simpler thanparamshooks as they don't need payloads. Thevm.Hooks.OverrideJumpTable()hook is used in bothNewEVMInterpreter()andLookupInstructionSet()to modify their respectiveJumpTables after creation. This couldn't be defined onparams.RulesHooksdue to a circular dependency.The
vm.OperationBuilderfactory is also introduced to allow creation of the otherwise unexportedoperationtype. I chose this pattern over a function because it makes arguments clearer at the usage site. To provide access to internal identifiers, the customOperationFuncis an extension of the regularexecutionFuncto also accept anOperationEnvironment(similar to aPrecompileEnvironment).How this was tested
Integration tests that demonstrate (a) honouring of the params hook signal; and (b) proper execution of a newly created
*operation.