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

Work with Multihashes directly #21

Merged
merged 4 commits into from
Feb 28, 2020
Merged

Work with Multihashes directly #21

merged 4 commits into from
Feb 28, 2020

Conversation

hsanjuan
Copy link
Contributor

@hsanjuan hsanjuan commented Feb 3, 2020

This will be breaking for anyone using filestore who has not migrated
the underlying storage and coverted CIDs to raw multihashes.

It keeps the outside interfaces intact except for ListRes which includes
a multihash rather than a Cid.

@hsanjuan hsanjuan self-assigned this Feb 3, 2020
filestore.go Show resolved Hide resolved
util.go Outdated Show resolved Hide resolved
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Other than the nits, my only real concern is that switching everything to multihashes is, strictly speaking, throwing away correct information that we have. We only reference "raw leaves" with the filestore so we always know the codec.

filestore.go Show resolved Hide resolved
fsrefstore.go Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
util.go Outdated Show resolved Hide resolved
util.go Outdated Show resolved Hide resolved
util.go Outdated Show resolved Hide resolved
util.go Outdated Show resolved Hide resolved
@hsanjuan
Copy link
Contributor Author

hsanjuan commented Feb 6, 2020

switching everything to multihashes is, strictly speaking, throwing away correct information that we have

We are doing this in general right? IPFS will lose any clue on what type of content it is storing in the datastore, other than that provided by context (in this case knowing that /filestore things are raw). But that was the case with CID v0s anyways..

@Stebalien
Copy link
Member

We are doing this in general right? IPFS will lose any clue on what type of content it is storing in the datastore, other than that provided by context (in this case knowing that /filestore things are raw). But that was the case with CID v0s anyways..

Not quite.

  • The blockstore stores blocks with different versions/formats. We're intentionally throwing this information away so we don't have to re-download blocks when we're just changing the CID version.
  • The filestore only stores CIDv1-raw blocks so we don't have this issue.

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Now we just need the filestore migration.

@Stebalien
Copy link
Member

Comment on where this lives in the migration process (context for anyone wondering):

This change isn't strictly speaking necessary for stage 1 as (a) the blockstore interfaces still use CIDs and (b) all blocks in the filestore will be cidv1 raw blocks. However, when we migrate the blockstore interfaces to multihashes, we'd either have to convert inbound multihashes back into CIDs when looking them up in the filestore's section of the datastore or we'd have to migrate at that point.

Migrating now and using multihashes everywhere is likely the cleanest solution.

@hsanjuan
Copy link
Contributor Author

hsanjuan commented Feb 7, 2020

Migrating now and using multihashes everywhere is likely the cleanest solution.

I'll add this to the migration

@hsanjuan
Copy link
Contributor Author

hsanjuan commented Feb 7, 2020

Do not merge: I need to set the right go-ipfs-blockstore dependency here before merging

This will be breaking for anyone using filestore who has not migrated
the underlying storage and coverted CIDs to raw multihashes.

It keeps the outside interfaces intact except for `ListRes` which includes
a multihash rather than a Cid.
@hsanjuan hsanjuan merged commit c64f279 into master Feb 28, 2020
@hsanjuan hsanjuan deleted the update/raw-mh branch February 28, 2020 13:07
@hsanjuan
Copy link
Contributor Author

I have fixed the deps and merged. I tagged v1.0.0 even if this was not breaking and things worked without needing a migration because all filestore blocks are CidV1.Raw. Just in case someone, somewhere, has done something else with this or misused it somehow.

@aschmahmann aschmahmann mentioned this pull request Dec 13, 2021
59 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants