Skip to content

feat!: adds input range check to optimize VerifyLeafHashes and VerifyInclusion methods#253

Merged
staheri14 merged 15 commits intomainfrom
sanaz/optimization
May 16, 2024
Merged

feat!: adds input range check to optimize VerifyLeafHashes and VerifyInclusion methods#253
staheri14 merged 15 commits intomainfrom
sanaz/optimization

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 evan-forbes, rootulp and walldiss 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

@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 requested review from rootulp and walldiss May 16, 2024 16:31
@staheri14 staheri14 enabled auto-merge (squash) May 16, 2024 16:31
@codecov
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
@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
topdev22 added a commit to topdev22/app-celestia that referenced this pull request Sep 26, 2025
Motivation: because celestia-core v1.37.0 uses NMT v0.21.0 which
contains this fix: celestiaorg/nmt#253
clevergoldfox added a commit to clevergoldfox/celestia-app that referenced this pull request Feb 3, 2026
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

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants