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

876e92b Testing: listsinceblock should display all transactions that were affected since the given block, including transactions that were removed due to a reorg. (Karl-Johan Alm)
f999c46 listsinceblock: optionally find and list any transactions that were undone due to reorg when requesting a non-main chain block in a new 'removed' array. (Karl-Johan Alm)

Pull request description:

  The following scenario will not notify the caller of the fact `tx0` has been dropped:

  1. User 1 receives BTC in tx0 from utxo1 in block aa1.
  2. User 2 receives BTC in tx1 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 tx0 is now invalidated.

  See `listsinceblock.py` commit for related test.

  The proposed fix is to iterate from the given block down to the fork point, and to check each transaction in the blocks against the wallet, in addition to including all transactions from the fork point to the active chain tip (the current behavior). Any transactions that were present will now also be listed in the `listsinceblock` output in a new `replaced` array. This operation may be a bit heavy but the circumstances (and perceived frequency of occurrence) warrant it, I believe.

  Example output:
  ```Python
  {
    'transactions': [],
    'replaced': [
      {
        'walletconflicts': [],
        'vout': 1,
        'account': '',
        'timereceived': 1485234857,
        'time': 1485234857,
        'amount': '1.00000000',
        'bip125-replaceable': 'unknown',
        'trusted': False,
        'category': 'receive',
        'txid': 'ce673859a30dee1d2ebdb3c05f2eea7b1da54baf68f93bb8bfe37c5f09ed22ff',
        'address': 'miqEt4kWp9zSizwGGuUWLAmxEcTW9bFUnQ',
        'label': '',
        'confirmations': -7
      }
    ],
    'lastblock': '7a388f27d09e3699102a4ebf81597d974fc4c72093eeaa02adffbbf7527f6715'
  }
  ```

  I believe this addresses the comment by @luke-jr in bitcoin/bitcoin#9516 (comment) but I could be wrong..

Tree-SHA512: 607b5dcaeccb9dc0d963d3de138c40490f3e923050b29821e6bd513d26beb587bddc748fbb194503fe618cfe34a6ed65d95e8d9c5764a882b6c5f976520cff35

Author: laanwj

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;
@@ -3117,7 +3155,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",             "listwallets",              &listwallets,              true,   {} },

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()