wallet: Replace -zapwallettxes with zapwallettxes RPC#19653
wallet: Replace -zapwallettxes with zapwallettxes RPC#19653achow101 wants to merge 2 commits intobitcoin:masterfrom
Conversation
achow101
force-pushed
the
zapwallettxes-rpc
branch
from
05b30c8 to
6f6e75a
Compare
achow101
force-pushed
the
zapwallettxes-rpc
branch
from
6f6e75a to
7c9971a
Compare
Updates the test to use this RPC. The test will also use multiple wallets instead of multiple nodes
achow101
force-pushed
the
zapwallettxes-rpc
branch
from
7c9971a to
e61f869
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
maflcko
left a comment
maflcko
left a comment
There was a problem hiding this comment.
Concept ACK (feel free to ignore style nit)
src/wallet/rpcwallet.cpp
Outdated
There was a problem hiding this comment.
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)
|
@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. |
|
If you forget to start with |
|
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
the
zapwallettxes-rpc
branch
from
e61f869 to
38ab0c1
Compare
Removes the startup option and replaces it with an error message telling users to use the RPC instead.
achow101
force-pushed
the
zapwallettxes-rpc
branch
from
38ab0c1 to
aaa9536
Compare
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 |
|
🐙 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". |
|
Closing in favor of #19700 |
5 participants
-zapwallettxesis known to not work with multiwallet and is currently disabled if multiple wallets are being loaded.This PR removes the
-zapwallettxesstartup option and replaces its functionality with azapwallettxesRPC 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
-zapwallettxeswould 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 doingzapwallettxesand restarting with-persistmempool=0.Also tests are updated to use the new RPC.