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

fix(cmds): CIDv1 and correct multicodecs in 'block put' and 'cid codecs’ #8568

Merged
merged 10 commits into from
Apr 21, 2022

Conversation

schomatis
Copy link
Contributor

@schomatis schomatis commented Nov 25, 2021

@lidel's blurb for Release Notes

BREAKING CHANGES

  • ipfs block commands
    • ipfs block put now produces CIDv1 with raw codec by default now
      • ipfs block put --cid-codec makes block put return CID with alternative codec
      • ipfs block put --format is deprecated. It used incorrect codec names and should be avoided for new deployments. Use it only if you need the old, invalid behavior, namely:
        • ipfs block put --format=v0 will produce CIDv0 (implicit dag-pb)
        • ipfs block put --format=cbor will produce CIDv1 with dag-cbor (!)
        • ipfs block put --format=protobuf will produce CIDv1 with dag-pb (!)
  • ipfs cid codecs

Depends on:

TODO:

Closes #8471

@BigLep
Copy link
Contributor

BigLep commented Mar 29, 2022

@schomatis : @lidel is going to take this over as it is being used to validate his work in ipfs/go-cid#137 . The issue has been reeassigned.

@BigLep BigLep removed the request for review from aschmahmann March 29, 2022 16:10
@lidel lidel force-pushed the schomatis/fix/use-ipld-multicodecs branch from 6c90cda to 1b7c968 Compare April 11, 2022 22:20
@lidel
Copy link
Member

lidel commented Apr 11, 2022

(wip rebase, need to rebase ipfs/interface-go-ipfs-core#80 too and switch this PR to that)

schomatis and others added 3 commits April 13, 2022 00:43
- switches to ipfs/interface-go-ipfs-core#80 (comment)
- updates helptext of block commands to reflect the fact we now use CIDs
  everywhere and document the defaults
- add tests for new --cid-codec and old --format behavior
@lidel lidel force-pushed the schomatis/fix/use-ipld-multicodecs branch from 1b7c968 to 70c13cf Compare April 13, 2022 00:37
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready for review together with ipfs/interface-go-ipfs-core#80

Comment on lines 162 to 166
if mhtval != mh.SHA2_256 || (mhlen != -1 && mhlen != 32) {
format = "protobuf"
} else {
format = "v0"
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lidel lidel requested a review from aschmahmann April 13, 2022 02:09
@lidel lidel marked this pull request as ready for review April 13, 2022 02:12
@lidel
Copy link
Member

lidel commented Apr 19, 2022

TODO: see if we can error if no format or cid-codec is provided over HTTP RPC (or if the default from CLI applies everywhere).

cmds.StringOption(mhtypeOptionName, "multihash hash function").WithDefault("sha2-256"),
cmds.IntOption(mhlenOptionName, "multihash hash length").WithDefault(-1),
cmds.BoolOption(pinOptionName, "pin added blocks recursively").WithDefault(false),
cmds.StringOption(blockCidCodecOptionName, "Multicodec to use in returned CID").WithDefault("raw"),
Copy link
Member

@lidel lidel Apr 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aschmahmann I did some tests around this and the good news is that I was wrong, there is a way to have unified handling of default opts that works the same in ipfs block put CLI and raw HTTP POST to /api/v0/block/put.

If we keep the .WithDefault("raw") here, then this implicit default is also applied when people use curl against /api/v0/block/put and forget to pass cid-codec. It will also produce good docs at https://docs.ipfs.io/reference/http/api/#api-v0-block-put.

If we want to be extra strict and return error, I'd have to remove WithDefault and add some glue code that detects cidCodec == "" and throws error, but that feels hacky and annoying because it would make cid-codec a mandatory parameter, making it bad UX for the common use case where people are fine with codec being raw.

My vote is to keep WithDefault("raw").

Copy link
Contributor

@aschmahmann aschmahmann Apr 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, the only breaking change is:

ipfs block put now produces CIDv1 with raw codec by default now

Which only effects the output CID, but ipfs block put <some-dagpb-stuff>; ipfs block get QmDagPBv0Thing will still work fine. This seems like a not too bad silent breakage, although I'm generally averse to silent breakages. WDYT?

Copy link
Member

@lidel lidel Apr 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sgtm. Added 0.13 breaking changes section to CHANGELOG.md in 2f5b8a4

@lidel
Copy link
Member

lidel commented Apr 21, 2022

As per triage verbal with Adin today, will merge as soon CI is green.

@lidel lidel merged commit 7b5fe80 into master Apr 21, 2022
@lidel lidel deleted the schomatis/fix/use-ipld-multicodecs branch April 21, 2022 16:19
lidel added a commit to ipfs/go-ipfs-http-client that referenced this pull request Apr 21, 2022
Applies necessary changes to ensure 'block/put' works
and is backward-compatible.

Context:
ipfs/kubo#8568
lidel added a commit to ipfs/go-ipfs-http-client that referenced this pull request Apr 21, 2022
* chore: interop with go-ipfs 0.13

Applies necessary changes to ensure 'block/put' works
and is backward-compatible.

Context:
ipfs/kubo#8568

* chore: 0.3.1

bumping as patch because we bumped to 0.3.0 recently, 
as part of other (unreleased) go-ipfs 0.13 work
@lidel lidel changed the title fix(cmds): use ipld multicodecs for cmds block put and cid fix(cmds): CIDv1 and correct multicodecs in 'block put' and 'cid codecs’ Apr 21, 2022
lidel added a commit to ipfs/go-ipfs-http-client that referenced this pull request Jun 3, 2022
This ensures cid-codec introduced in ipfs/kubo#8568
gets correctly passed (+ we maintain backward-compatibility with CIDv0)
lidel added a commit to ipfs/go-ipfs-http-client that referenced this pull request Jun 3, 2022
This ensures cid-codec introduced in ipfs/kubo#8568
gets correctly passed (+ we maintain backward-compatibility with CIDv0)
lidel added a commit to ipfs/go-ipfs-http-client that referenced this pull request Jun 3, 2022
This ensures cid-codec introduced in ipfs/kubo#8568
gets correctly passed (+ we maintain backward-compatibility with CIDv0)
lidel added a commit to ipfs/go-ipfs-http-client that referenced this pull request Jun 23, 2022
This ensures cid-codec introduced in ipfs/kubo#8568
gets correctly passed (+ we maintain backward-compatibility with CIDv0)
@BigLep BigLep mentioned this pull request Jul 26, 2022
68 tasks
Jorropo pushed a commit that referenced this pull request May 30, 2023
* chore: interop with go-ipfs 0.13

Applies necessary changes to ensure 'block/put' works
and is backward-compatible.

Context:
#8568

* chore: 0.3.1

bumping as patch because we bumped to 0.3.0 recently, 
as part of other (unreleased) go-ipfs 0.13 work

This commit was moved from ipfs/go-ipfs-http-client@ecf364c
Jorropo pushed a commit that referenced this pull request May 30, 2023
This ensures cid-codec introduced in #8568
gets correctly passed (+ we maintain backward-compatibility with CIDv0)

This commit was moved from ipfs/go-ipfs-http-client@9c9f43f
hannahhoward pushed a commit to filecoin-project/kubo-api-client that referenced this pull request Jun 19, 2023
…cs' (#8568)

BREAKING CHANGES: 
- see ipfs/kubo#8568 (comment)

Co-authored-by: Marcin Rataj <lidel@lidel.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Use standard IPLD codec names across the CLI/HTTP API
4 participants