Remove empty root when possible Version 2 #2160
Conversation
dboehm-avalabs
changed the base branch from
dev
to
merkledbImproveNodeLoading
dboehm-avalabs
requested review from
abi87,
StephenButtolph,
gyuho and
hexfusion
as code owners
| rootNode *node | ||
| rootID ids.ID |
There was a problem hiding this comment.
Can we add comments for these fields?
| rootNode *node | ||
| rootID ids.ID | ||
| nodes map[Path]*change[*node] | ||
| values map[Path]*change[maybe.Maybe[[]byte]] | ||
| } | ||
|
|
||
| func newChangeSummary(estimatedSize int) *changeSummary { |
There was a problem hiding this comment.
It feels kind of weird that we don't always populate rootNode and rootID in changeSummary. It makes it hard to reason about when these are/aren't needed.
There was a problem hiding this comment.
I'm not sure what you mean. They are always populated once the changes are finalized.
x/merkledb/trieview.go
Outdated
| // If the root has no value and only a single child, that child can become the root. | ||
| // Use valueDigest instead of value because proof verification only sets the digest |
There was a problem hiding this comment.
If the root has no value and a single child, why is it t.root here? Shouldn't its child become the root in remove/insert?
There was a problem hiding this comment.
There is a tricky case where it was hard to detect in insert/remove. It was easier to catch it here.
| } | ||
|
|
||
| // the node passed in was the old root so update the parent to be the new root | ||
| if parent == nil { |
There was a problem hiding this comment.
Can we document what it means when parent == nil in a comment on this method?
| ) | ||
|
|
||
| if !key.HasPrefix(t.root.key) { |
There was a problem hiding this comment.
// Always returns at least the root node.this is no longer true
Currently the MerkleDB always has the node with the nil key as its root. This isn't required for the trie structure whenever the root has no value and only a single child. In those cases, switch to using that child as the root of the trie.
Alternative to #2106