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 https://github.com/bitcoin/bitcoin/issues/11086
1541 | + + HelpExampleCli("dumpmempool", "") 1542 | + + HelpExampleRpc("dumpmempool", "") 1543 | + ); 1544 | + 1545 | + DumpMempool(); 1546 | + return "Mempool dumped";
This is not correct since it can fail.
1530 | @@ -1531,6 +1531,21 @@ UniValue getchaintxstats(const JSONRPCRequest& request) 1531 | return ret; 1532 | } 1533 | 1534 | +UniValue dumpmempool(const JSONRPCRequest& request) 1535 | +{ 1536 | + if (request.fHelp || request.params.size() > 0) 1537 | + throw std::runtime_error( 1538 | + "dumpmempool\n" 1539 | + "\nDumps to disk the mempool (mempool.dat).\n"
Remove the filename?
Please add a test.
The function can be changed to know if it failed or not.
Suggestion, reword Add dumpmempool RPC.
@promag thanks, updated as for feedback and squashed
1567 | @@ -1552,6 +1568,7 @@ static const CRPCCommand commands[] = 1568 | { "blockchain", "gettxoutsetinfo", &gettxoutsetinfo, true, {} }, 1569 | { "blockchain", "pruneblockchain", &pruneblockchain, true, {"height"} }, 1570 | { "blockchain", "verifychain", &verifychain, true, {"checklevel","nblocks"} }, 1571 | + { "blockchain", "dumpmempool", &dumpmempool, true, {} },
Sort, move before getblockchaininfo .
1540 | + "\nExamples:\n" 1541 | + + HelpExampleCli("dumpmempool", "") 1542 | + + HelpExampleRpc("dumpmempool", "") 1543 | + ); 1544 | + 1545 | + if (!DumpMempool())
Missing { }.
1542 | + + HelpExampleRpc("dumpmempool", "") 1543 | + ); 1544 | + 1545 | + if (!DumpMempool()) 1546 | + throw JSONRPCError(RPC_INTERNAL_ERROR, "Unable to dump mempool to disk"); 1547 | + return "Mempool dumped";
Nit, new line before return.
I wonder if it should throw RPC_INTERNAL_ERROR, since DumpMempool handles it's errors.
Please split in two commits:
Add return value to DumpMempoolAdd dumpmempool RPC.
@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?
I meant maybe not throw, and have the bool in the response.
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)
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.
1530 | @@ -1531,9 +1531,28 @@ UniValue getchaintxstats(const JSONRPCRequest& request) 1531 | return ret; 1532 | } 1533 | 1534 | +UniValue dumpmempool(const JSONRPCRequest& request) 1535 | +{ 1536 | + if (request.fHelp || request.params.size() > 0) 1537 | + throw std::runtime_error( 1538 | + "dumpmempool\n" 1539 | + "\nDumps to disk the mempool.\n"
I'm not the one to complain about this: but should it be `"Dumps the mempool to disk"?
1544 | + 1545 | + if (!DumpMempool()) { 1546 | + throw JSONRPCError(RPC_INTERNAL_ERROR, "Unable to dump mempool to disk"); 1547 | + } 1548 | + 1549 | + return "Mempool dumped";
Because this is machine to machine communication, use true as ret val at this point?
Concept ACK
thanks @jonasschnelli - good points
added a commit (which i think should be squashed at some point together with "Add dumpmempool RPC")
1541 | + + HelpExampleCli("dumpmempool", "") 1542 | + + HelpExampleRpc("dumpmempool", "") 1543 | + ); 1544 | + 1545 | + if (!DumpMempool()) { 1546 | + throw JSONRPCError(RPC_INTERNAL_ERROR, "Unable to dump mempool to disk");
Maybe RPC_MISC_ERROR. See what causes RPC_INTERNAL_ERROR, sounds like really bad exceptions.
utACK cd0ea487422028bec1f5df62ab4c57909c2bcc90
Maybe add an rational (possible use-cases) to the PR description.
@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?
27 | + self.num_nodes = 1 28 | + self.setup_clean_chain = True 29 | + 30 | + def run_test(self): 31 | + self.log.debug("Verify mempool file doesn't exists") 32 | + mempooldat = self.options.tmpdir + "/node0/regtest/mempool.dat"
Use os.path.join, so that it works on other operating systems as well.
28 | + self.setup_clean_chain = True 29 | + 30 | + def run_test(self): 31 | + self.log.debug("Verify mempool file doesn't exists") 32 | + mempooldat = self.options.tmpdir + "/node0/regtest/mempool.dat" 33 | + assert_raises(OSError, lambda: os.remove(mempooldat))
Might use os.path.isfile for better readability.
30 | + def run_test(self): 31 | + self.log.debug("Verify mempool file doesn't exists") 32 | + mempooldat = self.options.tmpdir + "/node0/regtest/mempool.dat" 33 | + assert_raises(OSError, lambda: os.remove(mempooldat)) 34 | + self.log.debug("Dump mempool to disk") 35 | + assert_equal(self.nodes[0].dumpmempool(), True)
nit: No need for assert_equal, you can just assert dumpmempool()
32 | + mempooldat = self.options.tmpdir + "/node0/regtest/mempool.dat" 33 | + assert_raises(OSError, lambda: os.remove(mempooldat)) 34 | + self.log.debug("Dump mempool to disk") 35 | + assert_equal(self.nodes[0].dumpmempool(), True) 36 | + assert_equal(os.path.isfile(mempooldat), True) 37 | + os.rename(self.options.tmpdir + "/node0/regtest", self.options.tmpdir + "/node0/regtest_bk")
I don't think we allow the datadir to be moved while running. The tests should not rely on this.
You might be able to achieve the same error condition by changing the file permissions, though.
Concept ACK, but I don't like the tests.
You might even combine the tests with the existing mempool persist test.
@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
4367 | @@ -4368,9 +4368,11 @@ void DumpMempool(void) 4368 | RenameOver(GetDataDir() / "mempool.dat.new", GetDataDir() / "mempool.dat"); 4369 | int64_t last = GetTimeMicros(); 4370 | LogPrintf("Dumped mempool: %gs to copy, %gs to dump\n", (mid-start)*0.000001, (last-mid)*0.000001); 4371 | + return true; 4372 | } catch (const std::exception& e) { 4373 | LogPrintf("Failed to dump mempool: %s. Continuing anyway.\n", e.what()); 4374 | } 4375 | + return false;
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.
16 | + 17 | +""" 18 | + 19 | +from test_framework.test_framework import BitcoinTestFramework 20 | +from test_framework.util import * 21 | +import os
nit: PEP8 import ordering please: std library imports first, then local project imports.
33 | + mempooldat = os.path.join(regtestdir, 'mempool.dat') 34 | + assert not os.path.isfile(mempooldat) 35 | + self.log.debug("Dump mempool to disk") 36 | + assert self.nodes[0].dumpmempool() 37 | + assert os.path.isfile(mempooldat) 38 | + moveddir = os.path.join(self.options.tmpdir, 'node0', 'regtest_bk')
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.
I think it makes sense to check that there is no file, you dump and there is a file.
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.
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.
A bit as in 1 transaction is enough? 😄
@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.
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).
@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.
the options are keeping it or removing it.
I vote for removing it
@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.
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)
utACK d0bfa6a11a66a8c08fe2630472546d0e60175fe3 @jonasschnelli Have your concerns been addressed?
90 | @@ -85,5 +91,29 @@ def run_test(self): 91 | self.nodes.append(self.start_node(0, self.options.tmpdir)) 92 | wait_until(lambda: len(self.nodes[0].getrawmempool()) == 5) 93 | 94 | + regtestdir0 = os.path.join(self.options.tmpdir, 'node0', 'regtest')
Intermediate regtestdir0 variable not required. os.path.join() can take any number of arguments.
Same for regtestdir1 below.
Makes sense. Will submit a fix for it
90 | @@ -85,5 +91,29 @@ def run_test(self): 91 | self.nodes.append(self.start_node(0, self.options.tmpdir)) 92 | wait_until(lambda: len(self.nodes[0].getrawmempool()) == 5) 93 | 94 | + regtestdir0 = os.path.join(self.options.tmpdir, 'node0', 'regtest') 95 | + mempooldat0 = os.path.join(regtestdir0, 'mempool.dat') 96 | + regtestdir1 = os.path.join(self.options.tmpdir, 'node1', 'regtest') 97 | + mempooldat1 = os.path.join(regtestdir1, 'mempool.dat') 98 | + self.log.debug("Remove the mempool.dat file. Verify that dumpmempool to disk via RPC re-creates it")
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.
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.
103 | + self.log.debug("Stop nodes, make node1 use mempool.dat from node0. Verify it has 5 transactions") 104 | + self.stop_nodes() 105 | + os.rename(mempooldat0, mempooldat1) 106 | + self.nodes.append(self.start_node(1, self.options.tmpdir)) 107 | + # Give bitcoind a second to reload the mempool 108 | + time.sleep(1)
No need for time.sleep() if using wait_until() below.
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.
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().
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 :)
@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)
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)
@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)
New commits look great. Can you squash?
@jnewbery done, didn't rebase however happy to if needed
strictly q On Aug 20, 2017 9:46 PM, "GreenAddress" notifications@github.com wrote:
@promag https://github.com/promag thanks, updated as for feedback and squashed
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/11099#issuecomment-323627960, or mute the thread https://github.com/notifications/unsubscribe-auth/AVfQPLGwbbeqOskROywlYWaVocTD-14rks5saOFhgaJpZM4O8sJC .
96 | + self.log.debug("Remove the mempool.dat file. Verify that dumpmempool to disk via RPC re-creates it") 97 | + os.remove(mempooldat0) 98 | + assert self.nodes[0].dumpmempool() 99 | + assert os.path.isfile(mempooldat0) 100 | + 101 | + self.log.debug("Stop nodes, make node1 use mempool.dat from node0. Verify it has 5 transactions")
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:
dumpmempool on a running node102 | + self.stop_nodes() 103 | + os.rename(mempooldat0, mempooldat1) 104 | + self.nodes.append(self.start_node(1, self.options.tmpdir)) 105 | + wait_until(lambda: len(self.nodes[1].getrawmempool()) == 5) 106 | + 107 | + self.log.debug("Force core to fail to save file to disk. Check it errors.")
nit: debug message could be clearer. Perhaps:
"Prevent bitcoind from writing mempool.dat to disk. Verify that `dumpmempool` fails"
Looks really good. Just a couple more nits (sorry!)
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.
@jnewbery thank you! I've updated the PR with your rebase, no need to apologize especially as you did all the work :)
1531 | @@ -1532,9 +1532,28 @@ UniValue getchaintxstats(const JSONRPCRequest& request) 1532 | return ret; 1533 | } 1534 | 1535 | +UniValue dumpmempool(const JSONRPCRequest& request) 1536 | +{ 1537 | + if (request.fHelp || request.params.size() != 0)
Nit, add braces.
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.
Suggest renaming it to savemempool or something.
The name dumpmempool to me suggested the same outcome as getrawmempool...
utACK 8ef87d253c88c2e19e875f9fec601159fb6da32f
utACK 8ef87d253c88c2e19e875f9fec601159fb6da32f
@greenaddress Will review after rebase
1546 | + 1547 | + if (!DumpMempool()) { 1548 | + throw JSONRPCError(RPC_MISC_ERROR, "Unable to dump mempool to disk"); 1549 | + } 1550 | + 1551 | + return true;
This return value is not documented. And I think we shouldn't return anything on this rpc.
Right, since on failure an exception is raised, on success it can be void.
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.
85 | @@ -86,9 +86,9 @@ def run_test(self): 86 | 87 | mempooldat0 = os.path.join(self.options.tmpdir, 'node0', 'regtest', 'mempool.dat') 88 | mempooldat1 = os.path.join(self.options.tmpdir, 'node1', 'regtest', 'mempool.dat') 89 | - self.log.debug("Remove the mempool.dat file. Verify that dumpmempool to disk via RPC re-creates it") 90 | + self.log.debug("Remove the mempool.dat file. Verify that savemempool to disk via RPC re-creates it") 91 | os.remove(mempooldat0) 92 | - assert self.nodes[0].dumpmempool() 93 | + assert self.nodes[0].savemempool()
Remove assert.
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)
utACK 1aa97ee.
utACK 1aa97ee088ea03dd208be054c5ad9198c1f13329
@greenaddress Thanks for sticking with the pull. Quite a pain to go through so much review iterations.
Quite a pain to go through so much review iterations.
You mean fun? :trollface: