continuation of conversion of multihash to string

[kevina]

Continuation of #83 using parts of #82.

[Stebalien]

(Code, Length, Digest). However, really, we can just return (Code, Digest).

[kevina]

Yeah your right since the length is primary useful in the binary representation as a sanity check and to know when the multihash ends in a byte stream.

[Stebalien]

Any reason not to have this return an error? I don't see this being called so often checking an error becomes onerous. For example, I expect we'll want to use this from go-cid when validating the CID's multihash.

(It may also be useful to have a FromBinaryUnchecked that doesn't even check but that's a separate issue).

[kevina]

I wanted something that was fast and convenient to use when we know the multihash is already valid. The error checking that is done are sanity checks. For example if the hash length is incorrect or bogus this will likely panic due to an out of bounds index check rather than return a proper error message.

I am somewhat against FromBinaryUnchecked if we can get away with (performance wise) always doing the checks. This way we know that the Multihash is valid if it is not empty.

See ipfs/go-cid#73 for how I use this. I am less against fixing this to return an error.

[Stebalien]

Got it. I'd like to expose some FromBinary function that returns an error. Currently, go-cid will have to allocate to check the multihash. That is, it'll:

1. Pass a []byte into mh.Cast.
2. mh.Cast will return a string backed multihash (allocating).

A FromBinary that returns an error would allow us to check without panicing or allocating.

[kevina]

Okay. I created such a function and called it New.

[Stebalien]

Why New? It doesn't really create a new multihash.

I am somewhat against FromBinaryUnchecked if we can get away with (performance wise) always doing the checks. This way we know that the Multihash is valid if it is not empty.

But we aren't always doing the checks! FromString leaves out one of the checks. Let's either check everything or nothing, checking somethings buys us nothing and just adds a bunch of code and two confusing functions that do slightly different things.

[kevina]

Okay fair enough. I renamed New to FromBinary when the Cid needs to return a Multihash it can just ignore the errors.

If necessary e can add an unchecked version if this becomes a performance problem (which I doubt).

[Stebalien]

SGTM. In that case, we can also profile which checks may still be worth keeping (although, in the method contract, I'd still just say "may panic, may not, don't feed us bad data"; debug checks shouldn't be relied on).

[kevina]

I'm confused, what are you advocating. A FromBinary() method function that returns an error or a FromBinary() function that panics?

[Stebalien]

As you said, we can consider adding an unchecked variant later. My point was that we can actually put "checks" in the unchecked version (as long as they're fast enough) as long as we don't make any promises. My issue with the previous FromBinary incarnation is that it sort of checked and made unclear guarantees. I'd rather have a fully checked version (that returns the error) and a version that makes absolutely no guarantees (but can choose to check under the covers).

TL;DR: I'm happy with the current version, we can take a look into a less checked variant later.

[kevina]

@Stebalien Do you want to get this is now with the Cid changes?

[Stebalien]

If you feel secure in your gx-fu. The package list is quite long:

go-libp2p-peer
go-multiaddr
go-multiaddr-net
mafmt
go-libp2p-peerstore
go-libp2p-net
go-conn-security
go-libp2p-secio
iptb
go-ipfs-util
go-libp2p-loggables
go-libp2p-transport
go-reuseport-transport
go-maddr-filter
go-libp2p-transport-upgrader
go-tcp-transport
go-testutil
go-libp2p-metrics
go-libp2p-interface-connmgr
go-libp2p-host
go-addr-util
go-conn-security-multistream
go-libp2p-swarm
go-libp2p-nat
go-libp2p-netutil
go-libp2p-blankhost
go-libp2p-circuit
go-multiaddr-dns
go-ws-transport
go-libp2p
go-cid
go-libp2p-record
go-libp2p-kbucket
go-libp2p-routing
go-libp2p-kad-dht
hang-fds
go-libp2p-floodsub
go-block-format
go-ipld-format
go-ipld-cbor
go-ipfs-cmds
go-ipld-git
go-libp2p-connmgr
go-ipfs-addr
go-ipfs-posinfo
go-ipfs-chunker
go-ipfs-ds-help
go-ipfs-routing
go-ipfs-blockstore
go-fs-lock
go-ipfs-exchange-interface
go-ipfs-blocksutil
go-ipfs-exchange-offline
go-libp2p-routing-helpers
go-libp2p-pubsub-router
go-ipns
go-ipfs

The package list for go-cid is only:

go-libp2p-routing
go-libp2p-kad-dht
go-block-format
go-ipld-format
go-ipld-cbor
go-ipld-git
go-ipfs-posinfo
go-ipfs-chunker
go-ipfs-ds-help
go-ipfs-routing
go-ipfs-blockstore
go-ipfs-exchange-interface
go-ipfs-blocksutil
go-ipfs-exchange-offline
go-libp2p-routing-helpers
go-libp2p-pubsub-router
go-ipfs

Regardless, let's be very careful not to get stuck half-way (i.e., no pushing to master until the end).

[kevina]

@Stebalien I can try (the full gx-update with multihash). It may be helpful if I do it at a time when you are around so you can walk me though and make sure everything looks good.

So that I am confident you are okay with all the changes please Approve this p.r., ipfs/go-cid#71 and ipfs/go-cid#73.

[Stebalien]

Only @kevina should merge this. CI fails because this breaks go-libp2p-peer.

@kevina modulo the one comment on go-cid, this all looks good to me. I'd:

1. Start by not using gx. Instead, just fix all the appropriate packages (picking up where I left off), testing with go.
2. Once that all works, we can use gx-workspace to make this happen (if it looks too gnarly, I can take a stab at this; this'll be interesting...).

[lidel]

Closing due to being stale since 2018 + merge conflicts.