This seems reasonable if abandontransaction is a good alternative. I was also wondering if -zapwallettxes is fully working because it loops over m_spk_managers on the dummy wallet without calling SetupLegacyScriptPubKeyMan first

I was also wondering if -zapwallettxes is fully working because it loops over m_spk_managers on the dummy wallet without calling SetupLegacyScriptPubKeyMan first

It looks like that's not even a reachable code path as WalletBatch::ZapWalletTx can't return DBErrors::NEED_REWRITE.

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• Refactor: Improve setup_clean_chain semantics #19168 (Refactor: Improve setup_clean_chain semantics by fjahr)
• wallet: upgradewallet fixes and additional tests #18836 (wallet: upgradewallet fixes and additional tests by achow101)
• tests: Update more tests to work with descriptor wallets #18788 (tests: Update more tests to work with descriptor wallets by achow101)
• Add BIP324 encrypted p2p transport de-/serializer (only used in tests) #18242 (Add BIP324 encrypted p2p transport de-/serializer (only used in tests) by jonasschnelli)

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.

Concept ACK

Removing all transactions from the wallet a sledge-hammer option. If there is any need for this it should be the wallet maintenance tool. But if its use is fully replaced by abandontransaction well, better to get rid of it.

If we do this, please keep the option as hidden option and exit with an error explaining to use abandontransaction if it is passed. People might find this option documented in old guides and such, it can be somewhat frustrating if options just disappear.

FWIW I asked on twitter and mastodon what people's uses are with this option. Nothing really surprising there. No one is screaming to keep it. @luke-jr mentions something about a RBF override for abandontranscation that would be a remaining use case for this option.

@luke-jr mentions something about a RBF override for abandontranscation that would be a remaining use case for this option.

To forcibly remove a particular transaction, people can use removeprunedfunds. It's intended use is for pruned wallets, but the RPC doesn't check for that and just removes the given transaction regardless. So if abandontransaction doesn't work, then removeprunedfunds will.

Concept ACK, I agree entirely with @laanwj and prefer this over #19700. I see no point in keeping this in either the wallet tool or the RPC.

Concept ACK

Let's move forward with this and not draw out the discussion too much. I think we can just remove this. I received no strong disagreements on my twitter/mastodon posts.

We can always add it back (to the wallet tool, as does #19700) if we receive reports of people missing it.

Concept ACK

Removing likely broken likely unused options make perfect sense :)

Also: +6 −182 gives a warm fuzzy feeling.

Code review ACK 6738e34

ACK 6738e34 -- patch looks correct

Restarted Travis to get rid of timeout

Looks good, but there's still DBErrors ZapWalletTx(std::list<CWalletTx>& vWtx); in wallet.h and walletdb.h.

Can you also add a release note after you've dropped them?

Looks good, but there's still DBErrors ZapWalletTx(std::list<CWalletTx>& vWtx); in wallet.h and walletdb.h.

Can you also add a release note after you've dropped them?

Done and done.

I don't see any problem in removing this. But the release note asserts that this functionality was added for the purpose of replacing transactions that didn't have RBF. Rather, the functionality was added to help rescue wallets that had been messed up by malleability attack: #3659 (comment)

It's my (non-researched) belief that the malleability motivation is no longer a reason to keep the functionality, because although wallets with non-segwit coins are still malleability-vulnerable, I believe other changes mean that conflicted transactions can just be abandoned. If there is another rash of malleability attacks, users will have to be advised to use abandon on the unsuccessful mutants rather than be advised to zap as was the case when these attacks were an issue, along with spendzeroconfchange=false (or a narrower version that only avoids vulnerable inputs, if one is ever implemented).

But the release note asserts that this functionality was added for the purpose of replacing transactions that didn't have RBF. Rather, the functionality was added to help rescue wallets that had been messed up by malleability attack

Thanks for pointing that out, I've updated the release notes draft.