NethermindEth / juno

Starknet client implementation.
https://juno.nethermind.io
Apache License 2.0
387 stars 167 forks source link

updating verify proof range to handle empty proof keys #1901

Closed rianhughes closed 3 months ago

rianhughes commented 3 months ago

This PR implements VerifyRangeProof.

It builds on an old PR (1873) which didn't handle empty proof keys, hence the name of this PR.

closes https://github.com/NethermindEth/juno/issues/1895 replaces / closes: https://github.com/NethermindEth/juno/pull/1873

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 71.50538% with 106 lines in your changes missing coverage. Please review.

Project coverage is 75.51%. Comparing base (ad21b2e) to head (f3b20cd).

Files Patch % Lines
core/trie/proof.go 67.35% 31 Missing and 32 partials :warning:
core/trie/trie.go 77.85% 16 Missing and 15 partials :warning:
core/trie/node.go 69.23% 7 Missing and 5 partials :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1901 +/- ## ========================================== - Coverage 75.72% 75.51% -0.21% ========================================== Files 97 97 Lines 8337 8651 +314 ========================================== + Hits 6313 6533 +220 - Misses 1487 1534 +47 - Partials 537 584 +47 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

rianhughes commented 3 months ago

@asdacap, @pnowosie in geth, the VerifyRangeProof executes the verification logic, but also logic to determine if there are more leaves to the right (see here). Imo it would be neater to separate these out into two different functions, hence why I didn't insert it in this implementation of VerifyRangeProof. What are your thoughts?

asdacap commented 3 months ago

You can separate them internally if you want. But at high level, I think its easier to combine them, otherwise, need to pass the keys of proof explicitly, which is implementation details IMO.