[wallet] fix zapwallettxes interaction with persistent mempool #10330

jnewbery · 2017-05-03T17:45:14Z

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:

     assert_raises(JSONRPCException, self.nodes[0].gettransaction, [txid3])
     #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 · 2017-05-04T06:10:05Z

utACK cfc234bf063ee2c3598e3638252dd7491d61cb49.

jnewbery · 2017-06-06T16:41:13Z

rebased

jnewbery · 2017-06-18T16:22:17Z

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!

maflcko · 2017-06-18T19:16:21Z

utACK 15e72f5

sipa · 2017-06-20T20:26:32Z

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

maflcko · 2017-06-20T20:51:27Z

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

jnewbery · 2017-06-27T14:08:06Z

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 · 2017-06-27T14:11:36Z

rebased

maflcko · 2017-07-02T15:27:21Z

Needs rebase

jnewbery · 2017-07-02T21:03:56Z

rebased

achow101 · 2017-07-12T22:32:42Z

utACK 46d9a4b151cfb88011029af1310e87c5cbbd87ec

morcos · 2017-07-13T17:57:52Z

utACK 46d9a4b

TheBlueMatt

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

TheBlueMatt · 2017-07-14T20:40:08Z

src/wallet/wallet.cpp

@@ -4010,6 +4010,11 @@ bool CWallet::ParameterInteraction()
        LogPrintf("%s: parameter interaction: -salvagewallet=1 -> setting -rescan=1\n", __func__);
    }

+    // -zapwallettx implies dropping the mempool on startup
+    if (GetBoolArg("-zapwallettxes", false) && SoftSetBoolArg("-persistmempool", false)) {


"=<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.

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!

sipa · 2017-07-15T19:03:43Z

Needs rebase.

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.

jnewbery · 2017-07-15T19:32:31Z

rebased

TheBlueMatt · 2017-07-16T15:55:22Z

utACK 4c3b538

maflcko · 2017-07-16T22:04:47Z

re-utACK 4c3b538

…mempool 4c3b538 [logs] fix zapwallettxes startup logs (John Newbery) e7a2181 [wallet] fix zapwallettxes interaction with persistent mempool (John Newbery) ff7365e [tests] fix flake8 warnings in zapwallettxes.py (John Newbery) Pull request description: 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: ```py assert_raises(JSONRPCException, self.nodes[0].gettransaction, [txid3]) #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 . Tree-SHA512: e3236efc7a2fd2b3bf1d9e2e8a7726d470c57f5d95cf41b7bde264edc8817bd36a6f3feff52f8de8db0ef64b7247c88b24e7ff7cefaa706cba86fe4e2135a508

…istent mempool 4c3b538 [logs] fix zapwallettxes startup logs (John Newbery) e7a2181 [wallet] fix zapwallettxes interaction with persistent mempool (John Newbery) ff7365e [tests] fix flake8 warnings in zapwallettxes.py (John Newbery) Pull request description: 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: ```py assert_raises(JSONRPCException, self.nodes[0].gettransaction, [txid3]) #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 bitcoin#9710 . Tree-SHA512: e3236efc7a2fd2b3bf1d9e2e8a7726d470c57f5d95cf41b7bde264edc8817bd36a6f3feff52f8de8db0ef64b7247c88b24e7ff7cefaa706cba86fe4e2135a508

9b9c616 Fix missing zapwallettxes mode in wallet_hd.py functional test (furszy) d6d0ad9 [logs] fix zapwallettxes startup logs (John Newbery) 006c503 [wallet] fix zapwallettxes interaction with persistent mempool (John Newbery) c6d45c6 Adapting and connecting mempool_persist.py functional test to the test runner. (furszy) 4f26a4e Control mempool persistence using a command line parameter. (John Newbery) 5d949de [Qt] Do proper shutdown (Jonas Schnelli) e60da98 Allow shutdown during LoadMempool, dump only when necessary (Jonas Schnelli) c0a0e81 Moving TxMempoolInfo tx member to CTransactionRef (furszy) f0c2255 Add mempool.dat to doc/files.md (furszy) 8e52226 Add DumpMempool and LoadMempool (Pieter Wuille) 44c635d Add AcceptToMemoryPoolWithTime function (Pieter Wuille) 6bbc6a9 Add feedelta to TxMempoolInfo (Pieter Wuille) 9979f3d [mempool] move removed and conflicts transaction ref list to vector. (furszy) 4f672c2 Make removed and conflicted arguments optional to remove (Pieter Wuille) e51c4b8 Bypass removeRecursive in removeForReorg (Pieter Wuille) 54cf7c0 Get rid of CTxMempool::lookup() entirely (furszy) 35bc2a9 Finished the switch CTransaction storage in mempool to CTransactionRef and introduced the timeLastMempoolReq coming from btc#8080. (furszy) d10583b An adapted version of btc@b5599147533103efea896a1fc4ff51f2d3ad5808 (furszy) cb4fc6c An adapted version of btc@ed7068302c7490e8061cb3a558a0f83a465beeea (furszy) 9645775 Split up and optimize transaction and block inv queues (furszy) 68bc68f Don't do mempool lookups for "mempool" command without a filter (furszy) 7624823 mapNextTx: use pointer as key, simplify value (furszy) 191c62e Return mempool queries in dependency order (Pieter Wuille) 23c9f3e Eliminate TX trickle bypass, sort TX invs for privacy and priority. (furszy) 6ebfd17 tiny test fix for mempool_tests (Alex Morcos) 8c0016e Check all ancestor state in CTxMemPool::check() (furszy) 91c6096 Add ancestor feerate index to mempool (Suhas Daftuar) 64e84e2 Add ancestor tracking to mempool This implements caching of ancestor state to each mempool entry, similar to descendant tracking, but also including caching sigops-with-ancestors (as that metric will be helpful to future code that implements better transaction selection in CreatenewBlock). (furszy) 8325bb4 Fix mempool limiting for PrioritiseTransaction Redo the feerate index to be based on mining score, rather than fee. (furszy) 1fa40ac Remove work limit in UpdateForDescendants() The work limit served to prevent the descendant walking algorithm from doing too much work by marking the parent transaction as dirty. However to implement ancestor tracking, it's not possible to similarly mark those descendant transactions as dirty without having to calculate them to begin with. This commit removes the work limit altogether. With appropriate chain limits (-limitdescendantcount) the concern about doing too much work inside this function should be mitigated. (furszy) ba32375 Rename CTxMemPool::remove -> removeRecursive remove is no longer called non-recursively, so simplify the logic and eliminate an unnecessary parameter (furszy) c30fa16 CTxMemPool::removeForBlock now uses RemoveStaged (furszy) Pull request description: Ending up 2020 with a large PR :). Included a good number of performance and privacy improvements over the mempool and inv processing/sending areas + added mempool cache dump/load. Almost finishing with #1726 work, getting closer to 9725, and getting closer to be able to decouple the message processing thread <-> wallet and the wallet <-> GUI locks dependencies (which will be another long story.. but well, step by step). The final goal is clear, a much faster syncing process using pivx-qt (a good speed up for pivxd as well), smoother visual navigation when big wallets are syncing, less thread synchronization issues, among all of the improvements that will be coming with the backports adaptations. Adapted the following PRs to our sources: bitcoin#7062 —> only eb30666. bitcoin#7174 —> complete. bitcoin#7562 —> only c5d746a. bitcoin#7594 —> complete. bitcoin#7840 —> complete. bitcoin#7997 —> complete. bitcoin#8080 —> complete bitcoin#8126 —> except e9b4780 (we don't have the mapRelay) and c2a4724 (we don't have the relay expiration vector). bitcoin#8448 —> complete bitcoin#8515 —> complete bitcoin#9408 —> complete bitcoin#9966 —> complete bitcoin#10330 —> complete ACKs for top commit: random-zebra: ACK 9b9c616 Fuzzbawls: ACK 9b9c616 Tree-SHA512: 186bd09bbb19b55ec0d46dc27291663a0df2d60d8238c661a8c9b8cbf3677b6f8a92fa56bd7346a3cb5293712eeccc5d6542ee3c441d35a4f61e7d12e2ce489a

jnewbery mentioned this pull request May 3, 2017

How should -zapwallettxes interact with persistent mempool? #9710

Closed

jonasschnelli added the Wallet label May 4, 2017

jnewbery force-pushed the zapwallettxes branch from cfc234b to 15e72f5 Compare June 6, 2017 16:41

maflcko added this to the 0.15.0 milestone Jun 20, 2017

jnewbery force-pushed the zapwallettxes branch from 15e72f5 to b035f30 Compare June 27, 2017 14:11

jnewbery changed the title ~~fix zapwallettxes interaction with persistent mempool~~ [wallet] fix zapwallettxes interaction with persistent mempool Jun 30, 2017

jnewbery force-pushed the zapwallettxes branch from b035f30 to 46d9a4b Compare July 2, 2017 21:03

This was referenced Jul 6, 2017

zapwallettxes.py test failure due to bitcoind return code #10678

Closed

[tests] skip zapwallettxes.py #10802

Closed

TheBlueMatt reviewed Jul 14, 2017

View reviewed changes

jnewbery added 3 commits July 15, 2017 15:15

[tests] fix flake8 warnings in zapwallettxes.py

ff7365e

[logs] fix zapwallettxes startup logs

4c3b538

jnewbery force-pushed the zapwallettxes branch from c0771ad to 4c3b538 Compare July 15, 2017 19:32

maflcko added the Tests label Jul 17, 2017

maflcko merged commit 4c3b538 into bitcoin:master Jul 17, 2017

jnewbery deleted the zapwallettxes branch August 10, 2017 15:36

jnewbery mentioned this pull request Aug 10, 2017

[wallet] getbalance: Add option to include non-mempool UTXOs #11020

Closed

jonasschnelli mentioned this pull request Feb 12, 2018

zapwallettxes should probably clear the mempoool? #12400

Closed

achow101 mentioned this pull request Aug 3, 2020

wallet: Replace -zapwallettxes with zapwallettxes RPC #19653

Closed

This was referenced Jan 18, 2021

Up to CTransactionRef point back ports to do list. PIVX-Project/PIVX#1726

Closed

[NET] Inv, mempool and validation improvements PIVX-Project/PIVX#2118

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

[wallet] fix zapwallettxes interaction with persistent mempool #10330

[wallet] fix zapwallettxes interaction with persistent mempool #10330

Uh oh!

Conversation

jnewbery commented May 3, 2017

Uh oh!

jonasschnelli commented May 4, 2017

Uh oh!

jnewbery commented Jun 6, 2017

Uh oh!

jnewbery commented Jun 18, 2017

Uh oh!

maflcko commented Jun 18, 2017

Uh oh!

sipa commented Jun 20, 2017

Uh oh!

maflcko commented Jun 20, 2017

Uh oh!

jnewbery commented Jun 27, 2017

Uh oh!

jnewbery commented Jun 27, 2017

Uh oh!

maflcko commented Jul 2, 2017

Uh oh!

jnewbery commented Jul 2, 2017

Uh oh!

achow101 commented Jul 12, 2017

Uh oh!

morcos commented Jul 13, 2017

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Jul 14, 2017

Choose a reason for hiding this comment

Uh oh!

jnewbery Jul 14, 2017

Choose a reason for hiding this comment

Uh oh!

sipa commented Jul 15, 2017

Uh oh!

jnewbery commented Jul 15, 2017

Uh oh!

TheBlueMatt commented Jul 16, 2017

Uh oh!

maflcko commented Jul 16, 2017

Uh oh!

Uh oh!