Bugfix: RPC: savemempool: Don't save until LoadMempool() is finished #12172

jtimon · 2018-01-12T21:00:49Z

The tests are a little bit slow, mempool_persist.py goes from about 20 s to about 120 s in my hardware.
Perhaps there's a better way to test this.

instagibbs · 2018-01-12T21:21:14Z

src/rpc/blockchain.cpp

@@ -1583,14 +1583,14 @@ UniValue savemempool(const JSONRPCRequest& request)
    if (request.fHelp || request.params.size() != 0) {
        throw std::runtime_error(
            "savemempool\n"
-            "\nDumps the mempool to disk.\n"
+            "\nDumps the mempool to disk. It will fail until the previous dump is fully loaded.\n"


this also cannot complete when -persistmempool is false, yes? Might want to add that to RPC notes, or set it to true always if persist is false.

I think it's probably important that this works if -persistmempool is false.

Indeed, definitely needs to work with -persistmempool false, that's the whole point of the savemempool command. AFAICT the current implementation does work in that case.

I agree it's best it works in that case, just didn't thought about it initially. But for the record, I'm using savemempool with -persistmempool to true, because I want to read it on restart too!
I think the whole point of savemempool is to cover use cases where for whatever reason (in my case, an insufficient knowledge of docker-compose) you can't or don't always let the daemon exit gracefully.

instagibbs · 2018-01-12T21:24:55Z

test/functional/mempool_persist.py

+        for i in range(NUM_TXS_IN_MEMPOOL):
+            self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), Decimal("0.00001"))
+        self.stop_nodes()
+        self.start_node(0)


is this test essentially racing the mempool acceptance?

stopping the node the mempool is dumped and starting the node, LoadMempool gets called again, hopefully with enough work ahead to catch the error when calling savemempool on the next line.

maflcko · 2018-01-12T21:45:20Z

src/rpc/blockchain.cpp

@@ -1583,14 +1583,14 @@ UniValue savemempool(const JSONRPCRequest& request)
    if (request.fHelp || request.params.size() != 0) {
        throw std::runtime_error(
            "savemempool\n"
-            "\nDumps the mempool to disk.\n"
+            "\nDumps the mempool to disk. It will fail until the previous dump is fully loaded or when -persistmempool=0.\n"


I don't see a reason to make it fail when nopersistmempool

maflcko · 2018-01-12T21:46:26Z

src/init.cpp

@@ -679,6 +679,7 @@ void ThreadImport(std::vector<fs::path> vImportFiles)
    } // End scope of CImportingNow
    if (gArgs.GetArg("-persistmempool", DEFAULT_PERSIST_MEMPOOL)) {
        LoadMempool();
+        g_is_mempool_loaded = true;


Move down two lines to break out of this scope?

jtimon · 2018-01-12T21:55:44Z

Modified so that savemempool keeps working when -persistmempool=0 as discussed.

maflcko · 2018-01-12T22:58:25Z

src/validation.cpp

@@ -227,6 +227,7 @@ CAmount maxTxFee = DEFAULT_TRANSACTION_MAXFEE;

 CBlockPolicyEstimator feeEstimator;
 CTxMemPool mempool(&feeEstimator);
+std::atomic_bool g_is_mempool_loaded(false);


style-nit: Might use g_is_mempool_loaded{false} for compile-time narrowing check.

TheBlueMatt · 2018-01-14T18:37:29Z

src/init.cpp

@@ -681,6 +681,7 @@ void ThreadImport(std::vector<fs::path> vImportFiles)
        LoadMempool();
        fDumpMempoolLater = !fRequestShutdown;
    }
+    g_is_mempool_loaded = true;


It would be nice to drop fDumpMempoolLater and only have one global for "mempool was loaded". Should be able to set g_is_mempool_loaded to !fRequestShutdown here, and then check -persistmempool before flushing during shutdown to avoid changing behavior without adding another boolean.

Perhaps I'm not understanding the code, but what I assumed is that fDumpMempoolLater is set to !fRequestShutdown instead of true to avoid more than one thread dumping the mempool. But in this case we want all threads to work with savemempool once the mempool is loaded.
Am I confused about this or missing something?

There is only one shutdown thread, not sure what you mean "more than one thread dumping the mempool". !fRequestShutdown is a check to make sure LoadMempool() didn't quit early, but its also safe to re-use that in RPC as if fRequestShutdown is set we'll be shutting down soon anyway, so RPCs can fail.

Alright, I wasn't understanding why !fRequestShutdown was set, it seems we can have a single boolean then and drop fDumpMempoolLater.

TheBlueMatt · 2018-01-14T18:37:53Z

src/rpc/blockchain.cpp

            "\nExamples:\n"
            + HelpExampleCli("savemempool", "")
            + HelpExampleRpc("savemempool", "")
        );
    }

-    if (!DumpMempool()) {
+    if (!g_is_mempool_loaded || !DumpMempool()) {


Would be nice to give a different error message here.

Agree, with the corresponding test.

Should the g_is_mempool_loaded check be inside of DumpMempool? No need to see g_is_mempool_loaded in the RPC code.

Note that with this change there is no need for a functional test, just a c++ test DumpMempool.

Well, if I put it inside DumpMempool() then I can't have the different meesage, right?
I don't have a strong opinion about putting it inside and removing the functional test, I would like to hear what others think before doing it. I'll change the error message for now.

TheBlueMatt · 2018-01-14T18:38:24Z

src/rpc/blockchain.cpp

@@ -1583,14 +1583,14 @@ UniValue savemempool(const JSONRPCRequest& request)
    if (request.fHelp || request.params.size() != 0) {
        throw std::runtime_error(
            "savemempool\n"
-            "\nDumps the mempool to disk.\n"
+            "\nDumps the mempool to disk. It will fail until the previous dump is fully loaded.\n"


Indeed, definitely needs to work with -persistmempool false, that's the whole point of the savemempool command. AFAICT the current implementation does work in that case.

promag

Concept ACK.

BTW, Should the RPC server start after the mempool is loaded?

promag · 2018-01-15T14:46:20Z

src/rpc/blockchain.cpp

            "\nExamples:\n"
            + HelpExampleCli("savemempool", "")
            + HelpExampleRpc("savemempool", "")
        );
    }

-    if (!DumpMempool()) {
+    if (!g_is_mempool_loaded || !DumpMempool()) {


Agree, with the corresponding test.

promag · 2018-01-15T14:46:58Z

src/rpc/blockchain.cpp

            "\nExamples:\n"
            + HelpExampleCli("savemempool", "")
            + HelpExampleRpc("savemempool", "")
        );
    }

-    if (!DumpMempool()) {
+    if (!g_is_mempool_loaded || !DumpMempool()) {


Should the g_is_mempool_loaded check be inside of DumpMempool? No need to see g_is_mempool_loaded in the RPC code.

Note that with this change there is no need for a functional test, just a c++ test DumpMempool.

jtimon · 2018-01-15T21:09:35Z

I guess starting the rpc server only after the mempool is loaded is another solution, but I'm not sure it is better.
Unified the 2 bool globals and separated the error message.

gmaxwell · 2018-01-16T05:01:11Z

src/rpc/blockchain.cpp

            "\nExamples:\n"
            + HelpExampleCli("savemempool", "")
            + HelpExampleRpc("savemempool", "")
        );
    }

+    if (!g_is_mempool_loaded && gArgs.GetArg("-persistmempool", DEFAULT_PERSIST_MEMPOOL)) {


Why is there still an argument check here? The setting of g_is_mempool_loaded appears to me to be unconditional now.

The code seems correct, but is just badly documented.

What about

const bool is_supposed_to_load_mempool{gArgs.GetArg("-persistmempool", DEFAULT_PERSIST_MEMPOOL)}; if (!g_is_mempool_loaded && is_supposed_to_load_mempool) {

Well, it is now not needed since the g_is_mempool_loaded is set out of the if gArgs.GetArg("-persistmempool".... I was thinking that perhaps we should allow to dump even before g_is_mempool_loaded is set if -persistmempool=0 (since in that case it's not loading it anyway).
But perhaps it's better that it waits for that line to be run when -persistmempool=0 too.
Or perhaps the we can set g_is_mempool_loaded only conditionally like fDumpMempoolLater was.

No strong opinion, I'm happy with any of the possible variations on this. But I don't think it's a matter of documentation. It's just choosing what we want here.

Why would there be anything in mempool, when -persistmempool=0 and you are not yet connected to any peers? So there wouldn't be any need to allow savemempool.

We are mostly nitpicking here, and I don't care too much what variation you pick, but the current code seems confusion/hard to read, otherwise we woulnd't have this conversation.

Alright, I'll just remove the gArgs.GetArg("-persistmempool", DEFAULT_PERSIST_MEMPOOL) condition from here and leave it as it was right before.

maflcko · 2018-01-17T00:26:48Z

utACK 5b80af6322036c141ffce8fb7f10f5d40b8bc0f0 (didn't look at tests)

maflcko · 2018-01-19T00:11:01Z

test/functional/mempool_persist.py

@@ -112,5 +112,17 @@ def run_test(self):
        assert_raises_rpc_error(-1, "Unable to dump mempool to disk", self.nodes[1].savemempool)
        os.remove(mempooldotnew1)

+        NUM_TXS_IN_MEMPOOL = 900
+        self.log.debug("Send %d transactions from node0 (to its own address)" % NUM_TXS_IN_MEMPOOL)
+        self.stop_nodes()


A comment explaining why this is needed?

I guess perhaps it is not needed...

Please remove it, when it is not needed. Starting and stopping nodes is where a ton of time is wasted in the test framework.

I assume you can modify the self.stop_nodes() a few lines above to say self.stop_node(1)

maflcko · 2018-01-19T00:11:23Z

test/functional/mempool_persist.py

+        self.start_node(0)
+        self.nodes[0].generate(12)
+        for i in range(NUM_TXS_IN_MEMPOOL):
+            self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), Decimal("0.00001"))


How will this not create too long mempool chains?

I'm not sure I understand the question. What's the problem?

Currently the mempool only accepts a chain of up to 26 unconfirmed transactions, since coin selection is not deterministic, I assume we will run into travis failures at some point.

Also, IIRC the wallet is extremely slow with coin selection, when a lot of the coins are unconfirmed change. There might be some helper functions in the test framework that can create a ton of spam transactions.

maflcko · 2018-01-21T21:54:24Z

Test takes 7-8 Minutes on travis. Should be fixed before merge.

maflcko · 2018-01-21T21:56:09Z

Test is failing for me locally. You might as well leave out the test and ask for some Tested ACKs instead.

jtimon · 2018-01-23T02:22:12Z

@MarcoFalke Sure, I'm happy to leave out the test if I get some tested acks. It's an edge case anyway and seems expensive to test this way, filling the mempool enough so that the loading takes long enough to get the error (plus that varies with the machine and machine load [this seems close to the minimum in my machine but it's not enough on yours and it's too much on travis]).
Additionally it is quite simple IMO to see this is correct without the tests with very little grep since it's only +9-4 without the tests.

jtimon · 2018-01-31T22:09:20Z

Needed rebase

maflcko · 2018-01-31T22:17:10Z

I am guessing you wouldn't have to rebase if you just dropped the test commit. Since I am going to NACK on merging the test commit for the reasons provided above, you might as well remove it.

jtimon · 2018-01-31T22:24:17Z

Yes, I'm also for removing it. I was just waiting for more people to agree with us and some tested ACKs as you said.

EDIT: but, no, it needed rebase and not for the tests, new code close to the one I was changing was changed in https://github.com/bitcoin/bitcoin/pull/12266/files#diff-c865a8939105e6350a50af02766291b7R204

greenaddress · 2018-02-04T10:37:41Z

BTW, Should the RPC server start after the mempool is loaded?

Seems to me this issue is not just about saving mempool but also about any RPC that interacts with the mempool like fetching raw transactions and so on - thus I also think it may be a good idea to not start the rpc server until the service is up and loaded the mempool - am I missing something?

jtimon · 2018-02-21T04:42:38Z

Rebased and added several commits inspired by @promag 's last suggestion but without looking much at WalletRescanReserver::reserver() and trying to do things with atomic booleans in mempool instead.

We can squash, leave for later PRs or reject forever each of the newly proposed commits.

I wish I could go as far as s/LoadMempool(/mempool.Load(/ and s/DumpMempool(/mempool.Dump(/ , moving code from validation.cpp to txmempool.cpp, but at some point we need to just fix #12142 and leave later improvements for later PRs.

promag · 2018-02-21T10:42:12Z

without looking much at WalletRescanReserver::reserver() and trying to do things with atomic booleans in mempool instead.

Any reason to do differently?

Not sure about representing the state with 2 atomic booleans. Will review later.

jtimon · 2018-02-28T04:33:46Z

@promag Yeah, sorry for pushing a half-baked thought. Please, let me come back to this again before you review it, perhaps I can convince myself it doesn't make sense and I save you some time.

EDIT: Or perhaps we can just leave it as it previously was and leave your request for a following PR, but I think it makes a lot of sense and it shouldn't be that hard if I hadn't tried to simplify the wheel.

maflcko · 2018-03-19T00:30:10Z

src/txmempool.h

@@ -452,6 +455,7 @@ class CTxMemPool
 public:

    static const int ROLLING_FEE_HALFLIFE = 60 * 60 * 12; // public only for testing
+    static const uint64_t MEMPOOL_DUMP_VERSION = 1;


Please remove unrelated move of variables from a bug fix pull request

maflcko · 2018-03-19T00:42:57Z

What is the status of this? Seems stale and can be closed?

jtimon · 2018-03-22T09:25:00Z

I'm sorry @MarcoFalke IIRC the status is that the first commit has been around for long and tested by myself many times (but I'm trusting my rebases on the same ting now), but then after @promag 's review and test (he gave a tested ack for the first commit), I got very excited about a suggestion he made to upgrade this PR to fix more related things. I think he is totally right in his suggestion of apart from not allowing saving the mempool until it has been loaded, also just return an error as suggested when you call the rpc to save the mempool and it's already in the process of saving.

But instead of listening to @promag and copy a part of the codebase that already works for the same king of thing, I tried to re-invent the wheel because I thought I could implement it simpler and I failed so far.

I suggest that we merge the first commit which is very simple and even has had tests in the past (until people asked them to be removed for being slow and non-deterministic) and also been tested manually (not very hard to test manually, btw). Then someone (ehem, promag) could create another so that no thread dares to save the mempool while already saving it. Please ping me to test that second part, since it's as easy to test manually as this one,

As a user that never gives more the daemon more than 60 secs to gracefully exit and thus depends on this rpc call to persist the mempool, I can just always wait longer for the first save, I can at most miss mempool data while re-deploying too often. Being prevented from calling savemempool twice is I think a cool improvement but not that much for users that are calling savemempool with a cron process like me (and I imagine, most savemempool users).

promag · 2018-03-22T11:12:23Z

I'm fine with @jtimon suggestion to move the fix forward. I can then submit a PR to improve concurrency.

jtimon · 2018-03-29T04:30:04Z

I'm assuming an agreement from @MarcoFalke 's lack of response.
Just in case, the branch that is not tested and that is trying to solve @promag 's very legitimate concern is in https://github.com/jtimon/bitcoin/tree/b16-bugfix-savemempool-extend. I will review or at a minimum test his more reasonable future proposal. But I tried to make it in an unorthodox way I would like to study more than I know the way @promag 's knows how to do it.
But please let's fix this first.

laanwj · 2018-03-29T22:25:56Z

utACK cb1e319

…) is finished cb1e319 Bugfix: RPC: savemempool: Don't save until LoadMempool() is finished (Jorge Timón) Pull request description: Fixes #12142 The tests are a little bit slow, mempool_persist.py goes from about 20 s to about 120 s in my hardware. Perhaps there's a better way to test this. Tree-SHA512: 9e6c24b32a9cf3774e8f0bd81c035b0deb53fba5ac3eb2532d85900579d21cef8a1135b75a4fa0a9d883e3822eb35e7d4b47a0838abf99789039205041962629

585b47c rpc: Prevent concurrent savemempool (João Barbosa) Pull request description: Follow up of #12172, this change prevents calling `savemempool` RPC concurrently. Tree-SHA512: 4759a7107658a9794f5c6ab7e3e3002276abadd64996010be67a2791914d284db6fe0377c071a8d6a42387bfb0178f219b73aeec077ce5c4fe5c634a30b3e081

…empool() is finished cb1e319 Bugfix: RPC: savemempool: Don't save until LoadMempool() is finished (Jorge Timón) Pull request description: Fixes bitcoin#12142 The tests are a little bit slow, mempool_persist.py goes from about 20 s to about 120 s in my hardware. Perhaps there's a better way to test this. Tree-SHA512: 9e6c24b32a9cf3774e8f0bd81c035b0deb53fba5ac3eb2532d85900579d21cef8a1135b75a4fa0a9d883e3822eb35e7d4b47a0838abf99789039205041962629

585b47c rpc: Prevent concurrent savemempool (João Barbosa) Pull request description: Follow up of bitcoin#12172, this change prevents calling `savemempool` RPC concurrently. Tree-SHA512: 4759a7107658a9794f5c6ab7e3e3002276abadd64996010be67a2791914d284db6fe0377c071a8d6a42387bfb0178f219b73aeec077ce5c4fe5c634a30b3e081

jtimon mentioned this pull request Jan 12, 2018

RPC: savemempool: Don't write mempool.dat while not having the previous one in memory #12142

Closed

jtimon force-pushed the b16-bugfix-savemempool branch from 6a52e42 to e2b17a5 Compare January 12, 2018 21:18

instagibbs reviewed Jan 12, 2018

View reviewed changes

jtimon force-pushed the b16-bugfix-savemempool branch from e2b17a5 to 4b30711 Compare January 12, 2018 21:39

maflcko reviewed Jan 12, 2018

View reviewed changes

jtimon force-pushed the b16-bugfix-savemempool branch from 4b30711 to d494001 Compare January 12, 2018 21:54

maflcko reviewed Jan 12, 2018

View reviewed changes

jtimon force-pushed the b16-bugfix-savemempool branch from d494001 to e685d0b Compare January 12, 2018 23:18

fanquake added RPC/REST/ZMQ Mempool labels Jan 13, 2018

TheBlueMatt reviewed Jan 14, 2018

View reviewed changes

promag reviewed Jan 15, 2018

View reviewed changes

jtimon force-pushed the b16-bugfix-savemempool branch from e685d0b to 4e9f522 Compare January 15, 2018 21:08

gmaxwell reviewed Jan 16, 2018

View reviewed changes

ajtowns mentioned this pull request Jan 16, 2018

[tests] Rename functional tests #11774

Merged

jtimon force-pushed the b16-bugfix-savemempool branch from 4e9f522 to 7333159 Compare January 16, 2018 21:13

maflcko reviewed Jan 19, 2018

View reviewed changes

jtimon force-pushed the b16-bugfix-savemempool branch from 7333159 to 31c1565 Compare January 31, 2018 22:08

bitcoin deleted a comment from ushan007 Feb 12, 2018

jtimon force-pushed the b16-bugfix-savemempool branch 3 times, most recently from 1a21663 to 14c00f1 Compare February 21, 2018 06:51

maflcko reviewed Mar 19, 2018

View reviewed changes

Bugfix: RPC: savemempool: Don't save until LoadMempool() is finished

cb1e319

jtimon force-pushed the b16-bugfix-savemempool branch from f2c13eb to cb1e319 Compare March 29, 2018 04:25

laanwj merged commit cb1e319 into bitcoin:master Mar 29, 2018

promag mentioned this pull request Mar 30, 2018

Prevent concurrent savemempool #12842

Merged

jtimon deleted the b16-bugfix-savemempool branch May 23, 2018 13:51

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

Bugfix: RPC: savemempool: Don't save until LoadMempool() is finished #12172

Bugfix: RPC: savemempool: Don't save until LoadMempool() is finished #12172

Uh oh!

Conversation

jtimon commented Jan 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtimon Jan 15, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtimon commented Jan 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtimon commented Jan 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jan 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jtimon commented Jan 12, 2018 •

edited

Loading

jtimon Jan 15, 2018 •

edited

Loading

jtimon commented Jan 12, 2018 •

edited

Loading

maflcko commented Jan 21, 2018 •

edited

Loading

maflcko commented Jan 31, 2018 •

edited

Loading

jtimon commented Jan 31, 2018 •

edited

Loading

jtimon commented Feb 21, 2018 •

edited

Loading

jtimon commented Feb 28, 2018 •

edited

Loading