chore: replace blake2b implementation by golang.org/x/crypto #157
Conversation
Jorropo
force-pushed
the
x/crypto
branch
2 times, most recently
from
2d9b6cf
to
fa6025f
Compare
|
@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. |
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'm surprised there is no blake2 tests, I'll add some, likely blake2's official test set. |
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. |
|
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. |
|
/cc @aschmahmann I have added blake2b tests, tested both on the old and new implementation they both work. RFM on my side |
Jorropo
force-pushed
the
x/crypto
branch
2 times, most recently
from
b22b476
to
5e5dc16
Compare
Fair enough, I belive the goal was more to ensure the new registering is done correctly. |
|
@Kubuxu I have fixed the lint rule, could I pls get this merged so I can open the bump PR in go-ipfs ? |
|
Hey, tests seem to fail on the CI. |
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
|
@Kubuxu I don't know if it's waiting on you, you are just the only one with merge permissions in the conversation afait. |
|
Thx, sorry for pinging you Kubuxu, weird that Marten only show up as Contributor. |
This remove github.com/minio/blake2b-simd and replace it with golang.org/x/crypto/blake2b which slightly faster and reduce the code surface. See multiformats/go-multihash#157 for more info.
This remove github.com/minio/blake2b-simd and replace it with golang.org/x/crypto/blake2b which slightly faster and reduce the code surface. See multiformats/go-multihash#157 for more info.
This remove github.com/minio/blake2b-simd and replace it with golang.org/x/crypto/blake2b which slightly faster and reduce the code surface. See multiformats/go-multihash#157 for more info.
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).