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

feat: v2 commp cid #5

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

aschmahmann
Copy link

@aschmahmann aschmahmann commented Jul 26, 2023

This is a PR for adding support for a new type of piece CID based on treating the piece like a single large block similar to Blake3 (another tree hash used in the IPFS ecosystem).

See the FRC for more details and discussion. This PR probably should not get merged until the FRC is merged.

Note: I switched to GitHub actions (copying the approach from https://github.com/protocol/.github) since my understanding was more projects are moving that way and I wanted to be able to test without relying on CircleCI, happy to remove this though. Can also register this project with https://github.com/protocol/.github for auto-updating of CI configs if that's desired.

cc @ribasushi @Kubuxu

@hannahhoward
Copy link
Contributor

@aschmahmann what happened to this PR? Is there a PieceCidV2 Implementation anywhere in go? Asking cause we've been happily using PieceCIDv2 in JS land for a while but are writing code now in go and I can't seem to find any PieceCIDv2 support, even though it was accepted as FRC a while ago.

cc: @rvagg @ribasushi

@ribasushi
Copy link
Contributor

@hannahhoward in short this was dropped on the floor due to nucleation-winter.

The most pressing issue is that 0x1011 was never formally accepted

As far as go-support: for ♠️ I ended up writing my own, with an arguably cleaner interface.

Afaik @magik6k is writing his own for Curio as well.

@aschmahmann
Copy link
Author

aschmahmann commented Oct 21, 2024

A few additional points of context:

  • Technical / Mechanical
    • The FRC that was accepted looks like Feat/v2 commp cid arbitrary size #6 rather than this PR
    • The function names in this repo (these PRs aside) are a mess... they indicate that these are "v1s" of things there were no "v2s" of and so changing the multihash and CID representations while the piece hash stays the same fits a little awkwardly in the naming. Deciding on the naming and merging this seems best done by anyone who was directly going to use the API a bunch (was never me)
    • The codec PR should get merged, if you need anything mechanical from me lmk, but I ran out of time to bikeshed names given I have little direct usage of these CIDs today
  • Historical
    • I stopped pushing on this from the Go side of things pre-nucleation since the Boost maintainers weren't interested at the time in even accepting PRs that would enable the use of v2 piece CIDs at all, including in the most obvious place (HTTP piece retrievals) and at the time the Lotus maintainers didn't have the time/interest to figure out what the UX would look like for changing the CIDs output by various commands
    • Post nucleation Boost/Curio didn't have time earlier this year, although in recent Slack channel messages I've seen some interest
    • IIUC @ribasushi ended up wanting this for applications he maintains and so built a different implementation unattached to the legacy stuff here (and any negotiations around merge permissions). The API looks good to me, although I might want to PR some more of the test cases from here into there because I'm a bit paranoid.

Certainly feel free to grab any code (whether operational or testing) from here in a future implementation, or ditch it entirely for someone else's. These branches are also able to be pushed to by the repo maintainers and they're free to do so if they'd like.

To me the most important thing here is that people intending to have content identifiers that map uniquely to a single piece of content (i.e. all users of CIDs) actually have that capability so they don't accidentally end up with problems down the line. Glad you're getting use out of these and pushing things forward @hannahhoward.

@ribasushi
Copy link
Contributor

The FRC that was accepted looks like #6 rather than this PR

@aschmahmann perhaps just close this one...?

I might want to PR some more of the test cases from here into there because I'm a bit paranoid.

Feel free, I'd take extra tests for sure.

@hannahhoward
Copy link
Contributor

hannahhoward commented Oct 28, 2024

hey it's a little nucleation drama, little fight over the multicodec table, and accepted FRC with an unmerged official PR due to one team's obstinance, and multiple alternate implementations -- this one is really just all the best typical PLN dramas rolled into one :)

I love it.

Ok well thanks for the guidance. I'm just gonna go see @magik6k implemented something in curio since that's who we're working directly with.

We absolutely use this all over our JS code -- https://github.com/storacha/data-segment/blob/main/src/multihash.js#L26C1-L31C1

When I have more time I'll at least pickup the multicodec fight.

@hannahhoward
Copy link
Contributor

Also cc: @alanshaw -- looks like we've got a bit of uncertainty on which piececid v2 to use over in go land.

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.

3 participants