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

chore: NMT specifications #101

Merged
merged 30 commits into from
Feb 21, 2023
Merged

chore: NMT specifications #101

merged 30 commits into from
Feb 21, 2023

Conversation

staheri14
Copy link
Contributor

@staheri14 staheri14 commented Feb 15, 2023

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.

@staheri14 staheri14 self-assigned this Feb 15, 2023
@codecov
Copy link

codecov bot commented Feb 15, 2023

Codecov Report

Merging #101 (b93c398) into master (fa98a59) will not change coverage.
The diff coverage is n/a.

@@           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.

@staheri14 staheri14 marked this pull request as ready for review February 15, 2023 17:03
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.

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.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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.

Copy link
Contributor Author

@staheri14 staheri14 Feb 15, 2023

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

@evan-forbes evan-forbes left a 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

spec/nmt.md Outdated Show resolved Hide resolved
spec/nmt.md Show resolved Hide resolved
@evan-forbes evan-forbes added the documentation Improvements or additions to documentation label Feb 16, 2023
Copy link
Collaborator

@rootulp rootulp left a 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

spec/nmt.md Show resolved Hide resolved
spec/nmt.md Outdated Show resolved Hide resolved
spec/nmt.md Outdated Show resolved Hide resolved
spec/nmt.md Outdated Show resolved Hide resolved
spec/nmt.md Outdated Show resolved Hide resolved
spec/nmt.md Show resolved Hide resolved
spec/nmt.md Outdated Show resolved Hide resolved
spec/nmt.md Show resolved Hide resolved
spec/nmt.md Outdated Show resolved Hide resolved
spec/nmt.md Outdated Show resolved Hide resolved
@staheri14
Copy link
Contributor Author

Thanks @rootulp for your comments! I addressed them all in the following commits:
a9d0d5a
03a7cdb
62dfd10
bad1cc0

@rootulp rootulp mentioned this pull request Feb 17, 2023
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.

Also good to go from my side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants