core/state: state reader abstraction

[rjl493456442]

This pull request introduces a state.Reader interface for state accessing.

The interface could be implemented in various ways. It can be pure trie
only reader, or the combination of trie and state snapshot. What's more,
this interface allows us to have more flexibility in the future, e.g. the
archive reader (for accessing archive state).

Additionally, this pull request removes the following metrics

• chain/snapshot/account/reads
• chain/snapshot/storage/reads

[holiman]

Deserve top level exposure: this is the actual state reader interface

// Reader defines the interface for accessing accounts or storage slots
// associated with a specific state.
type Reader interface {
	// Account retrieves the account associated with a particular address.
	//
	// - A nil account is returned if it's not existent
	// - An error is only returned if any unexpected error occurs
	// - The returned account is safe to modify afterwards
	Account(addr common.Address) (*types.StateAccount, error)

	// Storage retrieves the storage slot associated with a particular account
	// address and slot key.
	//
	// - An empty slot is returned if it's not existent
	// - An error is only returned if any unexpected error occurs
	// - The returned storage slot is safe to modify
	Storage(addr common.Address, storageRoot common.Hash, slot common.Hash) (common.Hash, error)

	// Copy returns a deep-copied state reader.
	Copy() Reader
}

[holiman]

The semantics here are a bit unclear. From a state reader, we can obtain a *types.StateAccount. We can mutate that object. If we query the reader again for the same addres, I see from the implementations that we obtain the original objects.

But then it becomes a bit strange why we have a Copy method. Because why would we deep-copy if we only ever hand out shallow copies of accounts/data?

For snapReader, the only reason to Copy would be if the caller wants to do multi-threaded querying, in which case we need to use a separate buff for each thread.

So perhaps the comment should say

// Copy returns a standalone deep-copied state reader, which is safe to access concurrent to the original.

[rjl493456442]

The copy function is mostly for trieReader. In the trie reader implementations, it will cache the resolved trie/trie nodes locally. If we do a state copy, i think it makes sense to all carry these trie/trienode cache for performance consideration.

Regarding the buffer in snapReader, deep-copy prevents the concurrency issue in the first place.

Personally i am inclined to have this copy function supported.

[holiman]

This subTries is a map that only ever grows, right? Seems like it could potentially cause problems at some point, e.g. if we have some long-lived thread doing caching or iterating the trie, and doing Storage lookups. Then we'll keep amassing this map and never delete data.

IMO we should use some sort of capped data structure for this.

[rjl493456442]

it's true that this map could be an out-of-memory issue. However, i think it's same now. In the current implementation, the storage tries will be linked to state object and be retained forever if the stateDB instnace is long-lived.

I think we should avoid this situation in the first place instead of introducing the complexities into the reader itself.

[holiman]

Just thinking aloud here. We could have a mptReader and a verkleReader, instead of a trieReader which implements both? Or would that be strange from an abstraction perspective? I haven't thought deeply about this like you, but I'd be interested to hear your thoughts on it?

[rjl493456442]

Right. It's possible to have merkleReader and verkleReader as two separate implementations. I am fine with both direction honestly.

The only catch up is Trie interface is still necessary in many places, e.g. in state object, we can't get rid of this interface simply. Therefore, I personally prefer to have a combined trie reader with a Trie interface handling the differentiation, instead of two trie reader implementations with basically same logic

[holiman]

Previously, we only started a prefetcher if s.snap was non-nil. I guess it's an old remain from when it was possible to run without snapshots. Technically, it still is.
I'm not 100% about the consequence of this particular change... ?

[rjl493456442]

Yes, originally we only construct the prefetcher if the snapshot is available. Maintaining the prefetcher with no snapshot could introduce an unnecessary overhead.

For example, the storage trie will be resolved when we access the storage slot without state snapshot (https://github.com/ethereum/go-ethereum/blob/master/core/state/state_object.go#L219C16-L219C23) and the prefetching will be scheduled in the mean time (https://github.com/ethereum/go-ethereum/blob/master/core/state/state_object.go#L236). We resolve the trie nodes twice

However, it's essential in this pull request, it's opaque to stateDB whether the state snapshot is available or not. We must always enable the prefetcher if it's requested.

[holiman]

Wow, the code becomes so much cleaner :)

[rjl493456442]

Highlight the change in the release note

[rjl493456442]

The benchmark has been set up for a few days. The result could be checked here

I think the overall performance is not affected, although this branch is ~40minutes behind.

[holiman]

Yeah let's gooo

[MariusVanDerWijden]

LGTM

[karalabe]

Just a question, we're sure it's fine to deep copy all the resolves subtries? I don't remember whether in the statedb we copy the caches or not.

[rjl493456442]

We do the same thing for the resolved storages in the contract, e.g.

originStorage:      s.originStorage.Copy(),

But to be fair, originalStorage coping is necessary as we filter out the non-changed slot commit at the end of the block, otherwise we need to explicitly resolve them from the reader if the original value is not found.

for key, val := range s.pendingStorage {
		// Skip the noop storage changes, it might be possible the value
		// of tracked slot is same in originStorage and pendingStorage
		// map, e.g. the storage slot is modified in tx_a and then reset
		// back in tx_b.
		if val == s.originStorage[key] {
			continue
		}
		hash := crypto.HashData(buf, key[:])
		if op.storages == nil {
			op.storages = make(map[common.Hash][]byte)
		}
		op.storages[hash] = encode(val)
		if op.storagesOrigin == nil {
			op.storagesOrigin = make(map[common.Hash][]byte)
		}
		op.storagesOrigin[hash] = encode(s.originStorage[key])

		// Overwrite the clean value of storage slots
		s.originStorage[key] = val
	}

It's also fine to NOT introduce the copy function to state reader, and we always re-create a fresh new reader when statedb.Copy happens

[karalabe]

LGTM