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. |
asdacap
requested review from
rianhughes and
pnowosie
and removed request for
rianhughes
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
Targeting #1901 not main.
Includes #1907
snap_support.gowhich contains more snap specific iteration.IterateAndGenerateProofwhich iterate and generate the proof at the same time.VerifyRangewhich is used to verify the output ofIterateAndGenerateProof. Does not integrate the proof verification though.VerifyRangealso have the logic to reconstruct the keys of the proof and check if more leaf is available based on the proof.