rpc: Expose g_is_mempool_loaded via getmempoolinfo #15323

Empact · 2019-02-01T22:14:42Z

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.

Empact · 2019-02-01T22:46:09Z

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:

maflcko · 2019-02-01T23:04:52Z

src/rpc/blockchain.cpp

@@ -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"


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

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?

How about this: ab48289

nit, why /initialized?

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

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

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

Maybe s/persist/-persistmempool/g?

maflcko · 2019-02-01T23:15:46Z

Ref #12863 (comment)

promag

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

bitcoin/src/init.cpp

Line 707 in b3a7153

g_is_mempool_loaded = !ShutdownRequested();

should change to

 g_is_mempool_loaded |= !ShutdownRequested();

DrahtBot · 2019-02-25T16:24:07Z

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 · 2019-02-25T16:33:25Z

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

Empact · 2019-02-26T09:40:46Z

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 · 2019-03-04T00:09:19Z

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

Empact · 2019-03-07T15:15:36Z

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

jnewbery · 2019-03-21T17:06:58Z

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.

test/functional/mempool_persist.py

Summary: And use it to fix a race condition in mempool_persist.py: https://travis-ci.org/Empact/bitcoin/jobs/487577243 Backport of Bitcoin Core PR15323 bitcoin/bitcoin#15323 Depends on D4234 and D4266. Test Plan: ``` ninja check-all ``` - Read the docs. Reviewers: Fabien, #bitcoin_abc, deadalnix Reviewed By: Fabien, #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D4243

2488ebc test: move sync_blocks and sync_mempool functions to test_framework.py (random-zebra) d076c81 [Test] Fix intermittent sync_blocks failures (random-zebra) f0de789 Move g_is_mempool_loaded into CTxMemPool::m_is_loaded (Ben Woosley) 0169a75 rpc: Expose g_is_mempool_loaded via getmempoolinfo and /rest/mempool/info.json (Ben Woosley) Pull request description: Backport the following PRs: - bitcoin#15323: rpc: Expose g_is_mempool_loaded via getmempoolinfo - bitcoin#18873: test: Fix intermittent sync_blocks failures - bitcoin#19208: test: move sync_blocks and sync_mempool functions to test_framework.py ACKs for top commit: furszy: nice finding!, code review ACK 2488ebc. Fuzzbawls: ACK 2488ebc Tree-SHA512: 4971a0076b4179b78c5338073a10fbf0649310c2cf3907cff1ab5c02f8dd67f5f7bb4bf524d69c554c8317bd98ce46411ff740aff676ed147525c75d83214976

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 bitcoin#12863 ACKs for commit effe81: MarcoFalke: utACK effe81f jnewbery: utACK effe81f Tree-SHA512: 74328b0c17a97efb8a000d4ee49b9a673c2b6dde7ea30c43a6a2eff961a233351c9471f9a42344412135786c02bdf2ee1b2526651bb8fed68bd94d2120c4ef86

Empact force-pushed the load-mempool-race branch 3 times, most recently from af9fced to 869e669 Compare February 1, 2019 22:37

maflcko reviewed Feb 1, 2019

View reviewed changes

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

Empact force-pushed the load-mempool-race branch from fce31d4 to ab48289 Compare February 2, 2019 01:11

promag reviewed Feb 2, 2019

View reviewed changes

Empact force-pushed the load-mempool-race branch 2 times, most recently from 68396b6 to b04c1f3 Compare February 2, 2019 10:12

maflcko added this to the 0.19.0 milestone Feb 22, 2019

Empact force-pushed the load-mempool-race branch 4 times, most recently from fe3b485 to ae12a83 Compare February 26, 2019 09:35

Empact force-pushed the load-mempool-race branch from ae12a83 to cf5a189 Compare March 4, 2019 00:07

Empact force-pushed the load-mempool-race branch from cf5a189 to 14642bc Compare March 4, 2019 00:10

DrahtBot added the Needs rebase label Mar 6, 2019

Empact force-pushed the load-mempool-race branch from 14642bc to 32e2018 Compare March 7, 2019 15:04

DrahtBot removed the Needs rebase label Mar 7, 2019

Empact force-pushed the load-mempool-race branch from 32e2018 to d2870af Compare March 9, 2019 16:26

maflcko reviewed Mar 21, 2019

View reviewed changes

test/functional/mempool_persist.py Outdated Show resolved Hide resolved

Empact force-pushed the load-mempool-race branch from d2870af to 1623a2a Compare March 21, 2019 19:02

random-zebra mentioned this pull request Mar 19, 2021

[Tests] sync_blocks and sync_mempool improvements PIVX-Project/PIVX#2254

Merged

This was referenced Dec 9, 2021

Backports from Bitcoin litecoin-project/litecoin#763

Closed

Backports from Bitcoin BTCGPU/BTCGPU#437

Open

Backports from Bitcoin digibyte/digibyte#238

Open

CharesFang mentioned this pull request Dec 11, 2021

Backports from Bitcoin dashpay/dash#4616

Closed

7 tasks

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

rpc: Expose g_is_mempool_loaded via getmempoolinfo #15323

rpc: Expose g_is_mempool_loaded via getmempoolinfo #15323

Uh oh!

Conversation

Empact commented Feb 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact commented Feb 1, 2019

Uh oh!

maflcko Feb 1, 2019

Choose a reason for hiding this comment

Uh oh!

Empact Feb 2, 2019 • 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!

Empact Feb 2, 2019 • 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!

promag Feb 2, 2019

Choose a reason for hiding this comment

Uh oh!

Empact Feb 2, 2019

Choose a reason for hiding this comment

Uh oh!

laanwj Feb 12, 2019

Choose a reason for hiding this comment

Uh oh!

jnewbery Feb 26, 2019

Choose a reason for hiding this comment

Uh oh!

maflcko Feb 26, 2019

Choose a reason for hiding this comment

Uh oh!

maflcko commented Feb 1, 2019

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Feb 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

promag commented Feb 25, 2019

Uh oh!

Empact commented Feb 26, 2019

Uh oh!

Empact commented Mar 4, 2019

Uh oh!

Empact commented Mar 7, 2019

Uh oh!

jnewbery commented Mar 21, 2019

Uh oh!

Uh oh!

Uh oh!

Empact commented Feb 1, 2019 •

edited

Loading

Empact Feb 2, 2019 •

edited

Loading

Empact Feb 2, 2019 •

edited

Loading

DrahtBot commented Feb 25, 2019 •

edited

Loading