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

Verification functions do not check the consistency between the NID of the supplied leaves and the queried NID #183

Closed
staheri14 opened this issue Apr 26, 2023 · 2 comments · Fixed by #207
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@staheri14
Copy link
Contributor

staheri14 commented Apr 26, 2023

Problem

None of the current proof verification functions i.e., VerifyNamespace and VerifyInclusion verify the consistency between the namespace ID of the leaves and the queried name ID (this inconsistency gets caught later in the root calculation). This can be addressed by incorporating a check at the beginning of the functions, which not only makes the code more clear and easier to maintain and debug, but also saves a significant amount of time by preventing unnecessary root calculations from invalid inputs and allowing for early returns from the function call.

@staheri14 staheri14 self-assigned this Apr 26, 2023
@staheri14 staheri14 added the enhancement New feature or request label May 2, 2023
@staheri14 staheri14 added this to the Mainnet milestone May 2, 2023
@staheri14
Copy link
Contributor Author

staheri14 commented May 2, 2023

Update:
VerifyNamespace does check the NID of the leaves against the queried NID in this line prior to hashing. Nothing to be done in this method.
VerifyInclusion does not have to do any check since the leaves passed as argument to this method are not namespaced, and get namespaced within the method (hence no check needed).
The only method that can do the check but does not is the, soon to be exposed, VerifyLeafHashes. However, incorporating this check depends on our decision on the following issue #110, so going to mark it as blocked until the other one gets concluded.

@staheri14
Copy link
Contributor Author

Update: VerifyLeafHashes should be revised to include a check that ensures the namespace of the leafHashes parameter (if it is present) matches the queried namespace. This required behavior should remain consistent regardless of whether the proof is a partial absence proof or a full absence proof (in the case of an absence proof, leafHashes will be empty). I will proceed with implementing this modification.

staheri14 added a commit that referenced this issue Jun 23, 2023
…es (#207)

## Overview
Closes #183. The modifications in this PR are also in line with
empowering light clients to furnish BEFPs for proofs that encounter
unsuccessful verification outcomes (where the early termination of
verification upon encountering invalid leaf hashes, with mismatching
namespaces, gives a clear indication of the failure root cause, enabling
them to decide on whether to craft BEFPs or not accordingly).

## Checklist

- [x] New and updated code has appropriate documentation
- [x] New and updated code has new and/or updated testing
- [x] Required CI checks are passing
- [x] Visual proof for any user-facing features like CLI or
documentation updates
- [x] Linked issues closed with keywords
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant