fix: remove `RangeProofs` #586

cool-develope · 2022-10-11T22:40:57Z

Closes #584
Closes #284

remove un

cool-develope · 2022-10-11T22:43:51Z

@tac0turtle, this is the initial PR, please review it. If it is OK for you, I will remove the unnecessary ones.
Benchmark results

# original
goos: linux
goarch: amd64
pkg: github.com/cosmos/iavl
cpu: Intel(R) Core(TM) i5-9400 CPU @ 2.90GHz
BenchmarkGetNonMembership/fast-6                    1000             76308 ns/op            8743 B/op        124 allocs/op
BenchmarkGetNonMembership/regular-6                 1000           4523531 ns/op         1324787 B/op      22035 allocs/op

# refactored
goos: linux
goarch: amd64
pkg: github.com/cosmos/iavl
cpu: Intel(R) Core(TM) i5-9400 CPU @ 2.90GHz
BenchmarkGetNonMembership/fast-6                    1000             76010 ns/op            8390 B/op        115 allocs/op
BenchmarkGetNonMembership/regular-6                 1000           4417003 ns/op         1324191 B/op      22028 allocs/op```

tac0turtle · 2022-10-12T19:49:38Z

cc @AdityaSripal @colin-axner

colin-axner

Thanks for cleaning up the code! I don't understand the leftMost addition, it seems unnecessary and potentially incorrect? Could you explain that to me? I haven't looked into the non membership proof code yet

proof_ics23.go

proof.go

tac0turtle · 2022-10-13T20:30:48Z

could we remove the proto messages https://github.com/cosmos/iavl/blob/master/proto/iavl/proof.proto for things that aren't needed

colin-axner

LGTM see minor comments, mostly just reviewed the ics23 changes. It might make sense to split this pr up? It seems to me you could have a pr fixing up the ics23 file and then a followup pr which removes all the unnecessary range proof stuff

I'm just a fan of splitting up prs since it is easier to feel confident approving less code diffs at a time

Also would appreciate a few approvals on these changes

proof_ics23.go

colin-axner · 2022-10-14T11:22:22Z

proof_ics23.go

+	value, err := t.Get(key)
+	if err != nil {
+		return nil, err
+	}
+	if value != nil {
+		return t.GetMembershipProof(key)
+	}
+	return t.GetNonMembershipProof(key)


Is this correct? I think a non membership proof is about the key not existing, not the value. If the key doesn't exist, we will return an err on 189 instead of a non membership proof

OK, I added isExist param to proof of both types (existence and non-existence).

Seems like the correct fix is:

value, err := t.Get(key) if value != nil { return t.GetMembershipProof(key) } return t.GetNonMembershipProof(key)

colin-axner · 2022-10-17T14:31:53Z

proof_ics23.go

+// GetProof gets the proof for the given key.
+func (t *ImmutableTree) GetProof(key []byte, isExist bool) (*ics23.CommitmentProof, error) {
 	if t.root == nil {
 		return nil, fmt.Errorf("cannot generate the proof with nil root")
 	}
-	value, err := t.Get(key)
-	if err != nil {
-		return nil, err
-	}
-	if value != nil {
+	if isExist {


sorry for not being more specific, I think you can use the Has function

exist, err := t.Has(key) if err != nil { return nil, err } if exist { return t.GetMembershipProof(key) } return t.GetNonMembershipProof(key)

yeah, it's a simple solution but I'd like to add isExist param to be more clear.

I guess when the SDK queries for this value, it should be performing the Has query then?

yeah, I think it would be related to each use case, here my purpose is to reduce checks in the GetProof function.

colin-axner

Nice work! Love to see this cleanup! Would appreciate at least another approval from someone familiar with ics23

proof_test.go

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

cool-develope · 2022-10-17T14:48:28Z

Nice work! Love to see this cleanup! Would appreciate at least another approval from someone familiar with ics23

cc: @tac0turtle

AdityaSripal

Changes look good. I have one remaining question about the API that can be addressed in subsequent PR.

Thanks for the cleanup!

AdityaSripal · 2022-10-18T16:29:33Z

proof_ics23.go

@@ -166,3 +178,34 @@ func convertVarIntToBytes(orig int64, buf [binary.MaxVarintLen64]byte) []byte {
 	n := binary.PutVarint(buf[:], orig)
 	return buf[:n]
 }
+
+// GetProof gets the proof for the given key.
+func (t *ImmutableTree) GetProof(key []byte, isExist bool) (*ics23.CommitmentProof, error) {


Seems weird to me that we would expose the API to take in an isExist bool.

Doesn't it make more sense to just check if key exists in the tree.

If it does, return an existence proof otherwise return a nonexistence proof

cool-develope · 2022-10-18T16:54:35Z

OK, I removed the isExist param in GetProof, because we have already GetMembershipProof and GetNonMembershipProof.
cc: @AdityaSripal @colin-axner

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> Co-authored-by: Marko <marbar3778@yahoo.com>

* feat: Deep tree structure with Updates (#13) * ICS23-style approach for DeepSubTree creation * Fix deepsubtree, all checks pass * Update documentation for deep subtrees * Fix deepsubtree to accomodate left/right paths both * Refactor test code * Refactor TestDeepSubtreeStepByStep * Address comments * Refactor code * Disable gocritic and unused * Add description * Refactor code to check err in Set * Modify traversal conditions to be clearer * feat: build deep subtree from ICS23 inclusion proofs (#9) * feat: build deep subtree from ICS23 inclusion proofs * feat: handle non-existence proofs * linter: goimports deepsubtree.go * refactor: addExistenceProofProof -> addExistenceProof * fix: un-hardcode size of byte array * Add more comments * Refactor ndb.Commit call outside for loop * verify that in the case that dst.root != nil that the root node hash matches the provided hash and check dst.root != nil first * Add strings and use strings.Repeat * Delete addPath and AddPath * Remove print statements * Refactor recomputeHash usage Co-authored-by: Matthew Sevey <mjsevey@gmail.com> * return err directly Co-authored-by: Matthew Sevey <mjsevey@gmail.com> * Address linting checks * Use tm-db instead of cosmos-db * Fix linter * Update comment Co-authored-by: Matthew Sevey <mjsevey@gmail.com> * Add error checking for node hash * turn lengthByte into a const * Refactor linkNode * Update comment Co-authored-by: Tomasz Zdybał <tomek@zdybal.lap.pl> Co-authored-by: Matthew Sevey <mjsevey@gmail.com> * feat: Support Adds in a Deep Subtree (#8) * ICS23-style approach for DeepSubTree creation * Fix deepsubtree, all checks pass * Update documentation for deep subtrees * Fix deepsubtree to accomodate left/right paths both * Refactor test code * Refactor TestDeepSubtreeStepByStep * Address comments * Refactor code * Disable gocritic and unused * Add description * Refactor code to check err in Set * Modify traversal conditions to be clearer * feat: build deep subtree from ICS23 inclusion proofs (#9) * feat: build deep subtree from ICS23 inclusion proofs * feat: handle non-existence proofs * linter: goimports deepsubtree.go * refactor: addExistenceProofProof -> addExistenceProof * fix: un-hardcode size of byte array * Add more comments * Refactor ndb.Commit call outside for loop * verify that in the case that dst.root != nil that the root node hash matches the provided hash and check dst.root != nil first * Add strings and use strings.Repeat * Delete addPath and AddPath * Remove print statements * Refactor recomputeHash usage Co-authored-by: Matthew Sevey <mjsevey@gmail.com> * return err directly Co-authored-by: Matthew Sevey <mjsevey@gmail.com> * Address linting checks * Use tm-db instead of cosmos-db * Fix linter * Add test to check add operation in deepsubtree: WIP * Add insert key functionality WIP * Try replicating non-inclusion proofs * Add sibling nodes to nonExistenceProofs * Refactor test code * Finish adding add functionality to dst * Add Remove functionality to dst * Fix linting errors * Fix GetSiblingNode * Remove more print statements * Add comment for each case in recursive set * Change which methods are exported and document exported methods * feat: Support Empty Hashes and Add constructor (#11) * Export siblings * Add deepsubtree constructor * Support empty root hashes * Use working hash instead of root.hash * Use tm-db instead of cosmos-db * Return nil in BuildTree * Address comments * Address more comments Co-authored-by: Tomasz Zdybał <tomek@zdybal.lap.pl> Co-authored-by: Matthew Sevey <mjsevey@gmail.com> * Add randomized tests for adding/removing keys (#16) * Add go fuzz tests * Add membership proof for existing keys * Build tree after adding membership proof * Make batch add fuzz tests work * Do not commit version twice for dst * Save version out of dst.Set condition * Set rootHash to workingHash * Fix edge cases * Refactor DST Non-Existence Proof * Change cacheSize * Add test data and sibling nodes for each node in path * Fix GetSiblingNodes * Add more test data * testing: fuzz: failing test case * Use set for keys * Add more test data * Refactor existence proofs code * Add required info for remove operation * Add children of siblings as well * Remove debug code * Add testdata that breaks current code * Fix bug * Add failing testcase * Add breaking testcase * IAVL with tracing * Fuzz tests pass * Refactor tracing code * Remove redundant code * Remove working hash in calls to AddExistenceProof * Clean up flag * Make build tree a private method * Add back whitespace in node.go * Add new ci for fuzz * Refactor more * Refactor out getKey method * Change name to reapInclusionProofs * Refactor set and remove in DST * Factor out add existence proofs from Remove DST for consistency with Set * Refactor into testContext * Clean up setInDST Co-authored-by: Tomasz Zdybał <tomek@zdybal.lap.pl> * Add tracing to Trees and Deep Subtrees (#18) * Add go fuzz tests * Add membership proof for existing keys * Build tree after adding membership proof * Make batch add fuzz tests work * Do not commit version twice for dst * Save version out of dst.Set condition * Set rootHash to workingHash * Fix edge cases * Refactor DST Non-Existence Proof * Change cacheSize * Add test data and sibling nodes for each node in path * Fix GetSiblingNodes * Add more test data * testing: fuzz: failing test case * Use set for keys * Add more test data * Refactor existence proofs code * Add required info for remove operation * Add children of siblings as well * Remove debug code * Add testdata that breaks current code * Fix bug * Add failing testcase * Add breaking testcase * IAVL with tracing * Fuzz tests pass * Refactor tracing code * Remove redundant code * Remove working hash in calls to AddExistenceProof * Clean up flag * Make build tree a private method * Add back whitespace in node.go * Add new ci for fuzz * Refactor more * Refactor out getKey method * Change name to reapInclusionProofs * Refactor set and remove in DST * Factor out add existence proofs from Remove DST for consistency with Set * Refactor into testContext * Clean up setInDST * Add method for get * Export methods * Add witness data to deep subtree * Verify operation in witness data * Refactor and verify operation for get and remove * Add set witness data * Add tracing to tree * add getter for witness data * Verify existence proofs in dst * Cleanup * Reset witness data on tracing enabled * Add node to keysAccessed even not in cache * Add initial root hash * Refactor GetInitialRootHash * Modify GetInitialRootHash * Add test data * Add get to dst tests: fails right now * Refactor and add tracing for root key as well * Add docs * Add more docs * Update comments * Update log message Co-authored-by: Tomasz Zdybał <tomek@zdybal.lap.pl> * allocate length Co-authored-by: Tomasz Zdybał <tomek@zdybal.lap.pl> * fix: remove `RangeProofs` (cosmos#586) Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> Co-authored-by: Marko <marbar3778@yahoo.com> * Update go.mod file * Add new line at end of .golangci.yml Co-authored-by: Tomasz Zdybał <tomek@zdybal.lap.pl> Co-authored-by: Matthew Sevey <mjsevey@gmail.com> Co-authored-by: cool-developer <51834436+cool-develope@users.noreply.github.com> Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> Co-authored-by: Marko <marbar3778@yahoo.com>

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> Co-authored-by: Marko <marbar3778@yahoo.com>

cool-develope added 2 commits October 11, 2022 17:20

remove ProofRange

01890c8

remove un

refactor proof

8dce332

cool-develope requested a review from a team as a code owner October 11, 2022 22:40

cool-develope marked this pull request as draft October 11, 2022 22:41

cool-develope changed the title ~~Remove ProofRange~~ fix: remove ProofRange Oct 12, 2022

cool-develope changed the title ~~fix: remove ProofRange~~ fix: remove RangeProofs Oct 12, 2022

colin-axner reviewed Oct 13, 2022

View reviewed changes

remove unnecessary ones

91ad406

cool-develope force-pushed the 584/remove-proofrange branch from fc62611 to 91ad406 Compare October 13, 2022 19:39

Merge branch 'master' into 584/remove-proofrange

7db6e22

cool-develope requested a review from colin-axner October 13, 2022 19:44

cool-develope marked this pull request as ready for review October 13, 2022 19:44

cool-develope added 2 commits October 13, 2022 15:59

small fix

a87b7b6

test fix

31bab40

remove proto

7a7dfca

tzdybal mentioned this pull request Oct 14, 2022

feat: Create deep tree structure using an ICS23-style approach: Updates rollkit/iavl#5

Closed

6 tasks

colin-axner reviewed Oct 14, 2022

View reviewed changes

changelog

bd9353b

cool-develope requested a review from colin-axner October 14, 2022 12:12

Merge branch 'master' into 584/remove-proofrange

87cbe24

colin-axner reviewed Oct 17, 2022

View reviewed changes

colin-axner approved these changes Oct 17, 2022

View reviewed changes

proof_test.go Outdated Show resolved Hide resolved

Update proof_test.go

3f1d232

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

tac0turtle requested a review from ebuchman October 17, 2022 21:38

AdityaSripal approved these changes Oct 18, 2022

View reviewed changes

p0mvn approved these changes Oct 18, 2022

View reviewed changes

revert the isExist

9c9915c

cool-develope requested review from AdityaSripal and colin-axner October 18, 2022 16:54

Merge branch 'master' into 584/remove-proofrange

fa0d2b3

tac0turtle enabled auto-merge (squash) October 19, 2022 08:03

tac0turtle merged commit 401725a into master Oct 19, 2022

tac0turtle deleted the 584/remove-proofrange branch October 19, 2022 08:07

Manav-Aggarwal pushed a commit to rollkit/iavl that referenced this pull request Jan 5, 2023

fix: remove RangeProofs (cosmos#586)

b53ed43

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> Co-authored-by: Marko <marbar3778@yahoo.com>

ulbqb pushed a commit to ulbqb/iavl that referenced this pull request Jun 12, 2023

fix: remove RangeProofs (cosmos#586)

0a27ee1

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> Co-authored-by: Marko <marbar3778@yahoo.com>

ulbqb added a commit to ulbqb/iavl that referenced this pull request Jun 12, 2023

Revert "fix: remove RangeProofs (cosmos#586)"

ea54cd6

coderabbitai bot mentioned this pull request May 13, 2024

chore: changelog onto master #946

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: remove `RangeProofs` #586

fix: remove `RangeProofs` #586

cool-develope commented Oct 11, 2022

cool-develope commented Oct 11, 2022 •

edited

Loading

tac0turtle commented Oct 12, 2022

colin-axner left a comment

tac0turtle commented Oct 13, 2022

colin-axner left a comment

colin-axner Oct 14, 2022

cool-develope Oct 14, 2022

AdityaSripal Oct 18, 2022

colin-axner Oct 17, 2022

cool-develope Oct 17, 2022

colin-axner Oct 17, 2022

cool-develope Oct 17, 2022

colin-axner left a comment

cool-develope commented Oct 17, 2022

AdityaSripal left a comment

AdityaSripal Oct 18, 2022

cool-develope commented Oct 18, 2022

fix: remove RangeProofs #586

fix: remove RangeProofs #586

Conversation

cool-develope commented Oct 11, 2022

cool-develope commented Oct 11, 2022 • edited Loading

tac0turtle commented Oct 12, 2022

colin-axner left a comment

Choose a reason for hiding this comment

tac0turtle commented Oct 13, 2022

colin-axner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner left a comment

Choose a reason for hiding this comment

cool-develope commented Oct 17, 2022

AdityaSripal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cool-develope commented Oct 18, 2022

fix: remove `RangeProofs` #586

fix: remove `RangeProofs` #586

cool-develope commented Oct 11, 2022 •

edited

Loading