Add a method for removing a single wallet tx. #3675

djpnewton commented at 4:56 AM on February 15, 2014: contributor

Also changes old ZapWalletTx function names to ZapWalletTxes to match the command line argument (-zapwallettxes)

This could be a nice conservative option for those who have zombie transactions and need to get rid of them to get an accurate account balance. I think the user might need to perform a rescan once removing the offending tx's.

I have not tested this yet, I need to figure out how to get a testnet wallet with some zombie transactions in it.

Add a method for removing a single wallet tx.

Also changes old ZapWalletTx function names to ZapWalletTxes to match the
command line argument (-zapwallettxes)

3ec0f4e8e2

jgarzik commented at 5:02 AM on February 15, 2014: contributor

This is completely insufficient. You must update dependencies... conditionally. And there are several edge cases even so. As implemented here, it works for some people and some situations, but leaves the wallet in a worse state otherwise.

djpnewton commented at 5:12 AM on February 15, 2014: contributor

Right ok, this is why "-zapwallettxes" only runs at startup before the wallet is loaded.

By dependencies I guess you mean the other modules that rely on the walletdb state?

laanwj commented at 7:40 AM on February 15, 2014: member

@djpnewton You'd have to update vfSpent for the appropriate outputs of parent transactions, but only if there isn't another transaction spending that output.

Also if you zap a transaction that has dependents, you need to zap those as well. And zapping those transactions, in turn, may trigger vfSpent updates in other transactions.

If you want to do this at runtime there is also other cached state that will need to be updated.

Also if you do this low-level you want to keep an inactive copy of the zapped transaction around for forensic purposes.

All in all ... it's very complex. The proposal is to get rid of vfSpent which would make this (among other things) easier. But that is quite involved (and error prone) in itself. That's why we went with the sledgehammer approach for now.

djpnewton commented at 8:17 AM on February 15, 2014: contributor

@jgarzik @laanwj Thanks for the comments.

Will a rescan update the vfSpent values if one were to zap a single transaction?

If so would it be feasible to zap a single transaction (and its dependents) at startup and rescan?

laanwj commented at 9:51 AM on February 15, 2014: member

IIRC a rescan will not touch existing transactions at all. It just adds transactions that were missing. (Though it may update vfSpent when new transactions appear that spend old outputs. It will never do the opposite and helpfully clear vfSpent. MarkSpent is a one-way street)

ZapWalletTx (single): added deletion of child transactions and also

updates the spent outputs of the parent transactions that are deleted.

There is some cacheing somewhere so the runtime (rpc) changes dont all
show up until a restart.

022aa3d043

djpnewton commented at 12:24 PM on February 16, 2014: contributor

OK I have a (probably still naive) implementation that kills child transactions and updates parent tx spent outputs. If the function is performed at runtime the some of the results do not show up until a restart.. some cache needs to be cleared.

sipa commented at 1:48 PM on February 16, 2014: member

Rescan will not fix vfSpent. It can only find missing transactions, and if necessary update the block they were in.

The solution is getting rid of vfSpent :)

laanwj commented at 9:43 AM on February 17, 2014: member

Even without vfSpent update it would be useful to be able to remove non-confirming transactions from others (like 1Enjoy 1Sochi spam). Though -zapwallettxes could be used for that too.

Remove CWalletTx::vfSpent

Use the spent outpoint multimap to figure out which wallet transaction
outputs are unspent, instead of a vfSpent array that is saved
to disk.

beca478992

gavinandresen commented at 5:14 PM on February 17, 2014: contributor

See #3694 RE: removing vfSpent-- review/testing there would be very appreciated.

A regression test or two in qa/rpc-tests (see the tests in #3694 for examples) that demonstrates zap-a-single-wallet-transaction working properly in various tricky scenarios would make me much more likely to give an ACK.

zapwallettx: remove erased tx's from mapWallet 49129486b8

start implementing zapwallettx tests... 1d39973a68

Merge branch 'master' into vfspent c3443d64db

Merge branch 'vfspent' of https://github.com/gavinandresen/bitcoin-git into zapwallettx-single-novfspent

Modification of spent outputs no longer needed.

Clear wallet tx cache variables

Conflicts:
	src/wallet.h

e5d2d0c702

Remove CWalletTx::vtxPrev 1fa606d93a

lock wallet while we zap some transactions 3ce78c1179

zapwallettx: add test case for single tx and a chain of dependant

transactions

6cfb210020

Merge branch 'vfspent' of https://github.com/gavinandresen/bitcoin-git into zapwallettx-single-novfspent 5717503f05

djpnewton commented at 10:32 AM on February 19, 2014: contributor

@gavinandresen I have now based it on your vfSpent work, cheers

BitcoinPullTester commented at 9:57 AM on February 23, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/5717503f0558dfb947ff985875d55e9ffbf00410 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.

laanwj commented at 9:56 AM on March 7, 2014: member

As the list of commits looks pretty messed up now, I suggest drastic recovery steps like:

FEATURE_BRANCH='zapwallettx-single'
NEW_FEATURE_BRANCH='zapwallettx-single-rebased'
MERGE_BASE=1fa606d
PATCH_FILE=zapwallettx.diff
# Make diff file
git diff $MERGE_BASE $FEATURE_BRANCH > $PATCH_FILE
# Create a new branch from master
git checkout master
git checkout -b $NEW_FEATURE_BRANCH
# Apply diff
patch -p1 --merge < $PATCH_FILE
# Check and test and fix up if there are errors (it compiled at least)
...
# Make one commit with all the changes
git add qa/rpc-tests/zapwallettx.sh # re-add new file
git commit -av
# Push new branch to github
git push origin $NEW_FEATURE_BRANCH

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

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

@laanwj thanks, I have created a new PR #3845

djpnewton closed this on Mar 11, 2014

DrahtBot locked this on Sep 8, 2021