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

Tests: Add blake2b Test Cases #8901

Merged
merged 2 commits into from Jul 7, 2023
Merged

Conversation

0xFFFC0000
Copy link
Collaborator

	* New files for blake2b test and testvector.
	* Adding the test to CMakeLists.txt
	* Adding brief documentation for the test.

Link to bounty:
https://bounties.monero.social/posts/75/6-500m-blake2b-c-dev-challenge-seraphis

Copy link
Collaborator

@selsta selsta left a comment

Choose a reason for hiding this comment

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

Thank you.

I think this should be added to the hash tests, no need for a whole new binary.

Also please add trailing new lines so that GitHub doesn't complain and change the commit title to something like "tests: add blake2b test cases" to be more inline with other commits.

@0xFFFC0000
Copy link
Collaborator Author

Thanks.

Actually I considered adding it into the hash test, but the reason why I went with the new binary is that bounty specifically asks for a new blake2b_test.cpp file.

@0xFFFC0000 0xFFFC0000 force-pushed the master branch 4 times, most recently from b6f9299 to e09c8a8 Compare June 9, 2023 21:20
@0xFFFC0000 0xFFFC0000 changed the title BLAKE2b C++ Dev challenge implementation. Tests: Add blake2b Test Cases Jun 9, 2023
@0xFFFC0000
Copy link
Collaborator Author

Commit message and No trailing newline (editor issue) fixed.

@UkoeHB
Copy link
Contributor

UkoeHB commented Jun 9, 2023

tests/hash works too

@0xFFFC0000
Copy link
Collaborator Author

I changed the commit and added the test logic to tests/main.cpp.

Copy link
Contributor

@UkoeHB UkoeHB left a comment

Choose a reason for hiding this comment

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

Looking closer at the distinction between tests/crypto and tests/hash, I agree with @selsta that tests/hash would be more appropriate.

tests/crypto/main.cpp Outdated Show resolved Hide resolved
tests/crypto/main.cpp Outdated Show resolved Hide resolved
tests/crypto/main.cpp Outdated Show resolved Hide resolved
@0xFFFC0000 0xFFFC0000 force-pushed the master branch 2 times, most recently from 0d91474 to d94939b Compare June 10, 2023 08:28
@0xFFFC0000
Copy link
Collaborator Author

Looking closer at the distinction between tests/crypto and tests/hash, I agree with @selsta that tests/hash would be more appropriate.

Will change the PR to tests/hash.

@0xFFFC0000
Copy link
Collaborator Author

0xFFFC0000 commented Jun 10, 2023

@selsta @UkoeHB thanks for your feedback.

Changed the test location to tests/hash folder and integrated it into the existing test suite.

ctest -R hash-blake2b

Copy link
Collaborator

@selsta selsta left a comment

Choose a reason for hiding this comment

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

A couple small comments.

tests/README.md Outdated Show resolved Hide resolved
tests/hash/main.cpp Outdated Show resolved Hide resolved
tests/hash/main.cpp Outdated Show resolved Hide resolved
Comment on lines 81 to 85
static void hash_blake2b(const void *data, size_t length, char *key_out){
// key_out is two buffer, first portion is key, second portion is out
return (void) blake2b(key_out + BLAKE2B_KEYBYTES, BLAKE2B_OUTBYTES, data, length, key_out, BLAKE2B_KEYBYTES);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
static void hash_blake2b(const void *data, size_t length, char *key_out){
// key_out is two buffer, first portion is key, second portion is out
return (void) blake2b(key_out + BLAKE2B_KEYBYTES, BLAKE2B_OUTBYTES, data, length, key_out, BLAKE2B_KEYBYTES);
}
static void hash_blake2b(const void *data, size_t length, char *hash_out){
// data = key || hash data
return (void) blake2b(hash_out, BLAKE2B_OUTBYTES, data + BLAKE2B_KEYBYTES, length, data, BLAKE2B_KEYBYTES);
}

The key_out is an out-param, it should not contain input data.

tests/hash/main.cpp Outdated Show resolved Hide resolved
tests/hash/main.cpp Outdated Show resolved Hide resolved
tests/hash/main.cpp Outdated Show resolved Hide resolved
tests/hash/main.cpp Outdated Show resolved Hide resolved
tests/hash/main.cpp Outdated Show resolved Hide resolved
tests/hash/main.cpp Outdated Show resolved Hide resolved
tests/hash/main.cpp Outdated Show resolved Hide resolved
tests/hash/main.cpp Outdated Show resolved Hide resolved
tests/hash/main.cpp Outdated Show resolved Hide resolved
tests/hash/main.cpp Outdated Show resolved Hide resolved
	* Adding blake2b test function to src/tests/hash
	* New files for testvector.
	* Adding the test to CMakeLists.txt
	* Adding brief documentation for the test.
@UkoeHB
Copy link
Contributor

UkoeHB commented Jun 12, 2023

LGTM, I need to check the test vector data then will approve.

tests/hash/main.cpp Outdated Show resolved Hide resolved
@UkoeHB
Copy link
Contributor

UkoeHB commented Jun 12, 2023

The test data is identical to the expected vectors.

Removing preprocessor macro and replacing it with constexpr.

Co-authored-by: Jeffro <jeffreyryan@tutanota.com>
@0xFFFC0000
Copy link
Collaborator Author

@UkoeHB @jeffro256 @selsta Thanks everyone for reviewing this. This is my first PR to the Monero repository and I am planning to send more PR regularly.

After @UkoeHB approval, I think at this point I am waiting for final approval.

@jeffro256
Copy link
Contributor

The test data is identical to the expected vectors.

Yup, it's the same file

Copy link
Contributor

@UkoeHB UkoeHB left a comment

Choose a reason for hiding this comment

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

I confirmed locally that the test vectors are being tested properly.

@0xFFFC0000
Copy link
Collaborator Author

[ RUN ] select_outputs.density /home/runner/work/monero/monero/tests/unit_tests/output_selection.cpp:175: Failure Expected: (fabsf(selected_ratio - chain_ratio)) < (0.025f), actual: 0.0289887 vs 0.025 [ FAILED ] select_outputs.density (3142 ms)

  1. Is this related to my PR?
  2. Does this happen in other cases too?

@UkoeHB
Copy link
Contributor

UkoeHB commented Jun 13, 2023

@0xFFFC0000 that is a statistical test that fails periodically, not related to this PR.

@0xFFFC0000
Copy link
Collaborator Author

@UkoeHB thanks.

@luigi1111 luigi1111 merged commit f44820b into monero-project:master Jul 7, 2023
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants