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

Integrating tromp solvers into miner #1570

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@sarath-hotspot

sarath-hotspot commented Oct 19, 2016

Integrated tromp equihash algorithm for mining.
Original solvers are available in https://github.com/tromp/equihash

Forum thread: https://forum.z.cash/t/tromps-solvers/2465/197

Able to generate multiple blocks on testnet.
https://explorer.testnet.z.cash/block/000a8464db4d2e6ab597acdda0237641ac0baa4e4cd629a02fa47b06fa31a2079

By default this solver is disabled. Need to add macro declaration to enable this solver.

How to enable tromp equihash solver:
In miner.cpp uncomment following line(35th line).
// #define USE_TROMP_EQUIHASH

Rebuild and run your zcshd.

Performance:
Single thread, on 4core, 8GB RAM machine, getting 1 Sol/s.

Integrating tromp solvers into miner
Able to generate multiple blocks on testnet.
https://explorer.testnet.z.cash/block/000a8464db4d2e6ab597acdda0237641ac0baa4e4cd629a02fa47b06fa31a2079

Integrating tromp's memory optimizations
Adding macro control to enable tromp solver

Original solvers are available in https://github.com/tromp/equihash

How to enable tromp equihash solver:
In miner.cpp uncomment following line(35th line).
// #define USE_TROMP_EQUIHASH

Rebuild and run your zcshd.

Performance:
Single thread, on 4core, 8GB RAM machine,  getting 1 Sol/s.
@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Oct 19, 2016

Contributor

Firstly, I've built this, it works (I mined several blocks), and it feels ridiculous (compared to my implementation). Thanks for integrating it @sarath-hotspot! 😄

I discussed this with @nathan-at-least, and we're very keen to get this merged in before launch, as well as any other solvers that are available and can be integrated. One of our desired goals is for mining to be decentralised, and having several different solvers built into zcashd is a good way to reduce the barrier to users quickly getting started with mining (without needing to get into standalone miners, pools etc).

(And yet another disclaimer: merging of this PR, or of any solvers submitted to the Miner Challenge, into zcashd does not constitute an endorsement or have any correlations with eventual judging results by myself or the other judges. All Challange submissions are welcome to have PRs considered for merging.)

The primary issue now is getting this done in time for RC 2, which looks to be the last code merge point before launch. I'm going to review this PR now, but there is definitely some work that needs to be done before we merge it. @sarath-hotspot, will you have time to work on this PR over the next 30 or so hours? Comments will need to be addressed by the end of Thursday US if we are to merge this PR on Friday. If you don't have time, that's fine - let me know, and I can do the remaining integration work myself. But as the OP, it would be fantastic to have your contributions directly merged ☺️

Contributor

str4d commented Oct 19, 2016

Firstly, I've built this, it works (I mined several blocks), and it feels ridiculous (compared to my implementation). Thanks for integrating it @sarath-hotspot! 😄

I discussed this with @nathan-at-least, and we're very keen to get this merged in before launch, as well as any other solvers that are available and can be integrated. One of our desired goals is for mining to be decentralised, and having several different solvers built into zcashd is a good way to reduce the barrier to users quickly getting started with mining (without needing to get into standalone miners, pools etc).

(And yet another disclaimer: merging of this PR, or of any solvers submitted to the Miner Challenge, into zcashd does not constitute an endorsement or have any correlations with eventual judging results by myself or the other judges. All Challange submissions are welcome to have PRs considered for merging.)

The primary issue now is getting this done in time for RC 2, which looks to be the last code merge point before launch. I'm going to review this PR now, but there is definitely some work that needs to be done before we merge it. @sarath-hotspot, will you have time to work on this PR over the next 30 or so hours? Comments will need to be addressed by the end of Thursday US if we are to merge this PR on Friday. If you don't have time, that's fine - let me know, and I can do the remaining integration work myself. But as the OP, it would be fantastic to have your contributions directly merged ☺️

@str4d str4d added this to the 1.0.0-rc2 milestone Oct 19, 2016

@sarath-hotspot

This comment has been minimized.

Show comment
Hide comment
@sarath-hotspot

sarath-hotspot Oct 19, 2016

Please let me know your comments. I could squeeze 3-4 hours from my work
schedule.
If I am done with the changes I will update the PR.

On Oct 20, 2016 1:02 AM, "str4d" notifications@github.com wrote:

Firstly, I've built this, it works (I mined several blocks), and it feels
ridiculous (compared to my implementation). Thanks for integrating it
@sarath-hotspot https://github.com/sarath-hotspot! 😄

I discussed this with @nathan-at-least
https://github.com/nathan-at-least, and we're very keen to get this
merged in before launch, as well as any other solvers that are available
and can be integrated. One of our desired goals is for mining to be
decentralised, and having several different solvers built into zcashd is
a good way to reduce the barrier to users quickly getting started with
mining (without needing to get into standalone miners, pools etc).

(And yet another disclaimer: merging of this PR, or of any solvers
submitted to the Miner Challenge, into zcashd does not constitute an
endorsement or have any correlations with eventual judging results by
myself or the other judges. All Challange submissions are welcome to have
PRs considered for merging.)

The primary issue now is getting this done in time for RC 2, which looks
to be the last code merge point before launch. I'm going to review this PR
now, but there is definitely some work that needs to be done before we
merge it. @sarath-hotspot https://github.com/sarath-hotspot, will you
have time to work on this PR over the next 30 or so hours? Comments will
need to be addressed by the end of Thursday US if we are to merge this PR
on Friday. If you don't have time, that's fine - let me know, and I can do
the remaining integration work myself. But as the OP, it would be fantastic
to have your contributions directly merged ☺️


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1570 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AH4skOZnLpGwgyFA8C08nhQS1SUYmUvkks5q1nBGgaJpZM4KagCF
.

sarath-hotspot commented Oct 19, 2016

Please let me know your comments. I could squeeze 3-4 hours from my work
schedule.
If I am done with the changes I will update the PR.

On Oct 20, 2016 1:02 AM, "str4d" notifications@github.com wrote:

Firstly, I've built this, it works (I mined several blocks), and it feels
ridiculous (compared to my implementation). Thanks for integrating it
@sarath-hotspot https://github.com/sarath-hotspot! 😄

I discussed this with @nathan-at-least
https://github.com/nathan-at-least, and we're very keen to get this
merged in before launch, as well as any other solvers that are available
and can be integrated. One of our desired goals is for mining to be
decentralised, and having several different solvers built into zcashd is
a good way to reduce the barrier to users quickly getting started with
mining (without needing to get into standalone miners, pools etc).

(And yet another disclaimer: merging of this PR, or of any solvers
submitted to the Miner Challenge, into zcashd does not constitute an
endorsement or have any correlations with eventual judging results by
myself or the other judges. All Challange submissions are welcome to have
PRs considered for merging.)

The primary issue now is getting this done in time for RC 2, which looks
to be the last code merge point before launch. I'm going to review this PR
now, but there is definitely some work that needs to be done before we
merge it. @sarath-hotspot https://github.com/sarath-hotspot, will you
have time to work on this PR over the next 30 or so hours? Comments will
need to be addressed by the end of Thursday US if we are to merge this PR
on Friday. If you don't have time, that's fine - let me know, and I can do
the remaining integration work myself. But as the OP, it would be fantastic
to have your contributions directly merged ☺️


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1570 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AH4skOZnLpGwgyFA8C08nhQS1SUYmUvkks5q1nBGgaJpZM4KagCF
.

@str4d

Good start! In addition to my specific comments, I'd like to see the commits refactored so that the first commit is a direct import of @tromp's mining code, as verbatim as possible, with the license and a link to the exact commit the code is taken from in the commit message (see this commit in my standalone miner for an example). This will make importing future updates much easier.

@@ -224,7 +224,7 @@ libbitcoin_server_a_SOURCES = \
validationinterface.cpp \
$(JSON_H) \
$(BITCOIN_CORE_H) \
$(LIBZCASH_H)
$(LIBZCASH_H)

This comment has been minimized.

@str4d

str4d Oct 19, 2016

Contributor

Remove the extra space.

@str4d

str4d Oct 19, 2016

Contributor

Remove the extra space.

@@ -243,6 +243,18 @@ libbitcoin_wallet_a_SOURCES = \
$(BITCOIN_CORE_H) \
$(LIBZCASH_H)
TROMPEQUIHASH_SOURCES = \

This comment has been minimized.

@str4d

str4d Oct 19, 2016

Contributor

I'd prefer to use src/equihash/SOLVERNAME for imported solver code, so this code would be located in src/equihash/tromp.

@str4d

str4d Oct 19, 2016

Contributor

I'd prefer to use src/equihash/SOLVERNAME for imported solver code, so this code would be located in src/equihash/tromp.

@@ -30,6 +31,10 @@
#include <boost/tuple/tuple.hpp>
#include <mutex>
#ifndef USE_TROMP_EQUIHASH
// #define USE_TROMP_EQUIHASH
#endif

This comment has been minimized.

@str4d

str4d Oct 19, 2016

Contributor

We expect that many users will just be using pre-built binaries (like our Debian packages), so instead we should use a runtime flag to determine which solver is run (see example below).

@str4d

str4d Oct 19, 2016

Contributor

We expect that many users will just be using pre-built binaries (like our Debian packages), so instead we should use a runtime flag to determine which solver is run (see example below).

@@ -559,6 +541,86 @@ void static BitcoinMiner(CWallet *pwallet)
std::lock_guard<std::mutex> lock{m_cs};
return cancelSolver;
};
#ifdef USE_TROMP_EQUIHASH

This comment has been minimized.

@str4d

str4d Oct 19, 2016

Contributor
if (solver == "tromp") {
@str4d

str4d Oct 19, 2016

Contributor
if (solver == "tromp") {
// I = the block header minus nonce and solution.
CEquihashInput I{*pblock};
CDataStream ss(SER_NETWORK, PROTOCOL_VERSION);
ss << I;

This comment has been minimized.

@str4d

str4d Oct 19, 2016

Contributor

Leave this here, since it is common code between all solvers.

@str4d

str4d Oct 19, 2016

Contributor

Leave this here, since it is common code between all solvers.

// Convert solution indices to charactar array(decompress) and pass it to validBlock method.
u32 nsols = 0;
for (unsigned s = 0; s < eq.nsols; s++) {

This comment has been minimized.

@str4d

str4d Oct 19, 2016

Contributor

size_t

@str4d

str4d Oct 19, 2016

Contributor

size_t

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);

This comment has been minimized.

@str4d

str4d Oct 19, 2016

Contributor

I'd say that this needs at least one call to check for cancellation inside the for loop, but perhaps given the speed of the solver this isn't necessary (or at least, we should check what the effect is on the speed of the solver). We can probably get away with just checking for cancellation before starting the solver.

@str4d

str4d Oct 19, 2016

Contributor

I'd say that this needs at least one call to check for cancellation inside the for loop, but perhaps given the speed of the solver this isn't necessary (or at least, we should check what the effect is on the speed of the solver). We can probably get away with just checking for cancellation before starting the solver.

This comment has been minimized.

@daira

daira Oct 19, 2016

Contributor

Note that if we move to more computationally intensive parameters, that could affect the decision of whether to check for cancellation per round or just once. For now, it's fine to just do whatever is simplest; it won't be hard to change.

@daira

daira Oct 19, 2016

Contributor

Note that if we move to more computationally intensive parameters, that could affect the decision of whether to check for cancellation per round or just once. For now, it's fine to just do whatever is simplest; it won't be hard to change.

// TROMP EQ SOLVER END
//////////////////////////////////////////////////////////////////////
#else

This comment has been minimized.

@str4d

str4d Oct 19, 2016

Contributor
} else { // Default solver
@str4d

str4d Oct 19, 2016

Contributor
} else { // Default solver
// (x_1, x_2, ...) = A(I, V, n, k)
LogPrint("pow", "Running Equihash solver with nNonce = %s\n",
pblock->nNonce.ToString());

This comment has been minimized.

@str4d

str4d Oct 19, 2016

Contributor

Remove this (replaced with earlier log line).

@str4d

str4d Oct 19, 2016

Contributor

Remove this (replaced with earlier log line).

@@ -568,6 +630,7 @@ void static BitcoinMiner(CWallet *pwallet)
std::lock_guard<std::mutex> lock{m_cs};
cancelSolver = false;
}
#endif

This comment has been minimized.

@str4d

str4d Oct 19, 2016

Contributor
}
@str4d

str4d Oct 19, 2016

Contributor
}
@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Oct 19, 2016

Contributor

Note that I haven't reviewed the solver code itself. For launch, we would leave the default solver as the current well-reviewed code, but users could set a runtime flag to choose a different maybe-not-as-well-reviewed solver.

Contributor

str4d commented Oct 19, 2016

Note that I haven't reviewed the solver code itself. For launch, we would leave the default solver as the current well-reviewed code, but users could set a runtime flag to choose a different maybe-not-as-well-reviewed solver.

@daira

This comment has been minimized.

Show comment
Hide comment
@daira

daira Oct 19, 2016

Contributor

Concept ACK; starting review now.

Contributor

daira commented Oct 19, 2016

Concept ACK; starting review now.

@@ -0,0 +1,72 @@
/*

This comment has been minimized.

@daira

daira Oct 19, 2016

Contributor

Given the changes @str4d described, I think there is no longer any need to include a BLAKE2b implementation separate from the one we use from libsodium, so strip out all of that code.

@daira

daira Oct 19, 2016

Contributor

Given the changes @str4d described, I think there is no longer any need to include a BLAKE2b implementation separate from the one we use from libsodium, so strip out all of that code.

This comment has been minimized.

@tromp

tromp Oct 19, 2016

The libsodium blake2b_update calls blake2b_compress only after it has at least 256 input bytes, preventing the extraction of a midstate.
My own copy of blake is modified to call blake2b_compress eagerly and reduces the buffer to 128 bytes.

@tromp

tromp Oct 19, 2016

The libsodium blake2b_update calls blake2b_compress only after it has at least 256 input bytes, preventing the extraction of a midstate.
My own copy of blake is modified to call blake2b_compress eagerly and reduces the buffer to 128 bytes.

This comment has been minimized.

@daira

daira Oct 19, 2016

Contributor

I'm pretty negative about maintaining another implementation of BLAKE2. Including multiple implementations of the same cryptographic primitive is a slippery slope that we've worked hard to avoid so far, and I think it will pay off in terms of auditability, responding to security bugs, including optimizations faster, etc.

Looking at the implementation of blake2b_update in libsodium, it seems quite feasible to implement the optimization you want in terms of blake2b_increment_counter and blake2b_compress. In fact I'll do that now.

@daira

daira Oct 19, 2016

Contributor

I'm pretty negative about maintaining another implementation of BLAKE2. Including multiple implementations of the same cryptographic primitive is a slippery slope that we've worked hard to avoid so far, and I think it will pay off in terms of auditability, responding to security bugs, including optimizations faster, etc.

Looking at the implementation of blake2b_update in libsodium, it seems quite feasible to implement the optimization you want in terms of blake2b_increment_counter and blake2b_compress. In fact I'll do that now.

This comment has been minimized.

@daira

daira Oct 22, 2016

Contributor

I didn't end up having time to include that optimization, but it should be relatively easy to add later.

@daira

daira Oct 22, 2016

Contributor

I didn't end up having time to include that optimization, but it should be relatively easy to add later.

#include "osx_barrier.h"
#include <machine/endian.h>
#include <libkern/OSByteOrder.h>
#define htole32(x) OSSwapHostToLittleInt32(x)

This comment has been minimized.

@daira

daira Oct 19, 2016

Contributor

We already define this in compat/endian.h. Include BLAKE2b functions from libsodium if needed.

@daira

daira Oct 19, 2016

Contributor

We already define this in compat/endian.h. Include BLAKE2b functions from libsodium if needed.

typedef u32 proof[PROOFSIZE];
void setheader(blake2b_state *ctx, const char *header, const u32 headerLen, const char* nce, const u32 nonceLen) {

This comment has been minimized.

@daira

daira Oct 19, 2016

Contributor

If possible share this code with the existing solver.

@daira

daira Oct 19, 2016

Contributor

If possible share this code with the existing solver.

typedef uint16_t u16;
typedef uint64_t u64;
#ifdef ATOMIC

This comment has been minimized.

@daira

daira Oct 19, 2016

Contributor

Prefix names of preprocessor variables specific to this implementation with EQUIHASH_TROMP_.

@daira

daira Oct 19, 2016

Contributor

Prefix names of preprocessor variables specific to this implementation with EQUIHASH_TROMP_.

@daira

This comment has been minimized.

Show comment
Hide comment
@daira

daira Oct 19, 2016

Contributor

Note re: @str4d's comment #1570 (review) : if the BLAKE2 code isn't present after refactoring then you don't need to add it in the first commit.

Contributor

daira commented Oct 19, 2016

Note re: @str4d's comment #1570 (review) : if the BLAKE2 code isn't present after refactoring then you don't need to add it in the first commit.

slot0 &xs = htl.hta.trees0[r/2][xorbucketid][xorslot];
xs.attr = xort;
for (u32 i=htl.dunits; i < htl.prevhashunits; i++)
xs.hash[i-htl.dunits].word = pslot0->hash[i].word ^ pslot1->hash[i].word;

This comment has been minimized.

@daira

daira Oct 19, 2016

Contributor

Braces around single-line loop/if bodies please. (If the body is short enough to put on the same line as the loop we can make an exception, but here it isn't.)

@daira

daira Oct 19, 2016

Contributor

Braces around single-line loop/if bodies please. (If the body is short enough to put on the same line as the loop we can make an exception, but here it isn't.)

@daira

This comment has been minimized.

Show comment
Hide comment
@daira

daira Oct 19, 2016

Contributor

This looks excellent! I'll work on removing the BLAKE2 implementation now.

Contributor

daira commented Oct 19, 2016

This looks excellent! I'll work on removing the BLAKE2 implementation now.

@daira daira self-assigned this Oct 20, 2016

u32 nsols = 0;
for (unsigned s = 0; s < eq.nsols; s++) {
nsols++;
LogPrintf("Checking solution %d \n", nsols);

This comment has been minimized.

@daira

daira Oct 20, 2016

Contributor

LogPrint("pow", "Checking solution %d\n", nsols);

@daira

daira Oct 20, 2016

Contributor

LogPrint("pow", "Checking solution %d\n", nsols);

@sarath-hotspot

This comment has been minimized.

Show comment
Hide comment
@sarath-hotspot

sarath-hotspot Oct 20, 2016

@daira I see you already submitted another PR with updated changes. I will skip making these changes.
#1578

Thanks for taking care of changes.

Are you planning to modify libsodium source code?

sarath-hotspot commented Oct 20, 2016

@daira I see you already submitted another PR with updated changes. I will skip making these changes.
#1578

Thanks for taking care of changes.

Are you planning to modify libsodium source code?

@tromp

This comment has been minimized.

Show comment
Hide comment
@tromp

tromp Oct 20, 2016

@daira I see you already submitted another PR with updated changes. I will skip making these changes.
#1578

Thanks for taking care of changes.

Are you planning to modify libsodium source code?

The impact of the blake change may be less than that of the speed
improvements I made after you snapshotted my code. Please consider
grabbing the latest code. Then again, I expect to have more speedups
today...

-John

tromp commented Oct 20, 2016

@daira I see you already submitted another PR with updated changes. I will skip making these changes.
#1578

Thanks for taking care of changes.

Are you planning to modify libsodium source code?

The impact of the blake change may be less than that of the speed
improvements I made after you snapshotted my code. Please consider
grabbing the latest code. Then again, I expect to have more speedups
today...

-John

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Oct 20, 2016

Contributor

@tromp #1578 uses the latest code from your repo.

Contributor

str4d commented Oct 20, 2016

@tromp #1578 uses the latest code from your repo.

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Oct 20, 2016

Contributor

Superceded by #1578.

Contributor

str4d commented Oct 20, 2016

Superceded by #1578.

@str4d str4d closed this Oct 20, 2016

zkbot pushed a commit that referenced this pull request Oct 21, 2016

zkbot
Auto merge of #1578 - daira:1570.tromp-equihash-with-libsodium-blake.…
…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 pushed a commit that referenced this pull request Oct 21, 2016

zkbot
Auto merge of #1578 - daira:1570.tromp-equihash-with-libsodium-blake.…
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment