From 7e12cf956c0a9a355d0dde1a378649e0c90bc2bb Mon Sep 17 00:00:00 2001 From: Gary Rong Date: Thu, 15 Dec 2022 15:59:12 +0800 Subject: [PATCH 1/4] trie: wrap deletion in case trie.root is nil --- trie/committer.go | 27 ++++++++++++-------- trie/trie.go | 8 +++++- trie/util_test.go | 64 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 87 insertions(+), 12 deletions(-) diff --git a/trie/committer.go b/trie/committer.go index 90191cf9b1dd..5f4b34fceb2f 100644 --- a/trie/committer.go +++ b/trie/committer.go @@ -56,17 +56,7 @@ func (c *committer) Commit(n node) (hashNode, *NodeSet, error) { // Some nodes can be deleted from trie which can't be captured by committer // itself. Iterate all deleted nodes tracked by tracer and marked them as // deleted only if they are present in database previously. - for _, path := range c.tracer.deleteList() { - // There are a few possibilities for this scenario(the node is deleted - // but not present in database previously), for example the node was - // embedded in the parent and now deleted from the trie. In this case - // it's noop from database's perspective. - val := c.tracer.getPrev(path) - if len(val) == 0 { - continue - } - c.nodes.markDeleted(path, val) - } + wrapDeletions(c.nodes, c.tracer) return h.(hashNode), c.nodes, nil } @@ -232,3 +222,18 @@ func estimateSize(n node) int { panic(fmt.Sprintf("node type %T", n)) } } + +// wrapDeletions puts all tracked deletions into the dirty nodeset. +func wrapDeletions(set *NodeSet, tracer *tracer) { + for _, path := range tracer.deleteList() { + // There are a few possibilities for this scenario(the node is deleted + // but not present in database previously), for example the node was + // embedded in the parent and now deleted from the trie. In this case + // it's noop from database's perspective. + val := tracer.getPrev(path) + if len(val) == 0 { + continue + } + set.markDeleted(path, val) + } +} diff --git a/trie/trie.go b/trie/trie.go index abc63f46749a..6616426d26f2 100644 --- a/trie/trie.go +++ b/trie/trie.go @@ -569,8 +569,14 @@ func (t *Trie) Hash() common.Hash { func (t *Trie) Commit(collectLeaf bool) (common.Hash, *NodeSet, error) { defer t.tracer.reset() + // Trie is empty and can be classified into two types of situations: + // - The trie was empty and no update happens + // - The trie was non-empty and all nodes are dropped if t.root == nil { - return emptyRoot, nil, nil + // Wrap tracked deletions as the return + set := NewNodeSet(t.owner) + wrapDeletions(set, t.tracer) + return emptyRoot, set, nil } // Derive the hash for all dirty nodes first. We hold the assumption // in the following procedure that all nodes are hashed. diff --git a/trie/util_test.go b/trie/util_test.go index e0e314205035..83073611a11f 100644 --- a/trie/util_test.go +++ b/trie/util_test.go @@ -242,3 +242,67 @@ func TestTrieTracePrevValue(t *testing.T) { } } } + +func TestDeleteAll(t *testing.T) { + db := NewDatabase(rawdb.NewMemoryDatabase()) + trie := NewEmpty(db) + trie.tracer = newTracer() + + // Insert a batch of entries, all the nodes should be marked as inserted + vals := []struct{ k, v string }{ + {"do", "verb"}, + {"ether", "wookiedoo"}, + {"horse", "stallion"}, + {"shaman", "horse"}, + {"doge", "coin"}, + {"dog", "puppy"}, + {"somethingveryoddindeedthis is", "myothernodedata"}, + } + for _, val := range vals { + trie.Update([]byte(val.k), []byte(val.v)) + } + root, set, err := trie.Commit(false) + if err := db.Update(NewWithNodeSet(set)); err != nil { + t.Fatal(err) + } + + // Delete entries from trie, ensure all values are detected + trie, _ = New(TrieID(root), db) + trie.tracer = newTracer() + trie.resolveAndTrack(root.Bytes(), nil) + + // Iterate all existent nodes + var ( + it = trie.NodeIterator(nil) + nodes = make(map[string][]byte) + ) + for it.Next(true) { + if it.Hash() != (common.Hash{}) { + nodes[string(it.Path())] = common.CopyBytes(it.NodeBlob()) + } + } + + // Perform deletion to purge the entire trie + for _, val := range vals { + trie.Delete([]byte(val.k)) + } + root, set, err = trie.Commit(false) + if err != nil { + t.Fatalf("Failed to delete trie %v", err) + } + if root != emptyRoot { + t.Fatalf("Invalid trie root %v", root) + } + for path, blob := range set.deletes { + prev, ok := nodes[path] + if !ok { + t.Fatalf("Extra node deleted %v", []byte(path)) + } + if !bytes.Equal(prev, blob) { + t.Fatalf("Unexpected previous value %v", []byte(path)) + } + } + if len(set.deletes) != len(nodes) { + t.Fatalf("Unexpected deletion set") + } +} From d3d9007dd4cfb1a6efbef0b9e12c080dd2e70fed Mon Sep 17 00:00:00 2001 From: Gary Rong Date: Fri, 16 Dec 2022 10:55:46 +0800 Subject: [PATCH 2/4] trie: address comments --- trie/committer.go | 17 +---------------- trie/trie.go | 2 +- trie/utils.go | 19 +++++++++++++++++++ 3 files changed, 21 insertions(+), 17 deletions(-) diff --git a/trie/committer.go b/trie/committer.go index 5f4b34fceb2f..01cee32d6340 100644 --- a/trie/committer.go +++ b/trie/committer.go @@ -56,7 +56,7 @@ func (c *committer) Commit(n node) (hashNode, *NodeSet, error) { // Some nodes can be deleted from trie which can't be captured by committer // itself. Iterate all deleted nodes tracked by tracer and marked them as // deleted only if they are present in database previously. - wrapDeletions(c.nodes, c.tracer) + c.tracer.handleDeletions(c.nodes) return h.(hashNode), c.nodes, nil } @@ -222,18 +222,3 @@ func estimateSize(n node) int { panic(fmt.Sprintf("node type %T", n)) } } - -// wrapDeletions puts all tracked deletions into the dirty nodeset. -func wrapDeletions(set *NodeSet, tracer *tracer) { - for _, path := range tracer.deleteList() { - // There are a few possibilities for this scenario(the node is deleted - // but not present in database previously), for example the node was - // embedded in the parent and now deleted from the trie. In this case - // it's noop from database's perspective. - val := tracer.getPrev(path) - if len(val) == 0 { - continue - } - set.markDeleted(path, val) - } -} diff --git a/trie/trie.go b/trie/trie.go index 6616426d26f2..cf8d6be5004b 100644 --- a/trie/trie.go +++ b/trie/trie.go @@ -575,7 +575,7 @@ func (t *Trie) Commit(collectLeaf bool) (common.Hash, *NodeSet, error) { if t.root == nil { // Wrap tracked deletions as the return set := NewNodeSet(t.owner) - wrapDeletions(set, t.tracer) + t.tracer.handleDeletions(set) return emptyRoot, set, nil } // Derive the hash for all dirty nodes first. We hold the assumption diff --git a/trie/utils.go b/trie/utils.go index d462b31bd205..90564c4f23b7 100644 --- a/trie/utils.go +++ b/trie/utils.go @@ -178,3 +178,22 @@ func (t *tracer) copy() *tracer { origin: origin, } } + +// handleDeletions puts all tracked deletions into the provided nodeset. +func (t *tracer) handleDeletions(set *NodeSet) { + // Tracer isn't used right now, remove this check later. + if t == nil { + return + } + for _, path := range t.deleteList() { + // There are a few possibilities for this scenario(the node is deleted + // but not present in database previously), for example the node was + // embedded in the parent and now deleted from the trie. In this case + // it's noop from database's perspective. + val := t.getPrev(path) + if len(val) == 0 { + continue + } + set.markDeleted(path, val) + } +} From 4138d1d16b8de23e19c2831b55fed302df93e81a Mon Sep 17 00:00:00 2001 From: Gary Rong Date: Fri, 16 Dec 2022 15:53:17 +0800 Subject: [PATCH 3/4] trie: fix lint --- trie/util_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/trie/util_test.go b/trie/util_test.go index 83073611a11f..d0f8f94f377c 100644 --- a/trie/util_test.go +++ b/trie/util_test.go @@ -262,10 +262,12 @@ func TestDeleteAll(t *testing.T) { trie.Update([]byte(val.k), []byte(val.v)) } root, set, err := trie.Commit(false) + if err != nil { + t.Fatal(err) + } if err := db.Update(NewWithNodeSet(set)); err != nil { t.Fatal(err) } - // Delete entries from trie, ensure all values are detected trie, _ = New(TrieID(root), db) trie.tracer = newTracer() From 2d61635a5239fb28f441c5c70e3b88ba86b37aaa Mon Sep 17 00:00:00 2001 From: Gary Rong Date: Fri, 16 Dec 2022 20:30:52 +0800 Subject: [PATCH 4/4] trie: update --- trie/committer.go | 9 +++++---- trie/trie.go | 2 +- trie/utils.go | 4 ++-- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/trie/committer.go b/trie/committer.go index 01cee32d6340..13c54d96980a 100644 --- a/trie/committer.go +++ b/trie/committer.go @@ -53,10 +53,11 @@ func (c *committer) Commit(n node) (hashNode, *NodeSet, error) { if err != nil { return nil, nil, err } - // Some nodes can be deleted from trie which can't be captured by committer - // itself. Iterate all deleted nodes tracked by tracer and marked them as - // deleted only if they are present in database previously. - c.tracer.handleDeletions(c.nodes) + // Some nodes can be deleted from trie which can't be captured + // by committer itself. Iterate all deleted nodes tracked by + // tracer and marked them as deleted only if they are present + // in database previously. + c.tracer.markDeletions(c.nodes) return h.(hashNode), c.nodes, nil } diff --git a/trie/trie.go b/trie/trie.go index cf8d6be5004b..1a456b8cfd56 100644 --- a/trie/trie.go +++ b/trie/trie.go @@ -575,7 +575,7 @@ func (t *Trie) Commit(collectLeaf bool) (common.Hash, *NodeSet, error) { if t.root == nil { // Wrap tracked deletions as the return set := NewNodeSet(t.owner) - t.tracer.handleDeletions(set) + t.tracer.markDeletions(set) return emptyRoot, set, nil } // Derive the hash for all dirty nodes first. We hold the assumption diff --git a/trie/utils.go b/trie/utils.go index 90564c4f23b7..5dce65cd2971 100644 --- a/trie/utils.go +++ b/trie/utils.go @@ -179,8 +179,8 @@ func (t *tracer) copy() *tracer { } } -// handleDeletions puts all tracked deletions into the provided nodeset. -func (t *tracer) handleDeletions(set *NodeSet) { +// markDeletions puts all tracked deletions into the provided nodeset. +func (t *tracer) markDeletions(set *NodeSet) { // Tracer isn't used right now, remove this check later. if t == nil { return