Skip to content
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

Closed
wants to merge 39 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
5cd826f
updating verify proof range to handle empty proof keys
Jun 7, 2024
3ecb395
test non set proof key - wip
Jun 7, 2024
d2fe491
wip - proof to Path doesn't work
Jun 7, 2024
2cd3157
store the hashes of children in ProofToPath
Jun 8, 2024
3603e28
ProoftoPath update to handle unset proof key
Jun 8, 2024
d8f5811
test - wip
Jun 8, 2024
40a3d04
test - wip
Jun 9, 2024
fb6c899
test passes! :D
Jun 10, 2024
07e99f5
lint
Jun 10, 2024
c0b933a
tidy
Jun 10, 2024
529aef2
fixing tests that broke wip
Jun 10, 2024
ca360e9
build4keyTrie passes again!
Jun 11, 2024
fdf6af2
fixed more tests
Jun 11, 2024
9f9f2fe
fix test wip
Jun 11, 2024
bad7267
fix more tests
Jun 11, 2024
92a98df
fix test -wip
Jun 11, 2024
727801b
fix tests wip
Jun 11, 2024
b4ae2b6
wip
Jun 11, 2024
a123730
all tests passgit add .
Jun 12, 2024
d249186
lint
Jun 12, 2024
21f9453
tidy
Jun 12, 2024
2d7c6e8
tidy logic
Jun 12, 2024
46c7b8a
tidy logic
Jun 12, 2024
0bb2edc
tidy logic
Jun 12, 2024
ce20e08
comment: update getLeftRightHash
Jun 12, 2024
ec2f917
update ProofToPath comment
Jun 12, 2024
dd21cc0
bubble up getLeftRightHash err
Jun 12, 2024
571eb6e
Trie iterate
asdacap Jun 14, 2024
b614c2e
Address comment
asdacap Jun 14, 2024
89cf035
Merge branch 'feature/trie-iterate' into snap/merge-range-and-proof
asdacap Jun 19, 2024
20e2aa0
Snap support
asdacap Jun 20, 2024
faa0884
Finished is needed internally
asdacap Jun 20, 2024
59440ac
Missed a printf
asdacap Jun 20, 2024
35b46d0
Format
asdacap Jun 20, 2024
cb26dd7
Fix lint
asdacap Jun 20, 2024
1454ad5
Update core/trie/key.go
asdacap Jun 25, 2024
d4b4232
Update core/trie/snap_support.go
asdacap Jun 25, 2024
4f4940f
address keys
asdacap Jun 27, 2024
909914d
Fix lint
asdacap Jun 27, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Fix lint
  • Loading branch information
asdacap committed Jun 20, 2024
commit cb26dd736a565d97ae177770ca324dc795cd1208
28 changes: 10 additions & 18 deletions core/trie/snap_support.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@
"github.com/NethermindEth/juno/core/felt"
)

func (t *Trie) IterateAndGenerateProof(startValue *felt.Felt, consumer func(key, value *felt.Felt) (bool, error)) ([]ProofNode, bool, error) {
func (t *Trie) IterateAndGenerateProof(startValue *felt.Felt, consumer func(key, value *felt.Felt) (bool, error),
Copy link
Contributor

@rianhughes rianhughes Jun 27, 2024

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

Copy link
Contributor

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?

Copy link
Contributor

@rianhughes rianhughes Jun 27, 2024

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?

Copy link
Contributor Author

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.

) ([]ProofNode, bool, error) {
var lastKey *felt.Felt

finished, err := t.Iterate(startValue, func(key, value *felt.Felt) (bool, error) {
Expand All @@ -13,7 +14,7 @@
return consumer(key, value)
})
if err != nil {
return nil, false, err

Check warning on line 17 in core/trie/snap_support.go

View check run for this annotation

Codecov / codecov/patch

core/trie/snap_support.go#L17

Added line #L17 was not covered by tests
}

proofset := map[felt.Felt]ProofNode{}
Expand All @@ -26,13 +27,12 @@
// actually check that the server did not skip leafs.
leftProof, err := GetProof(&startKey, t)
if err != nil {
return nil, false, err

Check warning on line 30 in core/trie/snap_support.go

View check run for this annotation

Codecov / codecov/patch

core/trie/snap_support.go#L30

Added line #L30 was not covered by tests
}
for _, proof := range leftProof {
// Well.. using the trie hash here is kinda slow... but I just need it to work right now.
proofset[*proof.Hash(t.hash)] = proof
}

}

if !finished && lastKey != nil {
Expand All @@ -40,7 +40,7 @@
lastKey := NewKey(t.height, feltBts[:])
rightProof, err := GetProof(&lastKey, t)
if err != nil {
return nil, false, err

Check warning on line 43 in core/trie/snap_support.go

View check run for this annotation

Codecov / codecov/patch

core/trie/snap_support.go#L43

Added line #L43 was not covered by tests
}

for _, proof := range rightProof {
Expand All @@ -56,7 +56,9 @@
return proofs, finished, nil
}

func VerifyRange(root, startKey *felt.Felt, keys, values []*felt.Felt, proofs []ProofNode, hash hashFunc) (hasMore bool, valid bool, err error) {
func VerifyRange(root, startKey *felt.Felt, keys, values []*felt.Felt, proofs []ProofNode, hash hashFunc,
asdacap marked this conversation as resolved.
Show resolved Hide resolved
treeHeight uint8,
) (hasMore, valid bool, oerr error) {
proofMap := map[felt.Felt]ProofNode{}
for _, proof := range proofs {
proofHash := proof.Hash(hash)
Expand All @@ -67,21 +69,21 @@
// Special case where the whole trie is sent in one go.
// We just need to completely reconstruct the trie.

tempTrie, err := newTrie(newMemStorage(), 251, hash) //nolint:gomnd
tempTrie, err := newTrie(newMemStorage(), treeHeight, hash)
if err != nil {
return false, false, err

Check warning on line 74 in core/trie/snap_support.go

View check run for this annotation

Codecov / codecov/patch

core/trie/snap_support.go#L74

Added line #L74 was not covered by tests
}

for i, key := range keys {
_, err := tempTrie.Put(key, values[i])
_, err = tempTrie.Put(key, values[i])
if err != nil {
return false, false, err

Check warning on line 80 in core/trie/snap_support.go

View check run for this annotation

Codecov / codecov/patch

core/trie/snap_support.go#L80

Added line #L80 was not covered by tests
}
}

recalculatedRoot, err := tempTrie.Root()
if err != nil {
return false, false, err

Check warning on line 86 in core/trie/snap_support.go

View check run for this annotation

Codecov / codecov/patch

core/trie/snap_support.go#L86

Added line #L86 was not covered by tests
}

if !root.Equal(recalculatedRoot) {
Expand All @@ -97,30 +99,20 @@
}

proofKeys := map[felt.Felt]Key{}
err = buildKeys(NewKey(0, []byte{}), root, proofMap, proofKeys, 0)
err := buildKeys(NewKey(0, []byte{}), root, proofMap, proofKeys, 0)
asdacap marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return false, false, err

Check warning on line 104 in core/trie/snap_support.go

View check run for this annotation

Codecov / codecov/patch

core/trie/snap_support.go#L104

Added line #L104 was not covered by tests
}

// No idea how this work
/*
proofValuesArray := []*felt.Felt{}
proofKeysArray := []*Key{}
for f, key := range proofKeys {
proofValuesArray = append(proofValuesArray, &f)
proofKeysArray = append(proofKeysArray, &key)
}

VerifyRangeProof(root, keys, values, nil, nil, nil, hash)
*/
// TODO: Verify here proof here
Copy link
Contributor

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?

Copy link
Contributor Author

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.


hasMoreKeyCheck := startKey
asdacap marked this conversation as resolved.
Show resolved Hide resolved
if len(keys) > 0 {
hasMoreKeyCheck = keys[len(keys)-1]
}

feltBytes := hasMoreKeyCheck.Bytes()
asdacap marked this conversation as resolved.
Show resolved Hide resolved
hasMoreKeyCheckKey := NewKey(251, feltBytes[:])
hasMoreKeyCheckKey := NewKey(treeHeight, feltBytes[:])

// does this actually work on all case?
hasMore = false
Expand All @@ -146,7 +138,7 @@
ch := proofNode.Edge.Child
err := buildKeys(chKey, ch, proofMap, keys, depth+1)
if err != nil {
return err

Check warning on line 141 in core/trie/snap_support.go

View check run for this annotation

Codecov / codecov/patch

core/trie/snap_support.go#L141

Added line #L141 was not covered by tests
}
} else {
binary := proofNode.Binary
Expand All @@ -155,14 +147,14 @@
ch := binary.LeftHash
err := buildKeys(chKey, ch, proofMap, keys, depth+1)
if err != nil {
return err

Check warning on line 150 in core/trie/snap_support.go

View check run for this annotation

Codecov / codecov/patch

core/trie/snap_support.go#L150

Added line #L150 was not covered by tests
}

chKey = currentKey.AppendBit(true)
ch = binary.RightHash
err = buildKeys(chKey, ch, proofMap, keys, depth+1)
if err != nil {
return err

Check warning on line 157 in core/trie/snap_support.go

View check run for this annotation

Codecov / codecov/patch

core/trie/snap_support.go#L157

Added line #L157 was not covered by tests
}
}

Expand Down
10 changes: 6 additions & 4 deletions core/trie/snap_support_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import (
"github.com/stretchr/testify/assert"
)

const trieHeight = 251

func TestRangeAndVerify(t *testing.T) {
scenarios := []struct {
name string
Expand Down Expand Up @@ -96,7 +98,7 @@ func TestRangeAndVerify(t *testing.T) {
assert.NoError(t, err)

for i := 0; i < 10; i++ {
_, err := testTrie.Put(numToFelt(i*100+1), numToFelt(i*100+2))
_, err = testTrie.Put(numToFelt(i*100+1), numToFelt(i*100+2))
assert.NoError(t, err)
}

Expand Down Expand Up @@ -126,7 +128,7 @@ func TestRangeAndVerify(t *testing.T) {
assert.Empty(t, proofs)
}

hasMore, valid, err := trie.VerifyRange(expectedRoot, startQuery, keys, values, proofs, crypto.Pedersen)
hasMore, valid, err := trie.VerifyRange(expectedRoot, startQuery, keys, values, proofs, crypto.Pedersen, trieHeight)
assert.NoError(t, err)
assert.True(t, valid)

Expand Down Expand Up @@ -245,7 +247,7 @@ func TestRangeAndVerifyReject(t *testing.T) {
assert.NoError(t, err)

for i := 0; i < 10; i++ {
_, err := testTrie.Put(numToFelt(i*100+1), numToFelt(i*100+2))
_, err = testTrie.Put(numToFelt(i*100+1), numToFelt(i*100+2))
assert.NoError(t, err)
}

Expand All @@ -268,7 +270,7 @@ func TestRangeAndVerifyReject(t *testing.T) {

keys, values, proofs = scenario.mutator(keys, values, proofs)

_, valid, err := trie.VerifyRange(expectedRoot, startQuery, keys, values, proofs, crypto.Pedersen)
_, valid, err := trie.VerifyRange(expectedRoot, startQuery, keys, values, proofs, crypto.Pedersen, trieHeight)
assert.NoError(t, err)
assert.False(t, valid)
})
Expand Down
Loading