Merge #17954: wallet: Remove calls to Chain::Lock methods

4897340 wallet: Avoid use of Chain::Lock in CWallet::GetKeyBirthTimes (Russell Yanofsky)
e958ff9 wallet: Avoid use of Chain::Lock in CWallet::CreateTransaction (Russell Yanofsky)
c0d07dc wallet: Avoid use of Chain::Lock in CWallet::ScanForWalletTransactions (Russell Yanofsky)
1be8ff2 wallet: Avoid use of Chain::Lock in rescanblockchain (Russell Yanofsky)
3cb85ac wallet refactor: Avoid use of Chain::Lock in CWallet::RescanFromTime (Russell Yanofsky)
f7ba881 wallet: Avoid use of Chain::Lock in listsinceblock (Russell Yanofsky)
bc96a9b wallet: Avoid use of Chain::Lock in importmulti (Russell Yanofsky)
25a9fcf wallet: Avoid use of Chain::Lock in importwallet and dumpwallet (Russell Yanofsky)
c1694ce wallet: Avoid use of Chain::Lock in importprunedfunds (Russell Yanofsky)
ade5f87 wallet refactor: Avoid use of Chain::Lock in qt wallettests (Russell Yanofsky)
f6da44c wallet: Avoid use of Chain::Lock in tryGetTxStatus and tryGetBalances (Russell Yanofsky)
bf30cd4 refactor: Add interfaces::FoundBlock class to selectively return block data (Russell Yanofsky)

Pull request description:

  This is a set of changes updating wallet code to make fewer calls to `Chain::Lock` methods, so the `Chain::Lock` class will be easier to remove in #16426 with fewer code changes and small changes to behavior.

ACKs for top commit:
  MarcoFalke:
    re-ACK 4897340, only change is fixing bug 📀
  fjahr:
    re-ACK 4897340, reviewed rebase and changes since last review, built and ran tests locally
  ariard:
    Coce Review ACK 4897340, only changes are one suggested by last review on more accurate variable naming, human-readable output, args comments in `findCommonAncestor`

Tree-SHA512: cfd2f559f976b6faaa032794c40c9659191d5597b013abcb6c7968d36b2abb2b14d4e596f8ed8b9a077e96522365261299a241a939b3111eaf729ba0c3ef519b

Author: MarcoFalke

src/Makefile.test.include

@@ -201,6 +201,7 @@ BITCOIN_TESTS =\
   test/fs_tests.cpp \
   test/getarg_tests.cpp \
   test/hash_tests.cpp \
+  test/interfaces_tests.cpp \
   test/key_io_tests.cpp \
   test/key_tests.cpp \
   test/limitedmap_tests.cpp \

src/interfaces/chain.cpp

@@ -38,6 +38,21 @@
 namespace interfaces {
 namespace {
 
+bool FillBlock(const CBlockIndex* index, const FoundBlock& block, UniqueLock<RecursiveMutex>& lock)
+{
+    if (!index) return false;
+    if (block.m_hash) *block.m_hash = index->GetBlockHash();
+    if (block.m_height) *block.m_height = index->nHeight;
+    if (block.m_time) *block.m_time = index->GetBlockTime();
+    if (block.m_max_time) *block.m_max_time = index->GetBlockTimeMax();
+    if (block.m_mtp_time) *block.m_mtp_time = index->GetMedianTimePast();
+    if (block.m_data) {
+        REVERSE_LOCK(lock);
+        if (!ReadBlockFromDisk(*block.m_data, index, Params().GetConsensus())) block.m_data->SetNull();
+    }
+    return true;
+}
+
 class LockImpl : public Chain::Lock, public UniqueLock<RecursiveMutex>
 {
     Optional<int> getHeight() override
@@ -65,20 +80,6 @@ class LockImpl : public Chain::Lock, public UniqueLock<RecursiveMutex>
         assert(block != nullptr);
         return block->GetBlockHash();
     }
-    int64_t getBlockTime(int height) override
-    {
-        LockAssertion lock(::cs_main);
-        CBlockIndex* block = ::ChainActive()[height];
-        assert(block != nullptr);
-        return block->GetBlockTime();
-    }
-    int64_t getBlockMedianTimePast(int height) override
-    {
-        LockAssertion lock(::cs_main);
-        CBlockIndex* block = ::ChainActive()[height];
-        assert(block != nullptr);
-        return block->GetMedianTimePast();
-    }
     bool haveBlockOnDisk(int height) override
     {
         LockAssertion lock(::cs_main);
@@ -95,20 +96,6 @@ class LockImpl : public Chain::Lock, public UniqueLock<RecursiveMutex>
         }
         return nullopt;
     }
-    Optional<int> findPruned(int start_height, Optional<int> stop_height) override
-    {
-        LockAssertion lock(::cs_main);
-        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
     {
         LockAssertion lock(::cs_main);
@@ -247,33 +234,74 @@ class ChainImpl : public Chain
         std::unique_ptr<Chain::Lock> result = std::move(lock); // Temporary to avoid CWG 1579
         return result;
     }
-    bool findBlock(const uint256& hash, CBlock* block, int64_t* time, int64_t* time_max) override
+    bool findBlock(const uint256& hash, const FoundBlock& block) 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();
+        WAIT_LOCK(cs_main, lock);
+        return FillBlock(LookupBlockIndex(hash), block, lock);
+    }
+    bool findFirstBlockWithTimeAndHeight(int64_t min_time, int min_height, const FoundBlock& block) override
+    {
+        WAIT_LOCK(cs_main, lock);
+        return FillBlock(ChainActive().FindEarliestAtLeast(min_time, min_height), block, lock);
+    }
+    bool findNextBlock(const uint256& block_hash, int block_height, const FoundBlock& next, bool* reorg) override {
+        WAIT_LOCK(cs_main, lock);
+        CBlockIndex* block = ChainActive()[block_height];
+        if (block && block->GetBlockHash() != block_hash) block = nullptr;
+        if (reorg) *reorg = !block;
+        return FillBlock(block ? ChainActive()[block_height + 1] : nullptr, next, lock);
+    }
+    bool findAncestorByHeight(const uint256& block_hash, int ancestor_height, const FoundBlock& ancestor_out) override
+    {
+        WAIT_LOCK(cs_main, lock);
+        if (const CBlockIndex* block = LookupBlockIndex(block_hash)) {
+            if (const CBlockIndex* ancestor = block->GetAncestor(ancestor_height)) {
+                return FillBlock(ancestor, ancestor_out, lock);
             }
         }
-        if (block && !ReadBlockFromDisk(*block, index, Params().GetConsensus())) {
-            block->SetNull();
-        }
-        return true;
+        return FillBlock(nullptr, ancestor_out, lock);
+    }
+    bool findAncestorByHash(const uint256& block_hash, const uint256& ancestor_hash, const FoundBlock& ancestor_out) override
+    {
+        WAIT_LOCK(cs_main, lock);
+        const CBlockIndex* block = LookupBlockIndex(block_hash);
+        const CBlockIndex* ancestor = LookupBlockIndex(ancestor_hash);
+        if (block && ancestor && block->GetAncestor(ancestor->nHeight) != ancestor) ancestor = nullptr;
+        return FillBlock(ancestor, ancestor_out, lock);
+    }
+    bool findCommonAncestor(const uint256& block_hash1, const uint256& block_hash2, const FoundBlock& ancestor_out, const FoundBlock& block1_out, const FoundBlock& block2_out) override
+    {
+        WAIT_LOCK(cs_main, lock);
+        const CBlockIndex* block1 = LookupBlockIndex(block_hash1);
+        const CBlockIndex* block2 = LookupBlockIndex(block_hash2);
+        const CBlockIndex* ancestor = block1 && block2 ? LastCommonAncestor(block1, block2) : nullptr;
+        return FillBlock(ancestor, ancestor_out, lock) & FillBlock(block1, block1_out, lock) & FillBlock(block2, block2_out, lock);
     }
     void findCoins(std::map<COutPoint, Coin>& coins) override { return FindCoins(m_node, coins); }
     double guessVerificationProgress(const uint256& block_hash) override
     {
         LOCK(cs_main);
         return GuessVerificationProgress(Params().TxData(), LookupBlockIndex(block_hash));
     }
+    bool hasBlocks(const uint256& block_hash, int min_height, Optional<int> max_height) override
+    {
+        // hasBlocks returns true if all ancestors of block_hash in specified
+        // range have block data (are not pruned), false if any ancestors in
+        // specified range are missing data.
+        //
+        // For simplicity and robustness, min_height and max_height are only
+        // used to limit the range, and passing min_height that's too low or
+        // max_height that's too high will not crash or change the result.
+        LOCK(::cs_main);
+        if (CBlockIndex* block = LookupBlockIndex(block_hash)) {
+            if (max_height && block->nHeight >= *max_height) block = block->GetAncestor(*max_height);
+            for (; block->nStatus & BLOCK_HAVE_DATA; block = block->pprev) {
+                // Check pprev to not segfault if min_height is too low
+                if (block->nHeight <= min_height || !block->pprev) return true;
+            }
+        }
+        return false;
+    }
     RBFTransactionState isRBFOptIn(const CTransaction& tx) override
     {
         LOCK(::mempool.cs);

src/interfaces/chain.h

@@ -30,6 +30,27 @@ namespace interfaces {
 class Handler;
 class Wallet;
 
+//! Helper for findBlock to selectively return pieces of block data.
+class FoundBlock
+{
+public:
+    FoundBlock& hash(uint256& hash) { m_hash = &hash; return *this; }
+    FoundBlock& height(int& height) { m_height = &height; return *this; }
+    FoundBlock& time(int64_t& time) { m_time = &time; return *this; }
+    FoundBlock& maxTime(int64_t& max_time) { m_max_time = &max_time; return *this; }
+    FoundBlock& mtpTime(int64_t& mtp_time) { m_mtp_time = &mtp_time; return *this; }
+    //! Read block data from disk. If the block exists but doesn't have data
+    //! (for example due to pruning), the CBlock variable will be set to null.
+    FoundBlock& data(CBlock& data) { m_data = &data; return *this; }
+
+    uint256* m_hash = nullptr;
+    int* m_height = nullptr;
+    int64_t* m_time = nullptr;
+    int64_t* m_max_time = nullptr;
+    int64_t* m_mtp_time = nullptr;
+    CBlock* m_data = nullptr;
+};
+
 //! Interface giving clients (wallet processes, maybe other analysis tools in
 //! the future) ability to access to the chain state, receive notifications,
 //! estimate fees, and submit transactions.
@@ -79,13 +100,6 @@ class Chain
         //! 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;
@@ -97,10 +111,6 @@ class Chain
         //! (to avoid the cost of a second lookup in case this information is needed.)
         virtual Optional<int> findFirstBlockWithTimeAndHeight(int64_t time, int height, uint256* hash) = 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 specified block if it is on the chain, otherwise
         //! return the height of the highest block on chain that's an ancestor
         //! of the specified block, or nullopt if there is no common ancestor.
@@ -127,14 +137,36 @@ class Chain
 
     //! 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;
+    virtual bool findBlock(const uint256& hash, const FoundBlock& block={}) = 0;
+
+    //! Find first block in the chain with timestamp >= the given time
+    //! and height >= than the given height, return false if there is no block
+    //! with a high enough timestamp and height. Optionally return block
+    //! information.
+    virtual bool findFirstBlockWithTimeAndHeight(int64_t min_time, int min_height, const FoundBlock& block={}) = 0;
+
+    //! Find next block if block is part of current chain. Also flag if
+    //! there was a reorg and the specified block hash is no longer in the
+    //! current chain, and optionally return block information.
+    virtual bool findNextBlock(const uint256& block_hash, int block_height, const FoundBlock& next={}, bool* reorg=nullptr) = 0;
+
+    //! Find ancestor of block at specified height and optionally return
+    //! ancestor information.
+    virtual bool findAncestorByHeight(const uint256& block_hash, int ancestor_height, const FoundBlock& ancestor_out={}) = 0;
+
+    //! Return whether block descends from a specified ancestor, and
+    //! optionally return ancestor information.
+    virtual bool findAncestorByHash(const uint256& block_hash,
+        const uint256& ancestor_hash,
+        const FoundBlock& ancestor_out={}) = 0;
+
+    //! Find most recent common ancestor between two blocks and optionally
+    //! return block information.
+    virtual bool findCommonAncestor(const uint256& block_hash1,
+        const uint256& block_hash2,
+        const FoundBlock& ancestor_out={},
+        const FoundBlock& block1_out={},
+        const FoundBlock& block2_out={}) = 0;
 
     //! Look up unspent output information. Returns coins in the mempool and in
     //! the current chain UTXO set. Iterates through all the keys in the map and
@@ -145,6 +177,11 @@ class Chain
     //! the specified block hash are verified.
     virtual double guessVerificationProgress(const uint256& block_hash) = 0;
 
+    //! Return true if data is available for all blocks in the specified range
+    //! of blocks. This checks all blocks that are ancestors of block_hash in
+    //! the height range from min_height to max_height, inclusive.
+    virtual bool hasBlocks(const uint256& block_hash, int min_height = 0, Optional<int> max_height = {}) = 0;
+
     //! Check if transaction is RBF opt in.
     virtual RBFTransactionState isRBFOptIn(const CTransaction& tx) = 0;
 

src/interfaces/wallet.cpp

@@ -14,6 +14,7 @@
 #include <sync.h>
 #include <ui_interface.h>
 #include <uint256.h>
+#include <util/check.h>
 #include <util/system.h>
 #include <wallet/feebumper.h>
 #include <wallet/fees.h>
@@ -62,7 +63,7 @@ WalletTx MakeWalletTx(CWallet& wallet, const CWalletTx& wtx)
 WalletTxStatus MakeWalletTxStatus(interfaces::Chain::Lock& locked_chain, const CWalletTx& wtx)
 {
     WalletTxStatus result;
-    result.block_height = locked_chain.getBlockHeight(wtx.m_confirm.hashBlock).get_value_or(std::numeric_limits<int>::max());
+    result.block_height = wtx.m_confirm.block_height > 0 ? wtx.m_confirm.block_height : std::numeric_limits<int>::max();
     result.blocks_to_maturity = wtx.GetBlocksToMaturity();
     result.depth_in_main_chain = wtx.GetDepthInMainChain();
     result.time_received = wtx.nTimeReceived;
@@ -318,13 +319,9 @@ class WalletImpl : public Wallet
         if (mi == m_wallet->mapWallet.end()) {
             return false;
         }
-        if (Optional<int> height = locked_chain->getHeight()) {
-            num_blocks = *height;
-            block_time = locked_chain->getBlockTime(*height);
-        } else {
-            num_blocks = -1;
-            block_time = -1;
-        }
+        num_blocks = m_wallet->GetLastBlockHeight();
+        block_time = -1;
+        CHECK_NONFATAL(m_wallet->chain().findBlock(m_wallet->GetLastBlockHash(), FoundBlock().time(block_time)));
         tx_status = MakeWalletTxStatus(*locked_chain, mi->second);
         return true;
     }
@@ -373,12 +370,12 @@ class WalletImpl : public Wallet
     {
         auto locked_chain = m_wallet->chain().lock(true /* try_lock */);
         if (!locked_chain) return false;
-        num_blocks = locked_chain->getHeight().get_value_or(-1);
-        if (!force && num_blocks == cached_num_blocks) return false;
         TRY_LOCK(m_wallet->cs_wallet, locked_wallet);
         if (!locked_wallet) {
             return false;
         }
+        num_blocks = m_wallet->GetLastBlockHeight();
+        if (!force && num_blocks == cached_num_blocks) return false;
         balances = getBalances();
         return true;
     }

src/qt/test/wallettests.cpp

@@ -152,12 +152,9 @@ void TestGUI(interfaces::Node& node)
         wallet->SetLastBlockProcessed(105, ::ChainActive().Tip()->GetBlockHash());
     }
     {
-        auto locked_chain = wallet->chain().lock();
-        LockAssertion lock(::cs_main);
-
         WalletRescanReserver reserver(wallet.get());
         reserver.reserve();
-        CWallet::ScanResult result = wallet->ScanForWalletTransactions(locked_chain->getBlockHash(0), {} /* stop_block */, reserver, true /* fUpdate */);
+        CWallet::ScanResult result = wallet->ScanForWalletTransactions(Params().GetConsensus().hashGenesisBlock, 0 /* block height */, {} /* max height */, reserver, true /* fUpdate */);
         QCOMPARE(result.status, CWallet::ScanResult::SUCCESS);
         QCOMPARE(result.last_scanned_block, ::ChainActive().Tip()->GetBlockHash());
         QVERIFY(result.last_failed_block.IsNull());

src/sync.h

@@ -210,7 +210,7 @@ class SCOPED_LOCKABLE UniqueLock : public Base
      friend class reverse_lock;
 };
 
-#define REVERSE_LOCK(g) decltype(g)::reverse_lock PASTE2(revlock, __COUNTER__)(g, #g, __FILE__, __LINE__)
+#define REVERSE_LOCK(g) typename std::decay<decltype(g)>::type::reverse_lock PASTE2(revlock, __COUNTER__)(g, #g, __FILE__, __LINE__)
 
 template<typename MutexArg>
 using DebugLock = UniqueLock<typename std::remove_reference<typename std::remove_pointer<MutexArg>::type>::type>;