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

Implement rsmt tree wrapper for nmt #238

Merged
merged 4 commits into from
Mar 21, 2021
Merged

Implement rsmt tree wrapper for nmt #238

merged 4 commits into from
Mar 21, 2021

Conversation

evan-forbes
Copy link
Member

Description

This PR was mostly taken from the closed PR from nmt. As mentioned in the comments their, it makes more sense to keep the lazyledger-core login in lazyledger-core. This PR should be merged before for #235 and #232.

Closes: #237

@evan-forbes evan-forbes requested review from adlerjohn and removed request for tac0turtle March 20, 2021 02:02
@evan-forbes evan-forbes self-assigned this Mar 20, 2021
@evan-forbes evan-forbes changed the title Implement rsmt tree wrapper for nmt 🌲 Implement rsmt tree wrapper for nmt Mar 20, 2021

// if the data is erasure data use the parity ns
default:
copy(nsID, types.ParitySharesNamespaceID)
Copy link
Member

Choose a reason for hiding this comment

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

Note that when celestiaorg/nmt#27 lands, and if we attach the parity nID when calling Push, then this method can be simplified to just calling push on the underlying NMT, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately we will still need a wrapper, because rsmt2d.RepairExtendedDatasquare is not aware of how we erasure namespaces and NamespacedMerkleTree would always assume that the first 8 bytes is the namespace. Now that we're erasuring namespaces, the first 8 bytes of the the 2nd half of the tree is not a namespace, it's just erasure data. nmt.Push would panic.

Copy link
Member

@liamsi liamsi Mar 21, 2021

Choose a reason for hiding this comment

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

Now that we're erasuring namespaces, the first 8 bytes of the the 2nd half of the tree is not a namespace,

I might still be missing something but why can't the caller of the NMT do the "preprocessing" and add the parity namespace in front of the parity shares (basically the same thing the wrapper currently does)? The tree (wrapper) would then not need any counter bc the calling code already knows when the erasured part starts.

Copy link
Member Author

@evan-forbes evan-forbes Mar 21, 2021

Choose a reason for hiding this comment

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

We can do that in lazyledger-core, but we would also have to do that in rsmt2d.RepairDataSquare.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see now. Thanks 🙏

Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

🚀 lgmt 👍🏼

p2p/ipld/nmt_wrapper.go Outdated Show resolved Hide resolved
p2p/ipld/nmt_wrapper.go Outdated Show resolved Hide resolved
p2p/ipld/nmt_wrapper.go Show resolved Hide resolved
evan-forbes and others added 2 commits March 20, 2021 18:21
Co-authored-by: John Adler <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a rsmt2d.Tree interface wrapper for NamespacedMerkleTree
3 participants