VerifyProofs #1873
VerifyProofs #1873
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1873 +/- ##
==========================================
- Coverage 75.36% 75.29% -0.08%
==========================================
Files 97 97
Lines 8342 8561 +219
==========================================
+ Hits 6287 6446 +159
- Misses 1523 1556 +33
- Partials 532 559 +27 ☔ View full report in Codecov by Sentry. |
rianhughes
force-pushed
the
rianhughes/proof-range
branch
4 times, most recently
from
4ad9b74 to
482e2fd
Compare
|
Note: I still need to account for edge cases (eg either single or no proof), but it would be useful to have a set of eyes on this now |
core/trie/proof.go
Outdated
| // If the trie is constructed incorrectly then the root will have an incorrect key(len,path), and value, | ||
| // and therefore it's hash will be incorrect. | ||
| // ref: https://github.com/ethereum/go-ethereum/blob/v1.14.3/trie/proof.go#L484 | ||
| func VerifyRangeProof(root *felt.Felt, keys, values []*felt.Felt, proofKeys, proofValues [2]*felt.Felt, |
There was a problem hiding this comment.
Note, proof is 1 array, not 2. It got deduplicated into one.
There was a problem hiding this comment.
A single proof is an array, but we need two proofs, right?
There was a problem hiding this comment.
Well, the easiest way to generate the proof is with the left (start query actually, probably will come back later) and right key separately. Then it is combined into one set. I'm guessing this is done because a lot of the time some of the top nodes are going to be duplicated.
There was a problem hiding this comment.
Geth seems to store proofs nodes in a key-value store, so I guess starkware are thinking about dong something similar.
There was a problem hiding this comment.
Mind if I open a separate issue for this?
core/trie/proof.go
Outdated
| // Set the children of the current node | ||
| // The child may need compressed. If so, point to its compressed form. | ||
| childIdx := i + squishedParent + 1 | ||
| if i+2+squishedParent < len(proofNodes)-1 { |
There was a problem hiding this comment.
This if-else here is pure magic - I cannot understand why we're looking at 3 different cases:
i+2+squishedParent < len(proofNodes)-1i+1+offset == len(proofNodes)-1- otherwise
also I feel me might want to refactor the crntNode construction into a functions which would do the leafKey.Test(...) because a lot of code is duplicated or similar and it's hard to follow what happens
There was a problem hiding this comment.
I added some comments and refactored a bit. Let me know if it's still unclear!
There was a problem hiding this comment.
Just an update: this logic is / will be simplified in this PR. It should be a lot clearer there, but I can push those changes here if you'd like?
There was a problem hiding this comment.
No need to push it here, we can review the changes in PR #1901
rianhughes
force-pushed
the
rianhughes/proof-range
branch
from
eb092d4 to
8de028b
Compare
|
This PR is / can be replaced by this #1901 |
|
closing for #1901 |
The PR implements VerifyRangeProof. It accounts for the cases where either 0, 1 or both boundary proofs are supplied. It does require the proof keys to be set.
Note: It does not account for the case where the proof keys are not set. I have opened a new PR that accounts for this case #1901