Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion core/state/dump.go
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,12 @@ func (s *StateDB) DumpToCollector(c DumpCollector, conf *DumpConfig) (nextKey []
}
if !conf.SkipStorage {
account.Storage = make(map[common.Hash]string)
storageIt := trie.NewIterator(obj.getTrie(s.db).NodeIterator(nil))
tr, err := obj.getTrie(s.db)
if err != nil {
log.Error("Failed to load storage trie", "err", err)
continue
}
storageIt := trie.NewIterator(tr.NodeIterator(nil))
for storageIt.Next() {
_, content, _, err := rlp.Split(storageIt.Value)
if err != nil {
Expand Down
77 changes: 53 additions & 24 deletions core/state/state_object.go
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,10 @@ func (s *stateObject) touch() {
}
}

func (s *stateObject) getTrie(db Database) Trie {
// getTrie returns the associated storage trie. The trie will be opened
// if it's not loaded previously. An error will be returned if trie can't
// be loaded.
func (s *stateObject) getTrie(db Database) (Trie, error) {
if s.trie == nil {
// Try fetching from prefetcher first
// We don't prefetch empty tries
Expand All @@ -158,15 +161,14 @@ func (s *stateObject) getTrie(db Database) Trie {
s.trie = s.db.prefetcher.trie(s.addrHash, s.data.Root)
}
if s.trie == nil {
var err error
s.trie, err = db.OpenStorageTrie(s.db.originalRoot, s.addrHash, s.data.Root)
tr, err := db.OpenStorageTrie(s.db.originalRoot, s.addrHash, s.data.Root)
if err != nil {
s.trie, _ = db.OpenStorageTrie(s.db.originalRoot, s.addrHash, common.Hash{})
s.setError(fmt.Errorf("can't create storage trie: %v", err))
return nil, err
}
s.trie = tr
}
}
return s.trie
return s.trie, nil
}

// GetState retrieves a value from the account storage trie.
Expand Down Expand Up @@ -221,7 +223,12 @@ func (s *stateObject) GetCommittedState(db Database, key common.Hash) common.Has
// If the snapshot is unavailable or reading from it fails, load from the database.
if s.db.snap == nil || err != nil {
start := time.Now()
enc, err = s.getTrie(db).TryGet(key.Bytes())
tr, err := s.getTrie(db)
if err != nil {
s.setError(err)
return common.Hash{}
}
enc, err = tr.TryGet(key.Bytes())
if metrics.EnabledExpensive {
s.db.StorageReads += time.Since(start)
}
Expand Down Expand Up @@ -304,23 +311,29 @@ func (s *stateObject) finalise(prefetch bool) {
}

// updateTrie writes cached storage modifications into the object's storage trie.
// It will return nil if the trie has not been loaded and no changes have been made
func (s *stateObject) updateTrie(db Database) Trie {
// It will return nil if the trie has not been loaded and no changes have been
// made. An error will be returned if the trie can't be loaded/updated correctly.
func (s *stateObject) updateTrie(db Database) (Trie, error) {
// Make sure all dirty slots are finalized into the pending storage area
s.finalise(false) // Don't prefetch anymore, pull directly if need be
if len(s.pendingStorage) == 0 {
return s.trie
return s.trie, nil
}
// Track the amount of time wasted on updating the storage trie
if metrics.EnabledExpensive {
defer func(start time.Time) { s.db.StorageUpdates += time.Since(start) }(time.Now())
}
// The snapshot storage map for the object
var storage map[common.Hash][]byte
var (
storage map[common.Hash][]byte
hasher = s.db.hasher
)
tr, err := s.getTrie(db)
if err != nil {
s.setError(err)
return nil, err
}
// Insert all the pending updates into the trie
tr := s.getTrie(db)
hasher := s.db.hasher

usedStorage := make([][]byte, 0, len(s.pendingStorage))
for key, value := range s.pendingStorage {
// Skip noop changes, persist actual changes
Expand All @@ -331,12 +344,18 @@ func (s *stateObject) updateTrie(db Database) Trie {

var v []byte
if (value == common.Hash{}) {
s.setError(tr.TryDelete(key[:]))
if err := tr.TryDelete(key[:]); err != nil {
s.setError(err)
return nil, err
}
s.db.StorageDeleted += 1
} else {
// Encoding []byte cannot fail, ok to ignore the error.
v, _ = rlp.EncodeToBytes(common.TrimLeftZeroes(value[:]))
s.setError(tr.TryUpdate(key[:], v))
if err := tr.TryUpdate(key[:], v); err != nil {
s.setError(err)
return nil, err
}
s.db.StorageUpdated += 1
}
// If state snapshotting is active, cache the data til commit
Expand All @@ -358,37 +377,47 @@ func (s *stateObject) updateTrie(db Database) Trie {
if len(s.pendingStorage) > 0 {
s.pendingStorage = make(Storage)
}
return tr
return tr, nil
}

// UpdateRoot sets the trie root to the current root hash of
// UpdateRoot sets the trie root to the current root hash of. An error
// will be returned if trie root hash is not computed correctly.
func (s *stateObject) updateRoot(db Database) {
tr, err := s.updateTrie(db)
if err != nil {
s.setError(fmt.Errorf("updateRoot (%x) error: %w", s.address, err))
return
}
// If nothing changed, don't bother with hashing anything
if s.updateTrie(db) == nil {
if tr == nil {
return
}
// Track the amount of time wasted on hashing the storage trie
if metrics.EnabledExpensive {
defer func(start time.Time) { s.db.StorageHashes += time.Since(start) }(time.Now())
}
s.data.Root = s.trie.Hash()
s.data.Root = tr.Hash()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an interesting change. One one hand, it's somewhat logical, less data access. On the other hand, previously we implicitly checked if s.trie was actually set to the newly created/update/returned tr. With the change, now we do not check this side effect.

On the other hand, the original code has no guarantee either that s.trie and tr is actually the same.

Soo, I guess my question is, is there a specific reason for updateTrie returning the Trie, instead of say a boolean flag to signal no update (the current nil return). If we were to change the update to just return a flag, there would be no duality of two potentially conflicting ways to access the - hopefully - same trie.

Copy link
Member Author

@rjl493456442 rjl493456442 Dec 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will keep the current behavior.

StateDB is really complicated :)

In commitTrie commit operation will be skipped if trie is nil. Commit only happens at the end of block(post-byzantium),
it can happen the last tx in block doesn't touch this storage trie, but this storage trie is indeed dirty.

If we simply use a boolean, it will lead data be missing. tr == nil magically represents this trie is not updated in the entire block :)

I agree we need to improve it. Give me a bit more time to figure out a correct solution.

// commitTrie submits the storage changes into the storage trie and re-computes
// the root. Besides, all trie changes will be collected in a nodeset and returned.
func (s *stateObject) commitTrie(db Database) (*trie.NodeSet, error) {
	tr, err := s.updateTrie(db)
	...
	// If nothing changed, don't bother with committing anything
	if tr == nil {
		return nil, nil
	}
	root, nodes, err := tr.Commit(false)
	if err == nil {
		s.data.Root = root
	}
	return nodes, err
}

}

// commitTrie submits the storage changes into the storage trie and re-computes
// the root. Besides, all trie changes will be collected in a nodeset and returned.
func (s *stateObject) commitTrie(db Database) (*trie.NodeSet, error) {
// If nothing changed, don't bother with hashing anything
if s.updateTrie(db) == nil {
return nil, nil
tr, err := s.updateTrie(db)
if err != nil {
return nil, err
}
if s.dbErr != nil {
return nil, s.dbErr
}
// If nothing changed, don't bother with committing anything
if tr == nil {
return nil, nil
}
// Track the amount of time wasted on committing the storage trie
if metrics.EnabledExpensive {
defer func(start time.Time) { s.db.StorageCommits += time.Since(start) }(time.Now())
}
root, nodes, err := s.trie.Commit(false)
root, nodes, err := tr.Commit(false)
if err == nil {
s.data.Root = root
}
Expand Down
35 changes: 24 additions & 11 deletions core/state/statedb.go
Original file line number Diff line number Diff line change
Expand Up @@ -336,13 +336,19 @@ func (s *StateDB) GetProofByHash(addrHash common.Hash) ([][]byte, error) {

// GetStorageProof returns the Merkle proof for given storage slot.
func (s *StateDB) GetStorageProof(a common.Address, key common.Hash) ([][]byte, error) {
var proof proofList
trie := s.StorageTrie(a)
trie, err := s.StorageTrie(a)
if err != nil {
return nil, err
}
if trie == nil {
return proof, errors.New("storage trie for requested address does not exist")
return nil, errors.New("storage trie for requested address does not exist")
}
err := trie.Prove(crypto.Keccak256(key.Bytes()), 0, &proof)
return proof, err
var proof proofList
err = trie.Prove(crypto.Keccak256(key.Bytes()), 0, &proof)
if err != nil {
return nil, err
}
return proof, nil
}

// GetCommittedState retrieves a value from the given account's committed storage trie.
Expand All @@ -359,15 +365,18 @@ func (s *StateDB) Database() Database {
return s.db
}

// StorageTrie returns the storage trie of an account.
// The return value is a copy and is nil for non-existent accounts.
func (s *StateDB) StorageTrie(addr common.Address) Trie {
// StorageTrie returns the storage trie of an account. The return value is a copy
// and is nil for non-existent accounts. An error will be returned if storage trie
// is existent but can't be loaded correctly.
func (s *StateDB) StorageTrie(addr common.Address) (Trie, error) {
stateObject := s.getStateObject(addr)
if stateObject == nil {
return nil
return nil, nil
}
cpy := stateObject.deepCopy(s)
cpy.updateTrie(s.db)
if _, err := cpy.updateTrie(s.db); err != nil {
return nil, err
}
return cpy.getTrie(s.db)
}

Expand Down Expand Up @@ -651,7 +660,11 @@ func (db *StateDB) ForEachStorage(addr common.Address, cb func(key, value common
if so == nil {
return nil
}
it := trie.NewIterator(so.getTrie(db.db).NodeIterator(nil))
tr, err := so.getTrie(db.db)
if err != nil {
return err
}
it := trie.NewIterator(tr.NodeIterator(nil))

for it.Next() {
key := common.BytesToHash(db.trie.GetKey(it.Key))
Expand Down
5 changes: 4 additions & 1 deletion eth/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -417,7 +417,10 @@ func (api *DebugAPI) StorageRangeAt(blockHash common.Hash, txIndex int, contract
}
defer release()

st := statedb.StorageTrie(contractAddress)
st, err := statedb.StorageTrie(contractAddress)
if err != nil {
return StorageRangeResult{}, err
}
if st == nil {
return StorageRangeResult{}, fmt.Errorf("account %x doesn't exist", contractAddress)
}
Expand Down
6 changes: 5 additions & 1 deletion eth/api_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,11 @@ func TestStorageRangeAt(t *testing.T) {
},
}
for _, test := range tests {
result, err := storageRangeAt(state.StorageTrie(addr), test.start, test.limit)
tr, err := state.StorageTrie(addr)
if err != nil {
t.Error(err)
}
result, err := storageRangeAt(tr, test.start, test.limit)
if err != nil {
t.Error(err)
}
Expand Down
6 changes: 4 additions & 2 deletions internal/ethapi/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -660,8 +660,10 @@ func (s *BlockChainAPI) GetProof(ctx context.Context, address common.Address, st
if state == nil || err != nil {
return nil, err
}

storageTrie := state.StorageTrie(address)
storageTrie, err := state.StorageTrie(address)
if err != nil {
return nil, err
}
storageHash := types.EmptyRootHash
codeHash := state.GetCodeHash(address)
storageProof := make([]StorageResult, len(storageKeys))
Expand Down