-
Notifications
You must be signed in to change notification settings - Fork 236
IPIP 0499: CID Profiles #499
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
base: main
Are you sure you want to change the base?
Conversation
lets make the fanout match the max links from files and rename profile to `-wide` this will make it easier to discuss in ipfs/specs#499
Co-authored-by: Bumblefudge <[email protected]>
Import.* config params for controlling DAG width were added in: ipfs/kubo#10774
|
Thank you for kicking this off, and filling initial state. I've incorporated specific "dag width" settings for Next:
|
This comment was marked as off-topic.
This comment was marked as off-topic.
| 1. UnixFS DAG layout (e.g. balanced, trickle) | ||
| 1. UnixFS DAG width (max number of links per `File` node) | ||
| 1. `HAMTDirectory` fanout (must be a power of 2) | ||
| 1. `HAMTDirectory` threshold (max `Directory` size before switching to `HAMTDirectory`): based on an estimate of the block size by counting the size of PNNode.Links |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this number is dynamic based on the lengths of the actual link entries in the dag, we will need to specify what algorithm that estimation follows. I would put such things in a special "ipfs legacy" profile to be honest, along with cidv0, non-raw leaves etc. We probably should heavily discourage coming up with profiles that do weird things, like dynamically setting params or not using raw-leaves for things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, each layout would have its own set of layout-params:
- balanced:
- max-links: N
- trickle:
- max-leaves-per-level: N
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably should heavily discourage coming up with profiles that do weird things, like dynamically setting params or not using raw-leaves for things.
Yeah, that's exactly what we're doing by defining this profile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait is kubo dynamically assigning HAMT Directory threshold, currently? i was assuming this was a static number!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current spec mentions fanout but not threshold, so i'm a little confused what current implementations are doing and if it's even worth fitting into the profile system or just giving up and letting a significant portion of HAMT-shared legacy data just but unprofiled/not-recreatable using the profiles...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lidel Is this written down in any of the specs? Or is it just in the code at this point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAK decision when to do HAMTDirectory is an implementation-specific behavior. So far the rule of thumb is to keep blocks under 1-2MiB and usually good idea to match chunk size defined (default or defined by user).
Implementation-wise both GO (Boxo/Kubo) and JS (Helia) have size-based heuristic that makes decision when to switch from normal Directory to HAMTDirectory:
- Kubo: Import.UnixFSHAMTDirectorySizeThreshold (with size threshold of 256KiB, as that is still the default chunk size in Kubo, user can override globally)
- Helia (ipfs-unixfs-importer ): shardSplitThresholdBytes (same, 256KiB by default, user can override per operation)
iirc (from 2 year old memory, something to check/confirm) is that the size estimation details may/are likely different between GO and JS. They both estimate the serialized DAGNode size by calculating the aggregate byte length of directory entries (link names + CIDs), though the JavaScript implementation appears to include additional metadata in its calculation:
- Kubo's size estimation method is likely
estimatedSize = sum(len(link.Name) + len(link.Cid.Bytes()) for each link) - Helia is likely "the size of the final DAGNode (including link names, sizes, optional metadata fields etc)"
If true, the slight differences in calculation methods might result in directories sharding at marginally different sizes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to be exact you have to take into account any non-zero value fields in the serialized root UnixFS metadata since these affect the block size.
It's quite possible that Kubo will produce a HAMT block that's too big with a certain combination of directory entry names if someone has also changed the encoded directory's default mtime or whatever, probably because the "should-I-shard" feature pre-dates Kubo's ability to add UnixFSv1.5 metadata to things.
Really there's no need to estimate anything - it's trivial to count the actual bytes that a block will take up and then shard if necessary.
src/ipips/ipip-0499.md
Outdated
| 1. Whether empty directories are included in the DAG | ||
| - Some implementations apply filtering before merkleizing filesystem entries in the DAG. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is weird, because then we need to consider empty files, hidden files, unreadable files, symlinks and symlink follows, so probably need to mention all those as part of the profile too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is motivated by Git's default behaviour which ignores empty directories.
But we can mention here the rest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait, do @hsanjuan , do you mean mentioning whether empty files, hidden files, etc affect the decision of whether a directory is empty, or do you mean that each of those files might be divergently handled by different implementations and should be a variable in the profile? I would much rather behavior for all of those file types be a UnixFS concern and specified in UnixFS spec, modulo any historic variations worth including in a profile...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have existing implementations that support filtering differently on all of these? Because unless we do, I would really rather not specify all possible variants. And I agree with @bumblefudge: let's have two behaviours if possible, and punt to the UnixFS spec for how to describe them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, these choices are being made today and it'd be nice to be explicit about them. e.g. default Helia leaves all of these options default (false): https://github.com/ipfs/helia/blob/027bd3549da9ef5a6f07eaac346942cf24f3fc24/packages/unixfs/src/utils/glob-source.ts#L12-L42
But in filecoin-pin currently I've opted to include hidden files: https://github.com/filecoin-project/filecoin-pin/blob/9ab3f8c110ce0b6c6bf21c1fcdbcf84ade557953/src/core/unixfs/car-builder.ts#L30-L32 (I'm rethinking that choice now, but I'd like to know Kubo's defaults as well).
I'd prefer to align to a standard profile for file filtering so we collectively have "one standard default behaviour", but I understand it's a bit more work to explicate all of that. So maybe it can be a hand-wave for now and tightened up later because you could argue it's external to a unixfs spec and more about the choice of what to feed into a unixfsification process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels like mixing abstractions, no? To me filtering of input is an userland software decision, out of scope. I don't think Ext4, NTFS or APFS specs guide implementers if hidden files are included when copying directories. We could mention it in "Appendix: Notes for Implementers" to flag potential discrepancy if filtering is involved, and that implementation should provide user with ability to disable/adjust default filters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, so we would include a directory if it's there, even if empty. I was never sure if there is a good reason for git's approach but always seemed exotic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filtering of input is an userland software decision
And yet it still impacts the resulting root CID, so I think it's in scope here, no? How about we make it a "SHOULD" advisory.
For consistent output implementations should by default not apply file and directory filtering and include empty directories, but may opt to allow user-driven decisions to filter out entities such as hidden files, dot files, and empty directories.
Co-authored-by: Hector Sanjuan <[email protected]>
Co-authored-by: Rod Vagg <[email protected]>
| 1. UnixFS DAG layout (e.g. balanced, trickle etc...) | ||
| 1. UnixFS DAG width (max number of links per `File` node) | ||
| 1. `HAMTDirectory` bitwidth, i.e. the number of bits determines the fanout of the `HAMTDirectory` (default bitwidth is 8 == 256 leaves). | ||
| 1. `HAMTDirectory` threshold (max `Directory` size before switching to `HAMTDirectory`): based on an estimate of the block size by counting the size of PNNode.Links |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 1. `HAMTDirectory` threshold (max `Directory` size before switching to `HAMTDirectory`): based on an estimate of the block size by counting the size of PNNode.Links | |
| 1. `HAMTDirectory` threshold (max `Directory` size before switching to `HAMTDirectory`): based on an estimate of the block size by counting the size of PNNode.Links. We do not include details about the estimation algorithm as we do not encourage implementations to support it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bit odd to discourage, when both most popular implementations in GO and JS use size-based heurstic - #499 (comment)
Unsure how to handle this. Perhaps clarify the heuristic is implementation-specific, and when deterministic behavior is expected, a specific heuristic should be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should be estimating the block size as it's trivial to calculate it exactly. Can we not just define this (and punt to the spec for the details) to make it less hand-wavey?
| 1. `HAMTDirectory` threshold (max `Directory` size before switching to `HAMTDirectory`): based on an estimate of the block size by counting the size of PNNode.Links | |
| 1. `HAMTDirectory` threshold (max `Directory` size before switching to `HAMTDirectory`): based on the final size of the serialized form of the [PBNode protobuf message](https://specs.ipfs.tech/unixfs/#dag-pb-node) that represents the directory. |
|
Hey, I'd love to be able to reference this, even if it's in "draft" form, could we just merge it and continue to iterate on top of it to get it right? |
Fixed outdated references, consistent profile names, streamlined Summary and Motivation sections.
🚀 Build Preview on IPFS ready
|
|
I made a few changes/fixes, aiming to land this early next week.
Open questions:
|
|
|
||
| As an alternative to profiles, users can store and transfer CAR files of UnixFS content, which include the merkle DAG nodes needed to verify the CID. | ||
|
|
||
| ## Test fixtures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noting this is (imo) a blocker.
We did not merge UnixFS spec until we had sensible set of fixtures that people could use as reference.
The spec may be incomplete, but a fixture will let people reverse-engineer any details, and then PR improvement to spec.
Without fixtures for each UnixFS node type, we risk unknown unknown silently impacting final CID (e.g. because we did not know that someone may decide to place leaves one level sooner as "optimization" and someone else always at bottom, as "formal consistency")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tracking this in ipfs/kubo#11071
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
- I will implement
kubo-*profiles as part of 0.40 and test fixtures will be part of that work. - Then we will be able to link to them form spec, like we did in https://specs.ipfs.tech/unixfs/#appendix-test-vectors
Co-authored-by: Rod Vagg <[email protected]>
Co-authored-by: Rod Vagg <[email protected]>
|
Just synced with @lidel. He wants to ship this with test fixtures in place, (tracked in kubo/issues/11071). In the meantime, we don't anticipate changes to the profiles themselves so you can can reference this PR. |
Co-authored-by: Rod Vagg <[email protected]>
|
Great work, glad to see this! Couple notes/questions:
|
Currently, CIDs can be generated with a variety of settings and optimizations for chunking, DAG width, and more. This means the same file can yield multiple, different CIDs depending on which tools and settings are used, and it is not possible to reliably reproduce or verify the CID.
This proposal introduces profiles for IPFS CIDs. Profiles explicitly define CID version, hash algorithm, chunk size, DAG width, layout, and other parameters. They can be used to verify data across implementations, provide recommended settings depending on retrieval performance goals, and more.