feat: ensures consistent use of the assumption that NMT nodes are ordered ascendingly

[staheri14]

Overview

Closes #121 and closes #148.
Please refer to this PR to compare the new and old version of namsespacre range calculation for a parent node in the HashNode().

Checklist

• New and updated code has appropriate documentation
• New and updated code has new and/or updated testing
• Required CI checks are passing
• Visual proof for any user facing features like CLI or documentation updates
• Linked issues closed with keywords

[staheri14]

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.

[codecov]

Codecov Report

Merging #188 (6768a68) into master (c41093d) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #188      +/-   ##
==========================================
- Coverage   95.41%   95.39%   -0.03%     
==========================================
  Files           5        5              
  Lines         567      564       -3     
==========================================
- Hits          541      538       -3     
  Misses         15       15              
  Partials       11       11              

Impacted Files	Coverage Δ
hasher.go	97.54% <100.00%> (-0.06%)	⬇️

[rootulp]

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

[staheri14]

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)
	}

.

[rootulp]

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.

[staheri14]

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.

[liamsi]

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.

[staheri14]

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