-
Notifications
You must be signed in to change notification settings - Fork 36.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
wallet: Replace -zapwallettxes with zapwallettxes RPC #19653
Conversation
05b30c8
to
6f6e75a
Compare
6f6e75a
to
7c9971a
Compare
Updates the test to use this RPC. The test will also use multiple wallets instead of multiple nodes
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK (feel free to ignore style nit)
@@ -4113,6 +4113,119 @@ static UniValue upgradewallet(const JSONRPCRequest& request) | |||
return error.original; | |||
} | |||
|
|||
static UniValue zapwallettxes(const JSONRPCRequest& request) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
e61f869
to
38ab0c1
Compare
Removes the startup option and replaces it with an error message telling users to use the RPC instead.
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 |
-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 azapwallettxes
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 doingzapwallettxes
and restarting with-persistmempool=0
.Also tests are updated to use the new RPC.