Control mempool persistence using a command line parameter #9966

jnewbery · 2017-03-09T22:19:11Z

Mempool persistence was added in
3f78562, and is always on. This commit
introduces a command-line parameter -persistmempool, which defaults to
true. When set to false:

mempool.dat is not loaded when the node starts.
mempool.dat is not written when the node stops.

This was suggested in the PR that introduced mempool persistence (#8448 (comment) and #8448 (comment)) , but wasn't implemented in the original PR. It is a pre-req for fixing #9710.

This PR also introduces tests for mempool persistence, as well as for the new command-line parameter.

luke-jr · 2017-03-09T22:20:39Z

Concept ACK.

Idea: Is there any reason a user might want to load or dump but not both?

Maybe it should be a debug option... dunno.

laanwj · 2017-03-10T06:58:44Z

src/init.cpp

@@ -374,6 +375,7 @@ std::string HelpMessage(HelpMessageMode mode)
    strUsage += HelpMessageOpt("-maxtimeadjustment", strprintf(_("Maximum allowed median peer time offset adjustment. Local perspective of time may be influenced by peers forward or backward by this amount. (default: %u seconds)"), DEFAULT_MAX_TIME_ADJUSTMENT));
    strUsage += HelpMessageOpt("-onion=<ip:port>", strprintf(_("Use separate SOCKS5 proxy to reach peers via Tor hidden services (default: %s)"), "-proxy"));
    strUsage += HelpMessageOpt("-onlynet=<net>", _("Only connect to nodes in network <net> (ipv4, ipv6 or onion)"));
+    strUsage += HelpMessageOpt("-persistmempool", strprintf(_("Whether to dump the mempool on shutdown and load on restart(default: %u)"), DEFAULT_PERSIST_MEMPOOL));


I'd use "save" instead of "dump" especially if this is to be an official translated option and not a debug option. Also missing a space before (

laanwj · 2017-03-10T06:59:00Z

utACK apart from the nit, seems straightforward enough.

We should indeed consider the debug/non-debug status of this option. Is it worth adding translation strings for this, or is it just for technical debugging/development purposes? Not entirely decided about this.

paveljanik · 2017-03-10T07:03:50Z

@luke-jr there is. I had the same idea while debugging #9810. I wanted to load the original mempool.dat, but not rewrite it at the exit. I solved it by copying mempool.dat back before the next start. And as the workaround is so simple, please let's not complicate the code with yet another option.

laanwj · 2017-03-10T07:04:18Z

And as the workaround is so simple, please let's not complicate the code with yet another option.

Agreed. So to be clear adding this option is good, but let's not add micro-management for loading/saving specifically.

paveljanik

tested ACK 89acdb0

I especially like the test and its description!

paveljanik · 2017-03-10T07:05:03Z

qa/rpc-tests/mempool_persist.py

@@ -0,0 +1,89 @@
+#!/usr/bin/env python3
+# Copyright (c) 2014-2016 The Bitcoin Core developers


jnewbery · 2017-03-10T15:03:40Z

Nits addressed and squashed.

Old tip: https://github.com/jnewbery/bitcoin/tree/pr9966.1 / jnewbery@89acdb0
New tip: https://github.com/jnewbery/bitcoin/tree/pr9966.2 / jnewbery@c4e6154

TheBlueMatt · 2017-03-10T15:59:25Z

utACK c4e6154

paveljanik · 2017-03-10T22:02:11Z

ACK c4e6154

maflcko · 2017-03-10T22:24:13Z

src/init.cpp

@@ -374,6 +375,7 @@ std::string HelpMessage(HelpMessageMode mode)
    strUsage += HelpMessageOpt("-maxtimeadjustment", strprintf(_("Maximum allowed median peer time offset adjustment. Local perspective of time may be influenced by peers forward or backward by this amount. (default: %u seconds)"), DEFAULT_MAX_TIME_ADJUSTMENT));
    strUsage += HelpMessageOpt("-onion=<ip:port>", strprintf(_("Use separate SOCKS5 proxy to reach peers via Tor hidden services (default: %s)"), "-proxy"));
    strUsage += HelpMessageOpt("-onlynet=<net>", _("Only connect to nodes in network <net> (ipv4, ipv6 or onion)"));
+    strUsage += HelpMessageOpt("-persistmempool", strprintf(_("Whether to save the mempool on shutdown and load on restart (default: %u)"), DEFAULT_PERSIST_MEMPOOL));


I might be missing something, but this is not a "connection option". Also, this could be formatted as an untranslated debug option.

Good catch, Marco! It should be moved out from "Connection options" help, e.g. after the mempoolexpiry help?

I think it should be normal option, non-debug, ie. translated.

sipa · 2017-03-12T06:28:56Z

utACK, but move the option in the help section as suggested.

jnewbery · 2017-03-15T15:44:40Z

Thanks @MarcoFalke . I've moved the new option to the general options section.

New tip: https://github.com/jnewbery/bitcoin/tree/pr9966.3 / jnewbery@d22d7b0

paveljanik · 2017-03-18T08:21:16Z

re-ACK d22d7b0

Mempool persistence was added in 3f78562, and is always on. This commit introduces a command-line parameter -persistmempool, which defaults to true. When set to false: - mempool.dat is not loaded when the node starts. - mempool.dat is not written when the node stops.

jnewbery · 2017-03-22T15:21:54Z

rebased https://github.com/jnewbery/bitcoin/tree/pr9966.3 -> https://github.com/jnewbery/bitcoin/tree/pr9966.4

Are any reviewers able to reACK? All review comments are addressed, so this should be ready to merge.

paveljanik · 2017-03-22T15:46:05Z

I think that qa/rpc-tests no longer exists in the master.

Adds tests for mempool persistence as well as for the new -persistmempool command line parameter.

jnewbery · 2017-03-22T15:57:57Z

Thanks @paveljanik! Fixed

paveljanik · 2017-03-22T16:39:14Z

tACK a750d77

jtimon · 2017-03-25T13:09:24Z

src/init.cpp

-    fDumpMempoolLater = !fRequestShutdown;
+    if (GetArg("-persistmempool", DEFAULT_PERSIST_MEMPOOL)) {
+        LoadMempool();
+        fDumpMempoolLater = !fRequestShutdown;


You can do else fDumpMempoolLater = false to avoid the extra GetArg("-persistmempool", DEFAULT_PERSIST_MEMPOOL) in Shutdown()

jtimon · 2017-03-25T13:10:04Z

Didn't read the tests utACK a750d77

a750d77 Add tests for mempool persistence (John Newbery) 91c91e1 Control mempool persistence using a command line parameter. (John Newbery) Tree-SHA512: 157d01cefd1903b8bfc5cbab42a3cc5e9c1094179bf4b64b3d34c0d4d9b976d593755bfea5c41c631cb758e1de17c6c2058c130d487d20560b7c0bafcddfa520

…parameter a750d77 Add tests for mempool persistence (John Newbery) 91c91e1 Control mempool persistence using a command line parameter. (John Newbery) Tree-SHA512: 157d01cefd1903b8bfc5cbab42a3cc5e9c1094179bf4b64b3d34c0d4d9b976d593755bfea5c41c631cb758e1de17c6c2058c130d487d20560b7c0bafcddfa520

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

fanquake added the Mempool label Mar 9, 2017

laanwj reviewed Mar 10, 2017

View reviewed changes

paveljanik approved these changes Mar 10, 2017

View reviewed changes

jnewbery force-pushed the mempoolpersistenceoption branch from 89acdb0 to c4e6154 Compare March 10, 2017 15:03

maflcko reviewed Mar 10, 2017

View reviewed changes

jnewbery force-pushed the mempoolpersistenceoption branch from c4e6154 to d22d7b0 Compare March 15, 2017 15:43

jnewbery force-pushed the mempoolpersistenceoption branch from d22d7b0 to fd0da4b Compare March 22, 2017 15:20

Add tests for mempool persistence

a750d77

Adds tests for mempool persistence as well as for the new -persistmempool command line parameter.

jnewbery force-pushed the mempoolpersistenceoption branch from fd0da4b to a750d77 Compare March 22, 2017 15:55

jtimon reviewed Mar 25, 2017

View reviewed changes

laanwj merged commit a750d77 into bitcoin:master May 3, 2017

laanwj mentioned this pull request May 3, 2017

TODO for release notes 0.15.0 #9889

Closed

12 tasks

jnewbery mentioned this pull request May 3, 2017

How should -zapwallettxes interact with persistent mempool? #9710

Closed

jnewbery deleted the mempoolpersistenceoption branch June 27, 2017 14:06

This was referenced Jan 14, 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

		@@ -0,0 +1,89 @@
		#!/usr/bin/env python3
		# Copyright (c) 2014-2016 The Bitcoin Core developers

Control mempool persistence using a command line parameter #9966

Control mempool persistence using a command line parameter #9966

Uh oh!

Conversation

jnewbery commented Mar 9, 2017

Uh oh!

luke-jr commented Mar 9, 2017

Uh oh!

laanwj Mar 10, 2017

Choose a reason for hiding this comment

Uh oh!

laanwj commented Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paveljanik commented Mar 10, 2017

Uh oh!

laanwj commented Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paveljanik left a comment

Choose a reason for hiding this comment

Uh oh!

paveljanik Mar 10, 2017

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt commented Mar 10, 2017

Uh oh!

paveljanik commented Mar 10, 2017

Uh oh!

maflcko Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paveljanik Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Mar 12, 2017

Uh oh!

jnewbery commented Mar 15, 2017

Uh oh!

paveljanik commented Mar 18, 2017

Uh oh!

jnewbery commented Mar 22, 2017

Uh oh!

paveljanik commented Mar 22, 2017

Uh oh!

jnewbery commented Mar 22, 2017

Uh oh!

paveljanik commented Mar 22, 2017

Uh oh!

jtimon Mar 25, 2017

Choose a reason for hiding this comment

Uh oh!

jtimon commented Mar 25, 2017

Uh oh!

Uh oh!

laanwj commented Mar 10, 2017 •

edited

Loading

laanwj commented Mar 10, 2017 •

edited

Loading

jnewbery commented Mar 10, 2017 •

edited

Loading

maflcko Mar 10, 2017 •

edited

Loading

paveljanik Mar 10, 2017 •

edited

Loading