Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support go-multihash hashing in LinkSystem #145

Closed
wants to merge 1 commit into from

Conversation

hannahhoward
Copy link
Collaborator

Goals

This is a strawman proposal for how to solve our various incompatibilities between go-multihash and the desire to write a new system for go-ipld-prime.

Per #143 (comment) -- it sounds like there is a desire to do a ground up rethink of go-multihash. We don't want to block that effort, but we also need to use go-multihash and go-cid to insure compatibility as we integrate IPLD prime in IPFS. Per #143 (comment) & #143 (comment) there is really no gaurantee a new system will work with all existing IPFS code without a lot of effort, and we don't want to undertake that now.

So, this is a proposal to allow for a rewrite while still allowing access to the tables in go-multihash should you want them. It also includes one change to go-multihash and one change to go-cid to enable this.

Implementation

  • Instead of using Hash.Hash, define a Hasher with the actual methods used: io.Writer & Sum
  • Expose function to sum bytes w/o Multihash encoding in go-multihash
  • Provide a Hasher implementation based on go-multihash
  • Expose a function on go-cid to sum a cid.Prefix from a digest (to clean up LinkPrototype.BuildLink for cidlink)
  • Expose individual encoder/decoder/hasher functions to make assembling custom link system quicker
  • Put back in byteAccessor cause we need the optimization (this is purely a @hannahhoward desire)

For discussion

I'm not sure this is the right final solution, or even a great one. However, I'm gonna officially say "my work here is done" -- because I am not a steward of go-multihash or go-cid, and I think at this point @warpfork and @Stebalien should decide a long term plan together and whether this works well for now.

@hannahhoward
Copy link
Collaborator Author

BTW, @warpfork one other compromise here I realized later we can make is to look up the new hasher in the table, and if it doesn't exist, delegate to the go-multihash implementation. That means the new code would still get used but we could rely on go-multihash as a backup.

@hannahhoward
Copy link
Collaborator Author

Presumably if it was important we could use hash.Hash as the baseline interface for now. Since the other methods are unused so we could just right empty implementations for the mhhash

@warpfork
Copy link
Collaborator

Okay, @Stebalien and @aschmahmann and I just had a conversation about this whole (waves widely) scenario, and it seems like:

  • we are all pretty much aligned on using hash.Hash as a baseline
    • (we did also talk about the subset interface idea, and it's got good parts, but ultimately decided in pareto prevalence, we'd probably write more glue code to write the re-typed factory functions than we'd write in the much rarer glue to expand something to hash.Hash.)
  • we... are probably just going bite the bullet, immediately and in its entirety, and just bring a bunch of this work into go-multihash next week.

So we'll see how that goes, but if it goes well, that'll remove any need for any more bridgery fiddlefaddle.

Base automatically changed from linksystem to master March 12, 2021 16:50
@warpfork
Copy link
Collaborator

Successfully got all this hairy stuff upstreamed, so we happily no longer need to consider this direction :)

@warpfork warpfork closed this Mar 12, 2021
@warpfork warpfork deleted the feat/multihash branch March 12, 2021 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants