use libevm: params, core/vm, eth/tracers/* + some of core/types #662

darioush · 2024-09-26T00:06:31Z

Why this should be merged

Improved maintainability and reducing copies of upstream packages with modified code. This reduces our tech debt by entirely using the upstream vm module + jump tables.

As we have the libevm branch to test these changes, I would like to defer some of the following cleanups to future PRs:

using "core/vm" instead of "/vmerrs" where possible
removal of IsProhibited
renaming "ava-labs/go-ethereum" to "ava-labs/libevm"

How this works

Uses core/vm, and eth/tracer/* packages from https://github.com/ava-labs/libevm as a module that expands geth code without making destructive modifications instead of using code copied from upstream and modified in this repo.
Instead, the avalanche specific functionality such as precompiles are re-introduced through the hooks provided by libevm

How this was tested

CI, reprocessed historical blocks of fuji/mainnet (probably should run this again with additional switches using upstream fork names)

params/config.go

precompile/contract/interfaces.go

ARR4N

I'd like to see what @ceyonur thinks too, but generally this LGTM. Only a few nits and questions as I see you've already merged my recommendations PR.

ARR4N · 2024-10-17T01:30:05Z

core/blockchain_test.go

-		Alloc:  types.GenesisAlloc{addr1: {Balance: genesisBalance}},
+		Config: params.WithExtra(
+			&params.ChainConfig{HomesteadBlock: new(big.Int)},
+			&params.ChainConfigExtra{},


Would a raw &params.ChainConfig{...} not work? params.GetExtra() guarantees that the extra won't be nil.

yes, this works

ARR4N · 2024-10-17T01:32:41Z

core/evm.go

 	"github.com/ethereum/go-ethereum/log"
 	"github.com/holiman/uint256"
 )

+var _ vm.Hooks = hooks{}


(nit) If you move the init() here then you don't need the type check because it's implied.

ARR4N · 2024-10-17T01:35:23Z

core/evm.go

+}
+
+func (s *StateDbAP1) GetCommittedState(addr common.Address, key common.Hash) common.Hash {
+	state.NormalizeStateKey(&key)


(no action required) So good to see that wrapping with just this one method does the trick for this!

ARR4N · 2024-10-21T02:28:37Z

core/state/statedb.go

@@ -238,13 +233,7 @@ func (s *StateDB) Error() error {
 // AddLog adds a log with the specified parameters to the statedb
 // Note: blockNumber is a required argument because StateDB does not
 // know the current block number.
-func (s *StateDB) AddLog(addr common.Address, topics []common.Hash, data []byte, blockNumber uint64) {


Is the change to StateDB.AddLog() just to move closer to upstream?

It's also needed so coreth/core/state.StateDB implements libevm/core/vm.StateDB
Before this PR importing coreth/core/types in the precompiles is not possible because of a circular dependency with from types -> params -> precompiles.

ARR4N · 2024-10-21T02:44:31Z

core/state_transition.go

@@ -329,6 +332,18 @@ func (st *StateTransition) buyGas() error {
 	return nil
 }

+// TODO: Probably can be removed -- extremely unlikely someone has private keys


Let's remove it in a separate PR next.

ARR4N · 2024-10-21T03:13:03Z

plugin/evm/export_tx.go

@@ -69,8 +69,9 @@ func (utx *UnsignedExportTx) InputUTXOs() set.Set[ids.ID] {
 // Verify this transaction is well-formed
 func (utx *UnsignedExportTx) Verify(
 	ctx *snow.Context,
-	rules params.Rules,
+	rules_ params.Rules,


(style) basicRules? That's the best word I can come up with, but I still think it's better than a suffix _ as that jumps out as very non-idiomatic Go.

Below as well.

Yeah I agree. I am switching to use using rules & rulesExtra in places where both the base rules and the extra rules are used.
I don't think "Extra" is very long but I am open to consider using like "rulesExt" if others prefer a shorter name.

ARR4N · 2024-10-21T03:13:36Z

plugin/evm/import_tx.go

@@ -62,8 +62,9 @@ func (utx *UnsignedImportTx) InputUTXOs() set.Set[ids.ID] {
 // Verify this transaction is well-formed
 func (utx *UnsignedImportTx) Verify(
 	ctx *snow.Context,
-	rules params.Rules,
+	rules_ params.Rules,


(style) As with the other rules_.

ARR4N · 2024-10-21T03:16:19Z

plugin/evm/vm.go

@@ -865,8 +867,9 @@ func (vm *VM) preBatchOnFinalizeAndAssemble(header *types.Header, state *state.S
 		// Note: snapshot is taken inside the loop because you cannot revert to the same snapshot more than
 		// once.
 		snapshot := state.Snapshot()
-		rules := vm.chainConfig.Rules(header.Number, header.Time)
-		if err := vm.verifyTx(tx, header.ParentHash, header.BaseFee, state, rules); err != nil {
+		rules_ := vm.chainConfig.Rules(header.Number, params.IsMergeTODO, header.Time)


(style) Here the variable can just be removed entirely.

rules := params.GetRulesExtra( vm.chainConfig.Rules(...), )

unfortunately rules is used to pass to verifyTx. I think we should maintain a consistent style to pass params.Rules and params.ChainConfig around in the code then get the extra in the function if needed.

ARR4N · 2024-10-21T03:16:46Z

plugin/evm/vm.go

@@ -1013,7 +1016,8 @@ func (vm *VM) onExtraStateChange(block *types.Block, state *state.StateDB) (*big
 		batchContribution *big.Int = big.NewInt(0)
 		batchGasUsed      *big.Int = big.NewInt(0)
 		header                     = block.Header()
-		rules                      = vm.chainConfig.Rules(header.Number, header.Time)
+		rules_                     = vm.chainConfig.Rules(header.Number, params.IsMergeTODO, header.Time)


(style) As above without the intermediate rules_.

same as above (also used in verifyTx)

ARR4N · 2024-10-21T05:42:11Z

predicate/predicate_bytes.go

@@ -62,3 +62,7 @@ func GetPredicateResultBytes(extraData []byte) ([]byte, bool) {
 	}
 	return extraData[params.DynamicFeeExtraDataSize:], true
 }
+
+func SetPredicateResultBytes(extraData []byte, predicateResults []byte) []byte {
+	return append(extraData[:params.DynamicFeeExtraDataSize], predicateResults...)


What is extra data supposed to be that we know the prefix? Please add a comment describing the parameter.

darioush force-pushed the use-libevm branch from 5b7c883 to 758c8bd Compare October 1, 2024 23:45

darioush added 9 commits October 1, 2024 16:47

format: as subnet-evm

314fe42

take changes up to d72ff25 from subnet-evm

72a4b00

complete use core/vm

b3647e3

format: as coreth

20754f7

ci fixes

6ff7f77

use race free version

512ae03

update version

0baae1a

rebase new libevm

4fe802b

remove WithExtras hack

df98c4e

darioush force-pushed the use-libevm branch from 758c8bd to df98c4e Compare October 2, 2024 00:23

ARR4N reviewed Oct 2, 2024

View reviewed changes

params/config.go Outdated Show resolved Hide resolved

darioush added 2 commits October 2, 2024 07:21

cleanup

8e223b9

fix genesis EthUpgrades

43dde81

darioush force-pushed the use-libevm branch from ca54d21 to 43dde81 Compare October 3, 2024 00:42

update libevm

0976d50

darioush force-pushed the use-libevm branch 2 times, most recently from f127a63 to 0976d50 Compare October 11, 2024 21:10

darioush added 12 commits October 11, 2024 14:11

Merge remote-tracking branch 'origin/master' into use-libevm

062dc35

fix for merge

80fb112

fix test

357fba6

move code to config/rules_extra.go

6bf7ca2

rename vars

fa77e8a

refactor: reduce calling params.GetExtra

44468d6

more reduce params.GetExtra

023849c

use upstream forks

9a158f0

pass bytes of predicate results to avoid parsing in params

aa77826

use more upstream types

2abcd61

Merge branch 'master' of github.com:ava-labs/coreth into use-libevm

a54dac6

nit

546b697

nit

ba3f538

darioush changed the base branch from master to libevm October 14, 2024 22:44

darioush marked this pull request as ready for review October 14, 2024 23:05

darioush requested review from ceyonur and a team as code owners October 14, 2024 23:05

darioush changed the title ~~wip: use libevm~~ use libevm: params, core/vm, eth/tracers/* + some of core/types Oct 14, 2024

darioush added 2 commits October 14, 2024 16:17

less use of params.Copy

bf9a87f

reduce diff

a1d7a11

darioush commented Oct 14, 2024

View reviewed changes

precompile/contract/interfaces.go Show resolved Hide resolved

ARR4N mentioned this pull request Oct 18, 2024

Review of params package in #662 #674

Merged

Review of params package in #662 (#674)

72a90dd

ARR4N reviewed Oct 21, 2024

View reviewed changes

darioush mentioned this pull request Oct 21, 2024

[wip] decouple precompiles from core/vm ava-labs/subnet-evm#1320

Closed

darioush added 2 commits October 21, 2024 11:54

review comments

4054dd4

GetPredicateResultBytes: remove extra return

8f6d766

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use libevm: params, core/vm, eth/tracers/* + some of core/types #662

use libevm: params, core/vm, eth/tracers/* + some of core/types #662

darioush commented Sep 26, 2024 •

edited

Loading

ARR4N left a comment

ARR4N Oct 17, 2024

darioush Oct 21, 2024

ARR4N Oct 17, 2024

ARR4N Oct 17, 2024

ARR4N Oct 21, 2024

darioush Oct 21, 2024

ARR4N Oct 21, 2024

darioush Oct 21, 2024

ARR4N Oct 21, 2024

darioush Oct 21, 2024 •

edited

Loading

ARR4N Oct 21, 2024

ARR4N Oct 21, 2024

darioush Oct 21, 2024

ARR4N Oct 21, 2024

darioush Oct 21, 2024

ARR4N Oct 21, 2024

use libevm: params, core/vm, eth/tracers/* + some of core/types #662

Are you sure you want to change the base?

use libevm: params, core/vm, eth/tracers/* + some of core/types #662

Conversation

darioush commented Sep 26, 2024 • edited Loading

Why this should be merged

How this works

How this was tested

ARR4N left a 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

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 Oct 21, 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

Choose a reason for hiding this comment

darioush commented Sep 26, 2024 •

edited

Loading

darioush Oct 21, 2024 •

edited

Loading