Skip to content
This repository has been archived by the owner on Dec 23, 2020. It is now read-only.

Add Blake2b support #113

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add Blake2b support #113

wants to merge 1 commit into from

Conversation

jminer
Copy link
Contributor

@jminer jminer commented Aug 29, 2017

The implementation was ported from Bouncy Castle. It passes the included tests on both the VM and in Firefox. I got the hash values from b2sum, the reference implementation. I changed the digest test method to test both update() and updateByte() instead of just update(). The tests don't include passing a key, salt, or personalization. Let me know if you want me to add that in this PR.

I changed Register64.setRange() to behave like List.setRange() in the standard library. Before, it always copied corresponding indexes, even if you passed a start != 0. No existing uses depended on this (surprising to me) behavior.

I did some brief optimization. Making the m list a member variable instead of a local made the benchmark about 6% faster (because it reduces allocations?). Adding a Register64.sumReg() method that doesn't have a branch for int made it an additional ~6% faster. I'm not sure it can be optimized further, as most of the time is spent in Register64 methods. Now on my i7-6700K 4.0GHz processor, I get about 40.5 MB/s.

The implementation was ported from Bouncy Castle.
@@ -254,6 +254,13 @@ class Register64 {
}
}

void sumReg(Register64 y) {
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind adding a line of comment to this method?

Also, do you think other digest implementation could benefit from this method?

Copy link
Member

Choose a reason for hiding this comment

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

With this tiny change I can merge the PR :)

@stevenroose
Copy link
Member

Sorry for the late reply btw. In my e-mail I had it starred, but I assumed it was an issue instead of a PR :)

@stevenroose
Copy link
Member

Looks great overall! Have you been using this in a working project? If you can address the one tiny nit, I'll try to get this merged asap.

@jminer
Copy link
Contributor Author

jminer commented Feb 20, 2018

Sorry, I've been working on other projects and lazy getting back to this, but I'm happy to fix what you mentioned. I used this code as a dependency to implement https://github.com/jminer/librsync-dart because it uses Blake2b to hash blocks, and the Dart library's output matches or is better than the C library's (and the Dart version is only 15% as many lines of code).

@stevenroose
Copy link
Member

We recently merged some big changes regarding the registry, so could you perhaps rebase this on top of master so that I could merge it? :)

@bbedward
Copy link
Contributor

I rebassd this pull request in my own fork (https://github.com/bbedward/pointycastle) as I need it for a flutter crypto currency wallet I'm working on. It passes all my unit tests and I'm using it in a modified tweetnacl (that uses blake2b instead of sha-512). I could pull request from my fork if you'd like

@stevenroose
Copy link
Member

@bbedward yes, please do! But try to keep the changes confined to just adding the Blake2b addition without other changes.

Thanks!

@bbedward bbedward mentioned this pull request Jan 11, 2019
@izaera
Copy link
Member

izaera commented Dec 23, 2020

I'm archiving this repo as per #239 (see the issue for more info).

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.

4 participants