wallet: Avoid use of Chain::Lock in listsinceblock

This is a step toward removing the Chain::Lock class and reducing cs_main
locking.

This change only affects behavior in the case where wallet last block processed
falls behind the chain tip. Previously listsinceblock might not have returned
all transactions up to the claimed "lastblock" value in this case, resulting in
race conditions and potentially missing transactions in cases where
listsinceblock was called in a loop like
bitcoin#14338 (comment)

Author: ryanofsky

src/interfaces/chain.cpp

@@ -259,6 +259,15 @@ class ChainImpl : public Chain
         }
         return true;
     }
+    uint256 findAncestorByHeight(const uint256& block_hash, int ancestor_height) override
+    {
+        LOCK(::cs_main);
+        if (const CBlockIndex* block = LookupBlockIndex(block_hash)) {
+            const CBlockIndex* ancestor = block->GetAncestor(ancestor_height);
+            if (ancestor) return ancestor->GetBlockHash();
+        }
+        return {};
+    }
     bool findAncestorByHash(const uint256& block_hash, const uint256& ancestor_hash, int* height) override
     {
         LOCK(::cs_main);
@@ -268,6 +277,22 @@ class ChainImpl : public Chain
         if (height) *height = ancestor->nHeight;
         return true;
     }
+    Optional<int> findCommonAncestor(const uint256& block_hash1,
+        const uint256& block_hash2,
+        uint256* ancestor_hash,
+        int* ancestor_height) override
+    {
+        LOCK(::cs_main);
+        const CBlockIndex* block1 = LookupBlockIndex(block_hash1);
+        const CBlockIndex* block2 = LookupBlockIndex(block_hash2);
+        if (block1 && block2) {
+            const CBlockIndex* ancestor = LastCommonAncestor(block1, block2);
+            if (ancestor_hash) *ancestor_hash = ancestor->GetBlockHash();
+            if (ancestor_height) *ancestor_height = ancestor->nHeight;
+            return block1->nHeight;
+        }
+        return nullopt;
+    }
     void findCoins(std::map<COutPoint, Coin>& coins) override { return FindCoins(m_node, coins); }
     double guessVerificationProgress(const uint256& block_hash) override
     {

src/interfaces/chain.h

@@ -133,12 +133,23 @@ class Chain
         int64_t* max_time = nullptr,
         int64_t* mtp_time = nullptr) = 0;
 
+    //! Find ancestor of block at specified height and return its hash.
+    virtual uint256 findAncestorByHeight(const uint256& block_hash, int ancestor_height) = 0;
+
     //! Return whether block descends from a specified ancestor, and
     //! optionally return height of the ancestor.
     virtual bool findAncestorByHash(const uint256& block_hash,
         const uint256& ancestor_hash,
         int* height = 0) = 0;
 
+    //! Find most recent common ancestor between two blocks and optionally
+    //! return its hash and/or height. Also return height of first block. Return
+    //! nullopt if either block is unknown or there is no common ancestor.
+    virtual Optional<int> findCommonAncestor(const uint256& block_hash1,
+        const uint256& block_hash2,
+        uint256* ancestor_hash,
+        int* ancestor_height) = 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
     //! populates the values.

src/test/interfaces_tests.cpp

@@ -2,7 +2,10 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
+#include <chainparams.h>
+#include <consensus/validation.h>
 #include <interfaces/chain.h>
+#include <script/standard.h>
 #include <test/util/setup_common.h>
 #include <validation.h>
 
@@ -19,6 +22,14 @@ BOOST_AUTO_TEST_CASE(findBlock)
     BOOST_CHECK_EQUAL(time_mtp, active[20]->GetMedianTimePast());
 }
 
+BOOST_AUTO_TEST_CASE(findAncestorByHeight)
+{
+    auto chain = interfaces::MakeChain(m_node);
+    auto& active = ChainActive();
+    BOOST_CHECK_EQUAL(chain->findAncestorByHeight(active[20]->GetBlockHash(), 10), active[10]->GetBlockHash());
+    BOOST_CHECK_EQUAL(chain->findAncestorByHeight(active[10]->GetBlockHash(), 20), uint256());
+}
+
 BOOST_AUTO_TEST_CASE(findAncestorByHash)
 {
     auto chain = interfaces::MakeChain(m_node);
@@ -29,4 +40,26 @@ BOOST_AUTO_TEST_CASE(findAncestorByHash)
     BOOST_CHECK(!chain->findAncestorByHash(active[10]->GetBlockHash(), active[20]->GetBlockHash()));
 }
 
+BOOST_AUTO_TEST_CASE(findCommonAncestor)
+{
+    auto chain = interfaces::MakeChain(m_node);
+    auto& active = ChainActive();
+    auto* orig_tip = active.Tip();
+    for (int i = 0; i < 10; ++i) {
+        BlockValidationState state;
+        ChainstateActive().InvalidateBlock(state, Params(), active.Tip());
+    }
+    BOOST_CHECK_EQUAL(active.Height(), orig_tip->nHeight - 10);
+    coinbaseKey.MakeNewKey(true);
+    for (int i = 0; i < 20; ++i) {
+        CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
+    }
+    BOOST_CHECK_EQUAL(active.Height(), orig_tip->nHeight + 10);
+    uint256 fork_hash;
+    int fork_height;
+    BOOST_CHECK_EQUAL(*chain->findCommonAncestor(orig_tip->GetBlockHash(), active.Tip()->GetBlockHash(), &fork_hash, &fork_height), orig_tip->nHeight);
+    BOOST_CHECK_EQUAL(fork_height, orig_tip->nHeight - 10);
+    BOOST_CHECK_EQUAL(fork_hash, active[fork_height]->GetBlockHash());
+}
+
 BOOST_AUTO_TEST_SUITE_END()

src/wallet/rpcwallet.cpp

@@ -1585,8 +1585,9 @@ static UniValue listsinceblock(const JSONRPCRequest& request)
     uint256 blockId;
     if (!request.params[0].isNull() && !request.params[0].get_str().empty()) {
         blockId = ParseHashV(request.params[0], "blockhash");
-        height = locked_chain->findFork(blockId, &altheight);
-        if (!height) {
+        height.emplace();
+        altheight = pwallet->chain().findCommonAncestor(blockId, pwallet->GetLastBlockHash(), nullptr, height.get_ptr());
+        if (!altheight) {
             throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found");
         }
     }
@@ -1605,8 +1606,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 && height ? (1 + *tip_height - *height) : -1;
+    int depth = height ? pwallet->GetLastBlockHeight() + 1 - *height : -1;
 
     UniValue transactions(UniValue::VARR);
 
@@ -1638,8 +1638,7 @@ static UniValue listsinceblock(const JSONRPCRequest& request)
         --*altheight;
     }
 
-    int last_height = tip_height ? *tip_height + 1 - target_confirms : -1;
-    uint256 lastblock = last_height >= 0 ? locked_chain->getBlockHash(last_height) : uint256();
+    uint256 lastblock = pwallet->chain().findAncestorByHeight(pwallet->GetLastBlockHash(), pwallet->GetLastBlockHeight() + 1 - target_confirms);
 
     UniValue ret(UniValue::VOBJ);
     ret.pushKV("transactions", transactions);