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

[Leaf Counter] Adding a cryptographically secure non-empty leaf counter #46

Merged
merged 20 commits into from
Jun 13, 2024

Conversation

Olshansk
Copy link
Member

@Olshansk Olshansk commented Jun 10, 2024

Summary

  • What? Introduce count to get the number of non-empty leaf nodes in every sub-trie
  • How? Very similar to sum in its implementation
  • Why? Needed to implement Relay Mining and get a reference value for the number of requests, not necessary their cost in terms of price or compute (i.e. compute units, weight, sum, etc...)
  • Other: This needs to be part of the commitment and irrespective of the underlying key-value store engine

Issue

Type of change

Please mark the relevant option(s):

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Documentation
  • Other (specify)

Testing

  • Run all unit tests: make test_all
  • Run all/relevant benchmarks (if optimising): make benchmark_{all | suite name}

Required Checklist

If Applicable Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • I have updated any relevant README(s)/documentation and left TODOs throughout the codebase
  • Add or update any relevant or supporting mermaid diagrams

@Olshansk Olshansk changed the title [WIP] Adding sub-trie leaf count next to sub-trie sum [Leaf Counter] Adding a cryptographically secure non-empty leaf counter Jun 10, 2024
@Olshansk Olshansk self-assigned this Jun 10, 2024
@Olshansk Olshansk added the enhancement New feature or request label Jun 10, 2024
@Olshansk Olshansk requested review from h5law and red-0ne June 10, 2024 03:16
@Olshansk Olshansk marked this pull request as ready for review June 10, 2024 03:21
Copy link
Contributor

@red-0ne red-0ne left a comment

Choose a reason for hiding this comment

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

I wish it would be possible to order the files of a PR to review but I finally got how non-empty leaf count is being added 😄

Left some change requests but look good overall.

types.go Outdated Show resolved Hide resolved
smst.go Outdated Show resolved Hide resolved
hasher.go Outdated Show resolved Hide resolved
smst.go Outdated Show resolved Hide resolved
Olshansk and others added 4 commits June 11, 2024 13:38
Co-authored-by: Redouane Lakrache <[email protected]>
Signed-off-by: Daniel Olshansky <[email protected]>
Co-authored-by: Redouane Lakrache <[email protected]>
Signed-off-by: Daniel Olshansky <[email protected]>
Co-authored-by: Redouane Lakrache <[email protected]>
Signed-off-by: Daniel Olshansky <[email protected]>
@Olshansk Olshansk requested a review from red-0ne June 11, 2024 20:42
Copy link
Collaborator

@h5law h5law left a comment

Choose a reason for hiding this comment

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

Looks good left a few comments, some suggestions on how count could be computed as a *big.Int instead which would result in variable digest lengths which would probably aid security.

docs/merkle-sum-trie.md Outdated Show resolved Hide resolved
hasher.go Outdated Show resolved Hide resolved
node_encoders.go Outdated Show resolved Hide resolved
node_encoders.go Show resolved Hide resolved
proofs.go Outdated Show resolved Hide resolved
smst.go Outdated Show resolved Hide resolved
smst.go Outdated Show resolved Hide resolved
smst.go Outdated Show resolved Hide resolved
smst.go Outdated Show resolved Hide resolved
smst_test.go Outdated Show resolved Hide resolved
Olshansk and others added 3 commits June 12, 2024 13:25
Co-authored-by: h5law <[email protected]>
Signed-off-by: Daniel Olshansky <[email protected]>
Co-authored-by: h5law <[email protected]>
Signed-off-by: Daniel Olshansky <[email protected]>
Co-authored-by: h5law <[email protected]>
Signed-off-by: Daniel Olshansky <[email protected]>
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 87.17949% with 20 lines in your changes missing coverage. Please review.

Project coverage is 82.96%. Comparing base (e3dbbbd) to head (0e03c57).
Report is 10 commits behind head on main.

Current head 0e03c57 differs from pull request most recent head e620a2b

Please upload reports for the commit e620a2b to get more accurate results.

Files Patch % Lines
trie_spec.go 66.66% 10 Missing ⚠️
proofs.go 76.92% 6 Missing ⚠️
smst.go 90.00% 2 Missing and 2 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #46      +/-   ##
==========================================
+ Coverage   81.31%   82.96%   +1.65%     
==========================================
  Files           9       14       +5     
  Lines        1493     1691     +198     
==========================================
+ Hits         1214     1403     +189     
- Misses        215      225      +10     
+ Partials       64       63       -1     

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

@Olshansk Olshansk requested a review from h5law June 12, 2024 20:31
Copy link
Collaborator

@h5law h5law left a comment

Choose a reason for hiding this comment

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

Going to approve - left a few comments but going to unblock. LGTM :shipit:

docs/merkle-sum-trie.md Outdated Show resolved Hide resolved
smst.go Outdated Show resolved Hide resolved
smst.go Outdated Show resolved Hide resolved
smst_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@red-0ne red-0ne left a comment

Choose a reason for hiding this comment

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

Perfect 👌

Olshansk and others added 3 commits June 12, 2024 20:49
Co-authored-by: h5law <[email protected]>
Signed-off-by: Daniel Olshansky <[email protected]>
Co-authored-by: h5law <[email protected]>
Signed-off-by: Daniel Olshansky <[email protected]>
@Olshansk Olshansk merged commit cece51c into main Jun 13, 2024
2 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
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants