-
Notifications
You must be signed in to change notification settings - Fork 164
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
Snap/merge range and proof #1913
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1913 +/- ##
==========================================
- Coverage 75.43% 75.31% -0.13%
==========================================
Files 97 98 +1
Lines 8342 8802 +460
==========================================
+ Hits 6293 6629 +336
- Misses 1519 1582 +63
- Partials 530 591 +61 ☔ View full report in Codecov by Sentry. |
Co-authored-by: Pawel Nowosielski <pnowosie@users.noreply.github.com>
Co-authored-by: Pawel Nowosielski <pnowosie@users.noreply.github.com>
return false, false, err | ||
} | ||
|
||
// TODO: Verify here proof here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to 1) verify the left and right proofs here (assuming both exist), and, 2) calculate the root hash of the new trie built from the keys and boundary proofs right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I don't know what you have in mind for this. IMO, last time, I just add the leaves first, then add the proof one by one (key->hash, not key->proffnode), unless the exact key already have a node.
if !finished || startValue != nil { | ||
feltBts := startValue.Bytes() | ||
startKey := NewKey(t.height, feltBts[:]) | ||
// Yes, the left proof is actually for the start query, not the actual leaf. Very confusing, yea I know. Need to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean that the left proof is for the start query, and not the actual leaf? I thought the left proof would be for the first key/leaf in the range?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually no. Its a very edge.. edge case. Can't remember at the top of my head why.
scenarios := []struct { | ||
name string | ||
startQuery *felt.Felt | ||
skip bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we skip tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it will fail if its not skipped.
"github.com/NethermindEth/juno/core/felt" | ||
) | ||
|
||
func (t *Trie) IterateAndGenerateProof(startValue *felt.Felt, consumer func(key, value *felt.Felt) (bool, error), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious why this approach was taken. Would it not have been simpler to check if there are any leaves to the left (/right) of the startQuery (/endQuery), and return a left (/right) proof if there were? I find the iteration and consumer logic a bit confusing tbh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose of this function is to just get the proofs required for the given range and trie right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we change the function signature to
func (t *Trie) GenerateProof(startRangeKey, endRangeKey *felt.Felt) ([]ProofNode, bool, error)
then we could remove the iteration and consumer logic, and simplify this a lot (by just checking if there are any nodes to the left of the startRangeKey, and the right of endRangeKey). What are your thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose of the function is BOTH iterate and generate proof. Think higher level. The reason its a consumer function is that the condition to stop getting leaves is complicated. It need to stop on:
- Context timeout
- Buffer full
- Max node reached
- At least one node found
- Limit node reached
Also, these are relatively large array. Where possible you don't wanna have one []*felt.Felt buffer. EG: in case of class, you actually just want the []core.Class, not the KV, but you do need the proof and you do need the class in the exact order.
Closing. This is already part of #2035 and we will continue from that one. |
Targeting #1901 not main.
Includes #1907
snap_support.go
which contains more snap specific iteration.IterateAndGenerateProof
which iterate and generate the proof at the same time.VerifyRange
which is used to verify the output ofIterateAndGenerateProof
. Does not integrate the proof verification though.VerifyRange
also have the logic to reconstruct the keys of the proof and check if more leaf is available based on the proof.