Add a method for removing a single wallet tx (rebased) #3845

djpnewton commented at 10:09 AM on March 11, 2014: contributor

rebased version of PR #3675

Add a method for removing a single wallet tx 08de928528

remove zapwallettx command line c0fd840cec

add rescan (default=true) parameter to zapwallettx rpc e0d31dd67b

laanwj added this to the milestone 0.10.0 on Mar 11, 2014

laanwj commented at 10:16 AM on March 11, 2014: member

Note that this basically implements what is requested in #3795, except that you can use it for confirmed transactions as well, which may be pointless (or maybe not... not sure).

use queue to avoid recursion e50b57541c

in src/walletdb.cpp:None in e0d31dd67b outdated

 776 | +            {
 777 | +                if (txin.prevout.hash == wtx.GetHash() && txin.prevout.n == nVoutIndex)
 778 | +                {
 779 | +                    LogPrintf("ZapWalletTx found child tx %s\n", wtxPotentialChild.GetHash().GetHex());
 780 | +                    //TODO: recursion could be a problem if it gets too deep
 781 | +                    DBErrors ret = ZapWalletTx(pwallet, wtxPotentialChild, vErasedTxes);

sipa commented at 3:55 PM on March 11, 2014:

Can you use a queue of to-be-deleted txids instead of recursion here?

BitcoinPullTester commented at 9:39 PM on March 18, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/e50b57541cf8fdce7671e065886d8458a78df6eb for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

djpnewton commented at 2:23 AM on March 19, 2014: contributor

What now?

laanwj commented at 10:28 AM on March 19, 2014: member

People need to test it and review the code and post ACKs.

laanwj commented at 9:53 AM on April 5, 2014: member

... which of course, no one does... Sigh. It happens every time, at one point everyone seems to want a certain feature, then it is implemented, and no one of the people that requested it is around anymore.

Is really no one interested in this? No one with 'zombie transactions' that wants the joy of zapping them one by one?

Maybe you could ask on the mailing list, IRC or forum whether someone wants to help test.

kjsisco commented at 1:42 AM on April 7, 2014: none

Pardon my ignorance, but what exactly does this fix?

laanwj commented at 6:57 AM on April 7, 2014: member

It is not a 'fix', it adds the possibility to remove individual stuck transactions.

cozz commented at 10:57 AM on April 9, 2014: contributor

I have some suggestions to this pull request:

Do not allow to erase confirmed transactions: They would be recovered through rescan, so why delete them? (Some wallet pruning funtionality, to clean up large wallets, and erase old spent transactions could make sense, but thats a different story.)
Forget about the rescan: If we dont allow erasing confirmed transaction there is no reason to do a rescan as the wallet vfSpent flags have been removed.
Rename "zapwallettx" to "erasewallettx": I think the term "erase" is preferred to be used in software over "zap" for deleting something permanently.
Do not make any changes in walletdb.cpp at all: Currently we pass a wallet pointer to walletdb.cpp and then loop mapWallet there. Instead simply loop mapWallet in wallet.cpp and fill vErasedTxes. Then loop vErasedTxes and call CWallet::EraseFromWallet for every tx to erase. This function deletes both, database and mapWallet. No code additions to walletdb.cpp needed.
Add special case parameter "all": If the passed txid is "all" => erase all unconfirmed transactions. The loop could be done in the rpc method and fill the queue with all unconfirmed transactions already. Then simply pass the queue directly to CWallet::EraseWalletTxes. So EraseWalletTxes takes a queue as parameter, which is either all or only 1 unconformed tx.
Remove zapwallettxes completely: The reason for zapwallettxes to run on startup and do a rescan was to avoid messing up the wallet vfSpent flags. Now that they are gone and if we had a "erasewallettx all/txid", I vote for removing zapwallettxes completely. Zapwallettxes doesnt really solve anything other than removing unconfirmed transactions. If your wallet is that corrupt then you are better off using either berkeley db repair tools or start a new clean wallet by using salvagewallet/pywallet/importwallet.
Remove from mapTxSpends: Every erased transaction needs to be removed from CWallet::mapTxSpends. The code could be added to CWallet::EraseFromWallet.
Remove from setLockedCoins: To have a clean state of CWallet::setLockedCoins the transaction should be removed there (if its in there). The code could be added to CWallet::EraseFromWallet.
(Remove from coin control selection): You could trigger a temporary minor bug, if you select a transaction in coin control and then erase it. But this would require to pass a signal to walletmodel. Its no critical bug, even if it happens, and very unlikely scenario, so I would be fine just forgetting about this.
Mention metadata in rpc help text: Transaction metadata like strFromAccount gets lost, if someone erases an unconfirmed transaction, which then gets confirmed because it had already been broadcasted. I think we shouldnt bother much, but at least mention this in the rpc help text: "Notice: An erased transaction which still gets confirmed by miners, will be readded to the wallet without transaction meta data e.g. account owner and payment request information."
About the workqueue:
- This loop can be removed: for (unsigned int nVoutIndex = 0; nVoutIndex < wtx.vout.size(); nVoutIndex++). It is enough to check the hash only. if (txin.prevout.hash == wtx.GetHash()) => Found child
- it is possible transactions get added multiple times to the queue, so to have a clean algorithm, I suggest adding txs into a set<uint256> to ensure every transaction only gets processed once. If a transaction is already in the set, skip it, because it is or was already in the queue.
- nit: use vector instead of <queue>, just because we dont use queue anywhere else. (not important)
Return bool instead of DBErrors: DBErrors is for when loading the wallet.
Remove DB_NEED_REWRITE and cr+: Those 2 dont make sense either. The "c" is for DB_CREATE if the wallet file does not exist and rewrite gets checked on wallet load. But they should remove anyway if we refactor the code from walletdb.cpp to wallet.cpp as I suggested above.
Notify GUI: For every erased transaction we need to update the GUI. This can be done when looping vErasedTxes. NotifyTransactionChanged(this, hash, CT_DELETED);

laanwj commented at 7:03 AM on June 3, 2014: member

It seems a lot of work and impact for something that has a very rare use-case.

There is also little interest in this pull from testers, I suppose most actual issues with stuck wallet transactions can be solved by passing -zapwalletxes. For this rare use-case, it's not a big issue to have to restart the daemon, avoiding all the changes to bookkeeping that would be necessary as @cozz mentions.

So I'm leaning toward closing this.

Diapolo commented at 7:40 AM on June 3, 2014: none

Or let @cozz do a rebase of this, if he has time and will :)? I also greatly dislike zap as a term for erase or remove!

cozz commented at 2:38 PM on June 3, 2014: contributor

I am fine with closing, as you said, if someone needs to clear his unconfirmed transactions he can use zapwallettxes. I would just like to see some changes to zapwallettxes:

do not delete confirmed txes. Why do we delete confirmed txes with zapwallettxes? This does not make sense anymore after removing of vfSpent. This would also prevent people from erasing transation meta data to their confirmed transactions.
no rescan anymore
change description to "Erase unconfirmed wallet transactions"
preferably rename zapwallettxes to eraseunconfirmedtxes

laanwj commented at 2:48 PM on June 3, 2014: member

Ok, closing this. I'm fine with improving zapwallettxes. Removing only unconfirmed transactions makes sense to me in the common case (but there may be scenarios in which one may want to remove all transactions, I don't know, I suppose we didn't add this without a reason). I also think you are a humorless bunch, zapping transactions is all the fun :)

laanwj closed this on Jun 3, 2014

cozz commented at 3:04 PM on June 3, 2014: contributor

ok, lets make it -nukerandomwallettx so people can play russian roulette with it :)

jgarzik commented at 4:25 PM on June 3, 2014: contributor

Additive rather than subtractive methods are more reliable. Thus, the feature (a) zaps everything, then lets the normal processing/rescanning code (b) add back that which is necessary.

"removing only X type of transactions" is a less reliable method that will inevitably need tweaking if that fails to meet your needs. As such, I don't view updating -zapwallettxes to remove only unconfirmed transactions as an improvement.

cozz commented at 4:37 PM on June 3, 2014: contributor

@jgarzik Hmm, is there really a use case where I want to zap my confirmed transactions, and then recover the exact same transactions through rescan other than a corrupt wallet?

jgarzik commented at 4:44 PM on June 3, 2014: contributor

A re-evaluation of the chain from clean slate is more secure. Who can say if your "confirmed" transactions are still on the latest/best chain?

cozz commented at 5:31 PM on June 3, 2014: contributor

Our software checks all the time with GetDepthInMainChain(..), how deep transactions are in the main chain.

I just dont see a scenario for zapping a confirmed transaction, if it gets recovered with rescan anyway. I still vote for not removing confirmed transactions and not do a rescan. But thats just my opinion. I guess I am just missing something here.

jgarzik commented at 5:39 PM on June 3, 2014: contributor

My question was a rhetorical question, pointing out the problem with alternative approaches. "Don't zap confirmed transactions" is strictly defined as "don't zap transactions which will probably but not 100% certainly remain confirmed" which is not equivalent.

Performing redundant work is not an issue here, as rescan is a one-time, offline recovery operation. There is no need to micro-optimize these situations. Indeed, micro-optimizing this situation may potentially make the feature less useful.

cozz commented at 6:05 PM on June 3, 2014: contributor

Ok, lets just stay with the current behavior then. I have submitted a pull for recovering metadata in the past #3674. This can be useful for people who only want to remove their unconfirmed transactions without losing their account balances and payment request information etc of the confirmed transactions.

DrahtBot locked this on Sep 8, 2021