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

feat: ensures VerifyLeafHashes rejects leafHashes with wrong namespaces #207

Merged
merged 7 commits into from
Jun 23, 2023

Conversation

staheri14
Copy link
Contributor

@staheri14 staheri14 commented Jun 15, 2023

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

  • 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 self-assigned this Jun 15, 2023
@staheri14 staheri14 added this to the Mainnet milestone Jun 15, 2023
@codecov
Copy link

codecov bot commented Jun 15, 2023

Codecov Report

Merging #207 (b1bd9ce) into master (033ce9e) will decrease coverage by 1.37%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #207      +/-   ##
==========================================
- Coverage   95.39%   94.03%   -1.37%     
==========================================
  Files           5        5              
  Lines         565      570       +5     
==========================================
- Hits          539      536       -3     
- Misses         15       19       +4     
- Partials       11       15       +4     
Impacted Files Coverage Δ
proof.go 88.51% <100.00%> (-5.20%) ⬇️

@staheri14 staheri14 marked this pull request as ready for review June 16, 2023 00:02
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.

@staheri14
Copy link
Contributor Author

After investigating the code to determine the cause of the coverage decline, I have discovered the reason:

In the updated version of the code, the computeRoot function no longer has the potential to fail or produce errors. Errors can only occur when attempting to use the HashNode, which can only happen when incorrect inputs are provided. However, as a result of this PR, all the inputs given to the function are guaranteed to be well-formatted and valid. Specifically, they meet the following criteria:

  • They have valid and properly sized namespace IDs.
  • The minimum and maximum namespaces of each leafHash are correctly ordered.
  • All the leafHashes have the same namespace, ensuring they are lexicographically ordered by namespace.

Therefore, it is safe to disregard the reduced coverage and proceed with merging the PR.

@staheri14 staheri14 merged commit cdc88e8 into master Jun 23, 2023
@staheri14 staheri14 deleted the ensure-leaves-nid-consistency-verify-leafhashes branch June 23, 2023 20:00
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.

Verification functions do not check the consistency between the NID of the supplied leaves and the queried NID
3 participants