-
Notifications
You must be signed in to change notification settings - Fork 45
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
chore: NMT specifications #101
Conversation
Codecov Report
@@ Coverage Diff @@
## master #101 +/- ##
=======================================
Coverage 96.21% 96.21%
=======================================
Files 6 6
Lines 423 423
=======================================
Hits 407 407
Misses 10 10
Partials 6 6 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
…ction name in the figure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great 👏🏼 Thanks!
A few comments (did just a quick first pass; will do a more thorough review in the next 2 days):
- We should consider separating the high-level spec from the library documentation (both are valuable to have and IMO the # NMT Library section could live in a separate document which could be linked from the main readme)
- Suggestion: should we add/append some test-vectors to the spec so implementers can check against these
- Simple single-leaf inclusion proofs or range-proofs that concern only a subset of a namespace are not covered. Q: Is this presumably because these are the same as for rfc6962? Does it still make sense to include these?
spec/nmt.md
Outdated
Namespaced Merkle Tree, NMT for short, is one of the core components of Celestia blockchain. | ||
Transactions in Celestia are associated with a namespace ID which signifies the application they belong to. | ||
Nodes interested in a specific application only need to download transactions of a certain namespace ID. | ||
The Namespaced Merkle Tree (NMT) was introduced in the [LazyLEdger article](https://arxiv.org/abs/1905.09274) to organize transactions in Celestia blocks based on their namespace IDs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Namespaced Merkle Tree (NMT) was introduced in the [LazyLEdger article](https://arxiv.org/abs/1905.09274) to organize transactions in Celestia blocks based on their namespace IDs. | |
The Namespaced Merkle Tree (NMT) was introduced in the [LazyLedger article](https://arxiv.org/abs/1905.09274) to organize transactions in Celestia blocks based on their namespace IDs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should consider separating the high-level spec from the library documentation (both are valuable to have and IMO the # NMT Library section could live in a separate document which could be linked from the main readme)
Separation of these two makes sense to me as well, I was also initially thinking to do the same. Do we want to put the file for the NMT Library description under the spec directory? or a separate directory?
Suggestion: should we add/append some test-vectors to the spec so implementers can check against these
Could you provide more information on the specific type of test vectors you're suggesting? Are you proposing to include these test vectors in the high-level specification or the NMT library section? Figure 1 already shows an NMT that one could create by following the code example in the NMT library section (the exact hash values (the first 7 hex digits) are provided in that figure). Additionally, I have also provided some sample namespace queries and proof results in the spec section. I can certainly provide more elaborate examples, please let me know.
Simple single-leaf inclusion proofs or range-proofs that concern only a subset of a namespace are not covered. Q: Is this presumably because these are the same as for rfc6962? Does it still make sense to include these?
The logic of a normal Merkle range proof and verification is actually already covered in the description of namespace proof generation and verification. That is, each namespace proof consists of two parts: 1- finding the range of leaves matching the queried namespace 2- calculating the Merkle range proof. Similarly, the namespace proof verification involves Merkle range proof verification. However, to make it more clear for the readers, I can add an extra subsection dedicated to the Merkle proofs only. Please let me know your thoughts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separation of these two makes sense to me as well, I was also initially thinking to do the same. Do we want to put the file for the NMT Library description under the spec directory? or a separate directory?
I think the following would make sense
/
|
- docs
|- specs
And I think the library description can go under docs and the specs under specs. No need to tackle this here. We can do this in followup PRs.
The logic of a normal Merkle range proof and verification is actually already covered in the description of namespace proof generation and verification.
Makes sense!
Could you provide more information on the specific type of test vectors you're suggesting?
I think it would be good to have an appendix with realistic testvectors that implementers of nmt (specifically for celestia) could test against. Ideally, these should have the same params as celestia then. We can generate these once we have full confidence in the implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to have an appendix with realistic testvectors that implementers of nmt (specifically for celestia) could test against. Ideally, these should have the same params as celestia then. We can generate these once we have full confidence in the implementation.
Thank you for the clarification. I have created a tracking issue (#111) to address this once the implementation becomes stable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
really great work! thorough && concise!
no blocking comments from my end, all LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks great to me! All my feedback is optional / non-blocking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also good to go from my side.
Overview
This pull request aligns with the celestiaorg/celestia-app#1296 EPIC and introduces an NMT specification that includes a description of NMT as a data structure, an explanation of the NMT library, and a sample code. In terms of description of the NMT library, the focus was on the portions that are used in the NMT wrapper to establish the foundation for the NMT wrapper specification. The NMT methods are illustrated through a running code example that was originally included in the Readme file of the NMT repository (with a minor change in namespace ID of one of the leaves in order to be able to provide an example of absence proof). This example has been expanded to provide a more detailed description of each method call, an interpretation of the parameters, and its relationship to the NMT high-level logic. The The corresponding NMT for the code example is also visualized to facilitate a better understanding and examination of the expected behavior of the library.