Page MenuHomePhabricator

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

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

Details

Summary

876e92b Testing: listsinceblock should display all transactions that were affected since the given block, including transactions that were removed due to a reorg. (Karl-Johan Alm)
f999c46 listsinceblock: optionally find and list any transactions that were undone due to reorg when requesting a non-main chain block in a new 'removed' array. (Karl-Johan Alm)

Pull request description:

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

1. User 1 receives BTC in tx0 from utxo1 in block aa1.
2. User 2 receives BTC in tx1 from utxo1 (same) in block bb1
3. User 1 sees 2 confirmations at block aa3.
4. Reorg into bb chain.
5. User 1 asks `listsinceblock aa3` and does not see that tx0 is now invalidated.

See `listsinceblock.py` commit for related test.

The proposed fix is to iterate from the given block down to the fork point, and to check each transaction in the blocks against the wallet, in addition to including all transactions from the fork point to the active chain tip (the current behavior). Any transactions that were present will now also be listed in the `listsinceblock` output in a new `replaced` array. This operation may be a bit heavy but the circumstances (and perceived frequency of occurrence) warrant it, I believe.

Example output:
```Python
{
  'transactions': [],
  'replaced': [
    {
      'walletconflicts': [],
      'vout': 1,
      'account': '',
      'timereceived': 1485234857,
      'time': 1485234857,
      'amount': '1.00000000',
      'bip125-replaceable': 'unknown',
      'trusted': False,
      'category': 'receive',
      'txid': 'ce673859a30dee1d2ebdb3c05f2eea7b1da54baf68f93bb8bfe37c5f09ed22ff',
      'address': 'miqEt4kWp9zSizwGGuUWLAmxEcTW9bFUnQ',
      'label': '',
      'confirmations': -7
    }
  ],
  'lastblock': '7a388f27d09e3699102a4ebf81597d974fc4c72093eeaa02adffbbf7527f6715'
}
```

I believe this addresses the comment by @luke-jr in https://github.com/bitcoin/bitcoin/pull/9516#issuecomment-274190081 but I could be wrong..

Tree-SHA512: 607b5dcaeccb9dc0d963d3de138c40490f3e923050b29821e6bd513d26beb587bddc748fbb194503fe618cfe34a6ed65d95e8d9c5764a882b6c5f976520cff35

Backport of Core PR 9622
https://github.com/bitcoin/bitcoin/pull/9622/files
Completes T551

Test Plan

make check
test_runner.py wallet_listsinceblock

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

test/functional/wallet_listsinceblock.py
120 ↗(On Diff #7706)

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

test/functional/wallet_listsinceblock.py
120 ↗(On Diff #7706)

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

test/functional/wallet_listsinceblock.py
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

test/functional/wallet_listsinceblock.py
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

Also this likely deserves a release note

linting, BTC -> BCH, and release notes

This revision is now accepted and ready to land.Mar 16 2019, 08:01