core, eth, trie: use TryGetAccount to read what TryUpdateAccount has written #25458 (#1106)

* core: use TryGetAccount to read where TryUpdateAccount has been used to write

* Gary's review feedback

* implement Gary's suggestion

* fix bug + rename NewSecure into NewStateTrie

* trie: add backwards-compatibility aliases for SecureTrie

* Update database.go

* make the linter happy

Co-authored-by: Guillaume Ballet <3272758+gballet@users.noreply.github.com>
Co-authored-by: Felix Lange <fjl@twurst.com>
Co-authored-by: rjl493456442 <garyrong0905@gmail.com>

XDCx/tradingstate/statedb.go

@@ -431,6 +431,7 @@ func (t *TradingStateDB) getStateExchangeObject(addr common.Hash) (stateObject *
 		return obj
 	}
 	// Load the object from the database.
+	// TODO(daniel): use trie.TryGetAccount, ref PR #25458
 	enc, err := t.trie.TryGet(addr[:])
 	if len(enc) == 0 {
 		t.setError(err)

XDCxlending/lendingstate/statedb.go

@@ -416,6 +416,7 @@ func (ls *LendingStateDB) getLendingExchange(addr common.Hash) (stateObject *len
 		return obj
 	}
 	// Load the object from the database.
+	// TODO(daniel): use trie.TryGetAccount, ref PR #25458
 	enc, err := ls.trie.TryGet(addr[:])
 	if len(enc) == 0 {
 		ls.setError(err)

core/blockchain.go

@@ -543,7 +543,7 @@ func (bc *BlockChain) FastSyncCommitHead(hash common.Hash) error {
 	if block == nil {
 		return fmt.Errorf("non existent block [%x..]", hash[:4])
 	}
-	if _, err := trie.NewSecure(common.Hash{}, block.Root(), bc.stateCache.TrieDB()); err != nil {
+	if _, err := trie.NewStateTrie(common.Hash{}, block.Root(), bc.stateCache.TrieDB()); err != nil {
 		return err
 	}
 

core/state/database.go

@@ -62,7 +62,7 @@ type Trie interface {
 	// GetKey returns the sha3 preimage of a hashed key that was previously used
 	// to store a value.
 	//
-	// TODO(fjl): remove this when SecureTrie is removed
+	// TODO(fjl): remove this when StateTrie is removed
 	GetKey([]byte) []byte
 
 	// TryGet returns the value for key stored in the trie. The value bytes must
@@ -70,8 +70,8 @@ type Trie interface {
 	// trie.MissingNodeError is returned.
 	TryGet(key []byte) ([]byte, error)
 
-	// TryUpdateAccount abstract an account write in the trie.
+	// TryGetAccount abstract an account read from the trie.
-	TryUpdateAccount(key []byte, account *types.StateAccount) error
+	TryGetAccount(key []byte) (*types.StateAccount, error)
 
 	// TryUpdate associates key with value in the trie. If value has length zero, any
 	// existing value is deleted from the trie. The value bytes must not be modified
@@ -79,6 +79,9 @@ type Trie interface {
 	// database, a trie.MissingNodeError is returned.
 	TryUpdate(key, value []byte) error
 
+	// TryUpdateAccount abstract an account write to the trie.
+	TryUpdateAccount(key []byte, account *types.StateAccount) error
+
 	// TryDelete removes any existing value for key from the trie. If a node was not
 	// found in the database, a trie.MissingNodeError is returned.
 	TryDelete(key []byte) error
@@ -135,18 +138,26 @@ type cachingDB struct {
 
 // OpenTrie opens the main account trie at a specific root hash.
 func (db *cachingDB) OpenTrie(root common.Hash) (Trie, error) {
-	return trie.NewSecure(common.Hash{}, root, db.db)
+	tr, err := trie.NewStateTrie(common.Hash{}, root, db.db)
+	if err != nil {
+		return nil, err
+	}
+	return tr, nil
 }
 
 // OpenStorageTrie opens the storage trie of an account.
 func (db *cachingDB) OpenStorageTrie(addrHash, root common.Hash) (Trie, error) {
-	return trie.NewSecure(addrHash, root, db.db)
+	tr, err := trie.NewStateTrie(addrHash, root, db.db)
+	if err != nil {
+		return nil, err
+	}
+	return tr, nil
 }
 
 // CopyTrie returns an independent copy of the given trie.
 func (db *cachingDB) CopyTrie(t Trie) Trie {
 	switch t := t.(type) {
-	case *trie.SecureTrie:
+	case *trie.StateTrie:
 		return t.Copy()
 	default:
 		panic(fmt.Errorf("unknown trie type %T", t))

core/state/statedb.go

@@ -537,21 +537,15 @@ func (s *StateDB) getDeletedStateObject(addr common.Address) *stateObject {
 	if obj := s.stateObjects[addr]; obj != nil {
 		return obj
 	}
-	// Track the amount of time wasted on loading the object from the database
-	start := time.Now()
 	// Load the object from the database
-	enc, err := s.trie.TryGet(addr.Bytes())
+	start := time.Now()
+	data, err := s.trie.TryGetAccount(addr.Bytes())
 	s.AccountReads += time.Since(start)
 	if err != nil {
-		s.setError(fmt.Errorf("getDeleteStateObject (%x) error: %v", addr.Bytes(), err))
+		s.setError(fmt.Errorf("getDeleteStateObject (%x) error: %w", addr.Bytes(), err))
 		return nil
 	}
-	if len(enc) == 0 {
+	if data == nil {
-		return nil
-	}
-	data := new(types.StateAccount)
-	if err := rlp.DecodeBytes(enc, data); err != nil {
-		log.Error("Failed to decode state object", "addr", addr, "err", err)
 		return nil
 	}
 	// Insert into the live set

eth/api.go

@@ -502,11 +502,11 @@ func (api *DebugAPI) getModifiedAccounts(startBlock, endBlock *types.Block) ([]c
 	}
 	triedb := api.eth.BlockChain().StateCache().TrieDB()
 
-	oldTrie, err := trie.NewSecure(common.Hash{}, startBlock.Root(), triedb)
+	oldTrie, err := trie.NewStateTrie(common.Hash{}, startBlock.Root(), triedb)
 	if err != nil {
 		return nil, err
 	}
-	newTrie, err := trie.NewSecure(common.Hash{}, endBlock.Root(), triedb)
+	newTrie, err := trie.NewStateTrie(common.Hash{}, endBlock.Root(), triedb)
 	if err != nil {
 		return nil, err
 	}

eth/downloader/downloader_test.go

@@ -192,7 +192,7 @@ func (dl *downloadTester) CurrentFastBlock() *types.Block {
 func (dl *downloadTester) FastSyncCommitHead(hash common.Hash) error {
 	// For now only check that the state trie is correct
 	if block := dl.GetBlockByHash(hash); block != nil {
-		_, err := trie.NewSecure(common.Hash{}, block.Root(), trie.NewDatabase(dl.stateDb))
+		_, err := trie.NewStateTrie(common.Hash{}, block.Root(), trie.NewDatabase(dl.stateDb))
 		return err
 	}
 	return fmt.Errorf("non existent block: %x", hash[:4])

trie/iterator_test.go

@@ -525,11 +525,11 @@ func (l *loggingDb) Close() error {
 }
 
 // makeLargeTestTrie create a sample test trie
-func makeLargeTestTrie() (*Database, *SecureTrie, *loggingDb) {
+func makeLargeTestTrie() (*Database, *StateTrie, *loggingDb) {
 	// Create an empty trie
 	logDb := &loggingDb{0, memorydb.New()}
 	triedb := NewDatabase(logDb)
-	trie, _ := NewSecure(common.Hash{}, common.Hash{}, triedb)
+	trie, _ := NewStateTrie(common.Hash{}, common.Hash{}, triedb)
 
 	// Fill it with some arbitrary data
 	for i := 0; i < 10000; i++ {

trie/proof.go

@@ -98,9 +98,9 @@ func (t *Trie) Prove(key []byte, fromLevel uint, proofDb ethdb.KeyValueWriter) e
 // Node and can be retrieved by verifying the proof.
 //
 // If the trie does not contain a value for key, the returned proof contains all
-// nodes of the longest existing prefix of the key (at least the root Node), ending
+// nodes of the longest existing prefix of the key (at least the root node), ending
-// with the Node that proves the absence of the key.
+// with the node that proves the absence of the key.
-func (t *SecureTrie) Prove(key []byte, fromLevel uint, proofDb ethdb.KeyValueWriter) error {
+func (t *StateTrie) Prove(key []byte, fromLevel uint, proofDb ethdb.KeyValueWriter) error {
 	return t.trie.Prove(key, fromLevel, proofDb)
 }
 

trie/secure_trie.go

@@ -25,25 +25,35 @@ import (
 	"github.com/XinFinOrg/XDPoSChain/rlp"
 )
 
-// SecureTrie wraps a trie with key hashing. In a secure trie, all
+// SecureTrie is the old name of StateTrie.
+// Deprecated: use StateTrie.
+type SecureTrie = StateTrie
+
+// NewSecure creates a new StateTrie.
+// Deprecated: use NewStateTrie.
+func NewSecure(owner common.Hash, root common.Hash, db *Database) (*SecureTrie, error) {
+	return NewStateTrie(owner, root, db)
+}
+
+// StateTrie wraps a trie with key hashing. In a secure trie, all
 // access operations hash the key using keccak256. This prevents
 // calling code from creating long chains of nodes that
 // increase the access time.
 //
-// Contrary to a regular trie, a SecureTrie can only be created with
+// Contrary to a regular trie, a StateTrie can only be created with
 // New and must have an attached database. The database also stores
 // the Preimage of each key.
 //
-// SecureTrie is not safe for concurrent use.
+// StateTrie is not safe for concurrent use.
-type SecureTrie struct {
+type StateTrie struct {
 	trie             Trie
 	preimages        *preimageStore
 	hashKeyBuf       [common.HashLength]byte
 	secKeyCache      map[string][]byte
-	secKeyCacheOwner *SecureTrie // Pointer to self, replace the key Cache on mismatch
+	secKeyCacheOwner *StateTrie // Pointer to self, replace the key cache on mismatch
 }
 
-// NewSecure creates a trie with an existing root node from a backing database
+// NewStateTrie creates a trie with an existing root node from a backing database
 // and optional intermediate in-memory node pool.
 //
 // If root is the zero hash or the sha3 hash of an empty string, the
@@ -54,20 +64,20 @@ type SecureTrie struct {
 // Loaded nodes are kept around until their 'cache generation' expires.
 // A new cache generation is created by each call to Commit.
 // cachelimit sets the number of past cache generations to keep.
-func NewSecure(owner common.Hash, root common.Hash, db *Database) (*SecureTrie, error) {
+func NewStateTrie(owner common.Hash, root common.Hash, db *Database) (*StateTrie, error) {
 	if db == nil {
-		panic("trie.NewSecure called without a database")
+		panic("trie.NewStateTrie called without a database")
 	}
 	trie, err := New(owner, root, db)
 	if err != nil {
 		return nil, err
 	}
-	return &SecureTrie{trie: *trie, preimages: db.preimages}, nil
+	return &StateTrie{trie: *trie, preimages: db.preimages}, nil
 }
 
 // Get returns the value for key stored in the trie.
 // The value bytes must not be modified by the caller.
-func (t *SecureTrie) Get(key []byte) []byte {
+func (t *StateTrie) Get(key []byte) []byte {
 	res, err := t.TryGet(key)
 	if err != nil {
 		log.Error(fmt.Sprintf("Unhandled trie error: %v", err))
@@ -77,20 +87,51 @@ func (t *SecureTrie) Get(key []byte) []byte {
 
 // TryGet returns the value for key stored in the trie.
 // The value bytes must not be modified by the caller.
-// If a Node was not found in the database, a MissingNodeError is returned.
+// If a node was not found in the database, a MissingNodeError is returned.
-func (t *SecureTrie) TryGet(key []byte) ([]byte, error) {
+func (t *StateTrie) TryGet(key []byte) ([]byte, error) {
 	return t.trie.TryGet(t.hashKey(key))
 }
 
+func (t *StateTrie) TryGetAccount(key []byte) (*types.StateAccount, error) {
+	var ret types.StateAccount
+	res, err := t.trie.TryGet(t.hashKey(key))
+	if err != nil {
+		log.Error(fmt.Sprintf("Unhandled trie error: %v", err))
+		return &ret, err
+	}
+	if res == nil {
+		return nil, nil
+	}
+	err = rlp.DecodeBytes(res, &ret)
+	return &ret, err
+}
+
+// TryGetAccountWithPreHashedKey does the same thing as TryGetAccount, however
+// it expects a key that is already hashed. This constitutes an abstraction leak,
+// since the client code needs to know the key format.
+func (t *StateTrie) TryGetAccountWithPreHashedKey(key []byte) (*types.StateAccount, error) {
+	var ret types.StateAccount
+	res, err := t.trie.TryGet(key)
+	if err != nil {
+		log.Error(fmt.Sprintf("Unhandled trie error: %v", err))
+		return &ret, err
+	}
+	if res == nil {
+		return nil, nil
+	}
+	err = rlp.DecodeBytes(res, &ret)
+	return &ret, err
+}
+
 // TryGetNode attempts to retrieve a trie node by compact-encoded path. It is not
 // possible to use keybyte-encoding as the path might contain odd nibbles.
-func (t *SecureTrie) TryGetNode(path []byte) ([]byte, int, error) {
+func (t *StateTrie) TryGetNode(path []byte) ([]byte, int, error) {
 	return t.trie.TryGetNode(path)
 }
 
 // TryUpdateAccount will abstract the write of an account to the
 // secure trie.
-func (t *SecureTrie) TryUpdateAccount(key []byte, acc *types.StateAccount) error {
+func (t *StateTrie) TryUpdateAccount(key []byte, acc *types.StateAccount) error {
 	hk := t.hashKey(key)
 	data, err := rlp.EncodeToBytes(acc)
 	if err != nil {
@@ -109,7 +150,7 @@ func (t *SecureTrie) TryUpdateAccount(key []byte, acc *types.StateAccount) error
 //
 // The value bytes must not be modified by the caller while they are
 // stored in the trie.
-func (t *SecureTrie) Update(key, value []byte) {
+func (t *StateTrie) Update(key, value []byte) {
 	if err := t.TryUpdate(key, value); err != nil {
 		log.Error(fmt.Sprintf("Unhandled trie error: %v", err))
 	}
@@ -122,8 +163,8 @@ func (t *SecureTrie) Update(key, value []byte) {
 // The value bytes must not be modified by the caller while they are
 // stored in the trie.
 //
-// If a Node was not found in the database, a MissingNodeError is returned.
+// If a node was not found in the database, a MissingNodeError is returned.
-func (t *SecureTrie) TryUpdate(key, value []byte) error {
+func (t *StateTrie) TryUpdate(key, value []byte) error {
 	hk := t.hashKey(key)
 	err := t.trie.TryUpdate(hk, value)
 	if err != nil {
@@ -134,15 +175,15 @@ func (t *SecureTrie) TryUpdate(key, value []byte) error {
 }
 
 // Delete removes any existing value for key from the trie.
-func (t *SecureTrie) Delete(key []byte) {
+func (t *StateTrie) Delete(key []byte) {
 	if err := t.TryDelete(key); err != nil {
 		log.Error(fmt.Sprintf("Unhandled trie error: %v", err))
 	}
 }
 
 // TryDelete removes any existing value for key from the trie.
-// If a Node was not found in the database, a MissingNodeError is returned.
+// If a node was not found in the database, a MissingNodeError is returned.
-func (t *SecureTrie) TryDelete(key []byte) error {
+func (t *StateTrie) TryDelete(key []byte) error {
 	hk := t.hashKey(key)
 	delete(t.getSecKeyCache(), string(hk))
 	return t.trie.TryDelete(hk)
@@ -150,7 +191,7 @@ func (t *SecureTrie) TryDelete(key []byte) error {
 
 // GetKey returns the sha3 Preimage of a hashed key that was
 // previously used to store a value.
-func (t *SecureTrie) GetKey(shaKey []byte) []byte {
+func (t *StateTrie) GetKey(shaKey []byte) []byte {
 	if key, ok := t.getSecKeyCache()[string(shaKey)]; ok {
 		return key
 	}
@@ -167,7 +208,7 @@ func (t *SecureTrie) GetKey(shaKey []byte) []byte {
 // All cached preimages will be also flushed if preimages recording is enabled.
 // Once the trie is committed, it's not usable anymore. A new trie must
 // be created with new root and updated trie database for following usage
-func (t *SecureTrie) Commit(collectLeaf bool) (common.Hash, *NodeSet, error) {
+func (t *StateTrie) Commit(collectLeaf bool) (common.Hash, *NodeSet, error) {
 	// Write all the pre-images to the actual disk database
 	if len(t.getSecKeyCache()) > 0 {
 		if t.preimages != nil {
@@ -183,15 +224,15 @@ func (t *SecureTrie) Commit(collectLeaf bool) (common.Hash, *NodeSet, error) {
 	return t.trie.Commit(collectLeaf)
 }
 
-// Hash returns the root hash of SecureTrie. It does not write to the
+// Hash returns the root hash of StateTrie. It does not write to the
 // database and can be used even if the trie doesn't have one.
-func (t *SecureTrie) Hash() common.Hash {
+func (t *StateTrie) Hash() common.Hash {
 	return t.trie.Hash()
 }
 
-// Copy returns a copy of SecureTrie.
+// Copy returns a copy of StateTrie.
-func (t *SecureTrie) Copy() *SecureTrie {
+func (t *StateTrie) Copy() *StateTrie {
-	return &SecureTrie{
+	return &StateTrie{
 		trie:        *t.trie.Copy(),
 		preimages:   t.preimages,
 		secKeyCache: t.secKeyCache,
@@ -200,14 +241,14 @@ func (t *SecureTrie) Copy() *SecureTrie {
 
 // NodeIterator returns an iterator that returns nodes of the underlying trie. Iteration
 // starts at the key after the given start key.
-func (t *SecureTrie) NodeIterator(start []byte) NodeIterator {
+func (t *StateTrie) NodeIterator(start []byte) NodeIterator {
 	return t.trie.NodeIterator(start)
 }
 
 // hashKey returns the hash of key as an ephemeral buffer.
 // The caller must not hold onto the return value because it will become
 // invalid on the next call to hashKey or secKey.
-func (t *SecureTrie) hashKey(key []byte) []byte {
+func (t *StateTrie) hashKey(key []byte) []byte {
 	h := newHasher(false)
 	h.sha.Reset()
 	h.sha.Write(key)
@@ -218,8 +259,8 @@ func (t *SecureTrie) hashKey(key []byte) []byte {
 
 // getSecKeyCache returns the current secure key Cache, creating a new one if
 // ownership changed (i.e. the current secure trie is a copy of another owning
-// the actual Cache).
+// the actual cache).
-func (t *SecureTrie) getSecKeyCache() map[string][]byte {
+func (t *StateTrie) getSecKeyCache() map[string][]byte {
 	if t != t.secKeyCacheOwner {
 		t.secKeyCacheOwner = t
 		t.secKeyCache = make(map[string][]byte)

trie/secure_trie_test.go

@@ -28,16 +28,16 @@ import (
 	"github.com/XinFinOrg/XDPoSChain/ethdb/memorydb"
 )
 
-func newEmptySecure() *SecureTrie {
+func newEmptySecure() *StateTrie {
-	trie, _ := NewSecure(common.Hash{}, common.Hash{}, NewDatabase(memorydb.New()))
+	trie, _ := NewStateTrie(common.Hash{}, common.Hash{}, NewDatabase(memorydb.New()))
 	return trie
 }
 
-// makeTestSecureTrie creates a large enough secure trie for testing.
+// makeTestStateTrie creates a large enough secure trie for testing.
-func makeTestSecureTrie() (*Database, *SecureTrie, map[string][]byte) {
+func makeTestStateTrie() (*Database, *StateTrie, map[string][]byte) {
 	// Create an empty trie
 	triedb := NewDatabase(memorydb.New())
-	trie, _ := NewSecure(common.Hash{}, common.Hash{}, triedb)
+	trie, _ := NewStateTrie(common.Hash{}, common.Hash{}, triedb)
 
 	// Fill it with some arbitrary data
 	content := make(map[string][]byte)
@@ -112,12 +112,12 @@ func TestSecureGetKey(t *testing.T) {
 	}
 }
 
-func TestSecureTrieConcurrency(t *testing.T) {
+func TestStateTrieConcurrency(t *testing.T) {
 	// Create an initial trie and copy if for concurrent access
-	_, trie, _ := makeTestSecureTrie()
+	_, trie, _ := makeTestStateTrie()
 
 	threads := runtime.NumCPU()
-	tries := make([]*SecureTrie, threads)
+	tries := make([]*StateTrie, threads)
 	for i := 0; i < threads; i++ {
 		tries[i] = trie.Copy()
 	}

trie/sync_test.go

@@ -27,11 +27,11 @@ import (
 	"github.com/XinFinOrg/XDPoSChain/ethdb/memorydb"
 )
 
-// makeTestTrie create a sample test trie to test Node-wise reconstruction.
+// makeTestTrie create a sample test trie to test node-wise reconstruction.
-func makeTestTrie() (*Database, *SecureTrie, map[string][]byte) {
+func makeTestTrie() (*Database, *StateTrie, map[string][]byte) {
 	// Create an empty trie
 	triedb := NewDatabase(memorydb.New())
-	trie, _ := NewSecure(common.Hash{}, common.Hash{}, triedb)
+	trie, _ := NewStateTrie(common.Hash{}, common.Hash{}, triedb)
 
 	// Fill it with some arbitrary data
 	content := make(map[string][]byte)
@@ -68,7 +68,7 @@ func makeTestTrie() (*Database, *SecureTrie, map[string][]byte) {
 // content map.
 func checkTrieContents(t *testing.T, db *Database, root []byte, content map[string][]byte) {
 	// Check root availability and trie contents
-	trie, err := NewSecure(common.Hash{}, common.BytesToHash(root), db)
+	trie, err := NewStateTrie(common.Hash{}, common.BytesToHash(root), db)
 	if err != nil {
 		t.Fatalf("failed to create trie at %x: %v", root, err)
 	}
@@ -85,7 +85,7 @@ func checkTrieContents(t *testing.T, db *Database, root []byte, content map[stri
 // checkTrieConsistency checks that all nodes in a trie are indeed present.
 func checkTrieConsistency(db *Database, root common.Hash) error {
 	// Create and iterate a trie rooted in a subnode
-	trie, err := NewSecure(common.Hash{}, root, db)
+	trie, err := NewStateTrie(common.Hash{}, root, db)
 	if err != nil {
 		return nil // Consider a non existent state consistent
 	}

trie/trie.go

@@ -25,7 +25,6 @@ import (
 	"github.com/XinFinOrg/XDPoSChain/common"
 	"github.com/XinFinOrg/XDPoSChain/core/types"
 	"github.com/XinFinOrg/XDPoSChain/log"
-	"github.com/XinFinOrg/XDPoSChain/rlp"
 )
 
 // LeafCallback is a callback type invoked when a trie operation reaches a leaf
@@ -434,14 +433,6 @@ func (t *Trie) Update(key, value []byte) {
 	}
 }
 
-func (t *Trie) TryUpdateAccount(key []byte, acc *types.StateAccount) error {
-	data, err := rlp.EncodeToBytes(acc)
-	if err != nil {
-		return fmt.Errorf("can't encode object at %x: %w", key[:], err)
-	}
-	return t.TryUpdate(key, data)
-}
-
 // TryUpdate associates key with value in the trie. Subsequent calls to
 // Get will return value. If value has length zero, any existing value
 // is deleted from the trie and calls to Get will return nil.
@@ -451,6 +442,12 @@ func (t *Trie) TryUpdateAccount(key []byte, acc *types.StateAccount) error {
 //
 // If a Node was not found in the database, a MissingNodeError is returned.
 func (t *Trie) TryUpdate(key, value []byte) error {
+	return t.tryUpdate(key, value)
+}
+
+// tryUpdate expects an RLP-encoded value and performs the core function
+// for TryUpdate and TryUpdateAccount.
+func (t *Trie) tryUpdate(key, value []byte) error {
 	t.unhashed++
 	k := keybytesToHex(key)
 	if len(value) != 0 {