Merge #9516: Bug-fix: listsinceblock: use fork point as reference for blocks in reorg'd chains

7ba0a00 Testing: listsinceblock should not use orphan block height. (Karl-Johan Alm)
ee5c1ce Bug-fix: listsinceblock: use closest common ancestor when a block hash was provided for a chain that was not the main chain. (Karl-Johan Alm)

3 files changed, 95 insertions, 4 deletions

diff --git a/qa/pull-tester/rpc-tests.py b/qa/pull-tester/rpc-tests.py
index c87d3c7127..26bc6a73df 100755
--- a/qa/pull-tester/rpc-tests.py
+++ b/qa/pull-tester/rpc-tests.py
@@ -153,6 +153,7 @@ testScripts = [
     'import-rescan.py',
     'bumpfee.py',
     'rpcnamedargs.py',
+    'listsinceblock.py',
 ]
 if ENABLE_ZMQ:
     testScripts.append('zmq_test.py')
diff --git a/qa/rpc-tests/listsinceblock.py b/qa/rpc-tests/listsinceblock.py
new file mode 100755
index 0000000000..ca67b8eceb
--- /dev/null
+++ b/qa/rpc-tests/listsinceblock.py
@@ -0,0 +1,80 @@
+#!/usr/bin/env python3
+# 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.
+
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import assert_equal
+
+class ListSinceBlockTest (BitcoinTestFramework):
+
+    def __init__(self):
+        super().__init__()
+        self.setup_clean_chain = True
+        self.num_nodes = 4
+
+    def run_test (self):
+        '''
+        `listsinceblock` did not behave correctly when handed a block that was
+        no longer in the main chain:
+
+             ab0
+          /       \
+        aa1 [tx0]   bb1
+         |           |
+        aa2         bb2
+         |           |
+        aa3         bb3
+                     |
+                    bb4
+
+        Consider a client that has only seen block `aa3` above. It asks the node
+        to `listsinceblock aa3`. But at some point prior the main chain switched
+        to the bb chain.
+
+        Previously: listsinceblock would find height=4 for block aa3 and compare
+        this to height=5 for the tip of the chain (bb4). It would then return
+        results restricted to bb3-bb4.
+
+        Now: listsinceblock finds the fork at ab0 and returns results in the
+        range bb1-bb4.
+
+        This test only checks that [tx0] is present.
+        '''
+
+        assert_equal(self.is_network_split, False)
+        self.nodes[2].generate(101)
+        self.sync_all()
+
+        assert_equal(self.nodes[0].getbalance(), 0)
+        assert_equal(self.nodes[1].getbalance(), 0)
+        assert_equal(self.nodes[2].getbalance(), 50)
+        assert_equal(self.nodes[3].getbalance(), 0)
+
+        # Split network into two
+        self.split_network()
+        assert_equal(self.is_network_split, True)
+
+        # send to nodes[0] from nodes[2]
+        senttx = self.nodes[2].sendtoaddress(self.nodes[0].getnewaddress(), 1)
+
+        # generate on both sides
+        lastblockhash = self.nodes[1].generate(6)[5]
+        self.nodes[2].generate(7)
+        print('lastblockhash=%s' % (lastblockhash))
+
+        self.sync_all()
+
+        self.join_network()
+
+        # listsinceblock(lastblockhash) should now include tx, as seen from nodes[0]
+        lsbres = self.nodes[0].listsinceblock(lastblockhash)
+        found = False
+        for tx in lsbres['transactions']:
+            if tx['txid'] == senttx:
+                found = True
+                break
+        assert_equal(found, True)
+
+if __name__ == '__main__':
+    ListSinceBlockTest().main()
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index 55e7558498..6d08f60483 100644
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@@ -1681,7 +1681,7 @@ UniValue listsinceblock(const JSONRPCRequest& request)
 
     LOCK2(cs_main, pwalletMain->cs_wallet);
 
-    CBlockIndex *pindex = NULL;
+    const CBlockIndex *pindex = NULL;
     int target_confirms = 1;
     isminefilter filter = ISMINE_SPENDABLE;
 
@@ -1692,7 +1692,16 @@ UniValue listsinceblock(const JSONRPCRequest& request)
         blockId.SetHex(request.params[0].get_str());
         BlockMap::iterator it = mapBlockIndex.find(blockId);
         if (it != mapBlockIndex.end())
+        {
             pindex = it->second;
+            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.size() > 1)
@@ -1703,9 +1712,10 @@ UniValue listsinceblock(const JSONRPCRequest& request)
             throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter");
     }
 
-    if(request.params.size() > 2)
-        if(request.params[2].get_bool())
-            filter = filter | ISMINE_WATCH_ONLY;
+    if (request.params.size() > 2 && request.params[2].get_bool())
+    {
+        filter = filter | ISMINE_WATCH_ONLY;
+    }
 
     int depth = pindex ? (1 + chainActive.Height() - pindex->nHeight) : -1;