Fix crash when mining with empty keypool. #6567

Merged
merged 1 commit into from Aug 19, 2015

Conversation

Projects
None yet
3 participants
@domob1812
Contributor

domob1812 commented Aug 18, 2015

Since the introduction of the ScriptForMining callback, the mining functions (setgenerate and generate) crash with an assertion failure (due to a NULL pointer script returned) if the keypool is empty. Fix this by giving a proper error.

Fix crash when mining with empty keypool.
Since the introduction of the ScriptForMining callback, the mining
functions (setgenerate and generate) crash with an assertion failure
(due to a NULL pointer script returned) if the keypool is empty.  Fix
this by giving a proper error.

@domob1812 domob1812 referenced this pull request in namecoin/namecoin-core Aug 18, 2015

Closed

Crash bug reported by Eligius #25

@@ -138,8 +138,12 @@ UniValue generate(const UniValue& params, bool fHelp)
boost::shared_ptr<CReserveScript> coinbaseScript;
GetMainSignals().ScriptForMining(coinbaseScript);
+ // If the keypool is exhausted, no script is returned at all. Catch this.
+ if (!coinbaseScript)
+ throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, "Error: Keypool ran out, please call keypoolrefill first");

This comment has been minimized.

@jonasschnelli

jonasschnelli Aug 18, 2015

Member

This could also be triggered if the connected signal listener does not provide a script for other reasons than a non-filled keypool. But yes, right now only a empty keypool would emit this exception.

@jonasschnelli

jonasschnelli Aug 18, 2015

Member

This could also be triggered if the connected signal listener does not provide a script for other reasons than a non-filled keypool. But yes, right now only a empty keypool would emit this exception.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Aug 18, 2015

Member

utACK.
In terms of modularity, the miner should not know the concept of "keypools" and therefore throw an generic exception with a possible error string passed over from the signal listener to the signal emitting part.

Member

jonasschnelli commented Aug 18, 2015

utACK.
In terms of modularity, the miner should not know the concept of "keypools" and therefore throw an generic exception with a possible error string passed over from the signal listener to the signal emitting part.

@domob1812

This comment has been minimized.

Show comment
Hide comment
@domob1812

domob1812 Aug 18, 2015

Contributor

I agree in principle (that's why the message/error is keypool specific only in the RPC code and not miner.cpp). However, the keypool is explicitly a "concept" part of the RPC interface (with the specific error code), thus I think it should be handled there.

Of course, if in the future more conditions can cause a missing script to be returned, one also needs to add a method to signal the error to the caller with more details.

Contributor

domob1812 commented Aug 18, 2015

I agree in principle (that's why the message/error is keypool specific only in the RPC code and not miner.cpp). However, the keypool is explicitly a "concept" part of the RPC interface (with the specific error code), thus I think it should be handled there.

Of course, if in the future more conditions can cause a missing script to be returned, one also needs to add a method to signal the error to the caller with more details.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Aug 19, 2015

Member

This has been broken and fixed multiple times. Thanks a lot for adding a test.
ACK

Member

laanwj commented Aug 19, 2015

This has been broken and fixed multiple times. Thanks a lot for adding a test.
ACK

@laanwj laanwj merged commit 2016576 into bitcoin:master Aug 19, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Aug 19, 2015

Merge pull request #6567
2016576 Fix crash when mining with empty keypool. (Daniel Kraft)

@domob1812 domob1812 deleted the domob1812:keypool-crash branch Aug 19, 2015

domob1812 added a commit to domob1812/namecore that referenced this pull request Aug 20, 2015

Fix crash with empty keypool for getauxblock.
Fix namecoin#25.  This implements the
upstream fix done in Bitcoin's bitcoin/bitcoin#6567
also for getauxblock.

@str4d str4d referenced this pull request in zcash/zcash Mar 29, 2017

Open

Bitcoin 0.12 wallet PRs 1 #2225

@dagurval dagurval referenced this pull request in bitcoinxt/bitcoinxt Sep 2, 2017

Merged

keypool test bump + detach wallet from miner #258

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment