Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Blake2B (precomp #9) tests, including with lengths up to 1024 #948

Merged
merged 2 commits into from
Sep 26, 2021

Conversation

qbzzt
Copy link
Collaborator

@qbzzt qbzzt commented Sep 23, 2021

No description provided.

@qbzzt qbzzt added the ready-to-merge PR is ready to merge and not WIP label Sep 23, 2021
@holgerd77
Copy link
Contributor

For reference: see also ethereumjs/ethereumjs-monorepo#1486

@jochem-brouwer
Copy link
Member

Hiya, just checking here - the bug we found in JS was when the message field input to Blake2B was were 5 or more of the first bytes were nonzero. The test cases in the EIP use the string abc (so the first 3 bytes are nonzero, the rest are), which are probably taken from the RFC. If we input the string abcde then we had a consensus bug due to wrongly converting the input. If these test cases have any input string where the m (message) field of the Blake2B input is 5 nonzero bytes or more, then this should cover our case.

@jochem-brouwer
Copy link
Member

The test case which should be added to the EIP to cover our case was;

  {
    input: '0000000c48c9bdf267e6096a3ba7ca8485ae67bb2bf894fe72f36e3cf1361d5f3af54fa5d182e6ad7f520e511f6c3e2b8c68059b6bbd41fbabd9831f79217e1319cde05b61626364650000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000500000000000000000000000000000001',
    expected: 'f3e89a60ec4b0b1854744984e421d22b82f181bd4601fb9b1726b2662da61c29dff09e75814acb2639fd79e56616e55fc135f8476f0302b3dc8d44e082eb83a8',
    name: 'vector 9'
  }

@qbzzt qbzzt removed the ready-to-merge PR is ready to merge and not WIP label Sep 23, 2021
@qbzzt
Copy link
Collaborator Author

qbzzt commented Sep 23, 2021

Hiya, just checking here - the bug we found in JS was when the message field input to Blake2B was were 5 or more of the first bytes were nonzero. The test cases in the EIP use the string abc (so the first 3 bytes are nonzero, the rest are), which are probably taken from the RFC. If we input the string abcde then we had a consensus bug due to wrongly converting the input. If these test cases have any input string where the m (message) field of the Blake2B input is 5 nonzero bytes or more, then this should cover our case.

Thank you, sorry I misunderstood. I'll add more test cases with different message lengths.

1. The case provided by jochem-brouwer
2. Cases with message length of 16 bytes, lots of rounds
3. Cases with message length of 120 bytes, a few options for rounds
@qbzzt qbzzt added the ready-to-merge PR is ready to merge and not WIP label Sep 24, 2021
@winsvega winsvega merged commit cfc80bd into develop Sep 26, 2021
@winsvega winsvega deleted the blake2b branch September 26, 2021 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge PR is ready to merge and not WIP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants