diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index 15d3f70647..a4482bb186 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -85,6 +85,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", 3, "addlocked" }, diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index cece3c442f..60f45ba2c2 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -1361,6 +1361,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; @@ -1681,14 +1692,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" @@ -1714,7 +1729,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" + " \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" @@ -1725,21 +1744,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 @@ -1750,19 +1767,20 @@ UniValue listsinceblock(const JSONRPCRequest& request) throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid blockhash"); } - 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); @@ -1770,8 +1788,27 @@ UniValue listsinceblock(const JSONRPCRequest& request) for (const std::pair& 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]; @@ -1779,6 +1816,7 @@ UniValue listsinceblock(const JSONRPCRequest& request) 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; @@ -3083,7 +3121,7 @@ static const CRPCCommand commands[] = { "wallet", "listlockunspent", &listlockunspent, false, {} }, { "wallet", "listreceivedbyaccount", &listreceivedbyaccount, false, {"minconf","addlocked","include_empty","include_watchonly"} }, { "wallet", "listreceivedbyaddress", &listreceivedbyaddress, false, {"minconf","addlocked","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, {} }, diff --git a/test/functional/listsinceblock.py b/test/functional/listsinceblock.py index a73f72ba33..ce2d556ef0 100755 --- a/test/functional/listsinceblock.py +++ b/test/functional/listsinceblock.py @@ -14,7 +14,15 @@ class ListSinceBlockTest (BitcoinTestFramework): 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 @@ class ListSinceBlockTest (BitcoinTestFramework): 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(), 500) - assert_equal(self.nodes[3].getbalance(), 0) - # Split network into two self.split_network() @@ -73,7 +73,177 @@ class ListSinceBlockTest (BitcoinTestFramework): 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()