rpc: Use mempool from node context instead of global

MarcoFalke commented at 8:31 pm on November 22, 2019: member

Currently they are identical, but in the future we might want to turn the mempool into a unique_ptr. Replacing the global with the mempool pointer from the node context simplifies this step.

MarcoFalke added the label Refactoring on Nov 22, 2019

MarcoFalke added the label RPC/REST/ZMQ on Nov 22, 2019

MarcoFalke force-pushed on Nov 22, 2019

DrahtBot commented at 11:41 pm on November 22, 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:

#16426 (Reverse cs_main, cs_wallet lock order and reduce cs_main locking by ariard)
#15836 (Add feerate histogram to getmempoolinfo by jonasschnelli)

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.

laanwj commented at 1:44 pm on November 23, 2019: member

Concept ACK

MarcoFalke force-pushed on Nov 23, 2019

practicalswift commented at 0:09 am on November 25, 2019: contributor

Concept ACK

in src/test/rpc_tests.cpp:117 in fa963d3818 outdated

113@@ -114,6 +114,7 @@ BOOST_AUTO_TEST_CASE(rpc_rawsign)
114     std::string privkey2 = "\"Kyhdf5LuKTRx4ge69ybABsiUAWjVRK4XGxAKk2FQLp2HjGMy87Z4\"";
115     NodeContext node;
116     node.chain = interfaces::MakeChain(node);
117+    node.mempool = std::move(m_node.mempool);

ryanofsky commented at 4:00 pm on November 25, 2019:

In commit “node: Use mempool from node context instead of global” (fa963d3818afea82061615b9939361444e5b5c2d)

I think you can get rid of this code now that the test framework is managing g_rpc_node:

 0--- a/src/test/rpc_tests.cpp
 1+++ b/src/test/rpc_tests.cpp
 2@@ -112,15 +112,10 @@ BOOST_AUTO_TEST_CASE(rpc_rawsign)
 3     std::string notsigned = r.get_str();
 4     std::string privkey1 = "\"KzsXybp9jX64P5ekX1KUxRQ79Jht9uzW7LorgwE65i5rWACL6LQe\"";
 5     std::string privkey2 = "\"Kyhdf5LuKTRx4ge69ybABsiUAWjVRK4XGxAKk2FQLp2HjGMy87Z4\"";
 6-    NodeContext node;
 7-    node.chain = interfaces::MakeChain(node);
 8-    node.mempool = std::move(m_node.mempool);
 9-    g_rpc_node = &node;
10     r = CallRPC(std::string("signrawtransactionwithkey ")+notsigned+" [] "+prevout);
11     BOOST_CHECK(find_value(r.get_obj(), "complete").get_bool() == false);
12     r = CallRPC(std::string("signrawtransactionwithkey ")+notsigned+" ["+privkey1+","+privkey2+"] "+prevout);
13     BOOST_CHECK(find_value(r.get_obj(), "complete").get_bool() == true);
14-    g_rpc_node = nullptr;
15 }

Seems to work

MarcoFalke commented at 4:11 pm on November 25, 2019:

Thanks. Fixed

ryanofsky approved

ryanofsky commented at 4:01 pm on November 25, 2019: member

Code review ACK fa963d3818afea82061615b9939361444e5b5c2d

MarcoFalke force-pushed on Nov 25, 2019

ryanofsky approved

ryanofsky commented at 4:56 pm on November 25, 2019: member

Code review ACK fa7c5017a9056b33e230d78f7ff6879e3d8941a5. Only change since last review is suggested rpc test simplification

in src/rest.cpp:493 in fa7c5017a9 outdated

489@@ -490,21 +490,22 @@ static bool rest_getutxos(HTTPRequest* req, const std::string& strURIPart)
490     std::vector<bool> hits;
491     bitmap.resize((vOutPoints.size() + 7) / 8);
492     {
493-        auto process_utxos = [&vOutPoints, &outs, &hits](const CCoinsView& view, const CTxMemPool& mempool) {
494+        auto process_utxos = [&vOutPoints, &outs, &hits](const CCoinsView& view, const CTxMemPool& tx_pool) {

jnewbery commented at 4:17 pm on December 2, 2019:

I don’t understand your insistence on using “transaction pool” in comments, function and variables names. Adding another name for something that is already commonly called mempool just adds to confusion for new contributors. Can we just call the mempool “the mempool” and be done with it?

instagibbs commented at 5:34 pm on December 3, 2019:

tend to agree. In fact, there are more “transaction pools” like “orphan pool” etc. “Mempool” seems unambiguous in comparison?

MarcoFalke commented at 6:25 pm on December 3, 2019:

I’d rather not use mempool here because it shadows the global of the same name. This is fragile, because the code could be moved around and change its meaning because instead of process_utxos::tx_pool it uses ::mempool instead.

MarcoFalke commented at 6:26 pm on December 3, 2019:

Happy to change it to anything else. Any suggestions?

ryanofsky commented at 6:53 pm on December 3, 2019:

Happy to change it to anything else. Any suggestions?

tx_mempool would be an ugly compromise. But personally, I would call it mempool and not worry about shadowing, because it seems like the global should be removed or renamed to g_mempool anyway, and doing that should be pretty easy, especially after these changes.

MarcoFalke commented at 6:42 pm on December 5, 2019:

Ok, left the name as is with two effects:

Smaller diff
mempool shadows ::mempool until the global is renamed to ::g_mempoool

in src/rest.cpp:504 in fa7c5017a9 outdated

502         };
503 
504         if (fCheckMemPool) {
505             // use db+mempool as cache backend in case user likes to query mempool
506-            LOCK2(cs_main, mempool.cs);
507+            const auto& tx_pool = EnsureMemPool();

jnewbery commented at 4:27 pm on December 2, 2019:

What’s the rationale for using auto here? I generally agree with Matt that we should avoid use of auto unless there are clear benefits: https://github.com/bitcoin/bitcoin/pull/12120/files.

MarcoFalke commented at 6:45 pm on December 5, 2019:

Removed auto

in src/rpc/blockchain.cpp:1076 in fa7c5017a9 outdated

1072@@ -1070,9 +1073,10 @@ UniValue gettxout(const JSONRPCRequest& request)
1073     CCoinsViewCache* coins_view = &::ChainstateActive().CoinsTip();
1074 
1075     if (fMempool) {
1076-        LOCK(mempool.cs);
1077-        CCoinsViewMemPool view(coins_view, mempool);
1078-        if (!view.GetCoin(out, coin) || mempool.isSpent(out)) {
1079+        const auto& tx_pool = EnsureMemPool();

jnewbery commented at 4:30 pm on December 2, 2019:

I think in a future where running without a mempool is possible, we could make this friendlier by just defaulting include_mempool to false for nodes without mempools rather than forcing the user to always pass false as an argument.

(no change needed for this PR but something to add when -nomempool is added as an option)

in src/rpc/mining.cpp:254 in fa7c5017a9 outdated

250@@ -251,7 +251,7 @@ static UniValue getmininginfo(const JSONRPCRequest& request)
251     if (BlockAssembler::m_last_block_num_txs) obj.pushKV("currentblocktx", *BlockAssembler::m_last_block_num_txs);
252     obj.pushKV("difficulty",       (double)GetDifficulty(::ChainActive().Tip()));
253     obj.pushKV("networkhashps",    getnetworkhashps(request));
254-    obj.pushKV("pooledtx",         (uint64_t)mempool.size());
255+    obj.pushKV("pooledtx",         (uint64_t)EnsureMemPool().size());

jnewbery commented at 4:32 pm on December 2, 2019:

I think I’d prefer to have this check explicitly called at the top of this function rather than hidden in a univalue value assignment.

Same is true for other changes in this file. I think it’s better to be explicit at the top of the function that a mempool is going to be required, and fail early if it isn’t available.

MarcoFalke commented at 6:46 pm on December 5, 2019:

Done: Moved EnsureMemPool right after parsing the args where applicable.

in src/rest.cpp:303 in fa7c5017a9 outdated

299@@ -300,7 +300,7 @@ static bool rest_mempool_info(HTTPRequest* req, const std::string& strURIPart)
300 
301     switch (rf) {
302     case RetFormat::JSON: {
303-        UniValue mempoolInfoObject = MempoolInfoToJSON(::mempool);
304+        UniValue mempoolInfoObject = MempoolInfoToJSON(EnsureMemPool());

jnewbery commented at 4:41 pm on December 2, 2019:

Are we really allowed to throw JSONRPCErrors in the rest code? I don’t see it happening anywhere else. Where does it get caught?

MarcoFalke commented at 7:20 pm on December 5, 2019:

good point. Changed it to return 404 when the mempool is not found.

in src/node/coin.h:20 in fa7c5017a9 outdated

17@@ -17,6 +18,6 @@ class Coin;
18  *
19  * @param[in,out] coins map to fill

jnewbery commented at 4:43 pm on December 2, 2019:

Please update doxygen comment

MarcoFalke commented at 6:46 pm on December 5, 2019:

Done

jnewbery commented at 4:47 pm on December 2, 2019: member

Concept ACK and approach ACK.

There are a few places where I think the new code is less clear than existing code, which I think could be improved.

MarcoFalke force-pushed on Dec 5, 2019

rpc: Use mempool from node context instead of global

Currently they are identical, but in the future we might want to turn
the mempool into a unique_ptr. Replacing the global with the mempool
pointer from the node context simplifies this step.

facbaf092f

MarcoFalke force-pushed on Dec 5, 2019

node: Use mempool from node context instead of global fa660d65d7

MarcoFalke force-pushed on Dec 5, 2019

ryanofsky approved

ryanofsky commented at 9:46 pm on December 13, 2019: member

Code review ACK faaa23984e4ce8b6dc71d243ba651fae3fb780f7. There were a lot of changes since my previous review, so I just reviewed it a second time.

practicalswift commented at 10:41 pm on December 13, 2019: contributor

ACK faaa23984e4ce8b6dc71d243ba651fae3fb780f7 – diff looks correct

Thanks for doing this!

in src/rest.cpp:76 in faaa23984e outdated

70@@ -69,6 +71,15 @@ static bool RESTERR(HTTPRequest* req, enum HTTPStatusCode status, std::string me
71     return false;
72 }
73 
74+static CTxMemPool* EnsureMemPool(HTTPRequest* req)
75+{
76+    CHECK_NONFATAL(g_rpc_node);

jnewbery commented at 10:57 pm on December 13, 2019:

Where is the NonFatalCheckError caught?

ryanofsky commented at 11:55 pm on December 13, 2019:

re: #17564 (review)

Where is the NonFatalCheckError caught?

I didn’t look into whether its caught but assumed this was basically functioning like an assert since it can only happen when bitcoind is not running, and in that case an abort, a caught exception, or an uncaught exception should all be preferable to a null dereference

MarcoFalke commented at 3:48 pm on December 16, 2019:

Yes, bitcoind will crash on uncaught exceptions (like this one)

MarcoFalke commented at 3:48 pm on December 16, 2019:

Code was removed, so closing this conv

in src/rest.cpp:310 in faaa23984e outdated

305@@ -295,12 +306,15 @@ static bool rest_mempool_info(HTTPRequest* req, const std::string& strURIPart)
306 {
307     if (!CheckWarmup(req))
308         return false;
309+    const CTxMemPool* mempool_ = EnsureMemPool(req);
310+    if (!mempool_) return false;

jnewbery commented at 10:59 pm on December 13, 2019:

I don’t understand how we can get !mempool_. EnsureMemPool() can’t return a nullptr. Same for the other calls to EnsureMemPool() below

ryanofsky commented at 11:56 pm on December 13, 2019:

re: #17564 (review)

I don’t understand how we can get !mempool_. EnsureMemPool() can’t return a nullptr. Same for the other calls to EnsureMemPool() below

It can be null because this version of EnsureMemPool only sets http status headers and doesn’t throw an exception. Personally I think this code would be a little clearer dropping the mempool reference varaiable, renaming the mempool_ pointer variable to just mempool, and renaming the EnsureMempool function in this file to something like ReportMissingMempool. But these are just style / naming preferences that I didn’t think were worth raising before. I’d be fine with any changes but am also happy with the current PR.

promag commented at 9:41 pm on December 15, 2019:

Or it could just throw an exception leaving the request handlers free of handling these error case. Only change here would be like

0-UniValue mempoolInfoObject = MempoolInfoToJSON(::mempool);
1+UniValue mempoolInfoObject = MempoolInfoToJSON(GetMemPoolOrThrow());

jnewbery commented at 3:52 am on December 15, 2019: member

It can be null because this version of EnsureMemPool only sets http status headers and doesn’t throw an exception.

Ah, you’re right. I was confused because the name suggests that it ensures the return value is valid, and the other version of EnsureMemPool throws if the mempool can’t be found.

I’ve implemented a version of Russ’s suggested changes here: https://github.com/jnewbery/bitcoin/commit/d27fb22337c28a4c38583cbf4d7bf714577d2886. What do you think, @MarcoFalke ?

promag commented at 9:50 pm on December 15, 2019: member

Code review ACK faaa23984e4ce8b6dc71d243ba651fae3fb780f7.

Why not also change ::mempool usages in ChainImpl?

rest: Use mempool from node context instead of global fa8e650b52

MarcoFalke force-pushed on Dec 16, 2019

MarcoFalke commented at 3:47 pm on December 16, 2019: member

Fixed up and force pushed commit by @jnewbery with suggested rename

ryanofsky approved

ryanofsky commented at 7:23 pm on December 16, 2019: member

Code review ACK fa8e650b525e9493bdfa393c0c3e34cb22c78c08, Only the discussed REST server changes since the last review.

jnewbery commented at 8:49 pm on December 16, 2019: member

Code review ACK fa8e650b5

MarcoFalke referenced this in commit 48d64d73c0 on Dec 16, 2019

MarcoFalke merged this on Dec 16, 2019

MarcoFalke closed this on Dec 16, 2019

MarcoFalke deleted the branch on Dec 16, 2019

sidhujag referenced this in commit d81f32ae67 on Dec 16, 2019

jasonbcox referenced this in commit 73d648a236 on Oct 16, 2020

sidhujag referenced this in commit 49046e1e33 on Nov 10, 2020

DrahtBot locked this on Feb 15, 2022

rpc: Use mempool from node context instead of global #17564

Conflicts