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

1570.tromp equihash with libsodium blake.2 #1578

Merged
merged 4 commits into from Oct 21, 2016

Conversation

@daira
Copy link
Contributor

@daira daira commented Oct 20, 2016

Remove BLAKE2b implementation from Tromp Equihash solver, and address almost all of @str4d's review comments on #1570. Supercedes #1576.

Signed-off-by: Daira Hopwood daira@jacaranda.org

daira added 2 commits Oct 20, 2016
(as of tromp/equihash commit 690fc5eff453bc0c1ec66b283395c9df87701e93).

Author: John Tromp <john.tromp@gmail.com>
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
…xtra BLAKE2b implementation.

Signed-off-by: Daira Hopwood <daira@jacaranda.org>
@daira daira force-pushed the 1570.tromp-equihash-with-libsodium-blake.2 branch from 422ccc4 to c7aaab7 Oct 20, 2016
str4d
str4d approved these changes Oct 20, 2016
Copy link
Contributor

@str4d str4d left a comment

ACK modulo my logging comment.

LogPrint("pow", "Running Equihash solver with nNonce = %s\n",
pblock->nNonce.ToString());
LogPrint("pow", "Running Equihash solver \"%s\" with nNonce = %s\n",
solver, pblock->nNonce.ToString());
Copy link
Contributor

@str4d str4d Oct 20, 2016

Hmm, with the extra log line above, we probably actually don't need this change here. Does not block.


if (solver == "tromp") {
// Create solver and initialize it.
equi eq(1);
Copy link
Contributor

@str4d str4d Oct 20, 2016

Currently -genproclimit is set up such that it will start multiple independent solvers. Now that there's a solver impl that supports internal threading, we should think about how to leverage that in a user-friendly way. Does not block.

Copy link
Contributor Author

@daira daira Oct 20, 2016

Ack, but I'd like to merge this for rc2 and improve it later.

// Intialization done, start algo driver.
eq.digit0(0);
eq.xfull = eq.bfull = eq.hfull = 0;
eq.showbsizes(0);
Copy link
Contributor

@str4d str4d Oct 20, 2016

This needs to be somehow wrapped in a LogPrint, either inside "pow" or somewhere else. I think the best way to do that would be to modify showbsizes() to return a string, and then print that.

for (u32 r = 1; r < WK; r++) {
(r&1) ? eq.digitodd(r, 0) : eq.digiteven(r, 0);
eq.xfull = eq.bfull = eq.hfull = 0;
eq.showbsizes(r);
Copy link
Contributor

@str4d str4d Oct 20, 2016

Ditto.

@str4d
Copy link
Contributor

@str4d str4d commented Oct 20, 2016

I'm not blocking this PR on inclusion of the BLAKE2b changes.

@str4d str4d added this to the 1.0.0-rc2 milestone Oct 20, 2016
@str4d
Copy link
Contributor

@str4d str4d commented Oct 20, 2016

@daira in addition to the log changes above, maybe you could also pull in the upstream changes that were landed this morning?

@str4d
Copy link
Contributor

@str4d str4d commented Oct 20, 2016

Also, the new config flag should be documented in the zcashd help text (in src/init.cpp).

daira added 2 commits Oct 20, 2016
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
@str4d str4d mentioned this pull request Oct 20, 2016
@daira
Copy link
Contributor Author

@daira daira commented Oct 21, 2016

John Tromp said on Slack:

that looks recent enough; i’ve not made any significant gains since

@daira
Copy link
Contributor Author

@daira daira commented Oct 21, 2016

I think we should just land this as-is and clean up the logging later.

@str4d
Copy link
Contributor

@str4d str4d commented Oct 21, 2016

Agree that we won't block on my logging comment; we can fix that in RC3 if we find it annoying.

utACK

@daira
Copy link
Contributor Author

@daira daira commented Oct 21, 2016

@zkbot r+

@zkbot
Copy link
Collaborator

@zkbot zkbot commented Oct 21, 2016

📌 Commit 5f0009b has been approved by daira

@zkbot
Copy link
Collaborator

@zkbot zkbot commented Oct 21, 2016

Testing commit 5f0009b with merge a5aa50d...

zkbot pushed a commit that referenced this issue Oct 21, 2016
…2, r=daira

1570.tromp equihash with libsodium blake.2

Remove BLAKE2b implementation from Tromp Equihash solver, and address almost all of @str4d's review comments on #1570. Supercedes #1576.

Signed-off-by: Daira Hopwood daira@jacaranda.org
@zkbot
Copy link
Collaborator

@zkbot zkbot commented Oct 21, 2016

💔 Test failed - zcash

@str4d
Copy link
Contributor

@str4d str4d commented Oct 21, 2016

Expected failure, #1598 hadn't been merged yet.

@daira
Copy link
Contributor Author

@daira daira commented Oct 21, 2016

@zkbot retry

zkbot pushed a commit that referenced this issue Oct 21, 2016
…2, r=daira

1570.tromp equihash with libsodium blake.2

Remove BLAKE2b implementation from Tromp Equihash solver, and address almost all of @str4d's review comments on #1570. Supercedes #1576.

Signed-off-by: Daira Hopwood daira@jacaranda.org
@zkbot
Copy link
Collaborator

@zkbot zkbot commented Oct 21, 2016

Testing commit 5f0009b with merge dc21a8a...

@zkbot
Copy link
Collaborator

@zkbot zkbot commented Oct 21, 2016

☀️ Test successful - zcash

@zkbot zkbot merged commit 5f0009b into zcash:master Oct 21, 2016
1 check passed
@zkbot zkbot mentioned this pull request Oct 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants