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!: adds input range check to optimize VerifyLeafHashes and VerifyInclusion methods #253

Merged
merged 15 commits into from
May 16, 2024

Conversation

staheri14
Copy link
Contributor

This PR optimizes the VerifyLeafHashes and VerifyInclusion methods by ensuring that the size of the provided leaves or leaf hashes matches the proof range and, if not, making an early return in order to prevent unnecessary hashing operations that would otherwise occur.
It introduces a breaking change by altering the behaviour of VerifyLeafHashes and VerifyInclusion. Previously, verification would succeed even if the provided leaves or leaf hashes exceeded the proof range, as long as the proof was valid and the extra leaves were appended at the end. In the new implementation, the verification will return false or an error in such cases.

@staheri14 staheri14 requested review from rootulp, walldiss and evan-forbes and removed request for rootulp May 13, 2024 18:23
@staheri14 staheri14 self-assigned this May 13, 2024
rootulp
rootulp previously approved these changes May 13, 2024
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.

LGTM

proof.go Outdated Show resolved Hide resolved
@staheri14
Copy link
Contributor Author

[To the reviewers] The codecov error is not specific to this PR, I am investigating it.

@rootulp
Copy link
Collaborator

rootulp commented May 13, 2024

Thanks yea agreed the CodeCov issue isn't caused by this PR: #250

rootulp
rootulp previously approved these changes May 13, 2024
walldiss
walldiss previously approved these changes May 14, 2024
@staheri14
Copy link
Contributor Author

Waiting on #255 to merge

@staheri14 staheri14 dismissed stale reviews from walldiss and rootulp via 864d8dc May 16, 2024 16:30
@staheri14 staheri14 enabled auto-merge (squash) May 16, 2024 16:31
Copy link

codecov bot commented May 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.50%. Comparing base (559fe2b) to head (a660fac).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #253      +/-   ##
==========================================
+ Coverage   66.24%   66.50%   +0.26%     
==========================================
  Files           6        6              
  Lines        1022     1030       +8     
==========================================
+ Hits          677      685       +8     
  Misses        328      328              
  Partials       17       17              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@staheri14 staheri14 merged commit 7831a96 into main May 16, 2024
8 checks passed
@staheri14 staheri14 deleted the sanaz/optimization branch May 16, 2024 19:00
rootulp added a commit to celestiaorg/celestia-app that referenced this pull request Jun 21, 2024
Motivation: because celestia-core v1.37.0 uses NMT v0.21.0 which
contains this fix: celestiaorg/nmt#253
0xchainlover pushed a commit to celestia-org/celestia-app that referenced this pull request Aug 1, 2024
Motivation: because celestia-core v1.37.0 uses NMT v0.21.0 which
contains this fix: celestiaorg/nmt#253
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants