[wallet] fix zapwallettxes interaction with persistent mempool

jnewbery commented at 5:45 pm on May 3, 2017: member

zapwallettxes previously did not interact well with persistent mempool. zapwallettxes would cause wallet transactions to be zapped, but they would then be reloaded from the mempool on startup. This commit softsets persistmempool to false if zapwallettxes is enabled so transactions are actually zapped.

This PR also fixes the zapwallettxes.py functional test, which did not properly test this feature. The test line:

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

is not actually testing the presence of the transaction since the RPC is being called incorrectly (with an array instead of a string). The assert_raises() passes since an assert is raised, but it’s not the one the test writer had in mind!

Fixes #9710 .

jonasschnelli added the label Wallet on May 4, 2017

jonasschnelli commented at 6:10 am on May 4, 2017: contributor

utACK cfc234bf063ee2c3598e3638252dd7491d61cb49.

jnewbery force-pushed on Jun 6, 2017

jnewbery commented at 4:41 pm on June 6, 2017: member

rebased

jnewbery commented at 4:22 pm on June 18, 2017: member

Long-term, I think zapwallettxes should be a function in a standalone wallet tool (#8745). That’s probably some time off, and in the mean time this is an improvement in behaviour. Any feedback on concept or implementation welcomed!

MarcoFalke commented at 7:16 pm on June 18, 2017: member

utACK 15e72f5

MarcoFalke added this to the milestone 0.15.0 on Jun 20, 2017

sipa commented at 8:26 pm on June 20, 2017: member

utACK 15e72f57081b2aedd3dc488364d00f0d7c99ffe0. I haven’t reviewed the test changes.

MarcoFalke commented at 8:51 pm on June 20, 2017: member

Let’s assume persistmempool is set in bitcoin.conf. Should there be an InitWarning when the user tries to setzapwallettxes?

jnewbery commented at 2:08 pm on June 27, 2017: member

Let’s assume persistmempool is set in bitcoin.conf. Should there be an InitWarning when the user tries to setzapwallettxes?

Perhaps, although this is an unlikely scenario since:

persistmempool is a new in 0.15, so no-one can be using it yet
the default value for persistmempool is true, so there’s no reason to set it in bitcoin.conf

I’m inclined not to add the warning, but let me know if you feel strongly about it, and I can look at adding it.

jnewbery force-pushed on Jun 27, 2017

jnewbery commented at 2:11 pm on June 27, 2017: member

rebased

jnewbery renamed this:
~~fix zapwallettxes interaction with persistent mempool~~
[wallet] fix zapwallettxes interaction with persistent mempool
on Jun 30, 2017

MarcoFalke commented at 3:27 pm on July 2, 2017: member

Needs rebase

jnewbery force-pushed on Jul 2, 2017

jnewbery commented at 9:03 pm on July 2, 2017: member

rebased

achow101 commented at 10:32 pm on July 12, 2017: member

utACK 46d9a4b151cfb88011029af1310e87c5cbbd87ec

morcos commented at 5:57 pm on July 13, 2017: member

utACK 46d9a4b

in src/wallet/wallet.cpp:4014 in 46d9a4b151 outdated

4009@@ -4010,6 +4010,11 @@ bool CWallet::ParameterInteraction()
4010         LogPrintf("%s: parameter interaction: -salvagewallet=1 -> setting -rescan=1\n", __func__);
4011     }
4012 
4013+    // -zapwallettx implies dropping the mempool on startup
4014+    if (GetBoolArg("-zapwallettxes", false) && SoftSetBoolArg("-persistmempool", false)) {

TheBlueMatt commented at 8:40 pm on July 14, 2017:

“=<mode>” seems like a super strange string when <mode> isnt replaced out in the logs (plus its a boolean, what does <mode> mean?). zapwallettxes looks like the only one where it looks like this.

jnewbery commented at 10:19 pm on July 14, 2017:

Yes, you’re right - that is strange. I just copied the existing log below.

I’ve fixed them both so they actually print the value of zapwallettxes.

Oh, and surprise! zapwallettxes isn’t a bool!

TheBlueMatt commented at 8:43 pm on July 14, 2017: member

Didnt look too closely at the tests, but they look sane. Aside from the strange log message, utACK 46d9a4b151cfb88011029af1310e87c5cbbd87ec

sipa commented at 7:03 pm on July 15, 2017: member

Needs rebase.

[tests] fix flake8 warnings in zapwallettxes.py ff7365e780

[wallet] fix zapwallettxes interaction with persistent mempool

zapwallettxes previously did not interact well with persistent mempool.
zapwallettxes would cause wallet transactions to be zapped, but they
would then be reloaded from the mempool on startup. This commit softsets
persistmempool to false if zapwallettxes is enabled so transactions are
actually zapped.

e7a2181b49

[logs] fix zapwallettxes startup logs 4c3b538c61

jnewbery force-pushed on Jul 15, 2017

jnewbery commented at 7:32 pm on July 15, 2017: member

rebased

TheBlueMatt commented at 3:55 pm on July 16, 2017: member

utACK 4c3b538c61532dc68d79bbe34729759a13b73f0c

MarcoFalke commented at 10:04 pm on July 16, 2017: member

re-utACK 4c3b538

MarcoFalke added the label Tests on Jul 17, 2017

MarcoFalke merged this on Jul 17, 2017

MarcoFalke closed this on Jul 17, 2017

MarcoFalke referenced this in commit bf0a08be28 on Jul 17, 2017

jnewbery deleted the branch on Aug 10, 2017

PastaPastaPasta referenced this in commit 9d50fabb4f on Jul 6, 2019

PastaPastaPasta referenced this in commit a4a6cacac8 on Jul 8, 2019

PastaPastaPasta referenced this in commit 7cffb17b9c on Jul 9, 2019

PastaPastaPasta referenced this in commit cebb3ce183 on Jul 11, 2019

PastaPastaPasta referenced this in commit 8867e49d49 on Jul 13, 2019

PastaPastaPasta referenced this in commit 61c0720ee5 on Jul 13, 2019

PastaPastaPasta referenced this in commit c67fd259e0 on Jul 17, 2019

PastaPastaPasta referenced this in commit 7471bafea7 on Jul 23, 2019

PastaPastaPasta referenced this in commit ea07a52136 on Jul 24, 2019

barrystyle referenced this in commit 144e663c4c on Jan 22, 2020

furszy referenced this in commit 1f010c0969 on Jan 23, 2021

DrahtBot locked this on Sep 8, 2021

[wallet] fix zapwallettxes interaction with persistent mempool #10330