multiformats / go-multihash

Multihash implementation in Go
MIT License
239 stars 57 forks source link

chore: replace blake2b implementation by golang.org/x/crypto #157

Closed Jorropo closed 2 years ago

Jorropo commented 2 years ago

In 2016-12-13 d8e61c69ab46ca38328da2f4995abaf93b252290 golang.org/x/crypto gained a blake2b AVX implementation.

There is also an AVX2 one.

It is faster than what we are using right now.

This commit allows for faster code and remove a dependency.

For future improvements (like NEON) /x/crypto seems more maintained (it isn't an archived repo).

benchmark              old ns/op     new ns/op     delta
BenchmarkSum128-12     252           140           -44.35%
BenchmarkSum1K-12      1221          986           -19.24%

benchmark              old MB/s     new MB/s     speedup
BenchmarkSum128-12     507.37       911.98       1.80x
BenchmarkSum1K-12      838.70       1038.43      1.24x
aschmahmann commented 2 years ago

@Kubuxu any issues with merging?

@Jorropo not blocking here, but it'd be nice if there was some verification that the hashes from the different blake2 libraries are the same. AFAICT we don't have any here, either a Go test or adding to https://github.com/multiformats/multihash/ and bumping the dependency here would be great.

marten-seemann commented 2 years ago

not blocking here, but it'd be nice if there was some verification that the hashes from the different blake2 libraries are the same

That would need to live in a separate Go module though, otherwise the dependency would stay in go.mod. Or maybe a one-time test would be enough to convince us that the libraries are doing the same?

Jorropo commented 2 years ago

I'm surprised there is no blake2 tests, I'll add some, likely blake2's official test set.

aschmahmann commented 2 years ago

That would need to live in a separate Go module though, otherwise the dependency would stay in go.mod. Or maybe a one-time test would be enough to convince us that the libraries are doing the same?

I was thinking that if you did a PR with the tests separately from this one we could merge that, and then rebase this PR on top of master and make sure if passes. Since the tests would be hitting test vectors that are constant it'd be fine.

Jorropo commented 2 years ago

I dont think we actually need to check boths, blake2 is a spec if an implementation isnt following it its wrong and that likely a big security issue.

The tests can just compare to well known values.

Jorropo commented 2 years ago

/cc @aschmahmann I have added blake2b tests, tested both on the old and new implementation they both work.

RFM on my side

Jorropo commented 2 years ago

I trust golang/x/crypto to implement Blake2 correctly.

Fair enough, I belive the goal was more to ensure the new registering is done correctly. Maybe one used to take bits now it takes byte sizes, or something weird like that.

Jorropo commented 2 years ago

@Kubuxu I have fixed the lint rule, could I pls get this merged so I can open the bump PR in go-ipfs ?

Kubuxu commented 2 years ago

Hey, tests seem to fail on the CI. Sorry for the delay, I didn't know you were waiting on me.

Jorropo commented 2 years ago

@Kubuxu Sorry for the tests I had fixed it locally it but forgot to push it.

I don't know if it's waiting on you, you are just the only one with merge permissions in the conversation afait.

Jorropo commented 2 years ago

Thx, sorry for pinging you Kubuxu, weird that Marten only show up as Contributor.