How should -zapwallettxes interact with persistent mempool? #9710

1. 
   jnewbery commented at 4:10 pm on February 7, 2017: member

   the zapwallettxes.py qa test is currently broken due to a syntax error:

   0     assert_raises(JSONRPCException, self.nodes[0].gettransaction, [txid3])
   1     #there must be a expection because the unconfirmed wallettx0 must be gone by now
   

   This doesn’t actually test that the transaction is not in the wallet, because gettransaction() is being called incorrectly (with an array instead of a string).

   Changing the test to assert_raises(JSONRPCException, self.nodes[0].gettransaction, txid3) causes the rpc to succeed and the test script to fail because txid3 is still in the wallet.

   This is because the mempool is now persistent and is saved to disk when bitcoind shuts down. zapwallettxes removes the unconfirmed transaction from the wallet but bitcoind will rescan the mempool.dat file on startup and add the unconfirmed transaction back into the wallet.

   I’ve ‘fixed’ zapwallettxes.py in https://github.com/jnewbery/bitcoin/tree/zapwallettxes , but the question remains: how should -zapwallettxes interact with persistent mempool? The use case for zapwallettxes is explained here: #3659 (comment) . My view is that even with zapwallettxes, bitcoind should rescan the mempool, but there should be better documentation or a warning that unconfirmed transaction in your mempool will be re-added to the wallet even if zapwallettxes is used. If you really want to remove all unconfirmed transactions from your wallet, you can delete your mempool.dat file and then restart with zapwallettxes.

2. laanwj added the label Wallet on Feb 7, 2017
3. laanwj added the label Tests on Feb 7, 2017
4. 
   luke-jr commented at 9:15 pm on February 9, 2017: member

   Maybe we should have an option -mempoolerase or something.
5. 
   jnewbery commented at 10:31 pm on February 9, 2017: member

   @luke-jr yes that might be helpful.

   To start with I’ll add a warning message when someone starts bitcoind with -zapwallettxes and there’s a mempool.dat file in their data directory. Something like:

   “bitcoind started with -zapwallettxes, but there is a mempool file in the data directory. Unconfirmed transactions in the mempool file may be scanned and re-added to the wallet. To remove all unconfirmed transactions, you may wish to delete the mempool.dat file from your data directory.”

   If users are finding it too onerous to remove the mempool.dat file manually we could add a -mempoolerase option.

   Sound good?

6. 
   jonasschnelli commented at 1:38 pm on February 12, 2017: contributor

   What about just not loading (=deleting/resetting) the mempool.dat-dump when -zapwallettxes=<n> was passed? Mention the new behaviour in the -zapwallettxes help/documentation.
7. 
   jnewbery commented at 7:10 pm on February 23, 2017: member

   @jonasschnelli you have much more experience of the wallet than I do. Not reloading the mempool when -zapwallettxes is set sounds like reasonable behaviour.

   Any other suggestions before I prepare a fix? @sipa you introduced the persistent mempool. @laanwj you provided the use case for zapwallettxes in #3659. Does not loading mempool when zapwallettxes is set sound like reasonable behaviour?

8. 
   laanwj commented at 10:37 am on February 24, 2017: member

   Yes, that may make sense. I’d implement it like this:

   • Add a new option to delete/ignore the mempool at startup.
   • Change parameter interaction so that -zapwallettxes implies that option.

   This leaves flexibility to do othewise, avoids a tight coupling between a wallet and mempool option, but makes the default case to get rid of transactions: which is generally what people want when zapping the transactions.

9. 
   jnewbery commented at 5:46 pm on May 3, 2017: member

   #9966 was a pre-req for fixing this. It’s now been merged so I’ve opened a PR to fix this issue at #10330
10. 
    Xekyo commented at 7:52 pm on May 13, 2017: member

    I just wanted to mention that a user on Bitcoin.Stackexchange complained that -zapwallettxes didn’t work as expected due to the mempool reload reintroducing the transaction to his wallet, and that a default behavior of -zapwallettxes at least blocking the own transactions from being loaded would be expected. :)
11. MarcoFalke closed this on Jul 17, 2017

    ---

12. MarcoFalke referenced this in commit bf0a08be28 on Jul 17, 2017
13. PastaPastaPasta referenced this in commit 9d50fabb4f on Jul 6, 2019
14. PastaPastaPasta referenced this in commit a4a6cacac8 on Jul 8, 2019
15. PastaPastaPasta referenced this in commit 7cffb17b9c on Jul 9, 2019
16. PastaPastaPasta referenced this in commit cebb3ce183 on Jul 11, 2019
17. PastaPastaPasta referenced this in commit 8867e49d49 on Jul 13, 2019
18. PastaPastaPasta referenced this in commit 61c0720ee5 on Jul 13, 2019
19. PastaPastaPasta referenced this in commit c67fd259e0 on Jul 17, 2019
20. PastaPastaPasta referenced this in commit 7471bafea7 on Jul 23, 2019
21. PastaPastaPasta referenced this in commit ea07a52136 on Jul 24, 2019
22. barrystyle referenced this in commit 144e663c4c on Jan 22, 2020
23. MarcoFalke locked this on Sep 8, 2021

Contributors
jnewbery luke-jr jonasschnelli laanwj Xekyo

Labels
Wallet Tests