wallet: Replace -zapwallettxes with zapwallettxes RPC

achow101 commented at 8:19 pm on August 3, 2020: member

-zapwallettxes is known to not work with multiwallet and is currently disabled if multiple wallets are being loaded.

This PR removes the -zapwallettxes startup option and replaces its functionality with a zapwallettxes RPC command. This RPC does almost the same thing that the startup command did in that it removes all of the transactions from the target wallet. A blockchain rescan can optionally be triggered, as well as a mempool rescan. The default is to rescan the blockchain but not the mempool.

However a large difference is that -zapwallettxes would prevent the mempool from being loaded (#10330) but the RPC does not clear the mempool. However this does mean that the usefulness of this is questionable as the unconfirmed transactions that are being removed from the wallet would still appear in the mempool. The original behavior can be replicated by doing zapwallettxes and restarting with -persistmempool=0.

Also tests are updated to use the new RPC.

achow101 force-pushed on Aug 3, 2020

DrahtBot added the label RPC/REST/ZMQ on Aug 3, 2020

DrahtBot added the label Tests on Aug 3, 2020

DrahtBot added the label Wallet on Aug 3, 2020

achow101 force-pushed on Aug 3, 2020

Add zapwallettxes RPC

Updates the test to use this RPC. The test will also use multiple
wallets instead of multiple nodes

bdc3c1636f

achow101 force-pushed on Aug 3, 2020

DrahtBot commented at 0:37 am on August 4, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19671 (wallet: Remove -zapwallettxes by achow101)
#19619 (Remove wallet.dat path handling from wallet.cpp, rpcwallet.cpp by ryanofsky)
#19602 (wallet: Migrate legacy wallets to descriptor wallets by achow101)
#19168 (Refactor: Improve setup_clean_chain semantics by fjahr)
#19077 (wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets by achow101)
#18836 (wallet: upgradewallet fixes and additional tests by achow101)
#17211 (Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101)
#16910 (wallet: reduce loading time by using unordered maps by achow101)
#16365 (Log RPC parameters (arguments) if -debug=rpcparams by LarryRuane)
#15719 (Wallet passive startup by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

MarcoFalke removed the label Tests on Aug 4, 2020

in src/wallet/rpcwallet.cpp:4116 in e61f86987c outdated

4112@@ -4113,6 +4113,119 @@ static UniValue upgradewallet(const JSONRPCRequest& request)
4113     return error.original;
4114 }
4115 
4116+static UniValue zapwallettxes(const JSONRPCRequest& request)

MarcoFalke commented at 3:11 pm on August 4, 2020:

style nit: Can use the new constructor: https://doxygen.bitcoincore.org/class_c_r_p_c_command.html#afa39f5bda9319f3b91c6b38bbc7c7434 , so that it doesn’t have to be changed in the future again (c.f. #19386)

MarcoFalke commented at 2:17 pm on August 5, 2020: member

Concept ACK (feel free to ignore style nit)

instagibbs commented at 2:31 pm on August 5, 2020: member

@laanwj brings up a point on irc that I think is good enough to bring up here: Why not just make this a wallet tool function? Might save some OP text space not worrying about mempool at all for this.

MarcoFalke commented at 2:34 pm on August 5, 2020: member

If you forget to start with -persistmempool=0, then even the wallet tool can’t save you

achow101 commented at 8:47 pm on August 6, 2020: member

From today’s IRC meeting, it seemed like the consensus was to make this part of the wallet tool. This seemed fine at first, but I started looking into that a bit more deeply and I think if we want to retain all of the same behavior, it doesn’t quite work. The issue is the option to keep transaction metadata. This works by keeping the removed txs in a temp variable and restoring the metadata for the txs found during the rescan. However moving this to the wallet tool means that we will lose this behavior as that metadata will be lost and the wallet tool cannot do the rescan.

achow101 force-pushed on Aug 6, 2020

Remove -zapwallettxes startup option

Removes the startup option and replaces it with an error message telling
users to use the RPC instead.

aaa953633f

achow101 force-pushed on Aug 6, 2020

ryanofsky commented at 11:33 am on August 7, 2020: member

However moving this to the wallet tool means that we will lose this behavior as that metadata will be lost and the wallet tool cannot do the rescan.

No need for metadata to be lost I think. Wallet tool could do the equivalent of current zap command line option but instead of stashing metadata in a temporary variable, stash it in temporary rows. So the offline part of zap would clear BESTBLOCK and rename TX rows to ZAPTX rows. Then the rescan would scavenge and clear the ZAPTX rows.

DrahtBot added the label Needs rebase on Aug 12, 2020

DrahtBot commented at 3:10 pm on August 12, 2020: member

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

achow101 commented at 4:31 pm on August 12, 2020: member

Closing in favor of #19700

achow101 closed this on Aug 12, 2020

DrahtBot locked this on Feb 15, 2022

wallet: Replace -zapwallettxes with zapwallettxes RPC #19653

Conflicts