[wip] decouple precompiles from core/vm #1320

darioush · 2024-08-28T23:20:02Z

picking up from #1094
(rebased to master)

Pursuing the goal of the code in core/vm only depends on the upstream param package,

since core/vm knows about the precompiles, it needs to pass in the chainConfig as a precompileconfig.ChainConfig (https://github.com/ava-labs/subnet-evm/blob/master/core/vm/evm.go#L631-L632)
so we might as well decouple the precompiles and reduce the diff with geth.

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.

darioush · 2024-08-29T00:08:34Z

core/vm/evm.go

+	Rules(blockNum *big.Int, timestamp uint64) params.Rules
+}
+
+var NewEVM = newEVM


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

I think it makes sense. IMHO globals are really annoying.

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() }

I tried to switch to this pattern. We still somehow need a factory since the arguments provided are used to build the config object.

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.

ceyonur · 2024-08-29T10:42:25Z

core/vm/contracts.go

-	// 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))
-		}
-	}
 }


I wonder if this can be added as a UT instead

ceyonur · 2024-08-29T10:48:34Z

core/vm/contracts.go

-	// precompile addresses.
-	for _, module := range modules.RegisteredModules() {
-		address := module.Address
-		if _, ok := PrecompileAllNativeAddresses[address]; ok {


We also probably don't need this PrecompileAllNativeAddresses at all after this.

ceyonur · 2024-08-29T12:36:39Z

core/vm/evm.go

+	Rules(blockNum *big.Int, timestamp uint64) params.Rules
+}
+
+var NewEVM = newEVM


I think it makes sense. IMHO globals are really annoying.

ARR4N · 2024-08-29T15:34:22Z

core/vm/evm.go

@@ -186,9 +171,19 @@ type EVM struct {
 	callGasTemp uint64
 }

+type ChainConfig interface {


Why is this being used instead of an actual params.ChainConfig?

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.

ARR4N · 2024-08-29T15:36:31Z

core/evm_ext.go

+)
+
+func init() {
+	vm.NewEVM = NewEVM


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.

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?

ARR4N · 2024-08-29T15:37:15Z

core/evm_ext.go

+type EVM struct {
+	*vm.EVM
+
+	chainConfig *params.ChainConfig


vm.EVM already carries a params.ChainConfig; why is this one needed?

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).

ARR4N · 2024-08-29T15:41:08Z

core/evm_ext.go

+	chainConfig vm.ChainConfig, config vm.Config,
+) *vm.EVM {
+	customChainConfig, ok := chainConfig.(*params.ChainConfig)
+	if !ok {


In what circumstances would someone pass another type?

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"

ARR4N · 2024-08-29T15:41:38Z

core/evm_ext.go

+	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")


Why does passing a non-geth type result in using the native geth implementation and vice versa? That seems back to front.

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)

ARR4N · 2024-08-29T15:43:31Z

core/evm_ext.go

+// (c) 2024, Ava Labs, Inc. All rights reserved.
+// See the file LICENSE for licensing terms.
+
+package core


Why does this live in core instead of vm? Being here blurs any existing separation of concerns.

It cannot be in core/vm since the purpose is moving to a direction where avalanche packages are not imported in geth code.

ARR4N · 2024-08-29T15:49:21Z

core/evm_ext.go

+	return evm.StateDB
+}
+
+func (evm *EVM) GetChainConfig() precompileconfig.ChainConfig {


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.

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.

ARR4N · 2024-08-29T15:50:21Z

core/vm/eips.go

@@ -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)


Why the change? Upstream uses chainConfig.

this PR makes chainConfig an interface and we cannot reference a field on that. Open to other suggestions

ARR4N · 2024-08-29T15:54:49Z

core/vm/evm.go

+	Rules(blockNum *big.Int, timestamp uint64) params.Rules
+}
+
+var NewEVM = newEVM


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() }

ARR4N · 2024-08-29T15:56:12Z

core/vm/evm.go

@@ -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)


This moves further from upstream, which uses RunPrecompiledContract() to wrap tracing and gas metering.

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.

darioush · 2024-08-29T23:21:30Z

core/vm/evm.go

-	"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"


^ These are the main goal of this PR

darioush · 2024-08-29T23:30:56Z

core/vm/evm.go

-	_ contract.AccessibleState = &EVM{}
-	_ contract.BlockContext    = &BlockContext{}


^ 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.

darioush added 4 commits August 28, 2024 16:19

[wip] decouple precompiles from core/vm

b1bfabb

update to core vm

22dd760

restore to vm.NewEVM

32a3895

avoid infite recursion

7684553

darioush commented Aug 29, 2024

View reviewed changes

ceyonur reviewed Aug 29, 2024

View reviewed changes

ARR4N requested changes Aug 29, 2024

View reviewed changes

try a factory approach

34baeec

darioush commented Aug 29, 2024

View reviewed changes

move more to ext

7221490

darioush commented Aug 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wip] decouple precompiles from core/vm #1320

[wip] decouple precompiles from core/vm #1320

darioush commented Aug 28, 2024 •

edited

Loading

darioush Aug 29, 2024 •

edited

Loading

ceyonur Aug 29, 2024

ARR4N Aug 29, 2024

darioush Aug 29, 2024 •

edited

Loading

ARR4N Sep 2, 2024

ceyonur Aug 29, 2024

ceyonur Aug 29, 2024

ceyonur Aug 29, 2024

ARR4N Aug 29, 2024

darioush Aug 29, 2024 •

edited

Loading

ARR4N Aug 29, 2024

darioush Aug 29, 2024 •

edited

Loading

ARR4N Aug 29, 2024

darioush Aug 29, 2024 •

edited

Loading

ARR4N Aug 29, 2024

darioush Aug 29, 2024

ARR4N Aug 29, 2024

darioush Aug 29, 2024 •

edited

Loading

ARR4N Aug 29, 2024

darioush Aug 29, 2024 •

edited

Loading

ARR4N Aug 29, 2024

darioush Aug 29, 2024 •

edited

Loading

ARR4N Aug 29, 2024

darioush Aug 29, 2024 •

edited

Loading

ARR4N Aug 29, 2024

ARR4N Aug 29, 2024

darioush Aug 29, 2024

darioush Aug 29, 2024

darioush Aug 29, 2024 •

edited

Loading

		_ contract.AccessibleState = &EVM{}
		_ contract.BlockContext = &BlockContext{}

[wip] decouple precompiles from core/vm #1320

Are you sure you want to change the base?

[wip] decouple precompiles from core/vm #1320

Conversation

darioush commented Aug 28, 2024 • edited Loading

darioush Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darioush Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darioush Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darioush Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darioush Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darioush Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darioush Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darioush Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darioush Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darioush Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

darioush commented Aug 28, 2024 •

edited

Loading

darioush Aug 29, 2024 •

edited

Loading

darioush Aug 29, 2024 •

edited

Loading

darioush Aug 29, 2024 •

edited

Loading

darioush Aug 29, 2024 •

edited

Loading

darioush Aug 29, 2024 •

edited

Loading

darioush Aug 29, 2024 •

edited

Loading

darioush Aug 29, 2024 •

edited

Loading

darioush Aug 29, 2024 •

edited

Loading

darioush Aug 29, 2024 •

edited

Loading

darioush Aug 29, 2024 •

edited

Loading