From 27f02ef62f413d0d070096747b37f91915b31723 Mon Sep 17 00:00:00 2001 From: Quentin McGaw Date: Tue, 30 Nov 2021 11:07:43 +0000 Subject: [PATCH 1/4] `lib/trie/recorder` subpackage --- lib/trie/lookup.go | 13 ++- lib/trie/proof.go | 14 ++- lib/trie/record/node.go | 7 ++ lib/trie/record/recorder.go | 38 ++++++++ lib/trie/record/recorder_test.go | 157 +++++++++++++++++++++++++++++++ lib/trie/recorder.go | 34 ------- 6 files changed, 218 insertions(+), 45 deletions(-) create mode 100644 lib/trie/record/node.go create mode 100644 lib/trie/record/recorder.go create mode 100644 lib/trie/record/recorder_test.go delete mode 100644 lib/trie/recorder.go diff --git a/lib/trie/lookup.go b/lib/trie/lookup.go index 4df79c6c33..e51596dc69 100644 --- a/lib/trie/lookup.go +++ b/lib/trie/lookup.go @@ -8,20 +8,27 @@ import ( "github.com/ChainSafe/gossamer/lib/trie/branch" "github.com/ChainSafe/gossamer/lib/trie/node" + "github.com/ChainSafe/gossamer/lib/trie/record" ) +var _ recorder = (*record.Recorder)(nil) + +type recorder interface { + Record(hash, rawData []byte) +} + // findAndRecord search for a desired key recording all the nodes in the path including the desired node -func findAndRecord(t *Trie, key []byte, recorder *recorder) error { +func findAndRecord(t *Trie, key []byte, recorder recorder) error { return find(t.root, key, recorder) } -func find(parent node.Node, key []byte, recorder *recorder) error { +func find(parent node.Node, key []byte, recorder recorder) error { enc, hash, err := parent.EncodeAndHash() if err != nil { return err } - recorder.record(hash, enc) + recorder.Record(hash, enc) b, ok := parent.(*branch.Branch) if !ok { diff --git a/lib/trie/proof.go b/lib/trie/proof.go index 12c1d382a0..de06cbb27f 100644 --- a/lib/trie/proof.go +++ b/lib/trie/proof.go @@ -1,6 +1,3 @@ -// Copyright 2021 ChainSafe Systems (ON) -// SPDX-License-Identifier: LGPL-3.0-only - package trie import ( @@ -12,6 +9,7 @@ import ( "github.com/ChainSafe/chaindb" "github.com/ChainSafe/gossamer/lib/common" "github.com/ChainSafe/gossamer/lib/trie/decode" + "github.com/ChainSafe/gossamer/lib/trie/record" ) var ( @@ -43,17 +41,17 @@ func GenerateProof(root []byte, keys [][]byte, db chaindb.Database) ([][]byte, e for _, k := range keys { nk := decode.KeyLEToNibbles(k) - recorder := new(recorder) + recorder := record.NewRecorder() err := findAndRecord(proofTrie, nk, recorder) if err != nil { return nil, err } - for !recorder.isEmpty() { - recNode := recorder.next() - nodeHashHex := common.BytesToHex(recNode.hash) + for !recorder.IsEmpty() { + recNode := recorder.Next() + nodeHashHex := common.BytesToHex(recNode.Hash) if _, ok := trackedProofs[nodeHashHex]; !ok { - trackedProofs[nodeHashHex] = recNode.rawData + trackedProofs[nodeHashHex] = recNode.RawData } } } diff --git a/lib/trie/record/node.go b/lib/trie/record/node.go new file mode 100644 index 0000000000..eb3299e9bc --- /dev/null +++ b/lib/trie/record/node.go @@ -0,0 +1,7 @@ +package record + +// Node represents a record of a visited node +type Node struct { + RawData []byte + Hash []byte +} diff --git a/lib/trie/record/recorder.go b/lib/trie/record/recorder.go new file mode 100644 index 0000000000..6651332cee --- /dev/null +++ b/lib/trie/record/recorder.go @@ -0,0 +1,38 @@ +// Copyright 2021 ChainSafe Systems (ON) +// SPDX-License-Identifier: LGPL-3.0-only + +package record + +// Recorder records the list of nodes found by Lookup.Find +type Recorder struct { + nodes []Node +} + +// NewRecorder creates a new recorder. +func NewRecorder() *Recorder { + return &Recorder{} +} + +// Record appends a node to the list of visited nodes. +func (r *Recorder) Record(hash, rawData []byte) { + r.nodes = append(r.nodes, Node{RawData: rawData, Hash: hash}) +} + +// Next returns the first node in the recorded list +// and removes it (shift operation). +func (r *Recorder) Next() (node *Node) { + if len(r.nodes) == 0 { + return nil + } + + node = &r.nodes[0] + r.nodes = r.nodes[1:] + + return node +} + +// IsEmpty returns true if no node is in in the current +// recorded list of nodes. +func (r *Recorder) IsEmpty() (empty bool) { + return len(r.nodes) == 0 +} diff --git a/lib/trie/record/recorder_test.go b/lib/trie/record/recorder_test.go new file mode 100644 index 0000000000..dd928816b5 --- /dev/null +++ b/lib/trie/record/recorder_test.go @@ -0,0 +1,157 @@ +package record + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func Test_NewRecorder(t *testing.T) { + t.Parallel() + + expected := &Recorder{} + + recorder := NewRecorder() + + assert.Equal(t, expected, recorder) +} + +func Test_Recorder_Record(t *testing.T) { + testCases := map[string]struct { + recorder *Recorder + hash []byte + rawData []byte + expectedRecorder *Recorder + }{ + "nil data": { + recorder: &Recorder{}, + expectedRecorder: &Recorder{ + nodes: []Node{ + {}, + }, + }, + }, + "insert in empty recorder": { + recorder: &Recorder{}, + hash: []byte{1, 2}, + rawData: []byte{3, 4}, + expectedRecorder: &Recorder{ + nodes: []Node{ + {Hash: []byte{1, 2}, RawData: []byte{3, 4}}, + }, + }, + }, + "insert in non-empty recorder": { + recorder: &Recorder{ + nodes: []Node{ + {Hash: []byte{5, 6}, RawData: []byte{7, 8}}, + }, + }, + hash: []byte{1, 2}, + rawData: []byte{3, 4}, + expectedRecorder: &Recorder{ + nodes: []Node{ + {Hash: []byte{5, 6}, RawData: []byte{7, 8}}, + {Hash: []byte{1, 2}, RawData: []byte{3, 4}}, + }, + }, + }, + } + + for name, testCase := range testCases { + testCase := testCase + t.Run(name, func(t *testing.T) { + t.Parallel() + + testCase.recorder.Record(testCase.hash, testCase.rawData) + + assert.Equal(t, testCase.expectedRecorder, testCase.recorder) + }) + } +} + +func Test_Recorder_Next(t *testing.T) { + testCases := map[string]struct { + recorder *Recorder + node *Node + expectedRecorder *Recorder + }{ + "no node": { + recorder: &Recorder{}, + expectedRecorder: &Recorder{}, + }, + "get single node from recorder": { + recorder: &Recorder{ + nodes: []Node{ + {Hash: []byte{1, 2}, RawData: []byte{3, 4}}, + }, + }, + node: &Node{Hash: []byte{1, 2}, RawData: []byte{3, 4}}, + expectedRecorder: &Recorder{ + nodes: []Node{}, + }, + }, + "get node from multiple nodes in recorder": { + recorder: &Recorder{ + nodes: []Node{ + {Hash: []byte{1, 2}, RawData: []byte{3, 4}}, + {Hash: []byte{5, 6}, RawData: []byte{7, 8}}, + {Hash: []byte{9, 6}, RawData: []byte{7, 8}}, + }, + }, + node: &Node{Hash: []byte{1, 2}, RawData: []byte{3, 4}}, + expectedRecorder: &Recorder{ + nodes: []Node{ + {Hash: []byte{5, 6}, RawData: []byte{7, 8}}, + {Hash: []byte{9, 6}, RawData: []byte{7, 8}}, + }, + }, + }, + } + + for name, testCase := range testCases { + testCase := testCase + t.Run(name, func(t *testing.T) { + t.Parallel() + + node := testCase.recorder.Next() + + assert.Equal(t, testCase.node, node) + assert.Equal(t, testCase.expectedRecorder, testCase.recorder) + }) + } +} + +func Test_Recorder_IsEmpty(t *testing.T) { + testCases := map[string]struct { + recorder *Recorder + empty bool + }{ + "nil nodes": { + recorder: &Recorder{}, + empty: true, + }, + "empty nodes": { + recorder: &Recorder{ + nodes: []Node{}, + }, + empty: true, + }, + "non-empty nodes": { + recorder: &Recorder{ + nodes: []Node{{}}, + }, + }, + } + + for name, testCase := range testCases { + testCase := testCase + t.Run(name, func(t *testing.T) { + t.Parallel() + + empty := testCase.recorder.IsEmpty() + + assert.Equal(t, testCase.empty, empty) + }) + } +} diff --git a/lib/trie/recorder.go b/lib/trie/recorder.go deleted file mode 100644 index 6db2a841d0..0000000000 --- a/lib/trie/recorder.go +++ /dev/null @@ -1,34 +0,0 @@ -// Copyright 2021 ChainSafe Systems (ON) -// SPDX-License-Identifier: LGPL-3.0-only - -package trie - -// nodeRecord represets a record of a visited node -type nodeRecord struct { - rawData []byte - hash []byte -} - -// recorder keeps the list of nodes find by Lookup.Find -type recorder []nodeRecord - -// record insert a node inside the recorded list -func (r *recorder) record(h, rd []byte) { - *r = append(*r, nodeRecord{rawData: rd, hash: h}) -} - -// next returns the current item the cursor is on and increment the cursor by 1 -func (r *recorder) next() *nodeRecord { - if !r.isEmpty() { - n := (*r)[0] - *r = (*r)[1:] - return &n - } - - return nil -} - -// isEmpty returns bool if there is data inside the slice -func (r *recorder) isEmpty() bool { - return len(*r) <= 0 -} From c629237201b84d9b1283933a6a0350298b87fadc Mon Sep 17 00:00:00 2001 From: Quentin McGaw Date: Tue, 30 Nov 2021 16:56:17 +0000 Subject: [PATCH 2/4] return an error on a call to Next() with no node --- lib/trie/proof.go | 5 ++++- lib/trie/record/recorder.go | 14 ++++++++++---- lib/trie/record/recorder_test.go | 14 ++++++++++---- 3 files changed, 24 insertions(+), 9 deletions(-) diff --git a/lib/trie/proof.go b/lib/trie/proof.go index de06cbb27f..46c22c1006 100644 --- a/lib/trie/proof.go +++ b/lib/trie/proof.go @@ -48,7 +48,10 @@ func GenerateProof(root []byte, keys [][]byte, db chaindb.Database) ([][]byte, e } for !recorder.IsEmpty() { - recNode := recorder.Next() + recNode, err := recorder.Next() + if err != nil { + return nil, fmt.Errorf("recorder failed for key 0x%x: %w", k, err) + } nodeHashHex := common.BytesToHex(recNode.Hash) if _, ok := trackedProofs[nodeHashHex]; !ok { trackedProofs[nodeHashHex] = recNode.RawData diff --git a/lib/trie/record/recorder.go b/lib/trie/record/recorder.go index 6651332cee..b086ea6065 100644 --- a/lib/trie/record/recorder.go +++ b/lib/trie/record/recorder.go @@ -3,6 +3,12 @@ package record +import "errors" + +var ( + ErrNoNextNode = errors.New("no next node") +) + // Recorder records the list of nodes found by Lookup.Find type Recorder struct { nodes []Node @@ -20,15 +26,15 @@ func (r *Recorder) Record(hash, rawData []byte) { // Next returns the first node in the recorded list // and removes it (shift operation). -func (r *Recorder) Next() (node *Node) { +func (r *Recorder) Next() (node Node, err error) { if len(r.nodes) == 0 { - return nil + return node, ErrNoNextNode } - node = &r.nodes[0] + node = r.nodes[0] r.nodes = r.nodes[1:] - return node + return node, nil } // IsEmpty returns true if no node is in in the current diff --git a/lib/trie/record/recorder_test.go b/lib/trie/record/recorder_test.go index dd928816b5..1c7e35983a 100644 --- a/lib/trie/record/recorder_test.go +++ b/lib/trie/record/recorder_test.go @@ -73,11 +73,13 @@ func Test_Recorder_Record(t *testing.T) { func Test_Recorder_Next(t *testing.T) { testCases := map[string]struct { recorder *Recorder - node *Node + node Node + err error expectedRecorder *Recorder }{ "no node": { recorder: &Recorder{}, + err: ErrNoNextNode, expectedRecorder: &Recorder{}, }, "get single node from recorder": { @@ -86,7 +88,7 @@ func Test_Recorder_Next(t *testing.T) { {Hash: []byte{1, 2}, RawData: []byte{3, 4}}, }, }, - node: &Node{Hash: []byte{1, 2}, RawData: []byte{3, 4}}, + node: Node{Hash: []byte{1, 2}, RawData: []byte{3, 4}}, expectedRecorder: &Recorder{ nodes: []Node{}, }, @@ -99,7 +101,7 @@ func Test_Recorder_Next(t *testing.T) { {Hash: []byte{9, 6}, RawData: []byte{7, 8}}, }, }, - node: &Node{Hash: []byte{1, 2}, RawData: []byte{3, 4}}, + node: Node{Hash: []byte{1, 2}, RawData: []byte{3, 4}}, expectedRecorder: &Recorder{ nodes: []Node{ {Hash: []byte{5, 6}, RawData: []byte{7, 8}}, @@ -114,8 +116,12 @@ func Test_Recorder_Next(t *testing.T) { t.Run(name, func(t *testing.T) { t.Parallel() - node := testCase.recorder.Next() + node, err := testCase.recorder.Next() + assert.ErrorIs(t, err, testCase.err) + if testCase.err != nil { + assert.EqualError(t, err, testCase.err.Error()) + } assert.Equal(t, testCase.node, node) assert.Equal(t, testCase.expectedRecorder, testCase.recorder) }) From 4078ae1faf6b7df866ae8c946d549fd0176768d7 Mon Sep 17 00:00:00 2001 From: Quentin McGaw Date: Tue, 30 Nov 2021 17:00:17 +0000 Subject: [PATCH 3/4] remove recorder `IsEmpty` method --- lib/trie/proof.go | 6 ++++-- lib/trie/record/recorder.go | 6 ------ lib/trie/record/recorder_test.go | 34 -------------------------------- 3 files changed, 4 insertions(+), 42 deletions(-) diff --git a/lib/trie/proof.go b/lib/trie/proof.go index 46c22c1006..eccae4ce11 100644 --- a/lib/trie/proof.go +++ b/lib/trie/proof.go @@ -47,9 +47,11 @@ func GenerateProof(root []byte, keys [][]byte, db chaindb.Database) ([][]byte, e return nil, err } - for !recorder.IsEmpty() { + for { recNode, err := recorder.Next() - if err != nil { + if errors.Is(err, record.ErrNoNextNode) { + break + } else if err != nil { return nil, fmt.Errorf("recorder failed for key 0x%x: %w", k, err) } nodeHashHex := common.BytesToHex(recNode.Hash) diff --git a/lib/trie/record/recorder.go b/lib/trie/record/recorder.go index b086ea6065..4281bd4dd2 100644 --- a/lib/trie/record/recorder.go +++ b/lib/trie/record/recorder.go @@ -36,9 +36,3 @@ func (r *Recorder) Next() (node Node, err error) { return node, nil } - -// IsEmpty returns true if no node is in in the current -// recorded list of nodes. -func (r *Recorder) IsEmpty() (empty bool) { - return len(r.nodes) == 0 -} diff --git a/lib/trie/record/recorder_test.go b/lib/trie/record/recorder_test.go index 1c7e35983a..624889c545 100644 --- a/lib/trie/record/recorder_test.go +++ b/lib/trie/record/recorder_test.go @@ -127,37 +127,3 @@ func Test_Recorder_Next(t *testing.T) { }) } } - -func Test_Recorder_IsEmpty(t *testing.T) { - testCases := map[string]struct { - recorder *Recorder - empty bool - }{ - "nil nodes": { - recorder: &Recorder{}, - empty: true, - }, - "empty nodes": { - recorder: &Recorder{ - nodes: []Node{}, - }, - empty: true, - }, - "non-empty nodes": { - recorder: &Recorder{ - nodes: []Node{{}}, - }, - }, - } - - for name, testCase := range testCases { - testCase := testCase - t.Run(name, func(t *testing.T) { - t.Parallel() - - empty := testCase.recorder.IsEmpty() - - assert.Equal(t, testCase.empty, empty) - }) - } -} From ba6cc4010f8c61c535cadcfca7ee50715084dfaa Mon Sep 17 00:00:00 2001 From: Quentin McGaw Date: Tue, 30 Nov 2021 17:06:05 +0000 Subject: [PATCH 4/4] Recorder `GetNodes()` --- lib/trie/proof.go | 8 +------ lib/trie/record/recorder.go | 23 ++++++-------------- lib/trie/record/recorder_test.go | 36 ++++++++++---------------------- 3 files changed, 18 insertions(+), 49 deletions(-) diff --git a/lib/trie/proof.go b/lib/trie/proof.go index eccae4ce11..094fe48f7e 100644 --- a/lib/trie/proof.go +++ b/lib/trie/proof.go @@ -47,13 +47,7 @@ func GenerateProof(root []byte, keys [][]byte, db chaindb.Database) ([][]byte, e return nil, err } - for { - recNode, err := recorder.Next() - if errors.Is(err, record.ErrNoNextNode) { - break - } else if err != nil { - return nil, fmt.Errorf("recorder failed for key 0x%x: %w", k, err) - } + for _, recNode := range recorder.GetNodes() { nodeHashHex := common.BytesToHex(recNode.Hash) if _, ok := trackedProofs[nodeHashHex]; !ok { trackedProofs[nodeHashHex] = recNode.RawData diff --git a/lib/trie/record/recorder.go b/lib/trie/record/recorder.go index 4281bd4dd2..130b434338 100644 --- a/lib/trie/record/recorder.go +++ b/lib/trie/record/recorder.go @@ -3,12 +3,6 @@ package record -import "errors" - -var ( - ErrNoNextNode = errors.New("no next node") -) - // Recorder records the list of nodes found by Lookup.Find type Recorder struct { nodes []Node @@ -24,15 +18,10 @@ func (r *Recorder) Record(hash, rawData []byte) { r.nodes = append(r.nodes, Node{RawData: rawData, Hash: hash}) } -// Next returns the first node in the recorded list -// and removes it (shift operation). -func (r *Recorder) Next() (node Node, err error) { - if len(r.nodes) == 0 { - return node, ErrNoNextNode - } - - node = r.nodes[0] - r.nodes = r.nodes[1:] - - return node, nil +// GetNodes returns all the nodes recorded. +// Note it does not copy its slice of nodes. +// It's fine to not copy them since the recorder +// is not used again after a call to GetNodes() +func (r *Recorder) GetNodes() (nodes []Node) { + return r.nodes } diff --git a/lib/trie/record/recorder_test.go b/lib/trie/record/recorder_test.go index 624889c545..638661b97a 100644 --- a/lib/trie/record/recorder_test.go +++ b/lib/trie/record/recorder_test.go @@ -70,17 +70,13 @@ func Test_Recorder_Record(t *testing.T) { } } -func Test_Recorder_Next(t *testing.T) { +func Test_Recorder_GetNodes(t *testing.T) { testCases := map[string]struct { - recorder *Recorder - node Node - err error - expectedRecorder *Recorder + recorder *Recorder + nodes []Node }{ "no node": { - recorder: &Recorder{}, - err: ErrNoNextNode, - expectedRecorder: &Recorder{}, + recorder: &Recorder{}, }, "get single node from recorder": { recorder: &Recorder{ @@ -88,10 +84,7 @@ func Test_Recorder_Next(t *testing.T) { {Hash: []byte{1, 2}, RawData: []byte{3, 4}}, }, }, - node: Node{Hash: []byte{1, 2}, RawData: []byte{3, 4}}, - expectedRecorder: &Recorder{ - nodes: []Node{}, - }, + nodes: []Node{{Hash: []byte{1, 2}, RawData: []byte{3, 4}}}, }, "get node from multiple nodes in recorder": { recorder: &Recorder{ @@ -101,12 +94,10 @@ func Test_Recorder_Next(t *testing.T) { {Hash: []byte{9, 6}, RawData: []byte{7, 8}}, }, }, - node: Node{Hash: []byte{1, 2}, RawData: []byte{3, 4}}, - expectedRecorder: &Recorder{ - nodes: []Node{ - {Hash: []byte{5, 6}, RawData: []byte{7, 8}}, - {Hash: []byte{9, 6}, RawData: []byte{7, 8}}, - }, + nodes: []Node{ + {Hash: []byte{1, 2}, RawData: []byte{3, 4}}, + {Hash: []byte{5, 6}, RawData: []byte{7, 8}}, + {Hash: []byte{9, 6}, RawData: []byte{7, 8}}, }, }, } @@ -116,14 +107,9 @@ func Test_Recorder_Next(t *testing.T) { t.Run(name, func(t *testing.T) { t.Parallel() - node, err := testCase.recorder.Next() + nodes := testCase.recorder.GetNodes() - assert.ErrorIs(t, err, testCase.err) - if testCase.err != nil { - assert.EqualError(t, err, testCase.err.Error()) - } - assert.Equal(t, testCase.node, node) - assert.Equal(t, testCase.expectedRecorder, testCase.recorder) + assert.Equal(t, testCase.nodes, nodes) }) } }