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

Implement new hash_memory proc #1519

Merged

Conversation

Fumuran
Copy link
Contributor

@Fumuran Fumuran commented Sep 30, 2024

This is the same PR as #1369 needed to re-add this procedure to the al-migrate-new-padding-rule branch.

Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I didn't review the logic of the function in detail as I'm assuming it is the same as in the previous PR. But I did leave a couple of small comments inline.

CHANGELOG.md Outdated Show resolved Hide resolved
stdlib/asm/crypto/hashes/rpo.masm Outdated Show resolved Hide resolved
stdlib/tests/crypto/rpo.rs Show resolved Hide resolved
@Fumuran Fumuran requested a review from bobbinth October 1, 2024 10:41
Copy link
Contributor

@bobbinth bobbinth 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 left a couple more comments inline. Also, it may be good to add a hash_memory_double_words procedure - but probably in a different PR.

stdlib/tests/crypto/rpo.rs Show resolved Hide resolved
stdlib/asm/crypto/hashes/rpo.masm Outdated Show resolved Hide resolved
Copy link
Collaborator

@Al-Kindi-0 Al-Kindi-0 left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you!

Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left one question inline - but it could also be addressed separately.

end
";

build_test!(source, &[]).expect_stack(&[0; 4]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not test against expected stack [0; 16]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My initial thought was to check the actual value which hash_memory_words produces, but we can check the entire stack, the result should be the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's check the entire stack - this way we'll be sure that the procedure doesn't "mess" with the rest of the stack.

@bobbinth bobbinth merged commit 8316863 into al-migrate-new-padding-rule Oct 15, 2024
9 checks passed
@bobbinth bobbinth deleted the andrew-hash-arbitrary-num-of-values branch October 15, 2024 07:56
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.

3 participants