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

feat: add ProveClosest method to SMT and SMST #18

Merged
merged 15 commits into from
Sep 25, 2023
Merged

feat: add ProveClosest method to SMT and SMST #18

merged 15 commits into from
Sep 25, 2023

Conversation

h5law
Copy link
Collaborator

@h5law h5law commented Aug 17, 2023

Description

Summary generated by Reviewpad on 25 Sep 23 17:51 UTC

This pull request includes the following changes:

  • The file diff for file example.go includes the addition of a TODO comment to research whether the SiblingData is required and remove it if not.

  • The file diff for file smst.go includes the addition of a new method called ProveClosest in the SMST struct. This method generates a SparseMerkleProof of inclusion for a given key, using the most common bits as the provided path. It returns the closest path, closest value hash, closest sum, the proof, and any error encountered during the process. Additionally, the Prove and Commit methods remain unchanged in this file.

  • The file diff for file types.go includes the addition of two new methods ProveClosest in the SparseMerkleTree and SparseMerkleSumTree interfaces. These methods compute a Merkle proof of inclusion for a key in the tree that is closest to the provided path. The search for the closest key is based on the longest common prefix and the key with the most common bits as the provided path.

  • The file diff for file hash.go includes the addition of an import statement for the "hash" package and a new function named "NoPrehashSpec" which returns a new TreeSpec with a nil Value Hasher and a nil Path Hasher. This function should only be used when values are already hashed and a path is used instead of a key during proof verification.

  • The file diff for file smt.go includes the addition of a new method ProveClosest in the SMST struct, which generates a SparseMerkleProof of inclusion for the first key with the most common bits as the path provided. It also includes updates to the Prove method to return a proof object, the addition of the recursiveLoad method to recursively load a hash, the addition of the split method to split an extension node, and the addition of the expand method to return the inner node that represents the start of the singly linked list that an extension node represents.

  • The file diff for file smst_proofs_test.go includes the addition of new test functions: TestSMST_ProveClosest, TestSMST_ProveClosestEmptyAndOneNode. These test the closest value proof in a sparse Merkle sum tree and test the closest value proof with an empty tree and a tree with a single node, respectively. Various updates and modifications were also made to the existing code and test cases in this file.

  • The file diff for file utils.go includes the addition of a new type nilPathHasher with methods for a path and path size, a new function newNilPathHasher, a comment indicating that the GetPathBit function should be unexported, and a new function flipPathBit to flip the bit at a given position in the data. The setPathBit function was also updated to use bitwise XOR for flipping the bit at a given position. No changes were made to the countSetBits function.

  • The file diff for file smt_proofs_test.go includes updates to the test cases in this file. A new test case TestSMT_ProveClosest was added to validate the closest key and value hash for a given path in the sparse Merkle tree. Another test case TestSMT_ProveClosestEmptyAndOneNode was added to test the ProveClosest function when the tree is empty or contains only one node. Some helper functions and variables were also added to support the new test cases.

Please review these changes in the code.

Issue

Fixes N/A

Part of the work related to relay mining: paper can be viewed here

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

  • Adds ProveClosest method to SMT
  • Adds ProveClosest method to SMST
  • Adds unit tests

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

@h5law h5law added the enhancement New feature or request label Aug 17, 2023
@h5law h5law requested review from Olshansk and adshmh August 17, 2023 20:25
@h5law h5law self-assigned this Aug 17, 2023
@reviewpad reviewpad bot added medium Pull request is medium waiting-for-review This PR is currently waiting to be reviewed labels Aug 17, 2023
@codecov
Copy link

codecov bot commented Aug 17, 2023

Codecov Report

Attention: 18 lines in your changes are missing coverage. Please review.

Comparison is base (a8abd2d) 84.69% compared to head (c0eba42) 84.77%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #18      +/-   ##
==========================================
+ Coverage   84.69%   84.77%   +0.07%     
==========================================
  Files           8        8              
  Lines        1117     1241     +124     
==========================================
+ Hits          946     1052     +106     
- Misses        126      138      +12     
- Partials       45       51       +6     
Files Coverage Δ
options.go 100.00% <100.00%> (ø)
proofs.go 88.46% <ø> (ø)
types.go 93.02% <ø> (ø)
utils.go 94.28% <100.00%> (+0.47%) ⬆️
smst.go 91.17% <75.00%> (-3.47%) ⬇️
smt.go 80.03% <84.53%> (+0.93%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Olshansk
Copy link
Member

Olshansk commented Sep 5, 2023

@h5law I'm going to review this this week. an you merge it with main?

I wanted to know if you still have your visualizer handy or accessible? Could be good for a visual check.

Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@h5law Took a first review, but will also take a second one tomorrow.

I think this is one of the MOST critical pieces of v1, so would really appreciate a few more comments through the implementation.

options.go Show resolved Hide resolved
smt.go Show resolved Hide resolved
smt.go Outdated Show resolved Hide resolved
smt.go Show resolved Hide resolved
smt.go Outdated Show resolved Hide resolved
smt.go Show resolved Hide resolved
types.go Outdated Show resolved Hide resolved
smt.go Show resolved Hide resolved
smt_proofs_test.go Outdated Show resolved Hide resolved
smt_proofs_test.go Outdated Show resolved Hide resolved
@h5law h5law requested a review from Olshansk September 11, 2023 14:13
@reviewpad reviewpad bot added large Pull request is large and removed medium Pull request is medium labels Sep 22, 2023
@h5law
Copy link
Collaborator Author

h5law commented Sep 22, 2023

@Olshansk this PR should be good to go now ready for final reviews

options.go Outdated Show resolved Hide resolved
utils.go Outdated Show resolved Hide resolved
utils.go Show resolved Hide resolved
smt.go Outdated Show resolved Hide resolved
smt.go Show resolved Hide resolved
smst_proofs_test.go Outdated Show resolved Hide resolved
smst_proofs_test.go Outdated Show resolved Hide resolved
smst_proofs_test.go Outdated Show resolved Hide resolved
smst_proofs_test.go Show resolved Hide resolved
smt_proofs_test.go Show resolved Hide resolved
@h5law
Copy link
Collaborator Author

h5law commented Sep 24, 2023

@Olshansk I addressed the comments and replied to some of the outstanding ones with reasonings, there are a few PRs that should come out of this one:

  1. Look into removing SiblingData from proofs
  2. Unexport GetPathBit

Otherwise I think the comments should clear things up that may have been unclear. I removed the original test cases in favour of the visual ones as what we are forcing is backstepping logic. Otherwise the proof functions the same and this logic is covered elsewhere.

@h5law h5law requested a review from Olshansk September 24, 2023 22:25
smt.go Show resolved Hide resolved
smt.go Show resolved Hide resolved
smt.go Show resolved Hide resolved
smt.go Show resolved Hide resolved
smt_proofs_test.go Outdated Show resolved Hide resolved
@h5law h5law requested a review from Olshansk September 25, 2023 12:29
@Olshansk
Copy link
Member

@h5law Seems like this is out of date with the main branch and might need to be rebased or merged.

@Olshansk
Copy link
Member

Look into removing SiblingData from proofs
Unexport GetPathBit

Leave TODOs next to the structure/function with next steps. It's the most lightweight way to keep track of work (no tickets, etc) without losing context if both of us are not around.

Olshansk
Olshansk previously approved these changes Sep 25, 2023
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@h5law h5law merged commit c5998f1 into main Sep 25, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request large Pull request is large waiting-for-review This PR is currently waiting to be reviewed
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants