New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Control mempool persistence using a command line parameter #9966

Merged
merged 2 commits into from May 3, 2017

Conversation

Projects
None yet
9 participants
@jnewbery
Member

jnewbery commented Mar 9, 2017

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.

@fanquake fanquake added the Mempool label Mar 9, 2017

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Mar 9, 2017

Member

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.

Member

luke-jr commented Mar 9, 2017

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.

Show outdated Hide outdated src/init.cpp Outdated
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 10, 2017

Member

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.

Member

laanwj commented Mar 10, 2017

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

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Mar 10, 2017

Contributor

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

Contributor

paveljanik commented Mar 10, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 10, 2017

Member

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.

Member

laanwj commented Mar 10, 2017

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!

Show outdated Hide outdated qa/rpc-tests/mempool_persist.py Outdated
@jnewbery

This comment has been minimized.

Show comment
Hide comment
Member

jnewbery commented Mar 10, 2017

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Mar 10, 2017

Contributor

utACK c4e6154

Contributor

TheBlueMatt commented Mar 10, 2017

utACK c4e6154

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Mar 10, 2017

Contributor

ACK c4e6154

Contributor

paveljanik commented Mar 10, 2017

ACK c4e6154

Show outdated Hide outdated src/init.cpp Outdated
@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Mar 12, 2017

Member

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

Member

sipa commented Mar 12, 2017

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

@jnewbery

This comment has been minimized.

Show comment
Hide comment
@jnewbery

jnewbery Mar 15, 2017

Member

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

Member

jnewbery commented Mar 15, 2017

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

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Mar 18, 2017

Contributor

re-ACK d22d7b0

Contributor

paveljanik commented Mar 18, 2017

re-ACK d22d7b0

Control mempool persistence using a command line parameter.
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

This comment has been minimized.

Show comment
Hide comment
@jnewbery

jnewbery Mar 22, 2017

Member

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.

Member

jnewbery commented Mar 22, 2017

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

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Mar 22, 2017

Contributor

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

Contributor

paveljanik commented Mar 22, 2017

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

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

This comment has been minimized.

Show comment
Hide comment
@jnewbery

jnewbery Mar 22, 2017

Member

Thanks @paveljanik! Fixed

Member

jnewbery commented Mar 22, 2017

Thanks @paveljanik! Fixed

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Mar 22, 2017

Contributor

tACK a750d77

Contributor

paveljanik commented Mar 22, 2017

tACK a750d77

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

This comment has been minimized.

@jtimon

jtimon Mar 25, 2017

Member

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

@jtimon

jtimon Mar 25, 2017

Member

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

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Mar 25, 2017

Member

Didn't read the tests utACK a750d77

Member

jtimon commented Mar 25, 2017

Didn't read the tests utACK a750d77

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@laanwj laanwj referenced this pull request May 3, 2017

Closed

TODO for release notes 0.15.0 #9889

12 of 12 tasks complete

laanwj added a commit that referenced this pull request May 3, 2017

Merge #9966: Control mempool persistence using a command line parameter
a750d77 Add tests for mempool persistence (John Newbery)
91c91e1 Control mempool persistence using a command line parameter. (John Newbery)

Tree-SHA512: 157d01cefd1903b8bfc5cbab42a3cc5e9c1094179bf4b64b3d34c0d4d9b976d593755bfea5c41c631cb758e1de17c6c2058c130d487d20560b7c0bafcddfa520

@jnewbery jnewbery deleted the jnewbery:mempoolpersistenceoption branch Jun 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment