-
Notifications
You must be signed in to change notification settings - Fork 221
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
[wip] decouple precompiles from core/vm #1320
base: master
Are you sure you want to change the base?
Conversation
core/vm/evm.go
Outdated
Rules(blockNum *big.Int, timestamp uint64) params.Rules | ||
} | ||
|
||
var NewEVM = newEVM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can replace use of the global variable with this pattern:
Convert functions to methods on the struct and reinstate the top-level function Foo() as an alias to DefaultConfig().Foo() (slog.Info() example).
as suggested by @ARR4N if others agree with the overall approach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense. IMHO globals are really annoying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow. The global is introduced so that core
can substitute it with a different implementation. The slog
pattern would be more like:
type EVMConfig struct {
//...
}
func (c *EVMConfig) New(...) *EVM {
// ... contents of original constructor, modified to use the `EVMConfig`
}
func DefaultEVMConfig() *EVMConfig {
//...
}
func NewEVM(...) *EVM {
// what used to be here is now in `EVMConfig.New()`
return DefaultEVMConfig().New()
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to switch to this pattern. We still somehow need a factory since the arguments provided are used to build the config object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requiring a factory is ok; what I'm saying is that it shouldn't be a global variable to allow it to be arbitrarily substituted. The slog.Info()
pattern unfortunately has a red herring in that you can swap out the default logger; this is a better example of what I intended.
// Ensure that this package will panic during init if there is a conflict present with the declared | ||
// precompile addresses. | ||
for _, module := range modules.RegisteredModules() { | ||
address := module.Address | ||
if _, ok := PrecompileAllNativeAddresses[address]; ok { | ||
panic(fmt.Errorf("precompile address collides with existing native address: %s", address)) | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this can be added as a UT instead
// precompile addresses. | ||
for _, module := range modules.RegisteredModules() { | ||
address := module.Address | ||
if _, ok := PrecompileAllNativeAddresses[address]; ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also probably don't need this PrecompileAllNativeAddresses
at all after this.
core/vm/evm.go
Outdated
Rules(blockNum *big.Int, timestamp uint64) params.Rules | ||
} | ||
|
||
var NewEVM = newEVM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense. IMHO globals are really annoying.
core/vm/evm.go
Outdated
@@ -186,9 +171,19 @@ type EVM struct { | |||
callGasTemp uint64 | |||
} | |||
|
|||
type ChainConfig interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this being used instead of an actual params.ChainConfig
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO to decouple the geth code from our code, a first important step is to avoid passing avalanche configurations to core/vm
.
To ensure this, .../core/vm
should not depend on the modified chain config type (github.com/ava-labs/subnet-evm/params.ChainConfig
).
(Further it shouldn't import any of the avalanche specific packages, like specific precompiles).
Later we can use the libevm/params.ChainConfig
or go-ethereum/params.ChainConfig
type here, but that would require the github.com/ava-labs/subnet-evm/params.ChainConfig
to be able to convert itself to one of these types, and I would like to do that refactor as a separate concern.
core/evm_ext.go
Outdated
) | ||
|
||
func init() { | ||
vm.NewEVM = NewEVM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and the above capture of the original of vm.NewEVM
strike me as a code smell. The non-linearity of the logic flow and the "action at a distance" (substituting a global in another package) are anti-patterns that lead to spaghetti code. It will also void all documentation if the implementation can be subbed out in ad hoc ways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can go back to a style that names the new function where it's used.
But this is problematic since we cannot control all the callsites, especially new ones that may materialize in upstream, which I also noticed in rebasing this PR. See this commit for the original style: b1bfabb
We already configure the VM by importing packages and altering global state, such as the precompile module registry.
I agree that substituting a global is up on the list of textbook anti-patterns, but I think it is preferable to callsite modifications. If we have a few such cases, we can document them explicitly.
What is your suggested alternative?
type EVM struct { | ||
*vm.EVM | ||
|
||
chainConfig *params.ChainConfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vm.EVM
already carries a params.ChainConfig
; why is this one needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is needed to return it as a precompileconfig.ChainConfig
, to pass as AccessibleState
to the precompiles.
This type implements the CustomPrecompiles function which passes it to the (decoupled) EVM's vm.Config
The purpose of this type is to keep our modifications of the EVM out of geth code, so we wrap all avalanche specific inputs and configurations and the geth code will try to call a generic callback to call precompiles instead (changing from contract.StatefulPrecompiledContract
to RunFn
in core/vm
).
chainConfig vm.ChainConfig, config vm.Config, | ||
) *vm.EVM { | ||
customChainConfig, ok := chainConfig.(*params.ChainConfig) | ||
if !ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what circumstances would someone pass another type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this PR it is not. But in the future it may be possible that geth / libevm code could use that, until the code is fully separated and there is no "bad imports"
if !ok { | ||
// If the chainConfig is not a params.ChainConfig, then we can't use the custom | ||
// EVM implementation, so we fall back to the default implementation. | ||
log.Warn("ChainConfig is not a *params.ChainConfig, falling back to default EVM") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does passing a non-geth type result in using the native geth implementation and vice versa? That seems back to front.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry if it was unclear, at the moment I do not consider github.com/ava-labs/subnet-evm/params
to be a geth package that we just added a few fields to (it imports precompiles, and completely has a different serialization. as seen in this PR, precompile behavior depends on it).
When we are able to properly instantiate geth without these avalanche modifications with its own configuration, many of the merge conflicts (especially in tests) and nuanced test modifications will begin to disappear (we can replace any critical behavior checks with tests that instantiate an avalanche VM)
// (c) 2024, Ava Labs, Inc. All rights reserved. | ||
// See the file LICENSE for licensing terms. | ||
|
||
package core |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this live in core
instead of vm
? Being here blurs any existing separation of concerns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It cannot be in core/vm
since the purpose is moving to a direction where avalanche packages are not imported in geth code.
return evm.StateDB | ||
} | ||
|
||
func (evm *EVM) GetChainConfig() precompileconfig.ChainConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this being returned as a precompileconfig.ChainConfig
?
My immediate thought, before inspecting the interface definition, was that it seems to be a non sequitur. We're in the core
package, accepting something akin to a params.ChainConfig
, and then it's converted into something to do with precompiles.
Upon inspection I see that it only has IsDurango()
; as a rule of thumb, return types should be as broadly useful as possible and accepted interfaces should require the minimum-viable signature to function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is already being returned as precompileconfig.ChainConfig
but from core/vm
, which seems worse.
The longer term goal of this refactoring direction is to separate avalanche specifics out of the params package.
it only has IsDurango();
Probably you looked at coreth, subnet-evm also includes GetFeeConfig
and AllowedFeeRecipients
.
(This is why we should probably add all this back to coreth...)
Some precompiles already access parameters from subnet-evm/params.ChainConfig
.
It is a separate discussion about how the precompiles should be structured, however at minimum it should be transparent to the core/vm
package, and later to the core
package as well, as you note it is not ideal.
@@ -114,7 +114,7 @@ func enable1344(jt *JumpTable) { | |||
|
|||
// opChainID implements CHAINID opcode | |||
func opChainID(pc *uint64, interpreter *EVMInterpreter, scope *ScopeContext) ([]byte, error) { | |||
chainId, _ := uint256.FromBig(interpreter.evm.chainConfig.ChainID) | |||
chainId, _ := uint256.FromBig(interpreter.evm.chainRules.ChainID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the change? Upstream uses chainConfig
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this PR makes chainConfig an interface and we cannot reference a field on that. Open to other suggestions
core/vm/evm.go
Outdated
Rules(blockNum *big.Int, timestamp uint64) params.Rules | ||
} | ||
|
||
var NewEVM = newEVM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow. The global is introduced so that core
can substitute it with a different implementation. The slog
pattern would be more like:
type EVMConfig struct {
//...
}
func (c *EVMConfig) New(...) *EVM {
// ... contents of original constructor, modified to use the `EVMConfig`
}
func DefaultEVMConfig() *EVMConfig {
//...
}
func NewEVM(...) *EVM {
// what used to be here is now in `EVMConfig.New()`
return DefaultEVMConfig().New()
}
@@ -305,7 +285,7 @@ func (evm *EVM) Call(caller ContractRef, addr common.Address, input []byte, gas | |||
} | |||
|
|||
if isPrecompile { | |||
ret, gas, err = RunStatefulPrecompiledContract(p, evm, caller.Address(), addr, input, gas, evm.interpreter.readOnly) | |||
ret, gas, err = p(caller.Address(), input, gas, evm.interpreter.readOnly) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This moves further from upstream, which uses RunPrecompiledContract()
to wrap tracing and gas metering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are also not using RunPrecompiledContract
, so I don't think it changes how far this is from upstream.
Also my long term plan is to propose the CustomPrecompiles to upstream as a configuration option, but it may not be accepted, so we see.
But open to suggestions.
"github.com/ava-labs/subnet-evm/precompile/contract" | ||
"github.com/ava-labs/subnet-evm/precompile/contracts/deployerallowlist" | ||
"github.com/ava-labs/subnet-evm/precompile/modules" | ||
"github.com/ava-labs/subnet-evm/precompile/precompileconfig" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ These are the main goal of this PR
_ contract.AccessibleState = &EVM{} | ||
_ contract.BlockContext = &BlockContext{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ The new EVM type in core
is there to implement these interfaces and wrap them as CustomPrecompiles
, in addition to specifying the avalanche specific behavioral modifications.
picking up from #1094
(rebased to master)
Pursuing the goal of the code in
core/vm
only depends on the upstreamparam
package,precompileconfig.ChainConfig
(https://github.com/ava-labs/subnet-evm/blob/master/core/vm/evm.go#L631-L632)This PR doesn't solve all the issues with the param package, like the one with the accessListGas also depending on which precompiles is active from the avalanche specific rules. But I consider it in the right direction.
Also in the future we can move this NewEVM to its own module, which may be helpful if we want to parameterize
core
more like this.