Skip to content
This repository has been archived by the owner on Jun 19, 2023. It is now read-only.

refactor: BlockPutSettings.CidPrefix #80

Merged
merged 7 commits into from
Apr 21, 2022

Conversation

schomatis
Copy link
Contributor

@schomatis schomatis commented Nov 18, 2021

Cleanup related to ipfs/kubo#8471, required by ipfs/kubo#8568

TLDR

CIDv1 returned by default

Part of https://github.com/ipfs/ipfs/issues/337.
The raw codec is used by default, unless custom one is set in BlockPutOptions via CidCodec func:

result, err := api.Block().Put(req.Context, file, options.Block.CidCodec(cidCodec))

CID prefix is now part of BlockPutSettings

caopts "github.com/ipfs/interface-go-ipfs-core/options"

- settings, pref, err := caopts.BlockPutOptions(opts...)
+ settings, err := caopts.BlockPutOptions(opts...)
...
-	calculatedCid, err := pref.Sum(data)
+	calculatedCid, err := settings.CidPrefix.Sum(data)

BREAKING CHANGES

  • implicit BlockPutSettings now produces CIDv1 with raw codec by default now
    • CidCodec makes block put return CID with alternative codec
    • 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 things described in fix(cmds): CIDv1 and correct multicodecs in 'block put' and 'cid codecs’  kubo#8568:
      • 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 (!)

Closes #51 (we default to raw and not dag-pb anymore)

@lidel lidel removed this from the go-ipfs 0.14 milestone Apr 12, 2022
options/block.go Outdated
Comment on lines 65 to 67
// FIXME(BLOCKING): Do we actually want to consult the CID codecs table
// even with the old --format options? Or do we always want to check
// the multicodec one?
Copy link
Member

@lidel lidel Mar 30, 2022

Choose a reason for hiding this comment

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

Manually maintained CID mappings are being removed in ipfs/go-cid#137, we want to only use go-multicodec from now. Will rebase + push refactor shortly.

@lidel lidel force-pushed the schomatis/feat/block-option/add-store-codec branch from 1187b24 to 48b4b81 Compare April 12, 2022 22:40
@lidel lidel changed the title feat(block options): add store codec refactor: BlockPutSettings.CidPrefix Apr 12, 2022
@lidel
Copy link
Member

lidel commented Apr 12, 2022

Pushed a6eb015:

Removes duplicated fields and replaces them with cid.Prefix

Codec, MhType and MhLength were  already in prefix, and we already return
prefix. A lot of duplicated values and code responsible for syncing them
did not really need to exist.

Will now take it for a spin in ipfs/kubo#8568 and adjust if necessary

Removes duplicated fields and replaces them with cid.Prefix

Codec, MhType and MhLength were  already in prefix, and we already return
prefix. A lot of duplicated values and code responsible for syncing them
did not really need to exist.
@lidel lidel force-pushed the schomatis/feat/block-option/add-store-codec branch from 48b4b81 to a6eb015 Compare April 12, 2022 23:37
lidel added a commit to ipfs/kubo that referenced this pull request Apr 13, 2022
- 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 marked this pull request as ready for review April 13, 2022 02:12
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.

Seems to be ready, this PR is being used in ipfs/kubo#8568.
See comments below for key changes.

Comment on lines 11 to 14
type BlockPutSettings struct {
Codec string
MhType uint64
MhLength int
Pin bool
CidPrefix cid.Prefix
Pin bool
}
Copy link
Member

Choose a reason for hiding this comment

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

ℹ️ this is the core change in this PR – codec/mh were present twice, this folds them into cid.Prefix that is then used in go-ipfs in https://github.com/ipfs/go-ipfs/pull/8568/files#diff-0c55f05d877cead8d5b53c5dea1912d738456bc774332cd9406efef74e224afcR44

Comment on lines +98 to +111
// Opt-in CIDv0 support for backward-compatibility
if format == "v0" {
settings.CidPrefix.Version = 0
}

// Fixup a legacy (invalid) names for dag-pb (0x70)
if format == "v0" || format == "protobuf" {
format = "dag-pb"
}

// Fixup invalid name for dag-cbor (0x71)
if format == "cbor" {
format = "dag-cbor"
}
Copy link
Member

@lidel lidel Apr 13, 2022

Choose a reason for hiding this comment

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

ℹ️ this maintains backward-compatibility for existing users of ipfs block put --format – see sharness+CI tests in ipfs/kubo#8568

Comment on lines +45 to +50
t.Run("TestBlockPut (get raw CIDv1)", tp.TestBlockPut)
t.Run("TestBlockPutCidCodec: dag-pb", tp.TestBlockPutCidCodecDagPb)
t.Run("TestBlockPutCidCodec: dag-cbor", tp.TestBlockPutCidCodecDagCbor)
t.Run("TestBlockPutFormat (legacy): cbor → dag-cbor", tp.TestBlockPutFormatDagCbor)
t.Run("TestBlockPutFormat (legacy): protobuf → dag-pb", tp.TestBlockPutFormatDagPb)
t.Run("TestBlockPutFormat (legacy): v0 → CIDv0", tp.TestBlockPutFormatV0)
Copy link
Member

Choose a reason for hiding this comment

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

ℹ️ these tests run in ipfs/kubo#8568

@github-actions
Copy link

Suggested version: v0.7.0
Comparing to: v0.6.2 (diff)

Changes in go.mod file(s):

diff --git a/go.mod b/go.mod
index 5d9726e..b7fc215 100644
--- a/go.mod
+++ b/go.mod
@@ -8,14 +8,59 @@ require (
 	github.com/ipfs/go-merkledag v0.6.0
 	github.com/ipfs/go-path v0.1.1
 	github.com/ipfs/go-unixfs v0.2.4
-	github.com/klauspost/cpuid/v2 v2.0.6 // indirect
 	github.com/libp2p/go-libp2p-core v0.8.5
 	github.com/multiformats/go-multiaddr v0.3.3
 	github.com/multiformats/go-multibase v0.0.3
+	github.com/multiformats/go-multicodec v0.3.0
 	github.com/multiformats/go-multihash v0.0.15
+)
+
+require (
+	github.com/btcsuite/btcd v0.21.0-beta // indirect
+	github.com/crackcomm/go-gitignore v0.0.0-20170627025303-887ab5e44cc3 // indirect
+	github.com/gogo/protobuf v1.3.2 // indirect
+	github.com/google/uuid v1.2.0 // indirect
+	github.com/hashicorp/golang-lru v0.5.4 // indirect
+	github.com/ipfs/bbloom v0.0.4 // indirect
+	github.com/ipfs/go-block-format v0.0.3 // indirect
+	github.com/ipfs/go-blockservice v0.3.0 // indirect
+	github.com/ipfs/go-datastore v0.5.0 // indirect
+	github.com/ipfs/go-ipfs-blockstore v1.2.0 // indirect
+	github.com/ipfs/go-ipfs-chunker v0.0.1 // indirect
+	github.com/ipfs/go-ipfs-ds-help v1.1.0 // indirect
+	github.com/ipfs/go-ipfs-exchange-interface v0.1.0 // indirect
+	github.com/ipfs/go-ipfs-posinfo v0.0.1 // indirect
+	github.com/ipfs/go-ipfs-util v0.0.2 // indirect
+	github.com/ipfs/go-ipld-legacy v0.1.0 // indirect
+	github.com/ipfs/go-log v1.0.5 // indirect
+	github.com/ipfs/go-log/v2 v2.3.0 // indirect
+	github.com/ipfs/go-metrics-interface v0.0.1 // indirect
+	github.com/ipfs/go-verifcid v0.0.1 // indirect
+	github.com/ipld/go-codec-dagpb v1.3.0 // indirect
+	github.com/ipld/go-ipld-prime v0.11.0 // indirect
+	github.com/jbenet/goprocess v0.1.4 // indirect
+	github.com/klauspost/cpuid/v2 v2.0.6 // indirect
+	github.com/libp2p/go-buffer-pool v0.0.2 // indirect
+	github.com/libp2p/go-openssl v0.0.7 // indirect
+	github.com/mattn/go-isatty v0.0.13 // indirect
+	github.com/minio/blake2b-simd v0.0.0-20160723061019-3f5f724cb5b1 // indirect
+	github.com/minio/sha256-simd v1.0.0 // indirect
+	github.com/mr-tron/base58 v1.2.0 // indirect
+	github.com/multiformats/go-base32 v0.0.3 // indirect
+	github.com/multiformats/go-base36 v0.1.0 // indirect
+	github.com/multiformats/go-varint v0.0.6 // indirect
+	github.com/opentracing/opentracing-go v1.2.0 // indirect
+	github.com/polydawn/refmt v0.0.0-20201211092308-30ac6d18308e // indirect
+	github.com/spacemonkeygo/spacelog v0.0.0-20180420211403-2296661a0572 // indirect
+	github.com/whyrusleeping/cbor-gen v0.0.0-20200123233031-1cdf64d27158 // indirect
+	github.com/whyrusleeping/chunker v0.0.0-20181014151217-fe64bd25879f // indirect
+	go.uber.org/atomic v1.7.0 // indirect
 	go.uber.org/multierr v1.7.0 // indirect
+	go.uber.org/zap v1.16.0 // indirect
 	golang.org/x/crypto v0.0.0-20210513164829-c07d793c2f9a // indirect
 	golang.org/x/sys v0.0.0-20210514084401-e8d321eab015 // indirect
+	golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1 // indirect
+	google.golang.org/protobuf v1.27.1 // indirect
 )
 
-go 1.16
+go 1.17

gorelease says:

# github.com/ipfs/interface-go-ipfs-core/options
## incompatible changes
BlockPutOptions: changed from func(...BlockPutOption) (*BlockPutSettings, github.com/ipfs/go-cid.Prefix, error) to func(...BlockPutOption) (*BlockPutSettings, error)
BlockPutSettings.Codec: removed
BlockPutSettings.MhLength: removed
BlockPutSettings.MhType: removed
## compatible changes
BlockPutSettings.CidPrefix: added
blockOpts.CidCodec: added

# github.com/ipfs/interface-go-ipfs-core/tests
## incompatible changes
(*TestSuite).TestBlockPutFormat: removed
## compatible changes
(*TestSuite).TestBlockPutCidCodecDagCbor: added
(*TestSuite).TestBlockPutCidCodecDagPb: added
(*TestSuite).TestBlockPutFormatDagCbor: added
(*TestSuite).TestBlockPutFormatDagPb: added
(*TestSuite).TestBlockPutFormatV0: added

# summary
Suggested version: v0.7.0

gocompat says:

(empty)

@lidel
Copy link
Member

lidel commented Apr 21, 2022

Merging as per verbal from Adin to unblock ipfs/kubo#8568 for go-ipfs 0.13

@lidel lidel merged commit a3374d9 into master Apr 21, 2022
@lidel lidel deleted the schomatis/feat/block-option/add-store-codec branch April 21, 2022 15:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

The block API shouldn't prefer CIDv0 dag-pb
4 participants