Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: compute leaf hashes in the Push method #172

Merged
merged 21 commits into from
Apr 6, 2023
Merged

feat: compute leaf hashes in the Push method #172

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
73dd229
adds hash computation to the Push method
staheri14 Mar 31, 2023
3a649ee
comments out computeLeafHashes from the main methods
staheri14 Mar 31, 2023
1beb01d
removes computeLeafHashes
staheri14 Mar 31, 2023
faf8fe5
removes computeLeafHashes method
staheri14 Mar 31, 2023
b4bfe30
fixes a test in the nmt proof file
staheri14 Mar 31, 2023
79d344a
modifies computeRoot to use the calculated leafHashes
staheri14 Mar 31, 2023
3942342
refactors the codes based on the new computeRoot logic
staheri14 Mar 31, 2023
3da6ebb
removes commented codes
staheri14 Mar 31, 2023
dc55e52
cleans up the tests
staheri14 Mar 31, 2023
2747ab7
a minor update on the docs
staheri14 Mar 31, 2023
e5a609a
modifies a test vector to only contain a corrupt leafHash
staheri14 Mar 31, 2023
6bfa019
keeps the leaf intact and only corrupts the leaf hash
staheri14 Mar 31, 2023
57be188
introduces MustHashLeaf
staheri14 Mar 31, 2023
ffb44f8
implements tests for the MustHashLeaf method
staheri14 Mar 31, 2023
200af3d
incorporates MustHashLeaf in the Push method
staheri14 Mar 31, 2023
e53775c
fixes gofumpt complaints
staheri14 Mar 31, 2023
88d38a0
adds a missing godoc for the Test_MustHashLeaf_panic
staheri14 Mar 31, 2023
40a4f4e
Merge branch 'master' into hash-and-push
staheri14 Apr 3, 2023
634687e
Merge remote-tracking branch 'origin/master' into hash-and-push
staheri14 Apr 4, 2023
d60367b
replaces MustHashLeaf with HashLeaf
staheri14 Apr 4, 2023
5b53987
Merge branch 'master' into hash-and-push
staheri14 Apr 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions hasher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,16 @@ func (n *Hasher) HashLeaf(ndata []byte) ([]byte, error) {
return nameSpacedHash, nil
}

// MustHashLeaf is a wrapper around HashLeaf that panics if an error is
// encountered. The ndata must be a valid leaf node.
func (n *Hasher) MustHashLeaf(ndata []byte) []byte {
res, err := n.HashLeaf(ndata)
if err != nil {
panic(err)
}
return res
}

// ValidateNodeFormat checks whether the supplied node conforms to the
// namespaced hash format and returns an error if it does not. Specifically, it returns ErrInvalidNodeLen if the length of the node is less than the 2*namespace length which indicates it does not match the namespaced hash format.
func (n *Hasher) ValidateNodeFormat(node []byte) (err error) {
Expand Down
26 changes: 26 additions & 0 deletions hasher_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -619,3 +619,29 @@ func TestValidateNodes(t *testing.T) {
})
}
}

// Test_MustHashLeaf_panic checks that the MustHashLeaf method panics only on invalid inputs.
func Test_MustHashLeaf_Panic(t *testing.T) {
hasher := NewNmtHasher(sha256.New(), 2, false)
tests := []struct {
name string
leaf []byte
wantPanic bool
}{
{"valid leaf length", []byte{0, 0}, false},
{"invalid leaf length", []byte{0}, true},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if tt.wantPanic {
assert.Panics(t, func() {
hasher.MustHashLeaf(tt.leaf)
})
} else {
assert.NotPanics(t, func() {
hasher.MustHashLeaf(tt.leaf)
})
}
})
}
}
37 changes: 6 additions & 31 deletions nmt.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,9 +167,6 @@ func (n *NamespacedMerkleTree) Prove(index int) (Proof, error) {
// then ProveRange returns an ErrInvalidRange error. Any errors rather than ErrInvalidRange are irrecoverable and indicate an illegal state of the tree (n).
func (n *NamespacedMerkleTree) ProveRange(start, end int) (Proof, error) {
isMaxNsIgnored := n.treeHasher.IsMaxNamespaceIDIgnored()
if err := n.computeLeafHashesIfNecessary(); err != nil {
return Proof{}, err // this never happens
}
// TODO: store nodes and re-use the hashes instead recomputing parts of the
// tree here
if start < 0 || start >= end || end > len(n.leafHashes) {
Expand Down Expand Up @@ -234,9 +231,6 @@ func (n *NamespacedMerkleTree) ProveNamespace(nID namespace.ID) (Proof, error) {
// case 3) At this point we either found leaves with the namespace nID in
// the tree or calculated the range it would be in (to generate a proof of
// absence and to return the corresponding leaf hashes).
if err := n.computeLeafHashesIfNecessary(); err != nil {
return Proof{}, err
}

proof, err := n.buildRangeProof(proofStart, proofEnd)
if err != nil {
Expand Down Expand Up @@ -418,8 +412,12 @@ func (n *NamespacedMerkleTree) Push(namespacedData namespace.PrefixedData) error
return err
}

// compute the leaf hash
res := n.treeHasher.MustHashLeaf(namespacedData)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not bubble the error here if there ever is one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two reasons why the error is not being bubbled up:

  • In line 410, the data validity is checked through the validateAndExtractNamespace function call. As a result, the HashLeaf function call cannot produce an error. To handle this scenario, the MustHashLeaf function is introduced, which assumes that the data passed to it has already been validated. This approach is similar to the current situation and avoids any unnecessary error handling.

  • If we were to use HashLeaf and bubble up the error, we would need to introduce an if block in our code i.e., the Push method. However, this if block would not be covered in our tests, leading to a reduction in test coverage percentage. Therefore, I have opted not to bubble up the error in this situation.

Please let me know if this addresses your question.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that makes sense, but documenting it in the code for posterity would be a good addition imo. we could also just ignore the error if its impossible (and we have comments), but in general I think its always best to bubble even if we don't strictly need to. Its one less thing the reader or future contributors have to think about.

I'm not blocking on it though! will defer to the author 🙂

Copy link
Collaborator Author

@staheri14 staheri14 Apr 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point, replaced MustHashLeaf with HashLeaf and bubbled up the error, please see d60367b
With this change, the code coverage percentage decreases below the set threshold, and there is no way to develop a test to cover the line that captures the returned error. In such situations, what should we do? Should we still merge the PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, 100% merge imo


// update relevant "caches":
n.leaves = append(n.leaves, namespacedData)
n.leafHashes = append(n.leafHashes, res)
n.updateNamespaceRanges()
n.updateMinMaxID(nID)
n.rawRoot = nil
Expand Down Expand Up @@ -472,13 +470,8 @@ func (n *NamespacedMerkleTree) computeRoot(start, end int) ([]byte, error) {
n.visit(rootHash)
return rootHash, nil
case 1:
leafHash, err := n.treeHasher.HashLeaf(n.leaves[start])
if err != nil { // this should never happen since leaves are added through the Push method, during which leaves formats are validated to make sure they are hashable.
return nil, fmt.Errorf("failed to hash leaf: %w", err)
}
if len(n.leafHashes) < len(n.leaves) {
n.leafHashes = append(n.leafHashes, leafHash)
}
leafHash := make([]byte, len(n.leafHashes[start]))
copy(leafHash, n.leafHashes[start])
n.visit(leafHash, n.leaves[start])
return leafHash, nil
default:
Expand Down Expand Up @@ -577,24 +570,6 @@ func (n *NamespacedMerkleTree) updateMinMaxID(id namespace.ID) {
}
}

// computes the leaf hashes if not already done in a previous call of
// NamespacedMerkleTree.Root()
// Any errors return by this method is irrecoverable and indicate an illegal state of the tree (n).
func (n *NamespacedMerkleTree) computeLeafHashesIfNecessary() error {
// check whether all the hash of all the existing leaves are available
if len(n.leafHashes) < len(n.leaves) {
n.leafHashes = make([][]byte, len(n.leaves))
for i, leaf := range n.leaves {
res, err := n.treeHasher.HashLeaf(leaf)
if err != nil { // should never happen since the validity of leaves is checked in the Push method
return err
}
n.leafHashes[i] = res
}
}
return nil
}

type leafRange struct {
// start and end denote the indices of a leaf in the tree. start ranges from
// 0 up to the total number of leaves minus 1 end ranges from 1 up to the
Expand Down
85 changes: 20 additions & 65 deletions nmt_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -879,17 +879,14 @@ func swap(slice [][]byte, i int, j int) {
func Test_buildRangeProof_Err(t *testing.T) {
// create a nmt, 8 leaves namespaced sequentially from 1-8
treeWithCorruptLeafHash := exampleTreeWithEightLeaves()
err := treeWithCorruptLeafHash.computeLeafHashesIfNecessary()
require.NoError(t, err)
// corrupt a leaf hash
treeWithCorruptLeafHash.leafHashes[4] = treeWithCorruptLeafHash.leafHashes[4][:treeWithCorruptLeafHash.NamespaceSize()]

// create an NMT with 8 sequentially namespaced leaves, numbered from 1 to 8.
treeWithUnorderedLeafHashes := exampleTreeWithEightLeaves()
// swap the positions of the 4th and 5th leaves
swap(treeWithUnorderedLeafHashes.leaves, 4, 5)
err = treeWithUnorderedLeafHashes.computeLeafHashesIfNecessary()
require.NoError(t, err)
swap(treeWithUnorderedLeafHashes.leafHashes, 4, 5)

tests := []struct {
name string
Expand Down Expand Up @@ -918,24 +915,16 @@ func Test_buildRangeProof_Err(t *testing.T) {

// Test_ProveRange_Err tests that ProveRange returns an error when the underlying tree has an invalid state e.g., leaves are not ordered by namespace ID or a leaf hash is corrupted.
func Test_ProveRange_Err(t *testing.T) {
// create an NMT with 8 sequentially namespaced leaves, numbered from 1 to 8.
treeWithCorruptLeaf := exampleTreeWithEightLeaves()
// corrupt a leaf
treeWithCorruptLeaf.leaves[4] = treeWithCorruptLeaf.leaves[4][:treeWithCorruptLeaf.NamespaceSize()-1]

// create an NMT with 8 sequentially namespaced leaves, numbered from 1 to 8.
treeWithCorruptLeafHash := exampleTreeWithEightLeaves()
err := treeWithCorruptLeafHash.computeLeafHashesIfNecessary()
require.NoError(t, err)
// corrupt a leaf hash
treeWithCorruptLeafHash.leafHashes[4] = treeWithCorruptLeafHash.leafHashes[4][:treeWithCorruptLeafHash.NamespaceSize()]

// create an NMT with 8 sequentially namespaced leaves, numbered from 1 to 8.
treeWithUnorderedLeafHashes := exampleTreeWithEightLeaves()
// swap the positions of the 4th and 5th leaves
swap(treeWithUnorderedLeafHashes.leaves, 4, 5)
err = treeWithUnorderedLeafHashes.computeLeafHashesIfNecessary()
require.NoError(t, err)
swap(treeWithUnorderedLeafHashes.leafHashes, 4, 5)

tests := []struct {
name string
Expand All @@ -944,7 +933,6 @@ func Test_ProveRange_Err(t *testing.T) {
wantErr bool
errType error
}{
{"corrupt leaf", treeWithCorruptLeaf, 4, 5, true, ErrInvalidLeafLen},
{"corrupt leaf hash", treeWithCorruptLeafHash, 4, 5, true, ErrInvalidNodeLen},
{"unordered leaf hashes: the out of order leaf", treeWithUnorderedLeafHashes, 4, 5, true, ErrUnorderedSiblings},
{"unordered leaf hashes: first leaf", treeWithUnorderedLeafHashes, 1, 2, true, ErrUnorderedSiblings}, // for a tree with an unordered set of leaves, the ProveRange method should produce an error for any input range,
Expand All @@ -965,24 +953,16 @@ func Test_ProveRange_Err(t *testing.T) {

// The Test_ProveNamespace_Err function tests that ProveNamespace returns an error when the underlying tree is in an invalid state, such as when the leaves are not ordered by namespace ID or when a leaf hash is corrupt.
func Test_ProveNamespace_Err(t *testing.T) {
// create an NMT with 8 sequentially namespaced leaves, numbered from 1 to 8.
treeWithCorruptLeaf := exampleTreeWithEightLeaves()
// corrupt a leaf
treeWithCorruptLeaf.leaves[4] = treeWithCorruptLeaf.leaves[4][:treeWithCorruptLeaf.NamespaceSize()-1]

// create an NMT with 8 sequentially namespaced leaves, numbered from 1 to 8.
treeWithCorruptLeafHash := exampleTreeWithEightLeaves()
err := treeWithCorruptLeafHash.computeLeafHashesIfNecessary()
require.NoError(t, err)
// corrupt a leaf hash
treeWithCorruptLeafHash.leafHashes[4] = treeWithCorruptLeafHash.leafHashes[4][:treeWithCorruptLeafHash.NamespaceSize()]

// create an NMT with 8 sequentially namespaced leaves, numbered from 1 to 8.
treeWithUnorderedLeafHashes := exampleTreeWithEightLeaves()
// swap the positions of the 4th and 5th leaves
swap(treeWithUnorderedLeafHashes.leaves, 4, 5)
err = treeWithUnorderedLeafHashes.computeLeafHashesIfNecessary()
require.NoError(t, err)
swap(treeWithUnorderedLeafHashes.leafHashes, 4, 5)

tests := []struct {
name string
Expand All @@ -991,7 +971,6 @@ func Test_ProveNamespace_Err(t *testing.T) {
wantErr bool
errType error
}{
{"corrupt leaf", treeWithCorruptLeaf, namespace.ID{5, 5}, true, ErrInvalidLeafLen},
{"corrupt leaf hash", treeWithCorruptLeafHash, namespace.ID{5, 5}, true, ErrInvalidNodeLen},
{"unordered leaf hashes: the queried namespace falls in the corrupted range", treeWithUnorderedLeafHashes, namespace.ID{5, 5}, true, ErrUnorderedSiblings},
{"unordered leaf hashes: query for the first namespace", treeWithUnorderedLeafHashes, namespace.ID{1, 1}, true, ErrUnorderedSiblings}, // for a tree with an unordered set of leaves,
Expand All @@ -1013,22 +992,23 @@ func Test_ProveNamespace_Err(t *testing.T) {
// Test_Root_Error tests that the Root method returns an error when the underlying tree is in an invalid state, such as when the leaves are not ordered by namespace ID or when a leaf is corrupt.
func Test_Root_Error(t *testing.T) {
// create an NMT with 8 sequentially namespaced leaves, numbered from 1 to 8.
treeWithCorruptLeaf := exampleTreeWithEightLeaves()
// corrupt a leaf
treeWithCorruptLeaf.leaves[4] = treeWithCorruptLeaf.leaves[4][:treeWithCorruptLeaf.NamespaceSize()-1]
treeWithCorruptLeafHash := exampleTreeWithEightLeaves()
// corrupt a leaf hash
treeWithCorruptLeafHash.leafHashes[4] = treeWithCorruptLeafHash.leafHashes[4][:treeWithCorruptLeafHash.NamespaceSize()-1]

// create an NMT with 8 sequentially namespaced leaves, numbered from 1 to 8.
treeWithUnorderedLeaves := exampleTreeWithEightLeaves()
// swap the positions of the 4th and 5th leaves
swap(treeWithUnorderedLeaves.leaves, 4, 5)
swap(treeWithUnorderedLeaves.leafHashes, 4, 5)

tests := []struct {
name string
tree *NamespacedMerkleTree
wantErr bool
errType error
}{
{"corrupt leaf hash", treeWithCorruptLeaf, true, ErrInvalidLeafLen},
{"corrupt leaf hash", treeWithCorruptLeafHash, true, ErrInvalidNodeLen},
{"unordered leaf hashes", treeWithUnorderedLeaves, true, ErrUnorderedSiblings},
}
for _, tt := range tests {
Expand All @@ -1045,14 +1025,15 @@ func Test_Root_Error(t *testing.T) {
// Test_computeRoot_Error tests that the computeRoot method returns an error when the underlying tree is in an invalid state, such as when the leaves are not ordered by namespace ID or when a leaf is corrupt.
func Test_computeRoot_Error(t *testing.T) {
// create an NMT with 8 sequentially namespaced leaves, numbered from 1 to 8.
treeWithCorruptLeaf := exampleTreeWithEightLeaves()
// corrupt a leaf
treeWithCorruptLeaf.leaves[4] = treeWithCorruptLeaf.leaves[4][:treeWithCorruptLeaf.NamespaceSize()-1]
treeWithCorruptLeafHash := exampleTreeWithEightLeaves()
// corrupt a leaf hash
treeWithCorruptLeafHash.leafHashes[4] = treeWithCorruptLeafHash.leafHashes[4][:treeWithCorruptLeafHash.NamespaceSize()-1]

// create an NMT with 8 sequentially namespaced leaves, numbered from 1 to 8.
treeWithUnorderedLeaves := exampleTreeWithEightLeaves()
// swap the positions of the 4th and 5th leaves
swap(treeWithUnorderedLeaves.leaves, 4, 5)
swap(treeWithUnorderedLeaves.leafHashes, 4, 5)

tests := []struct {
name string
Expand All @@ -1061,10 +1042,9 @@ func Test_computeRoot_Error(t *testing.T) {
wantErr bool
errType error
}{
{"corrupt leaf: the entire tree", treeWithCorruptLeaf, 0, 7, true, ErrInvalidLeafLen},
{"corrupt leaf: the corrupt node", treeWithCorruptLeaf, 4, 5, true, ErrInvalidLeafLen},
{"corrupt leaf: from the corrupt node until the end of the tree", treeWithCorruptLeaf, 4, 7, true, ErrInvalidLeafLen},
{"corrupt leaf: the corrupt node and the node to its left", treeWithCorruptLeaf, 3, 5, true, ErrInvalidLeafLen},
{"corrupt leaf hash: the entire tree", treeWithCorruptLeafHash, 0, 7, true, ErrInvalidNodeLen},
{"corrupt leaf: from the corrupt node until the end of the tree", treeWithCorruptLeafHash, 4, 7, true, ErrInvalidNodeLen},
{"corrupt leaf: the corrupt node and the node to its left", treeWithCorruptLeafHash, 3, 5, true, ErrInvalidNodeLen},
{"unordered leaves: the entire tree", treeWithUnorderedLeaves, 0, 7, true, ErrUnorderedSiblings},
{"unordered leaves: the unordered portion", treeWithUnorderedLeaves, 4, 6, true, ErrUnorderedSiblings},
{"unordered leaves: a portion of the tree containing the unordered leaves", treeWithUnorderedLeaves, 3, 7, true, ErrUnorderedSiblings},
Expand All @@ -1083,22 +1063,23 @@ func Test_computeRoot_Error(t *testing.T) {
// Test_MinMaxNamespace_Err tests that the MinNamespace and MaxNamespace methods return an error when the underlying tree is in an invalid state, such as when the leaves are not ordered by namespace ID or when a leaf is corrupt.
func Test_MinMaxNamespace_Err(t *testing.T) {
// create an NMT with 8 sequentially namespaced leaves, numbered from 1 to 8.
treeWithCorruptLeaf := exampleTreeWithEightLeaves()
// corrupt a leaf
treeWithCorruptLeaf.leaves[4] = treeWithCorruptLeaf.leaves[4][:treeWithCorruptLeaf.NamespaceSize()-1]
treeWithCorruptLeafHash := exampleTreeWithEightLeaves()
// corrupt a leaf hash
treeWithCorruptLeafHash.leafHashes[4] = treeWithCorruptLeafHash.leafHashes[4][:treeWithCorruptLeafHash.NamespaceSize()-1]

// create an NMT with 8 sequentially namespaced leaves, numbered from 1 to 8.
treeWithUnorderedLeaves := exampleTreeWithEightLeaves()
// swap the positions of the 4th and 5th leaves
swap(treeWithUnorderedLeaves.leaves, 4, 5)
swap(treeWithUnorderedLeaves.leafHashes, 4, 5)

tests := []struct {
name string
tree *NamespacedMerkleTree
wantErr bool
errType error
}{
{"corrupt leaf", treeWithCorruptLeaf, true, ErrInvalidLeafLen},
{"corrupt leaf hash", treeWithCorruptLeafHash, true, ErrInvalidNodeLen},
{"unordered leaves", treeWithUnorderedLeaves, true, ErrUnorderedSiblings},
}
for _, tt := range tests {
Expand All @@ -1117,29 +1098,3 @@ func Test_MinMaxNamespace_Err(t *testing.T) {
})
}
}

// Test_computeLeafHashesIfNecessary_err tests that the computeLeafHashesIfNecessary method returns an error when the underlying tree is in an invalid state, such as when a leaf is corrupt.
func Test_computeLeafHashesIfNecessary_err(t *testing.T) {
// create an NMT with 8 sequentially namespaced leaves, numbered from 1 to 8.
treeWithCorruptLeaf := exampleTreeWithEightLeaves()
// corrupt a leaf
treeWithCorruptLeaf.leaves[4] = treeWithCorruptLeaf.leaves[4][:treeWithCorruptLeaf.NamespaceSize()-1]

tests := []struct {
name string
tree *NamespacedMerkleTree
wantErr bool
errType error
}{
{"corrupt leaf", treeWithCorruptLeaf, true, ErrInvalidLeafLen},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := tt.tree.computeLeafHashesIfNecessary()
assert.Equal(t, tt.wantErr, err != nil)
if tt.wantErr {
assert.True(t, errors.Is(err, tt.errType))
}
})
}
}
1 change: 0 additions & 1 deletion proof_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ func TestProof_VerifyNamespace_False(t *testing.T) {
leafIndex := 3
inclusionProofOfLeafIndex, err := n.buildRangeProof(leafIndex, leafIndex+1)
require.NoError(t, err)
require.NoError(t, n.computeLeafHashesIfNecessary())
leafHash := n.leafHashes[leafIndex] // the only data item with namespace ID = 2 in the constructed tree is at index 3
invalidAbsenceProof := NewAbsenceProof(leafIndex, leafIndex+1, inclusionProofOfLeafIndex, leafHash, false)

Expand Down