cmd, core/state, eth, tests, trie: improve state reader

[rjl493456442]

The state availability is checked when a state reader is requested to be created.

• In hash-based database, if the specified root node is not existent in disk, then
  the state reader won't be created and an error will be returned.

• In path-based database, if the specified state layer is not available, then the
  state reader won't be created and an error will be returned.

And also this PR emphasize a notion that: once the statedb instance is committed,
it's not usable. A new instance must be created with new root upon the updated
database in order to access full state correctly. This behavior is aligned with underlying
trie.

[gballet]

that's a big problem for me, as this means calling OpenTrie, which becomes an expensive operation with verkle. Is there a way that the trie could be reused?

[rjl493456442]

In our case, we always re-create a new state-db after processing a block, even now.

Re reusing the trie, it will be slightly ugly. Perhaps we can introduce some cache layer above the trie database which caches the decoded nodes. So that reloading trie node can be cheap?

[holiman]

For me, it's still a bit unclear what Once the state is committed, it's not usable anymore means, in practice.

For example, cmd/evm/runner.go does this:

	if ctx.Bool(DumpFlag.Name) {
		statedb.Commit(true)
		statedb.IntermediateRoot(true)
		fmt.Println(string(statedb.Dump(nil)))
	}

Is it "wrong"? Is it relying on undefined behaviour but is ok? Will it misbehave?

And in tests/state_test_util.go, we return a statedb which has already committed:

	root, _ := statedb.Commit(config.IsEIP158(block.Number()))
	return snaps, statedb, root, err
}

Later on, that statedb is used, for intermediate root and rawdumping:

			_, s, err := test.Run(st, cfg, false)
			// print state root for evmlab tracing
			if s != nil {
				root := s.IntermediateRoot(false)
..
				if dump && s != nil {
					dump := s.RawDump(nil)

And in cmd/evm/internal/t8ntool/execution.go, we commit and later ask for logs

	root, err := statedb.Commit(chainConfig.IsEIP158(vmContext.BlockNumber))
	if err != nil {
		return nil, nil, NewError(ErrorEVM, fmt.Errorf("could not commit state: %v", err))
	}
	execRs := &ExecutionResult{
...
		LogsHash:    rlpHash(statedb.Logs()),

We do some funky commit then commit in core/chain_makers.go

			// Write state changes to db
			root, err := statedb.Commit(config.IsEIP158(b.header.Number))
			if err != nil {
				panic(fmt.Sprintf("state write error: %v", err))
			}
			if err := statedb.Database().TrieDB().Commit(root, false); err != nil {
				panic(fmt.Sprintf("trie write error: %v", err))
			}

So, TLDR is that it needs to be more obvious what is allowed vs what is not allowed, and potentially we should even make it explicit: e.g. we could panic if the api is misused (or maybe, in production code, print a very stern warning is better than panic)

[holiman]

Note for reviewers: This is the big functional change, as far as I understand. This change makes it so that a Commit-then-Copy leads to a non-functional statedb, since the trieReader does not actually have a reader.

[rjl493456442]

e.g. we could panic if the api is misused (or maybe, in production code, print a very stern warning is better than panic)

I will think about it. I am not a big fan of this approach. But I totally get it. I didn't do it because we need a marker to represent this state-db is committed and following operations should be rejected. It's a big change then.

But now we just assume that users won't use committed state-db, and a part of APIs will still work, and may not work(e.g. a copied state-db from the committed one). It's just too implicit.

Maybe I need to find a line, that this API is not usable after commit, and these APIs can still work.

[holiman]

I'm curious though... Those examples I listed, what is the status on them? Ok or Not?

[rjl493456442]

I'm curious though... Those examples I listed, what is the status on them? Ok or Not?

They can still work..

statedb.IntermediateRoot(true) works because root hash is cached.
dump := s.RawDump(nil) rawdump shouldn't work, I will check it later.
statedb.Logs() logs work.

In another word, only the trie is not functional after commit, if trie is not touched, then it's fine.

[rjl493456442]

@holiman And in tests/state_test_util.go, we return a statedb which has already committed:

The case you mentioned above is actually be fixed by re-initialize a new one here https://github.com/rjl493456442/go-ethereum/blob/fix-state-commit-2/tests/state_test_util.go#L208

[holiman]

Review-wise this looks fine to me now, but I haven't tested the code thoroughly to check if there are any new places where it fails now, due to the new error-condition.
Did you run it on any of our bench-machines @rjl493456442 ?

[rjl493456442]

@holiman deployed on benchmark08, on top of a sync'd node. Let's see if it goes well.

[rjl493456442]

After running it for a few hours on benchmark08, nothing is wrong.