From 4eaf6bb6be92ccf8b8e38e1776b1b588b975caf9 Mon Sep 17 00:00:00 2001 From: Quentin McGaw Date: Tue, 4 Oct 2022 11:11:00 +0000 Subject: [PATCH 1/5] fix(trie): `PopulateMerkleValues` behavior - Include Merkle value of the parent node passed as argument - Calculate missing Merkle values if needed - Add unit test --- dot/state/offline_pruner.go | 5 +- lib/trie/database.go | 41 ++++++++++++---- lib/trie/database_test.go | 95 ++++++++++++++++++++++++++++++++++++- 3 files changed, 130 insertions(+), 11 deletions(-) diff --git a/dot/state/offline_pruner.go b/dot/state/offline_pruner.go index 5a69307f7b..5c591e5483 100644 --- a/dot/state/offline_pruner.go +++ b/dot/state/offline_pruner.go @@ -121,7 +121,10 @@ func (p *OfflinePruner) SetBloomFilter() (err error) { return err } - tr.PopulateMerkleValues(tr.RootNode(), merkleValues) + err = tr.PopulateMerkleValues(tr.RootNode(), merkleValues) + if err != nil { + return fmt.Errorf("populating Merkle values from trie: %w", err) + } // get parent header of current block header, err = p.blockState.GetHeader(header.ParentHash) diff --git a/lib/trie/database.go b/lib/trie/database.go index fe908df6ec..bd0501d34c 100644 --- a/lib/trie/database.go +++ b/lib/trie/database.go @@ -187,21 +187,44 @@ func (t *Trie) loadNode(db Database, n *Node) error { // PopulateMerkleValues writes the Merkle value of each children of the node given // as keys to the map merkleValues. -func (t *Trie) PopulateMerkleValues(n *Node, merkleValues map[string]struct{}) { - if n.Kind() != node.Branch { - return +func (t *Trie) PopulateMerkleValues(n *Node, + merkleValues map[string]struct{}) (err error) { + if n == nil { + return nil } - branch := n - for _, child := range branch.Children { - if child == nil { - continue + merkleValue := n.MerkleValue + if len(merkleValue) == 0 { + // Compute and cache node Merkle value if it is absent. + if n == t.root { + merkleValue, err = n.CalculateRootMerkleValue() + if err != nil { + return fmt.Errorf("calculating Merkle value for root node: %w", err) + } + } else { + merkleValue, err = n.CalculateMerkleValue() + if err != nil { + return fmt.Errorf("calculating Merkle value for node: %w", err) + } } + } - merkleValues[string(child.MerkleValue)] = struct{}{} + merkleValues[string(merkleValue)] = struct{}{} + + if n.Kind() == node.Leaf { + return nil + } - t.PopulateMerkleValues(child, merkleValues) + branch := n + for _, child := range branch.Children { + err = t.PopulateMerkleValues(child, merkleValues) + if err != nil { + // Note: do not wrap error since this is recursive. + return err + } } + + return nil } // GetFromDB retrieves a value at the given key from the trie using the database. diff --git a/lib/trie/database_test.go b/lib/trie/database_test.go index 41fb537953..cfca9497dd 100644 --- a/lib/trie/database_test.go +++ b/lib/trie/database_test.go @@ -158,7 +158,100 @@ func Test_Trie_WriteDirty_ClearPrefix(t *testing.T) { assert.Equal(t, trie.String(), trieFromDB.String()) } -func Test_Trie_GetFromDB(t *testing.T) { +func Test_PopulateMerkleValues(t *testing.T) { + t.Parallel() + + someNode := &Node{Key: []byte{1}, SubValue: []byte{2}} + + testCases := map[string]struct { + trie *Trie + node *Node + merkleValues map[string]struct{} + errSentinel error + errMessage string + }{ + "nil node": { + trie: &Trie{}, + merkleValues: map[string]struct{}{}, + }, + "leaf node": { + trie: &Trie{}, + node: &Node{MerkleValue: []byte("a")}, + merkleValues: map[string]struct{}{ + "a": {}, + }, + }, + "leaf node without Merkle value": { + trie: &Trie{}, + node: &Node{Key: []byte{1}, SubValue: []byte{2}}, + merkleValues: map[string]struct{}{ + "A\x01\x04\x02": {}, + }, + }, + "root leaf node without Merkle value": { + trie: &Trie{ + root: someNode, + }, + node: someNode, + merkleValues: map[string]struct{}{ + "`Qm\v\xb6\xe1\xbb\xfb\x12\x93\xf1\xb2v\xea\x95\x05\xe9\xf4\xa4\xe7ُb\r\x05\x11^\v\x85'J\xe1": {}, + }, + }, + "branch node": { + trie: &Trie{}, + node: &Node{ + MerkleValue: []byte("a"), + Children: padRightChildren([]*Node{ + {MerkleValue: []byte("b")}, + }), + }, + merkleValues: map[string]struct{}{ + "a": {}, + "b": {}, + }, + }, + "nested branch node": { + trie: &Trie{}, + node: &Node{ + MerkleValue: []byte("a"), + Children: padRightChildren([]*Node{ + {MerkleValue: []byte("b")}, + { + MerkleValue: []byte("c"), + Children: padRightChildren([]*Node{ + {MerkleValue: []byte("d")}, + }), + }, + }), + }, + merkleValues: map[string]struct{}{ + "a": {}, + "b": {}, + "c": {}, + "d": {}, + }, + }, + } + + for name, testCase := range testCases { + testCase := testCase + t.Run(name, func(t *testing.T) { + t.Parallel() + + merkleValues := make(map[string]struct{}) + + err := testCase.trie.PopulateMerkleValues(testCase.node, merkleValues) + + assert.ErrorIs(t, err, testCase.errSentinel) + if testCase.errSentinel != nil { + assert.EqualError(t, err, testCase.errMessage) + } + assert.Equal(t, testCase.merkleValues, merkleValues) + }) + } +} + +func Test_GetFromDB(t *testing.T) { t.Parallel() const size = 1000 From 190201b7944586c46479e4b2d4bdf30bb2d47d01 Mon Sep 17 00:00:00 2001 From: Quentin McGaw Date: Tue, 4 Oct 2022 13:27:50 +0000 Subject: [PATCH 2/5] No Merkle value computation in function - Update comment that all Merkle values are expected to be set - Panic if merkle value is not set - Avoid leaving the map in a bad state --- dot/state/offline_pruner.go | 5 +---- lib/trie/database.go | 39 +++++++++++-------------------------- lib/trie/database_test.go | 39 +++++++++++-------------------------- 3 files changed, 23 insertions(+), 60 deletions(-) diff --git a/dot/state/offline_pruner.go b/dot/state/offline_pruner.go index 5c591e5483..72d400260b 100644 --- a/dot/state/offline_pruner.go +++ b/dot/state/offline_pruner.go @@ -121,10 +121,7 @@ func (p *OfflinePruner) SetBloomFilter() (err error) { return err } - err = tr.PopulateMerkleValues(tr.RootNode(), merkleValues) - if err != nil { - return fmt.Errorf("populating Merkle values from trie: %w", err) - } + trie.PopulateMerkleValues(tr.RootNode(), merkleValues) // get parent header of current block header, err = p.blockState.GetHeader(header.ParentHash) diff --git a/lib/trie/database.go b/lib/trie/database.go index bd0501d34c..e5c59f8897 100644 --- a/lib/trie/database.go +++ b/lib/trie/database.go @@ -185,46 +185,29 @@ func (t *Trie) loadNode(db Database, n *Node) error { return nil } -// PopulateMerkleValues writes the Merkle value of each children of the node given -// as keys to the map merkleValues. -func (t *Trie) PopulateMerkleValues(n *Node, - merkleValues map[string]struct{}) (err error) { +// PopulateMerkleValues writes the Merkle values of the node given and of +// all its descendant nodes as keys to the map merkleValues. +// It is assumed the node and its descendant nodes have their Merkle value already +// computed. +func PopulateMerkleValues(n *Node, merkleValues map[string]struct{}) { if n == nil { - return nil + return } - merkleValue := n.MerkleValue - if len(merkleValue) == 0 { - // Compute and cache node Merkle value if it is absent. - if n == t.root { - merkleValue, err = n.CalculateRootMerkleValue() - if err != nil { - return fmt.Errorf("calculating Merkle value for root node: %w", err) - } - } else { - merkleValue, err = n.CalculateMerkleValue() - if err != nil { - return fmt.Errorf("calculating Merkle value for node: %w", err) - } - } + if len(n.MerkleValue) == 0 { + panic(fmt.Sprintf("node with key 0x%x has no Merkle value computed", n.Key)) } - merkleValues[string(merkleValue)] = struct{}{} + merkleValues[string(n.MerkleValue)] = struct{}{} if n.Kind() == node.Leaf { - return nil + return } branch := n for _, child := range branch.Children { - err = t.PopulateMerkleValues(child, merkleValues) - if err != nil { - // Note: do not wrap error since this is recursive. - return err - } + PopulateMerkleValues(child, merkleValues) } - - return nil } // GetFromDB retrieves a value at the given key from the trie using the database. diff --git a/lib/trie/database_test.go b/lib/trie/database_test.go index cfca9497dd..63a2883d1a 100644 --- a/lib/trie/database_test.go +++ b/lib/trie/database_test.go @@ -161,44 +161,25 @@ func Test_Trie_WriteDirty_ClearPrefix(t *testing.T) { func Test_PopulateMerkleValues(t *testing.T) { t.Parallel() - someNode := &Node{Key: []byte{1}, SubValue: []byte{2}} - testCases := map[string]struct { - trie *Trie node *Node merkleValues map[string]struct{} - errSentinel error - errMessage string + panicValue interface{} }{ "nil node": { - trie: &Trie{}, merkleValues: map[string]struct{}{}, }, "leaf node": { - trie: &Trie{}, node: &Node{MerkleValue: []byte("a")}, merkleValues: map[string]struct{}{ "a": {}, }, }, "leaf node without Merkle value": { - trie: &Trie{}, - node: &Node{Key: []byte{1}, SubValue: []byte{2}}, - merkleValues: map[string]struct{}{ - "A\x01\x04\x02": {}, - }, - }, - "root leaf node without Merkle value": { - trie: &Trie{ - root: someNode, - }, - node: someNode, - merkleValues: map[string]struct{}{ - "`Qm\v\xb6\xe1\xbb\xfb\x12\x93\xf1\xb2v\xea\x95\x05\xe9\xf4\xa4\xe7ُb\r\x05\x11^\v\x85'J\xe1": {}, - }, + node: &Node{Key: []byte{1}, SubValue: []byte{2}}, + panicValue: "node with key 0x01 has no Merkle value computed", }, "branch node": { - trie: &Trie{}, node: &Node{ MerkleValue: []byte("a"), Children: padRightChildren([]*Node{ @@ -211,7 +192,6 @@ func Test_PopulateMerkleValues(t *testing.T) { }, }, "nested branch node": { - trie: &Trie{}, node: &Node{ MerkleValue: []byte("a"), Children: padRightChildren([]*Node{ @@ -240,12 +220,15 @@ func Test_PopulateMerkleValues(t *testing.T) { merkleValues := make(map[string]struct{}) - err := testCase.trie.PopulateMerkleValues(testCase.node, merkleValues) - - assert.ErrorIs(t, err, testCase.errSentinel) - if testCase.errSentinel != nil { - assert.EqualError(t, err, testCase.errMessage) + if testCase.panicValue != nil { + assert.PanicsWithValue(t, testCase.panicValue, func() { + PopulateMerkleValues(testCase.node, merkleValues) + }) + return } + + PopulateMerkleValues(testCase.node, merkleValues) + assert.Equal(t, testCase.merkleValues, merkleValues) }) } From 1fe78debea8fe089a6cba1aacc026625a87a1008 Mon Sep 17 00:00:00 2001 From: Quentin McGaw Date: Fri, 14 Oct 2022 11:36:20 -0400 Subject: [PATCH 3/5] Update `PopulateMerkleValues` method comment Co-authored-by: Kishan Sagathiya --- lib/trie/database.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/trie/database.go b/lib/trie/database.go index e5c59f8897..3018b8872a 100644 --- a/lib/trie/database.go +++ b/lib/trie/database.go @@ -186,7 +186,7 @@ func (t *Trie) loadNode(db Database, n *Node) error { } // PopulateMerkleValues writes the Merkle values of the node given and of -// all its descendant nodes as keys to the map merkleValues. +// all its descendant nodes as keys to the merkleValues map. // It is assumed the node and its descendant nodes have their Merkle value already // computed. func PopulateMerkleValues(n *Node, merkleValues map[string]struct{}) { From daab3501914e0a35880169ea3caf33b5ea073809 Mon Sep 17 00:00:00 2001 From: Quentin McGaw Date: Tue, 18 Oct 2022 07:23:52 +0000 Subject: [PATCH 4/5] Always ignore inlined nodes --- dot/state/offline_pruner.go | 2 +- lib/trie/database.go | 17 +++++---- lib/trie/database_test.go | 70 ++++++++++++++++++++++++------------- 3 files changed, 57 insertions(+), 32 deletions(-) diff --git a/dot/state/offline_pruner.go b/dot/state/offline_pruner.go index 72d400260b..62ceb7b86c 100644 --- a/dot/state/offline_pruner.go +++ b/dot/state/offline_pruner.go @@ -121,7 +121,7 @@ func (p *OfflinePruner) SetBloomFilter() (err error) { return err } - trie.PopulateMerkleValues(tr.RootNode(), merkleValues) + trie.PopulateNodeHashes(tr.RootNode(), merkleValues) // get parent header of current block header, err = p.blockState.GetHeader(header.ParentHash) diff --git a/lib/trie/database.go b/lib/trie/database.go index 3018b8872a..78120867a0 100644 --- a/lib/trie/database.go +++ b/lib/trie/database.go @@ -185,20 +185,25 @@ func (t *Trie) loadNode(db Database, n *Node) error { return nil } -// PopulateMerkleValues writes the Merkle values of the node given and of -// all its descendant nodes as keys to the merkleValues map. +// PopulateNodeHashes writes the node hash values of the node given and of +// all its descendant nodes as keys to the nodeHashes map. // It is assumed the node and its descendant nodes have their Merkle value already // computed. -func PopulateMerkleValues(n *Node, merkleValues map[string]struct{}) { +func PopulateNodeHashes(n *Node, nodeHashes map[string]struct{}) { if n == nil { return } - if len(n.MerkleValue) == 0 { + switch { + case len(n.MerkleValue) == 0: panic(fmt.Sprintf("node with key 0x%x has no Merkle value computed", n.Key)) + case len(n.MerkleValue) < 32: + // Inlined node where its Merkle value is its + // encoding and not the encoding hash digest. + return } - merkleValues[string(n.MerkleValue)] = struct{}{} + nodeHashes[string(n.MerkleValue)] = struct{}{} if n.Kind() == node.Leaf { return @@ -206,7 +211,7 @@ func PopulateMerkleValues(n *Node, merkleValues map[string]struct{}) { branch := n for _, child := range branch.Children { - PopulateMerkleValues(child, merkleValues) + PopulateNodeHashes(child, nodeHashes) } } diff --git a/lib/trie/database_test.go b/lib/trie/database_test.go index 63a2883d1a..341eaa2e88 100644 --- a/lib/trie/database_test.go +++ b/lib/trie/database_test.go @@ -158,57 +158,77 @@ func Test_Trie_WriteDirty_ClearPrefix(t *testing.T) { assert.Equal(t, trie.String(), trieFromDB.String()) } -func Test_PopulateMerkleValues(t *testing.T) { +func Test_PopulateNodeHashes(t *testing.T) { t.Parallel() + const ( + merkleValue32Zeroes = "00000000000000000000000000000000" + merkleValue32Ones = "11111111111111111111111111111111" + merkleValue32Twos = "22222222222222222222222222222222" + merkleValue32Threes = "33333333333333333333333333333333" + ) + testCases := map[string]struct { - node *Node - merkleValues map[string]struct{} - panicValue interface{} + node *Node + nodeHashes map[string]struct{} + panicValue interface{} }{ "nil node": { - merkleValues: map[string]struct{}{}, + nodeHashes: map[string]struct{}{}, + }, + "inlined leaf node": { + node: &Node{MerkleValue: []byte("a")}, + nodeHashes: map[string]struct{}{}, }, "leaf node": { - node: &Node{MerkleValue: []byte("a")}, - merkleValues: map[string]struct{}{ - "a": {}, + node: &Node{MerkleValue: []byte(merkleValue32Zeroes)}, + nodeHashes: map[string]struct{}{ + merkleValue32Zeroes: {}, }, }, "leaf node without Merkle value": { node: &Node{Key: []byte{1}, SubValue: []byte{2}}, panicValue: "node with key 0x01 has no Merkle value computed", }, - "branch node": { + "inlined branch node": { node: &Node{ MerkleValue: []byte("a"), Children: padRightChildren([]*Node{ {MerkleValue: []byte("b")}, }), }, - merkleValues: map[string]struct{}{ - "a": {}, - "b": {}, + nodeHashes: map[string]struct{}{}, + }, + "branch node": { + node: &Node{ + MerkleValue: []byte(merkleValue32Zeroes), + Children: padRightChildren([]*Node{ + {MerkleValue: []byte(merkleValue32Ones)}, + }), + }, + nodeHashes: map[string]struct{}{ + merkleValue32Zeroes: {}, + merkleValue32Ones: {}, }, }, "nested branch node": { node: &Node{ - MerkleValue: []byte("a"), + MerkleValue: []byte(merkleValue32Zeroes), Children: padRightChildren([]*Node{ - {MerkleValue: []byte("b")}, + {MerkleValue: []byte(merkleValue32Ones)}, { - MerkleValue: []byte("c"), + MerkleValue: []byte(merkleValue32Twos), Children: padRightChildren([]*Node{ - {MerkleValue: []byte("d")}, + {MerkleValue: []byte(merkleValue32Threes)}, }), }, }), }, - merkleValues: map[string]struct{}{ - "a": {}, - "b": {}, - "c": {}, - "d": {}, + nodeHashes: map[string]struct{}{ + merkleValue32Zeroes: {}, + merkleValue32Ones: {}, + merkleValue32Twos: {}, + merkleValue32Threes: {}, }, }, } @@ -218,18 +238,18 @@ func Test_PopulateMerkleValues(t *testing.T) { t.Run(name, func(t *testing.T) { t.Parallel() - merkleValues := make(map[string]struct{}) + nodeHashes := make(map[string]struct{}) if testCase.panicValue != nil { assert.PanicsWithValue(t, testCase.panicValue, func() { - PopulateMerkleValues(testCase.node, merkleValues) + PopulateNodeHashes(testCase.node, nodeHashes) }) return } - PopulateMerkleValues(testCase.node, merkleValues) + PopulateNodeHashes(testCase.node, nodeHashes) - assert.Equal(t, testCase.merkleValues, merkleValues) + assert.Equal(t, testCase.nodeHashes, nodeHashes) }) } } From 319543002f1bd9128b6ba10a3676ec09c8cd3cda Mon Sep 17 00:00:00 2001 From: Quentin McGaw Date: Wed, 2 Nov 2022 15:50:33 +0000 Subject: [PATCH 5/5] Add TODO comment --- lib/trie/database.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/trie/database.go b/lib/trie/database.go index 78120867a0..c2ca4bde70 100644 --- a/lib/trie/database.go +++ b/lib/trie/database.go @@ -196,6 +196,8 @@ func PopulateNodeHashes(n *Node, nodeHashes map[string]struct{}) { switch { case len(n.MerkleValue) == 0: + // TODO remove once lazy loading of nodes is implemented + // https://github.com/ChainSafe/gossamer/issues/2838 panic(fmt.Sprintf("node with key 0x%x has no Merkle value computed", n.Key)) case len(n.MerkleValue) < 32: // Inlined node where its Merkle value is its