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: verifies namespace hash format to prevent panic in validateSiblingsNamespaceOrder #185

Merged
merged 34 commits into from
May 1, 2023
Merged

feat: verifies namespace hash format to prevent panic in validateSiblingsNamespaceOrder #185

Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
e49d7b1
updates criteria of an empty proof
staheri14 Apr 21, 2023
5e9091c
replaces slice literal with nID1
staheri14 Apr 21, 2023
b6e3cd3
refactors the code to reject invalid empty proof, adds tests
staheri14 Apr 24, 2023
5c32cbc
corrects IsOfEmptyProof description
staheri14 Apr 24, 2023
84865d7
prefixes min and max with root
staheri14 Apr 24, 2023
c8a8059
fixes linter issues
staheri14 Apr 24, 2023
8a27636
revises some typos
staheri14 Apr 25, 2023
9e3d8de
renames IsOfEmptyProof to IsEmptyProof
staheri14 Apr 25, 2023
20b2205
returns immediately on invalid range
staheri14 Apr 25, 2023
6222d5f
adds unit tests
staheri14 Apr 25, 2023
30cbde4
Merge remote-tracking branch 'origin/master' into verifies-proof-range
staheri14 Apr 25, 2023
9fd31f2
considers start=end as invalid
staheri14 Apr 25, 2023
167629c
defines a utility function for proof range verification
staheri14 Apr 25, 2023
fcd3cd0
returns the error returned by validateRange
staheri14 Apr 26, 2023
d11e9c2
adds proof range check
staheri14 Apr 26, 2023
bb07d07
incorporates further tests covering new range check
staheri14 Apr 26, 2023
d491b52
returns err produced by validateRange
staheri14 Apr 26, 2023
e6cb4dc
Merge branch 'verifies-proof-range' into panics-in-verifyleafhashes
staheri14 Apr 26, 2023
a449d68
implements the necessary logic to handle empty proofs
staheri14 Apr 26, 2023
29007f4
develops tests
staheri14 Apr 26, 2023
8fdaab2
revises the err message
staheri14 Apr 26, 2023
409f0f8
extracts the NID from the leaf
staheri14 Apr 26, 2023
2d504d5
removes an excess line
staheri14 Apr 26, 2023
48010b1
Merge branch 'master' into panics-in-verifyleafhashes
staheri14 Apr 27, 2023
712ed08
revises tests descriptions
staheri14 Apr 27, 2023
e01ce18
declares a variable for nonEmptyProof for readability
staheri14 Apr 27, 2023
fc595aa
removes an excess line
staheri14 Apr 27, 2023
021adc0
replaces manual extraction of min and max NID with proper func calls
staheri14 Apr 27, 2023
951ad75
adds node format validation to the validateSiblingsNamespaceOrder
staheri14 Apr 27, 2023
81e86bf
implements test for the invalid siblings format
staheri14 Apr 28, 2023
f70d2d7
resolves linter issues
staheri14 Apr 28, 2023
53597b9
Merge branch 'master' into resolves-panic-validateSiblings
staheri14 May 1, 2023
f3076fe
revises the error message
staheri14 May 1, 2023
fb8ee89
deletes stale comments
staheri14 May 1, 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
16 changes: 10 additions & 6 deletions hasher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,14 +207,18 @@ func (n *Hasher) ValidateNodeFormat(node []byte) (err error) {
// validateSiblingsNamespaceOrder checks whether left and right as two sibling
// nodes in an NMT have correct namespace IDs relative to each other, more
// specifically, the maximum namespace ID of the left sibling should not exceed
// the minimum namespace ID of the right sibling. It returns ErrUnorderedSiblings error if the check fails. Note that the function assumes
// that the left and right nodes are in correct format, i.e., they are
// namespaced hash values. Otherwise, it panics.
// the minimum namespace ID of the right sibling. It returns ErrUnorderedSiblings error if the check fails.
Copy link
Collaborator

Choose a reason for hiding this comment

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

[unrelated] since a namespace now consists of a version and an ID, "namespace" may be clearer than "namespace ID". Thoughts on renaming usage of "namespace ID" to "namespace" in this repo so that the term matches its usage in celestia-app. If yes, I can create a new issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that the renaming is a good idea, as long as we're consistent with the terms we use. Regarding the namespace version, would it be something that the nmt lib needs to handle, or is it transparent and won't impact implementation? As far as I understand, the only change is the size of the namespace, and any further parsing of the namespace byte slice would depend on the specific logic of the application, correct?

Copy link
Collaborator

Choose a reason for hiding this comment

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

or is it transparent and won't impact implementation?

Won't impact implementation. The NMT lib is already used by celestia-app so no further changes are necessary to support namespaces with versions.

As far as I understand, the only change is the size of the namespace, and any further parsing of the namespace byte slice would depend on the specific logic of the application, correct?

Correct.

func (n *Hasher) validateSiblingsNamespaceOrder(left, right []byte) (err error) {
if err := n.ValidateNodeFormat(left); err != nil {
return fmt.Errorf("%w: left node does not match the namesapce hash format", err)
}
if err := n.ValidateNodeFormat(right); err != nil {
return fmt.Errorf("%w: right node does not match the namesapce hash format", err)
}
// each NMT node has two namespace IDs for the min and max
rootulp marked this conversation as resolved.
Show resolved Hide resolved
totalNamespaceLen := 2 * n.NamespaceLen
leftMaxNs := namespace.ID(left[n.NamespaceLen:totalNamespaceLen])
rightMinNs := namespace.ID(right[:n.NamespaceLen])
// totalNamespaceLen := 2 * n.NamespaceLen
rootulp marked this conversation as resolved.
Show resolved Hide resolved
leftMaxNs := namespace.ID(MaxNamespace(left, n.NamespaceSize()))
rightMinNs := namespace.ID(MinNamespace(right, n.NamespaceSize()))

// check the namespace range of the left and right children
if rightMinNs.Less(leftMaxNs) {
Expand Down
19 changes: 16 additions & 3 deletions hasher_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,9 @@ func TestHashNode_ChildrenNamespaceRange(t *testing.T) {
}

func TestValidateSiblingsNamespaceOrder(t *testing.T) {
// create a dummy hash to use as the digest of the left and right child
randHash := createByteSlice(sha256.Size, 0x01)

type children struct {
l []byte // namespace hash of the left child with the format of MinNs||MaxNs||h
r []byte // namespace hash of the right child with the format of MinNs||MaxNs||h
Expand All @@ -288,19 +291,29 @@ func TestValidateSiblingsNamespaceOrder(t *testing.T) {
children children
wantErr bool
}{
{
"wrong left node format", 2,
children{concat([]byte{0, 0, 1, 1}, randHash[:len(randHash)-1]), concat([]byte{0, 0, 1, 1}, randHash)},
true,
},
{
"wrong right node format", 2,
children{concat([]byte{0, 0, 1, 1}, randHash), concat([]byte{0, 0, 1, 1}, randHash[:len(randHash)-1])},
true,
},
{
"left.maxNs>right.minNs", 2,
children{[]byte{0, 0, 1, 1}, []byte{0, 0, 1, 1}},
children{concat([]byte{0, 0, 1, 1}, randHash), concat([]byte{0, 0, 1, 1}, randHash)},
true,
},
{
"left.maxNs=right.minNs", 2,
children{[]byte{0, 0, 1, 1}, []byte{1, 1, 2, 2}},
children{concat([]byte{0, 0, 1, 1}, randHash), concat([]byte{1, 1, 2, 2}, randHash)},
false,
},
{
"left.maxNs<right.minNs", 2,
children{[]byte{0, 0, 1, 1}, []byte{2, 2, 3, 3}},
children{concat([]byte{0, 0, 1, 1}, randHash), concat([]byte{2, 2, 3, 3}, randHash)},
false,
},
}
Expand Down