diff --git a/doc/release-notes.md b/doc/release-notes.md
--- a/doc/release-notes.md
+++ b/doc/release-notes.md
@@ -3,3 +3,6 @@
This release includes the following features and fixes:
+ - Added parameter `include_removed` to `listsinceblock` for better tracking of
+ transactions during a reorg. See `bitcoin-cli help listsinceblock` for more
+ details.
diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp
--- a/src/rpc/client.cpp
+++ b/src/rpc/client.cpp
@@ -69,6 +69,7 @@
{"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"},
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@@ -1710,6 +1710,17 @@
}
}
+/**
+ * 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) {
@@ -2112,19 +2123,30 @@
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"
+ "include_watchonly include_removed )\n"
"\nGet all transactions in blocks since block [blockhash], or all "
- "transactions if omitted\n"
+ "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"
+ "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')"
+ "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"
@@ -2180,6 +2202,13 @@
" \"to\": \"...\", (string) If a comment to is "
"associated with the transaction.\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"
@@ -2201,7 +2230,11 @@
LOCK2(cs_main, pwallet->cs_wallet);
+ // Block index of the specified block or the common ancestor, if the block
+ // provided was in a deactivated chain.
const CBlockIndex *pindex = nullptr;
+ // Block index of the specified block, even if it's in a deactivated chain.
+ const CBlockIndex *paltindex = nullptr;
int target_confirms = 1;
isminefilter filter = ISMINE_SPENDABLE;
@@ -2211,7 +2244,7 @@
blockId.SetHex(request.params[0].get_str());
BlockMap::iterator it = mapBlockIndex.find(blockId);
if (it != mapBlockIndex.end()) {
- pindex = it->second;
+ 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
@@ -2229,10 +2262,13 @@
}
}
- 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);
@@ -2245,12 +2281,36 @@
}
}
+ // 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, config)) {
+ throw JSONRPCError(RPC_INTERNAL_ERROR,
+ "Can't read block from disk");
+ }
+ for (const CTransactionRef &tx : block.vtx) {
+ if (pwallet->mapWallet.count(tx->GetId()) > 0) {
+ // We want all transactions regardless of confirmation count to
+ // appear here, even negative confirmation ones, hence the big
+ // negative.
+ ListTransactions(pwallet, pwallet->mapWallet.at(tx->GetId()),
+ "*", -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.pushKV("transactions", transactions);
+ if (include_removed) {
+ ret.pushKV("removed", removed);
+ }
ret.pushKV("lastblock", lastblock.GetHex());
return ret;
@@ -3875,7 +3935,7 @@
{ "wallet", "listreceivedbylabel", listreceivedbylabel, {"minconf","include_empty","include_watchonly"} },
{ "wallet", "listreceivedbyaccount", listreceivedbylabel, {"minconf","include_empty","include_watchonly"} },
{ "wallet", "listreceivedbyaddress", listreceivedbyaddress, {"minconf","include_empty","include_watchonly","address_filter"} },
- { "wallet", "listsinceblock", listsinceblock, {"blockhash","target_confirmations","include_watchonly"} },
+ { "wallet", "listsinceblock", listsinceblock, {"blockhash","target_confirmations","include_watchonly","include_removed"} },
{ "wallet", "listtransactions", listtransactions, {"account","count","skip","include_watchonly"} },
{ "wallet", "listunspent", listunspent, {"minconf","maxconf","addresses","include_unsafe","query_options"} },
{ "wallet", "listwallets", listwallets, {} },
diff --git a/test/functional/wallet_listsinceblock.py b/test/functional/wallet_listsinceblock.py
--- a/test/functional/wallet_listsinceblock.py
+++ b/test/functional/wallet_listsinceblock.py
@@ -14,6 +14,14 @@
self.extra_args = [["-noparkdeepreorg"], ["-noparkdeepreorg"], [], []]
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:
@@ -42,14 +50,6 @@
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,179 @@
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 BCH in tx1 from utxo1 in block aa1.
+ 2. User 2 receives BCH 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}'.format(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}'.format(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__':