chore: improves documentation #96

staheri14 · 2023-02-10T01:11:03Z

A PR inline with EPIC celestiaorg/celestia-app#1296
Aiming at improving the documentation of the NMT library.
As part of this PR, some variables have been renamed to more accurately reflect their values.

…commendations.

…oundInRange

…nd some scattered clarification on the godocs

codecov · 2023-02-10T21:13:58Z

Codecov Report

Merging #96 (04dd6f9) into master (b04eea5) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #96      +/-   ##
==========================================
+ Coverage   96.21%   96.27%   +0.05%     
==========================================
  Files           6        6              
  Lines         423      429       +6     
==========================================
+ Hits          407      413       +6     
  Misses         10       10              
  Partials        6        6

Impacted Files	Coverage Δ
namespace/data.go	`100.00% <ø> (ø)`
namespace/id.go	`100.00% <ø> (ø)`
hasher.go	`96.05% <100.00%> (+0.05%)`	⬆️
nmt.go	`98.80% <100.00%> (ø)`
proof.go	`96.77% <100.00%> (+0.18%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

rootulp

Thanks for all your effort in improving the documentation in this repo!

rootulp · 2023-02-10T21:04:54Z

hasher.go

-		NodePrefix),
-		l...),
-		r...)
+	data := append(append(append(make([]byte, 0, 1+len(left)+len(right)), NodePrefix), left...), right...)


[optional] I find this a lot more readable but it's subjective

data := make([]byte, 0, 1+len(left)+len(right)) data = append(data, NodePrefix) data = append(data, left...) data = append(data, right...)

Agree! please see 26b1ea6

rootulp · 2023-02-10T21:07:18Z

hasher.go

+// which is "res = min(left.minNID, right.minNID) || max(left.maxNID,
+// 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,


[not blocking] for my learning, what is the purpose of the ignoreMaxNs field? Do you think it would help readers to add docs to:

type Hasher struct { baseHasher hash.Hash NamespaceLen namespace.IDSize // ignoreMaxNs is an optimization that does ... ignoreMaxNs bool }

Disregard, I learned from below. Maybe a variation of this comment would be helpful here:

// The "IgnoreMaxNamespace" flag influences the calculation of the // namespace ID range for intermediate nodes in the tree. // This flag signals that, when determining the upper limit of the // namespace ID range for a tree node, the maximum possible namespace ID // (equivalent to "NamespaceIDSize" bytes of 0xFF, or 2^NamespaceIDSize-1) // should be omitted if feasible. For a more in-depth understanding of // this field, refer to the "HashNode" method in the "Hasher.

Sure, done 26b1ea6

rootulp · 2023-02-10T21:08:37Z

namespace/id.go

@@ -4,22 +4,27 @@ import "bytes"

 type ID []byte

+// Less returns true if nid < ID, otherwise, false.


[optional] since both types that are being compared are of type ID I think the variable name helps disambiguate

Suggested change

// Less returns true if nid < ID, otherwise, false.

// Less returns true if nid < other, otherwise, false.

Addressed 26b1ea6

rootulp · 2023-02-10T21:08:44Z

namespace/id.go

 func (nid ID) Less(other ID) bool {
 	return bytes.Compare(nid, other) < 0
 }

+// Equal returns true if nid == ID, otherwise, false.


Suggested change

// Equal returns true if nid == ID, otherwise, false.

// Equal returns true if nid == other, otherwise, false.

Please see 26b1ea6

rootulp · 2023-02-10T21:08:50Z

namespace/id.go

 func (nid ID) Equal(other ID) bool {
 	return bytes.Equal(nid, other)
 }

+// LessOrEqual returns true if nid <= ID, otherwise, false.


Suggested change

// LessOrEqual returns true if nid <= ID, otherwise, false.

// LessOrEqual returns true if nid <= other, otherwise, false.

Done 26b1ea6

rootulp · 2023-02-10T21:25:44Z

nmt.go

 		newIncludeNode := includeNode
+		// check whether the subtree representing the [start, end) range of leaves has overlap with the
+		// queried proof range i.e., [proofStart, proofEnd)
+		// if not


was the rest of this comment lost?

No, it was not lost, if not refers to the following if statement which is entered when the said condition is not met. I tried to clarify it in this commit 26b1ea6.

rootulp · 2023-02-10T21:25:55Z

nmt.go

 		if (end <= proofStart || start >= proofEnd) && includeNode {
+			// setting newIncludeNode to false indicates that non of the subtrees (left and right) of the current


Suggested change

// setting newIncludeNode to false indicates that non of the subtrees (left and right) of the current

// setting newIncludeNode to false indicates that none of the subtrees (left and right) of the current

Fixed 26b1ea6

rootulp · 2023-02-10T21:29:10Z

nmt.go

+// If no leaves are found, foundInRange returns (false, 0, 0).
+//
+// The endIndex is non-inclusive,
+// meaning it does not include the leaf at that index in the range.
 func (n *NamespacedMerkleTree) foundInRange(nID namespace.ID) (bool, int, int) {


[no change needed][optional] we can remove some of the godoc comment by using named return values https://go.dev/tour/basics/7 but that's a debated topic on our team so it may not make sense to adopt for this isolated use-case.

Suggested change

func (n *NamespacedMerkleTree) foundInRange(nID namespace.ID) (bool, int, int) {

func (n *NamespacedMerkleTree) foundInRange(nID namespace.ID) (found bool, startIndex int, endIndex int) {

Thanks, incorporated your suggestion 26b1ea6

rootulp · 2023-02-10T21:41:05Z

proof.go

+func NewInclusionProof(
+	proofStart, proofEnd int, proofNodes [][]byte, ignoreMaxNamespace bool) Proof {


out of curiosity, why split this across two lines?

Wasn't meant to be broken down, fixed it now 26b1ea6

rootulp · 2023-02-10T21:42:16Z

proof.go

+// `data` items MUST be ordered according to their index in the tree, with `data[0]` corresponding to the namespaced data at index `start`,
+//
+//	and the last element in `data` corresponding to the data item at index
+//	`end-1` of the tree.
+//
+// `root` is the root of the NMT against which the `proof` is verified.


line wrapping got messed up here

Corrected 26b1ea6

evan-forbes

wow, this is really great!!

[minor nit]
as Rootul mentioned, there are few lines where it appears that we could wrap more consistently, and by doing so we would also be more consistent with other the other celestia repos. I can also see a few spots where not wrapping makes it more readable, and since we don't have a CI linter in this repo, I'll defer to the author on when exactly to do so.

other than that I couldn't even find spelling errors, and reviewing was a good forcing function to think about nmt again! 👍 👍 easy ✔️

nmt.go

rootulp · 2023-02-14T02:42:54Z

Woohoo! Thanks for addressing my pedantic feedback.

staheri14 · 2023-02-14T20:26:10Z

Thank for your comments and feedback! @rootulp @evan-forbes

staheri14 added 30 commits January 25, 2023 16:31

clarifies the unit of nidSize

57481a0

adds function description for foundInRange

d842ff6

adds missing func descriptions or makes further clarifications

5495171

adds TODOs and missing function descriptions

f1f1a53

refactors gotLeafHashes to leafHashes

0d3d318

corrects the description of generateLeafData

be869c9

adds todos and comments and missing description of methods/functions

69dd211

adds some optimization ideas

b02c28b

adds todo for early hash calculation

da16c5d

adds two more questions about the namespace len size

2c82fbb

modifies the Prove documentation

12cc868

updates the comments

66fd0a8

adds godoc for calculateAbsenceIndex

256a705

adds the function name to the description

810de44

adds further clarification about the absence proof

3dbcd83

deletes excess line

184a0a7

removes a todo

bf514d0

elaborates on the return values of foundInRange

d659113

removes a todo

ef7c36f

adds a suggestion for early return in VerifyNamespace

dc838f1

removes some old questions

28b38b3

explains the order of nodes in the proof

393db51

removes a question

33cf50f

revises the type of the proof

ac87f1c

changes the capitalization of the end

4466bf8

adds documentation for id

f4504d1

expands the proof module by incorporating additional questions and re…

8310cea

…commendations.

adds documentation for the Proof and VerifyNamespace

2d81750

adds further documentation

5d7a9a7

explains the ignore max logic

6b76848

staheri14 added 7 commits February 10, 2023 10:04

minor update

19d6620

expands on Prove and ProveRange

7dea8b9

elaborates on ProveNamespace

de897e4

expands on the godoc for validateandextract namespace, computeRoot, f…

82f9a1e

…oundInRange

adds explanation of isMaxNamespaceIDIgnored to the Proof data type, a…

c60ee20

…nd some scattered clarification on the godocs

fixes wrapped function arguments

0095ca6

fix bugs introduced by wrapped lines

530a279

staheri14 marked this pull request as ready for review February 10, 2023 20:19

Merge tag 'v0.14.0' into nmt-improved-documentation

9effd59

staheri14 requested review from liamsi and evan-forbes February 10, 2023 20:19

staheri14 self-assigned this Feb 10, 2023

staheri14 added 2 commits February 10, 2023 12:59

adds newlines to the end of the files

8742307

minor change

e68e750

staheri14 added 3 commits February 10, 2023 13:17

reverts the minor change

87437b1

fixes a bug

bb85479

fixes another bug

f53c3af

staheri14 added the documentation Improvements or additions to documentation label Feb 10, 2023

rootulp reviewed Feb 10, 2023

View reviewed changes

evan-forbes approved these changes Feb 11, 2023

View reviewed changes

nmt.go Outdated Show resolved Hide resolved

staheri14 added 2 commits February 13, 2023 16:20

addresses comments

26b1ea6

fixes CI failures

04dd6f9

staheri14 requested a review from rootulp February 14, 2023 00:46

rootulp approved these changes Feb 14, 2023

View reviewed changes

staheri14 merged commit b67b45e into master Feb 14, 2023

staheri14 deleted the nmt-improved-documentation branch February 14, 2023 20:26

staheri14 mentioned this pull request Mar 9, 2023

EPIC: Specify data root construction using rsmt2d, nmt, and the nmt wrapper celestiaorg/celestia-app#1296

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: improves documentation #96

chore: improves documentation #96

staheri14 commented Feb 10, 2023 •

edited

Loading

codecov bot commented Feb 10, 2023 •

edited

Loading

rootulp left a comment

rootulp Feb 10, 2023

staheri14 Feb 14, 2023 •

edited

Loading

rootulp Feb 10, 2023

rootulp Feb 10, 2023

staheri14 Feb 14, 2023

rootulp Feb 10, 2023

staheri14 Feb 14, 2023

rootulp Feb 10, 2023

staheri14 Feb 14, 2023

rootulp Feb 10, 2023

staheri14 Feb 14, 2023 •

edited

Loading

rootulp Feb 10, 2023

staheri14 Feb 14, 2023

rootulp Feb 10, 2023

staheri14 Feb 14, 2023

rootulp Feb 10, 2023

staheri14 Feb 14, 2023

rootulp Feb 10, 2023

staheri14 Feb 14, 2023

rootulp Feb 10, 2023

staheri14 Feb 14, 2023

evan-forbes left a comment

rootulp commented Feb 14, 2023

staheri14 commented Feb 14, 2023

		@@ -4,22 +4,27 @@ import "bytes"

		type ID []byte

		// Less returns true if nid < ID, otherwise, false.

	// Less returns true if nid < ID, otherwise, false.
	// Less returns true if nid < other, otherwise, false.

	// Equal returns true if nid == ID, otherwise, false.
	// Equal returns true if nid == other, otherwise, false.

	// LessOrEqual returns true if nid <= ID, otherwise, false.
	// LessOrEqual returns true if nid <= other, otherwise, false.

		if (end <= proofStart \|\| start >= proofEnd) && includeNode {
		// setting newIncludeNode to false indicates that non of the subtrees (left and right) of the current

	func (n *NamespacedMerkleTree) foundInRange(nID namespace.ID) (bool, int, int) {
	func (n *NamespacedMerkleTree) foundInRange(nID namespace.ID) (found bool, startIndex int, endIndex int) {

		func NewInclusionProof(
		proofStart, proofEnd int, proofNodes [][]byte, ignoreMaxNamespace bool) Proof {

chore: improves documentation #96

chore: improves documentation #96

Conversation

staheri14 commented Feb 10, 2023 • edited Loading

codecov bot commented Feb 10, 2023 • edited Loading

Codecov Report

rootulp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staheri14 Feb 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staheri14 Feb 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan-forbes left a comment

Choose a reason for hiding this comment

rootulp commented Feb 14, 2023

staheri14 commented Feb 14, 2023

staheri14 commented Feb 10, 2023 •

edited

Loading

codecov bot commented Feb 10, 2023 •

edited

Loading

staheri14 Feb 14, 2023 •

edited

Loading

staheri14 Feb 14, 2023 •

edited

Loading