sikkienl/dash
1
0
Fork 0
mirror of https://github.com/dashpay/dash.git synced 2024-12-25 20:12:57 +01:00
Code Issues Actions Packages Projects Releases Wiki Activity

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

Browse Source 
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 https://github.com/bitcoin/bitcoin/pull/9516#issuecomment-274190081 but I could be wrong..

Tree-SHA512: 607b5dcaeccb9dc0d963d3de138c40490f3e923050b29821e6bd513d26beb587bddc748fbb194503fe618cfe34a6ed65d95e8d9c5764a882b6c5f976520cff35
This commit is contained in:
Wladimir J. van der Laan 2017-07-24 12:56:56 +02:00 committed by Pasta
parent d10e4e769e
commit f393729498
No known key found for this signature in database
GPG Key ID: D362C9F7142766AE
3 changed files with 240 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
src/rpc/client.cpp
View File

@ -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" },

80
src/wallet/rpcwallet.cpp
View File

@ -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"
" <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"
@ -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<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];
@ -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, {} },

190
test/functional/listsinceblock.py
View File

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