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

kallewoof · 2017-01-24T05:41:12Z

The following scenario will not notify the caller of the fact tx0 has been dropped:

User 1 receives BTC in tx0 from utxo1 in block aa1.
User 2 receives BTC in tx1 from utxo1 (same) in block bb1
User 1 sees 2 confirmations at block aa3.
Reorg into bb chain.
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 · 2017-01-24T08:01:45Z

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 · 2017-01-24T08:11:39Z

@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 · 2017-01-24T12:59:35Z

@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 · 2017-01-24T13:24:39Z

@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 461d5a37a3d83edbeedb701ed207bc14412dee0d with minor suggestions

ryanofsky · 2017-01-24T21:03:46Z

src/wallet/rpcwallet.cpp

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 · 2017-01-24T21:14:15Z

src/wallet/rpcwallet.cpp

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 · 2017-01-24T21:16:20Z

src/wallet/rpcwallet.cpp

Need documentation update to accompany this.

ryanofsky · 2017-01-24T21:26:03Z

qa/rpc-tests/listsinceblock.py

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

ryanofsky · 2017-01-24T21:27:38Z

qa/rpc-tests/listsinceblock.py

unused variable

ryanofsky · 2017-01-24T21:34:54Z

qa/rpc-tests/listsinceblock.py

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 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.

ryanofsky · 2017-01-25T15:18:07Z

qa/rpc-tests/listsinceblock.py

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 · 2017-01-25T15:28:02Z

src/wallet/rpcwallet.cpp

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 · 2017-01-25T22:47:45Z

@ryanofsky Thanks for all the feedback! Updated.

gmaxwell · 2017-01-26T07:19:46Z

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.

TheBlueMatt · 2017-01-28T20:09:46Z

src/wallet/rpcwallet.cpp

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 · 2017-01-30T01:59:14Z

@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 · 2017-01-30T08:45:28Z

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 · 2017-01-30T09:05:11Z

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

laanwj · 2017-01-30T11:32:30Z

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

…ected since the given block, including transactions that were removed due to a reorg. Github-Pull: bitcoin#9622 Rebased-From: ef3db3bdd14329719a65c1ccb6cf6440678327c8

sipa · 2017-07-12T23:05:11Z

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

sipa · 2017-07-12T22:57:22Z

src/wallet/rpcwallet.cpp

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)

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

sipa · 2017-07-12T22:57:40Z

src/wallet/rpcwallet.cpp

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

sipa · 2017-07-12T23:03:03Z

src/wallet/rpcwallet.cpp

+        CBlock block;
+        if (!ReadBlockFromDisk(block, paltindex, Params().GetConsensus()))
+        {
+            throw JSONRPCError(RPC_INTERNAL_ERROR, "Can't read block from disk");


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

kallewoof · 2017-07-13T01:34:22Z

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

TheBlueMatt · 2017-07-14T18:31:41Z

src/wallet/rpcwallet.cpp

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 · 2017-07-14T18:32:12Z

src/wallet/rpcwallet.cpp

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 · 2017-07-14T18:34:08Z

test/functional/listsinceblock.py

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

TheBlueMatt · 2017-07-14T18:34:31Z

test/functional/listsinceblock.py

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

kallewoof · 2017-07-18T04:33:19Z

@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 · 2017-07-18T21:36:20Z

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

kallewoof · 2017-07-19T02:12:55Z

That's disheartening, but ah well.

TheBlueMatt · 2017-07-19T20:42:46Z

utACK d6115c21a790e139bb45ec6dee421a976bc21eaf. 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 · 2017-07-19T23:12:00Z

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

TheBlueMatt · 2017-07-19T23:18:39Z

Heh, yes, if things have been sitting for a while definitely be noisy!

…

On July 19, 2017 7:12:08 PM EDT, kallewoof ***@***.***> wrote: I'd say let's just push it to 16 milestone. I'll be noisy about it this time. :)

laanwj · 2017-07-20T15:06:20Z

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.

…ndone due to reorg when requesting a non-main chain block in a new 'removed' array.

…ected since the given block, including transactions that were removed due to a reorg.

kallewoof · 2017-07-21T02:01:58Z

@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 · 2017-07-21T17:11:22Z

utACK

@luke-jr

…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

@luke-jr

…ions when 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 bitcoin#9516 (comment) but I could be wrong.. Tree-SHA512: 607b5dcaeccb9dc0d963d3de138c40490f3e923050b29821e6bd513d26beb587bddc748fbb194503fe618cfe34a6ed65d95e8d9c5764a882b6c5f976520cff35

jonasschnelli added RPC/REST/ZMQ Wallet labels Jan 24, 2017

ryanofsky approved these changes Jan 24, 2017

View reviewed changes

kallewoof force-pushed the listsinceblock-include-lost-txs branch 2 times, most recently from 4faf084 to 131df5a Compare January 25, 2017 04:59

maflcko added this to the 0.14.0 milestone Jan 25, 2017

ryanofsky reviewed Jan 25, 2017

View reviewed changes

kallewoof force-pushed the listsinceblock-include-lost-txs branch from e66a1b1 to d92496c Compare January 27, 2017 12:30

TheBlueMatt reviewed Jan 28, 2017

View reviewed changes

kallewoof force-pushed the listsinceblock-include-lost-txs branch from 544e77a to 7dcfa98 Compare January 30, 2017 03:02

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

kallewoof force-pushed the listsinceblock-include-lost-txs branch 6 times, most recently from a157931 to 31654ae Compare January 30, 2017 11:00

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

sipa reviewed Jul 12, 2017

View reviewed changes

kallewoof force-pushed the listsinceblock-include-lost-txs branch from 5d35204 to 0d457c7 Compare July 13, 2017 01:34

TheBlueMatt reviewed Jul 14, 2017

View reviewed changes

kallewoof force-pushed the listsinceblock-include-lost-txs branch from 0d457c7 to d6115c2 Compare July 18, 2017 04:33

kallewoof added 2 commits July 21, 2017 09:50

listsinceblock: optionally find and list any transactions that were u…

f999c46

…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…

876e92b

…ected since the given block, including transactions that were removed due to a reorg.

kallewoof force-pushed the listsinceblock-include-lost-txs branch from d6115c2 to 876e92b Compare July 21, 2017 00:51

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

kallewoof deleted the listsinceblock-include-lost-txs branch July 24, 2017 11:17

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

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

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

Uh oh!

Conversation

kallewoof commented Jan 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented Jan 24, 2017

Uh oh!

kallewoof commented Jan 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented Jan 24, 2017

Uh oh!

kallewoof commented Jan 24, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jan 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kallewoof Jan 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kallewoof commented Jan 25, 2017

Uh oh!

gmaxwell commented Jan 26, 2017

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kallewoof commented Jan 30, 2017

Uh oh!

JeremyRubin commented Jan 30, 2017

Uh oh!

kallewoof commented Jan 30, 2017

Uh oh!

laanwj commented Jan 30, 2017

Uh oh!

sipa commented Jul 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kallewoof commented Jul 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kallewoof commented Jul 18, 2017

Uh oh!

kallewoof commented Jan 24, 2017 •

edited

Loading

kallewoof commented Jan 24, 2017 •

edited

Loading

ryanofsky Jan 24, 2017 •

edited

Loading

kallewoof Jan 25, 2017 •

edited

Loading