Skip to content
This repository has been archived by the owner on Sep 26, 2019. It is now read-only.

[PAN-2842] Blake 2b tweaks #1862

Merged
merged 6 commits into from
Aug 19, 2019
Merged

[PAN-2842] Blake 2b tweaks #1862

merged 6 commits into from
Aug 19, 2019

Conversation

shemnon
Copy link
Contributor

@shemnon shemnon commented Aug 16, 2019

PR description

  • Adjust endianess on byte payload
  • f flag can have only 0 or 1
  • Fewer successful vector tests
  • Tests for failure and edge cases

Fixed Issue(s)

* Adjust endianess on byte payload
* f flag can have only 0 or 1
* Fewer successful vector tests
* Tests for failure and edge cases
* added a test for not-finalized value
Copy link
Contributor

@mbaxter mbaxter left a comment

Choose a reason for hiding this comment

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

Can you update the description with a link to the EIP and any relevant notes about these specific changes if there are any?

@@ -27,21 +27,21 @@
private Blake2bfMessageDigest messageDigest;

// output when input is all 0
private byte[] blake2bfAllZero =
private static final byte[] BLAKE2F_ALL_ZERO =
Copy link
Contributor

Choose a reason for hiding this comment

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

Where are these test cases coming from? Is there a spec we can link to? Would the tests be clearer if kept these values as hexary strings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests are not as useful as the precompiled test vectors. Those have more coverage. As far as I can tell they were calculated from the blake RFC.

@@ -224,7 +224,7 @@ private int bytesToInt(final byte[] bytes) {
}

private long bytesToLong(final byte[] bytes) {
return Pack.bigEndianToLong(bytes, 0);
return Pack.littleEndianToLong(bytes, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty bizarre that the payload is a mix of big endian and little endian values :\

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. But that's what some web3/solidity folks asked for in the EIP and got.

@shemnon shemnon merged commit 5137155 into PegaSysEng:master Aug 19, 2019
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.

3 participants