Page MenuHomePhabricator

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

Authored by jasonbcox on Thu, Mar 14, 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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jasonbcox created this revision.Thu, Mar 14, 18:14
Herald added a reviewer: Restricted Project. · View Herald TranscriptThu, Mar 14, 18:14
Herald added a subscriber: schancel. · View Herald Transcript
jasonbcox added inline comments.Thu, Mar 14, 18:42
test/functional/wallet_listsinceblock.py
120 ↗(On Diff #7706)

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

Fabien added inline comments.Fri, Mar 15, 11:11
test/functional/wallet_listsinceblock.py
120 ↗(On Diff #7706)

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

Fabien added inline comments.Fri, Mar 15, 11:17
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.Fri, Mar 15, 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.Fri, Mar 15, 12:25
Fabien added a comment.Fri, Mar 15, 12:38

Also this likely deserves a release note

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

linting, BTC -> BCH, and release notes

Fabien accepted this revision.Sat, Mar 16, 08:01
This revision is now accepted and ready to land.Sat, Mar 16, 08:01
Closed by commit rABCd1c1b1620a48: Merge #9622: [rpc] listsinceblock should include lost transactions when… (authored by Wladimir J. van der Laan <laanwj@gmail.com>, committed by jasonbcox). · Explain WhySat, Mar 16, 15:31
This revision was automatically updated to reflect the committed changes.