node: Add reference to mempool in NodeContext #17407

pull MarcoFalke wants to merge 3 commits into bitcoin:master from MarcoFalke:1911-txPoolOptional changing 11 files +100 −75
  1. MarcoFalke commented at 9:52 pm on November 7, 2019: member

    This is the first step toward making the mempool a global that is not initialized before main.

    Motivation

    Currently the mempool is a global that is initialized before the main function. This is confusing and easy to get wrong. E.g. the mempool constructor queries state that has not been initialized, like randomness (fixed), or command line arguments (not an issue last time I checked). Also without having the chainstate (chain tip) initialized first, it doesn’t make conceptually sense to have a mempool, since the mempool builds txs on top of the utxo set (chain tip).

    Finally, in the future someone might want to run a consensus-only full node (-nowallet -noblockfilter -no... -nomempool command line options) that only verifies blocks and updates the utxo set.

    This is conceptually the same change that has already been done for the connection manager CConnman.

  2. MarcoFalke added the label Refactoring on Nov 7, 2019
  3. MarcoFalke added the label Mempool on Nov 7, 2019
  4. in src/rpc/blockchain.cpp:522 in ae73b97223 outdated 
    517@@ -518,7 +518,9 @@ static UniValue getrawmempool(const JSONRPCRequest& request)
518     if (!request.params[0].isNull())
519         fVerbose = request.params[0].get_bool();
520 
521-    return MempoolToJSON(::mempool, fVerbose);
522+    const CTxMemPool* tx_pool = MempoolInstance();
523+    CHECK_NONFATAL(tx_pool);
    ariard commented at 10:53 pm on November 7, 2019:
    Due to RPC requests only being served after SetRPCWarmupFinished even after mempool is switched to a struct initialized during init sequence, it shouldn’t never be null (but better to be secure if we refactor our init logic)
    MarcoFalke commented at 11:04 pm on November 7, 2019:
    Yes, obviously this should be adjusted when the mempool is truly optional. However I don’t see the reason to add dead code right now.
    ariard commented at 11:11 pm on November 7, 2019:
    My point was more currently the CHECK_FATAL is useless because RPC server accepting requests is the last thing in our init sequence. CHECK_FATAL hitting would be a glaring logic error.
    MarcoFalke commented at 11:19 pm on November 7, 2019:

    CHECK_FATAL is useless

    CHECK_NONFATAL is not useless. Its exact use case it to be used when the condition is assumed to be always true. This is the case here. See https://dev.visucore.com/bitcoin/doxygen/check_8h.html#a46a3e27097aa5e94bbf62075bad7016f

    I plan to introduce a -nomempool command line option, which makes the mempool go away (optional). When I fail to update this line of code, it will turn into a logic error and will properly throw a bug report.

  5. ariard approved
  6. ariard commented at 10:58 pm on November 7, 2019: member

    Code review ACK ae73b97.

    Going further in this direction, what would be the next steps ? Passing a mempool pointer to g_chainstate ?

  7. MarcoFalke commented at 11:03 pm on November 7, 2019: member

    Going further in this direction, what would be the next steps ? Passing a mempool pointer to g_chainstate ?

    good question. I have decided against that to not interwind chainstate (consensus) with mempool (policy). Otherwise this should be a member function: CChainState::GetMempool.

  8. ariard commented at 11:15 pm on November 7, 2019: member

    I have decided against that to not interwind chainstate (consensus) with mempool (policy)

    That means keeping ::mempool as a global on the long-term? Anyway better to process incrementally, curious of your next PRs on this.

  9. MarcoFalke commented at 11:21 pm on November 7, 2019: member

    That means keeping ::mempool as a global on the long-term? Anyway better to process incrementally, curious of your next PRs on this.

    Yes, but it will become Optional<CTxMemPool> (or similar)

  10. in src/validation.cpp:138 in ae73b97223 outdated 
    131@@ -132,6 +132,10 @@ CTxMemPool mempool(&feeEstimator);
132 /** Constant stuff for coinbase transactions we create: */
133 CScript COINBASE_FLAGS;
134 
135+CTxMemPool* MempoolInstance()
136+{
137+    return &::mempool;
138+}
    ryanofsky commented at 1:56 am on November 8, 2019:

    If this function is supposed to replace uses of ::mempool in existing code, I think it’d be better to return a reference:

    0CTxMemPool& MempoolInstance()
1{
2    return ::mempool;
3}

    now and change it to:

    0CTxMemPool& MempoolInstance()
1{
2    CHECK_NONFATAL(::g_mempool);
3    return *::g_mempool;
4}

    in the future when mempool is replaced by an optional or unique_ptr or similar.

    Using a reference instead of a pointer for existing code is better because it won’t burden the code with checking for nulls.

    re: https://github.com/bitcoin/bitcoin/pull/17407/files#issue-338259412

    Finally, in the future someone might want to run a consensus-only full node"

    If new code is going to be added or existing code is going to be changed that expects the mempool to be null, this code could probably just access g_mempool directly instead of using the helper function. Or you could add different helper functions CTxMemPool* OptionalMempoolInstance() or bool MempoolEnabled() if referencing the global is a problem.

    MarcoFalke commented at 2:11 am on November 8, 2019:

    If this function is supposed to replace uses of ::mempool in existing code, I think it’d be better to return a reference:

    It is not supposed to replace ::mempool in existing code. It is supposed to be what you refer to as “different helper function”. I am happy to rename it to OptionalMempoolInstance, but I think that that is a bit too verbose.

    ryanofsky commented at 2:48 am on November 8, 2019:
    Unclear what the next steps are here. It’s hard to see how a function that doesn’t log or check anything but just returns a pointer to a global could be useful, but it’d help if you could describe more literally how the function and global and function calls will change in the future.
  11. jnewbery commented at 3:00 am on November 8, 2019: member

    Currently the mempool is a global that is initialized before the main function.

    This PR doesn’t appear to change that, but it sounds like this is the first step in some larger changes. Perhaps you could link to a branch that illustrates what you’re planning to do next.

  12. MarcoFalke added the label Waiting for author on Nov 8, 2019
  13. MarcoFalke commented at 3:06 am on November 8, 2019: member
    Ok, will upload my branches tomorrow
  14. MarcoFalke force-pushed on Nov 8, 2019
  15. MarcoFalke removed the label Waiting for author on Nov 8, 2019
  16. DrahtBot commented at 4:28 pm on November 8, 2019: member

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

    Conflicts

    No conflicts as of last run.

  17. MarcoFalke renamed this:
    Add MempoolInstance() to get the current mempool
    node: Add reference to mempool in NodeContext
    on Nov 8, 2019
  18. MarcoFalke force-pushed on Nov 8, 2019
  19. MarcoFalke commented at 5:51 pm on November 8, 2019: member
    Ok, added some more documentation about future code design in the first commit
  20. in src/rpc/blockchain.cpp:57 in fa5329abb9 outdated 
    53@@ -53,6 +54,14 @@ static Mutex cs_blockchange;
54 static std::condition_variable cond_blockchange;
55 static CUpdatedBlock latestblock;
56 
57+CTxMemPool& EnsureMemPool()
    ryanofsky commented at 6:56 pm on November 14, 2019:

    In commit “node: Add reference to mempool in NodeContext” (fa5329abb91ec4876f61967848901c6805ab5503)

    This looks good. It would be nice in the future to have similar functions for connman and banman in rpc/net.cpp to minimize uses of g_rpc_node, and help that global go away at some point.

    (Maybe relevant: One way g_rpc_node might go away is adding a std::any context member to JSONRPCRequest assigned to the node context, and having functions like EnsureMempool take request arguments and pull the context from the request.)

    MarcoFalke commented at 8:51 pm on November 14, 2019:
    Sounds like a nice follow-up pull request
  21. in src/rpc/blockchain.cpp:60 in fa5329abb9 outdated 
    53@@ -53,6 +54,14 @@ static Mutex cs_blockchange;
54 static std::condition_variable cond_blockchange;
55 static CUpdatedBlock latestblock;
56 
57+CTxMemPool& EnsureMemPool()
58+{
59+    if (!g_rpc_node->mempool) {
    ryanofsky commented at 6:58 pm on November 14, 2019:

    In commit “node: Add reference to mempool in NodeContext” (fa5329abb91ec4876f61967848901c6805ab5503)

    Could also check g_rpc_node is not null, maybe using assert

    MarcoFalke commented at 8:52 pm on November 14, 2019:
    Done. added assert
  22. in src/rpc/protocol.h:67 in fa5329abb9 outdated 
    62@@ -63,6 +63,9 @@ enum RPCErrorCode
63     RPC_CLIENT_INVALID_IP_OR_SUBNET = -30, //!< Invalid IP/Subnet
64     RPC_CLIENT_P2P_DISABLED         = -31, //!< No valid connection manager instance found
65 
66+    //! Chain errors
67+    RPC_CLIENT_TX_POOL_DISABLED     = -33, //!< No transaction pool instance found
    ryanofsky commented at 7:05 pm on November 14, 2019:

    In commit “node: Add reference to mempool in NodeContext” (fa5329abb91ec4876f61967848901c6805ab5503)

    Would it be clearer to refer to the mempool instead of the tx pool or transaction pool? (I don’t actually know, I’m just used to mempool over transaction pool.)

    MarcoFalke commented at 8:53 pm on November 14, 2019:
    Done. Renamed TX_POOL to MEMPOOL
  23. in src/test/miner_tests.cpp:200 in fa19ddc2bf outdated 
    196@@ -203,6 +197,11 @@ static void TestPackageSelection(const CChainParams& chainparams, const CScript&
197 // NOTE: These tests rely on CreateNewBlock doing its own self-validation!
198 BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
199 {
200+    const auto TestSequenceLocks = [this](const CTransaction& tx, int flags) {
    ryanofsky commented at 7:15 pm on November 14, 2019:

    In commit “test: Replace recursive lock with AssertLockHeld” (fa19ddc2bf8ee8f1ce621611a90f8b9e380b1517)

    Instead of TestSequenceLocks being a lambda and TestPackageSelection in fafe3d88d34f04c798d000ca0b315830d63a80eb taking an unconventionally named m_node argument, I think it might be better if both were test fixture methods:

    0class MinerTest : public TestingSetup
1{
2    bool TestSequenceLocks(...);
3    void TestPackageSelection(...);
4};
5
6bool MinerTest::TestSequenceLocks(...) {...}
7void MinerTest::TestPackageSelection(...) {...}
8
9BOOST_FIXTURE_TEST_CASE(CreateNewBlock_validity, MinerTest) { ... }

    This should make the diffs here smaller and scripted diff simpler because TestSequenceLocks wouldn’t need to move and TestPackageSelection arguments wouldn’t have to change.

    MarcoFalke commented at 8:53 pm on November 14, 2019:
    Fixed, I think
  24. ryanofsky approved
  25. ryanofsky commented at 7:27 pm on November 14, 2019: member
    Light code review ACK fafe3d88d34f04c798d000ca0b315830d63a80eb (can review more, just let me know)
  26. MarcoFalke force-pushed on Nov 14, 2019
  27. MarcoFalke force-pushed on Nov 14, 2019
  28. MarcoFalke commented at 8:53 pm on November 14, 2019: member
    Addressed @ryanofsky feedback
  29. jnewbery commented at 10:37 pm on November 14, 2019: member
    Concept ACK. Can you update the PR description, which makes it sound like this PR changes mempool from being a global that is initialized before main.
  30. MarcoFalke commented at 1:02 pm on November 15, 2019: member
    Updated OP as requested by @jnewbery
  31. ryanofsky approved
  32. ryanofsky commented at 1:22 pm on November 15, 2019: member

    Code review ACK faf24f1e82e77e625bba616412ff5ee3f12e88a5. Just a few suggested changes since last review, constant rename, test / scripted diff cleanup.

    It’d be nice to see followup beginning to make use of the new EnsureMempool function. Could be done either here or in a separate PR depending on how complicated it is.

  33. MarcoFalke commented at 1:44 pm on November 15, 2019: member

    Could be done either here or in a separate PR depending on how complicated it is.

    Not complicated, but not a scripted-diff either. Maybe it could be combined with #17407 (review), but that sounded more complicated to me.

  34. in src/rpc/blockchain.cpp:61 in faf24f1e82 outdated 
    53@@ -53,6 +54,15 @@ static Mutex cs_blockchange;
54 static std::condition_variable cond_blockchange;
55 static CUpdatedBlock latestblock;
56 
57+CTxMemPool& EnsureMemPool()
58+{
59+    CHECK_NONFATAL(g_rpc_node);
60+    if (!g_rpc_node->mempool) {
61+        throw JSONRPCError(RPC_CLIENT_MEMPOOL_DISABLED, "Transaction pool disabled or instance not found");
    jnewbery commented at 5:49 pm on November 15, 2019:
    Please use the name “mempool”. I’ve never heard anyone refer to it as a ’transaction pool'
    MarcoFalke commented at 6:41 pm on November 15, 2019:
    Done. Removed “transaction pool”
  35. jnewbery commented at 6:08 pm on November 15, 2019: member

    Looks good. Please change ’transaction pool’ to mempool in the user facing error message and code comments.

    Is there a reason we can’t make mempool a unique ptr in NodeContext?

  36. MarcoFalke commented at 6:19 pm on November 15, 2019: member

    Is there a reason we can’t make mempool a unique ptr in NodeContext?

    Yes, the memory is not yet managed in NodeContext. The mempool is created before main, and the NodeContext is only created after main.

  37. node: Add reference to mempool in NodeContext 
    Currently it is an alias to the global ::mempool and should be used as
follows.

* Node code (validation and transaction relay) can use either ::mempool
  or node.mempool, whichever seems a better fit.
* RPC code should use the added convenience getter EnsureMempool, which
  makes sure the mempool exists before use. This prepares the RPC code
  to a future where the mempool might be disabled at runtime or compile
  time.
* Test code should use m_node.mempool directly, as the mempool is always
  initialized for tests.
    fac07f2038
  38. test: Replace recursive lock with locking annotations 
    Also, use m_node.mempool instead of the global
    8888ad02e2
  39. scripted-diff: Replace ::mempool with m_node.mempool in tests 
    -BEGIN VERIFY SCRIPT-
 # tx pool member access (mempool followed by dot)
 sed --regexp-extended -i -e 's/(::)?\<mempool\>\.([a-zA-Z])/m_node.mempool->\2/g' $(git grep -l mempool ./src/test)
 # plain global (mempool not preceeded by dot, but followed by comma)
 sed --regexp-extended -i -e 's/([^\.])(::)?\<mempool\>,/\1*m_node.mempool,/g'     $(git grep -l mempool ./src/test)
-END VERIFY SCRIPT-
    fa538813b1
  40. MarcoFalke force-pushed on Nov 15, 2019
  41. jnewbery commented at 6:52 pm on November 15, 2019: member

    utACK fa538813b1c382cf135cbf2a0cc3fa01f36964d8

    Looking forward to mempool being a unique ptr managed by NodeContext in a future PR.

  42. in src/rpc/protocol.h:66 in fac07f2038 outdated 
    62@@ -63,6 +63,9 @@ enum RPCErrorCode
63     RPC_CLIENT_INVALID_IP_OR_SUBNET = -30, //!< Invalid IP/Subnet
64     RPC_CLIENT_P2P_DISABLED         = -31, //!< No valid connection manager instance found
65 
66+    //! Chain errors
    ariard commented at 5:06 pm on November 18, 2019:
    nit: I think this change dissociate further chain from mempool, so may qualify better as a mempool error.
    MarcoFalke commented at 2:20 pm on November 20, 2019:
    I think it is fine to keep as is, as mempool and chain are closely related. Also, I’d rather not invalidate ACKs over this.
  43. ariard commented at 5:44 pm on November 18, 2019: member
    Tested ACK fa53881.
  44. MarcoFalke referenced this in commit ae6943620a on Nov 21, 2019
  45. MarcoFalke merged this on Nov 21, 2019
  46. MarcoFalke closed this on Nov 21, 2019
  47. MarcoFalke deleted the branch on Nov 21, 2019
  48. sidhujag referenced this in commit 8202f7b098 on Nov 22, 2019
  49. deadalnix referenced this in commit 6aad0088cf on Jun 8, 2020
  50. deadalnix referenced this in commit 7f47e3411d on Jun 8, 2020
  51. sidhujag referenced this in commit 440519c926 on Nov 10, 2020
  52. MarcoFalke locked this on Dec 16, 2021


MarcoFalke ariard ryanofsky jnewbery DrahtBot

Labels
Refactoring Mempool

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me