node: Add reference to mempool in NodeContext #17407

maflcko · 2019-11-07T21:52:27Z

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.

ariard

Code review ACK ae73b97.

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

src/rpc/blockchain.cpp

maflcko · 2019-11-07T23:03:37Z

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.

ariard · 2019-11-07T23:15:07Z

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.

maflcko · 2019-11-07T23:21:08Z

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)

src/validation.cpp

jnewbery · 2019-11-08T03:00:28Z

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.

maflcko · 2019-11-08T03:06:59Z

Ok, will upload my branches tomorrow

DrahtBot · 2019-11-08T16:28:30Z

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

Conflicts

No conflicts as of last run.

maflcko · 2019-11-08T17:51:00Z

Ok, added some more documentation about future code design in the first commit

ryanofsky

Light code review ACK fafe3d8 (can review more, just let me know)

ryanofsky · 2019-11-14T18:56:01Z

src/rpc/blockchain.cpp

@@ -53,6 +54,14 @@ static Mutex cs_blockchange;
 static std::condition_variable cond_blockchange;
 static CUpdatedBlock latestblock;

+CTxMemPool& EnsureMemPool()


In commit "node: Add reference to mempool in NodeContext" (fa5329a)

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.)

Sounds like a nice follow-up pull request

src/rpc/blockchain.cpp

src/rpc/protocol.h

src/test/miner_tests.cpp

maflcko · 2019-11-14T20:53:57Z

Addressed @ryanofsky feedback

jnewbery · 2019-11-14T22:37:48Z

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.

maflcko · 2019-11-15T13:02:16Z

Updated OP as requested by @jnewbery

ryanofsky

Code review ACK faf24f1. 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.

maflcko · 2019-11-15T13:44:00Z

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 (comment), but that sounded more complicated to me.

jnewbery

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?

src/rpc/blockchain.cpp

maflcko · 2019-11-15T18:19:51Z

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.

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.

Also, use m_node.mempool instead of the global

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

jnewbery · 2019-11-15T18:52:33Z

utACK fa53881

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

ariard

Tested ACK fa53881.

ariard · 2019-11-18T17:06:06Z

src/rpc/protocol.h

@@ -63,6 +63,9 @@ enum RPCErrorCode
    RPC_CLIENT_INVALID_IP_OR_SUBNET = -30, //!< Invalid IP/Subnet
    RPC_CLIENT_P2P_DISABLED         = -31, //!< No valid connection manager instance found

+    //! Chain errors


nit: I think this change dissociate further chain from mempool, so may qualify better as a mempool error.

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.

fa53881 scripted-diff: Replace ::mempool with m_node.mempool in tests (MarcoFalke) 8888ad0 test: Replace recursive lock with locking annotations (MarcoFalke) fac07f2 node: Add reference to mempool in NodeContext (MarcoFalke) Pull request description: 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`. ACKs for top commit: jnewbery: utACK fa53881 ariard: Tested ACK fa53881. Tree-SHA512: 2c446a8a51476354aad7126c2b833500d36b24490caa94f847b2bdc622054de0dae28980f23e3d91b1b492dc32931656d98dbd019af9e4e58f2f8c5375aac694

Summary: 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. This is a partial backport of Core [[bitcoin/bitcoin#17407 | PR17407]] : bitcoin/bitcoin@fac07f2 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D6437

Summary: Also, use m_node.mempool instead of the global This is a partial backport of Core [[bitcoin/bitcoin#17407 | PR17407]] : bitcoin/bitcoin@8888ad0 Depends on D6437 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D6438

fa53881 scripted-diff: Replace ::mempool with m_node.mempool in tests (MarcoFalke) 8888ad0 test: Replace recursive lock with locking annotations (MarcoFalke) fac07f2 node: Add reference to mempool in NodeContext (MarcoFalke) Pull request description: 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`. ACKs for top commit: jnewbery: utACK fa53881 ariard: Tested ACK fa53881. Tree-SHA512: 2c446a8a51476354aad7126c2b833500d36b24490caa94f847b2bdc622054de0dae28980f23e3d91b1b492dc32931656d98dbd019af9e4e58f2f8c5375aac694

maflcko added Refactoring Mempool labels Nov 7, 2019

ariard approved these changes Nov 7, 2019

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

ryanofsky reviewed Nov 8, 2019

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

maflcko added the Waiting for author label Nov 8, 2019

maflcko force-pushed the 1911-txPoolOptional branch from ae73b97 to faf3207 Compare November 8, 2019 15:31

maflcko removed the Waiting for author label Nov 8, 2019

maflcko changed the title ~~Add MempoolInstance() to get the current mempool~~ node: Add reference to mempool in NodeContext Nov 8, 2019

maflcko force-pushed the 1911-txPoolOptional branch from faf3207 to fafe3d8 Compare November 8, 2019 17:12

maflcko mentioned this pull request Nov 13, 2019

Turn the global mempool into a shared pointer jnewbery/bitcoin#8

Closed

ryanofsky approved these changes Nov 14, 2019

View reviewed changes

maflcko force-pushed the 1911-txPoolOptional branch 2 times, most recently from fa2d0d3 to faf24f1 Compare November 14, 2019 20:44

ryanofsky approved these changes Nov 15, 2019

View reviewed changes

jnewbery reviewed Nov 15, 2019

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

MarcoFalke added 3 commits November 15, 2019 13:40

test: Replace recursive lock with locking annotations

8888ad0

Also, use m_node.mempool instead of the global

maflcko force-pushed the 1911-txPoolOptional branch from faf24f1 to fa53881 Compare November 15, 2019 18:40

ariard reviewed Nov 18, 2019

View reviewed changes

maflcko merged commit fa53881 into bitcoin:master Nov 21, 2019

maflcko deleted the 1911-txPoolOptional branch November 21, 2019 15:23

maflcko mentioned this pull request Nov 21, 2019

rpc: Removing g_rpc_node #17548

Closed

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

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

node: Add reference to mempool in NodeContext #17407

node: Add reference to mempool in NodeContext #17407

Uh oh!

Conversation

maflcko commented Nov 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Nov 7, 2019

Uh oh!

ariard commented Nov 7, 2019

Uh oh!

maflcko commented Nov 7, 2019

Uh oh!

Uh oh!

jnewbery commented Nov 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Nov 8, 2019

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

maflcko commented Nov 8, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Nov 14, 2019

Choose a reason for hiding this comment

Uh oh!

maflcko Nov 14, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko commented Nov 14, 2019

Uh oh!

jnewbery commented Nov 14, 2019

Uh oh!

maflcko commented Nov 15, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Nov 15, 2019

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Nov 15, 2019

Uh oh!

jnewbery commented Nov 15, 2019

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

ariard Nov 18, 2019

Choose a reason for hiding this comment

Uh oh!

maflcko Nov 20, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Nov 7, 2019 •

edited

Loading

jnewbery commented Nov 8, 2019 •

edited

Loading

DrahtBot commented Nov 8, 2019 •

edited

Loading