[rpc] listsinceblock should include lost transactions when parameter is a reorg'd block

src/rpc/client.cpp

@@ -68,6 +68,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
     { "getblocktemplate", 0, "template_request" },
     { "listsinceblock", 1, "target_confirmations" },
     { "listsinceblock", 2, "include_watchonly" },
+    { "listsinceblock", 3, "include_removed" },
     { "sendmany", 1, "amounts" },
     { "sendmany", 2, "minconf" },
     { "sendmany", 4, "subtractfeefrom" },

src/wallet/rpcwallet.cpp

@@ -1426,6 +1426,17 @@ static void MaybePushAddress(UniValue & entry, const CTxDestination &dest)
         entry.push_back(Pair("address", addr.ToString()));
 }
 
+/**
+ * List transactions based on the given criteria.
+ *
+ * @param  pwallet    The wallet.
+ * @param  wtx        The wallet transaction.
+ * @param  strAccount The account, if any, or "*" for all.
+ * @param  nMinDepth  The minimum confirmation depth.
+ * @param  fLong      Whether to include the JSON version of the transaction.
+ * @param  ret        The UniValue into which the result is stored.
+ * @param  filter     The "is mine" filter bool.
+ */
 void ListTransactions(CWallet* const pwallet, const CWalletTx& wtx, const std::string& strAccount, int nMinDepth, bool fLong, UniValue& ret, const isminefilter& filter)
 {
     CAmount nFee;
@@ -1742,14 +1753,18 @@ UniValue listsinceblock(const JSONRPCRequest& request)
         return NullUniValue;
     }
 
-    if (request.fHelp || request.params.size() > 3)
+    if (request.fHelp || request.params.size() > 4)
         throw std::runtime_error(
-            "listsinceblock ( \"blockhash\" target_confirmations include_watchonly)\n"
-            "\nGet all transactions in blocks since block [blockhash], or all transactions if omitted\n"
+            "listsinceblock ( \"blockhash\" target_confirmations include_watchonly include_removed )\n"
+            "\nGet all transactions in blocks since block [blockhash], or all transactions if omitted.\n"
+            "If \"blockhash\" is no longer a part of the main chain, transactions from the fork point onward are included.\n"
+            "Additionally, if include_removed is set, transactions affecting the wallet which were removed are returned in the \"removed\" array.\n"
             "\nArguments:\n"
             "1. \"blockhash\"            (string, optional) The block hash to list transactions since\n"
-            "2. target_confirmations:    (numeric, optional) The confirmations required, must be 1 or more\n"
-            "3. include_watchonly:       (bool, optional, default=false) Include transactions to watch-only addresses (see 'importaddress')"
+            "2. target_confirmations:    (numeric, optional, default=1) The confirmations required, must be 1 or more\n"
+            "3. include_watchonly:       (bool, optional, default=false) Include transactions to watch-only addresses (see 'importaddress')\n"
+            "4. include_removed:         (bool, optional, default=true) Show transactions that were removed due to a reorg in the \"removed\" array\n"
+            "                                                           (not guaranteed to work on pruned nodes)\n"
             "\nResult:\n"
             "{\n"
             "  \"transactions\": [\n"
@@ -1774,7 +1789,11 @@ UniValue listsinceblock(const JSONRPCRequest& request)
             "    \"comment\": \"...\",       (string) If a comment is associated with the transaction.\n"
             "    \"label\" : \"label\"       (string) A comment for the address/transaction, if any\n"
             "    \"to\": \"...\",            (string) If a comment to is associated with the transaction.\n"
-             "  ],\n"
+            "  ],\n"
+            "  \"removed\": [\n"
+            "    <structure is the same as \"transactions\" above, only present if include_removed=true>\n"
+            "    Note: transactions that were readded in the active chain will appear as-is in this array, and may thus have a positive confirmation count.\n"
+            "  ],\n"
             "  \"lastblock\": \"lastblockhash\"     (string) The hash of the last block\n"
             "}\n"
             "\nExamples:\n"
@@ -1785,21 +1804,19 @@ UniValue listsinceblock(const JSONRPCRequest& request)
 
     LOCK2(cs_main, pwallet->cs_wallet);
 
-    const CBlockIndex *pindex = NULL;
+    const CBlockIndex* pindex = NULL;    // Block index of the specified block or the common ancestor, if the block provided was in a deactivated chain.
+    const CBlockIndex* paltindex = NULL; // Block index of the specified block, even if it's in a deactivated chain.
     int target_confirms = 1;
     isminefilter filter = ISMINE_SPENDABLE;
 
-    if (!request.params[0].isNull())
-    {
+    if (!request.params[0].isNull()) {
         uint256 blockId;
 
         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)
-            {
+        if (it != mapBlockIndex.end()) {
+            paltindex = 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
@@ -1808,35 +1825,56 @@ UniValue listsinceblock(const JSONRPCRequest& request)
         }
     }
 
-    if (!request.params[1].isNull())
-    {
+    if (!request.params[1].isNull()) {
         target_confirms = request.params[1].get_int();
 
-        if (target_confirms < 1)
+        if (target_confirms < 1) {
             throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter");
+        }
     }
 
-    if (request.params.size() > 2 && request.params[2].get_bool())
-    {
+    if (!request.params[2].isNull() && request.params[2].get_bool()) {
         filter = filter | ISMINE_WATCH_ONLY;
     }
 
+    bool include_removed = (request.params[3].isNull() || request.params[3].get_bool());
+
     int depth = pindex ? (1 + chainActive.Height() - pindex->nHeight) : -1;
 
     UniValue transactions(UniValue::VARR);
 
     for (const std::pair<uint256, CWalletTx>& pairWtx : pwallet->mapWallet) {
         CWalletTx tx = pairWtx.second;
 
-        if (depth == -1 || tx.GetDepthInMainChain() < depth)
+        if (depth == -1 || tx.GetDepthInMainChain() < depth) {
             ListTransactions(pwallet, tx, "*", 0, true, transactions, filter);
+        }
+    }
+
+    // when a reorg'd block is requested, we also list any relevant transactions
+    // in the blocks of the chain that was detached
+    UniValue removed(UniValue::VARR);
+    while (include_removed && paltindex && paltindex != pindex) {
+        CBlock block;
+        if (!ReadBlockFromDisk(block, paltindex, Params().GetConsensus())) {
+            throw JSONRPCError(RPC_INTERNAL_ERROR, "Can't read block from disk");
+        }
+        for (const CTransactionRef& tx : block.vtx) {
+            if (pwallet->mapWallet.count(tx->GetHash()) > 0) {
+                // We want all transactions regardless of confirmation count to appear here,
+                // even negative confirmation ones, hence the big negative.
+                ListTransactions(pwallet, pwallet->mapWallet[tx->GetHash()], "*", -100000000, true, removed, filter);
+            }
+        }
+        paltindex = paltindex->pprev;
     }
 
     CBlockIndex *pblockLast = chainActive[chainActive.Height() + 1 - target_confirms];
     uint256 lastblock = pblockLast ? pblockLast->GetBlockHash() : uint256();
 
     UniValue ret(UniValue::VOBJ);
     ret.push_back(Pair("transactions", transactions));
+    if (include_removed) ret.push_back(Pair("removed", removed));
     ret.push_back(Pair("lastblock", lastblock.GetHex()));
 
     return ret;
@@ -3082,7 +3120,7 @@ static const CRPCCommand commands[] =
     { "wallet",             "listlockunspent",          &listlockunspent,          false,  {} },
     { "wallet",             "listreceivedbyaccount",    &listreceivedbyaccount,    false,  {"minconf","include_empty","include_watchonly"} },
     { "wallet",             "listreceivedbyaddress",    &listreceivedbyaddress,    false,  {"minconf","include_empty","include_watchonly"} },
-    { "wallet",             "listsinceblock",           &listsinceblock,           false,  {"blockhash","target_confirmations","include_watchonly"} },
+    { "wallet",             "listsinceblock",           &listsinceblock,           false,  {"blockhash","target_confirmations","include_watchonly","include_removed"} },
     { "wallet",             "listtransactions",         &listtransactions,         false,  {"account","count","skip","include_watchonly"} },
     { "wallet",             "listunspent",              &listunspent,              false,  {"minconf","maxconf","addresses","include_unsafe","query_options"} },
     { "wallet",             "lockunspent",              &lockunspent,              true,   {"unlock","transactions"} },

test/functional/listsinceblock.py

@@ -14,7 +14,15 @@ def __init__(self):
         self.setup_clean_chain = True
         self.num_nodes = 4
 
-    def run_test (self):
+    def run_test(self):
+        self.nodes[2].generate(101)
+        self.sync_all()
+
+        self.test_reorg()
+        self.test_double_spend()
+        self.test_double_send()
+
+    def test_reorg(self):
         '''
         `listsinceblock` did not behave correctly when handed a block that was
         no longer in the main chain:
@@ -43,14 +51,6 @@ def run_test (self):
         This test only checks that [tx0] is present.
         '''
 
-        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()
 
@@ -73,7 +73,177 @@ def run_test (self):
             if tx['txid'] == senttx:
                 found = True
                 break
-        assert_equal(found, True)
+        assert found
+
+    def test_double_spend(self):
+        '''
+        This tests the case where the same UTXO is spent twice on two separate
+        blocks as part of a reorg.
+
+             ab0
+          /       \
+        aa1 [tx1]   bb1 [tx2]
+         |           |
+        aa2         bb2
+         |           |
+        aa3         bb3
+                     |
+                    bb4
+
+        Problematic case:
+
+        1. User 1 receives BTC in tx1 from utxo1 in block aa1.
+        2. User 2 receives BTC in tx2 from utxo1 (same) in block bb1
+        3. User 1 sees 2 confirmations at block aa3.
+        4. Reorg into bb chain.
+        5. User 1 asks `listsinceblock aa3` and does not see that tx1 is now
+           invalidated.
+
+        Currently the solution to this is to detect that a reorg'd block is
+        asked for in listsinceblock, and to iterate back over existing blocks up
+        until the fork point, and to include all transactions that relate to the
+        node wallet.
+        '''
+
+        self.sync_all()
+
+        # Split network into two
+        self.split_network()
+
+        # share utxo between nodes[1] and nodes[2]
+        utxos = self.nodes[2].listunspent()
+        utxo = utxos[0]
+        privkey = self.nodes[2].dumpprivkey(utxo['address'])
+        self.nodes[1].importprivkey(privkey)
+
+        # send from nodes[1] using utxo to nodes[0]
+        change = '%.8f' % (float(utxo['amount']) - 1.0003)
+        recipientDict = {
+            self.nodes[0].getnewaddress(): 1,
+            self.nodes[1].getnewaddress(): change,
+        }
+        utxoDicts = [{
+            'txid': utxo['txid'],
+            'vout': utxo['vout'],
+        }]
+        txid1 = self.nodes[1].sendrawtransaction(
+            self.nodes[1].signrawtransaction(
+                self.nodes[1].createrawtransaction(utxoDicts, recipientDict))['hex'])
+
+        # send from nodes[2] using utxo to nodes[3]
+        recipientDict2 = {
+            self.nodes[3].getnewaddress(): 1,
+            self.nodes[2].getnewaddress(): change,
+        }
+        self.nodes[2].sendrawtransaction(
+            self.nodes[2].signrawtransaction(
+                self.nodes[2].createrawtransaction(utxoDicts, recipientDict2))['hex'])
+
+        # generate on both sides
+        lastblockhash = self.nodes[1].generate(3)[2]
+        self.nodes[2].generate(4)
+
+        self.join_network()
+
+        self.sync_all()
+
+        # gettransaction should work for txid1
+        assert self.nodes[0].gettransaction(txid1)['txid'] == txid1, "gettransaction failed to find txid1"
+
+        # listsinceblock(lastblockhash) should now include txid1, as seen from nodes[0]
+        lsbres = self.nodes[0].listsinceblock(lastblockhash)
+        assert any(tx['txid'] == txid1 for tx in lsbres['removed'])
+
+        # but it should not include 'removed' if include_removed=false
+        lsbres2 = self.nodes[0].listsinceblock(blockhash=lastblockhash, include_removed=False)
+        assert 'removed' not in lsbres2
+
+    def test_double_send(self):
+        '''
+        This tests the case where the same transaction is submitted twice on two
+        separate blocks as part of a reorg. The former will vanish and the
+        latter will appear as the true transaction (with confirmations dropping
+        as a result).
+
+             ab0
+          /       \
+        aa1 [tx1]   bb1
+         |           |
+        aa2         bb2
+         |           |
+        aa3         bb3 [tx1]
+                     |
+                    bb4
+
+        Asserted:
+
+        1. tx1 is listed in listsinceblock.
+        2. It is included in 'removed' as it was removed, even though it is now
+           present in a different block.
+        3. It is listed with a confirmations count of 2 (bb3, bb4), not
+           3 (aa1, aa2, aa3).
+        '''
+
+        self.sync_all()
+
+        # Split network into two
+        self.split_network()
+
+        # create and sign a transaction
+        utxos = self.nodes[2].listunspent()
+        utxo = utxos[0]
+        change = '%.8f' % (float(utxo['amount']) - 1.0003)
+        recipientDict = {
+            self.nodes[0].getnewaddress(): 1,
+            self.nodes[2].getnewaddress(): change,
+        }
+        utxoDicts = [{
+            'txid': utxo['txid'],
+            'vout': utxo['vout'],
+        }]
+        signedtxres = self.nodes[2].signrawtransaction(
+                self.nodes[2].createrawtransaction(utxoDicts, recipientDict))
+        assert signedtxres['complete']
+
+        signedtx = signedtxres['hex']
+
+        # send from nodes[1]; this will end up in aa1
+        txid1 = self.nodes[1].sendrawtransaction(signedtx)
+
+        # generate bb1-bb2 on right side
+        self.nodes[2].generate(2)
+
+        # send from nodes[2]; this will end up in bb3
+        txid2 = self.nodes[2].sendrawtransaction(signedtx)
+
+        assert_equal(txid1, txid2)
+
+        # generate on both sides
+        lastblockhash = self.nodes[1].generate(3)[2]
+        self.nodes[2].generate(2)
+
+        self.join_network()
+
+        self.sync_all()
+
+        # gettransaction should work for txid1
+        self.nodes[0].gettransaction(txid1)
+
+        # listsinceblock(lastblockhash) should now include txid1 in transactions
+        # as well as in removed
+        lsbres = self.nodes[0].listsinceblock(lastblockhash)
+        assert any(tx['txid'] == txid1 for tx in lsbres['transactions'])
+        assert any(tx['txid'] == txid1 for tx in lsbres['removed'])
+
+        # find transaction and ensure confirmations is valid
+        for tx in lsbres['transactions']:
+            if tx['txid'] == txid1:
+                assert_equal(tx['confirmations'], 2)
+
+        # the same check for the removed array; confirmations should STILL be 2
+        for tx in lsbres['removed']:
+            if tx['txid'] == txid1:
+                assert_equal(tx['confirmations'], 2)
 
 if __name__ == '__main__':
     ListSinceBlockTest().main()