core/state: return error when storage trie can't be opened #26350
Conversation
| 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() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
}
fjl
changed the title
fjl
changed the title
…26350) This changes the StorageTrie method to return an error when the trie is not available. It used to return an 'empty trie' in this case, but that's not possible anymore under PBSS.
This PR surfaces an error in case storage tries can't be constructed(e.g. database is corrupted).
Previously, if a storage trie is not found in database, we create an empty trie as the alternative.
This approach assumes the trie is always non-empty and finally the db error will be detected.
This PR surfaces the dbErr directly instead of creating an empty trie, since in some scenarios
creating empty trie can also fail.