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.

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: ensures consistent use of the assumption that NMT nodes are ordered ascendingly #188

Merged
merged 49 commits into from
May 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
49 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
e46a1b2
compares min and max
staheri14 May 1, 2023
fb8ee89
deletes stale comments
staheri14 May 1, 2023
05c2ca0
Merge branch 'resolves-panic-validateSiblings' into check-namespace-r…
staheri14 May 1, 2023
224d8cf
adds a comment
staheri14 May 1, 2023
01ecb89
adds tests
staheri14 May 1, 2023
e0905fe
removes an invalid test case
staheri14 May 1, 2023
d64c018
updates error messages and removes some duplicate helper functions
staheri14 May 1, 2023
c398e19
simplifies the namespace computation under the IgnoreMaxNamespace flag
staheri14 May 1, 2023
c0af83b
deletes unused min and max functions
staheri14 May 1, 2023
b0eef2f
revises the IgnoreMaxNamespace description to match the implementation
staheri14 May 1, 2023
21fccff
Merge branch 'master' into use-namespace-ordering-assumption-consiste…
staheri14 May 8, 2023
e73b843
adds computeRange and its unittests
staheri14 May 8, 2023
a3fcfe0
deletes the old compute range function
staheri14 May 8, 2023
4373c00
adds function descriptions
staheri14 May 8, 2023
9c070ad
edits function name
staheri14 May 8, 2023
6768a68
Merge branch 'master' into use-namespace-ordering-assumption-consiste…
staheri14 May 12, 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
39 changes: 18 additions & 21 deletions hasher.go
Original file line number Diff line number Diff line change
Expand Up @@ -256,12 +256,9 @@ func (n *Hasher) ValidateNodes(left, right []byte) error {
// right.maxNID) || H(NodePrefix, left, right)`. `res` refers to the return
// value of the HashNode. However, if the `ignoreMaxNs` property of the Hasher
// is set to true, the calculation of the namespace ID range of the node
// slightly changes. In this case, when setting the upper range, the maximum
// possible namespace ID (i.e., 2^NamespaceIDSize-1) should be ignored if
// possible. This is achieved by taking the maximum value among only those namespace
// IDs available in the range of its left and right children that are not
// equal to the maximum possible namespace ID value. If all the namespace IDs are equal
// to the maximum possible value, then the maximum possible value is used.
// slightly changes. Let MAXNID be the maximum possible namespace ID value i.e., 2^NamespaceIDSize-1.
// If the namespace range of the right child is start=end=MAXNID, indicating that it represents the root of a subtree whose leaves all have the namespace ID of `MAXNID`, then exclude the right child from the namespace range calculation. Instead,
// assign the namespace range of the left child as the parent's namespace range.
func (n *Hasher) HashNode(left, right []byte) ([]byte, error) {
// validate the inputs
if err := n.ValidateNodes(left, right); err != nil {
Expand All @@ -271,21 +268,11 @@ func (n *Hasher) HashNode(left, right []byte) ([]byte, error) {
h := n.baseHasher
h.Reset()

// the actual hash result of the children got extended (or flagged) by their
// children's minNs || maxNs; hence the flagLen = 2 * NamespaceLen:
flagLen := 2 * n.NamespaceLen
leftMinNs, leftMaxNs := left[:n.NamespaceLen], left[n.NamespaceLen:flagLen]
rightMinNs, rightMaxNs := right[:n.NamespaceLen], right[n.NamespaceLen:flagLen]

minNs := min(leftMinNs, rightMinNs)
var maxNs []byte
if n.ignoreMaxNs && n.precomputedMaxNs.Equal(leftMinNs) {
maxNs = n.precomputedMaxNs
Copy link
Contributor Author

@staheri14 staheri14 May 1, 2023

Choose a reason for hiding this comment

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

If leftMinNs represents the maximum possible namespace ID, then by definition, rightMax must also be the maximum possible namespace ID (left and right nodes are already checked to be ordered based on their namespaces). Therefore, this check can be removed.

} else if n.ignoreMaxNs && n.precomputedMaxNs.Equal(rightMinNs) {
maxNs = leftMaxNs
} else {
maxNs = max(leftMaxNs, rightMaxNs)
}
leftMinNs, leftMaxNs := MinNamespace(left, n.NamespaceLen), MaxNamespace(left, n.NamespaceLen)
rightMinNs, rightMaxNs := MinNamespace(right, n.NamespaceLen), MaxNamespace(right, n.NamespaceLen)

// compute the namespace range of the parent node
minNs, maxNs := computeNsRange(leftMinNs, leftMaxNs, rightMinNs, rightMaxNs, n.ignoreMaxNs, n.precomputedMaxNs)

res := make([]byte, 0)
res = append(res, minNs...)
Expand Down Expand Up @@ -316,3 +303,13 @@ func min(ns []byte, ns2 []byte) []byte {
}
return ns2
}

// computeNsRange computes the namespace range of the parent node based on the namespace ranges of its left and right children.
func computeNsRange(leftMinNs, leftMaxNs, rightMinNs, rightMaxNs []byte, ignoreMaxNs bool, precomputedMaxNs namespace.ID) (minNs []byte, maxNs []byte) {
minNs = leftMinNs
maxNs = rightMaxNs
if ignoreMaxNs && bytes.Equal(precomputedMaxNs, rightMinNs) {
maxNs = leftMaxNs
}
return minNs, maxNs
}
108 changes: 108 additions & 0 deletions hasher_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -839,3 +839,111 @@ func TestMin(t *testing.T) {
})
}
}

// TestComputeNsRange tests the ComputeRange function.
func TestComputeNsRange(t *testing.T) {
nIDSize := 1
precomputedMaxNs := bytes.Repeat([]byte{0xFF}, nIDSize)

testCases := []struct {
leftMinNs, leftMaxNs, rightMinNs, rightMaxNs, expectedMinNs, expectedMaxNs []byte
ignoreMaxNs bool
}{
{
ignoreMaxNs: true,
leftMinNs: precomputedMaxNs,
leftMaxNs: precomputedMaxNs,
rightMinNs: precomputedMaxNs,
rightMaxNs: precomputedMaxNs,
expectedMinNs: precomputedMaxNs,
expectedMaxNs: precomputedMaxNs,
},
{
ignoreMaxNs: true,
leftMinNs: []byte{0x00},
leftMaxNs: precomputedMaxNs,
rightMinNs: precomputedMaxNs,
rightMaxNs: precomputedMaxNs,
expectedMinNs: []byte{0x00},
expectedMaxNs: precomputedMaxNs,
},
{
ignoreMaxNs: true,
leftMinNs: []byte{0x00},
leftMaxNs: []byte{0x01},
rightMinNs: precomputedMaxNs,
rightMaxNs: precomputedMaxNs,
expectedMinNs: []byte{0x00},
expectedMaxNs: []byte{0x01},
},
{
ignoreMaxNs: true,
leftMinNs: []byte{0x00},
leftMaxNs: []byte{0x01},
rightMinNs: []byte{0x02},
rightMaxNs: precomputedMaxNs,
expectedMinNs: []byte{0x00},
expectedMaxNs: precomputedMaxNs,
Copy link
Collaborator

Choose a reason for hiding this comment

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

[non-blocking][question] this test case has me puzzled. Based on the feature ignoreMaxNs I would expect max ns to be []byte{0x02}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, the IgnoreMaxNamespace ignores the MaxNs from the namespace range of the parent node when the right subtree (the right child) of the parent is filled with the leaves all with MaxNs. In this testcase, the right subtree is not populated only by MaxNs leaves, so, the condition is not met hence the normal range calculation takes place. This is how it is done in the original code as well, please see

nmt/hasher.go

Lines 280 to 288 in fd00c52

minNs := min(leftMinNs, rightMinNs)
var maxNs []byte
if n.ignoreMaxNs && n.precomputedMaxNs.Equal(leftMinNs) {
maxNs = n.precomputedMaxNs
} else if n.ignoreMaxNs && n.precomputedMaxNs.Equal(rightMinNs) {
maxNs = leftMaxNs
} else {
maxNs = max(leftMaxNs, rightMaxNs)
}
.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for explaining. I think we can def merge this PR.

No concrete feedback but I think the ignoreMaxNs feature is confusing b/c it only applies under certain circumstances.

Copy link
Contributor Author

@staheri14 staheri14 May 10, 2023

Choose a reason for hiding this comment

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

I agree with the confusion part. It takes time for one's mental model to be shaped around the semantics of IgnoreMaxNamespace, especially without any context regarding its use case in Celestia. Without such context, it may never make sense.

it only applies under certain circumstances.

To provide further clarity, this condition applies only under two circumstances. Firstly, when the tree has more than one leaf, and secondly, when the right half of a (parent) node is fully occupied by values that all have the Maximum namespace. In essence, the first condition can be deduced from the second one, so there is effectively only one condition.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should (eventually) move the ignoreNS logic to celestia somehow as it is really difficult to grasp in the context of the NMT only.

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 think we should (eventually) move the ignoreNS logic to celestia somehow as it is really difficult to grasp in the context of the NMT only.

Agree with this, created the tracking issue as well #195

},
{
ignoreMaxNs: true,
leftMinNs: []byte{0x00},
leftMaxNs: []byte{0x01},
rightMinNs: []byte{0x02},
rightMaxNs: []byte{0x03},
expectedMinNs: []byte{0x00},
expectedMaxNs: []byte{0x03},
},
{
ignoreMaxNs: false,
leftMinNs: precomputedMaxNs,
leftMaxNs: precomputedMaxNs,
rightMinNs: precomputedMaxNs,
rightMaxNs: precomputedMaxNs,
expectedMinNs: precomputedMaxNs,
expectedMaxNs: precomputedMaxNs,
},
{
ignoreMaxNs: false,
leftMinNs: []byte{0x00},
leftMaxNs: precomputedMaxNs,
rightMinNs: precomputedMaxNs,
rightMaxNs: precomputedMaxNs,
expectedMinNs: []byte{0x00},
expectedMaxNs: precomputedMaxNs,
},
{
ignoreMaxNs: false,
leftMinNs: []byte{0x00},
leftMaxNs: []byte{0x01},
rightMinNs: precomputedMaxNs,
rightMaxNs: precomputedMaxNs,
expectedMinNs: []byte{0x00},
expectedMaxNs: precomputedMaxNs,
},
{
ignoreMaxNs: false,
leftMinNs: []byte{0x00},
leftMaxNs: []byte{0x01},
rightMinNs: []byte{0x02},
rightMaxNs: precomputedMaxNs,
expectedMinNs: []byte{0x00},
expectedMaxNs: precomputedMaxNs,
},
{
ignoreMaxNs: false,
leftMinNs: []byte{0x00},
leftMaxNs: []byte{0x01},
rightMinNs: []byte{0x02},
rightMaxNs: []byte{0x03},
expectedMinNs: []byte{0x00},
expectedMaxNs: []byte{0x03},
},
}

for _, tc := range testCases {
minNs, maxNs := computeNsRange(tc.leftMinNs, tc.leftMaxNs, tc.rightMinNs, tc.rightMaxNs, tc.ignoreMaxNs, precomputedMaxNs)
assert.True(t, bytes.Equal(tc.expectedMinNs, minNs))
assert.True(t, bytes.Equal(tc.expectedMaxNs, maxNs))
}
}