Add RPC call abandontransaction #7312

morcos · 2016-01-07T17:33:07Z

Unconfirmed transactions that are not in your mempool either due to eviction or other means may be unlikely to be mined. abandontransaction gives the wallet a way to no longer consider as spent the coins that are inputs to such a transaction. All dependent transactions in the wallet will also be marked as abandoned.

@laanwj also for 0.12

This is the basic functionality.
There are more things to add though. I have an RPC test in progress, but if anyone else wants to work on the remaining items, please do:

Return abandoned status in listtransactions
Return abandoned status in GUI
Fix any issues with how abandoned txs should sort
Add a way to abandon transactions from GUI

I built this off of #7306 to make sure the tests would work correctly.

luke-jr · 2016-01-07T17:35:35Z

This is way too late for 0.12...

morcos · 2016-01-07T17:45:25Z

@luke-jr I don't think we can release 0.12 without this. With mempool eviction combined with the new behaviour that unconfirmed txs still tie up the coins they spend even if the txs are not in the mempool, users may end up with permanently unspendable coins frequently.

We discussed making this change in conjunction with the other changes back in November, but it appears to have slipped through the cracks.

luke-jr · 2016-01-07T17:52:19Z

This situation isn't really any worse than the status quo from older versions...

jonasschnelli · 2016-01-07T20:05:04Z

src/wallet/wallet.cpp

CMerkleTx::GetDepthInMainChain() is slightly expansive (and IsSpent() gets called a lot). Would caching mit->second.GetDepthInMainChain() make sense? Or do we expect the compiler does it internally (I don't think so)?

sure can fix

jonasschnelli · 2016-01-07T20:17:31Z

Concept ACK.
I agree it late for 0.12,... but I don't take a position on this (if it's saver to go with or without a such feature).

Would be nice to see some tests (especially when we tag this for 0.12).

morcos · 2016-01-07T21:42:33Z

Addressed @jonasschnelli's comment (thanks for quick review!) and reordered commits for simpler reading.

morcos · 2016-01-08T03:47:28Z

oops, i didn't mean to push that RPC test yet, it was still in progress, but might as well leave it there for now...

kanzure · 2016-01-08T12:58:15Z

src/wallet/rpcwallet.cpp

consistency nit: all the other fHelp messages use "transaction" and "transactions" instead of "tx" and "txs".

kanzure · 2016-01-08T13:01:44Z

I think a name like "abandonlocaltransaction" or "abandonunconfirmedtransaction" could help to minimize ambiguity. But I would consider this a very low priority suggestion, because this RPC call is only available when the wallet is enabled.

maflcko · 2016-01-08T13:05:34Z

abandonunconfirmedtransaction

How would you type that without any typos? I'd rather make the rpc short ("abandontx") and the documentation/GUI verbose.

maflcko · 2016-01-08T13:06:47Z

Concept ACK 6d74a63154cca756d698d6022c694b21e7f43ac5

morcos · 2016-01-08T16:46:06Z

Thanks for the review.
Addressed nits in RPC help and finalized the RPC test

maflcko · 2016-01-08T17:09:18Z

qa/pull-tester/rpc-tests.py

abandonconflicts.py: No such file or directory

maflcko · 2016-01-08T17:52:29Z

Reviewed-tests ACK 21702ff

@morcos Please fix the travis issue (file not found)

morcos · 2016-01-08T19:05:15Z

oops sorry.
fixed typo and removed bitcoind argument from rpc test.

maflcko · 2016-01-08T19:43:11Z

qa/rpc-tests/abandonconflict.py

Shouldn't resendwallettransactions do the same? I get the empty array on resendwallettransactions.

I thought it made sense that abandoned transactions would not be automatically resent.
That way when you restart a bitcoind with transactions that you've tried to abandon, it wouldn't automatically try to send them all again

maflcko · 2016-01-08T19:44:57Z

QT-tested ACK d716866

Nit: Qt balance != getwalletinfo

jonasschnelli · 2016-01-11T09:54:55Z

src/wallet/wallet.cpp

I think here we also need a NotifyTransactionChanged(this, wtx.GetHash(), CT_UPDATED); Otherwise the signal listeners (UI) can't recalculate the balance.

jonasschnelli · 2016-01-11T10:02:56Z

Tested ACK (also ack on a 0.12 bp, because it's a wallet only change)
Nits:

missing update signal (see above)
missing flag in listtransactions.

jonasschnelli · 2016-01-11T10:21:54Z

Addressed the nits and a cosmetic change for GUI in: morcos#6

morcos · 2016-01-11T19:27:50Z

@jonasschnelli , thanks! I added your notify commit. I left the other changes for a separate pull.

morcos · 2016-01-11T21:47:52Z

Ok updated after speaking to @jonasschnelli
I think this is good now

PRabahy · 2016-01-12T04:39:54Z

Does https://github.com/bitcoin/bitcoin/blob/master/contrib/bitcoind.bash-completion need to get updated with this change?

maflcko · 2016-01-12T07:29:54Z

@PRabahy sure, but I don't think this is part of this pull.

Unconfirmed transactions that are not in your mempool either due to eviction or other means may be unlikely to be mined. abandontransaction gives the wallet a way to no longer consider as spent the coins that are inputs to such a transaction. All dependent transactions in the wallet will also be marked as abandoned.

morcos · 2016-01-13T13:46:03Z

Addressed @laanwj's concern about clearly identifying constant

d11fc16 [Wallet] Call notification signal when a transaction is abandoned (Jonas Schnelli) df0e222 Add RPC test for abandoned and conflicted transactions. (Alex Morcos) 01e06d1 Add new rpc call: abandontransaction (Alex Morcos) 9e69717 Make wallet descendant searching more efficient (Alex Morcos)

- Make wallet descendant searching more efficient - Add new rpc call: abandontransaction Unconfirmed transactions that are not in your mempool either due to eviction or other means may be unlikely to be mined. abandontransaction gives the wallet a way to no longer consider as spent the coins that are inputs to such a transaction. All dependent transactions in the wallet will also be marked as abandoned. - Add RPC test for abandoned and conflicted transactions. - [Wallet] Call notification signal when a transaction is abandoned Github-Pull: #7312 Rebased-From: 9e69717 01e06d1 df0e222 d11fc16

- Make wallet descendant searching more efficient - Add new rpc call: abandontransaction Unconfirmed transactions that are not in your mempool either due to eviction or other means may be unlikely to be mined. abandontransaction gives the wallet a way to no longer consider as spent the coins that are inputs to such a transaction. All dependent transactions in the wallet will also be marked as abandoned. - Add RPC test for abandoned and conflicted transactions. - [Wallet] Call notification signal when a transaction is abandoned Github-Pull: bitcoin#7312 Rebased-From: 9e69717 01e06d1 df0e222 d11fc16

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 - fcc07c3562 backport of bitcoin#9311 - 5ed5e266794 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

morcos mentioned this pull request Jan 7, 2016

Wallet fixes needed for 0.12 #7305

Closed

morcos force-pushed the forgetstuck branch from 5d31aad to 9726a52 Compare January 7, 2016 19:05

laanwj added the Wallet label Jan 7, 2016

jonasschnelli reviewed Jan 7, 2016
View reviewed changes

Make wallet descendant searching more efficient

9e69717

morcos force-pushed the forgetstuck branch from 9726a52 to b7d3dcd Compare January 7, 2016 21:41

kanzure reviewed Jan 8, 2016
View reviewed changes

laanwj added this to the 0.12.0 milestone Jan 8, 2016

morcos force-pushed the forgetstuck branch from 6d74a63 to 21702ff Compare January 8, 2016 16:45

maflcko reviewed Jan 8, 2016
View reviewed changes

qa/pull-tester/rpc-tests.py Outdated

Copy link

Member

maflcko Jan 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

abandonconflicts.py: No such file or directory

morcos force-pushed the forgetstuck branch from 21702ff to d716866 Compare January 8, 2016 19:04

maflcko reviewed Jan 8, 2016
View reviewed changes

jonasschnelli reviewed Jan 11, 2016
View reviewed changes

morcos force-pushed the forgetstuck branch from 1878647 to 7998625 Compare January 11, 2016 21:46

morcos and others added 3 commits January 13, 2016 08:42

Add RPC test for abandoned and conflicted transactions.

df0e222

[Wallet] Call notification signal when a transaction is abandoned

d11fc16

morcos force-pushed the forgetstuck branch from 7998625 to d11fc16 Compare January 13, 2016 13:45

laanwj merged commit d11fc16 into bitcoin:master Jan 13, 2016

laanwj mentioned this pull request Jan 13, 2016

release note fixups #7336

Merged

laanwj mentioned this pull request Feb 29, 2016

[WIP] [RPC] Add call zaptransaction to delete transaction from wallet #7602

Closed

laanwj mentioned this pull request Mar 17, 2016

Pending issues for abandontransaction #7704

Closed

5 tasks

daira mentioned this pull request Jun 26, 2017

Support abandontransaction RPC call zcash/zcash#2482

Open

Warrows mentioned this pull request Jul 30, 2019

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

Merged

ryanofsky mentioned this pull request May 5, 2020

[wallet] Track conflicted transactions removed from mempool and fix UI notifications #18600

Closed

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

Add RPC call abandontransaction #7312

Add RPC call abandontransaction #7312

Uh oh!

Conversation

morcos commented Jan 7, 2016

Uh oh!

luke-jr commented Jan 7, 2016

Uh oh!

morcos commented Jan 7, 2016

Uh oh!

luke-jr commented Jan 7, 2016

Uh oh!

jonasschnelli Jan 7, 2016

Choose a reason for hiding this comment

Uh oh!

morcos Jan 7, 2016

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Jan 7, 2016

Uh oh!

morcos commented Jan 7, 2016

Uh oh!

morcos commented Jan 8, 2016

Uh oh!

kanzure Jan 8, 2016

Choose a reason for hiding this comment

Uh oh!

kanzure commented Jan 8, 2016

Uh oh!

maflcko commented Jan 8, 2016

Uh oh!

maflcko commented Jan 8, 2016

Uh oh!

morcos commented Jan 8, 2016

Uh oh!

maflcko Jan 8, 2016

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jan 8, 2016

Uh oh!

morcos commented Jan 8, 2016

Uh oh!

maflcko Jan 8, 2016

Choose a reason for hiding this comment

Uh oh!

morcos Jan 13, 2016

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jan 8, 2016

Uh oh!

jonasschnelli Jan 11, 2016

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Jan 11, 2016

Uh oh!

jonasschnelli commented Jan 11, 2016

Uh oh!

morcos commented Jan 11, 2016

Uh oh!

morcos commented Jan 11, 2016

Uh oh!

PRabahy commented Jan 12, 2016

Uh oh!

maflcko commented Jan 12, 2016

Uh oh!

morcos commented Jan 13, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants