rpc: Expose g_is_mempool_loaded via getmempoolinfo

Empact commented at 10:14 pm on February 1, 2019: member

And use it to fix a race condition in mempool_persist.py: https://travis-ci.org/Empact/bitcoin/jobs/487577243

Since e.g. getrawmempool returns errors based on this status, this enables users to test it for readiness.

Fixes #12863

Empact force-pushed on Feb 1, 2019

Empact commented at 10:46 pm on February 1, 2019: member

The test failure, for reference:

 0stdout:
 12019-02-01T20:25:18.578000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_₿_🏃_20190201_201908/mempool_persist_74
 22019-02-01T20:25:27.680000Z TestFramework (ERROR): Assertion failed
 3Traceback (most recent call last):
 4  File "/home/travis/build/Empact/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/util.py", line 113, in try_rpc
 5    fun(*args, **kwds)
 6  File "/home/travis/build/Empact/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/coverage.py", line 47, in __call__
 7    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
 8  File "/home/travis/build/Empact/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/authproxy.py", line 136, in __call__
 9    raise JSONRPCException(response['error'])
10test_framework.authproxy.JSONRPCException: The mempool was not loaded yet (-1)
11During handling of the above exception, another exception occurred:
12Traceback (most recent call last):
13  File "/home/travis/build/Empact/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/test_framework.py", line 174, in main
14    self.run_test()
15  File "/home/travis/build/Empact/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/mempool_persist.py", line 130, in run_test
16    assert_raises_rpc_error(-1, "Unable to dump mempool to disk", self.nodes[1].savemempool)
17  File "/home/travis/build/Empact/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/util.py", line 105, in assert_raises_rpc_error
18    assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"
19  File "/home/travis/build/Empact/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/util.py", line 119, in try_rpc
20    raise AssertionError("Expected substring not found:" + e.error['message'])
21AssertionError: Expected substring not found:The mempool was not loaded yet
222019-02-01T20:25:27.682000Z TestFramework (INFO): Stopping nodes
232019-02-01T20:25:27.985000Z TestFramework (WARNING): Not cleaning up dir /tmp/test_runner_₿_🏃_20190201_201908/mempool_persist_74
242019-02-01T20:25:27.985000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/test_runner_₿_🏃_20190201_201908/mempool_persist_74/test_framework.log
252019-02-01T20:25:27.985000Z TestFramework (ERROR): Hint: Call /home/travis/build/Empact/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/combine_logs.py '/tmp/test_runner_₿_🏃_20190201_201908/mempool_persist_74' to consolidate all logs
26stderr:

in src/rpc/blockchain.cpp:1512 in 869e66948e outdated

1508@@ -1508,6 +1509,7 @@ static UniValue getmempoolinfo(const JSONRPCRequest& request)
1509                 {},
1510                 RPCResult{
1511             "{\n"
1512+            "  \"loaded\": true|false         (boolean) True if the mempool has been loaded/initialized.\n"

MarcoFalke commented at 11:04 pm on February 1, 2019:

Should clarify that this means loaded from disk and if -persistmempool is on?

Empact commented at 0:11 am on February 2, 2019:

If -persistmempool is off, g_is_mempool_loaded is set at the end of ThreadImport, after block import. Ideas on how to articulate that? Perhaps we could set it earlier in that case -persistmempool?

Empact commented at 0:49 am on February 2, 2019:

How about this: https://github.com/bitcoin/bitcoin/pull/15323/commits/ab48289be80bfbbbc378431e5383d1879ec9fff4

promag commented at 1:14 am on February 2, 2019:

nit, why /initialized?

Empact commented at 11:05 pm on February 2, 2019:

Because it will also be true in cases where it has not been loaded from disk, but only “initialized”. Maybe not worth mentioning.

laanwj commented at 2:45 pm on February 12, 2019:

Would prefer something like True if the mempool has been loaded from disk (if persistent)

jnewbery commented at 4:55 pm on February 26, 2019:

I think True if the mempool has been loaded from disk (if persistent), or need not be is much less clear than the more simple True if the mempool is fully loaded. The first raises more questions than it answers (“What is persistent? When need it not be?”)

MarcoFalke commented at 5:51 pm on February 26, 2019:

Maybe s/persist/-persistmempool/g?

MarcoFalke commented at 11:15 pm on February 1, 2019: member

Ref #12863 (comment)

jonasschnelli added the label RPC/REST/ZMQ on Feb 2, 2019

Empact force-pushed on Feb 2, 2019

promag commented at 1:14 am on February 2, 2019: member

Concept ACK. Could add a release note with the new key. This introduces a weird case, which is loaded can change from true to false. Maybe https://github.com/bitcoin/bitcoin/blob/b3a715301a0fd972fb2f3bd36e2680b3cdbbab26/src/init.cpp#L707 should change to

0 g_is_mempool_loaded |= !ShutdownRequested();

Empact force-pushed on Feb 2, 2019

MarcoFalke added this to the milestone 0.19.0 on Feb 22, 2019

DrahtBot commented at 4:24 pm on February 25, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

promag commented at 4:33 pm on February 25, 2019: member

/rest/mempool/info.json is also affected.

Empact force-pushed on Feb 26, 2019

Empact commented at 9:40 am on February 26, 2019: member

Revised and cleaned up:

Updated /rest/mempool/info.json to include the attribute (thanks @promag)
Split the setting of g_is_mempool_loaded strictly between the persistmempool and non-persist case - note it’s only relevant on shutdown if persistmempool is set: https://github.com/bitcoin/bitcoin/blob/8f470ecc534d49cb5f7045ea9564481acd3e2b4b/src/init.cpp#L238-L240

Empact force-pushed on Mar 4, 2019

Empact commented at 0:09 am on March 4, 2019: member

Updated comments and documentation as per John and Marco’s suggestions #15323 (review)

Empact force-pushed on Mar 4, 2019

DrahtBot added the label Needs rebase on Mar 6, 2019

Empact force-pushed on Mar 7, 2019

Empact commented at 3:15 pm on March 7, 2019: member

Rebase for #15473, and add another commit moving g_is_mempool_loaded into CTxMemPool, given that it is mempool-specific.

DrahtBot removed the label Needs rebase on Mar 7, 2019

in test/functional/mempool_persist.py:106 in 32e2018724 outdated

100@@ -100,14 +101,16 @@ def run_test(self):
101         self.log.debug("Stop-start node0 with -persistmempool=0. Verify that it doesn't load its mempool.dat file.")
102         self.stop_nodes()
103         self.start_node(0, extra_args=["-persistmempool=0"])
104+        assert self.nodes[0].getmempoolinfo()["loaded"]
105         # Give bitcoind a second to reload the mempool
106         time.sleep(1)

MarcoFalke commented at 4:45 pm on March 7, 2019:

Can remove the sleep, given the assert?

Empact force-pushed on Mar 9, 2019

jnewbery commented at 5:06 pm on March 21, 2019: member

ACK the first and third commits (rpc: Expose g_is_mempool_loaded via getmempoolinfo and /rest/mempool/info.json and Move g_is_mempool_loaded into CTxMemPool::m_is_loaded). I like the tidy up in Move g_is_mempool_loaded into CTxMemPool::m_is_loaded to remove the g_is_mempool_loaded global.

I don’t see the need for the second commit (rpc: Set g_is_mempool_loaded early if the mempool is not persisted). Having two places where m_is_loaded_ gets set seems to add complexity for no good reason. I don’t think any users will see any behaviour difference before and after that commit.

Empact force-pushed on Mar 21, 2019

rpc: Expose g_is_mempool_loaded via getmempoolinfo and /rest/mempool/info.json

And use it to fix a race condition in mempool_persist.py:
https://travis-ci.org/Empact/bitcoin/jobs/487577243

Since e.g. getrawmempool returns errors based on this status, this
enables users to test it for readiness.

bb8ae2c419

Move g_is_mempool_loaded into CTxMemPool::m_is_loaded

So the loaded state is explicitly mempool-specific.

effe81f750

Empact force-pushed on Mar 22, 2019

MarcoFalke commented at 10:44 pm on April 30, 2019: member

utACK effe81f750

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3utACK effe81f750
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgQDgv/Vu+DeQYZkt0Au46dAPXOWF5uIfLHOGhOHbKB+YZ6zFNIrmw6VF5XrYDl
 8IJqLlqX2WtDQd8gYcQ9dDvVSBktaJrk6Qm9laEX8qITMavz5VHK8rHfqmJ9nrqC8
 9DNgWMlwhoQsJqflqXeaUKnkXv9eoMLvuHVn+F3kDkr9Wu/AjRjLxu1zaE9qnr0sl
10lO8Z+mdVig4NDWbCHwZHyvP+eM8skn5WeEXTSi6Vl1yqgEP9T/9TEFQtlQ1ikTjn
11t+ACMSxzfest155gsZt0vxn5ABlfw1phw3b/x6DEFIFNzs0cBYVRxu8kPhTarqYL
12gMcrAvVlfs9knGDUaU4XaZbfor/cQRAduFCKqowSxob4jwb2FcWX9jDCxuyCBge7
13kPS1qFNI0eBRw0OLSKKq3A5wH8Oyvx5XzagoRkFcKCz2hF1KqFfLKT+EyBViBWbR
14lW6qMTO+A26VJsslpzaTpfnQrTU3mfyUxJB3+IPVQ0Mcibt2jBdfYJCq0/W+DIGe
15RtTjJIFt
16=u6Rr
17-----END PGP SIGNATURE-----

Timestamp of file with hash fcca790e597327e6dbf958e78e5e5c02c0911ef653160c591bf56423b214127d -

jnewbery commented at 11:50 pm on April 30, 2019: member

utACK effe81f7503d2ca3c88cfdea687f9f997f353e0d @Empact - apologies for not reACKing this earlier. Force-pushes don’t trigger new notifications in github. Feel free to ping me or add a comment to the PR if you need a reACK

MarcoFalke merged this on May 1, 2019

MarcoFalke closed this on May 1, 2019

MarcoFalke referenced this in commit 12aa2ac988 on May 1, 2019

sidhujag referenced this in commit 8a975dc539 on May 1, 2019

Empact deleted the branch on May 2, 2019

Mengerian referenced this in commit f316cd272e on Oct 31, 2019

jonspock referenced this in commit 9107c980e9 on Dec 28, 2019

jonspock referenced this in commit 42d6068686 on Dec 29, 2019

jonspock referenced this in commit 7d544c6ecd on Dec 29, 2019

furszy referenced this in commit a51b254f27 on Mar 20, 2021

vijaydasmp referenced this in commit 185d1f3546 on Oct 21, 2021

vijaydasmp referenced this in commit e27c1a393b on Oct 21, 2021

vijaydasmp referenced this in commit 47375dbecd on Oct 22, 2021

vijaydasmp referenced this in commit 8e8588a542 on Oct 22, 2021

vijaydasmp referenced this in commit bfe027d095 on Oct 23, 2021

vijaydasmp referenced this in commit b9561673fa on Oct 26, 2021

vijaydasmp referenced this in commit 19ba92ca28 on Dec 6, 2021

vijaydasmp referenced this in commit 1634550d9e on Dec 10, 2021

vijaydasmp referenced this in commit d5e24debd4 on Dec 10, 2021

vijaydasmp referenced this in commit 71941b7c23 on Dec 10, 2021

vijaydasmp referenced this in commit 1520ebfaa2 on Dec 11, 2021

vijaydasmp referenced this in commit f6bc96c0ca on Dec 11, 2021

vijaydasmp referenced this in commit 9966abbd90 on Dec 11, 2021

vijaydasmp referenced this in commit 92032654d6 on Dec 11, 2021

vijaydasmp referenced this in commit 4a2dceb202 on Dec 11, 2021

vijaydasmp referenced this in commit 75ad56190f on Dec 13, 2021

vijaydasmp referenced this in commit c5f428c2d2 on Dec 13, 2021

vijaydasmp referenced this in commit 3c44a4aa24 on Dec 13, 2021

vijaydasmp referenced this in commit 4bb33614e8 on Dec 14, 2021

vijaydasmp referenced this in commit b25e8edf15 on Dec 14, 2021

vijaydasmp referenced this in commit 516865bf80 on Dec 15, 2021

vijaydasmp referenced this in commit 497b047cc6 on Dec 15, 2021

DrahtBot locked this on Dec 16, 2021

rpc: Expose g_is_mempool_loaded via getmempoolinfo #15323

Conflicts