[Enhancement] Add Merkle Sum tree functionality (SMT Wrapper)

[h5law]

Description

Summary generated by Reviewpad on 29 Jun 23 08:49 UTC

This pull request includes changes to multiple files in the repository. Here is a summary of the changes:

1. In the smt_testutil.go file:

• Renamed the file to smt_utils_test.go.
• Modified the SMTWithStorage struct to improve code organization and add comments.
• Modified the Update method to improve error handling and include comments.
• Simplified error handling in the Delete method and added comments.
• Added comments and improved error handling in the ProveCompact function.

2. In the hasher.go file:

• Added the encoding/binary package import.
• Added several functions related to hashing and encoding nodes in the sum tree.

3. In the types.go file:

• Added constants and variables related to sum tree operations.
• Added a new interface SparseMerkleSumTree and methods for updating, deleting, getting, and computing the Merkle root and sum.
• Modified the SparseMerkleTree interface's Prove method to return a pointer to the SparseMerkleProof struct.
• Added new methods for serializing and hashing nodes in the sum tree.

4. In the smt.go file:

• Renamed the test functions in the smt_test.go file to improve readability.
• Added imports for required packages.
• Updated the VerifySumProof and VerifyCompactSumProof functions for sum tree proofs.
• Modified functions related to proof serialization and hashing.

5. In the README.md file:

• Modified the indentation of subheadings and sections.
• Added new subheadings and updated existing sections.

6. In the proofs.go file:

• Added an import statement and functions related to verifying and generating Merkle proofs.
• Modified function signatures and return types for various functions.

7. In the smt_proofs_test.go file:

• Renamed the test function and modified variable types.
• Added a new test function and initialized variables.

8. In the utils.go file:

• Added utility functions for different types of tree nodes.

9. In the tree.go file:

• Added a new method and field to the treeNode interface.
• Modified function and method signatures.
• Added new functions related to the SMT struct.

10. In the licenses file:

• Updated the copyright holders in the license.

11. In the bulk_test.go file:

• Added package imports and restructured the code for better organization.

12. In a new file implementing a Sparse Merkle Sum Tree (SMST):

• Added functions for creating, importing, updating, deleting, and generating proofs for an SMST.

13. In a file related to proof generation:

• Renamed variables and modified functions to improve randomness and compactness of proofs.

14. In a file with renamed variables and types:

• Changed variable and type names, updated function arguments.

Please review these changes and ensure they meet the requirements of the codebase.

Issue

Fixes #10

Type of change

Please mark the relevant option(s):

• New feature, functionality or library
• Bug fix
• Code health or cleanup
• Major breaking change
• Documentation
• Other

List of changes

• Implement the SMST to wrap the current SMT and encode sums into the values
• Add unit tests covering get,update,delete,prove,verify,sum

Testing

• Task specific tests or benchmarks: go test ...
• New tests or benchmarks: go test ...
• All tests: go test -v

Required Checklist

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added, or updated, godoc format comments on touched members (see: tip.golang.org/doc/comment)
• I have tested my changes using the available tooling

If Applicable Checklist

• Update any relevant README(s)
• Add or update any relevant or supporting mermaid diagrams
• I have added tests that prove my fix is effective or that my feature works

[codecov]

Codecov Report

Patch coverage: 87.41% and project coverage change: +2.25 🎉

Comparison is base (cc555d9) 84.53% compared to head (9a41773) 86.78%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #13      +/-   ##
==========================================
+ Coverage   84.53%   86.78%   +2.25%     
==========================================
  Files           8        8              
  Lines         776      984     +208     
==========================================
+ Hits          656      854     +198     
- Misses         85       94       +9     
- Partials       35       36       +1     

Impacted Files	Coverage Δ
smt.go	79.10% <62.50%> (-1.82%)	⬇️
proofs.go	90.44% <84.44%> (-2.79%)	⬇️
utils.go	93.02% <88.67%> (-6.98%)	⬇️
types.go	93.02% <92.50%> (+5.78%)	⬆️
smst.go	94.64% <94.64%> (ø)
hasher.go	100.00% <100.00%> (ø)
options.go	100.00% <100.00%> (+50.00%)	⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[Olshansk]

Did a partial review and will finish it tomorrow

[Olshansk]

Shouldn't this be not default AND sum != 0?

[h5law]

!(defaultValue && sum == 0) == !defaultValue || sum != 0 (de morgans law)?

[h5law]

@Olshansk Looking into this more from this example we have the following current functionality:

package main

import (
	"crypto/sha256"
	"fmt"

	"github.com/pokt-network/smt"
)

func main() {
	nodeStore := smt.NewSimpleMap()
	tree := smt.NewSparseMerkleSumTree(nodeStore, sha256.New())
	tree.Update([]byte("key"), []byte(nil), 5)
	tree.Update([]byte("key2"), []byte("val2"), 5)
	tree.Update([]byte("key3"), []byte("val3"), 5)
	tree.Update([]byte("key4"), []byte("val4"), 5)
	root := tree.Root()
	proof, err := tree.Prove([]byte("key"))
	if err != nil {
		panic(err)
	}
	if valid := smt.VerifySumProof(proof, root, []byte("key"), nil, 5, tree.Spec()); valid { // valid
		fmt.Println("valid")
	} else {
		fmt.Println("invalid")
	}

	nodeStore2 := smt.NewSimpleMap()
	tree2 := smt.NewSparseMerkleTree(nodeStore2, sha256.New())
	tree2.Update([]byte("key"), []byte(nil))
	tree2.Update([]byte("key2"), []byte("val2"))
	tree2.Update([]byte("key3"), []byte("val3"))
	tree2.Update([]byte("key4"), []byte("val4"))
	root2 := tree2.Root()
	proof2, err := tree2.Prove([]byte("key"))
	if err != nil {
		panic(err)
	}
	if valid := smt.VerifyProof(proof2, root2, []byte("key"), nil, tree2.Spec()); valid { // invalid
		fmt.Println("valid")
	} else {
		fmt.Println("invalid")
	}
}

This makes me think that although we can set a nil value in both trees it will still be a placeholder. For the SMST this is true ONLY if we do not give it a sum. This aligns with the check in the Has function as if either the sum or the value are set the key is in the tree and is not a placeholder

[Olshansk]

if either the sum or the value are set the key is in the tree and is not a placeholder

I feel like this implies that we can set a value to be empty (i.e. the placeholder) with a non-zero weight/sum. I can potentially see arguments for it, but feel like it creates room for error/confusion.

In your example above, what I'm seeing is:

1. Empty key with weight -> valid proof
2. Empty key w/o weight -> invalid proof
3. Empty key with weight -> has=True
4. Empty key w/o weight -> has=False

I'm open to be swayed in the other direction, but I think that empty (i.e. placeholder) leaves should not be able to have a weight.

[h5law]

@Olshansk I think we should allow ppl to set nil keys as they exist anyway, setting a key to nil is basically a noop, it does change the tree but the key is still a placeholder and this doesn't "count" as being in the tree. When we add a weight suddenly it does count as being in the tree as its not the default value anymore.

With this in mind I think the current functionality is correct. Weird edge case that nobody will probably utilise but I think the logic is sound. If the key is default in every then its not in the tree (placeholder) but if it has a value and 0 sum - in the tree, if it has no value but has a sum - in the tree. Think about the reverse should a key-value pair with a 0 sum be a placeholder?

[Olshansk]

Makes sense to me. On this note, what do you think of adding a new section in the docs.

Stupid question: You mentioned nil keys, but are we not talking about nil values?

	...
	tree2.Update([]byte("key"), []byte(nil))
	...
	tree.Update([]byte("key"), []byte(nil), 5)
	...

# Nil Values

A `Nil` value is treated as a `noop` as it utilizes the placeholder value in the SMT.

Assume `(key, value, weight)` pairs as follows:
* `(key, nil, 0)` -> DOES NOT modify the `root` hash -> noop
* `(key, nil, > 0)` -> DOES modify the `root` hash
* `(key, value, 0)` -> DOES modify the `root` hash
* `(key, value, > 0)` -> DOES modify the `root` hash

[h5law]

Actually all of these will change the root from the default placeholder [32]byte which may be confusing. I think this is because they are all actually creating a new leaf node thus changing the tree. However depending on the leaf's value its either the same as a placeholder leaf or not and thus doesn't count as being included in the tree.

[Olshansk]

@h5law Other than this one comment, which we just need to discuss and gree on, I'm ready to approve. #13 (comment)

Is there anything else you want to do in this PR?

[Olshansk]

Left one last comment: #13 (comment)

Leaving the call to you if we sholdn't or shouldn't update the docs, but feel free to merge regardless

[reviewpad]

Reviewpad Report

Fatal

• License files cannot be modified