Add findFork and findBlock to the Chain interface

And use them to remove uses of chainActive and mapBlockIndex in wallet code

This commit does not change behavior.

Co-authored-by: Ben Woosley <ben.woosley@gmail.com>

4 files changed, 75 insertions, 19 deletions

diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp
index ca2533bb1d..6e2224bc3e 100644
--- a/src/interfaces/chain.cpp
+++ b/src/interfaces/chain.cpp
@@ -5,6 +5,8 @@
 #include <interfaces/chain.h>
 
 #include <chain.h>
+#include <chainparams.h>
+#include <primitives/block.h>
 #include <sync.h>
 #include <uint256.h>
 #include <util/system.h>
@@ -58,6 +60,22 @@ class LockImpl : public Chain::Lock
         assert(block != nullptr);
         return block->GetMedianTimePast();
     }
+    Optional<int> findFork(const uint256& hash, Optional<int>* height) override
+    {
+        const CBlockIndex* block = LookupBlockIndex(hash);
+        const CBlockIndex* fork = block ? ::chainActive.FindFork(block) : nullptr;
+        if (height) {
+            if (block) {
+                *height = block->nHeight;
+            } else {
+                height->reset();
+            }
+        }
+        if (fork) {
+            return fork->nHeight;
+        }
+        return nullopt;
+    }
 };
 
 class LockingStateImpl : public LockImpl, public UniqueLock<CCriticalSection>
@@ -77,6 +95,27 @@ public:
         return std::move(result);
     }
     std::unique_ptr<Chain::Lock> assumeLocked() override { return MakeUnique<LockImpl>(); }
+    bool findBlock(const uint256& hash, CBlock* block, int64_t* time, int64_t* time_max) override
+    {
+        CBlockIndex* index;
+        {
+            LOCK(cs_main);
+            index = LookupBlockIndex(hash);
+            if (!index) {
+                return false;
+            }
+            if (time) {
+                *time = index->GetBlockTime();
+            }
+            if (time_max) {
+                *time_max = index->GetBlockTimeMax();
+            }
+        }
+        if (block && !ReadBlockFromDisk(*block, index, Params().GetConsensus())) {
+            block->SetNull();
+        }
+        return true;
+    }
 };
 
 } // namespace
diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h
index 1db9d3861d..8c7bd8c6ea 100644
--- a/src/interfaces/chain.h
+++ b/src/interfaces/chain.h
@@ -12,6 +12,7 @@
 #include <string>
 #include <vector>
 
+class CBlock;
 class CScheduler;
 class uint256;
 
@@ -56,6 +57,13 @@ public:
         //! Get block median time past. Height must be valid or this function
         //! will abort.
         virtual int64_t getBlockMedianTimePast(int height) = 0;
+
+        //! Return height of the highest block on the chain that is an ancestor
+        //! of the specified block, or nullopt if no common ancestor is found.
+        //! Also return the height of the specified block as an optional output
+        //! parameter (to avoid the cost of a second hash lookup in case this
+        //! information is desired).
+        virtual Optional<int> findFork(const uint256& hash, Optional<int>* height) = 0;
     };
 
     //! Return Lock interface. Chain is locked when this is called, and
@@ -66,6 +74,17 @@ public:
     //! method is temporary and is only used in a few places to avoid changing
     //! behavior while code is transitioned to use the Chain::Lock interface.
     virtual std::unique_ptr<Lock> assumeLocked() = 0;
+
+    //! Return whether node has the block and optionally return block metadata
+    //! or contents.
+    //!
+    //! If a block pointer is provided to retrieve the block contents, and the
+    //! block exists but doesn't have data (for example due to pruning), the
+    //! block will be empty and all fields set to null.
+    virtual bool findBlock(const uint256& hash,
+        CBlock* block = nullptr,
+        int64_t* time = nullptr,
+        int64_t* max_time = nullptr) = 0;
 };
 
 //! Interface to let node manage chain clients (wallets, or maybe tools for
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index c835d2928d..298a674b07 100644
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@@ -102,7 +102,10 @@ static void WalletTxToJSON(interfaces::Chain& chain, interfaces::Chain::Lock& lo
     {
         entry.pushKV("blockhash", wtx.hashBlock.GetHex());
         entry.pushKV("blockindex", wtx.nIndex);
-        entry.pushKV("blocktime", LookupBlockIndex(wtx.hashBlock)->GetBlockTime());
+        int64_t block_time;
+        bool found_block = chain.findBlock(wtx.hashBlock, nullptr /* block */, &block_time);
+        assert(found_block);
+        entry.pushKV("blocktime", block_time);
     } else {
         entry.pushKV("trusted", wtx.IsTrusted(locked_chain));
     }
@@ -1573,24 +1576,18 @@ static UniValue listsinceblock(const JSONRPCRequest& request)
     auto locked_chain = pwallet->chain().lock();
     LOCK(pwallet->cs_wallet);
 
-    const CBlockIndex* pindex = nullptr;    // Block index of the specified block or the common ancestor, if the block provided was in a deactivated chain.
-    const CBlockIndex* paltindex = nullptr; // Block index of the specified block, even if it's in a deactivated chain.
+    Optional<int> height;    // Height of the specified block or the common ancestor, if the block provided was in a deactivated chain.
+    Optional<int> altheight; // Height of the specified block, even if it's in a deactivated chain.
     int target_confirms = 1;
     isminefilter filter = ISMINE_SPENDABLE;
 
+    uint256 blockId;
     if (!request.params[0].isNull() && !request.params[0].get_str().empty()) {
-        uint256 blockId(ParseHashV(request.params[0], "blockhash"));
-
-        paltindex = pindex = LookupBlockIndex(blockId);
-        if (!pindex) {
+        blockId = ParseHashV(request.params[0], "blockhash");
+        height = locked_chain->findFork(blockId, &altheight);
+        if (!height) {
             throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found");
         }
-        if (chainActive[pindex->nHeight] != pindex) {
-            // the block being asked for is a part of a deactivated chain;
-            // we don't want to depend on its perceived height in the block
-            // chain, we want to instead use the last common ancestor
-            pindex = chainActive.FindFork(pindex);
-        }
     }
 
     if (!request.params[1].isNull()) {
@@ -1608,7 +1605,7 @@ static UniValue listsinceblock(const JSONRPCRequest& request)
     bool include_removed = (request.params[3].isNull() || request.params[3].get_bool());
 
     const Optional<int> tip_height = locked_chain->getHeight();
-    int depth = tip_height && pindex ? (1 + *tip_height - pindex->nHeight) : -1;
+    int depth = tip_height && height ? (1 + *tip_height - *height) : -1;
 
     UniValue transactions(UniValue::VARR);
 
@@ -1623,9 +1620,9 @@ static 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 && paltindex && paltindex != pindex) {
+    while (include_removed && altheight && *altheight > *height) {
         CBlock block;
-        if (!ReadBlockFromDisk(block, paltindex, Params().GetConsensus())) {
+        if (!pwallet->chain().findBlock(blockId, &block) || block.IsNull()) {
             throw JSONRPCError(RPC_INTERNAL_ERROR, "Can't read block from disk");
         }
         for (const CTransactionRef& tx : block.vtx) {
@@ -1636,7 +1633,8 @@ static UniValue listsinceblock(const JSONRPCRequest& request)
                 ListTransactions(*locked_chain, pwallet, it->second, -100000000, true, removed, filter, nullptr /* filter_label */);
             }
         }
-        paltindex = paltindex->pprev;
+        blockId = block.hashPrevBlock;
+        --*altheight;
     }
 
     int last_height = tip_height ? *tip_height + 1 - target_confirms : -1;
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 35660372cf..32275c6180 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -3775,7 +3775,8 @@ unsigned int CWallet::ComputeTimeSmart(const CWalletTx& wtx) const
 {
     unsigned int nTimeSmart = wtx.nTimeReceived;
     if (!wtx.hashUnset()) {
-        if (const CBlockIndex* pindex = LookupBlockIndex(wtx.hashBlock)) {
+        int64_t blocktime;
+        if (chain().findBlock(wtx.hashBlock, nullptr /* block */, &blocktime)) {
             int64_t latestNow = wtx.nTimeReceived;
             int64_t latestEntry = 0;
 
@@ -3801,7 +3802,6 @@ unsigned int CWallet::ComputeTimeSmart(const CWalletTx& wtx) const
                 }
             }
 
-            int64_t blocktime = pindex->GetBlockTime();
             nTimeSmart = std::max(latestEntry, std::min(blocktime, latestNow));
         } else {
             WalletLogPrintf("%s: found %s in block %s not in index\n", __func__, wtx.GetHash().ToString(), wtx.hashBlock.ToString());