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

Avoid SIGSEGV in tromp solver #556

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/Makefile.ktest.include
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ bin_PROGRAMS += komodo-test
test-komodo/test_netbase_tests.cpp \
test-komodo/test_events.cpp \
test-komodo/test_hex.cpp \
test-komodo/test_haraka_removal.cpp
test-komodo/test_haraka_removal.cpp \
test-komodo/test_miner.cpp

komodo_test_CPPFLAGS = $(komodod_CPPFLAGS)

Expand Down
92 changes: 77 additions & 15 deletions src/miner.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1193,6 +1193,27 @@ CBlockIndex *get_chainactive(int32_t height)
int32_t gotinvalid;
extern int32_t getkmdseason(int32_t height);

bool check_tromp_solution(equi &eq, std::function<bool(std::vector<unsigned char>)> validBlock)
{
// Convert solution indices to byte array (decompress) and pass it to validBlock method.
for (size_t s = 0; s < std::min(MAXSOLS, eq.nsols); s++)
{
LogPrint("pow", "Checking solution %d\n", s+1);
std::vector<eh_index> index_vector(PROOFSIZE);
for (size_t i = 0; i < PROOFSIZE; i++) {
index_vector[i] = eq.sols[s][i];
}
std::vector<unsigned char> sol_char = GetMinimalFromIndices(index_vector, DIGITBITS);

if (validBlock(sol_char)) {
// If we find a POW solution, do not try other solutions
// because they become invalid as we created a new block in blockchain.
return true;
}
}
return false;
}

#ifdef ENABLE_WALLET
void static BitcoinMiner(CWallet *pwallet)
#else
Expand Down Expand Up @@ -1616,21 +1637,8 @@ void static BitcoinMiner()
eq.digitK(0);
ehSolverRuns.increment();

// Convert solution indices to byte array (decompress) and pass it to validBlock method.
for (size_t s = 0; s < eq.nsols; s++) {
LogPrint("pow", "Checking solution %d\n", s+1);
std::vector<eh_index> index_vector(PROOFSIZE);
for (size_t i = 0; i < PROOFSIZE; i++) {
index_vector[i] = eq.sols[s][i];
}
std::vector<unsigned char> sol_char = GetMinimalFromIndices(index_vector, DIGITBITS);

if (validBlock(sol_char)) {
// If we find a POW solution, do not try other solutions
// because they become invalid as we created a new block in blockchain.
break;
}
}
check_tromp_solution(eq, validBlock);
Copy link

Choose a reason for hiding this comment

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

May be here we need:

if (check_tromp_solution(eq, validBlock))
    break;

To preserve the original logic? If we find a POW solution, do not try other solutions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As I can tell, check_tromp_solution does not change the original logic:
the original logic breaks the loop if a solution is found
and check_tromp_solution in this case just returns also breaking solution searching.
It looks like the boolean return is needed for tests only

Copy link

@DeckerSU DeckerSU Aug 1, 2022

Choose a reason for hiding this comment

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

Yes, seems you are right, sorry. The logic is fine.

p.s. It was "small display issue" (I mixed up the scopes somehow and what it breaks).


} else {
try {
// If we find a valid block, we rebuild
Expand Down Expand Up @@ -1775,3 +1783,57 @@ void static BitcoinMiner()
}

#endif // ENABLE_MINING

/****
* This should only be called from a unit test, as the equihash code
* can only be included once (no separation of implementation from declaration).
* This verifies that std::min(MAXSOLS, eq.nsols) prevents a SIGSEGV.
*/
bool test_tromp_equihash()
{
// get the sols to be less than nsols

// create a context
CBlock block;
const CChainParams& params = Params();

crypto_generichash_blake2b_state state;
EhInitialiseState(params.EquihashN(), params.EquihashK(), state);
// I = the block header minus nonce and solution.
CEquihashInput I{block};
CDataStream ss(SER_NETWORK, PROTOCOL_VERSION);
ss << I;
// H(I||...
crypto_generichash_blake2b_update(&state, (unsigned char*)&ss[0], ss.size());
// H(I||V||...
crypto_generichash_blake2b_state curr_state;
curr_state = state;
crypto_generichash_blake2b_update(&state,block.nNonce.begin(),block.nNonce.size());

// Create solver and initialize it.
equi eq(1);
eq.setstate(&curr_state);

// Initialization done, start algo driver.
eq.digit0(0);
eq.xfull = eq.bfull = eq.hfull = 0;
eq.showbsizes(0);
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);
}
eq.digitK(0);

// force nsols to be more than MAXSOLS (8)
while (eq.nsols <= MAXSOLS * 800)
{
tree t(1);
eq.candidate(t);
}

auto func = [](std::vector<unsigned char>) { return false; };

// this used to throw a SIGSEGV
return check_tromp_solution(eq, func);
}
8 changes: 8 additions & 0 deletions src/test-komodo/test_miner.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
#include <gtest/gtest.h>

bool test_tromp_equihash();

TEST(test_miner, check)
{
EXPECT_FALSE(test_tromp_equihash());
}