WIP: Use new blockstore that stores Multihashes and not CIDs. #5510

kevina · 2018-09-22T07:36:15Z

Depends on ipfs/go-ipfs-blockstore#13.

Will not even compile yet.

Pressing issues that need to be addressed:

The dagservice has a Delete method. This is problematic. For example if you insert a raw and a Protobuf version of the same Node then delete the one of those nodes via the dagservice both will be deleted. We should probably delete these methods. The only place the Delete method seams to be used is in dagutils/utils.go. I have marked two of the three instances with comments. Also see RFC: Remove Delete methods from DagService go-ipld-format#43
Due to the choice to keep Bitswap using Cid's there is one place where we have multihashes but need Cid's. For now I converted them to Raw CIDs for lack of anything better to do. See NewBlockstoreProvider in exchange/reprovide/providers.go (https://github.com/ipfs/go-ipfs/pull/5510/files#diff-a23df8bd7581d29e09f90ef684b34631L16). Is this function even used, that is do we ever publish the complete lists of everything in the blockstore, and if so will this create a problem.

As an aside, in these cases I believe we should introduce a new Unknown or Invalid codec type to mark the fact that we really don't know the original Codec as it was created via a multihash. Raw may be technically correct but is less informative. This was discussed in blockstore: switch from Cid to Multihash boxo#361 (with the last comment being blockstore: switch from Cid to Multihash boxo#361)
Raw multihashes will now be exposed to the users in several places. Two of those places are ipfs refs local and ipfs repo gc. How should this be handled. Here are some options (a) Use the default string method for multihashes which is a hex string (with no multibase prefix) (b) Tack on a multibase prefix and then display it like a CID (c) Make it a Cid using the Raw codec and display that (d) Make it a Cid using a new codec type to specify an Unknown or Invalid codec as discussed in (2). Right now am using (a) as it makes it very obvious that what is being returned is not a Cid as cids are hardly ever encoded in hex. (b) and (c) don't make it obvious what is going on. The special codec type might be helpful in other situations but it is less obvious than (a) on visual inspection.

CC @Stebalien @whyrusleeping

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

kevina · 2018-09-24T05:57:35Z

dagutils/utils.go

@@ -70,7 +70,7 @@ func addLink(ctx context.Context, ds ipld.DAGService, root *dag.ProtoNode, child
 		return nil, err
 	}

-	_ = ds.Remove(ctx, root.Cid())
+	//_ = ds.Remove(ctx, root.Cid())


What is the purpose of this call to remove.

kevina · 2018-09-24T06:03:34Z

dagutils/utils.go

@@ -124,7 +124,7 @@ func (e *Editor) insertNodeAtPath(ctx context.Context, root *dag.ProtoNode, path
 		return nil, err
 	}

-	_ = e.tmp.Remove(ctx, root.Cid())
+	//_ = e.tmp.Remove(ctx, root.Cid())


This is working on a temporary in memory dag service. Maybe we should (1) implement a direct in memory dag service that does that even use a blockservice or (2) just use a different data structure, like maybe something as simple as a go map?

Kubuxu · 2018-10-09T18:35:51Z

How would publishing providers records work with this?

Stebalien · 2018-10-10T11:48:20Z

1

Honestly, I think we can just keep it and say "be careful". We're not really going to be making this worse because we can already (accidentally) reuse nodes from other DAGs. Basically, there's no way to know that dag.Delete(cid) won't interfere with some other DAG. A safe interface would use pins or a key-value store but we can work on designing that later.

2

This will, unfortunately, break bitswap compatibility with existing nodes. This is also a problem for providing. We'll have to pull in @diasdavid and @whyrusleeping because I know they've done quite a bit of thinking on this but here are some ideas:

DAG-based-bitswap

We can use a DAGService instead of a Blockservice to drive bitswap. We'll do this with graphsync anyways.

a. Unfortunately, for full compatibility, we may still have to issue bitswap queries and provider records for both CID versions (which is going to suck and may yield duplicate blocks). However, we can (a) make all nodes accept any CID version and (b) eventually stop sending CIDv0 queries. IMO, this is the cleanest solution from a "where we're going" standpoint.

b. We could also push this issue up the stack and say that the user (programmer) must make both requests if they're unsure (e.g., they've received a base32 CID from the API). This may be the best solution from a performance standpoint but it will annoying to use...

c. For slightly worse compatibility, we could forcibly downgrade all CIDv1 DagPB CIDs to CIDv0 when we hit the provider system/bitswap. However, that's going to suck for IA (which uses CIDv1 DagPB CIDs, IIRC). It's also completely backwards because we're trying to upgrade to CIDv1, not downgrade.

Just use multihashes

Allow raw multihashes and eventually switch over to them entirely. Basically:

A CID will (for now) always start with 0x01 and a multihash will start with something else.
CIDv0 CIDs are already valid raw multihashes.

This means CIDv0 CIDs will still work. However, this is kind of crappy from a compatibility perspective and looses information relative to the dag-based-bitswap solution.

Upgrade the network

This is basically the solution you've presented plus an upgrade path:

Make bitswap serve requests regardless of the codec (i.e., plug in a blockstore that doesn't care about the codec).
Upgrade the network.
Start asking for blocks using the raw codec.

However, this would be a major break.

New Bitswap Version

We can, in fact, introduce a new bitswap version. This solution can be combined with one of the other solutions. For example, it can be combined with DAG-based-bitswap, variant a: Per-peer, bitswap decides to either send (a) both CIDs or (b) a single CIDv1 CID (or a raw multihash, or a raw CID, etc).

3

So, multihashes + multibase is probably the most correct solution. Unfortunately, they're ambiguous with CIDs so it's probably confusing from a user perspective.

As you say, we could also use hex-based multihashes (0x...). However, that would be the only place in all of IPFS where we'd be using them so I'm a bit reluctant to go with that. Basically, I'd rather not introduce a new concept to users.

Honestly, I think raw nodes is probably the right balance. We could also have a special codec for "unknown" but I think we should talk to users to see what they think before we add a new codec. My primary reservation is that I'd rather not (a) have a codec that can't be decoded or (b) have a codec duplicating raw. But we'll have to discuss this with users.

kevina · 2018-10-11T21:15:12Z

@Stebalien when you get a minute, can you outline the results of our discussion on lunch today (Thursday the 11th of October) to make sure we are all on the same page.

kevina · 2018-10-12T08:11:25Z

@Stebalien

However, that would be the only place in all of IPFS where we'd be using them so I'm a bit reluctant to go with that. Basically, I'd rather not introduce a new concept to users.

I think it better to introduce a new concept rather then mislead users. By using the raw codec we might give the user the impression that nothing but raw leaves are stored in the blockstore.

We could also have a special codec for "unknown" but I think we should talk to users to see what they think before we add a new codec. My primary reservation is that I'd rather not (a) have a codec that can't be decoded or (b) have a codec duplicating raw. But we'll have to discuss this with users.'

I am going to have to disagree with you on this one. For (a) that is kind of the point. It is a red flag that something is special about this CID. For (b) I do not consider it a duplicate at all, it conveys a different meaning.

kevina · 2018-10-12T08:14:24Z

1

Honestly, I think we can just keep it and say "be careful". We're not really going to be making this worse because we can already (accidentally) reuse nodes from other DAGs. Basically, there's no way to know that dag.Delete(cid) won't interfere with some other DAG. A safe interface would use pins or a key-value store but we can work on designing that later.

I am not exactly conformable with this option, but you are not going to get much of an argument from me.

Stebalien · 2018-10-16T20:22:22Z

Conclusions from the lab-week discussion:

The blockservice/blockstore continue to take CIDs for Add/Get. The blockservice returns multihashes from AllNodesChan. I believe this is what you're already doing.
Bitswap continues to work with CIDs.
We change provide and find providers to take raw multihashes.
Locally, wantlists need to be indexed by multihash, not CID. That is map[Multihash][]Cid (or something like that).

kevina · 2018-10-16T20:41:11Z

@Stebalien thanks. I work to push those changes though and let you know if I run into any problems.

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

kevina · 2018-10-29T08:04:12Z

This depends on:

RFC: Change Blockstore to work with Multihashes instead of CIDs. go-ipfs-blockstore#13
WIP: Datastore keys are now based on Multihash not Cid. go-ipfs-ds-help#4
WIP: Modify blockservice to work with Multihashes instead of CIDs. go-blockservice#8
WIP: Modify dag service to work with new multihash based blockservice. go-merkledag#17
WIP: Change ContentRouting interface to work with Multihash's not CIDs. libp2p/go-libp2p-routing#33
WIP: Changes to reflect the fact that Provide/FindProviders use multihashes libp2p/go-libp2p-kad-dht#203
WIP: Changes to reflect the fact that Provide/FindProviders use multihashes libp2p/go-libp2p-pubsub-router#14
WIP: Changes to reflect the fact that Provide/FindProviders now use multhases. libp2p/go-libp2p-routing-helpers#13
WIP: Changes to reflect the fact that Provide/FindProviders use multhases. go-bitswap#18
WIP: Changes to reflect the fact that Provide/FindProviders use multihashes. go-ipfs-routing#15

lidel · 2021-12-09T19:00:56Z

Superseded by #6816

Use new blockstore that stores Multihashes and not CIDs.

490fc98

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

kevina added the topic/cidv1b32 Topic cidv1b32 label Sep 22, 2018

kevina requested a review from Kubuxu as a code owner September 22, 2018 07:36

ghost assigned kevina Sep 22, 2018

ghost added the status/in-progress In progress label Sep 22, 2018

kevina commented Sep 24, 2018

View reviewed changes

kevina mentioned this pull request Sep 24, 2018

RFC: Remove Delete methods from DagService ipfs/go-ipld-format#43

Open

kevina requested a review from Stebalien October 3, 2018 01:07

kevina added 2 commits October 28, 2018 22:17

Partly revert last commit, the dagservice 'Remove' method is staying.

b1099cb

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

Convert the providers list to work with raw multihashes.

68dd6ca

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

kevina mentioned this pull request Oct 29, 2018

WIP: Change ContentRouting interface to work with Multihash's not CIDs. libp2p/go-libp2p-routing#33

Closed

lidel mentioned this pull request Mar 3, 2019

Migration towards base32 CIDv1 #5358

Open

8 tasks

koivunej mentioned this pull request Mar 10, 2020

BlockStore considerations rs-ipfs/rust-ipfs#84

Closed

Stebalien unassigned kevina Apr 22, 2021

lidel closed this Dec 9, 2021

lidel deleted the kevina/multihash branch December 9, 2021 19:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Use new blockstore that stores Multihashes and not CIDs. #5510

WIP: Use new blockstore that stores Multihashes and not CIDs. #5510

kevina commented Sep 22, 2018 •

edited

Loading

kevina Sep 24, 2018 •

edited

Loading

kevina Sep 24, 2018

Kubuxu commented Oct 9, 2018

Stebalien commented Oct 10, 2018

kevina commented Oct 11, 2018

kevina commented Oct 12, 2018

kevina commented Oct 12, 2018

Stebalien commented Oct 16, 2018

kevina commented Oct 16, 2018

kevina commented Oct 29, 2018

lidel commented Dec 9, 2021

WIP: Use new blockstore that stores Multihashes and not CIDs. #5510

WIP: Use new blockstore that stores Multihashes and not CIDs. #5510

Conversation

kevina commented Sep 22, 2018 • edited Loading

kevina Sep 24, 2018 • edited Loading

Choose a reason for hiding this comment

kevina Sep 24, 2018

Choose a reason for hiding this comment

Kubuxu commented Oct 9, 2018

Stebalien commented Oct 10, 2018

kevina commented Oct 11, 2018

kevina commented Oct 12, 2018

kevina commented Oct 12, 2018

Stebalien commented Oct 16, 2018

kevina commented Oct 16, 2018

kevina commented Oct 29, 2018

lidel commented Dec 9, 2021

kevina commented Sep 22, 2018 •

edited

Loading

kevina Sep 24, 2018 •

edited

Loading