Fix crash when mining with empty keypool. #6567

pull domob1812 wants to merge 1 commits into bitcoin:master from domob1812:keypool-crash changing 3 files +24 −3

domob1812 commented at 7:10 AM on August 18, 2015: contributor

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.

2016576998

in src/rpcmining.cpp:None in 2016576998

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

jonasschnelli commented at 7:14 AM on August 18, 2015:

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 commented at 7:17 AM on August 18, 2015: contributor

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 commented at 7:22 AM on August 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.
laanwj commented at 1:01 PM on August 19, 2015: member

This has been broken and fixed multiple times. Thanks a lot for adding a test. ACK
laanwj merged this on Aug 19, 2015
laanwj closed this on Aug 19, 2015
laanwj referenced this in commit 0f0f323c9a on Aug 19, 2015
domob1812 deleted the branch on Aug 19, 2015
domob1812 referenced this in commit eb6be23fb4 on Aug 20, 2015
zkbot referenced this in commit 025bd44543 on Nov 21, 2020
zkbot referenced this in commit 7a0a268054 on Dec 2, 2020
zkbot referenced this in commit c8896f9907 on Dec 2, 2020
DrahtBot locked this on Sep 8, 2021

Contributors

domob1812

jonasschnelli

laanwj