From f7ba881bc669451a60fedac58a449794702a3e23 Mon Sep 17 00:00:00 2001
From: Russell Yanofsky <russ@yanofsky.org>
Date: Tue, 21 Jan 2020 15:00:33 -0500
Subject: [PATCH] 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
https://github.com/bitcoin/bitcoin/issues/14338#issuecomment-426706574
---
 src/interfaces/chain.cpp      | 18 +++++++++++++++++
 src/interfaces/chain.h        | 12 ++++++++++++
 src/test/interfaces_tests.cpp | 37 +++++++++++++++++++++++++++++++++++
 src/wallet/rpcwallet.cpp      | 12 ++++++------
 4 files changed, 73 insertions(+), 6 deletions(-)

diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp
index dbc49eca1b..8cd4ab0b6d 100644
--- a/src/interfaces/chain.cpp
+++ b/src/interfaces/chain.cpp
@@ -260,6 +260,16 @@ public:
         WAIT_LOCK(cs_main, lock);
         return FillBlock(LookupBlockIndex(hash), block, 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);
+            }
+        }
+        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);
@@ -268,6 +278,14 @@ public:
         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
     {
diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h
index 4c711a618d..ee79b3e6dc 100644
--- a/src/interfaces/chain.h
+++ b/src/interfaces/chain.h
@@ -146,12 +146,24 @@ public:
     //! or contents.
     virtual bool findBlock(const uint256& hash, const FoundBlock& block={}) = 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
     //! populates the values.
diff --git a/src/test/interfaces_tests.cpp b/src/test/interfaces_tests.cpp
index caa988df0d..f95743ec40 100644
--- a/src/test/interfaces_tests.cpp
+++ b/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>
 
@@ -44,6 +47,16 @@ BOOST_AUTO_TEST_CASE(findBlock)
     BOOST_CHECK(!chain->findBlock({}, FoundBlock()));
 }
 
+BOOST_AUTO_TEST_CASE(findAncestorByHeight)
+{
+    auto chain = interfaces::MakeChain(m_node);
+    auto& active = ChainActive();
+    uint256 hash;
+    BOOST_CHECK(chain->findAncestorByHeight(active[20]->GetBlockHash(), 10, FoundBlock().hash(hash)));
+    BOOST_CHECK_EQUAL(hash, active[10]->GetBlockHash());
+    BOOST_CHECK(!chain->findAncestorByHeight(active[10]->GetBlockHash(), 20));
+}
+
 BOOST_AUTO_TEST_CASE(findAncestorByHash)
 {
     auto chain = interfaces::MakeChain(m_node);
@@ -54,4 +67,28 @@ 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;
+    int orig_height;
+    BOOST_CHECK(chain->findCommonAncestor(orig_tip->GetBlockHash(), active.Tip()->GetBlockHash(), FoundBlock().height(fork_height).hash(fork_hash), FoundBlock().height(orig_height)));
+    BOOST_CHECK_EQUAL(orig_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()
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index 1e6d37c462..14e2066c90 100644
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@@ -1581,8 +1581,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.emplace();
+        if (!pwallet->chain().findCommonAncestor(blockId, pwallet->GetLastBlockHash(), /* ancestor out */ FoundBlock().height(*height), /* blockId out */ FoundBlock().height(*altheight))) {
             throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found");
         }
     }
@@ -1601,8 +1602,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);
 
@@ -1634,8 +1634,8 @@ 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;
+    CHECK_NONFATAL(pwallet->chain().findAncestorByHeight(pwallet->GetLastBlockHash(), pwallet->GetLastBlockHeight() + 1 - target_confirms, FoundBlock().hash(lastblock)));
 
     UniValue ret(UniValue::VOBJ);
     ret.pushKV("transactions", transactions);