/bitcoin

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

Already on GitHub? Sign in to your account

[RPC][mempool]: Add savemempool RPC #11099

Merged
merged 2 commits into from Sep 6, 2017
+53 −3

Conversation

Reviewers

@jnewbery jnewbery

@promag promag

@MarcoFalke MarcoFalke

@jonasschnelli jonasschnelli

Assignees
No one assigned
Labels
RPC/REST/ZMQ
Projects
None yet
Milestone
No milestone
10 participants
@greenaddress @promag @jonasschnelli @MarcoFalke @jnewbery @sipa @deno49 @MeshCollider @luke-jr @fanquake
@greenaddress
Contributor

greenaddress commented Aug 20, 2017 •
Edited 1 time

Adds a simple parameterless rpc command to dump the mempool.

Rationale:

Sometimes there can be a crash for whatever reason (bug, power loss, etc) causing the mempool.dat file to not be saved.

This change allows to script/cron the rpc call to have more regular saves to the file as well as cli/ad-hoc.

This should solve issue #11086
@promag

promag reviewed Aug 20, 2017
View changes

Please add a test.

The function can be changed to know if it failed or not.

src/rpc/blockchain.cpp
+ if (request.fHelp || request.params.size() > 0)
+ throw std::runtime_error(
+ "dumpmempool\n"
+ "\nDumps to disk the mempool (mempool.dat).\n"
@promag

promag Aug 20, 2017

Contributor

Remove the filename?

src/rpc/blockchain.cpp
+ );
+
+ DumpMempool();
+ return "Mempool dumped";
@promag

promag Aug 20, 2017

Contributor

This is not correct since it can fail.

@promag
Contributor

promag commented Aug 20, 2017

Suggestion, reword Add dumpmempool RPC.

@fanquake fanquake added the RPC/REST/ZMQ label Aug 20, 2017

@greenaddress greenaddress changed the title from [RPC][mempool]: add rpc command to dump the mempool to disk to [RPC][mempool]: Add dumpmempool RPC Aug 21, 2017

@greenaddress
Contributor

greenaddress commented Aug 21, 2017

@promag thanks, updated as for feedback and squashed
@promag

promag reviewed Aug 21, 2017
View changes

I wonder if it should throw RPC_INTERNAL_ERROR, since DumpMempool handles it's errors.

Please split in two commits:

  • Add return value to DumpMempool
  • Add dumpmempool RPC.
src/rpc/blockchain.cpp
+ + HelpExampleRpc("dumpmempool", "")
+ );
+
+ if (!DumpMempool())
@promag

promag Aug 21, 2017

Contributor

Missing { }.

src/rpc/blockchain.cpp
+
+ if (!DumpMempool())
+ throw JSONRPCError(RPC_INTERNAL_ERROR, "Unable to dump mempool to disk");
+ return "Mempool dumped";
@promag

promag Aug 21, 2017

Contributor

Nit, new line before return.

src/rpc/blockchain.cpp
@@ -1552,6 +1568,7 @@ static const CRPCCommand commands[] =
{ "blockchain", "gettxoutsetinfo", &gettxoutsetinfo, true, {} },
{ "blockchain", "pruneblockchain", &pruneblockchain, true, {"height"} },
{ "blockchain", "verifychain", &verifychain, true, {"checklevel","nblocks"} },
+ { "blockchain", "dumpmempool", &dumpmempool, true, {} },
@promag

promag Aug 21, 2017 •
Edited 1 time

Contributor

Sort, move before getblockchaininfo.

@greenaddress
Contributor

greenaddress commented Aug 21, 2017

@promag thanks, updated as for feedback and split in two commits

re: RPC_INTERNAL_ERROR - didn't immediately found something better, very open to suggestions

Anything else you would add to the test?
@promag
Contributor

promag commented Aug 21, 2017

I meant maybe not throw, and have the bool in the response.
@greenaddress
Contributor

greenaddress commented Aug 21, 2017

I think the condition is exceptional and deserve a throw of an exception & consistent with the other rpcs (though I don't know if there's a more appropriate exception for it)
@promag
Contributor

promag commented Aug 21, 2017

I almost agree with that if it wasn't the fact that the function doesn't throw.. but I have no strong opinion on that.

If the throw stays, then maybe RPC_MISC_ERROR. See what causes RPC_INTERNAL_ERROR, sound like really bad excpetions.
@jonasschnelli

jonasschnelli requested changes Aug 22, 2017
View changes

Concept ACK

src/rpc/blockchain.cpp
+ if (request.fHelp || request.params.size() > 0)
+ throw std::runtime_error(
+ "dumpmempool\n"
+ "\nDumps to disk the mempool.\n"
@jonasschnelli

jonasschnelli Aug 22, 2017

Member

I'm not the one to complain about this: but should it be `"Dumps the mempool to disk"?

src/rpc/blockchain.cpp
+ throw JSONRPCError(RPC_INTERNAL_ERROR, "Unable to dump mempool to disk");
+ }
+
+ return "Mempool dumped";
@jonasschnelli

jonasschnelli Aug 22, 2017

Member

Because this is machine to machine communication, use true as ret val at this point?

@greenaddress
Contributor

greenaddress commented Aug 22, 2017

thanks @jonasschnelli - good points

added a commit (which i think should be squashed at some point together with "Add dumpmempool RPC")
src/rpc/blockchain.cpp
+ );
+
+ if (!DumpMempool()) {
+ throw JSONRPCError(RPC_INTERNAL_ERROR, "Unable to dump mempool to disk");
@promag

promag Aug 23, 2017

Contributor

Maybe RPC_MISC_ERROR. See what causes RPC_INTERNAL_ERROR, sounds like really bad exceptions.

@jonasschnelli
Member

jonasschnelli commented Aug 23, 2017 •
Edited 1 time

utACK cd0ea48

Maybe add an rational (possible use-cases) to the PR description.
@greenaddress
Contributor

greenaddress commented Aug 23, 2017

@promag updated the exception, thanks

@jonasschnelli updated the description. Please note your utACK points to a commit from @TheBlueMatt. GitHub also tells me there's some changes required, should I ignore that?
@MarcoFalke

MarcoFalke reviewed Aug 23, 2017
View changes

Concept ACK, but I don't like the tests.

test/functional/mempool_dump.py
+
+ def run_test(self):
+ self.log.debug("Verify mempool file doesn't exists")
+ mempooldat = self.options.tmpdir + "/node0/regtest/mempool.dat"
@MarcoFalke

MarcoFalke Aug 23, 2017

Member

Use os.path.join, so that it works on other operating systems as well.

test/functional/mempool_dump.py
+ def run_test(self):
+ self.log.debug("Verify mempool file doesn't exists")
+ mempooldat = self.options.tmpdir + "/node0/regtest/mempool.dat"
+ assert_raises(OSError, lambda: os.remove(mempooldat))
@MarcoFalke

MarcoFalke Aug 23, 2017

Member

Might use os.path.isfile for better readability.

test/functional/mempool_dump.py
+ mempooldat = self.options.tmpdir + "/node0/regtest/mempool.dat"
+ assert_raises(OSError, lambda: os.remove(mempooldat))
+ self.log.debug("Dump mempool to disk")
+ assert_equal(self.nodes[0].dumpmempool(), True)
@MarcoFalke

MarcoFalke Aug 23, 2017

Member

nit: No need for assert_equal, you can just assert dumpmempool()

test/functional/mempool_dump.py
+ self.log.debug("Dump mempool to disk")
+ assert_equal(self.nodes[0].dumpmempool(), True)
+ assert_equal(os.path.isfile(mempooldat), True)
+ os.rename(self.options.tmpdir + "/node0/regtest", self.options.tmpdir + "/node0/regtest_bk")
@MarcoFalke

MarcoFalke Aug 23, 2017

Member

I don't think we allow the datadir to be moved while running. The tests should not rely on this.

@MarcoFalke

MarcoFalke Aug 23, 2017

Member

You might be able to achieve the same error condition by changing the file permissions, though.

@MarcoFalke
Member

MarcoFalke commented Aug 23, 2017

You might even combine the tests with the existing mempool persist test.
@greenaddress
Contributor

greenaddress commented Aug 23, 2017

@MarcoFalke I tried with permissions and didn't work. I couldn't find another way to trigger the error.

Will address the other feedback now
src/validation.cpp
} catch (const std::exception& e) {
LogPrintf("Failed to dump mempool: %s. Continuing anyway.\n", e.what());
}
+ return false;
@jnewbery

jnewbery Aug 23, 2017

Member

Not sure what the convention is, or what's best practice, but to me having the return false statement within the catch block is more obvious.

test/functional/mempool_dump.py
+
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import *
+import os
@jnewbery

jnewbery Aug 23, 2017

Member

nit: PEP8 import ordering please: std library imports first, then local project imports.

test/functional/mempool_dump.py
+ self.log.debug("Dump mempool to disk")
+ assert self.nodes[0].dumpmempool()
+ assert os.path.isfile(mempooldat)
+ moveddir = os.path.join(self.options.tmpdir, 'node0', 'regtest_bk')
@jnewbery

jnewbery Aug 23, 2017

Member

I'm not sure if this failure test is necessary. If bitcoind can't write to its data directory, I'm sure all kinds of bad things happen.

@greenaddress

greenaddress Aug 23, 2017

Contributor

I think it makes sense to check that there is no file, you dump and there is a file.

@MarcoFalke

MarcoFalke Aug 23, 2017

Member

I think it makes sense to check that there is no file, you dump and there is a file.

You do exactly this in the lines above. The comment is about assert_raises_jsonrpc, which raises due to the missing datadir. As mentioned earlier, this will likely introduce hard to reproduce test failures now or in the future.

With the current flakiness of the test framework in mind I am against adding such a failure test.

@MarcoFalke

MarcoFalke Aug 23, 2017

Member

What you might try instead is to copy the dumped mempool from node0 to node1 and check that it can be loaded. (The nodes are not connected, but share the same blockchain)

Anyway, I just noticed you are using a clean blockchain and dump the empty mempool. Might be better to populate it a bit.

@promag

promag Aug 23, 2017

Contributor

A bit as in 1 transaction is enough? 😄

@greenaddress

greenaddress Aug 24, 2017

Contributor

@MarcoFalke I made that change (about to push commit) and moved the test to mempool_persist as advised - however your suggestion still doesn't test the exception so I left that in, can easily remove the last 4 lines of the test if other agrees is better to not have it.

@jnewbery
Member

jnewbery commented Aug 23, 2017

If the throw stays, then maybe RPC_MISC_ERROR. See what causes RPC_INTERNAL_ERROR, sound like really bad excpetions.

I think either is fine. bitcoind should always be able to write to its data directory, so I would say failure to do so counts as an internal error.

Concept ACK, but I don't like the tests.

I also don't like the test framework messing with bitcoind's data directory. I don't think the failure testcase is necessary. If bitcoind can't write to its datadirectory, there are more pressing issues than the RPC raising the correct error.

Agree with @MarcoFalke that it might be a good idea to combine this with the mempool_persist.py test (if it can be done tidily).
@greenaddress
Contributor

greenaddress commented Aug 23, 2017 •
Edited 1 time

@jnewbery thanks, addressed some feedback.

Re: testing I also don't like messing with the datadir but unless someone has better suggestions the options are keeping it or removing it.
@jnewbery
Member

jnewbery commented Aug 23, 2017

the options are keeping it or removing it.

I vote for removing it
@promag
Contributor

promag commented Aug 23, 2017

bitcoind should always be able to write to its data directory

@jnewbery DumpMempool() ignores failures, treats them irrelevant, hence my initial point of removing the throw in the RPC.
@greenaddress
Contributor

greenaddress commented Aug 24, 2017 •
Edited 1 time

@MarcoFalke I've merged the tests and added your suggestion "copy the dumped mempool from node0 to node1 and check that it can be loaded. (The nodes are not connected, but share the same blockchain)"

I left in the exception test for now.
@greenaddress
Contributor

greenaddress commented Aug 27, 2017 •
Edited 1 time

Updated slightly the tests only: removed the ugly rename of the node directory in favor of using permissions to make it fail in the test

@MarcoFalke Permissions initially didn't work because there was a rename involved from a tmp file but it works if I use permissions on the tmp file that gets renamed ('mempool.dat.new') so switched to that

I also removed from the test some redundant asserts re: os.remove doing its job (it already throws if it can't find a file or fails to remove it)
@sipa
Owner

sipa commented Aug 28, 2017 •
Edited 1 time

utACK d0bfa6a

@jonasschnelli Have your concerns been addressed?
@jnewbery

jnewbery reviewed Aug 28, 2017
View changes

I much, much prefer using file permissions on the mempool.dat file instead of deleting directories.

A few nits in the testcase, but otherwise looks great. Thanks for indulging all of my review comments :)

test/functional/mempool_persist.py
@@ -85,5 +91,29 @@ def run_test(self):
self.nodes.append(self.start_node(0, self.options.tmpdir))
wait_until(lambda: len(self.nodes[0].getrawmempool()) == 5)
+ regtestdir0 = os.path.join(self.options.tmpdir, 'node0', 'regtest')
@jnewbery

jnewbery Aug 28, 2017

Member

Intermediate regtestdir0 variable not required. os.path.join() can take any number of arguments.

Same for regtestdir1 below.

@greenaddress

greenaddress Aug 29, 2017

Contributor

Makes sense. Will submit a fix for it

test/functional/mempool_persist.py
+ mempooldat0 = os.path.join(regtestdir0, 'mempool.dat')
+ regtestdir1 = os.path.join(self.options.tmpdir, 'node1', 'regtest')
+ mempooldat1 = os.path.join(regtestdir1, 'mempool.dat')
+ self.log.debug("Remove the mempool.dat file. Verify that dumpmempool to disk via RPC re-creates it")
@jnewbery

jnewbery Aug 28, 2017

Member

Supernit: I like logging in the test script to be 'info' level, so when running the test locally, progress can be seen on the console by default.

@greenaddress

greenaddress Aug 29, 2017

Contributor

All the other logs in the test are debug. Either we change them all or I don't think it makes sense for me to just change mine.

test/functional/mempool_persist.py
+ os.rename(mempooldat0, mempooldat1)
+ self.nodes.append(self.start_node(1, self.options.tmpdir))
+ # Give bitcoind a second to reload the mempool
+ time.sleep(1)
@jnewbery

jnewbery Aug 28, 2017

Member

No need for time.sleep() if using wait_until() below.

@greenaddress

greenaddress Aug 29, 2017

Contributor

I thought so too (and indeed it works without time.sleep()) - however the test has that as a pattern and I think we either remove it for the rest of the calls (at least within the same test) or it's inconsistent.

@jnewbery

jnewbery Aug 29, 2017

Member

I'm all for trying to match local style, but there's no benefit in copying bad patterns.

The time.sleep in L76 should be removed (and the assert_equal() in L78 replaced with a wait_until()) in a different PR.

The time.sleep in L85 serves a purpose - the mempool takes a while to load so we want to give it time before asserting it's empty. There's probably a better way of testing this, but we can't just replace the assert_equal() with a wait_until().

@greenaddress
Contributor

greenaddress commented Aug 29, 2017

@jnewbery thanks! Keep in mind we are not doing permissions on the mempool.dat file as that doesn't work (the dump gets done on a tmp file which then replace the mempool.dat and ignores permissions so we are acting on the tmp file instead)
@jnewbery
Member

jnewbery commented Aug 29, 2017

Keep in mind we are not doing permissions on the mempool.dat file as that doesn't work (the dump gets done on a tmp file which then replace the mempool.dat and ignores permissions so we are acting on the tmp file instead)

Ah. Very crafty! Can you add a comment to the test to explain that (mempool.dat.new is an implementation detail - it could potentially be changed in future, breaking this test, so a comment would be helpful)
@greenaddress
Contributor

greenaddress commented Aug 30, 2017

@jnewbery I added the comment and removed the sleep.
I didn't change the logging debug vs info as that imho should be done in a separate PR for all the log statements in the file (i.e. better to be consistent for now)
@jnewbery
Member

jnewbery commented Aug 31, 2017

New commits look great. Can you squash?
@greenaddress
Contributor

greenaddress commented Aug 31, 2017

@jnewbery done, didn't rebase however happy to if needed
@deno49

deno49 commented Aug 31, 2017

@jnewbery

jnewbery reviewed Sep 1, 2017
View changes

Looks really good. Just a couple more nits (sorry!)

test/functional/mempool_persist.py
+ assert self.nodes[0].dumpmempool()
+ assert os.path.isfile(mempooldat0)
+
+ self.log.debug("Stop nodes, make node1 use mempool.dat from node0. Verify it has 5 transactions")
@jnewbery

jnewbery Sep 1, 2017

Member

I think the intent of this section of the tests is to verify that the dumped mempool from the dumpmempool RPC contains all the transactions. Correct? The way it's written right now doesn't test this. The act of stopping node0 causes it to dump its mempool on shutdown, and hence this test is just a repeat of the test on line 77. I think you need to:

  • run dumpmempool on a running node
  • move that mempool.dat to a temp location
  • shutdown the node
  • move the mempool.dat back to the correct location
  • start the node
  • check that its mempool contains the correct transactions
test/functional/mempool_persist.py
+ self.nodes.append(self.start_node(1, self.options.tmpdir))
+ wait_until(lambda: len(self.nodes[1].getrawmempool()) == 5)
+
+ self.log.debug("Force core to fail to save file to disk. Check it errors.")
@jnewbery

jnewbery Sep 1, 2017

Member

nit: debug message could be clearer. Perhaps:

"Prevent bitcoind from writing mempool.dat to disk. Verify that `dumpmempool` fails"
@jnewbery
Member

jnewbery commented Sep 1, 2017

Needed slight rebase changes after #11121 (apologies - I keep making changes to the test_framework API).

I've done that and pushed here: https://github.com/jnewbery/bitcoin/tree/pr11099 . Feel free to reset your branch onto that commit.
@greenaddress
Contributor

greenaddress commented Sep 1, 2017

@jnewbery thank you! I've updated the PR with your rebase, no need to apologize especially as you did all the work :)
@jnewbery
Member

jnewbery commented Sep 1, 2017

Tested ACK 67d307f. Looks great. Thanks for sticking with this.

Expect Travis to fail (for this and all other PRs) until #11215 is merged.
@promag

promag reviewed Sep 1, 2017
View changes

There is only one test for dumpmempool failure, which is the can't write to mempool.dat.new. However there is another return false; inside the catch (which I can't understand why) that should be tested.

Also, RenameOver() doesn't throw, and IMO we should fail if the mempool.dat can't be updated.

src/rpc/blockchain.cpp
@@ -1532,9 +1532,28 @@ UniValue getchaintxstats(const JSONRPCRequest& request)
return ret;
}
+UniValue dumpmempool(const JSONRPCRequest& request)
+{
+ if (request.fHelp || request.params.size() != 0)
@promag

promag Sep 1, 2017

Contributor

Nit, add braces.

@MeshCollider
Member

MeshCollider commented Sep 1, 2017

utACK 67d307f
@luke-jr
Member

luke-jr commented Sep 2, 2017

Suggest renaming it to savemempool or something.

The name dumpmempool to me suggested the same outcome as getrawmempool...

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Sep 2, 2017

@greenaddress @luke-jr
Add return value to DumpMempool 
Github-Pull: #11099
Rebased-From: 609e818
c263bec

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Sep 2, 2017

@greenaddress @luke-jr
Add dumpmempool RPC 
Github-Pull: #11099
Rebased-From: 67d307f
f6bed74
@greenaddress
Contributor

greenaddress commented Sep 2, 2017 •
Edited 1 time

@luke-jr changed the name, savemempool sounds better to me too.

@promag added brackets.

didn't squash and rename one commit yet but happy to when people are happy with the changes

@greenaddress greenaddress changed the title from [RPC][mempool]: Add dumpmempool RPC to [RPC][mempool]: Add savemempool RPC Sep 2, 2017

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Sep 3, 2017

@greenaddress @luke-jr
rename to savemempool, add missing brackets 
Github-Pull: #11099
Rebased-From: 8ef87d2
bfcfc98
@jnewbery
Member

jnewbery commented Sep 4, 2017

utACK 8ef87d2
@MarcoFalke
Member

MarcoFalke commented Sep 5, 2017

@greenaddress Will review after rebase
src/rpc/blockchain.cpp
+ throw JSONRPCError(RPC_MISC_ERROR, "Unable to dump mempool to disk");
+ }
+
+ return true;
@MarcoFalke

MarcoFalke Sep 5, 2017

Member

This return value is not documented. And I think we shouldn't return anything on this rpc.

@promag

promag Sep 5, 2017

Contributor

Right, since on failure an exception is raised, on success it can be void.

@jnewbery

jnewbery Sep 6, 2017

Member

Yes, it appears that many RPCs that perform an action which is expected to succeed return NullUniValue on success. See several of the methods in src/rpc/net.cpp for example.

@promag

promag reviewed Sep 5, 2017
View changes

Please see my above comment.

src/rpc/blockchain.cpp
+ throw JSONRPCError(RPC_MISC_ERROR, "Unable to dump mempool to disk");
+ }
+
+ return true;
@promag

promag Sep 5, 2017

Contributor

Right, since on failure an exception is raised, on success it can be void.

test/functional/mempool_persist.py
os.remove(mempooldat0)
- assert self.nodes[0].dumpmempool()
+ assert self.nodes[0].savemempool()
@promag

promag Sep 5, 2017

Contributor

Remove assert.

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Sep 5, 2017

@greenaddress @luke-jr
Add savemempool RPC 
Github-Pull: #11099
Rebased-From: 67d307f 8ef87d2
1b896d0

greenaddress added some commits Aug 21, 2017

@greenaddress
Add return value to DumpMempool
This commit was signed with a verified signature.
@greenaddress greenaddress GreenAddress
GPG key ID: 17AB8BC320B151D8 Learn about signing commits
467cbbc
@greenaddress
Add savemempool RPC
This commit was signed with a verified signature.
@greenaddress greenaddress GreenAddress
GPG key ID: 17AB8BC320B151D8 Learn about signing commits
1aa97ee
@greenaddress
Contributor

greenaddress commented Sep 6, 2017

rebased & squashed and reworded the second commit to the new savemempool name - a lot of thanks for all feedback

@promag @jnewbery @MarcoFalke I changed the return value to NullUniValue (and accordingly updated the test)
@promag
Contributor

promag commented Sep 6, 2017

utACK 1aa97ee.
@jnewbery
Member

jnewbery commented Sep 6, 2017

utACK 1aa97ee
@MarcoFalke
Member

MarcoFalke commented Sep 6, 2017

@greenaddress Thanks for sticking with the pull. Quite a pain to go through so much review iterations.

@MarcoFalke MarcoFalke merged commit 1aa97ee into bitcoin:master Sep 6, 2017
1 check passed

1 check passed

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

MarcoFalke added a commit that referenced this pull request Sep 6, 2017

@MarcoFalke
Merge #11099: [RPC][mempool]: Add savemempool RPC 
1aa97ee Add savemempool RPC (Lawrence Nahum)
467cbbc Add return value to DumpMempool (Lawrence Nahum)

Pull request description:

  Adds a simple parameterless rpc command to dump the mempool.

  Rationale:

  Sometimes there can be a crash for whatever reason (bug, power loss, etc) causing the mempool.dat file to not be saved.

  This change allows to script/cron the rpc call to have more regular saves to the file as well as cli/ad-hoc.

  This should solve issue #11086

Tree-SHA512: e856ae9777425a4521279c9b58e69285d8e374790bebefd3284cf91931eac0e456f86224f427a087a01bf70440bf6e439fa02c8a34940eb1046ae473e98b6aaa
This commit was signed with a verified signature.
@MarcoFalke MarcoFalke
GPG key ID: D2EA4850E7528B25 Learn about signing commits
bc561b4
@promag
Contributor

promag commented Sep 6, 2017

Quite a pain to go through so much review iterations.

You mean fun? :trollface:
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment