Skip to content
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

rpc: Expose g_is_mempool_loaded via getmempoolinfo #15323

Merged
merged 2 commits into from May 1, 2019

Conversation

Projects
None yet
7 participants
@Empact
Copy link
Member

commented Feb 1, 2019

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 Empact force-pushed the Empact:load-mempool-race branch 3 times, most recently from af9fced to 869e669 Feb 1, 2019

@Empact

This comment has been minimized.

Copy link
Member Author

commented Feb 1, 2019

The test failure, for reference:

stdout:
2019-02-01T20:25:18.578000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_₿_🏃_20190201_201908/mempool_persist_74
2019-02-01T20:25:27.680000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/home/travis/build/Empact/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/util.py", line 113, in try_rpc
    fun(*args, **kwds)
  File "/home/travis/build/Empact/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/coverage.py", line 47, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
  File "/home/travis/build/Empact/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/authproxy.py", line 136, in __call__
    raise JSONRPCException(response['error'])
test_framework.authproxy.JSONRPCException: The mempool was not loaded yet (-1)
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
  File "/home/travis/build/Empact/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/test_framework.py", line 174, in main
    self.run_test()
  File "/home/travis/build/Empact/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/mempool_persist.py", line 130, in run_test
    assert_raises_rpc_error(-1, "Unable to dump mempool to disk", self.nodes[1].savemempool)
  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
    assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"
  File "/home/travis/build/Empact/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/util.py", line 119, in try_rpc
    raise AssertionError("Expected substring not found:" + e.error['message'])
AssertionError: Expected substring not found:The mempool was not loaded yet
2019-02-01T20:25:27.682000Z TestFramework (INFO): Stopping nodes
2019-02-01T20:25:27.985000Z TestFramework (WARNING): Not cleaning up dir /tmp/test_runner_₿_🏃_20190201_201908/mempool_persist_74
2019-02-01T20:25:27.985000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/test_runner_₿_🏃_20190201_201908/mempool_persist_74/test_framework.log
2019-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
stderr:
@@ -1508,6 +1509,7 @@ static UniValue getmempoolinfo(const JSONRPCRequest& request)
{},
RPCResult{
"{\n"
" \"loaded\": true|false (boolean) True if the mempool has been loaded/initialized.\n"

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Feb 1, 2019

Member

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

This comment has been minimized.

Copy link
@Empact

Empact Feb 2, 2019

Author Member

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?

This comment has been minimized.

Copy link
@Empact

Empact Feb 2, 2019

Author Member

How about this: ab48289

This comment has been minimized.

Copy link
@promag

promag Feb 2, 2019

Member

nit, why /initialized?

This comment has been minimized.

Copy link
@Empact

Empact Feb 2, 2019

Author Member

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

This comment has been minimized.

Copy link
@laanwj

laanwj Feb 12, 2019

Member

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

This comment has been minimized.

Copy link
@jnewbery

jnewbery Feb 26, 2019

Member

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?")

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Feb 26, 2019

Member

Maybe s/persist/-persistmempool/g?

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Feb 1, 2019

Ref #12863 (comment)

@Empact Empact force-pushed the Empact:load-mempool-race branch from fce31d4 to ab48289 Feb 2, 2019

@promag
Copy link
Member

left a comment

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

g_is_mempool_loaded = !ShutdownRequested();

should change to

 g_is_mempool_loaded |= !ShutdownRequested(); 

@Empact Empact force-pushed the Empact:load-mempool-race branch 2 times, most recently from 68396b6 to b04c1f3 Feb 2, 2019

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

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

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

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

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

@Empact Empact force-pushed the Empact:load-mempool-race branch 4 times, most recently from fe3b485 to ae12a83 Feb 26, 2019

@Empact

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2019

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:

    bitcoin/src/init.cpp

    Lines 238 to 240 in 8f470ec

    if (g_is_mempool_loaded && gArgs.GetArg("-persistmempool", DEFAULT_PERSIST_MEMPOOL)) {
    DumpMempool();
    }

@Empact Empact force-pushed the Empact:load-mempool-race branch from ae12a83 to cf5a189 Mar 4, 2019

@Empact

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2019

Updated comments and documentation as per John and Marco's suggestions #15323 (comment)

@Empact Empact force-pushed the Empact:load-mempool-race branch from cf5a189 to 14642bc Mar 4, 2019

@Empact Empact force-pushed the Empact:load-mempool-race branch from 14642bc to 32e2018 Mar 7, 2019

@Empact

This comment has been minimized.

Copy link
Member Author

commented Mar 7, 2019

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

@DrahtBot DrahtBot removed the Needs rebase label Mar 7, 2019

@Empact Empact force-pushed the Empact:load-mempool-race branch from 32e2018 to d2870af Mar 9, 2019

@jnewbery

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

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 Empact force-pushed the Empact:load-mempool-race branch from d2870af to 1623a2a Mar 21, 2019

Empact added some commits Feb 1, 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.
Move g_is_mempool_loaded into CTxMemPool::m_is_loaded
So the loaded state is explicitly mempool-specific.

@Empact Empact force-pushed the Empact:load-mempool-race branch from 1623a2a to effe81f Mar 22, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

utACK effe81f

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

utACK effe81f750
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgQDgv/Vu+DeQYZkt0Au46dAPXOWF5uIfLHOGhOHbKB+YZ6zFNIrmw6VF5XrYDl
IJqLlqX2WtDQd8gYcQ9dDvVSBktaJrk6Qm9laEX8qITMavz5VHK8rHfqmJ9nrqC8
DNgWMlwhoQsJqflqXeaUKnkXv9eoMLvuHVn+F3kDkr9Wu/AjRjLxu1zaE9qnr0sl
lO8Z+mdVig4NDWbCHwZHyvP+eM8skn5WeEXTSi6Vl1yqgEP9T/9TEFQtlQ1ikTjn
t+ACMSxzfest155gsZt0vxn5ABlfw1phw3b/x6DEFIFNzs0cBYVRxu8kPhTarqYL
gMcrAvVlfs9knGDUaU4XaZbfor/cQRAduFCKqowSxob4jwb2FcWX9jDCxuyCBge7
kPS1qFNI0eBRw0OLSKKq3A5wH8Oyvx5XzagoRkFcKCz2hF1KqFfLKT+EyBViBWbR
lW6qMTO+A26VJsslpzaTpfnQrTU3mfyUxJB3+IPVQ0Mcibt2jBdfYJCq0/W+DIGe
RtTjJIFt
=u6Rr
-----END PGP SIGNATURE-----

Timestamp of file with hash fcca790e597327e6dbf958e78e5e5c02c0911ef653160c591bf56423b214127d -

@jnewbery

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

utACK effe81f

@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 MarcoFalke merged commit effe81f into bitcoin:master May 1, 2019

2 checks passed

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

MarcoFalke added a commit that referenced this pull request May 1, 2019

Merge #15323: rpc: Expose g_is_mempool_loaded via getmempoolinfo
effe81f Move g_is_mempool_loaded into CTxMemPool::m_is_loaded (Ben Woosley)
bb8ae2c rpc: Expose g_is_mempool_loaded via getmempoolinfo and /rest/mempool/info.json (Ben Woosley)

Pull request description:

  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

ACKs for commit effe81:
  MarcoFalke:
    utACK effe81f
  jnewbery:
    utACK effe81f

Tree-SHA512: 74328b0c17a97efb8a000d4ee49b9a673c2b6dde7ea30c43a6a2eff961a233351c9471f9a42344412135786c02bdf2ee1b2526651bb8fed68bd94d2120c4ef86

sidhujag added a commit to syscoin/syscoin that referenced this pull request May 1, 2019

@Empact Empact deleted the Empact:load-mempool-race branch May 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.