fixup! Remove uses of chainActive and mapBlockIndex in wallet code

Return unset optional instead of -1 from functions trying to return heights of
blocks that might not exist.

Change suggested by Jeremy Rubin <jeremy.l.rubin@gmail.com>
bitcoin#10973 (comment)

src/interface/chain.cpp

@@ -51,21 +51,29 @@ namespace {
 class LockImpl : public Chain::Lock
 {
 public:
-    int getHeight() override { return ::chainActive.Height(); }
-    int getBlockHeight(const uint256& hash) override
+    boost::optional<int> getHeight() override
+    {
+        int height = ::chainActive.Height();
+        if (height >= 0) {
+            return height;
+        }
+        return {};
+    }
+    boost::optional<int> getBlockHeight(const uint256& hash) override
     {
         auto it = ::mapBlockIndex.find(hash);
         if (it != ::mapBlockIndex.end() && it->second) {
             if (::chainActive.Contains(it->second)) {
                 return it->second->nHeight;
             }
         }
-        return -1;
+        return {};
     }
     int getBlockDepth(const uint256& hash) override
     {
-        int height = getBlockHeight(hash);
-        return height < 0 ? 0 : ::chainActive.Height() - height + 1;
+        const auto tip_height = getHeight();
+        const auto height = getBlockHeight(hash);
+        return tip_height && height ? *tip_height - *height + 1 : 0;
     }
     uint256 getBlockHash(int height) override { return ::chainActive[height]->GetBlockHash(); }
     int64_t getBlockTime(int height) override { return ::chainActive[height]->GetBlockTime(); }
@@ -84,41 +92,58 @@ class LockImpl : public Chain::Lock
     {
         return GuessVerificationProgress(Params().TxData(), ::chainActive[height]);
     }
-    int findEarliestAtLeast(int64_t time) override
+    boost::optional<int> findEarliestAtLeast(int64_t time) override
     {
         CBlockIndex* block = ::chainActive.FindEarliestAtLeast(time);
-        return block ? block->nHeight : -1;
+        if (block) {
+            return block->nHeight;
+        }
+        return {};
     }
-    int findLastBefore(int64_t time, int start_height) override
+    boost::optional<int> findLastBefore(int64_t time, int start_height) override
     {
         CBlockIndex* block = ::chainActive[start_height];
         while (block && block->GetBlockTime() < time) {
             block = ::chainActive.Next(block);
         }
-        return block ? block->nHeight : -1;
+        if (block) {
+            return block->nHeight;
+        }
+        return {};
     }
     bool isPotentialTip(const uint256& hash) override
     {
         if (::chainActive.Tip()->GetBlockHash() == hash) return true;
         auto it = ::mapBlockIndex.find(hash);
         return it != ::mapBlockIndex.end() && it->second->GetAncestor(::chainActive.Height()) == ::chainActive.Tip();
     }
-    int findFork(const uint256& hash, int* height) override
+    boost::optional<int> findFork(const uint256& hash, boost::optional<int>* height) override
     {
         const CBlockIndex *block{nullptr}, *fork{nullptr};
         auto it = ::mapBlockIndex.find(hash);
         if (it != ::mapBlockIndex.end()) {
             block = it->second;
             fork = ::chainActive.FindFork(block);
         }
-        if (height) *height = block ? block->nHeight : -1;
-        return fork ? fork->nHeight : -1;
+        if (height) {
+            if (block) {
+                *height = block->nHeight;
+            } else {
+                height->reset();
+            }
+        }
+        if (fork) {
+            return fork->nHeight;
+        }
+        return {};
     }
     CBlockLocator getLocator() override { return ::chainActive.GetLocator(); }
-    int findLocatorFork(const CBlockLocator& locator) override
+    boost::optional<int> findLocatorFork(const CBlockLocator& locator) override
     {
-        CBlockIndex* fork = FindForkInGlobalIndex(::chainActive, locator);
-        return fork ? fork->nHeight : -1;
+        if (CBlockIndex* fork = FindForkInGlobalIndex(::chainActive, locator)) {
+            return fork->nHeight;
+        }
+        return {};
     }
     bool checkFinalTx(const CTransaction& tx) override { return CheckFinalTx(tx); }
     bool isWitnessEnabled() override { return ::IsWitnessEnabled(::chainActive.Tip(), Params().GetConsensus()); }

src/interface/chain.h

@@ -6,6 +6,7 @@
 #include <policy/rbf.h>             // For RBFTransactionState
 #include <primitives/transaction.h> // For CTransactionRef
 
+#include <boost/optional/optional.hpp>
 #include <memory>
 #include <stddef.h>
 #include <stdint.h>
@@ -46,12 +47,12 @@ class Chain
         //! Get current chain height, not including genesis block (returns 0 if
         //! chain only contains genesis block, -1 if chain does not contain any
         //! blocks).
-        virtual int getHeight() = 0;
+        virtual boost::optional<int> getHeight() = 0;
 
         //! Get block height above genesis block. Returns 0 for genesis block, 1 for
         //! following block, and so on. Returns -1 for a block not included in the
         //! current chain.
-        virtual int getBlockHeight(const uint256& hash) = 0;
+        virtual boost::optional<int> getBlockHeight(const uint256& hash) = 0;
 
         //! Get block depth. Returns 1 for chain tip, 2 for preceding block, and
         //! so on. Returns 0 for a block not included in the current chain.
@@ -80,19 +81,19 @@ class Chain
         virtual double guessVerificationProgress(int height) = 0;
 
         //! Return height of earliest block in chain with timestamp equal or
-        //! greater than the given time, or -1 if there is no block with a high
-        //! enough timestamp.
-        virtual int findEarliestAtLeast(int64_t time) = 0;
+        //! greater than the given time, or nothing if there is no block with a
+        //! high enough timestamp.
+        virtual boost::optional<int> findEarliestAtLeast(int64_t time) = 0;
 
-        //! Return height of last block in chain with timestamp less than the given,
-        //! and height less than or equal to the given, or -1 if there is no such
-        //! block.
-        virtual int findLastBefore(int64_t time, int start_height) = 0;
+        //! Return height of last block in chain with timestamp less than the
+        //! given, and height less than or equal to the given, or nothing if
+        //! there is no such block.
+        virtual boost::optional<int> findLastBefore(int64_t time, int start_height) = 0;
 
         //! Return height of the highest block on the chain that is an ancestor
         //! of the specified block. Also, optionally return the height of the
         //! specified block.
-        virtual int findFork(const uint256& hash, int* height) = 0;
+        virtual boost::optional<int> findFork(const uint256& hash, boost::optional<int>* height) = 0;
 
         //! Return true if block hash points to the current chain tip, or to a
         //! possible descendant of the current chain tip that isn't currently
@@ -103,7 +104,7 @@ class Chain
         virtual CBlockLocator getLocator() = 0;
 
         //! Return height of block on the chain using locator.
-        virtual int findLocatorFork(const CBlockLocator& locator) = 0;
+        virtual boost::optional<int> findLocatorFork(const CBlockLocator& locator) = 0;
 
         //! Check if transaction will be final given chain height current time.
         virtual bool checkFinalTx(const CTransaction& tx) = 0;

src/wallet/rpcdump.cpp

@@ -485,8 +485,8 @@ UniValue importwallet(const JSONRPCRequest& request)
     if (!file.is_open())
         throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot open wallet dump file");
 
-    const int tip_height = locked_chain->getHeight();
-    int64_t nTimeBegin = tip_height >= 0 ? locked_chain->getBlockTime(locked_chain->getHeight()) : 0;
+    const auto tip_height = locked_chain->getHeight();
+    int64_t nTimeBegin = tip_height ? locked_chain->getBlockTime(*tip_height) : 0;
 
     bool fGood = true;
 
@@ -648,9 +648,9 @@ UniValue dumpwallet(const JSONRPCRequest& request)
     // produce output
     file << strprintf("# Wallet dump created by Bitcoin %s\n", CLIENT_BUILD);
     file << strprintf("# * Created on %s\n", EncodeDumpTime(GetTime()));
-    const int tip_height = locked_chain->getHeight();
-    file << strprintf("# * Best block at time of backup was %i (%s),\n", tip_height, tip_height >= 0 ? locked_chain->getBlockHash(tip_height).ToString() : "(missing block hash)");
-    file << strprintf("#   mined on %s\n", tip_height ? EncodeDumpTime(locked_chain->getBlockTime(tip_height)) : "(missing block time)");
+    const auto tip_height = locked_chain->getHeight();
+    file << strprintf("# * Best block at time of backup was %i (%s),\n", tip_height ? *tip_height : -1, tip_height ? locked_chain->getBlockHash(*tip_height).ToString() : "(missing block hash)");
+    file << strprintf("#   mined on %s\n", tip_height ? EncodeDumpTime(locked_chain->getBlockTime(*tip_height)) : "(missing block time)");
     file << "\n";
 
     // add the base58check encoded extended master if the wallet uses HD 
@@ -1093,8 +1093,8 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
     EnsureWalletIsUnlocked(pwallet);
 
     // Verify all timestamps are present before importing any keys.
-    const int tip_height = locked_chain->getHeight();
-    const int64_t now = tip_height >= 0 ? locked_chain->getBlockMedianTimePast(tip_height) : 0;
+    const auto tip_height = locked_chain->getHeight();
+    const int64_t now = tip_height ? locked_chain->getBlockMedianTimePast(*tip_height) : 0;
     for (const UniValue& data : requests.getValues()) {
         GetImportTimestamp(data, now);
     }
@@ -1103,8 +1103,8 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
     const int64_t minimumTimestamp = 1;
     int64_t nLowestTimestamp = 0;
 
-    if (fRescan && tip_height >= 0) {
-        nLowestTimestamp = locked_chain->getBlockTime(tip_height);
+    if (fRescan && tip_height) {
+        nLowestTimestamp = locked_chain->getBlockTime(*tip_height);
     } else {
         fRescan = false;
     }

src/wallet/rpcwallet.cpp

@@ -1940,8 +1940,8 @@ UniValue listsinceblock(const JSONRPCRequest& request)
     auto locked_chain = pwallet->chain().lock();
     LOCK(pwallet->cs_wallet);
 
-    int height = -1;    // Height of the specified block or the common ancestor, if the block provided was in a deactivated chain.
-    int altheight = -1; // Height of the specified block, even if it's in a deactivated chain.
+    boost::optional<int> height;    // Height of the specified block or the common ancestor, if the block provided was in a deactivated chain.
+    boost::optional<int> altheight; // Height of the specified block, even if it's in a deactivated chain.
     int target_confirms = 1;
     isminefilter filter = ISMINE_SPENDABLE;
 
@@ -1965,8 +1965,8 @@ UniValue listsinceblock(const JSONRPCRequest& request)
 
     bool include_removed = (request.params[3].isNull() || request.params[3].get_bool());
 
-    const int tip_height = locked_chain->getHeight();
-    int depth = height >= 0 ? (1 + tip_height - height) : -1;
+    const auto tip_height = locked_chain->getHeight();
+    int depth = tip_height && height ? (1 + *tip_height - *height) : -1;
 
     UniValue transactions(UniValue::VARR);
 
@@ -1981,7 +1981,7 @@ UniValue listsinceblock(const JSONRPCRequest& request)
     // when a reorg'd block is requested, we also list any relevant transactions
     // in the blocks of the chain that was detached
     UniValue removed(UniValue::VARR);
-    while (include_removed && altheight >= 0 && altheight > height) {
+    while (include_removed && altheight && *altheight > (height ? *height : -1)) {
         CBlock block;
         if (!pwallet->chain().findBlock(blockId, &block) || block.IsNull()) {
             throw JSONRPCError(RPC_INTERNAL_ERROR, "Can't read block from disk");
@@ -1995,10 +1995,10 @@ UniValue listsinceblock(const JSONRPCRequest& request)
             }
         }
         blockId = block.hashPrevBlock;
-        --altheight;
+        --*altheight;
     }
 
-    int last_height = tip_height + 1 - target_confirms;
+    int last_height = tip_height ? *tip_height + 1 - target_confirms : -1;
     uint256 lastblock = last_height >= 0 ? locked_chain->getBlockHash(last_height) : uint256();
 
     UniValue ret(UniValue::VOBJ);