Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
laanwj merged 2 commits into from
Jul 24, 2017
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f999c46
listsinceblock: optionally find and list any transactions that were u…
kallewoof Jan 24, 2017
876e92b
Testing: listsinceblock should display all transactions that were aff…
kallewoof Jan 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/rpc/client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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" },
Expand Down
80 changes: 59 additions & 21 deletions src/wallet/rpcwallet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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"
Expand All @@ -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"
Copy link
Contributor

@TheBlueMatt TheBlueMatt Jul 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add something to note that transactions which were re-added are included here anyway, and may have, at that point, positive confirmations value in this array?

" <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"
Expand All @@ -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
Expand All @@ -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;
Expand Down Expand Up @@ -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"} },
Expand Down
190 changes: 180 additions & 10 deletions test/functional/listsinceblock.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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()

Expand All @@ -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(
Copy link
Contributor

@jnewbery jnewbery Apr 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to use the sendtoaddress RPC here instead of {create,sign,send}rawtransaction? The rawtransaction RPCs force you to explicitly do things like set change addresses and make assumptions about fee rates, that could break this test in future.

Copy link
Contributor Author

@kallewoof kallewoof Apr 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately sendtoaddress doesn't let me choose the UTXO so I don't think I can use it here.

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']:
Copy link
Contributor

@TheBlueMatt TheBlueMatt Jul 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you duplicate this loop for "removed", noting that the tx listed in "removed" should also have a confirmations of 2.

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