Bug-fix: listsinceblock: use fork point as reference for blocks in reorg’d chains #9516

1. 
   kallewoof commented at 9:06 am on January 11, 2017: member

   When listsinceblock is called with a block hash that belongs to a non-main chain, the system will mistakenly use its relative position in its chain vs the current active chain’s height to determine which transactions should be included.

   This patch locates the fork point and uses that as reference instead.

   The problem can be observed in the following scenario: with address A belonging to node n1 in a network with n1 through n4:

   1. Split network into two partitions, n12 and n34.
   2. Create and send a transaction tx from n3 to address A.
   3. Generate 6 blocks on n1. Note last block hash as <lastKnownHash>.
   4. Generate 6 blocks on n3.
   5. Merge network into n1234.
   6. Generate 1 block on n3 to activate n34 chain.
   7. Do listsinceblock <lastKnownHash> on n1.

   The resulting transaction list should contain tx, but it does not.

2. fanquake added the label RPC/REST/ZMQ on Jan 11, 2017
3. fanquake added the label Wallet on Jan 11, 2017
4. 
   MarcoFalke commented at 10:02 am on January 11, 2017: member

   Thanks for including steps to reproduce. Those should be trivial to translate to our python test suite.

   Mind to add a commit with the new test?

5. 
   kallewoof commented at 10:04 am on January 11, 2017: member

   @MarcoFalke Will do!
6. 
   kallewoof commented at 11:05 am on January 11, 2017: member

   @MarcoFalke Done. Feedback welcome!
7. in qa/rpc-tests/listsinceblock.py: in 35ad7efa23 outdated

   0@@ -0,0 +1,83 @@
   1+#!/usr/bin/env python3
   2+# Copyright (c) 2014-2017 The Bitcoin Core developers
   3+# Distributed under the MIT software license, see the accompanying
   4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
   5+
   6+from test_framework.test_framework import BitcoinTestFramework
   7+from test_framework.util import *
   

   ---

   ---

   

   MarcoFalke commented at 11:23 am on January 11, 2017:

   Nit: It is sufficient to only import what you need. (E.g. assert_equal)

   Also, +x is missing on the permissions of this file.

   ---

   

   kallewoof commented at 11:29 am on January 11, 2017:

   Fixed.

   ---

   

   MarcoFalke commented at 12:00 pm on January 11, 2017:

   Thx, feel free to squash.

   ---

   

   kallewoof commented at 12:37 pm on January 11, 2017:

   Done.
8. kallewoof force-pushed on Jan 11, 2017
9. 
   laanwj commented at 1:06 pm on January 11, 2017: member

   Good catch. utACK 8d78648
10. 
    gmaxwell commented at 8:47 am on January 13, 2017: contributor

    Why isn’t this using LastCommonAncestor/FindFork or equivalent? I believe doing that would give precisely the correct behavior.

    (Aside: Good find. I had just assumed this was doing the LCA thing. :( )

11. 
    kallewoof commented at 9:01 am on January 13, 2017: member

    @gmaxwell Nice! I didn’t realize it existed. FindFork works like a charm.
12. kallewoof force-pushed on Jan 13, 2017
13. kallewoof renamed this:
    Bug-fix: listsinceblock: use max depth value for blocks in reorg'd chains
    Bug-fix: listsinceblock: use fork point as reference for blocks in reorg'd chains
    on Jan 13, 2017
14. 
    gmaxwell commented at 6:19 pm on January 13, 2017: contributor

    utACK.
15. Bug-fix: listsinceblock: use closest common ancestor when a block hash was provided for a chain that was not the main chain. ee5c1ce5a6
16. kallewoof force-pushed on Jan 18, 2017
17. Testing: listsinceblock should not use orphan block height. 7ba0a00aae
18. kallewoof force-pushed on Jan 18, 2017
19. 
    luke-jr commented at 9:54 pm on January 20, 2017: member

    Looks like improvement, but I’m not sure it fixes all the issues: if a transaction was confirmed on the “since” block, and no longer is, we should include it in the results even if it did not change between the fork-point and tip.
20. 
    kallewoof commented at 1:03 am on January 23, 2017: member

    @luke-jr Yeah, a double spend of the same UTXO will result in no transaction showing up, which is definitely not ideal..
21. MarcoFalke added this to the milestone 0.14.0 on Jan 23, 2017
22. laanwj merged this on Jan 23, 2017
23. laanwj closed this on Jan 23, 2017

    ---

24. laanwj referenced this in commit 727a798360 on Jan 23, 2017
25. kallewoof deleted the branch on Jan 24, 2017
26. laanwj referenced this in commit 6ef3c7ec62 on Jul 24, 2017
27. mempko referenced this in commit 83822eb637 on Sep 28, 2017
28. codablock referenced this in commit 75a5874e54 on Jan 19, 2018
29. codablock referenced this in commit 0d7eb548ad on Jan 20, 2018
30. codablock referenced this in commit 119c3fe62f on Jan 21, 2018
31. andvgal referenced this in commit 5d4555e6d5 on Jan 6, 2019
32. CryptoCentric referenced this in commit 289c26eae1 on Feb 27, 2019
33. jasonbcox referenced this in commit d1c1b1620a on Mar 26, 2019
34. jonspock referenced this in commit 71b06f1ea3 on Apr 6, 2019
35. jonspock referenced this in commit f2bf033e02 on Apr 8, 2019
36. jonspock referenced this in commit 251d8051e1 on Apr 8, 2019
37. jonspock referenced this in commit 335b53bc56 on Apr 8, 2019
38. jonspock referenced this in commit e41df7d91d on Apr 8, 2019
39. PastaPastaPasta referenced this in commit 69ddb22ee7 on Aug 6, 2019
40. PastaPastaPasta referenced this in commit 0f9878f438 on Aug 6, 2019
41. PastaPastaPasta referenced this in commit 747085410c on Aug 6, 2019
42. PastaPastaPasta referenced this in commit a026b74f9e on Aug 7, 2019
43. PastaPastaPasta referenced this in commit 6ccec5915f on Aug 8, 2019
44. PastaPastaPasta referenced this in commit f393729498 on Aug 12, 2019
45. charlesrocket referenced this in commit 1a196c293d on Nov 30, 2019
46. barrystyle referenced this in commit a23d53ae0a on Jan 22, 2020
47. DrahtBot locked this on Sep 8, 2021

Contributors
kallewoof MarcoFalke laanwj gmaxwell luke-jr

Labels
Wallet RPC/REST/ZMQ

Milestone
0.14.0