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

Make private forest multivalue #78

Merged
merged 6 commits into from
Nov 2, 2022
Merged

Conversation

matheus23
Copy link
Member

@matheus23 matheus23 commented Oct 26, 2022

This PR only changes the data format, but minimally changes the actual data stored. Specifically: We never actually generate multivalues in rs-wnfs. So why add support for them?
The reason is being future-proof: We know we'll make use of multivalues in the future, so we add support for them today, so that today's version of rs-wnfs will have some support for them even when future versions of rs-wnfs actually write multivalues.

In this PR:

  • the PrivateForest type alias changed to use BTreeSet<Cid> as value, instead of just Cid
  • PrivateForest.set was renamed to put, and it adds to the multivalues at a given key, instead of replacing. There's a performance optimization opportunity here to avoid walking the HAMT twice by creating a modify/upsert implementation in hamt/node.rs. But that turned out to be quite some effort, so I'm postponing that for now.
  • PrivateForest.get now expects a resolve_bias function parameter. Today that mostly ends up being BTreeSet::first, in order to fetch the smallest CID. That's the tie-breaking that's specified for WNFS by default, but that will be something different e.g. when we have previous pointers, then we'll bias towards those pointers.
  • PrivateForest.get_encrypted returns the whole BTreeSet now.
  • There's some helper functions in PrivateForest like resolve_lowest, resolve_single and resolve_one_of that are supposed to be helpers for use with PrivateForest.get.

Closes issue: #74

@matheus23 matheus23 self-assigned this Oct 26, 2022
@matheus23 matheus23 requested a review from a team as a code owner October 26, 2022 18:33
@codecov
Copy link

codecov bot commented Oct 26, 2022

Codecov Report

Merging #78 (d1276f1) into main (11b73a7) will increase coverage by 1.58%.
The diff coverage is 35.41%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #78      +/-   ##
==========================================
+ Coverage   64.56%   66.15%   +1.58%     
==========================================
  Files          25       26       +1     
  Lines        1795     1873      +78     
  Branches      458      480      +22     
==========================================
+ Hits         1159     1239      +80     
- Misses        235      240       +5     
+ Partials      401      394       -7     
Impacted Files Coverage Δ
wnfs/common/utils.rs 70.00% <ø> (ø)
wnfs/examples/private.rs 0.00% <0.00%> (ø)
wnfs/private/hamt/hamt.rs 48.57% <0.00%> (ø)
wnfs/private/key.rs 62.50% <0.00%> (+4.16%) ⬆️
wnfs/private/namefilter/bloomfilter.rs 88.57% <0.00%> (+2.85%) ⬆️
wnfs/private/previous.rs 71.26% <0.00%> (+1.14%) ⬆️
wnfs/public/directory.rs 69.96% <0.00%> (+0.70%) ⬆️
wnfs/private/directory.rs 72.82% <33.33%> (+0.16%) ⬆️
wnfs/private/forest.rs 56.33% <45.83%> (-1.85%) ⬇️
wnfs/private/node.rs 69.36% <60.00%> (+2.11%) ⬆️
... and 11 more

wnfs/examples/private.rs Outdated Show resolved Hide resolved
Copy link
Member

@appcypher appcypher 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 to me! 🎉
Only minor fixes to be addressed.

@matheus23 matheus23 merged commit 7a187bc into main Nov 2, 2022
@matheus23 matheus23 deleted the matheus23/private-multivalue branch November 2, 2022 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants