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

minor nmt-wrapper doc addition #306

Merged
merged 5 commits into from
Apr 30, 2021
Merged
Changes from 1 commit
Commits
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
26 changes: 17 additions & 9 deletions p2p/ipld/nmt_wrapper.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,23 @@ var _ rsmt2d.TreeConstructorFn = ErasuredNamespacedMerkleTree{}.Constructor
var _ rsmt2d.Tree = &ErasuredNamespacedMerkleTree{}

// ErasuredNamespacedMerkleTree wraps NamespaceMerkleTree to conform to the
// rsmt2d.Tree interface while catering specifically to erasure data. For the
// first half of the tree, it uses the first DefaultNamespaceIDLen number of
// bytes of the data pushed to determine the namespace. For the second half, it
// uses the parity namespace ID
// rsmt2d.Tree interface while also providing the correct namespaces to the
// underlying NamespaceMerkleTree. It does this by adding the already included
// namespace to the first half of the tree, and then uses the it uses the parity
// namespace ID for each share pushed to the second half of the tree. This
// allows for the namespaces to be included in the erasure data, while also
// keeping constant share size.
type ErasuredNamespacedMerkleTree struct {
squareSize uint64 // note: this refers to the width of the original square before erasure-coded
options []nmt.Option
tree *nmt.NamespacedMerkleTree
}

// NewErasuredNamespacedMerkleTree issues a new ErasuredNamespacedMerkleTree
// NewErasuredNamespacedMerkleTree issues a new ErasuredNamespacedMerkleTree. Panics for squareSize of zero.
Copy link
Member

Choose a reason for hiding this comment

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

Super nitpicky optional nit: I'm a friend of stating how this is to be used instead of saying when it panics. E.g. I'd write: "squareSize must be greater than zero" or "squareSize must be greater than zero or else it will panic".

Copy link
Member Author

Choose a reason for hiding this comment

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

classy 422c573

func NewErasuredNamespacedMerkleTree(squareSize uint64, setters ...nmt.Option) ErasuredNamespacedMerkleTree {
if squareSize == 0 {
panic("cannot create a ErasuredNamespacedMerkleTree of squareSize == 0")
}
Copy link
Member Author

Choose a reason for hiding this comment

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

added a more explicit panic here

tree := nmt.New(sha256.New(), setters...)
return ErasuredNamespacedMerkleTree{squareSize: squareSize, options: setters, tree: tree}
}
Expand All @@ -42,8 +47,8 @@ func (w ErasuredNamespacedMerkleTree) Constructor() rsmt2d.Tree {
// Push adds the provided data to the underlying NamespaceMerkleTree, and
// automatically uses the first DefaultNamespaceIDLen number of bytes as the
// namespace unless the data pushed to the second half of the tree. Fulfills the
// rsmt.Tree interface. NOTE: panics if there's an error pushing to underlying
// NamespaceMerkleTree or if the tree size is exceeded
// rsmt.Tree interface. NOTE: panics if an error is encountered while pushing or
// if the tree size is exceeded.
func (w *ErasuredNamespacedMerkleTree) Push(data []byte, idx rsmt2d.SquareIndex) {
// determine the namespace based on where in the tree we're pushing
nsID := make(namespace.ID, types.NamespaceSize)
Expand All @@ -52,11 +57,14 @@ func (w *ErasuredNamespacedMerkleTree) Push(data []byte, idx rsmt2d.SquareIndex)
panic("pushed past predetermined square size")
Copy link
Member

Choose a reason for hiding this comment

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

Optional nit: This panic could also be more helpful by printing the boundary 2*uint(w.squareSize) and the idx using fmt.Sprintf.

Copy link
Member Author

Choose a reason for hiding this comment

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

good idea 6b7cb81

}

// use the parity namespace if the cell is not in Q0 of the extended datasquare
// if the cell is empty it means we got an empty block so we need to use TailPaddingNamespaceID
// use the parity namespace if the cell is not in Q0 of the extended
// datasquare if the cell is empty it means we got an empty block so we need
// to use TailPaddingNamespaceID
if idx.Axis+1 > uint(w.squareSize) || idx.Cell+1 > uint(w.squareSize) {
copy(nsID, types.ParitySharesNamespaceID)
} else {
// empty shares use the TailPaddingNamespaceID if the data is empty, so
// here we check if the share is empty (namepsace == [0,0,0,0,0,0,0,0])
if bytes.Equal(data[:types.NamespaceSize], nsID) {
Copy link
Member

@liamsi liamsi Apr 30, 2021

Choose a reason for hiding this comment

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

What would help with understanding the code better, is to introduce another variable (if there isn't a package constant already) that was called emptyNsID, then the if-condition would read:

	// empty shares use the TailPaddingNamespaceID if the data is empty
	if bytes.Equal(data[:types.NamespaceSize], emptyNsID) {

Copy link
Member Author

Choose a reason for hiding this comment

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

💯 422c573

copy(nsID, types.TailPaddingNamespaceID)
} else {
Expand Down