Namefilter and HAMT Implementation and Tests #31
Conversation
- Abstract hasher - Hamt node tests - Hamt serialisation
There was a problem hiding this comment.
HAMT
I have some comments about canonicity. It should be the case that given the same keys and values that your HAMT stores, you should always get back the same root CID, no matter what order the keys were inserted in or which intermediate operations were run. It's some quite tricky logic, but that's what gives us the really strong "if the CID is equal, the tree is equal" logic.
I think we've somewhat diverged from the original codebase now, but we can still take some inspiration. Apparently they just implement a clean() function, maybe that's the way to go for us, too?
https://github.com/filecoin-project/ref-fvm/blob/master/ipld/hamt/src/pointer.rs#L114
Before we put this into production we should definitely get some property tests going for this for sure, but that should be another PR ✌️
Namefilter
Saturation needs a slight change to be correct. It's described in the pseudocode in the spec branch.
Same thing with the property tests applies here, too. These structures have so many interesting mathematical properties, and we should write property tests to check them :) (In another PR)
Codecov Report
@@ Coverage Diff @@
## main #31 +/- ##
==========================================
+ Coverage 77.11% 79.87% +2.75%
==========================================
Files 17 18 +1
Lines 970 1262 +292
==========================================
+ Hits 748 1008 +260
- Misses 222 254 +32
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
There was a problem hiding this comment.
Just a typo and a minor suggestion, but otherwise good to go 👍
I see that we're doing a bunch of values.iter().position(|p| p.key == *key) at the moment, an we've talked about "lookup-by-hash-of-key", so yeah that'd need to change to support that eventually, which seems like a time vs. space trade-off. I think in the worst case it's going from 3 256-byte equality checks to 3 256-byte sha3 + 32-byte equality checks. I'm leaning towards space in this case 🤔
👆 That's me musing about what we've talked about today, not really related to the PR.
| let mut node = (**self).clone(); | ||
| node.pointers[value_index] = Pointer::Link(Link::from(child)); | ||
| if value.is_some() { | ||
| // If something has been deleted, we attempt toc canonicalize the pointer. |
| if value.is_some() { | ||
| // If something has been deleted, we attempt toc canonicalize the pointer. | ||
| if let Some(pointer) = | ||
| Pointer::Link(Link::from(child)).canonicalize(store).await? |
There was a problem hiding this comment.
Hmm. This seems somewhat awkward. Would it help if canonicalize would work on a &mut self and you'd just call it on node.pointers with the child modified?
There was a problem hiding this comment.
I was trying to avoid a situation where mutation happens in a call to another function and doesn't happen in the local scope. I probably took the local mutation thing too far 😅. I don't mind making it an &mut self.
There was a problem hiding this comment.
I will merge this PR but I'm going to add the change to the other PR since I have made some other changes there too.
Summary
Implement the updated HAMT and Namefilter specs. The new spec updates the the saturation and hash algorithm used by the namefilter. Some decisions were also made around what kind of degree WNFS HAMT should have among other things.
This PR implements the following features
Test plan (required)
Testing the Rust core.
cargo test -p wnfs --releaseTesting the wasm bindings.
cd crates/wasmyarn playwright testClosing issues
Fixes #26
Fixes #24