Add support for using CidV1 in 'ipfs add' #3743

kevina · 2017-03-03T02:35:55Z

License: MIT
Signed-off-by: Kevin Atkinson [email protected]

kevina · 2017-03-03T02:44:40Z

@whyrusleeping I am not sure I am happy with this

What I described in #3699 (comment) was only sufficient for adding files with CidV1. It did not handle symbolic links or directories.

In order to handle directories I needed to touch the mfs package. I added a cidVer parameter to mfs.Mkdir and added a SetCidVersion method to the Directory class.

To make sure the root node was CidV1 I decided it was best to pass in the Cid version when creating the adder rather then setting it later.

The changes to the merkeldag package are not strictly necessary, but it seams the best way to set the Cid version of a ProtoNode.

kevina · 2017-03-03T02:45:33Z

@whyrusleeping one question: Do you think --cid-version=1 should imply --raw-leaves? I think it should and would be one less combination to test for.

whyrusleeping · 2017-03-03T04:44:37Z

@kevina hrm... I think setting cid version to 1 could probably change the default value for raw leaves to true, but i think people should still be able to set raw leaves to false and cid version to 1. Even if we don't allow it through the command line people could manually construct graphs that were that way

kevina · 2017-03-03T23:02:05Z

@whyrusleeping okay, i think this is ready for review

Kubuxu · 2017-03-05T20:33:11Z

core/commands/add.go

@@ -78,6 +79,7 @@ You can now refer to the added file in a gateway, like so:
 		cmds.StringOption(chunkerOptionName, "s", "Chunking algorithm to use."),
 		cmds.BoolOption(pinOptionName, "Pin this object when adding.").Default(true),
 		cmds.BoolOption(rawLeavesOptionName, "Use raw blocks for leaf nodes. (experimental)"),
+		cmds.IntOption(cidVersionOptionName, "Cid version. (experimental)").Default(0),


This should mention enables raw blocks.

I agree, but got stuck on how to state in concisely suggestion welcome.

CID Version. Non-zero value will change default of 'rawleaves' to true.

Kubuxu

I don't like the fact that now every MFS API function will have to take cidversion argument. We will have to either live with it or think about it.

Kubuxu · 2017-03-05T20:34:16Z

core/commands/add.go

 		if err != nil {
 			res.SetError(err, cmds.ErrNormal)
 			return
 		}

+		outChan := make(chan interface{}, 8)


Extract this constant, maybe use the same as AllKeysChan or some other/new but I would prefer it extracted.

This is not new code I just moved things around a little. We can do this in a separate p.r.

This would have taken 10s. General rule many of us try to strive for is to leave code in better shape than we've found it.

Kubuxu · 2017-03-05T20:35:22Z

merkledag/cidversion.go

+)
+
+// CidVersion is a a cid version number that is guaranteed to be valid
+type CidVersion struct {


I would make it type CidVersion int. It will be simpler to use and more idiomatic.

Also there isn't a reason it couldn't be just a constant?

type CidVerision int const ( CID0 CidVersion = 0 CID1 CidVersion = 1 )

You can still have methods on it.

I think you went the way you did because as a way of preventing creation of different CID versions but IMO it should be done with helper function when CID version is from User Interface.

Because nothing prevents me from doing CidVersion{2} either way.

Because nothing prevents me from doing CidVersion{2} either way.

Um, if you do that outside the merkeldag package you will get

implicit assignment of unexported field 'version' in merkledag.CidVersion literal

The idea was to provide some additional type safety so once you have a CidVersion you don't need to return an error if the version is invalid as it has already been checked.

Ahh, missed that. Ok, but I would still be for having variables aliasing usable values.

Alias added, although not currently used.

Also note, I am not against doing the way you suggested.

I just want to be able to assume that once we have a CidVersion we can guarantee we have something valid and don't want to have to return an error (instead we can panic). I think it will be reasonable to assume that the user (in this case the programmer) won't do a cast from an int to to CidVersion and will use the helper function.

@whyrusleeping thoughts.

Kubuxu · 2017-03-05T20:42:36Z

Also the code is not formatter right: https://ci.ipfs.team/blue/organizations/jenkins/go-ipfs/detail/PR-3743/5/pipeline

whyrusleeping · 2017-03-05T20:30:03Z

core/commands/add.go

@@ -78,6 +79,7 @@ You can now refer to the added file in a gateway, like so:
 		cmds.StringOption(chunkerOptionName, "s", "Chunking algorithm to use."),
 		cmds.BoolOption(pinOptionName, "Pin this object when adding.").Default(true),
 		cmds.BoolOption(rawLeavesOptionName, "Use raw blocks for leaf nodes. (experimental)"),
+		cmds.IntOption(cidVersionOptionName, "Cid version. (experimental)").Default(0),


perhaps this option should be a global option. (on the ipfs command, same places as the timout option).

Otherwise we will have to add this in (and maintain it) for each command that wants to be able to select the output cid version

I agree, but thought it would be simpler just to add it for the adder for now.

For example, should every command check for this flag if it is applicable, and if it is not supported yet error out?

whyrusleeping · 2017-03-05T20:30:34Z

core/commands/add.go

+		rawblks, rawblksFound, _ := req.Option(rawLeavesOptionName).Bool()
+		cidVer, _, _ := req.Option(cidVersionOptionName).Int()
+
+		if cidVer >= 1 && !rawblksFound {


We should probably check that cidVer is either zero or one. No other values are supported right now

This check is already done in the call to NewAdder.

kevina · 2017-03-06T04:09:19Z

I don't like the fact that now every MFS API function will have to take cidversion argument. We will have to either live with it or think about it.

Neither do I. It is only the MkDir call that needs an cidversion argument. I added it the MkDir call because it can create multiple nodes that all will need to be set to the correct Cid version. There are two additional options.

Create a new function MkDirWCid and have MkDir call it.
Walk the tree created and fix the Cid version after the fact. I see this as inefficient both in terms of speed and amount of code, but will avoid having to modify the MkDir call.

@whyrusleeping thoughs?

whyrusleeping · 2017-03-07T01:13:22Z

core/coreunix/add.go

@@ -67,8 +67,15 @@ type AddedObject struct {
 	Bytes int64  `json:",omitempty"`
 }

-func NewAdder(ctx context.Context, p pin.Pinner, bs bstore.GCBlockstore, ds dag.DAGService) (*Adder, error) {
-	mr, err := mfs.NewRoot(ctx, ds, unixfs.EmptyDirNode(), nil)
+func NewAdder(ctx context.Context, p pin.Pinner, bs bstore.GCBlockstore, ds dag.DAGService, v int) (*Adder, error) {


Lets not change the parameters here. Instead, lets just add a SetCidVersion method to the adder.

whyrusleeping · 2017-03-07T01:14:38Z

importer/helpers/dagbuilder.go

@@ -33,6 +35,9 @@ type DagBuilderParams struct {
 	// instead of using the unixfs TRaw type
 	RawLeaves bool

+	// Prefix specifies cid version and hashing function


CidVersion only specifies the cid version, not the hashing function

Sorry, fixed. It was a leftover comment from an earlier attempt.

whyrusleeping · 2017-03-07T01:15:40Z

importer/helpers/dagbuilder.go

@@ -139,7 +155,11 @@ func (db *DagBuilderHelper) GetNextDataNode() (*UnixfsNode, error) {
 			raw:     true,
 		}, nil
 	} else {
-		blk := NewUnixfsBlock()
+		blk := &UnixfsNode{


no NewUnixfsBlock anymore?

Correct. It was only used in one place so I just inlined the code. I removed the function to make sure I cached all possible places where it was called to make sure the correct Cid version was always set.

If you want I can turn it into a method like I did for NewUnixfsNode. But since it was only used in one place I decided it wasn't worth the effort.

Yeah... just to maintain continuity with the previous methods. the whole 'raw' thing here is realllly just legacy code to prevent hashes from changing at this point

whyrusleeping · 2017-03-07T01:56:02Z

mfs/ops.go

@@ -104,7 +105,7 @@ func PutNode(r *Root, path string, nd node.Node) error {

 // Mkdir creates a directory at 'path' under the directory 'd', creating
 // intermediary directories as needed if 'mkparents' is set to true
-func Mkdir(r *Root, pth string, mkparents bool, flush bool) error {
+func Mkdir(r *Root, pth string, mkparents bool, flush bool, cidVer dag.CidVersion) error {


Lets have this be a field on the Root object, that way we can avoid changing the mfs APIs

Good idea. Done.

whyrusleeping

I'd like to avoid changing the existing apis on these things.
Other than those comments though, this LGTM.

kevina · 2017-03-08T20:28:09Z

@whyrusleeping okay, this should be ready now

kevina · 2017-03-17T20:11:31Z

@whyrusleeping target this for 0.4.8? Or do you want to try for a global option instead?

whyrusleeping · 2017-03-18T05:30:08Z

importer/helpers/dagbuilder.go

@@ -33,6 +35,9 @@ type DagBuilderParams struct {
 	// instead of using the unixfs TRaw type
 	RawLeaves bool

+	// CID Version to use
+	CidVersion dag.CidVersion


I think instead of using a struct for this, we should just use a cid.Prefix. That way we make it much easier for us to switch the hash function in the future

whyrusleeping

Construction looks good to me, Just want to switch to setting the whole cid prefix instead of just the version. Will make future changes of this sort much easier on us

kevina · 2017-03-18T23:34:05Z

@whyrusleeping okay, changed to setting the prefix instead of just the CID version

whyrusleeping · 2017-03-19T21:34:02Z

merkledag/node.go

+}
+
+func (n *ProtoNode) SetPrefix(prefix cid.Prefix) {
+	n.Prefix = prefix


Lets make sure to set n.Prefix.Codec = cid.DagProtobuf, just for sanitys sake

Good idea will do.

Well it turned out not to be such a good idea...

kevina · 2017-03-25T03:06:27Z

So commit bb09ffd (implement an HAMT for unixfs directory sharding) complicated things. It should have a fix by the end of Monday.

whyrusleeping · 2017-03-25T03:28:47Z

@kevina alright, i'm gonna bump this to 0.4.9 then. Thanks!

kevina · 2017-03-27T06:45:08Z

@whyrusleeping so setting the prefix for the MFS root after the fact became problematic. Have a look at the commit "Fixes" and let me know what you think. This commit will be squashed but I kept it separate to make it easier to see the new changes.

whyrusleeping · 2017-03-27T18:00:19Z

core/coreunix/add.go

 }

 func (adder *Adder) SetMfsRoot(r *mfs.Root) {
-	adder.mr = r
+	adder.mroot = r


I'd prefer not changing the naming of things, but not a big deal

I can change it back. I did this as a trick to make sure I cached where all instance mr was used. I thought about naming the method mr() but that seamed a bit terse for a method name, even if it was only for internal use.

whyrusleeping · 2017-03-27T18:02:33Z

unixfs/io/dirbuilder.go

+	if d.dirnode != nil {
+		d.dirnode.SetPrefix(prefix)
+	}
+	// FIXME: Should we do this? -- kevina


Yeah, we should probably set the shards prefix info too

Okay, will do. Have something in a few days.

whyrusleeping · 2017-03-30T00:57:59Z

@kevina update here?

kevina · 2017-03-30T01:01:11Z

@whyrusleeping Have something by the end of tomorrow, if not today.

kevina · 2017-03-31T05:06:03Z

@whyrusleeping sorry for the delay, this should be good to go. I will squash the "Fixes" commit once you are okay with my changes.

whyrusleeping

LGTM, just needs a rebase 👍

whyrusleeping · 2017-04-26T02:15:35Z

Oh, and could you fix the codeclimate issues?

License: MIT Signed-off-by: Kevin Atkinson <[email protected]>

…n nil License: MIT Signed-off-by: Kevin Atkinson <[email protected]>

License: MIT Signed-off-by: Kevin Atkinson <[email protected]>

kevina · 2017-04-26T03:49:24Z

Rebased but tests are failing again, will fix.

kevina · 2017-04-26T03:49:47Z

But codeclimate is at least happy.

kevina · 2017-04-26T05:51:16Z

Some how "make deps" in the shareness directory was not rebuilding things correctly for me. In any case we a green and this is now ready to merge.

kevina added the status/in-progress In progress label Mar 3, 2017

kevina force-pushed the kevina/cidv1-add branch 2 times, most recently from 65ea7f3 to 33c65ba Compare March 3, 2017 22:52

kevina changed the title ~~WIP: adder: add support for using CidV1~~ adder: add support for using CidV1 Mar 3, 2017

kevina requested a review from whyrusleeping March 3, 2017 23:02

Kubuxu reviewed Mar 5, 2017

View reviewed changes

whyrusleeping reviewed Mar 5, 2017

View reviewed changes

whyrusleeping reviewed Mar 7, 2017

View reviewed changes

whyrusleeping requested changes Mar 7, 2017

View reviewed changes

kevina force-pushed the kevina/cidv1-add branch 3 times, most recently from 48d361f to 2ba6d8a Compare March 8, 2017 01:48

whyrusleeping added this to the Ipfs 0.4.8 milestone Mar 18, 2017

whyrusleeping reviewed Mar 18, 2017

View reviewed changes

whyrusleeping requested changes Mar 18, 2017

View reviewed changes

whyrusleeping reviewed Mar 19, 2017

View reviewed changes

kevina force-pushed the kevina/cidv1-add branch from 70fd13d to 4df10c0 Compare March 19, 2017 22:01

whyrusleeping modified the milestones: Ipfs 0.4.9, Ipfs 0.4.8 Mar 25, 2017

kevina force-pushed the kevina/cidv1-add branch 2 times, most recently from cef50ea to 5adb2a3 Compare March 27, 2017 04:20

whyrusleeping reviewed Mar 27, 2017

View reviewed changes

whyrusleeping approved these changes Apr 26, 2017

View reviewed changes

kevina added 7 commits April 25, 2017 23:01

adder: fix bug where errors from calls to NewAdder caused daemon to hang

b7eae36

License: MIT Signed-off-by: Kevin Atkinson <[email protected]>

adder: make chan size a constant

cf56240

License: MIT Signed-off-by: Kevin Atkinson <[email protected]>

merkledag: provide better diagnostics when Prefix.Sum fails

9b58fa7

License: MIT Signed-off-by: Kevin Atkinson <[email protected]>

adder: add support for using CidV1

518adec

License: MIT Signed-off-by: Kevin Atkinson <[email protected]>

merkeldag: change SetPrefix param to a pointer and reset the prefix o…

076d0bd

…n nil License: MIT Signed-off-by: Kevin Atkinson <[email protected]>

hamt: support using CIDv1 by allowing the prefix to be set

8a47786

License: MIT Signed-off-by: Kevin Atkinson <[email protected]>

Documentation

9350c51

License: MIT Signed-off-by: Kevin Atkinson <[email protected]>

kevina force-pushed the kevina/cidv1-add branch from 66b3194 to 9350c51 Compare April 26, 2017 03:48

kevina added RFM and removed status/in-progress In progress labels Apr 26, 2017

whyrusleeping merged commit e5529cd into master Apr 27, 2017

whyrusleeping deleted the kevina/cidv1-add branch April 27, 2017 01:54

kevina mentioned this pull request May 2, 2017

Add flag to switch over to using CIDv1 #3243

Open

Add support for using CidV1 in 'ipfs add' #3743

Add support for using CidV1 in 'ipfs add' #3743

Conversation

kevina commented Mar 3, 2017

kevina commented Mar 3, 2017

kevina commented Mar 3, 2017

whyrusleeping commented Mar 3, 2017

kevina commented Mar 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kubuxu Mar 5, 2017 • edited Loading

Choose a reason for hiding this comment

Kubuxu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kubuxu Mar 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kubuxu commented Mar 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevina commented Mar 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whyrusleeping left a comment

Choose a reason for hiding this comment

kevina commented Mar 8, 2017

kevina commented Mar 17, 2017

Choose a reason for hiding this comment

whyrusleeping left a comment

Choose a reason for hiding this comment

kevina commented Mar 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevina Mar 24, 2017 • edited Loading

Choose a reason for hiding this comment

kevina commented Mar 25, 2017

whyrusleeping commented Mar 25, 2017

kevina commented Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whyrusleeping commented Mar 30, 2017

kevina commented Mar 30, 2017 • edited Loading

kevina commented Mar 31, 2017

whyrusleeping left a comment

Choose a reason for hiding this comment

whyrusleeping commented Apr 26, 2017

kevina commented Apr 26, 2017

kevina commented Apr 26, 2017

kevina commented Apr 26, 2017

Kubuxu Mar 5, 2017 •

edited

Loading

Kubuxu Mar 5, 2017 •

edited

Loading

kevina Mar 24, 2017 •

edited

Loading

kevina commented Mar 27, 2017 •

edited

Loading

kevina commented Mar 30, 2017 •

edited

Loading