From 95d1f887ca66feb1d573affc761d1fa957ac88e9 Mon Sep 17 00:00:00 2001
From: Daniel Kraft <d@domob.eu>
Date: Tue, 18 Aug 2015 09:07:33 +0200
Subject: [PATCH] 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.

Zcash: Adapted to our MinerAddress type.

Co-authored-by: Jack Grigg <jack@electriccoin.co>
---
 qa/rpc-tests/keypool.py | 17 ++++++++++++++++-
 src/miner.cpp           |  6 +-----
 src/miner.h             | 19 ++++++++++++++++++-
 src/rpc/mining.cpp      | 10 ++++++++--
 src/wallet/wallet.cpp   |  6 +++++-
 5 files changed, 48 insertions(+), 10 deletions(-)

diff --git a/qa/rpc-tests/keypool.py b/qa/rpc-tests/keypool.py
index 945f29776..ac4b92a9a 100755
--- a/qa/rpc-tests/keypool.py
+++ b/qa/rpc-tests/keypool.py
@@ -8,7 +8,7 @@
 # Add python-bitcoinrpc to module search path:
 
 from test_framework.authproxy import JSONRPCException
-from test_framework.util import check_json_precision, initialize_chain, \
+from test_framework.util import assert_equal, check_json_precision, initialize_chain, \
     start_nodes, start_node, stop_nodes, wait_bitcoinds, bitcoind_processes
 
 import os
@@ -72,6 +72,21 @@ def run_test(nodes, tmpdir):
     except JSONRPCException as e:
         assert(e.error['code']==-12)
 
+    # refill keypool with three new addresses
+    nodes[0].walletpassphrase('test', 12000)
+    nodes[0].keypoolrefill(3)
+    nodes[0].walletlock()
+
+    # drain them by mining
+    nodes[0].generate(1)
+    nodes[0].generate(1)
+    nodes[0].generate(1)
+    nodes[0].generate(1)
+    try:
+        nodes[0].generate(1)
+        raise AssertionError('Keypool should be exhausted after three addesses')
+    except JSONRPCException as e:
+        assert_equal(e.error['code'], -12)
 
 def main():
     import optparse
diff --git a/src/miner.cpp b/src/miner.cpp
index 7c599ed88..b3cf0f1f0 100644
--- a/src/miner.cpp
+++ b/src/miner.cpp
@@ -116,10 +116,6 @@ void UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParams,
     }
 }
 
-bool IsValidMinerAddress(const MinerAddress& minerAddr) {
-    return minerAddr.which() != 0;
-}
-
 class AddFundingStreamValueToTx : public boost::static_visitor<bool>
 {
 private:
@@ -742,7 +738,7 @@ void static BitcoinMiner(const CChainParams& chainparams)
 
     try {
         // Throw an error if no address valid for mining was provided.
-        if (!IsValidMinerAddress(minerAddress)) {
+        if (!boost::apply_visitor(IsValidMinerAddress(), minerAddress)) {
             throw std::runtime_error("No miner address available (mining requires a wallet or -mineraddress)");
         }
 
diff --git a/src/miner.h b/src/miner.h
index 0fa2dbb33..f7852c69c 100644
--- a/src/miner.h
+++ b/src/miner.h
@@ -41,7 +41,24 @@ public:
     }
 };
 
-bool IsValidMinerAddress(const MinerAddress& minerAddr);
+class IsValidMinerAddress : public boost::static_visitor<bool>
+{
+public:
+    IsValidMinerAddress() {}
+
+    bool operator()(const InvalidMinerAddress &invalid) const {
+        return false;
+    }
+    bool operator()(const libzcash::SaplingPaymentAddress &pa) const {
+        return true;
+    }
+    bool operator()(const boost::shared_ptr<CReserveScript> &coinbaseScript) const {
+        // Return false if no script was provided.  This can happen
+        // due to some internal error but also if the keypool is empty.
+        // In the latter case, already the pointer is NULL.
+        return coinbaseScript.get() && !coinbaseScript->reserveScript.empty();
+    }
+};
 
 struct CBlockTemplate
 {
diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
index 74ebcccd6..486590110 100644
--- a/src/rpc/mining.cpp
+++ b/src/rpc/mining.cpp
@@ -188,8 +188,14 @@ UniValue generate(const UniValue& params, bool fHelp)
     MinerAddress minerAddress;
     GetMainSignals().AddressForMining(minerAddress);
 
+    // If the keypool is exhausted, no script is returned at all.  Catch this.
+    auto resv = boost::get<boost::shared_ptr<CReserveScript>>(&minerAddress);
+    if (resv && !resv->get()) {
+        throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, "Error: Keypool ran out, please call keypoolrefill first");
+    }
+
     // Throw an error if no address valid for mining was provided.
-    if (!IsValidMinerAddress(minerAddress)) {
+    if (!boost::apply_visitor(IsValidMinerAddress(), minerAddress)) {
         throw JSONRPCError(RPC_INTERNAL_ERROR, "No miner address available (mining requires a wallet or -mineraddress)");
     }
 
@@ -622,7 +628,7 @@ UniValue getblocktemplate(const UniValue& params, bool fHelp)
         GetMainSignals().AddressForMining(minerAddress);
 
         // Throw an error if no address valid for mining was provided.
-        if (!IsValidMinerAddress(minerAddress)) {
+        if (!boost::apply_visitor(IsValidMinerAddress(), minerAddress)) {
             throw JSONRPCError(RPC_INTERNAL_ERROR, "No miner address available (mining requires a wallet or -mineraddress)");
         }
 
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 6915f9ea3..ee17d9e69 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -4418,8 +4418,12 @@ void CWallet::GetAddressForMining(MinerAddress &minerAddress)
 
     boost::shared_ptr<CReserveKey> rKey(new CReserveKey(this));
     CPubKey pubkey;
-    if (!rKey->GetReservedKey(pubkey))
+    if (!rKey->GetReservedKey(pubkey)) {
+        // Explicitly return nullptr to indicate that the keypool is empty.
+        rKey = nullptr;
+        minerAddress = rKey;
         return;
+    }
 
     rKey->reserveScript = CScript() << OP_DUP << OP_HASH160 << ToByteVector(pubkey.GetID()) << OP_EQUALVERIFY << OP_CHECKSIG;
     minerAddress = rKey;