Page MenuHomePhabricator

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

Authored by jasonbcox on Mar 14 2019, 18:14.



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 `` 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 but I could be wrong..

Tree-SHA512: 607b5dcaeccb9dc0d963d3de138c40490f3e923050b29821e6bd513d26beb587bddc748fbb194503fe618cfe34a6ed65d95e8d9c5764a882b6c5f976520cff35

Backport of Core PR 9622
Completes T551

Test Plan

make check wallet_listsinceblock

Diff Detail

rABC Bitcoin ABC
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jasonbcox created this revision.Mar 14 2019, 18:14
Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 14 2019, 18:14
Herald added a subscriber: schancel. · View Herald Transcript
jasonbcox added inline comments.Mar 14 2019, 18:42
120 ↗(On Diff #7706)

Hm, during linting I thought "autofix" meant it was going to fix it for me.

Fabien added inline comments.Mar 15 2019, 11:11
120 ↗(On Diff #7706)

It is supposed to, just like clang-format. I will check.

Fabien added inline comments.Mar 15 2019, 11:17
120 ↗(On Diff #7706)

I tested locally, it proposed to update with an autofix warning, as yes is the default I just pressed enter and it updated the file as expected.
However for some reason it did not amend the commit, you have to do it manually. Some option must be missing in the arcanist configuration, I will take a look.

Fabien requested changes to this revision.Mar 15 2019, 12:25

Apart from the linter warnings and the BTC=>BCH nit it looks good to me

95 ↗(On Diff #7706)

Update BTC => BCH

120 ↗(On Diff #7706)

I didn't get offered to amend because I used arc lint and nor arc diff.

This revision now requires changes to proceed.Mar 15 2019, 12:25
Fabien added a comment.Mar 15 2019, 12:38

Also this likely deserves a release note

jasonbcox updated this revision to Diff 7714.Mar 15 2019, 16:30

linting, BTC -> BCH, and release notes

Fabien accepted this revision.Mar 16 2019, 08:01
This revision is now accepted and ready to land.Mar 16 2019, 08:01
This revision was automatically updated to reflect the committed changes.