commands/block: use CIDv1 with custom mhtype #4563

magik6k · 2018-01-08T13:23:36Z

Closes #4741 ~Kubuxu

kevina

This looks fine.

Note that this differs from how a multi-hash is specified in the add and files command which use just --hash.

Stebalien · 2018-01-09T00:51:31Z

Shouldn't this actually just return an error if the user passes -f v0 --mhtype=other-hash --mhlen=other-len?

magik6k · 2018-01-22T17:36:47Z

Done

Stebalien

Should be correct once you remove the default from the --format flag (you should probably note the behavior in the command's help itself).

Stebalien · 2018-01-28T06:16:47Z

core/commands/block.go

 		var pref cid.Prefix
 		pref.Version = 1

-		format, _ := req.Options["format"].(string)
+		format, userFormat := req.Options["format"].(string)


Unfortunately, userFormat will always be true there is a default value. The only way I know to fix this, is to simply remove the default.

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

Kubuxu · 2018-03-01T15:01:26Z

test/sharness/t0050-block.sh

@@ -209,4 +209,16 @@ test_expect_success "no panic in output" '
  test_expect_code 1 grep "panic" stat_out
 '

+test_expect_success "can set multihash type and length on block put without format" '
+  HASH=$(echo "foooo" | ipfs block put --mhtype=sha3 --mhlen=16)


16 will be too short of a hash length after cid changes come through, please change it to 20.

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

Stebalien · 2018-03-03T03:03:51Z

core/commands/block.go

 `,
 	},

 	Arguments: []cmdkit.Argument{
 		cmdkit.FileArg("data", true, false, "The data to be stored as an IPFS block.").EnableStdin(),
 	},
 	Options: []cmdkit.Option{
-		cmdkit.StringOption("format", "f", "cid format for blocks to be created with.").WithDefault("v0"),
+		cmdkit.StringOption("format", "f", "cid format for blocks to be created with.").WithDefault(""),


I wouldn't specify a default at all. As it is, passing -f "" will be considered valid even though it makes no sense.

If you don't specify a default, you can tell if the parameter was passed with format, formatSet := req.Options["format"].(string).

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

schomatis · 2018-03-11T00:39:16Z

This PR looks correct to me (and it fixes the addressed issue) but let me raise an off-topic question: shouldn't part of this prefix preprocessing logic be in go-cid?

It seems to me there are a couple of hard-coded pieces of information of what constitutes a valid CID prefix that the ipfs block command doesn't need to know because it shouldn't be its responsibility to check for it, e.g., the (now) default CIDv1 specified here (pref.Version = 1) will have to be updated if (or when) a new CID version is released.

The ipfs block command doesn't do anything with the version and hash function except forward them to Prefix, which is the one that computes the hash/CID that ipfs block is really interested in (to create block).

Kubuxu · 2018-03-11T00:49:40Z

I think most of the processing here is due to 1. us wanting to provide good error messages for user 2. be lenient with the user input. One check in question (CIDv0 can only be full length SHA256) is currently not implemented but we are in process of changing that (https://github.com/ipfs/go-cid/pull/40/files#diff-beb384810133d725af6b502e9b6e3de8R139)

I am not sure if go-cid would be right place to cover all of those areas but having a helper function somewhere in go-ipfs would not hurt.

schomatis · 2018-03-11T01:08:13Z

us wanting to provide good error messages for user

That's a good point I hadn't considered, but looking at the errors:

unrecognized multihash function
unrecognized format
cannot generate CIDv0 with non-sha256 hash function

I think all of them can be outsourced without losing expressiveness for the user (the missing option "mhlen" seems to me the only error that truly belongs here).

I am not sure if go-cid would be right place to cover all of those areas but having a helper function somewhere in go-ipfs would not hurt.

That's fine, my main point wasn't where this logic should be placed but that it should be encapsulated somewhere outside this command function.

Anyway, maybe this should be discussed in another issue after this PR is merged, trying to change all this here in this PR now would be to messy, but I wanted to leave this suggestion documented somewhere.

Stebalien · 2018-03-11T01:40:23Z

I think the issue here is that we want to catch all of these errors early and provide reasonable defaults (that may change based on the command in question). However, we probably could have something in go-cid that takes in a set of CID "constraints" and spits out either an error or the set of CID parameters.

An alternative solution would be to add flag "sets" to go-ipfs-cmds and create a flag set for CID options somewhere.

whyrusleeping · 2018-03-23T06:04:19Z

First PR merged of the 0.4.15 cycle goes to @magik6k :) 👏

ghost assigned magik6k Jan 8, 2018

ghost added the status/in-progress In progress label Jan 8, 2018

kevina approved these changes Jan 8, 2018

View reviewed changes

magik6k mentioned this pull request Jan 11, 2018

Block put with sha2-512 #4577

Closed

magik6k force-pushed the fix/block-mhtype branch from 4c180b8 to dbe0074 Compare January 22, 2018 17:36

magik6k force-pushed the fix/block-mhtype branch from dbe0074 to 3b5a78c Compare January 25, 2018 17:40

Stebalien requested changes Jan 28, 2018

View reviewed changes

magik6k force-pushed the fix/block-mhtype branch 2 times, most recently from 9c43c2c to fd5479a Compare February 1, 2018 23:57

commands/block: use CIDv1 with custom mhtype

8c176d2

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

magik6k force-pushed the fix/block-mhtype branch from fd5479a to 8c176d2 Compare February 2, 2018 00:56

Kubuxu reviewed Mar 1, 2018

View reviewed changes

commands/block: Increase mhlen in sharness

6f68eac

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

Kubuxu approved these changes Mar 1, 2018

View reviewed changes

Kubuxu requested a review from Stebalien March 1, 2018 16:44

magik6k mentioned this pull request Mar 2, 2018

Issue with block get and keccak256 - illegal base32 data at input byte X #4741

Closed

Stebalien reviewed Mar 3, 2018

View reviewed changes

commands/block: don't use default for -f

efb7419

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

Kubuxu requested a review from Stebalien March 10, 2018 22:56

Stebalien approved these changes Mar 11, 2018

View reviewed changes

Kubuxu added RFM and removed status/in-progress In progress labels Mar 11, 2018

Kubuxu added this to the go-ipfs 0.4.15 milestone Mar 13, 2018

whyrusleeping merged commit a9dfe41 into master Mar 23, 2018

whyrusleeping deleted the fix/block-mhtype branch March 23, 2018 06:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

commands/block: use CIDv1 with custom mhtype #4563

commands/block: use CIDv1 with custom mhtype #4563

magik6k commented Jan 8, 2018 •

edited by Kubuxu

Loading

kevina left a comment •

edited

Loading

Stebalien commented Jan 9, 2018

magik6k commented Jan 22, 2018

Stebalien left a comment

Stebalien Jan 28, 2018

Kubuxu Mar 1, 2018

magik6k Mar 1, 2018

Stebalien Mar 3, 2018

schomatis commented Mar 11, 2018

Kubuxu commented Mar 11, 2018

schomatis commented Mar 11, 2018

Stebalien commented Mar 11, 2018

whyrusleeping commented Mar 23, 2018

commands/block: use CIDv1 with custom mhtype #4563

commands/block: use CIDv1 with custom mhtype #4563

Conversation

magik6k commented Jan 8, 2018 • edited by Kubuxu Loading

kevina left a comment • edited Loading

Choose a reason for hiding this comment

Stebalien commented Jan 9, 2018

magik6k commented Jan 22, 2018

Stebalien left a comment

Choose a reason for hiding this comment

Stebalien Jan 28, 2018

Choose a reason for hiding this comment

Kubuxu Mar 1, 2018

Choose a reason for hiding this comment

magik6k Mar 1, 2018

Choose a reason for hiding this comment

Stebalien Mar 3, 2018

Choose a reason for hiding this comment

schomatis commented Mar 11, 2018

Kubuxu commented Mar 11, 2018

schomatis commented Mar 11, 2018

Stebalien commented Mar 11, 2018

whyrusleeping commented Mar 23, 2018

magik6k commented Jan 8, 2018 •

edited by Kubuxu

Loading

kevina left a comment •

edited

Loading