rpc: Fix that CWallet::AbandonTransaction would leave the grandchildren, etc. active #13652

pull Empact wants to merge 1 commits into bitcoin:master from Empact:fix-abandon-transaction changing 2 files +11 −3
  1. Empact commented at 9:20 PM on July 12, 2018: member

    Prior to this change, it would mark only the first layer of child transactions abandoned, due to always following the input hashTx rather than the current now tx.

  2. Empact commented at 9:23 PM on July 12, 2018: member

    Has been this way since introduction: 01e06d1fa365cedb7f5d5e17e6bdf0b526e700c5 /cc @morcos The very similar method MarkConflicted has been implemented this way since introduction. 9ac63d6d30

  3. Empact force-pushed on Jul 12, 2018
  4. Empact commented at 9:43 PM on July 12, 2018: member

    Added test.

  5. Empact force-pushed on Jul 12, 2018
  6. Empact commented at 9:49 PM on July 12, 2018: member

    Without this change, test line 133 fails, because re-sending the 1st child automatically re-activates the 2nd child, because it has never been marked abandoned: https://github.com/bitcoin/bitcoin/pull/13652/files#diff-065f504c44f2c3fc4eebe8287af0ed6eR133

    test output before:

    $ python3 test/functional/wallet_abandonconflict.py 
2018-07-12T21:47:02.018000Z TestFramework (INFO): Initializing test directory /var/folders/_v/0klckbk95bxgt21rhcj_6qb40000gn/T/test2k5hh2on
2018-07-12T21:47:09.864000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/bitcoin/test/functional/test_framework/test_framework.py", line 160, in main
    self.run_test()
  File "test/functional/wallet_abandonconflict.py", line 133, in run_test
    assert_equal(newbalance, balance - Decimal("10") - Decimal("14.99998") + Decimal("24.9996"))
  File "/bitcoin/test/functional/test_framework/util.py", line 39, in assert_equal
    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(2469.99988720 == 2494.99948720)
2018-07-12T21:47:09.918000Z TestFramework (INFO): Stopping nodes
2018-07-12T21:47:10.452000Z TestFramework (WARNING): Not cleaning up dir /var/folders/_v/0klckbk95bxgt21rhcj_6qb40000gn/T/test2k5hh2on
2018-07-12T21:47:10.452000Z TestFramework (ERROR): Test failed. Test logging available at /var/folders/_v/0klckbk95bxgt21rhcj_6qb40000gn/T/test2k5hh2on/test_framework.log
2018-07-12T21:47:10.453000Z TestFramework (ERROR): Hint: Call /Users/ben/Development/Bitcoin/bitcoin/test/functional/combine_logs.py '/var/folders/_v/0klckbk95bxgt21rhcj_6qb40000gn/T/test2k5hh2on' to consolidate all logs

    And after:

    $ python3 test/functional/wallet_abandonconflict.py 
2018-07-12T21:45:39.393000Z TestFramework (INFO): Initializing test directory /var/folders/_v/0klckbk95bxgt21rhcj_6qb40000gn/T/testohym14hk
2018-07-12T21:45:47.471000Z TestFramework (INFO): If balance has not declined after invalidateblock then out of mempool wallet tx which is no longer
2018-07-12T21:45:47.471000Z TestFramework (INFO): conflicted has not resumed causing its inputs to be seen as spent.  See Issue [#7315](/bitcoin-bitcoin/7315/)
2018-07-12T21:45:47.471000Z TestFramework (INFO): 2489.99988720 -> 2489.99988720 ?
2018-07-12T21:45:47.527000Z TestFramework (INFO): Stopping nodes
2018-07-12T21:45:48.058000Z TestFramework (INFO): Cleaning up /var/folders/_v/0klckbk95bxgt21rhcj_6qb40000gn/T/testohym14hk on exit
2018-07-12T21:45:48.058000Z TestFramework (INFO): Tests successful
  7. Empact renamed this:
    Fix that CWallet::AbandonTransaction would only traverse one level
    Fix that CWallet::AbandonTransaction would leave the grandchildren, etc. active
    on Jul 12, 2018
  8. Empact renamed this:
    Fix that CWallet::AbandonTransaction would leave the grandchildren, etc. active
    rpc: Fix that CWallet::AbandonTransaction would leave the grandchildren, etc. active
    on Jul 12, 2018
  9. morcos commented at 10:19 PM on July 12, 2018: member

    Nice catch. That does look like a long standing bug. Concept ACK.

  10. fanquake added the label Wallet on Jul 12, 2018
  11. in test/functional/wallet_abandonconflict.py:78 in 4401e8f182 outdated 
      69 | @@ -70,9 +70,19 @@ def run_test(self):
  70 |          signed2 = self.nodes[0].signrawtransactionwithwallet(self.nodes[0].createrawtransaction(inputs, outputs))
  71 |          txABC2 = self.nodes[0].sendrawtransaction(signed2["hex"])
  72 |  
  73 | +        #Create a child tx spending ABC2
  74 | +        signed3_change = Decimal("24.999")
  75 | +        inputs = []
  76 | +        inputs.append({"txid":txABC2, "vout":0})
  77 | +        outputs = {}
  78 | +        outputs[self.nodes[0].getnewaddress()] = signed3_change
    promag commented at 2:03 PM on July 13, 2018:

    outputs = { self.nodes[0].getnewaddress(): signed3_change }

  12. in test/functional/wallet_abandonconflict.py:76 in 4401e8f182 outdated 
      69 | @@ -70,9 +70,19 @@ def run_test(self):
  70 |          signed2 = self.nodes[0].signrawtransactionwithwallet(self.nodes[0].createrawtransaction(inputs, outputs))
  71 |          txABC2 = self.nodes[0].sendrawtransaction(signed2["hex"])
  72 |  
  73 | +        #Create a child tx spending ABC2
  74 | +        signed3_change = Decimal("24.999")
  75 | +        inputs = []
  76 | +        inputs.append({"txid":txABC2, "vout":0})
    promag commented at 2:04 PM on July 13, 2018:

    inputs = [{ 'txid': txABC2, 'vout': 0 }]

  13. in test/functional/wallet_abandonconflict.py:73 in 4401e8f182 outdated 
      69 | @@ -70,9 +70,19 @@ def run_test(self):
  70 |          signed2 = self.nodes[0].signrawtransactionwithwallet(self.nodes[0].createrawtransaction(inputs, outputs))
  71 |          txABC2 = self.nodes[0].sendrawtransaction(signed2["hex"])
  72 |  
  73 | +        #Create a child tx spending ABC2
    promag commented at 2:05 PM on July 13, 2018:

    nit, space after #.

  14. promag commented at 2:18 PM on July 13, 2018: member

    utACK 4401e8f.

  15. Fix that CWallet::AbandonTransaction would only traverse one level
    Prior to this change, it would mark only the first layer of
child transactions abandoned, due to always following the input hashTx
rather than the current now tx.
    89e70f9d7f
  16. Empact force-pushed on Jul 13, 2018
  17. fanquake added the label Needs backport on Jul 16, 2018
  18. fanquake added this to the milestone 0.16.2 on Jul 16, 2018
  19. fanquake added this to the "Blockers" column in a project
  20. laanwj commented at 11:57 AM on July 16, 2018: member

    utACK 89e70f9d7fe384ef9de4fa3828d4c80523290186, thanks for adding better tests too

  21. MarcoFalke commented at 12:12 PM on July 16, 2018: member

    utACK 89e70f9d7fe384ef9de4fa3828d4c80523290186

    Checked that the test in 89e70f9d7fe384ef9de4fa3828d4c80523290186 fails without compiling the bitcoind

  22. promag commented at 12:32 PM on July 16, 2018: member

    re-utACK 89e70f9.

  23. laanwj merged this on Jul 16, 2018
  24. laanwj closed this on Jul 16, 2018
  25. laanwj referenced this in commit 17943f77bd on Jul 16, 2018
  26. fanquake removed this from the "Blockers" column in a project
  27. laanwj referenced this in commit 20461fc272 on Jul 16, 2018
  28. fanquake removed the label Needs backport on Jul 16, 2018
  29. Empact deleted the branch on Jul 16, 2018
  30. HashUnlimited referenced this in commit bb87978075 on Jan 11, 2019
  31. random-zebra referenced this in commit 0f1764a3db on Oct 9, 2019
  32. PastaPastaPasta referenced this in commit f1d026e0ca on Apr 12, 2020
  33. PastaPastaPasta referenced this in commit bf0269e9a8 on Apr 16, 2020
  34. PastaPastaPasta referenced this in commit bbce45d8bd on Apr 16, 2020
  35. ckti referenced this in commit 38c887b743 on Mar 28, 2021
  36. MarcoFalke locked this on Sep 8, 2021
Contributors
EmpactmorcospromaglaanwjMarcoFalke
Labels
Wallet

Milestone
0.16.2

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-21 15:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me