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.

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

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

Merged
merged 2 commits into from Jul 24, 2017

Conversation

Projects
None yet
@kallewoof
Member

kallewoof commented Jan 24, 2017

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:

{
  '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 #9516 (comment) but I could be wrong..

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jan 24, 2017

Member

I'm not 100% sure if we want this. If we, we would need at least to updated the listsinceblock's RPC help message to mention that if one requests tx's from a block outside the main-chain, that the result will also contain transactions not in the main chain.

Member

jonasschnelli commented Jan 24, 2017

I'm not 100% sure if we want this. If we, we would need at least to updated the listsinceblock's RPC help message to mention that if one requests tx's from a block outside the main-chain, that the result will also contain transactions not in the main chain.

@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Jan 24, 2017

Member

@jonasschnelli Updated the help output. I am not sure what the reasons for not wanting this are, unless you're referring to resource consumption. I think it's a rare (but important) enough case to warrant it.

It would definitely make it easier for RPC applications checking the validity of existing transactions to explicitly provide these when a reorg affects them. The only other alternative right now is to keep a list of transactions with confirmations less than some arbitrary number (100) and to loop through these every time a reorg is encountered to ensure they're actually still present.

Edit: one concern of my own is whether a naive implementation would ignore the confirmations value and simply think the transaction existed in the chain, even though the opposite is the case. I wondered if maybe a different key for the returned results should be used, e.g. "reorged" or something. I.e. you would get {"transactions": [list of txs that changed], "reorged": [list of txs that disappeared], "lastblock": <hash>}...

Member

kallewoof commented Jan 24, 2017

@jonasschnelli Updated the help output. I am not sure what the reasons for not wanting this are, unless you're referring to resource consumption. I think it's a rare (but important) enough case to warrant it.

It would definitely make it easier for RPC applications checking the validity of existing transactions to explicitly provide these when a reorg affects them. The only other alternative right now is to keep a list of transactions with confirmations less than some arbitrary number (100) and to loop through these every time a reorg is encountered to ensure they're actually still present.

Edit: one concern of my own is whether a naive implementation would ignore the confirmations value and simply think the transaction existed in the chain, even though the opposite is the case. I wondered if maybe a different key for the returned results should be used, e.g. "reorged" or something. I.e. you would get {"transactions": [list of txs that changed], "reorged": [list of txs that disappeared], "lastblock": <hash>}...

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jan 24, 2017

Member

@kallewoof: I missed the point that if you pass in an orphan block to list since, you also get the transactions upwards the chain-fork on the main chain. At first sight, I though you get only tx from the re-orged-off chain in that case.

Concept ACK (and I think it would be clever to list them in an extra array element reorged:[].

Member

jonasschnelli commented Jan 24, 2017

@kallewoof: I missed the point that if you pass in an orphan block to list since, you also get the transactions upwards the chain-fork on the main chain. At first sight, I though you get only tx from the re-orged-off chain in that case.

Concept ACK (and I think it would be clever to list them in an extra array element reorged:[].

@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Jan 24, 2017

Member

@jonasschnelli Gotcha. I updated the OP to clarify that it's also including transactions from the fork point to the active chain tip. I also moved the off chain transactions into a new 'reorged' array. (f501acc & 461d5a3)

Member

kallewoof commented Jan 24, 2017

@jonasschnelli Gotcha. I updated the OP to clarify that it's also including transactions from the fork point to the active chain tip. I also moved the off chain transactions into a new 'reorged' array. (f501acc & 461d5a3)

@ryanofsky

utACK 461d5a3 with minor suggestions

Show outdated Hide outdated src/wallet/rpcwallet.cpp
{
if (pwalletMain->mapWallet.count(tx->GetHash()))
{
ListTransactions(pwalletMain->mapWallet[tx->GetHash()], "*", -depth, true, transactions, filter);

This comment has been minimized.

@ryanofsky

ryanofsky Jan 24, 2017

Contributor

I think it would be helpful to add a comment about -depth here. I was staring at this a long time to figure out how it worked. Comment could say: "Pass -depth as minDepth to prevent any filtering in ListTransactions. (Works because tx can only conflict with transactions after pindex, so GetDepthInMainChain will always return at least (1-depth))."

@ryanofsky

ryanofsky Jan 24, 2017

Contributor

I think it would be helpful to add a comment about -depth here. I was staring at this a long time to figure out how it worked. Comment could say: "Pass -depth as minDepth to prevent any filtering in ListTransactions. (Works because tx can only conflict with transactions after pindex, so GetDepthInMainChain will always return at least (1-depth))."

Show outdated Hide outdated src/wallet/rpcwallet.cpp
@@ -1641,7 +1641,9 @@ UniValue listsinceblock(const JSONRPCRequest& request)
if (request.fHelp)
throw runtime_error(
"listsinceblock ( \"blockhash\" target_confirmations include_watchonly)\n"
"\nGet all transactions in blocks since block [blockhash], or all transactions if omitted\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, all transactions affecting the node wallet back to the fork point are included, as well as those from \n"

This comment has been minimized.

@ryanofsky

ryanofsky Jan 24, 2017

Contributor

Would s/back to the fork point/from blockhash back to the fork point/ to clarify, because it sounds to me like this is referring to transactions between the active tip and the fork point (making the rest of the sentence confusing).

@ryanofsky

ryanofsky Jan 24, 2017

Contributor

Would s/back to the fork point/from blockhash back to the fork point/ to clarify, because it sounds to me like this is referring to transactions between the active tip and the fork point (making the rest of the sentence confusing).

Show outdated Hide outdated src/wallet/rpcwallet.cpp
@@ -1756,6 +1757,7 @@ UniValue listsinceblock(const JSONRPCRequest& request)
UniValue ret(UniValue::VOBJ);
ret.push_back(Pair("transactions", transactions));
ret.push_back(Pair("reorged", reorged));

This comment has been minimized.

@ryanofsky

ryanofsky Jan 24, 2017

Contributor

Need documentation update to accompany this.

@ryanofsky

ryanofsky Jan 24, 2017

Contributor

Need documentation update to accompany this.

Show outdated Hide outdated qa/rpc-tests/listsinceblock.py
privkey = self.nodes[2].dumpprivkey(utxo['address'])
self.nodes[1].importprivkey(privkey)
# send from nodes[2] using utxo to nodes[0]

This comment has been minimized.

@ryanofsky

ryanofsky Jan 24, 2017

Contributor

Really sending from node 1 here (even if key originally comes from node 2)

@ryanofsky

ryanofsky Jan 24, 2017

Contributor

Really sending from node 1 here (even if key originally comes from node 2)

Show outdated Hide outdated qa/rpc-tests/listsinceblock.py
self.join_network()
# gettransaction should work for txid1
tmp = self.nodes[0].gettransaction(txid1)

This comment has been minimized.

@ryanofsky

ryanofsky Jan 24, 2017

Contributor

unused variable

@ryanofsky

ryanofsky Jan 24, 2017

Contributor

unused variable

Show outdated Hide outdated qa/rpc-tests/listsinceblock.py
# listsinceblock(lastblockhash) should now include txid1, as seen from nodes[0]
lsbres = self.nodes[0].listsinceblock(lastblockhash)
found = False

This comment has been minimized.

@ryanofsky

ryanofsky Jan 24, 2017

Contributor

Maybe condense this block to a single line, and add a check for txid2 (untested)

assert_equal(any(tx['txid'] == txid1 for tx in lsbres['reorged']), True)
assert_equal(any(tx['txid'] == txid2 for tx in lsbres['transactions']), True)
@ryanofsky

ryanofsky Jan 24, 2017

Contributor

Maybe condense this block to a single line, and add a check for txid2 (untested)

assert_equal(any(tx['txid'] == txid1 for tx in lsbres['reorged']), True)
assert_equal(any(tx['txid'] == txid2 for tx in lsbres['transactions']), True)

This comment has been minimized.

@kallewoof

kallewoof Jan 25, 2017

Member

@ryanofsky Ahh, nice! I didn't know about any().

Since txid2 is not related to nodes[0], it will not list it anywhere so that second assert_equal will not be true.

Thanks for all the feedback! I believe everything you suggested is in 9caa0ec & 131df5a.

@kallewoof

kallewoof Jan 25, 2017

Member

@ryanofsky Ahh, nice! I didn't know about any().

Since txid2 is not related to nodes[0], it will not list it anywhere so that second assert_equal will not be true.

Thanks for all the feedback! I believe everything you suggested is in 9caa0ec & 131df5a.

@MarcoFalke MarcoFalke added this to the 0.14.0 milestone Jan 25, 2017

Show outdated Hide outdated qa/rpc-tests/listsinceblock.py
ab0
/ \
aa1 [tx0] bb1 [tx1]

This comment has been minimized.

@ryanofsky

ryanofsky Jan 25, 2017

Contributor

Maybe change tx0 and tx1 to tx1 and tx2 to match code. Also code is generating 6 and 7 blocks instead of 3 and 4, so maybe that could be changed to match the example.

@ryanofsky

ryanofsky Jan 25, 2017

Contributor

Maybe change tx0 and tx1 to tx1 and tx2 to match code. Also code is generating 6 and 7 blocks instead of 3 and 4, so maybe that could be changed to match the example.

Show outdated Hide outdated src/wallet/rpcwallet.cpp
if (pwalletMain->mapWallet.count(tx->GetHash()))
{
// Use -depth as minDepth parameter to ListTransactions, as these transactions have
// negative (at minimum -depth) confirmations.

This comment has been minimized.

@ryanofsky

ryanofsky Jan 25, 2017

Contributor

Would remove parentheses since (at minimum -depth) is the important part. Also, technically I believe the minimum is 1-depth. Maybe:

// Use -depth as minDepth parameter to ListTransactions to prevent incoming
// transactions from being filtered. These transactions have negative
// confirmations, but always greater than -depth.
@ryanofsky

ryanofsky Jan 25, 2017

Contributor

Would remove parentheses since (at minimum -depth) is the important part. Also, technically I believe the minimum is 1-depth. Maybe:

// Use -depth as minDepth parameter to ListTransactions to prevent incoming
// transactions from being filtered. These transactions have negative
// confirmations, but always greater than -depth.
@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Jan 25, 2017

Member

@ryanofsky Thanks for all the feedback! Updated.

Member

kallewoof commented Jan 25, 2017

@ryanofsky Thanks for all the feedback! Updated.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jan 26, 2017

Member

Concept ACK.

Member

gmaxwell commented Jan 26, 2017

Concept ACK.

@TheBlueMatt

I'm not sure I'd call this a bugfix - we already correctly print transactions which were included in blocks from the common fork point from the blockhash provided to listsinceblock.

Show outdated Hide outdated src/wallet/rpcwallet.cpp
// Use -depth as minDepth parameter to ListTransactions to prevent incoming
// transactions from being filtered. These transactions have negative
// confirmations, but always greater than -depth.
ListTransactions(pwalletMain->mapWallet[tx->GetHash()], "*", -depth, true, reorged, filter);

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Jan 28, 2017

Contributor

I dont believe this prevents listing transactions which were on both sides of the fork? I'm pretty sure we dont want to list such transactions in a "reorged" list.

@TheBlueMatt

TheBlueMatt Jan 28, 2017

Contributor

I dont believe this prevents listing transactions which were on both sides of the fork? I'm pretty sure we dont want to list such transactions in a "reorged" list.

@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Jan 30, 2017

Member

@TheBlueMatt I guess it comes down to what the user expects. I personally expected listsinceblock to show me anything I needed to keep an updated score of what's going on since the given block. In that sense, not showing a transaction vanishing would probably be considered a bug.

Regarding showing an existing transaction in both reorged and transactions, I patched this by ensuring that any tx shown from the main chain is skipped over in the reorged list. See the new test_double_send test.

Member

kallewoof commented Jan 30, 2017

@TheBlueMatt I guess it comes down to what the user expects. I personally expected listsinceblock to show me anything I needed to keep an updated score of what's going on since the given block. In that sense, not showing a transaction vanishing would probably be considered a bug.

Regarding showing an existing transaction in both reorged and transactions, I patched this by ensuring that any tx shown from the main chain is skipped over in the reorged list. See the new test_double_send test.

@JeremyRubin

This comment has been minimized.

Show comment
Hide comment
@JeremyRubin

JeremyRubin Jan 30, 2017

Contributor

I agree with Matt, this isn't a bugfix, it's feature :)

I think I'm in favor of adding this. I would be more supportive of naming them replaced rather than reorged if I understand the semantics properly. Strictly speaking, I'd say a reorged txn is any transaction that was between the old tip and the fork point?

I would also think maybe making this information available with a default false parameter may be a good idea to introduce less changes for current users/legacy code.

Contributor

JeremyRubin commented Jan 30, 2017

I agree with Matt, this isn't a bugfix, it's feature :)

I think I'm in favor of adding this. I would be more supportive of naming them replaced rather than reorged if I understand the semantics properly. Strictly speaking, I'd say a reorged txn is any transaction that was between the old tip and the fork point?

I would also think maybe making this information available with a default false parameter may be a good idea to introduce less changes for current users/legacy code.

@kallewoof kallewoof changed the title from [rpc] Bug-fix: listsinceblock should include lost transactions when parameter is a reorg'd block to [rpc] listsinceblock should include lost transactions when parameter is a reorg'd block Jan 30, 2017

@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Jan 30, 2017

Member

@JeremyRubin Changed as you said, except setting default of include_replaced to true as I think that will be the most useful case.

Member

kallewoof commented Jan 30, 2017

@JeremyRubin Changed as you said, except setting default of include_replaced to true as I think that will be the most useful case.

@laanwj laanwj modified the milestones: 0.15.0, 0.14.0 Jan 30, 2017

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jan 30, 2017

Member

Moving milestone to 0.15 as this was re-classified as a feature and the feature freeze is long past.

Member

laanwj commented Jan 30, 2017

Moving milestone to 0.15 as this was re-classified as a feature and the feature freeze is long past.

@luke-jr

luke-jr approved these changes Feb 2, 2017

utACK as-is, with a few nits. (IMO this is a bug fix.)

Show outdated Hide outdated src/wallet/rpcwallet.cpp
"\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')"
"4. include_replaced: (bool, optional, default=true) Show transactions that were replaced due to a reorg in the \"replaced\" array"

This comment has been minimized.

@luke-jr

luke-jr Feb 2, 2017

Member

Not sure this needs to be optional...?

@luke-jr

luke-jr Feb 2, 2017

Member

Not sure this needs to be optional...?

This comment has been minimized.

@kallewoof

kallewoof Feb 2, 2017

Member

Me neither. It was mostly suggested as a way for legacy code to cope better, but only really crappy legacy code (that would break when a new key was added to an existing dictionary) would be affected I guess.

@kallewoof

kallewoof Feb 2, 2017

Member

Me neither. It was mostly suggested as a way for legacy code to cope better, but only really crappy legacy code (that would break when a new key was added to an existing dictionary) would be affected I guess.

Show outdated Hide outdated src/wallet/rpcwallet.cpp
for (map<uint256, CWalletTx>::iterator it = pwalletMain->mapWallet.begin(); it != pwalletMain->mapWallet.end(); it++)
{
CWalletTx tx = (*it).second;
if (depth == -1 || tx.GetDepthInMainChain() < depth)
ListTransactions(tx, "*", 0, true, transactions, filter);
{
if (ListTransactions(tx, "*", 0, true, transactions, filter) && include_replaced)

This comment has been minimized.

@luke-jr

luke-jr Feb 2, 2017

Member

Why do we need ListTransactions's return value? If it doesn't get inserted here, it won't later-on either, right?

@luke-jr

luke-jr Feb 2, 2017

Member

Why do we need ListTransactions's return value? If it doesn't get inserted here, it won't later-on either, right?

This comment has been minimized.

@kallewoof

kallewoof Feb 2, 2017

Member

See above case with a tx being sent on both chains.

@kallewoof

kallewoof Feb 2, 2017

Member

See above case with a tx being sent on both chains.

Show outdated Hide outdated src/wallet/rpcwallet.cpp
* @param filter The "is mine" filter bool.
* @return true if appends were made to ret, false otherwise.
*/
bool ListTransactions(const CWalletTx& wtx, const string& strAccount, int nMinDepth, bool fLong, UniValue& ret, const isminefilter& filter)

This comment has been minimized.

@luke-jr

luke-jr Feb 2, 2017

Member

IMO this PR doesn't need to modify ListTransactions, and this ought to be move to a separate PR.

@luke-jr

luke-jr Feb 2, 2017

Member

IMO this PR doesn't need to modify ListTransactions, and this ought to be move to a separate PR.

This comment has been minimized.

@kallewoof

kallewoof Feb 2, 2017

Member

For the case where tx0 is in reorged and main chain both (see test_double_send), both versions of the transaction will appear in the results (once in 'transactions' and once in 'replaced'). To prevent that, I check if ListTransactions appended to the transactions array -- if it did, I need to not include in replaced.

@kallewoof

kallewoof Feb 2, 2017

Member

For the case where tx0 is in reorged and main chain both (see test_double_send), both versions of the transaction will appear in the results (once in 'transactions' and once in 'replaced'). To prevent that, I check if ListTransactions appended to the transactions array -- if it did, I need to not include in replaced.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Feb 10, 2017

Member

It occurs to me that the key "replaced" may not be ideal, since there is no guarantee these transactions are now conflicted and won't get re-mined. Perhaps "removed"?

Member

luke-jr commented Feb 10, 2017

It occurs to me that the key "replaced" may not be ideal, since there is no guarantee these transactions are now conflicted and won't get re-mined. Perhaps "removed"?

@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Feb 10, 2017

Member

I tried to find actual cases where this happened, but had a hard time finding a case that wasn't the cause of a double spend. It would require that the transaction is NOT in the mempool of the node, or it would appear as normal in transactions. There may be a case I haven't thought of though, so you may be right, removed is a safer bet for sure.

Member

kallewoof commented Feb 10, 2017

I tried to find actual cases where this happened, but had a hard time finding a case that wasn't the cause of a double spend. It would require that the transaction is NOT in the mempool of the node, or it would appear as normal in transactions. There may be a case I haven't thought of though, so you may be right, removed is a safer bet for sure.

@luke-jr

Two more "replaced" instances. Also need to update the named param list at the bottom of the file, and in rpc/client.cpp

Show outdated Hide outdated src/wallet/rpcwallet.cpp
@@ -1654,12 +1671,15 @@ UniValue listsinceblock(const JSONRPCRequest& request)
if (request.fHelp)
throw 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_replaced )\n"

This comment has been minimized.

@luke-jr

luke-jr Feb 10, 2017

Member

"replaced"

@luke-jr

luke-jr Feb 10, 2017

Member

"replaced"

This comment has been minimized.

@kallewoof

kallewoof Feb 10, 2017

Member

Done

Show outdated Hide outdated src/wallet/rpcwallet.cpp
"\nGet all transactions in blocks since block [blockhash], or all transactions if omitted\n"
"listsinceblock ( \"blockhash\" target_confirmations include_watchonly include_replaced )\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, all transactions affecting the node wallet from blockhash back to the fork point are included in the \"replaced\" array.\n"

This comment has been minimized.

@luke-jr

luke-jr Feb 10, 2017

Member

"replaced"

@luke-jr

luke-jr Feb 10, 2017

Member

"replaced"

This comment has been minimized.

@kallewoof

kallewoof Feb 10, 2017

Member

Done

@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Feb 10, 2017

Member

@luke-jr Sorry about sloppiness, I thought I got all of them in the initial two squashme's.

Member

kallewoof commented Feb 10, 2017

@luke-jr Sorry about sloppiness, I thought I got all of them in the initial two squashme's.

@ryanofsky

This comment has been minimized.

Show comment
Hide comment
@ryanofsky

ryanofsky Feb 23, 2017

Contributor

utACK ad57cef.

Comparing against previously ACKed 461d5a3, the both-sides dedup code and the new API tweaks and comments looked good.

Contributor

ryanofsky commented Feb 23, 2017

utACK ad57cef.

Comparing against previously ACKed 461d5a3, the both-sides dedup code and the new API tweaks and comments looked good.

@TheBlueMatt

utACK ad57cef +/- some slight API nitpicks.

Show outdated Hide outdated src/wallet/rpcwallet.cpp
"\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, all transactions affecting the node wallet from blockhash back to the fork point are included in the \"removed\" array.\n"

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Feb 24, 2017

Contributor

Maybe instead of this and the next line,
"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"

@TheBlueMatt

TheBlueMatt Feb 24, 2017

Contributor

Maybe instead of this and the next line,
"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"

This comment has been minimized.

@kallewoof

kallewoof Feb 24, 2017

Member

Makes sense. Changed.

@kallewoof

kallewoof Feb 24, 2017

Member

Makes sense. Changed.

Show outdated Hide outdated src/wallet/rpcwallet.cpp
CBlock block;
if (!ReadBlockFromDisk(block, paltindex, Params().GetConsensus()))
{
throw JSONRPCError(RPC_INTERNAL_ERROR, "Can't read block from disk");

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Feb 24, 2017

Contributor

Hmm, not sure how I feel about this. For pruning nodes, it may be more useful to return a partial list and a message indicating the results are incomplete rather than throw wholesale.

@TheBlueMatt

TheBlueMatt Feb 24, 2017

Contributor

Hmm, not sure how I feel about this. For pruning nodes, it may be more useful to return a partial list and a message indicating the results are incomplete rather than throw wholesale.

This comment has been minimized.

@kallewoof

kallewoof Feb 24, 2017

Member

Isn't that slightly dangerous? I mean, if people don't realize the node only has a subset of all blocks in some cases, they might miss the "partial" flag and never realize they potentially missed something. Even if it's their own node. Someone could even time an attack based on that.

To take a step back, the situation here is that some software which uses listsinceblock believes the chain is on one chain when it is on another one. Presuming this fork is long, the node ends up pruning beyond the fork point. And/or, the node ends up pruning out the alt chain that was deactivated. It feels like the caller (requester) should blow up at this point and either require manual assistance, or fall back to scanning over all unsafe tx's, effectively "resetting" to the current chain tip.

@kallewoof

kallewoof Feb 24, 2017

Member

Isn't that slightly dangerous? I mean, if people don't realize the node only has a subset of all blocks in some cases, they might miss the "partial" flag and never realize they potentially missed something. Even if it's their own node. Someone could even time an attack based on that.

To take a step back, the situation here is that some software which uses listsinceblock believes the chain is on one chain when it is on another one. Presuming this fork is long, the node ends up pruning beyond the fork point. And/or, the node ends up pruning out the alt chain that was deactivated. It feels like the caller (requester) should blow up at this point and either require manual assistance, or fall back to scanning over all unsafe tx's, effectively "resetting" to the current chain tip.

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Feb 24, 2017

Contributor

True. I'm not sure what the right answer here is...clearly we dont want it to be so easy to miss transactions, but on the other hand, it sucks to not have the option here. A different approach might be to iterate over mapWallet and find all transactions which have a hash of a disconnected block, which I believe is correct as long as the block was, at some point, on the active chain, though might not be if the user is calling listsinceblock on a block that was never the active chain.

@TheBlueMatt

TheBlueMatt Feb 24, 2017

Contributor

True. I'm not sure what the right answer here is...clearly we dont want it to be so easy to miss transactions, but on the other hand, it sucks to not have the option here. A different approach might be to iterate over mapWallet and find all transactions which have a hash of a disconnected block, which I believe is correct as long as the block was, at some point, on the active chain, though might not be if the user is calling listsinceblock on a block that was never the active chain.

This comment has been minimized.

@kallewoof

kallewoof Feb 24, 2017

Member

Hm.. It almost feels like I can just get the block headers by iterating altchainindex up to the fork point and put those in a set and find the removed transactions in the initial mapWallet iteration. That way there's no need to read in the blocks at all and can get rid of listed as well. Will try that, thanks for the nudge!

@kallewoof

kallewoof Feb 24, 2017

Member

Hm.. It almost feels like I can just get the block headers by iterating altchainindex up to the fork point and put those in a set and find the removed transactions in the initial mapWallet iteration. That way there's no need to read in the blocks at all and can get rid of listed as well. Will try that, thanks for the nudge!

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Feb 24, 2017

Contributor

Yea, I was heasitant to recommend doing it alone because I'm not confident about how safe it is to make sure the wallet's data is always right, will have to think more and review once you have it coded :).

@TheBlueMatt

TheBlueMatt Feb 24, 2017

Contributor

Yea, I was heasitant to recommend doing it alone because I'm not confident about how safe it is to make sure the wallet's data is always right, will have to think more and review once you have it coded :).

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Feb 24, 2017

Contributor

There is a second con to ② - it cannot list transactions which went from confirmed to 0-conf, as they now have a hashBlock of null, which I think is somewhat of a blocker to using that approach. Another option is to have an optional parameter (default to off) which allows you to get partial-removed-data.

@TheBlueMatt

TheBlueMatt Feb 24, 2017

Contributor

There is a second con to ② - it cannot list transactions which went from confirmed to 0-conf, as they now have a hashBlock of null, which I think is somewhat of a blocker to using that approach. Another option is to have an optional parameter (default to off) which allows you to get partial-removed-data.

This comment has been minimized.

@kallewoof

kallewoof Feb 25, 2017

Member

A 0-conf tx will be in the mempool right? Presuming it is, it will in fact be in the transactions array (I think).

The allow partial parameter makes sense to me. I think I'll do that for starters until we make a decision (which may be another PR).

@kallewoof

kallewoof Feb 25, 2017

Member

A 0-conf tx will be in the mempool right? Presuming it is, it will in fact be in the transactions array (I think).

The allow partial parameter makes sense to me. I think I'll do that for starters until we make a decision (which may be another PR).

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Feb 25, 2017

Contributor

That isnt a guarantee, because mempool may be limited.

@TheBlueMatt

TheBlueMatt Feb 25, 2017

Contributor

That isnt a guarantee, because mempool may be limited.

This comment has been minimized.

@kallewoof

kallewoof Apr 17, 2017

Member

@TheBlueMatt I took the liberty to remove he allow_partial flag as I've seen more support for always throwing than for having the flag and only throwing sometimes. The unsquashed tree has everything in it still so it'd be no effort to resurrect this, if you or someone felt strongly enough about it.

@kallewoof

kallewoof Apr 17, 2017

Member

@TheBlueMatt I took the liberty to remove he allow_partial flag as I've seen more support for always throwing than for having the flag and only throwing sometimes. The unsquashed tree has everything in it still so it'd be no effort to resurrect this, if you or someone felt strongly enough about it.

This comment has been minimized.

@sipa

sipa Jul 12, 2017

Member

I agree with (1), and not having allow_partial. We may want to document that this feature stops working with pruning.

@sipa

sipa Jul 12, 2017

Member

I agree with (1), and not having allow_partial. We may want to document that this feature stops working with pruning.

@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Feb 25, 2017

Member

@TheBlueMatt I didn't think about that.

But would the wallet actually do that to its own transaction? Throw out of mempool I mean.

Member

kallewoof commented Feb 25, 2017

@TheBlueMatt I didn't think about that.

But would the wallet actually do that to its own transaction? Throw out of mempool I mean.

@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Feb 25, 2017

Member

s/wallet/node/

Member

kallewoof commented Feb 25, 2017

s/wallet/node/

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Feb 25, 2017

Member
Member

sipa commented Feb 25, 2017

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Feb 25, 2017

Member

IMO if the stale block is pruned, throwing an exception is the right thing to do. The caller should be able to figure out its own rewinding back to the last common block.

(P.S. Perhaps the tests here should cover the case where there are three potential-chains involved?)

Member

luke-jr commented Feb 25, 2017

IMO if the stale block is pruned, throwing an exception is the right thing to do. The caller should be able to figure out its own rewinding back to the last common block.

(P.S. Perhaps the tests here should cover the case where there are three potential-chains involved?)

Show outdated Hide outdated src/wallet/rpcwallet.cpp
throw JSONRPCError(RPC_INTERNAL_ERROR, "Can't read block from disk");
}
}
else for (const CTransactionRef& tx : block.vtx)

This comment has been minimized.

@luke-jr

luke-jr Feb 25, 2017

Member

No else for please.

} else {
    for (const CTransactionRef& tx : block.vtx) {
@luke-jr

luke-jr Feb 25, 2017

Member

No else for please.

} else {
    for (const CTransactionRef& tx : block.vtx) {
@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Feb 25, 2017

Member

Good point about testing 3 chains. Will add a test like that.

Member

kallewoof commented Feb 25, 2017

Good point about testing 3 chains. Will add a test like that.

@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Feb 25, 2017

Member

I just realized the split network feature in the QA framework only supports one split (2 chains) right now. I think I'll make a separate PR to extend that functionality and add more tests for 3+ chains after this is merged.

Member

kallewoof commented Feb 25, 2017

I just realized the split network feature in the QA framework only supports one split (2 chains) right now. I think I'll make a separate PR to extend that functionality and add more tests for 3+ chains after this is merged.

@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Feb 26, 2017

Member

Squashed commits. Unsquashed branch preserved at https://github.com/kallewoof/bitcoin/tree/listsinceblock-include-lost-txs-unsquashed for comparison.

Member

kallewoof commented Feb 26, 2017

Squashed commits. Unsquashed branch preserved at https://github.com/kallewoof/bitcoin/tree/listsinceblock-include-lost-txs-unsquashed for comparison.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Feb 26, 2017

Member
Member

MarcoFalke commented Feb 26, 2017

@kallewoof

This comment has been minimized.

Show comment
Hide comment
Member

kallewoof commented Feb 27, 2017

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Feb 27, 2017

Member

Concept ACK. Agree with @luke-jr in throwing the exception.

Member

jtimon commented Feb 27, 2017

Concept ACK. Agree with @luke-jr in throwing the exception.

@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Mar 22, 2017

Member

Rebased.

Member

kallewoof commented Mar 22, 2017

Rebased.

'txid': utxo['txid'],
'vout': utxo['vout'],
}]
txid1 = self.nodes[1].sendrawtransaction(

This comment has been minimized.

@jnewbery

jnewbery Apr 12, 2017

Member

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.

@jnewbery

jnewbery Apr 12, 2017

Member

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.

This comment has been minimized.

@kallewoof

kallewoof Apr 13, 2017

Member

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

@kallewoof

kallewoof Apr 13, 2017

Member

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

Show outdated Hide outdated test/functional/listsinceblock.py
# generate on both sides
lastblockhash = self.nodes[1].generate(3)[2]
self.nodes[2].generate(4)
print('lastblockhash=%s' % (lastblockhash))

This comment has been minimized.

@jnewbery

jnewbery Apr 12, 2017

Member

Please don't use print() calls in test cases. Either remove the line entirely, or add a self.log.debug() call if you think this information will be useful for debugging in future.

There are several other instances of this below. I won't add additional review comments.

@jnewbery

jnewbery Apr 12, 2017

Member

Please don't use print() calls in test cases. Either remove the line entirely, or add a self.log.debug() call if you think this information will be useful for debugging in future.

There are several other instances of this below. I won't add additional review comments.

This comment has been minimized.

@kallewoof

kallewoof Apr 13, 2017

Member

Removed all print()s. They seemed vaguely useful but not enough to warrant keeping them around long term.

@kallewoof

kallewoof Apr 13, 2017

Member

Removed all print()s. They seemed vaguely useful but not enough to warrant keeping them around long term.

Show outdated Hide outdated test/functional/listsinceblock.py
# listsinceblock(lastblockhash) should now include txid1, as seen from nodes[0]
lsbres = self.nodes[0].listsinceblock(lastblockhash)
assert_equal(any(tx['txid'] == txid1 for tx in lsbres['removed']), True)

This comment has been minimized.

@jnewbery

jnewbery Apr 12, 2017

Member

nit: no need for an assert_equal(True, True) here. Instead:

assert any(tx['txid'] == txid1 for tx in lsbres['removed']), "txid1 not found in listsinceblock removed list"
@jnewbery

jnewbery Apr 12, 2017

Member

nit: no need for an assert_equal(True, True) here. Instead:

assert any(tx['txid'] == txid1 for tx in lsbres['removed']), "txid1 not found in listsinceblock removed list"
Show outdated Hide outdated test/functional/listsinceblock.py
# but it should not include 'removed' if include_removed=false
lsbres2 = self.nodes[0].listsinceblock(lastblockhash, 1, False, False)
assert_equal('removed' in lsbres2, False)

This comment has been minimized.

@jnewbery

jnewbery Apr 12, 2017

Member

again, no need to assert_equal(False, False). Instead:

assert 'removed' not in lsbres2

In general, we use assert_equal when comparing two variables because it prints the values of those variables, ie assert_equal(x, y) will print the values of x and y if they're not equal, but assert x == y will not print the values of x and y. If you're asserting the truthiness of one variable, you can just assert on that explicitly.

@jnewbery

jnewbery Apr 12, 2017

Member

again, no need to assert_equal(False, False). Instead:

assert 'removed' not in lsbres2

In general, we use assert_equal when comparing two variables because it prints the values of those variables, ie assert_equal(x, y) will print the values of x and y if they're not equal, but assert x == y will not print the values of x and y. If you're asserting the truthiness of one variable, you can just assert on that explicitly.

This comment has been minimized.

@kallewoof

kallewoof Apr 17, 2017

Member

I somehow skipped over this one. Fixed in squashed, unsquashed commit is kallewoof@72ec1ea

@kallewoof

kallewoof Apr 17, 2017

Member

I somehow skipped over this one. Fixed in squashed, unsquashed commit is kallewoof@72ec1ea

Show outdated Hide outdated test/functional/listsinceblock.py
2. It is not included in 'removed' because it was not removed.
3. It is listed with a confirmations count of 2 (bb3, bb4), not
3 (aa1, aa2, aa3).
'''

This comment has been minimized.

@jnewbery

jnewbery Apr 12, 2017

Member

Great docstrings 👍

@jnewbery

jnewbery Apr 12, 2017

Member

Great docstrings 👍

Show outdated Hide outdated test/functional/listsinceblock.py
3 (aa1, aa2, aa3).
'''
assert_equal(self.is_network_split, False)

This comment has been minimized.

@jnewbery

jnewbery Apr 12, 2017

Member

as above for assert_equal(False, False). There are more instances of this below. I won't add additional comments.

@jnewbery

jnewbery Apr 12, 2017

Member

as above for assert_equal(False, False). There are more instances of this below. I won't add additional comments.

Show outdated Hide outdated test/functional/listsinceblock.py
self.join_network()
# gettransaction should work for txid1
self.nodes[0].gettransaction(txid1)

This comment has been minimized.

@jnewbery

jnewbery Apr 12, 2017

Member

If you're expecting this to work, can you assert on the returned value?

@jnewbery

jnewbery Apr 12, 2017

Member

If you're expecting this to work, can you assert on the returned value?

This comment has been minimized.

@kallewoof

kallewoof Apr 13, 2017

Member

If the transaction cannot be gotten, an exception is raised (I swapped the txid out for a random hex below):

2017-04-13 02:22:05.191000 TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
  File "/Users/karljohan-alm/workspace/bitcoin-kw/test/functional/test_framework/test_framework.py", line 148, in main
    self.run_test()
  File "./listsinceblock.py", line 250, in run_test
    self.test_double_spend()
  File "./listsinceblock.py", line 155, in test_double_spend
    self.nodes[0].gettransaction("000000000009f75870f0ebe6f317af6928e3a74c676a60a0f11d3eab51c88ff3")
  File "/Users/karljohan-alm/workspace/bitcoin-kw/test/functional/test_framework/coverage.py", line 46, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
  File "/Users/karljohan-alm/workspace/bitcoin-kw/test/functional/test_framework/authproxy.py", line 153, in __call__
    raise JSONRPCException(response['error'])
test_framework.authproxy.JSONRPCException: Invalid or non-wallet transaction id (-5)
2017-04-13 02:22:05.193000 TestFramework (INFO): Stopping nodes
2017-04-13 02:22:14.337000 TestFramework (WARNING): Not cleaning up dir /var/folders/b8/znqr8hj918772gfmd875gzgdd3ypz1/T/test5fli8q5i/13217
2017-04-13 02:22:14.337000 TestFramework (ERROR): Test failed. Test logging available at /var/folders/b8/znqr8hj918772gfmd875gzgdd3ypz1/T/test5fli8q5i/13217/test_framework.log

Asserting on the returned value is thus not necessary but I'm adding a comment indicating this.

@kallewoof

kallewoof Apr 13, 2017

Member

If the transaction cannot be gotten, an exception is raised (I swapped the txid out for a random hex below):

2017-04-13 02:22:05.191000 TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
  File "/Users/karljohan-alm/workspace/bitcoin-kw/test/functional/test_framework/test_framework.py", line 148, in main
    self.run_test()
  File "./listsinceblock.py", line 250, in run_test
    self.test_double_spend()
  File "./listsinceblock.py", line 155, in test_double_spend
    self.nodes[0].gettransaction("000000000009f75870f0ebe6f317af6928e3a74c676a60a0f11d3eab51c88ff3")
  File "/Users/karljohan-alm/workspace/bitcoin-kw/test/functional/test_framework/coverage.py", line 46, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
  File "/Users/karljohan-alm/workspace/bitcoin-kw/test/functional/test_framework/authproxy.py", line 153, in __call__
    raise JSONRPCException(response['error'])
test_framework.authproxy.JSONRPCException: Invalid or non-wallet transaction id (-5)
2017-04-13 02:22:05.193000 TestFramework (INFO): Stopping nodes
2017-04-13 02:22:14.337000 TestFramework (WARNING): Not cleaning up dir /var/folders/b8/znqr8hj918772gfmd875gzgdd3ypz1/T/test5fli8q5i/13217
2017-04-13 02:22:14.337000 TestFramework (ERROR): Test failed. Test logging available at /var/folders/b8/znqr8hj918772gfmd875gzgdd3ypz1/T/test5fli8q5i/13217/test_framework.log

Asserting on the returned value is thus not necessary but I'm adding a comment indicating this.

This comment has been minimized.

@jnewbery

jnewbery Apr 20, 2017

Member

This comment is fine. Alternatively you could assert something like:

assert self.nodes[0].gettransaction(txid1)['txid'] == txid1, "gettransaction failed to find txid1 not found"
@jnewbery

jnewbery Apr 20, 2017

Member

This comment is fine. Alternatively you could assert something like:

assert self.nodes[0].gettransaction(txid1)['txid'] == txid1, "gettransaction failed to find txid1 not found"

This comment has been minimized.

@kallewoof

kallewoof Apr 21, 2017

Member

Ohh, I didn't realize assert also captured exceptions. Thanks, changed.

@kallewoof

kallewoof Apr 21, 2017

Member

Ohh, I didn't realize assert also captured exceptions. Thanks, changed.

Show outdated Hide outdated test/functional/listsinceblock.py
if tx['txid'] == txid1:
assert_equal(tx['confirmations'], 2)
def run_test(self):

This comment has been minimized.

@jnewbery

jnewbery Apr 12, 2017

Member

nit: I'd prefer run_test() to appear at the top of the test case, with the sub-tests below. When I come to read a test-case, it flows better for me if run test (which is called first) appears at the top.

@jnewbery

jnewbery Apr 12, 2017

Member

nit: I'd prefer run_test() to appear at the top of the test case, with the sub-tests below. When I come to read a test-case, it flows better for me if run test (which is called first) appears at the top.

This comment has been minimized.

@kallewoof

kallewoof Apr 13, 2017

Member

Good idea. Changing.

@kallewoof

kallewoof Apr 13, 2017

Member

Good idea. Changing.

Show outdated Hide outdated src/wallet/rpcwallet.cpp
while (include_removed && paltindex && paltindex != pindex)
{
CBlock block;
if (!ReadBlockFromDisk(block, paltindex, Params().GetConsensus()))

This comment has been minimized.

@jnewbery

jnewbery Apr 12, 2017

Member

nit: it'd be nice to test this branch in the functional test, both with and without the partial flag.

@jnewbery

jnewbery Apr 12, 2017

Member

nit: it'd be nice to test this branch in the functional test, both with and without the partial flag.

Show outdated Hide outdated src/wallet/rpcwallet.cpp
* @param ret The UniValue into which the result is stored.
* @param filter The "is mine" filter bool.
* @return true if appends were made to ret, false otherwise.
*/

This comment has been minimized.

@jnewbery

jnewbery Apr 12, 2017

Member

👍 comments!

@jnewbery

jnewbery Apr 12, 2017

Member

👍 comments!

@jnewbery

This comment has been minimized.

Show comment
Hide comment
@jnewbery

jnewbery Apr 12, 2017

Member

I've only reviewed the test code. It's a shame that this makes the listsinceblock test runtime increase (from 32s to 72s on recent travis runs), although #10198 and #10082 will help with that.

One design comment: I'm not convinced it's a good idea to not including duplicate transactions in the removed array. I think that could be a trap for users. My expectation would be that I could periodically run this RPC to get the list of transactions since last time I ran the command (by passing in the lastblock value from the previous call). I'd expect to be able to keep a set of transactions, adding the transactions array to my set and subtracting the removed array. If you don't include the duplicate transactions in the removed array, then the user might end up with more than one instance of a transaction in their set.

Obviously the user should look out for duplicates and not add them to their set, but if they're doing something daft like keying off the blockhash and txid, then they'll get into this inconsistent state.

I don't think that's enough to block this PR, but I think it's considering.

Member

jnewbery commented Apr 12, 2017

I've only reviewed the test code. It's a shame that this makes the listsinceblock test runtime increase (from 32s to 72s on recent travis runs), although #10198 and #10082 will help with that.

One design comment: I'm not convinced it's a good idea to not including duplicate transactions in the removed array. I think that could be a trap for users. My expectation would be that I could periodically run this RPC to get the list of transactions since last time I ran the command (by passing in the lastblock value from the previous call). I'd expect to be able to keep a set of transactions, adding the transactions array to my set and subtracting the removed array. If you don't include the duplicate transactions in the removed array, then the user might end up with more than one instance of a transaction in their set.

Obviously the user should look out for duplicates and not add them to their set, but if they're doing something daft like keying off the blockhash and txid, then they'll get into this inconsistent state.

I don't think that's enough to block this PR, but I think it's considering.

@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Apr 13, 2017

Member

@jnewbery Thanks a lot for the review! I've addressed most of your concerns, except for the nit about testing allow_partial flag.

Regarding duplicates in removed, I'm neutral personally. Allowing duplicates decreases the code size, so I'm all for the switch personally. (Clarification: I changed the code to allow duplicates.)

Unsquashed (kallewoof@8871340 is previous head): https://github.com/kallewoof/bitcoin/tree/listsinceblock-include-lost-txs-unsquashed

Member

kallewoof commented Apr 13, 2017

@jnewbery Thanks a lot for the review! I've addressed most of your concerns, except for the nit about testing allow_partial flag.

Regarding duplicates in removed, I'm neutral personally. Allowing duplicates decreases the code size, so I'm all for the switch personally. (Clarification: I changed the code to allow duplicates.)

Unsquashed (kallewoof@8871340 is previous head): https://github.com/kallewoof/bitcoin/tree/listsinceblock-include-lost-txs-unsquashed

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Apr 17, 2017

Member

Do you plan to remove the allow_partial so it always throws an exception?

Member

luke-jr commented Apr 17, 2017

Do you plan to remove the allow_partial so it always throws an exception?

@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Apr 17, 2017

Member

@luke-jr I would love to, personally. I wasn't sure if it was the agreed approach, in the end, but more people have agreed about throwing than not, so I'm going to remove it for now.

Edit: I removed the allow_partial functionality. (The unsquashed results are in the same spot (https://github.com/kallewoof/bitcoin/tree/listsinceblock-include-lost-txs-unsquashed) but since the end result was dropping of commits with some conflict fixes the unsquashed tree still has the allow_partial related commits.)

Member

kallewoof commented Apr 17, 2017

@luke-jr I would love to, personally. I wasn't sure if it was the agreed approach, in the end, but more people have agreed about throwing than not, so I'm going to remove it for now.

Edit: I removed the allow_partial functionality. (The unsquashed results are in the same spot (https://github.com/kallewoof/bitcoin/tree/listsinceblock-include-lost-txs-unsquashed) but since the end result was dropping of commits with some conflict fixes the unsquashed tree still has the allow_partial related commits.)

@jnewbery

This comment has been minimized.

Show comment
Hide comment
@jnewbery

jnewbery Apr 17, 2017

Member

Thanks @kallewoof - I don't really mind too much either way. If I were coding this from scratch, I would prefer to show duplicates in removed, but I'm happy to go along with the consensus and since you've already put in the code to hide duplicates, that's also fine.

Member

jnewbery commented Apr 17, 2017

Thanks @kallewoof - I don't really mind too much either way. If I were coding this from scratch, I would prefer to show duplicates in removed, but I'm happy to go along with the consensus and since you've already put in the code to hide duplicates, that's also fine.

@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Apr 17, 2017

Member

@jnewbery Duplicates are shown in removed now. What I removed was a different feature, where a user could pass in an 'allow partial' flag which would prevent the node from throwing an exception if a block couldn't be read (useful for pruned nodes), but several people preferred always throwing.

Member

kallewoof commented Apr 17, 2017

@jnewbery Duplicates are shown in removed now. What I removed was a different feature, where a user could pass in an 'allow partial' flag which would prevent the node from throwing an exception if a block couldn't be read (useful for pruned nodes), but several people preferred always throwing.

@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Apr 20, 2017

Member

Realized I had missed one assert_equal(x, false) -> assert not x.

Unsquashed history:

Member

kallewoof commented Apr 20, 2017

Realized I had missed one assert_equal(x, false) -> assert not x.

Unsquashed history:

@jnewbery

Looks good. A few nits but otherwise ACK a8c56bf

Show outdated Hide outdated src/wallet/rpcwallet.cpp
@@ -1792,6 +1810,8 @@ UniValue listsinceblock(const JSONRPCRequest& request)
filter = filter | ISMINE_WATCH_ONLY;
}
bool include_removed = (request.params.size() < 4 || request.params[3].get_bool());

This comment has been minimized.

@jnewbery

jnewbery Apr 20, 2017

Member

When adding RPCs or arguments, good advice is to treat null arguments the same as missing arguments: #10143 (comment)

I think that means this line would become:

bool include_removed = (request.params.size() < 4 || request.params[3].isNull() || request.params[3].get_bool());

@jnewbery

jnewbery Apr 20, 2017

Member

When adding RPCs or arguments, good advice is to treat null arguments the same as missing arguments: #10143 (comment)

I think that means this line would become:

bool include_removed = (request.params.size() < 4 || request.params[3].isNull() || request.params[3].get_bool());

Show outdated Hide outdated test/functional/listsinceblock.py
self.join_network()
# gettransaction should work for txid1
self.nodes[0].gettransaction(txid1)

This comment has been minimized.

@jnewbery

jnewbery Apr 20, 2017

Member

This comment is fine. Alternatively you could assert something like:

assert self.nodes[0].gettransaction(txid1)['txid'] == txid1, "gettransaction failed to find txid1 not found"
@jnewbery

jnewbery Apr 20, 2017

Member

This comment is fine. Alternatively you could assert something like:

assert self.nodes[0].gettransaction(txid1)['txid'] == txid1, "gettransaction failed to find txid1 not found"
Show outdated Hide outdated test/functional/listsinceblock.py
@@ -14,7 +14,12 @@ def __init__(self):
self.setup_clean_chain = True
self.num_nodes = 4
def run_test (self):
def run_test(self):
self.test_reorg()

This comment has been minimized.

@jnewbery

jnewbery Apr 20, 2017

Member

Now that you've split this into sub-tests, can you move the lines:

self.nodes[2].generate(101)
self.sync_all()

into the run_test() function before calling test_reorg(). It's best if sub-tests have no shared state between them. You should be able to re-order or skip tests without breaking them, and generating the chain in test_reorg() breaks that assumption.

We want sub-tests to be as independent as possible so if they break it's easier to debug where the problem is.

@jnewbery

jnewbery Apr 20, 2017

Member

Now that you've split this into sub-tests, can you move the lines:

self.nodes[2].generate(101)
self.sync_all()

into the run_test() function before calling test_reorg(). It's best if sub-tests have no shared state between them. You should be able to re-order or skip tests without breaking them, and generating the chain in test_reorg() breaks that assumption.

We want sub-tests to be as independent as possible so if they break it's easier to debug where the problem is.

This comment has been minimized.

@kallewoof

kallewoof Apr 21, 2017

Member

Done - but some tweaking is necessary to make them truly independent of order (e.g. instead of asserting on getbalance()s, should just grab balance and compare). Will not do that work in this PR, but will try to address it asap unless someone else gets to it before me.

@kallewoof

kallewoof Apr 21, 2017

Member

Done - but some tweaking is necessary to make them truly independent of order (e.g. instead of asserting on getbalance()s, should just grab balance and compare). Will not do that work in this PR, but will try to address it asap unless someone else gets to it before me.

Show outdated Hide outdated test/functional/listsinceblock.py
self.nodes[3].getnewaddress(): 1,
self.nodes[2].getnewaddress(): change,
}
txid2 = self.nodes[2].sendrawtransaction(

This comment has been minimized.

@jnewbery

jnewbery Apr 20, 2017

Member

txid2 is unused so you don't need to assign it here.

@jnewbery

jnewbery Apr 20, 2017

Member

txid2 is unused so you don't need to assign it here.

Show outdated Hide outdated test/functional/listsinceblock.py
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(lastblockhash, 1, False, False)

This comment has been minimized.

@jnewbery

jnewbery Apr 20, 2017

Member

consider using named arguments here so it's clear to the reader what the arguments are for (you can also omit the optional target_confirmations and include_watchonly arguments)

@jnewbery

jnewbery Apr 20, 2017

Member

consider using named arguments here so it's clear to the reader what the arguments are for (you can also omit the optional target_confirmations and include_watchonly arguments)

Show outdated Hide outdated src/wallet/rpcwallet.cpp
@@ -1756,7 +1773,8 @@ UniValue listsinceblock(const JSONRPCRequest& request)
LOCK2(cs_main, pwallet->cs_wallet);
const CBlockIndex *pindex = NULL;
const CBlockIndex* pindex = NULL;

This comment has been minimized.

@jnewbery

jnewbery Apr 20, 2017

Member

These variables might warrant their own comments now, since it's not immediately obvious what they're doing:

pindex - transactions from this block onwards should be included in the result. If the specified block was not in the main chain, pindex is the block where the chain forked
paltindex - used to count back from the specified block to the fork point to collect transactions for the removed array.

You can probably come up with better wording.

@jnewbery

jnewbery Apr 20, 2017

Member

These variables might warrant their own comments now, since it's not immediately obvious what they're doing:

pindex - transactions from this block onwards should be included in the result. If the specified block was not in the main chain, pindex is the block where the chain forked
paltindex - used to count back from the specified block to the fork point to collect transactions for the removed array.

You can probably come up with better wording.

@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Apr 21, 2017

Member

@jnewbery Thanks for feedback! I think I addressed all of the stuff you mentioned. Compare 14⊱1 & 15⊱2.

[...]:

Member

kallewoof commented Apr 21, 2017

@jnewbery Thanks for feedback! I think I addressed all of the stuff you mentioned. Compare 14⊱1 & 15⊱2.

[...]:

@jnewbery

This comment has been minimized.

Show comment
Hide comment
@jnewbery

jnewbery Apr 28, 2017

Member

Looks good. ReACK 4578a21

Member

jnewbery commented Apr 28, 2017

Looks good. ReACK 4578a21

@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof May 8, 2017

Member

Rebased due to conflicts.

Edit: removed some irrelevant asserts that started causing errors in tests. (Not sure why a clean chain with generate(101) would yield 5100 BTC instead of 50, but no matter.)

Member

kallewoof commented May 8, 2017

Rebased due to conflicts.

Edit: removed some irrelevant asserts that started causing errors in tests. (Not sure why a clean chain with generate(101) would yield 5100 BTC instead of 50, but no matter.)

@jnewbery

This comment has been minimized.

Show comment
Hide comment
@jnewbery

jnewbery May 15, 2017

Member

Edit: removed some irrelevant asserts that started causing errors in tests. (Not sure why a clean chain with generate(101) would yield 5100 BTC instead of 50, but no matter.)

Looks like you've reintroduced the lines:

        self.nodes[2].generate(101)
        self.sync_all()

into the test_reorg() function, which means you're now generating 202 blocks (so node 2 now has 102 spendable coinbases = 5100 BTC).

I think this is just a bad rebase.

Member

jnewbery commented May 15, 2017

Edit: removed some irrelevant asserts that started causing errors in tests. (Not sure why a clean chain with generate(101) would yield 5100 BTC instead of 50, but no matter.)

Looks like you've reintroduced the lines:

        self.nodes[2].generate(101)
        self.sync_all()

into the test_reorg() function, which means you're now generating 202 blocks (so node 2 now has 102 spendable coinbases = 5100 BTC).

I think this is just a bad rebase.

@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof May 16, 2017

Member

@jnewbery Ahh, thanks. Yeah, that is obviously it. I am just going to remove the double generate in test_reorg.

[...]:

Member

kallewoof commented May 16, 2017

@jnewbery Ahh, thanks. Yeah, that is obviously it. I am just going to remove the double generate in test_reorg.

[...]:

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jun 15, 2017

listsinceblock: optionally find and list any transactions that were u…
…ndone due to reorg when requesting a non-main chain block in a new 'removed' array.

Github-Pull: #9622
Rebased-From: b90171595fb1005c6463b37d260ff6782e6b8904

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jun 15, 2017

Testing: listsinceblock should display all transactions that were aff…
…ected since the given block, including transactions that were removed due to a reorg.

Github-Pull: #9622
Rebased-From: ef3db3bdd14329719a65c1ccb6cf6440678327c8

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jun 18, 2017

Testing: listsinceblock should display all transactions that were aff…
…ected since the given block, including transactions that were removed due to a reorg.

Github-Pull: #9622
Rebased-From: ef3db3bdd14329719a65c1ccb6cf6440678327c8
@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 12, 2017

Member

utACK 5d35204 with a few nits. I did not review the tests.

Member

sipa commented Jul 12, 2017

utACK 5d35204 with a few nits. I did not review the tests.

Show outdated Hide outdated src/wallet/rpcwallet.cpp
int target_confirms = 1;
isminefilter filter = ISMINE_SPENDABLE;
if (request.params.size() > 0)
if (request.params.size() > 0 && !request.params[0].isNull())

This comment has been minimized.

@sipa

sipa Jul 12, 2017

Member

Nit: the request.params.size() > 0 check is superfluous with this, as the operator[] will always return a null UniValue if out of range. (and several other places)

@sipa

sipa Jul 12, 2017

Member

Nit: the request.params.size() > 0 check is superfluous with this, as the operator[] will always return a null UniValue if out of range. (and several other places)

This comment has been minimized.

@kallewoof

kallewoof Jul 13, 2017

Member

Oh, I didn't know that. That makes things cleaner, thanks.

@kallewoof

kallewoof Jul 13, 2017

Member

Oh, I didn't know that. That makes things cleaner, thanks.

Show outdated Hide outdated src/wallet/rpcwallet.cpp
// 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)

This comment has been minimized.

@sipa

sipa Jul 12, 2017

Member

Nit: braces on the same line (and many other places).

@sipa

sipa Jul 12, 2017

Member

Nit: braces on the same line (and many other places).

Show outdated Hide outdated src/wallet/rpcwallet.cpp
CBlock block;
if (!ReadBlockFromDisk(block, paltindex, Params().GetConsensus()))
{
throw JSONRPCError(RPC_INTERNAL_ERROR, "Can't read block from disk");

This comment has been minimized.

@sipa

sipa Jul 12, 2017

Member

I agree with (1), and not having allow_partial. We may want to document that this feature stops working with pruning.

@sipa

sipa Jul 12, 2017

Member

I agree with (1), and not having allow_partial. We may want to document that this feature stops working with pruning.

@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Jul 13, 2017

Member

@sipa Thanks for the review. I've addressed your nits in kallewoof@3a10e7c

Member

kallewoof commented Jul 13, 2017

@sipa Thanks for the review. I've addressed your nits in kallewoof@3a10e7c

Show outdated Hide outdated src/wallet/rpcwallet.cpp
// Use -depth as minDepth parameter to ListTransactions to prevent incoming
// transactions from being filtered. These transactions have negative
// confirmations, but always greater than -depth.
ListTransactions(pwallet, pwallet->mapWallet[tx->GetHash()], "*", -depth, true, removed, filter);

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Jul 14, 2017

Contributor

Hmm, technically -depth isnt sufficient here. In the rare case of a reorg-to-lower-block-height this would be insufficient. You already got the block from the chain, just call it - 1000000000 or so.

@TheBlueMatt

TheBlueMatt Jul 14, 2017

Contributor

Hmm, technically -depth isnt sufficient here. In the rare case of a reorg-to-lower-block-height this would be insufficient. You already got the block from the chain, just call it - 1000000000 or so.

@@ -1761,7 +1776,10 @@ 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"

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Jul 14, 2017

Contributor

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?

@TheBlueMatt

TheBlueMatt Jul 14, 2017

Contributor

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?

Show outdated Hide outdated test/functional/listsinceblock.py
Asserted:
1. tx1 is listed in listsinceblock.
2. It is not included in 'removed' because it was not removed.

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Jul 14, 2017

Contributor

This is wrong (and the check and later comment contradict this).

@TheBlueMatt

TheBlueMatt Jul 14, 2017

Contributor

This is wrong (and the check and later comment contradict this).

assert any(tx['txid'] == txid1 for tx in lsbres['removed'])
# find transaction and ensure confirmations is valid
for tx in lsbres['transactions']:

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Jul 14, 2017

Contributor

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

@TheBlueMatt

TheBlueMatt Jul 14, 2017

Contributor

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

@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Jul 18, 2017

Member

@TheBlueMatt Thanks for the review! I've addressed all of your nits, I believe. The unsquashed changes are in kallewoof@5166d0e and kallewoof@299c00c.

Member

kallewoof commented Jul 18, 2017

@TheBlueMatt Thanks for the review! I've addressed all of your nits, I believe. The unsquashed changes are in kallewoof@5166d0e and kallewoof@299c00c.

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Jul 18, 2017

Contributor

I think this maybe missed 15, sadly. Its a nice change, but not a bugfix.

Contributor

TheBlueMatt commented Jul 18, 2017

I think this maybe missed 15, sadly. Its a nice change, but not a bugfix.

@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Jul 19, 2017

Member

That's disheartening, but ah well.

Member

kallewoof commented Jul 19, 2017

That's disheartening, but ah well.

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Jul 19, 2017

Contributor

utACK d6115c2. Yea, Core has been in a near-constant state of growing pains for some time it seems. Recently its grown active enough to be just beyond the ability of any individual to keep up with everything, and so things occasionally move slower than they should :(. Anyway, up to @laanwj, this isnt a bugfix, but its pretty clean so maybe he's still willing to pull it for 15.

Contributor

TheBlueMatt commented Jul 19, 2017

utACK d6115c2. Yea, Core has been in a near-constant state of growing pains for some time it seems. Recently its grown active enough to be just beyond the ability of any individual to keep up with everything, and so things occasionally move slower than they should :(. Anyway, up to @laanwj, this isnt a bugfix, but its pretty clean so maybe he's still willing to pull it for 15.

@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Jul 19, 2017

Member

I'd say let's just push it to 16 milestone. I'll be noisy about it this time. :)

Member

kallewoof commented Jul 19, 2017

I'd say let's just push it to 16 milestone. I'll be noisy about it this time. :)

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Jul 19, 2017

Contributor
Contributor

TheBlueMatt commented Jul 19, 2017

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 20, 2017

Member

Needs rebase.

I'd say let's just push it to 16 milestone. I'll be noisy about it this time. :)

Up to you, though given how long this has been open and how much interest and review this has I don't have a particular problem with merging this into 0.15 still.

Member

laanwj commented Jul 20, 2017

Needs rebase.

I'd say let's just push it to 16 milestone. I'll be noisy about it this time. :)

Up to you, though given how long this has been open and how much interest and review this has I don't have a particular problem with merging this into 0.15 still.

kallewoof added some commits Jan 24, 2017

listsinceblock: optionally find and list any transactions that were u…
…ndone due to reorg when requesting a non-main chain block in a new 'removed' array.
Testing: listsinceblock should display all transactions that were aff…
…ected since the given block, including transactions that were removed due to a reorg.
@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Jul 21, 2017

Member

@laanwj Rebased. And I'd love to have this in 15, personally. I just don't wanna rush anything for the sake of the PR being old.

Member

kallewoof commented Jul 21, 2017

@laanwj Rebased. And I'd love to have this in 15, personally. I just don't wanna rush anything for the sake of the PR being old.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 21, 2017

Member

utACK

Member

sipa commented Jul 21, 2017

utACK

@laanwj laanwj merged commit 876e92b into bitcoin:master Jul 24, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jul 24, 2017

Merge #9622: [rpc] listsinceblock should include lost transactions wh…
…en 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 #9516 (comment) but I could be wrong..

Tree-SHA512: 607b5dcaeccb9dc0d963d3de138c40490f3e923050b29821e6bd513d26beb587bddc748fbb194503fe618cfe34a6ed65d95e8d9c5764a882b6c5f976520cff35

@kallewoof kallewoof deleted the kallewoof:listsinceblock-include-lost-txs branch Jul 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment