feat: Support Adds in a Deep Subtree #8

Manav-Aggarwal · 2022-10-21T06:13:07Z

We need to support adds in a Deep Subtree, because a transaction being fraud-proven may add keys.

Description of how to do it here: #1 (comment)

Note that there is extra data that needs to go into the proof for this in order to allow IAVL to have the data it needs to do rebalancing.

liamsi · 2022-10-28T09:41:28Z

deepsubtree_test.go

+	fmt.Println("PRINT TREE")
+	_ = printNode(tree.ndb, tree.root, 0)
+	fmt.Println("PRINT TREE END")


Don't use Println in tests. Use t.Log.

Removed Println in tests as other parts of the codebase do: https://github.com/celestiaorg/iavl/blob/9511ba01120acfea75cd903886a55210b80047c8/tree_test.go#L1189

liamsi · 2022-10-28T09:43:49Z

deepsubtree_test.go

+		// Check root hashes are equal
+		require.Equal(dst.root.hash, tree.root.hash)
+	}
+}


As @musalbas mentioned add a bunch of (potentially larger) randomized tests, that:

batch-add

batch-remove

IMO, looking into the SMT tests for inspiration (even if it does not do rebalancing), is a good idea. Add some light fuzzing maybe.

Also, add table-driven tests for each public method.

Documented in #15

deepsubtree.go

liamsi · 2022-10-28T09:46:01Z

deepsubtree.go

-		if !bytes.Equal(key, node.key) {
-			return nil, false, fmt.Errorf("adding new keys is not supported")
+		switch bytes.Compare(key, node.key) {
+		case -1:


add comment to each case

* Export siblings * Add deepsubtree constructor * Support empty root hashes * Use working hash instead of root.hash

deepsubtree.go

tzdybal

I'm approving as this is just a PR for topic branch.

Cleanup suggested by Matt make a lot of sense 👍 , but we may as well revisit it later.

tzdybal · 2022-11-07T09:50:21Z

deepsubtree.go

+}
+
+// Returns the sibling node of a leaf node with given key
+func (tree *ImmutableTree) getSiblingNode(key []byte) (*Node, error) {


This looks kind of redundant. recursiveGetSiblingNode already returns node or error. I guess this may be a leftover after refactoring.

Turned getSiblingNode into an exported method. Cleaner to have a wrapper method with fewer arguments and stay consistent with how Remove and Set have recursiveRemove and recursiveSet as well.

MSevey · 2022-11-08T14:12:27Z

but we may as well revisit it later.

Why not just address it now? Not addressing it now means I wasted my time reviewing and will need to do the same review again in the future which doesn't make sense.

Additionally, addressing reviews helps developers apply those improves to future code, thus further reducing the review burden.

MSevey

Looks like you maybe didn't push the changes? I do see the diff reflecting your comments.

Manav-Aggarwal · 2022-11-09T19:51:50Z

Sorry about that, just pushed the changes.

codecov-commenter · 2022-11-09T19:59:23Z

Codecov Report

❗ No coverage uploaded for pull request base (deepsubtrees@690c59a). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff               @@
##             deepsubtrees       #8   +/-   ##
===============================================
  Coverage                ?   63.48%           
===============================================
  Files                   ?       28           
  Lines                   ?     4869           
  Branches                ?        0           
===============================================
  Hits                    ?     3091           
  Misses                  ?     1369           
  Partials                ?      409

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

MSevey · 2022-11-10T19:02:37Z

deepsubtree.go

+	orphans := dst.prepareOrphansSlice()
+	node.persisted = false
+	newNode, err := dst.balance(node, &orphans)
+	node.persisted = true


Should this be be after the error check?

It doesn't change code behavior, but you're correct, it's cleaner to do the error check right after the call :)

MSevey · 2022-11-10T19:03:43Z

deepsubtree.go

@@ -247,26 +522,40 @@ func (node *Node) getLowestKey() []byte {
 	return lowestKey
 }

-func (dst *DeepSubTree) AddProof(proof *ics23.CommitmentProof) error {
-	proof = ics23.Decompress(proof)
+func (dst *DeepSubTree) AddExistenceProof(existProof *ics23.ExistenceProof) error {


Export should have comment

MSevey

One question and one comment, both can be handled in a follow up. 👍

Manav-Aggarwal · 2022-11-10T20:23:47Z

Closing this, thanks for all the valuable comments @MSevey, @tzdybal, and @liamsi!

* 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 <[email protected]> * return err directly Co-authored-by: Matthew Sevey <[email protected]> * 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ł <[email protected]> Co-authored-by: Matthew Sevey <[email protected]>

* 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 <[email protected]> * return err directly Co-authored-by: Matthew Sevey <[email protected]> * Address linting checks * Use tm-db instead of cosmos-db * Fix linter * Update comment Co-authored-by: Matthew Sevey <[email protected]> * Add error checking for node hash * turn lengthByte into a const * Refactor linkNode * Update comment Co-authored-by: Tomasz Zdybał <[email protected]> Co-authored-by: Matthew Sevey <[email protected]> * 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 <[email protected]> * return err directly Co-authored-by: Matthew Sevey <[email protected]> * 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ł <[email protected]> Co-authored-by: Matthew Sevey <[email protected]> * 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ł <[email protected]> * 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ł <[email protected]> * allocate length Co-authored-by: Tomasz Zdybał <[email protected]> * fix: remove `RangeProofs` (cosmos#586) Co-authored-by: colin axnér <[email protected]> Co-authored-by: Marko <[email protected]> * Update go.mod file * Add new line at end of .golangci.yml Co-authored-by: Tomasz Zdybał <[email protected]> Co-authored-by: Matthew Sevey <[email protected]> Co-authored-by: cool-developer <[email protected]> Co-authored-by: colin axnér <[email protected]> Co-authored-by: Marko <[email protected]>

Manav-Aggarwal linked an issue Oct 21, 2022 that may be closed by this pull request

Support Adds in a Deep Subtree #6

Closed

Manav-Aggarwal self-assigned this Oct 21, 2022

Manav-Aggarwal changed the title ~~Add test to check add operation in deepsubtree: WIP~~ feat: Add test to check add operation in deepsubtree: WIP Oct 21, 2022

Manav-Aggarwal changed the title ~~feat: Add test to check add operation in deepsubtree: WIP~~ feat: Support Adds and Deletes in a Deep Subtree Oct 21, 2022

Manav-Aggarwal force-pushed the manva/add_add_and_delete branch 3 times, most recently from 4b0cf54 to 4f54740 Compare October 27, 2022 13:50

Manav-Aggarwal requested a review from tzdybal October 27, 2022 13:52

Manav-Aggarwal marked this pull request as ready for review October 28, 2022 09:29

Manav-Aggarwal requested review from liamsi, musalbas and MSevey October 28, 2022 09:29

liamsi reviewed Oct 28, 2022

View reviewed changes

Manav-Aggarwal force-pushed the tzdybal/simple_dst branch from 8818904 to 8de40cc Compare October 28, 2022 12:08

Manav-Aggarwal force-pushed the manva/add_add_and_delete branch 2 times, most recently from c8fabd7 to 99c8266 Compare October 28, 2022 12:22

Manav-Aggarwal force-pushed the tzdybal/simple_dst branch from 484218b to 17d57c2 Compare October 28, 2022 12:23

Manav-Aggarwal force-pushed the manva/add_add_and_delete branch from 99c8266 to 9511ba0 Compare October 28, 2022 12:24

tzdybal and others added 12 commits November 1, 2022 16:57

ICS23-style approach for DeepSubTree creation

3b69d3a

Fix deepsubtree, all checks pass

f2b2183

Update documentation for deep subtrees

e2e1698

Fix deepsubtree to accomodate left/right paths both

3312469

Refactor test code

9b9af93

Refactor TestDeepSubtreeStepByStep

632434e

Address comments

cf3e561

Refactor code

9748514

Disable gocritic and unused

97334b4

Add description

865f266

Refactor code to check err in Set

ebabdd1

Modify traversal conditions to be clearer

ce21251

Manav-Aggarwal added 2 commits November 1, 2022 17:29

Change which methods are exported and document exported methods

6623985

feat: Support Empty Hashes and Add constructor (#11)

b12d7d6

* Export siblings * Add deepsubtree constructor * Support empty root hashes * Use working hash instead of root.hash

Manav-Aggarwal force-pushed the manva/add_add_and_delete branch from bdca532 to b12d7d6 Compare November 1, 2022 16:31

Manav-Aggarwal changed the base branch from tzdybal/simple_dst to manav/dst_with_updates November 1, 2022 16:31

Use tm-db instead of cosmos-db

98568c2

MSevey requested changes Nov 3, 2022

View reviewed changes

Manav-Aggarwal changed the base branch from manav/dst_with_updates to deepsubtrees November 3, 2022 21:56

Manav-Aggarwal added 2 commits November 3, 2022 23:19

Merge branch 'deepsubtrees' into manva/add_add_and_delete

a068e86

Return nil in BuildTree

376feb5

Manav-Aggarwal changed the title ~~feat: Support Adds and Deletes in a Deep Subtree~~ feat: Support Adds in a Deep Subtree Nov 4, 2022

tzdybal approved these changes Nov 7, 2022

View reviewed changes

Manav-Aggarwal requested review from tzdybal and MSevey November 9, 2022 05:39

MSevey reviewed Nov 9, 2022

View reviewed changes

Address comments

510010a

Olshansk mentioned this pull request Nov 9, 2022

[Persistence] Evaluate Celestia's Sparse Map Tree implementation pokt-network/pocket#199

Closed

3 tasks

Manav-Aggarwal requested a review from MSevey November 10, 2022 16:18

MSevey reviewed Nov 10, 2022

View reviewed changes

MSevey approved these changes Nov 10, 2022

View reviewed changes

Address more comments

e0630ac

Manav-Aggarwal merged commit 6142e14 into deepsubtrees Nov 10, 2022

Manav-Aggarwal mentioned this pull request Nov 21, 2022

Add randomized tests for adding/removing keys #15

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Support Adds in a Deep Subtree #8

feat: Support Adds in a Deep Subtree #8

Manav-Aggarwal commented Oct 21, 2022 •

edited

Loading

liamsi Oct 28, 2022

Manav-Aggarwal Oct 28, 2022

liamsi Oct 28, 2022

liamsi Oct 28, 2022

Manav-Aggarwal Nov 9, 2022

liamsi Oct 28, 2022

tzdybal left a comment

tzdybal Nov 7, 2022

Manav-Aggarwal Nov 9, 2022 •

edited

Loading

MSevey commented Nov 8, 2022

MSevey left a comment

Manav-Aggarwal commented Nov 9, 2022

codecov-commenter commented Nov 9, 2022

MSevey Nov 10, 2022

Manav-Aggarwal Nov 10, 2022 •

edited

Loading

MSevey Nov 10, 2022

MSevey left a comment

Manav-Aggarwal commented Nov 10, 2022 •

edited

Loading

feat: Support Adds in a Deep Subtree #8

feat: Support Adds in a Deep Subtree #8

Conversation

Manav-Aggarwal commented Oct 21, 2022 • edited Loading

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

tzdybal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manav-Aggarwal Nov 9, 2022 • edited Loading

Choose a reason for hiding this comment

MSevey commented Nov 8, 2022

MSevey left a comment

Choose a reason for hiding this comment

Manav-Aggarwal commented Nov 9, 2022

codecov-commenter commented Nov 9, 2022

Codecov Report

Choose a reason for hiding this comment

Manav-Aggarwal Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSevey left a comment

Choose a reason for hiding this comment

Manav-Aggarwal commented Nov 10, 2022 • edited Loading

Manav-Aggarwal commented Oct 21, 2022 •

edited

Loading

Manav-Aggarwal Nov 9, 2022 •

edited

Loading

Manav-Aggarwal Nov 10, 2022 •

edited

Loading

Manav-Aggarwal commented Nov 10, 2022 •

edited

Loading