Add support for HamtV0 from forest(commit hash: b622af5a6) #1808
Conversation
creativcoder
changed the title
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #1808 +/- ##
==========================================
- Coverage 75.31% 70.40% -4.92%
==========================================
Files 149 101 -48
Lines 14558 8524 -6034
==========================================
- Hits 10965 6001 -4964
+ Misses 3593 2523 -1070
|
ipld/hamt/src/hamt.rs
Outdated
| @@ -33,21 +34,26 @@ use crate::{Config, Error, Hash, HashAlgorithm, Sha256}; | |||
| /// assert_eq!(map.get::<_>(&1).unwrap(), None); | |||
| /// let cid = map.flush().unwrap(); | |||
| /// ``` | |||
| pub type Hamt<BS, V, K = BytesKey> = HamtImpl<BS, V, version::V3, K, Sha256>; | |||
There was a problem hiding this comment.
We need to continue to take H as a type parameter to avoid breaking the API.
ipld/hamt/src/lib.rs
Outdated
|
|
||
| /// Default bit width for indexing a hash at each depth level | ||
| const DEFAULT_BIT_WIDTH: u32 = 8; | ||
| /// Default bit width for indexing a hash at each depth level for Hamt v0 | ||
| pub const DEFAULT_BIT_WIDTH_V0: u32 = 5; |
There was a problem hiding this comment.
Let's make this private unless we need it to be public for some reason.
There was a problem hiding this comment.
I've removed this field due to #1828 and filecoin-project/builtin-actors#1346. Basically, the user should just pass the bit-width they need and I'm going to deprecate the "default" methods as they're useless.
As far as I can tell, the v0 bit-width was also 8 by default. We just set it to 5 everywhere.
ipld/hamt/src/pointer.rs
Outdated
| let ipld = Ipld::deserialize(deserializer)?; | ||
| let (_key, value) = match ipld { | ||
| Ipld::Map(map) => map | ||
| .into_iter() | ||
| .next() | ||
| .ok_or("Expected at least one element".to_string()), | ||
| other => Err(format!("Expected `Ipld::Map`, got {:#?}", other)), | ||
| } |
There was a problem hiding this comment.
We should decode this as a tagged enum instead of using Ipld and looking at the types.
#[derive(Deserialize, Serialize)]
enum PointerDe<K, V> {
#[serde(name = "l")]
Link(Cid),
#[serde(name = "v")]
Values(KeyValuePair<K, V>),
}There was a problem hiding this comment.
i had to change l and v to 0 and 1 respectively to read genesis header in forest and I'm able to boot forest daemon.
Changes made:
#[derive(Serialize)]
pub(super) enum PointerSer<'a, K, V> {
#[serde(rename = "0")]
Vals(&'a [KeyValuePair<K, V>]),
#[serde(rename = "1")]
Link(&'a Cid),
}
#[derive(Deserialize, Serialize)]
pub enum PointerDe<K, V> {
#[serde(rename = "0")]
Link(Cid),
#[serde(rename = "1")]
Vals(Vec<KeyValuePair<K, V>>),
}
But Hamtv0 tests in this repo fails with:
thread 'test_default::test_hamtv0' panicked at 'called `Result::unwrap()` on an `Err` value: Dynamic(Serialization error for Cbor protocol: TypeMismatch { name: "CBOR tag", byte: 1 })', ipld/hamt/tests/hamt_tests.rs:1022:69
with the above changes.
There was a problem hiding this comment.
Huh. I thought it was k/v. I must be thinking of IPFS HAMTs.
But Hamtv0 tests in this repo fails with:
Hm. Something funky is going on there. Can you push the code?
There was a problem hiding this comment.
I've pushed the changes with failing hamtv0 tests.
There was a problem hiding this comment.
It was a sneaky bug. The Vals/Pointer variants were swapped.
There was a problem hiding this comment.
Ah i see, interesting! thanks
| @@ -15,18 +15,37 @@ use super::node::Node; | |||
| use super::{Error, Hash, HashAlgorithm, KeyValuePair}; | |||
| use crate::Config; | |||
|
|
|||
| pub mod version { | |||
There was a problem hiding this comment.
Let's make this crate-private if possible, or #[doc(hidden)] otherwise.
ipld/hamt/src/pointer.rs
Outdated
| pub(super) enum PointerSer<'a, K, V> { | ||
| Vals(&'a [KeyValuePair<K, V>]), | ||
| Link(&'a Cid), | ||
| } |
There was a problem hiding this comment.
I'm a bit confused at how this works. For v0, we want to serialize to a map of "0"/"1" to a cid/list of links. As far as I know, this here will serialize use "Vals" and "Link" as the keys?
Well, that was an sneaky bug.
The user needs to pass the bit width based on the context. This constant isn't actually used anywhere except tests anyways (which, IMO, is confusing as I'd expect it to be used by-default for v0 hamts if specified like that).
More context: ChainSafe/forest#3060 (review)