core/state: access trie through Database interface, track errors #14589

fjl · 2017-06-06T11:40:18Z

Motivation for this change:

We can remove the light client's duplicated copy of core/state.
The light client now supports node iteration, so tracing and storage enumeration can
work with the light client.

It opens the door to more code deletions later:

We can remove superfluous Trie methods Get, Update, ... in a later PR because they're
no longer used by core/state.
We can remove SecureTrie because core/state is now aware of key hashing.
We can remove ForEachStorage because we now have a StorageTrie method.

bailantaotao · 2017-06-07T13:08:05Z

trie/iterator.go

+	// Callers must not retain references to the return value after calling Next
+	Path() []byte
+
+	// Leaf returns true iff the current node is a leaf node.


typo: "iff"

see https://en.wiktionary.org/wiki/iff. I think it's not needed here, but the word was already there and I didn't want to change the original comment.

oh... got it, thanks

holiman · 2017-06-08T14:33:48Z

core/blockchain.go

@@ -531,7 +526,7 @@ func (bc *BlockChain) HasBlockAndState(hash common.Hash) bool {
 		return false
 	}
 	// Ensure the associated state is also present
-	_, err := state.New(block.Root(), bc.chainDb)
+	_, err := state.New(block.Root(), bc.stateCache)


The state.New does a lot of allocation and things (https://github.com/ethereum/go-ethereum/blob/master/core/state/statedb.go#L89) . Seems a bit wasteful to instantiate one and just throw it away. Couldn't we just verify the state without actually allocating the entire instance in this case?

I suppose we could use stateCache.OpenTrie to check.

holiman · 2017-06-08T14:38:34Z

core/blockchain.go

 		}
+		state, err := state.New(parent.Root(), bc.stateCache)


For inserting N blocks, you'll be instantiating a new state N times. Is that really needed? In most situations (after the first block), the state will already be where you want it to be - no action needed.

The old version used Reset on stateCache, which was a StateDB. In the new code, stateCache is state.Database and state.New is about as expensive as Reset was.

fjl · 2017-06-16T08:18:36Z

Note: the first few commits are part of #14615 and can be removed when that's merged.

zsfelfoldi

LGTM in general, see inline comments.

zsfelfoldi · 2017-06-21T00:56:13Z

light/trie.go

+	return &odrDatabase{ctx, StateTrieID(head), odr}
+}
+
+type odrDatabase struct {


Since we are implementing the entire odr database backend for the state here (not just the trie), maybe we should rename this file to something like odr_database.go.

zsfelfoldi · 2017-06-21T00:56:51Z

core/state/database.go

+func (m cachedTrie) CommitTo(dbw trie.DatabaseWriter) (common.Hash, error) {
+	root, err := m.SecureTrie.CommitTo(dbw)
+	if err == nil {
+		m.db.pushTrie(m.SecureTrie)


Maybe I'm mistaken but it seems like now we are caching storage tries too, which makes the entire pastTries cache useless since there are usually a lot of storage tries processed per block.

OpenStorageTrie doesn't return cachedTrie, it returns *trie.SecureTrie.

zsfelfoldi · 2017-06-21T00:58:04Z

light/trie.go

+	if codeHash == sha3_nil {
+		return nil, nil
+	}
+	if code, err := db.backend.Database().Get(codeHash[:]); err == nil {


Since StateDB is using a cachingDB which also checks the database, isn't this check redundant?

StateDB doesn't use cachingDB is the Database returned by state.NewDatabase. It's not used here.

zsfelfoldi · 2017-06-21T01:00:42Z

light/trie.go

+	return len(code), err
+}
+
+type odrTrie struct {


This is something I should have done already but please either add a comment stating that odrTrie is a SecureTrie (key caching is included) or rename it to odrSecureTrie. Maybe the comment is enough.

I will move hashing to StateDB in a follow-up PR.

Arachnid · 2017-06-21T12:53:45Z

core/state/database.go

@@ -0,0 +1,152 @@
+// Copyright 2014 The go-ethereum Authors


s/2014/21017/

Arachnid · 2017-06-21T12:55:03Z

core/state/database.go

+	CommitTo(trie.DatabaseWriter) (common.Hash, error)
+	Hash() common.Hash
+	NodeIterator(startKey []byte) trie.NodeIterator
+	GetKey([]byte) []byte // TODO(fjl): remove this


When SecureTrie is removed.

Can you add a note to that effect? Otherwise there's no way to know when this todo is relevant for.

Arachnid · 2017-06-21T12:58:34Z

core/state/database.go

+const (
+	// Number of past tries to keep. This value is chosen such that
+	// reasonable chain reorg depths will hit an existing trie.
+	maxPastTries = 12


Mmm, pastries.

Arachnid · 2017-06-21T12:59:23Z

core/state/database.go

+type Database interface {
+	// Accessing tries
+	OpenTrie(root common.Hash) (Trie, error)
+	OpenStorageTrie(addrHash, root common.Hash) (Trie, error)


Why can't we just use OpenTrie here? And what is addrHash used for?

IMHO it's useful to distinguish between storage tries and the main accounts trie. For example, the cachingDB caches the accounts trie on commit, but doesn't cache storage at all. addrHash is used by the light client, you can see how in les/trie.go.

Also improve error reporting in ODR tests because it was impossible to see what's wrong when the only output is "odr mismatch".

SheldonZhong · 2018-11-08T05:48:41Z

light/trie.go

+func (t *odrTrie) TryUpdate(key, value []byte) error {
+	key = crypto.Keccak256(key)
+	return t.do(key, func() error {
+		return t.trie.TryDelete(key)


I have written my question in here. I'm not sure whether anybody would be notified to see it. So I comment again in this PR. @fjl

Hi there, I'm confused here a little bit. Why TryUpdate does not call t.trie.TryUpdate(key, value) and calls t.trie.TryDelete instead? This is not apparent from my point of view. The update operation simply deletes the corresponding entry, though it could retrieve later by odr. However, it adds further network overhead.

bailantaotao reviewed Jun 7, 2017

View reviewed changes

fjl force-pushed the state-errtrack branch from fc023e5 to a4f9acd Compare June 7, 2017 15:31

holiman reviewed Jun 8, 2017

View reviewed changes

fjl mentioned this pull request Jun 13, 2017

trie: more node iterator improvements #14615

Merged

fjl force-pushed the state-errtrack branch 3 times, most recently from ee25500 to 8a31557 Compare June 16, 2017 08:15

fjl force-pushed the state-errtrack branch 2 times, most recently from 8e76ab5 to 1d90246 Compare June 20, 2017 16:32

zsfelfoldi self-requested a review June 20, 2017 23:36

zsfelfoldi reviewed Jun 21, 2017

View reviewed changes

zsfelfoldi approved these changes Jun 21, 2017

View reviewed changes

fjl added this to the 1.6.7 milestone Jun 21, 2017

Arachnid reviewed Jun 21, 2017

View reviewed changes

fjl force-pushed the state-errtrack branch from e0683a8 to 916ee18 Compare June 27, 2017 10:47

Arachnid approved these changes Jun 27, 2017

View reviewed changes

fjl added 4 commits June 27, 2017 14:00

core/state: access trie through Database interface, track errors

44c5e48

les, light: use core/state implementation of state

cc860e3

Also improve error reporting in ODR tests because it was impossible to see what's wrong when the only output is "odr mismatch".

trie: don't crash for invalid proof (needs investigation)

df0056c

core: use OpenTrie to check state presence

c5a5bf7

fjl force-pushed the state-errtrack branch from f364f52 to c5a5bf7 Compare June 27, 2017 12:00

fjl merged commit 9e5f03b into ethereum:master Jun 27, 2017

holiman mentioned this pull request Jul 16, 2017

Impossible reorg, please file an issue #14740

Closed

SheldonZhong reviewed Nov 8, 2018

View reviewed changes

holiman mentioned this pull request May 11, 2023

internal/ethapi: remove unused returned error in GetEVM function #27240

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core/state: access trie through Database interface, track errors #14589

core/state: access trie through Database interface, track errors #14589

fjl commented Jun 6, 2017 •

edited

Loading

bailantaotao Jun 7, 2017

fjl Jun 7, 2017

bailantaotao Jun 7, 2017

holiman Jun 8, 2017

fjl Jun 21, 2017

holiman Jun 8, 2017

fjl Jun 21, 2017

fjl commented Jun 16, 2017

zsfelfoldi left a comment

zsfelfoldi Jun 21, 2017

zsfelfoldi Jun 21, 2017

fjl Jun 21, 2017

zsfelfoldi Jun 21, 2017

fjl Jun 21, 2017

zsfelfoldi Jun 21, 2017

fjl Jun 21, 2017

Arachnid Jun 21, 2017

Arachnid Jun 21, 2017

fjl Jun 26, 2017

Arachnid Jun 27, 2017

Arachnid Jun 21, 2017

Arachnid Jun 21, 2017

fjl Jun 27, 2017

SheldonZhong Nov 8, 2018 •

edited

Loading

core/state: access trie through Database interface, track errors #14589

core/state: access trie through Database interface, track errors #14589

Conversation

fjl commented Jun 6, 2017 • 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

fjl commented Jun 16, 2017

zsfelfoldi 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

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

SheldonZhong Nov 8, 2018 • edited Loading

Choose a reason for hiding this comment

fjl commented Jun 6, 2017 •

edited

Loading

SheldonZhong Nov 8, 2018 •

edited

Loading