wallet: Remove -zapwallettxes

achow101 commented at 8:44 pm on August 5, 2020: member

It’s not clear what use there is to keeping -zapwallettxes given that it’s intended usage has been superseded by abandontransaction. So this removes it outright.

Alternative to #19700

ryanofsky commented at 9:43 pm on August 5, 2020: member

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

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

DrahtBot added the label Tests on Aug 5, 2020

DrahtBot added the label Wallet on Aug 5, 2020

achow101 commented at 11:00 pm on August 5, 2020: member

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.

DrahtBot commented at 0:17 am on August 6, 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:

#19168 (Refactor: Improve setup_clean_chain semantics by fjahr)
#18836 (wallet: upgradewallet fixes and additional tests by achow101)
#18788 (tests: Update more tests to work with descriptor wallets by achow101)
#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.

laanwj commented at 7:00 am on August 6, 2020: member

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.

achow101 force-pushed on Aug 6, 2020

laanwj commented at 3:56 pm on August 9, 2020: member

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.

achow101 commented at 4:41 pm on August 9, 2020: member

@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.

meshcollider commented at 0:07 am on August 13, 2020: contributor

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.

fanquake commented at 6:13 am on August 14, 2020: member

Concept ACK

fjahr commented at 9:19 pm on August 14, 2020: member

Concept ACK

MarcoFalke removed the label Tests on Aug 15, 2020

DrahtBot added the label Needs rebase on Aug 27, 2020

laanwj commented at 12:32 pm on August 27, 2020: member

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.

achow101 force-pushed on Aug 27, 2020

DrahtBot removed the label Needs rebase on Aug 27, 2020

practicalswift commented at 1:39 am on August 29, 2020: contributor

Concept ACK

Removing likely broken likely unused options make perfect sense :)

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

laanwj commented at 3:59 pm on August 30, 2020: member

Code review ACK 6738e3457824871c577bcdeb95bac599a3e09428

MarcoFalke added the label Needs release note on Aug 30, 2020

practicalswift commented at 6:00 pm on August 30, 2020: contributor

ACK 6738e3457824871c577bcdeb95bac599a3e09428 – patch looks correct

Restarted Travis to get rid of timeout

fanquake commented at 3:32 am on August 31, 2020: member

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?

Remove -zapwallettxes

-zapwallettxes is made a hidden option to inform users that it is
removed and they should be using abandontransaction to do the stuck
transaction thing.

3340dbadd3

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

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.

achow101 force-pushed on Aug 31, 2020

meshcollider commented at 10:44 pm on August 31, 2020: contributor

utACK 3340dbadd38f5624642cf0e14dddbe6f83a3863b

fanquake removed the label Needs release note on Sep 1, 2020

fanquake approved

fanquake commented at 1:25 am on September 1, 2020: member

ACK 3340dbadd38f5624642cf0e14dddbe6f83a3863b - remaining manpage references will get cleaned up pre-release.

fanquake merged this on Sep 1, 2020

fanquake closed this on Sep 1, 2020

sidhujag referenced this in commit d996825ddb on Sep 3, 2020

gmaxwell commented at 2:57 pm on January 10, 2021: contributor

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).

achow101 commented at 5:41 pm on January 10, 2021: member

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.

deadalnix referenced this in commit 64f1961e18 on Sep 22, 2021

kittywhiskers referenced this in commit 100d70a4ae on Apr 12, 2022

kittywhiskers referenced this in commit 8834463594 on Apr 12, 2022

kittywhiskers referenced this in commit 18b9699ad9 on Apr 13, 2022

kittywhiskers referenced this in commit b926bad8a9 on Apr 16, 2022

kittywhiskers referenced this in commit 0626248938 on Apr 20, 2022

kittywhiskers referenced this in commit 89fe1c2707 on May 7, 2022

kittywhiskers referenced this in commit a48ecb7228 on May 9, 2022

kittywhiskers referenced this in commit 01d00f3227 on May 13, 2022

malbit referenced this in commit c51b219273 on Aug 7, 2022

DrahtBot locked this on Aug 16, 2022

wallet: Remove -zapwallettxes #19671

Conflicts