cmd/evm/internal/t8ntool, trie: support for verkle-at-genesis, use UBT, and move the transition tree to its own package

[gballet]

This is broken off of #31730 to only focus on testing networks that start with verkle at genesis.

The PR has seen a lot of work since its creation, and it now targets creating and re-executing tests for a binary tree testnet without the transition (so it starts at genesis). The transition tree has been moved to its own package. It also replaces verkle with the binary tree for this specific application.

[gballet]

@CPerezz drop this file

[gballet]

@CPerezz don't change that name yet

[gballet]

@CPerezz please replace all instances of trie.VerkleTrie with bintrie.BinaryTrie

[gballet]

verkle-specific ci pipeline, remove

[rjl493456442]

Any particular reason to add this condition?

I would prefer to always explicitly specify the setting for path database for verkle cases.

[CPerezz]

The Binary Trie implementation requires the pathdb backend (not hashdb). Adding || config.IsVerkle provides automatic pathdb selection when Binary Trie is enabled, even if PathDB config isn't explicitly provided.

You prefer to embed this within some Default behaviour or similar?

[rjl493456442]

I still think we shouldn't automatically select path mode when Verkle is true. It’s a minor point, but it feels odd to force the use of the path database for the binary tree.

BinaryTree should be totally compatible with hash mode.

[rjl493456442]

Just to confirm it's intentional, for verkle, the tests are based on the Shanghai fork right? Specifically the Cancun, Prague and Osaka will all be deactivated

[gballet]

yes, because our testing branch is an offshoot of the execution spec-tests rooted at shanghai. When they complete the "MELD" we will rebase our branch and re-root the thing at prague/osaka

[CPerezz]

Yes indeed! That is the case for now!

[rjl493456442]

Please don't populate the trie referenced by the stateDB.

[rjl493456442]

You can create a standalone binary tree with trie, err := s.db.OpenTrie(s.originalRoot) and operate with it

[CPerezz]

That's my bad! Thanks for the catch! Still new to Go and Geth so it's taking me some time to get up to speed with best practices!

[rjl493456442]

originally we dump out the states (accounts and storage slots) with s.DumpToCollector(collector, nil), i guess it's no longer possible with binarytrie right?

[CPerezz]

Correct. Binary Trie fundamentally cannot enumerate storage slots (it only supports key-based access). That's why we use DumpBinTrieLeaves to output leaf nodes in VKT format instead of the traditional alloc format.

This honestly is a pain. And one of the reasons for my previous comment (#32445 (comment))

This will need some investigation as the issue is not just doing it, is doing it on a performant way..
Help is always welcome 😛

[gballet]

it could be done by keeping some metadata in the tree, that is only used for tests and not hashed. It'd be ok as long as the state isn't too large. But that brings the question of what would then be tested, the production code, or the tests themselves? I want to think more about this.

[gballet]

by which I mean, let's not hold merging this PR for this.

[rjl493456442]

It's not used, maybe remove it from this PR?

[CPerezz]

True! Plus we aren't even testing witgen at all when comparing against Besu.

[rjl493456442]

Please elaborate the use case of using the specific binaryTrie data as the pre-state.

[CPerezz]

I thought mostly formatting and ease of use/test.

But can definitely change my mind here.

[gballet]

same thing as the other comment: it's impossible to go from a tree leaf to an account, so you need to rebuild the tree from scratch at each new block.

[rjl493456442]

How about we use a more general name? TreeLeaves ?

[gballet]

I did the renaming, but I also looked into the possibility of caching the accounts/slots and I don't think it makes sense to do that: I would have to cache the data at some global variable since the tree is opened independently multiple times (e.g. in system contracts), which means I would have to pass that global cache all over the code and that is a very invasive change for something we hope can disappear.

[rjl493456442]

Please apply these nitpick diffs

diff --git a/cmd/evm/internal/t8ntool/transition.go b/cmd/evm/internal/t8ntool/transition.go
index c47561e57b..3f148cd1e4 100644
--- a/cmd/evm/internal/t8ntool/transition.go
+++ b/cmd/evm/internal/t8ntool/transition.go
@@ -42,6 +42,7 @@ import (
 	"github.com/ethereum/go-ethereum/tests"
 	"github.com/ethereum/go-ethereum/trie/bintrie"
 	"github.com/ethereum/go-ethereum/triedb"
+	"github.com/ethereum/go-ethereum/triedb/database"
 	"github.com/holiman/uint256"
 	"github.com/urfave/cli/v2"
 )
@@ -118,13 +119,14 @@ func Transition(ctx *cli.Context) error {
 		}
 	}
 	prestate.Pre = inputData.Alloc
+
 	if btStr != stdinSelector && btStr != "" {
 		if err := readFile(btStr, "BT", &inputData.BT); err != nil {
 			return err
 		}
 	}
-
 	prestate.BT = inputData.BT
+
 	// Set the block environment
 	if envStr != stdinSelector {
 		var env stEnv
@@ -195,8 +197,10 @@ func Transition(ctx *cli.Context) error {
 		return err
 	}
 	// Dump the execution result
-	collector := make(Alloc)
-	var btleaves map[common.Hash]hexutil.Bytes
+	var (
+		collector = make(Alloc)
+		btleaves  map[common.Hash]hexutil.Bytes
+	)
 	isBinary := chainConfig.IsVerkle(big.NewInt(int64(prestate.Env.Number)), prestate.Env.Timestamp)
 	if !isBinary {
 		s.DumpToCollector(collector, nil)
@@ -382,9 +386,7 @@ func dispatchOutput(ctx *cli.Context, baseDir string, result *ExecutionResult, a
 	return nil
 }
 
-// The logic for tree key
-// BinKey computes the tree key given an address and an optional
-// slot number.
+// BinKey computes the tree key given an address and an optional slot number.
 func BinKey(ctx *cli.Context) error {
 	if ctx.Args().Len() == 0 || ctx.Args().Len() > 2 {
 		return errors.New("invalid number of arguments: expecting an address and an optional slot number")
@@ -423,8 +425,10 @@ func BinKeys(ctx *cli.Context) error {
 			return err
 		}
 	}
+	db := triedb.NewDatabase(rawdb.NewMemoryDatabase(), triedb.VerkleDefaults)
+	defer db.Close()
 
-	bt, err := genBinTrieFromAlloc(alloc)
+	bt, err := genBinTrieFromAlloc(alloc, db)
 	if err != nil {
 		return fmt.Errorf("error generating bt: %w", err)
 	}
@@ -465,8 +469,10 @@ func BinTrieRoot(ctx *cli.Context) error {
 			return err
 		}
 	}
+	db := triedb.NewDatabase(rawdb.NewMemoryDatabase(), triedb.VerkleDefaults)
+	defer db.Close()
 
-	bt, err := genBinTrieFromAlloc(alloc)
+	bt, err := genBinTrieFromAlloc(alloc, db)
 	if err != nil {
 		return fmt.Errorf("error generating bt: %w", err)
 	}
@@ -476,15 +482,11 @@ func BinTrieRoot(ctx *cli.Context) error {
 }
 
 // TODO(@CPerezz): Should this go to `bintrie` module?
-func genBinTrieFromAlloc(alloc core.GenesisAlloc) (*bintrie.BinaryTrie, error) {
-	bt, err := bintrie.NewBinaryTrie(types.EmptyBinaryHash, triedb.NewDatabase(rawdb.NewMemoryDatabase(),
-		&triedb.Config{
-			IsVerkle: true,
-		}))
+func genBinTrieFromAlloc(alloc core.GenesisAlloc, db database.NodeDatabase) (*bintrie.BinaryTrie, error) {
+	bt, err := bintrie.NewBinaryTrie(types.EmptyBinaryHash, db)
 	if err != nil {
 		return nil, err
 	}
-
 	for addr, acc := range alloc {
 		for slot, value := range acc.Storage {
 			err := bt.UpdateStorage(addr, slot.Bytes(), value.Big().Bytes())
@@ -492,7 +494,6 @@ func genBinTrieFromAlloc(alloc core.GenesisAlloc) (*bintrie.BinaryTrie, error) {
 				return nil, fmt.Errorf("error inserting storage: %w", err)
 			}
 		}
-
 		account := &types.StateAccount{
 			Balance:  uint256.MustFromBig(acc.Balance),
 			Nonce:    acc.Nonce,
@@ -503,7 +504,6 @@ func genBinTrieFromAlloc(alloc core.GenesisAlloc) (*bintrie.BinaryTrie, error) {
 		if err != nil {
 			return nil, fmt.Errorf("error inserting account: %w", err)
 		}
-
 		err = bt.UpdateContractCode(addr, common.BytesToHash(account.CodeHash), acc.Code)
 		if err != nil {
 			return nil, fmt.Errorf("error inserting code: %w", err)
diff --git a/core/state/dump.go b/core/state/dump.go
index 4a17d1adde..829d106ed3 100644
--- a/core/state/dump.go
+++ b/core/state/dump.go
@@ -18,6 +18,7 @@ package state
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"time"
 
@@ -224,17 +225,17 @@ func (s *StateDB) DumpToCollector(c DumpCollector, conf *DumpConfig) (nextKey []
 
 // DumpBinTrieLeaves collects all binary trie leaf nodes into the provided map.
 func (s *StateDB) DumpBinTrieLeaves(collector map[common.Hash]hexutil.Bytes) error {
-	if s.trie == nil {
-		trie, err := s.db.OpenTrie(s.originalRoot)
-		if err != nil {
-			return err
-		}
-		s.trie = trie
+	tr, err := s.db.OpenTrie(s.originalRoot)
+	if err != nil {
+		return err
 	}
-
-	it, err := s.trie.(*bintrie.BinaryTrie).NodeIterator(nil)
+	btr, ok := tr.(*bintrie.BinaryTrie)
+	if !ok {
+		return errors.New("trie is not a binary trie")
+	}
+	it, err := btr.NodeIterator(nil)
 	if err != nil {
-		panic(err)
+		return err
 	}
 	for it.Next(true) {
 		if it.Leaf() {
diff --git a/core/state/reader.go b/core/state/reader.go
index fb23c84837..93083c8ae2 100644
--- a/core/state/reader.go
+++ b/core/state/reader.go
@@ -276,7 +276,6 @@ func newTrieReader(root common.Hash, db *triedb.Database, cache *utils.PointCach
 			// by using TransitionTrie when there's no actual transition happening.
 			tr = transitiontrie.NewTransitionTrie(nil, binTrie, false)
 		}
-		err = binErr
 	}
 	if err != nil {
 		return nil, err
diff --git a/tests/block_test_util.go b/tests/block_test_util.go
index 99ce051573..2ced18787a 100644
--- a/tests/block_test_util.go
+++ b/tests/block_test_util.go
@@ -118,8 +118,8 @@ func (t *BlockTest) Run(snapshotter bool, scheme string, witness bool, tracer *t
 	}
 	// import pre accounts & construct test genesis block & state root
 	// Commit genesis state
-	gspec := t.genesis(config)
 	var (
+		gspec = t.genesis(config)
 		db    = rawdb.NewMemoryDatabase()
 		tconf = &triedb.Config{
 			Preimages: true,

[rjl493456442]

if root != emptyRoot {

}

[rjl493456442]

why change the var name here?

[gballet]

to differentiate between the MPT root and a verkle root, this is not apparent here but for the transition there will be two concurrent roots. But I will revert for now, since there's not going to be any "rebase".

[rjl493456442]

In the MPT, the statedb re-init is essential, as after the commit, the mutated nodes are all converted as the hash and de-reference from the trie.

The only way to access the them is re-open the trie with new root hash.

I am not sure how it's handled in the binaryTrie. I guess it somehow still reference the new nodes after the commit.

[CPerezz]

After Commit(), all mutated nodes are converted to hash nodes (line 763 in trie.go: t.root = newCommitter(...).Commit(...)).

1. The committer replaces all dirty nodes with their hash representations
2. The trie becomes unusable because nodes are now just hashes, not full nodes
   That's why we need to re-open the trie with state.New(root, sdb) to get a fresh trie that can load nodes from the database.

In Binary Trie (Verkle/Binary):

• The ``Commit()` method collects nodes but doesn't convert them to hashes
• The trie maintains its in-memory node structure after commit
• The trie remains usable after commit because nodes are still accessible

So you are right, we should check if we're in Bintrie EIP. And if so, do this, otherwise, if we are using MPT we should avoid doing that.

Does that make sense @gballet ?

[rjl493456442]

output.vkt is confusing, it essentially refers to the file storing the trie leaves right? It doesn't matter it's verkle/bintree/mpt

[gballet]

agreed, but the reason why we keep this is because we are dependent on a branch of execution spec tests that is no longer maintained and it has all these fields hardcoded.

[gballet]

so I'm banking on the fact this code can be removed soon.

[rjl493456442]

How about we use a more general name? TreeLeaves ?

[rjl493456442]

I still think we shouldn't automatically select path mode when Verkle is true. It’s a minor point, but it feels odd to force the use of the path database for the binary tree.

BinaryTree should be totally compatible with hash mode.