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

Remove the assumption that n/(k+1) is a multiple of 8 #1172

Conversation

str4d
Copy link
Contributor

@str4d str4d commented Aug 5, 2016

This version works, but generates the initial rows in a way that is probably
not what we want to specify.

Closes #1148

@str4d str4d added A-pow Area: Proof-of-Work and mining S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 5, 2016
@str4d
Copy link
Contributor Author

str4d commented Aug 5, 2016

When we are happy with this, I will open a new issue for fixing the implementation to match what we want to specify.


if (8*cByteLen == cBitLen) {
// We are colliding an integer number of bytes; hLen == N/8
crypto_generichash_blake2b_final(&state, hash, hLen);
Copy link
Contributor

Choose a reason for hiding this comment

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

Keeping the hash call outside the if would probably result in easier-to-follow code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that even when we change the expansion method, the same expression should work for the hash output length in both cases.

@daira
Copy link
Contributor

daira commented Aug 5, 2016

ACK.

The first commit has me as committer and Signed-off-by even though it's not identical to the code I wrote. In such cases can you recommit as you, and add "Author: Daira Hopwood" in the commit message, or some other way that reflects joint authorship, please? (There's no need to do this when fixing trivial conflicts in rebases, it's just that there are nontrivial changes here.)

This version works, but generates the initial rows in a way that is not what we
want to specify. See zcash#1175 for resolving this.

Co-author: Daira Hopwood <daira@jacaranda.org>
@str4d str4d force-pushed the 1148-remove-equihash-collision-length-assumption branch from eebbce5 to 036dcbd Compare August 5, 2016 14:58
@str4d
Copy link
Contributor Author

str4d commented Aug 5, 2016

Rebased on .latest and fixed the commit message.

@daira
Copy link
Contributor

daira commented Aug 5, 2016

ACK

@bitcartel
Copy link
Contributor

ACK

@zkbot r+

@zkbot
Copy link
Contributor

zkbot commented Aug 5, 2016

📌 Commit 036dcbd has been approved by bitcartel

@zkbot
Copy link
Contributor

zkbot commented Aug 5, 2016

⌛ Testing commit 036dcbd with merge 7647097...

zkbot pushed a commit that referenced this pull request Aug 5, 2016
…umption, r=bitcartel

Remove the assumption that n/(k+1) is a multiple of 8

This version works, but generates the initial rows in a way that is probably
not what we want to specify.

Closes #1148
@zkbot
Copy link
Contributor

zkbot commented Aug 5, 2016

☀️ Test successful - zcash

@zkbot zkbot merged commit 036dcbd into zcash:zc.v0.11.2.latest Aug 5, 2016
@str4d str4d deleted the 1148-remove-equihash-collision-length-assumption branch August 5, 2016 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-pow Area: Proof-of-Work and mining S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants