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

Empact · 2018-07-12T21:20:31Z

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.

Empact · 2018-07-12T21:23:33Z

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

Empact · 2018-07-12T21:43:29Z

Added test.

Empact · 2018-07-12T21:49:49Z

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
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

morcos · 2018-07-12T22:19:10Z

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

promag

utACK 4401e8f.

promag · 2018-07-13T14:03:35Z

test/functional/wallet_abandonconflict.py

+        inputs = []
+        inputs.append({"txid":txABC2, "vout":0})
+        outputs = {}
+        outputs[self.nodes[0].getnewaddress()] = signed3_change


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

promag · 2018-07-13T14:04:18Z

test/functional/wallet_abandonconflict.py

+        #Create a child tx spending ABC2
+        signed3_change = Decimal("24.999")
+        inputs = []
+        inputs.append({"txid":txABC2, "vout":0})


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

promag · 2018-07-13T14:05:46Z

test/functional/wallet_abandonconflict.py

@@ -70,9 +70,19 @@ def run_test(self):
        signed2 = self.nodes[0].signrawtransactionwithwallet(self.nodes[0].createrawtransaction(inputs, outputs))
        txABC2 = self.nodes[0].sendrawtransaction(signed2["hex"])

+        #Create a child tx spending ABC2


nit, space after #.

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.

laanwj · 2018-07-16T11:57:48Z

utACK 89e70f9, thanks for adding better tests too

maflcko · 2018-07-16T12:12:03Z

utACK 89e70f9

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

promag · 2018-07-16T12:32:07Z

re-utACK 89e70f9.

…he grandchildren, etc. active 89e70f9 Fix that CWallet::AbandonTransaction would only traverse one level (Ben Woosley) Pull request description: 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. Tree-SHA512: df068b49637d299ad73237c7244005fe5aa966d6beae57aff12e6948f173d9381e1b5d08533f7e3a1416991ed57f9f1f7b834057141d85c07dc60bb1f0872cea

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. Github-Pull: #13652 Rebased-From: 89e70f9 Tree-SHA512: 403da0cc400a807e5a30038bd505881a68208c3f9e96ad5a7755e763666982bc3c19564ac13a5757612c8b6efc331fb2ad0edbaf7e830993b84bc64624423e54

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. Github-Pull: bitcoin#13652 Rebased-From: 89e70f9 Tree-SHA512: 403da0cc400a807e5a30038bd505881a68208c3f9e96ad5a7755e763666982bc3c19564ac13a5757612c8b6efc331fb2ad0edbaf7e830993b84bc64624423e54

91b48c7 [Build] Add new merkle files to CMake lists (warrows) 48a3aff [Wallet] Ignore coinbase and zc tx "conflicts" (warrows) 3572354 [Wallet] Fix an error in tx depth computation (warrows) 6928369 [Tests] Enable abandonconflict functional test (warrows) 34cd496 Fix that CWallet::AbandonTransaction would only traverse one level (Ben Woosley) aba5b75 Fix calculation of balances and available coins. (Alex Morcos) 48d705f Remove stale wallet transactions on initial load. (presstab) 12985ae Flush wallet after abandontransaction (Alex Morcos) 8f87956 [Wallet] sort pending wallet transactions before reaccepting (dexX7) 9c2f445 [Wallet] Call notification signal when a transaction is abandoned (Jonas Schnelli) 778ebf3 Add new rpc call: abandontransaction (Alex Morcos) 0e86c3e Make wallet descendant searching more efficient (Alex Morcos) d0083a8 Make sure conflicted wallet tx's update balances (Alex Morcos) 6a50e03 [Wallet] Keep track of explicit wallet conflicts instead of using mempool (warrows) 7ccb2b5 [Wallet] Do not flush the wallet in AddToWalletIfInvolvingMe(..) (warrows) 47345be [Refactor] Move wallet functions out of header (warrows) ab9efb8 [Wallet] Switch to a constant-space Merkle root/branch algorithm (warrows) 5447622 [Wallet] Do not store Merkle branches in the wallet (warrows) Pull request description: This pull request is a happy melting pot of improvements regarding transactions handling. Most of them are backports from bitcoin. I advise reviewers to check the code of the different commits independently to understand them more easily. However, testing is probably better done all at once. I am making a single pull request because these changes are all entangled and introducing some of them without others would probably introduce temporary bugs. ## Commits details ## - 6c3e2ac backport of bitcoin#6550 - 5304fdf backport of bitcoin#6508 - c3eeeac simple code move from the header to the cpp file. It contains no functional change. - 6cc4d37 backport of bitcoin#4805 - 10be1db backport of bitcoin#7105 - 8a34c32 backport of bitcoin#7306 - 3caf123, 9e17178 and 240f5b4 are the backport for bitcoin#7312 - ad6d0b1 backport of bitcoin#5511 - fcc07c3 backport of bitcoin#9311 - 5ed5e26 is an update of #825 - 392d504 backport of bitcoin#7715 - 7199f3a backport of bitcoin#13652 - f09d999 enables and fixes the test from bitcoin#7312 - 4fd43c5 fixes an oversight in bitcoin#7105 backport ACKs for top commit: random-zebra: ACK 91b48c7 Fuzzbawls: ACK 91b48c7 Tree-SHA512: 2628cebe98805b8048b920b51ee26fd4f0c53643d78da9b8cb265aede52dfe1d40c8c19d34293c232c5c35be7f1ab89ff5b4a07073a4b27c371ea70eb8708669

…leave the grandchildren, etc. active 89e70f9 Fix that CWallet::AbandonTransaction would only traverse one level (Ben Woosley) Pull request description: 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. Tree-SHA512: df068b49637d299ad73237c7244005fe5aa966d6beae57aff12e6948f173d9381e1b5d08533f7e3a1416991ed57f9f1f7b834057141d85c07dc60bb1f0872cea

Empact force-pushed the fix-abandon-transaction branch from c7d8363 to 22de256 Compare July 12, 2018 21:43

Empact force-pushed the fix-abandon-transaction branch from 22de256 to 4401e8f Compare July 12, 2018 21:49

Empact changed the title ~~Fix that CWallet::AbandonTransaction would only traverse one level~~ Fix that CWallet::AbandonTransaction would leave the grandchildren, etc. active Jul 12, 2018

Empact changed the title ~~Fix that CWallet::AbandonTransaction would leave the grandchildren, etc. active~~ rpc: Fix that CWallet::AbandonTransaction would leave the grandchildren, etc. active Jul 12, 2018

fanquake added the Wallet label Jul 12, 2018

promag reviewed Jul 13, 2018

View reviewed changes

Fix that CWallet::AbandonTransaction would only traverse one level

89e70f9

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.

Empact force-pushed the fix-abandon-transaction branch from 4401e8f to 89e70f9 Compare July 13, 2018 15:16

fanquake added the Needs backport label Jul 16, 2018

fanquake added this to the 0.16.2 milestone Jul 16, 2018

laanwj merged commit 89e70f9 into bitcoin:master Jul 16, 2018

fanquake removed the Needs backport label Jul 16, 2018

Empact deleted the fix-abandon-transaction branch July 16, 2018 19:54

Bushstar mentioned this pull request Jul 19, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#356

Merged

This was referenced Jul 28, 2019

Minor wallet issue with conflicted txs #7315

Closed

[Wallet] Various transaction handling improvements PIVX-Project/PIVX#970

Merged

UdjinM6 mentioned this pull request Apr 14, 2020

Backports 0.16 pr17 dashpay/dash#3407

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

Empact commented Jul 12, 2018 •

edited

Loading

Uh oh!

Empact commented Jul 12, 2018

Uh oh!

Empact commented Jul 12, 2018

Uh oh!

Empact commented Jul 12, 2018 •

edited

Loading

Uh oh!

morcos commented Jul 12, 2018

Uh oh!

promag left a comment

Uh oh!

promag Jul 13, 2018

Uh oh!

promag Jul 13, 2018

Uh oh!

promag Jul 13, 2018

Uh oh!

laanwj commented Jul 16, 2018

Uh oh!

maflcko commented Jul 16, 2018

Uh oh!

promag commented Jul 16, 2018

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

Empact commented Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact commented Jul 12, 2018

Uh oh!

Empact commented Jul 12, 2018

Uh oh!

Empact commented Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

morcos commented Jul 12, 2018

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Jul 13, 2018

Choose a reason for hiding this comment

Uh oh!

promag Jul 13, 2018

Choose a reason for hiding this comment

Uh oh!

promag Jul 13, 2018

Choose a reason for hiding this comment

Uh oh!

laanwj commented Jul 16, 2018

Uh oh!

maflcko commented Jul 16, 2018

Uh oh!

promag commented Jul 16, 2018

Uh oh!

Uh oh!

Empact commented Jul 12, 2018 •

edited

Loading

Empact commented Jul 12, 2018 •

edited

Loading