Merge bitcoin#14711: Remove uses of chainActive and mapBlockIndex in …

…wallet code

44de156 Remove remaining chainActive references from CWallet (Russell Yanofsky)
db21f02 Convert CWallet::ScanForWalletTransactions and SyncTransaction to the new Chain apis (Russell Yanofsky)
2ffb079 Add findFork and findBlock to the Chain interface (Russell Yanofsky)
d93c4c1 Add time methods to the Chain interface (Russell Yanofsky)
700c42b Add height, depth, and hash methods to the Chain interface (Russell Yanofsky)

Pull request description:

  This change removes uses of `chainActive` and `mapBlockIndex` globals in wallet code. It is a refactoring change which does not affect external behavior.

  This is the next step in the larger bitcoin#10973 refactoring change, which removes all other accesses to node global variables from wallet code. Doing this is useful to provide a better defined interface between the wallet and node, and necessary to allow wallet and node code to run in separate processes in bitcoin#10102.

Tree-SHA512: 4dcec8a31c458f54e2ea6ecf01e430469b0994c5b41a21a2d150efa67cd209f4c93ae210a101e064b3a87c52c6edfc70b070e979992be0e3a00fd425de6230a8

# Conflicts:
#	src/Makefile.am
#	src/interfaces/wallet.cpp
#	src/qt/test/wallettests.cpp
#	src/wallet/rpcdump.cpp
#	src/wallet/rpcwallet.cpp
#	src/wallet/test/wallet_tests.cpp
#	src/wallet/wallet.cpp
#	src/wallet/wallet.h

src/Makefile.am

@@ -230,6 +230,7 @@ BITCOIN_CORE_H = \
   node/coinstats.h \
   node/transaction.h \
   noui.h \
+  optional.h \
   policy/feerate.h \
   policy/fees.h \
   policy/policy.h \

src/interfaces/chain.cpp

@@ -4,7 +4,11 @@
 
 #include <interfaces/chain.h>
 
+#include <chain.h>
+#include <chainparams.h>
+#include <primitives/block.h>
 #include <sync.h>
+#include <uint256.h>
 #include <util/system.h>
 #include <validation.h>
 
@@ -16,6 +20,118 @@ namespace {
 
 class LockImpl : public Chain::Lock
 {
+    Optional<int> getHeight() override
+    {
+        int height = ::ChainActive().Height();
+        if (height >= 0) {
+            return height;
+        }
+        return nullopt;
+    }
+    Optional<int> getBlockHeight(const uint256& hash) override
+    {
+        CBlockIndex* block = LookupBlockIndex(hash);
+        if (block && ::ChainActive().Contains(block)) {
+            return block->nHeight;
+        }
+        return nullopt;
+    }
+    int getBlockDepth(const uint256& hash) override
+    {
+        const Optional<int> tip_height = getHeight();
+        const Optional<int> height = getBlockHeight(hash);
+        return tip_height && height ? *tip_height - *height + 1 : 0;
+    }
+    uint256 getBlockHash(int height) override
+    {
+        CBlockIndex* block = ::ChainActive()[height];
+        assert(block != nullptr);
+        return block->GetBlockHash();
+    }
+    int64_t getBlockTime(int height) override
+    {
+        CBlockIndex* block = ::ChainActive()[height];
+        assert(block != nullptr);
+        return block->GetBlockTime();
+    }
+    int64_t getBlockMedianTimePast(int height) override
+    {
+        CBlockIndex* block = ::ChainActive()[height];
+        assert(block != nullptr);
+        return block->GetMedianTimePast();
+    }
+    bool haveBlockOnDisk(int height) override
+    {
+        CBlockIndex* block = ::ChainActive()[height];
+        return block && ((block->nStatus & BLOCK_HAVE_DATA) != 0) && block->nTx > 0;
+    }
+    Optional<int> findFirstBlockWithTime(int64_t time, uint256* hash) override
+    {
+        CBlockIndex* block = ::ChainActive().FindEarliestAtLeast(time);
+        if (block) {
+            if (hash) *hash = block->GetBlockHash();
+            return block->nHeight;
+        }
+        return nullopt;
+    }
+    Optional<int> findFirstBlockWithTimeAndHeight(int64_t time, int height) override
+    {
+        // TODO: Could update CChain::FindEarliestAtLeast() to take a height
+        // parameter and use it with std::lower_bound() to make this
+        // implementation more efficient and allow combining
+        // findFirstBlockWithTime and findFirstBlockWithTimeAndHeight into one
+        // method.
+        for (CBlockIndex* block = ::ChainActive()[height]; block; block = ::ChainActive().Next(block)) {
+            if (block->GetBlockTime() >= time) {
+                return block->nHeight;
+            }
+        }
+        return nullopt;
+    }
+    Optional<int> findPruned(int start_height, Optional<int> stop_height) override
+    {
+        if (::fPruneMode) {
+            CBlockIndex* block = stop_height ? ::ChainActive()[*stop_height] : ::ChainActive().Tip();
+            while (block && block->nHeight >= start_height) {
+                if ((block->nStatus & BLOCK_HAVE_DATA) == 0) {
+                    return block->nHeight;
+                }
+                block = block->pprev;
+            }
+        }
+        return nullopt;
+    }
+    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;
+    }
+    bool isPotentialTip(const uint256& hash) override
+    {
+        if (::ChainActive().Tip()->GetBlockHash() == hash) return true;
+        CBlockIndex* block = LookupBlockIndex(hash);
+        return block && block->GetAncestor(::ChainActive().Height()) == ::ChainActive().Tip();
+    }
+    CBlockLocator getLocator() override { return ::ChainActive().GetLocator(); }
+    Optional<int> findLocatorFork(const CBlockLocator& locator) override
+    {
+        LockAnnotation lock(::cs_main);
+        if (CBlockIndex* fork = FindForkInGlobalIndex(::ChainActive(), locator)) {
+            return fork->nHeight;
+        }
+        return nullopt;
+    }
 };
 
 class LockingStateImpl : public LockImpl, public UniqueLock<CCriticalSection>
@@ -35,6 +151,32 @@ class ChainImpl : public Chain
         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;
+    }
+    double guessVerificationProgress(const uint256& block_hash) override
+    {
+        LOCK(cs_main);
+        return GuessVerificationProgress(Params().TxData(), LookupBlockIndex(block_hash));
+    }
 };
 
 } // namespace

src/interfaces/chain.h

@@ -5,11 +5,17 @@
 #ifndef BITCOIN_INTERFACES_CHAIN_H
 #define BITCOIN_INTERFACES_CHAIN_H
 
+#include <optional.h>
+
 #include <memory>
+#include <stdint.h>
 #include <string>
 #include <vector>
 
+class CBlock;
 class CScheduler;
+class uint256;
+struct CBlockLocator;
 
 namespace interfaces {
 
@@ -28,6 +34,74 @@ class Chain
     {
     public:
         virtual ~Lock() {}
+
+        //! Get current chain height, not including genesis block (returns 0 if
+        //! chain only contains genesis block, nullopt if chain does not contain
+        //! any blocks).
+        virtual Optional<int> getHeight() = 0;
+
+        //! Get block height above genesis block. Returns 0 for genesis block,
+        //! 1 for following block, and so on. Returns nullopt for a block not
+        //! included in the current chain.
+        virtual 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.
+        virtual int getBlockDepth(const uint256& hash) = 0;
+
+        //! Get block hash. Height must be valid or this function will abort.
+        virtual uint256 getBlockHash(int height) = 0;
+
+        //! Get block time. Height must be valid or this function will abort.
+        virtual int64_t getBlockTime(int height) = 0;
+
+        //! Get block median time past. Height must be valid or this function
+        //! will abort.
+        virtual int64_t getBlockMedianTimePast(int height) = 0;
+
+        //! Check that the block is available on disk (i.e. has not been
+        //! pruned), and contains transactions.
+        virtual bool haveBlockOnDisk(int height) = 0;
+
+        //! Return height of the first block in the chain with timestamp equal
+        //! or greater than the given time, or nullopt if there is no block with
+        //! a high enough timestamp. Also return the block hash as an optional
+        //! output parameter (to avoid the cost of a second lookup in case this
+        //! information is needed.)
+        virtual Optional<int> findFirstBlockWithTime(int64_t time, uint256* hash) = 0;
+
+        //! Return height of the first block in the chain with timestamp equal
+        //! or greater than the given time and height equal or greater than the
+        //! given height, or nullopt if there is no such block.
+        //!
+        //! Calling this with height 0 is equivalent to calling
+        //! findFirstBlockWithTime, but less efficient because it requires a
+        //! linear instead of a binary search.
+        virtual Optional<int> findFirstBlockWithTimeAndHeight(int64_t time, int height) = 0;
+
+        //! Return height of last block in the specified range which is pruned, or
+        //! nullopt if no block in the range is pruned. Range is inclusive.
+        virtual Optional<int> findPruned(int start_height = 0, Optional<int> stop_height = nullopt) = 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 true if block hash points to the current chain tip, or to a
+        //! possible descendant of the current chain tip that isn't currently
+        //! connected.
+        virtual bool isPotentialTip(const uint256& hash) = 0;
+
+        //! Get locator for the current chain tip.
+        virtual CBlockLocator getLocator() = 0;
+
+        //! Return height of the latest block common to locator and chain, which
+        //! is guaranteed to be an ancestor of the block used to create the
+        //! locator.
+        virtual Optional<int> findLocatorFork(const CBlockLocator& locator) = 0;
     };
 
     //! Return Lock interface. Chain is locked when this is called, and
@@ -38,6 +112,21 @@ class Chain
     //! 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;
+
+    //! Estimate fraction of total transactions verified if blocks up to
+    //! the specified block hash are verified.
+    virtual double guessVerificationProgress(const uint256& block_hash) = 0;
 };
 
 //! Interface to let node manage chain clients (wallets, or maybe tools for

src/interfaces/wallet.cpp

@@ -382,8 +382,12 @@ class WalletImpl : public Wallet
         if (mi == m_wallet->mapWallet.end()) {
             return false;
         }
+        if (Optional<int> height = locked_chain->getHeight()) {
+            block_time = locked_chain->getBlockTime(*height);
+        } else {
+            block_time = -1;
+        }
         tx_status = MakeWalletTxStatus(*locked_chain, mi->second);
-        block_time = ::ChainActive().Tip()->GetBlockTime();
         return true;
     }
     WalletTx getWalletTxDetails(const uint256& txid,
@@ -396,7 +400,7 @@ class WalletImpl : public Wallet
         LOCK(m_wallet->cs_wallet);
         auto mi = m_wallet->mapWallet.find(txid);
         if (mi != m_wallet->mapWallet.end()) {
-            num_blocks = ::ChainActive().Height();
+            num_blocks = locked_chain->getHeight().value_or(-1);
             in_mempool = mi->second.InMempool();
             order_form = mi->second.vOrderForm;
             tx_status = MakeWalletTxStatus(*locked_chain, mi->second);
@@ -431,7 +435,7 @@ class WalletImpl : public Wallet
             return false;
         }
         balances = getBalances();
-        num_blocks = ::ChainActive().Height();
+        num_blocks = locked_chain->getHeight().value_or(-1);
         return true;
     }
     CAmount getBalance() override

src/optional.h

@@ -0,0 +1,17 @@
+// Copyright (c) 2017 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef BITCOIN_OPTIONAL_H
+#define BITCOIN_OPTIONAL_H
+
+#include <boost/optional.hpp>
+
+//! Substitute for C++17 std::optional
+template <typename T>
+using Optional = boost::optional<T>;
+
+//! Substitute for C++17 std::nullopt
+static auto& nullopt = boost::none;
+
+#endif // BITCOIN_OPTIONAL_H

src/qt/test/wallettests.cpp

@@ -2,6 +2,7 @@
 #include <qt/test/util.h>
 
 #include <coinjoin/client.h>
+#include <init.h>
 #include <interfaces/chain.h>
 #include <interfaces/node.h>
 #include <base58.h>
@@ -123,13 +124,10 @@ void TestGUI()
 
         WalletRescanReserver reserver(wallet.get());
         reserver.reserve();
-        const CBlockIndex* const null_block = nullptr;
-        const CBlockIndex *stop_block, *failed_block;
-        QCOMPARE(
-            wallet->ScanForWalletTransactions(::ChainActive().Genesis(), nullptr, reserver, failed_block, stop_block, true /* fUpdate */),
-            CWallet::ScanResult::SUCCESS);
-        QCOMPARE(stop_block, ::ChainActive().Tip());
-        QCOMPARE(failed_block, null_block);
+        CWallet::ScanResult result = wallet->ScanForWalletTransactions(locked_chain->getBlockHash(0), {} /* stop_block */, reserver, true /* fUpdate */);
+        QCOMPARE(result.status, CWallet::ScanResult::SUCCESS);
+        QCOMPARE(result.stop_block, ::ChainActive().Tip()->GetBlockHash());
+        QVERIFY(result.failed_block.IsNull());
     }
     wallet->SetBroadcastTransactions(true);