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

doc: introduces short namespace absence proof in the NMT specs #209

Merged
merged 7 commits into from
Jun 28, 2023

Conversation

staheri14
Copy link
Collaborator

@staheri14 staheri14 commented Jun 23, 2023

Overview

Closes #205
Additionally, in line with this, to ensure consistency with how namespace ID and namespace (i.e., a combination of namespace ID and version) are utilized in the core-app, I have replaced all instances of words that refer to or imply namespace ID with the more accurate term "namespace".

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@staheri14 staheri14 force-pushed the short-absence-proof-specs branch from 21e24fb to a4d56d0 Compare June 23, 2023 22:02
@codecov
Copy link

codecov bot commented Jun 23, 2023

Codecov Report

Merging #209 (5946b91) into master (cdc88e8) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #209   +/-   ##
=======================================
  Coverage   94.03%   94.03%           
=======================================
  Files           5        5           
  Lines         570      570           
=======================================
  Hits          536      536           
  Misses         19       19           
  Partials       15       15           

@staheri14 staheri14 added documentation Improvements or additions to documentation enhancement New feature or request labels Jun 23, 2023
@staheri14 staheri14 self-assigned this Jun 23, 2023
@staheri14 staheri14 requested review from rootulp and evan-forbes June 23, 2023 22:24
@staheri14 staheri14 marked this pull request as ready for review June 23, 2023 22:25
@staheri14 staheri14 changed the title doc: incorporating the description of short namespace absence proof in the NMT specs doc: introduces short namespace absence proof in the NMT specs Jun 23, 2023
@rootulp
Copy link
Collaborator

rootulp commented Jun 23, 2023

Closes #206

rootulp
rootulp previously approved these changes Jun 23, 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 LGTM, thanks for addressing #206

I think we may want to also rename

type ID []byte
but that could be a follow up

@staheri14
Copy link
Collaborator Author

Overall LGTM, thanks for addressing #206

I think we may want to also rename

type ID []byte

but that could be a follow up

Sure, thanks for mentioning it, I will take care of the code change as well, however, as this will constitute a breaking change, it deserves a separate PR.

@staheri14 staheri14 requested a review from rootulp June 26, 2023 20:30
Copy link

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks for the explanation yesterday. I think it's not so intuitive why you only need one SubTreeHash i.e. that the values in nodes which represent the proof path provides the other side necessary to prove that the namespace does not exist in the tree.

- Compute the tree root `T'` using the leaves and the `proof.nodes`.
If the computed root `T'` is equal to the expected root `T` of the tree, then the `proof` is valid.
To compute the tree root `T'`, the [namespaced hash function](#namespaced-hash) should be utilized.

### Short Namespace Absence Proof

Choose a reason for hiding this comment

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

Can this eventually be combined as a single proof? Asked another way, is there anything different to the guarantees behind this proof to the standard absence proof?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, there is not. It has been expanded on in this forum post.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can this eventually be combined as a single proof?

Yes, they can. The reason they are not unified is twofold. Firstly, the current implementation does not support short-proof generation yet. And secondly, the original long version is more conventional and familiar to the readers, so discussing them separately can help to avoid confusion and potential questions.

Comment on lines +203 to +204
1) `start` and `end` range: These represent the indices of the `SubtreeHash` within its respective level.
Nodes at each level are indexed from left to right starting at index `0`.

Choose a reason for hiding this comment

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

I don't understand why there are two indices. Don't you just need one, i.e. the index of that node. In the example you say start 1 and end 2 but afaiu it would just be index 1. Can you explain that to me?

Copy link
Collaborator Author

@staheri14 staheri14 Jun 27, 2023

Choose a reason for hiding this comment

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

We use a general proof struct that applies to all namespace proofs. When there are multiple leaves that match the target namespace, start and end indicate a range of indices covering multiple nodes (end is always non-inclusive). However, in cases where only one matching leaf exists or for absence proofs, start and end represent the index of a single node. So, in this example with a single node, start is 1, and end is 2.

@cmwaters
Copy link

Overall LGTM, thanks for addressing #206

I think we may want to also rename

type ID []byte

but that could be a follow up

If we do, I don't think it needs to remain in its own package. It should just be nmt.Namespace

It is important to note that there can be multiple candidates, each belonging to a different level of the tree, that satisfy this requirement for the `SubtreeHash`.
All such candidates are deemed valid from the verification perspective.
2) The verification of Merkle inclusion proof of the returned `SubtreeHash` is valid.
3) The `proof` satisfies the proof completeness as explained in the [Verification of NMT Inclusion Proof](#verification-of-nmt-inclusion-proof).
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the explanation yesterday. I think it's not so intuitive why you only need one SubTreeHash i.e. that the values in nodes which represent the proof path provides the other side necessary to prove that the namespace does not exist in the tree.

@cmwaters regarding this comment you made, I think you are referring to the completeness property, it should be verified on the verification side and is mentioned in here.
The nodes returned as part of the proof.nodes originally serve as normal Merkle inclusion proof for the proof range i.e., [proof.start, proof.end). Additionally, we perform a completeness check on them to ensure that no leaf matching the target namespace is left uncovered by the proof range. Does this address your question?

Copy link
Collaborator Author

@staheri14 staheri14 Jun 27, 2023

Choose a reason for hiding this comment

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

The completeness property and the intuition behind it are explained earlier in the specs i.e., https://github.com/celestiaorg/nmt/blob/master/docs/spec/nmt.md#namespace-proof-verification

Completeness: There are no other leaves matching nID that do not belong to the returned range [start, end).
Proof inclusion can be verified via a regular Merkle range proof verification. However, completeness of the proof requires additional checks. Specifically, 1) the maximum namespace ID of the nodes in the proof that are on the left side of the branch connecting the start leaf to the root must be less than the provided namespace ID (nID), and 2) the minimum namespace ID of the nodes in the proof that are on the right side of the branch connecting the end-1 leaf to the root must be greater than the provided namespace ID (nID).

@staheri14
Copy link
Collaborator Author

Overall LGTM, thanks for addressing #206
I think we may want to also rename

type ID []byte

but that could be a follow up

If we do, I don't think it needs to remain in its own package. It should just be nmt.Namespace

Thanks for your comment, I noted this in its respective issue.

@staheri14
Copy link
Collaborator Author

Going to merge this PR, but will be monitoring this PR for addressing your questions and comments.

@staheri14 staheri14 merged commit 9f0f36e into master Jun 28, 2023
@staheri14 staheri14 deleted the short-absence-proof-specs branch June 28, 2023 17:52
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 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

doc: incorporating the description of partial absence proof in the NMT specs
3 participants