Skip to content

Commit 25ad2c6

Browse files
author
MarcoFalke
committed
Merge #18740: Remove g_rpc_node global
b3f7f37 refactor: Remove g_rpc_node global (Russell Yanofsky) ccb5059 scripted-diff: Remove g_rpc_node references (Russell Yanofsky) 6fca33b refactor: Pass NodeContext to RPC and REST methods through util::Ref (Russell Yanofsky) 691c817 Add util::Ref class as temporary alternative for c++17 std::any (Russell Yanofsky) Pull request description: This PR removes the `g_rpc_node` global, to get same benefits we see removing other globals and make RPC code more testable, modular, and reusable. This uses a hybrid of the approaches suggested in #17548. Instead of using `std::any`, which isn't available in c++11, or `void*`, which isn't type safe, it uses a small new `util::Ref` helper class, which acts like a simplified `std::any` that only holds references, not values. Motivation for writing this was to provide an simpler alternative to #18647 by Harris Brakmić (brakmic) which avoids some shortcomings of that PR (bitcoin/bitcoin#18647 (comment)) ACKs for top commit: MarcoFalke: re-ACK b3f7f37, only change is adding back const and more tests 🚾 ajtowns: ACK b3f7f37 Tree-SHA512: 56292268a001bdbe34d641db1180c215351503966ff451e55cc96c9137f1d262225d7d7733de9c9da7ce7d7a4b34213a98c2476266b58c89dbbb0f3cb5aa5d70
2 parents 97b21b3 + b3f7f37 commit 25ad2c6

22 files changed

+262
-131
lines changed

‎src/Makefile.am

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,7 @@ BITCOIN_CORE_H = \
227227
util/message.h \
228228
util/moneystr.h \
229229
util/rbf.h \
230+
util/ref.h \
230231
util/settings.h \
231232
util/string.h \
232233
util/threadnames.h \

‎src/Makefile.test.include

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -229,6 +229,7 @@ BITCOIN_TESTS =\
229229
test/prevector_tests.cpp \
230230
test/raii_event_tests.cpp \
231231
test/random_tests.cpp \
232+
test/ref_tests.cpp \
232233
test/reverselock_tests.cpp \
233234
test/rpc_tests.cpp \
234235
test/sanity_tests.cpp \

‎src/bitcoind.cpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
#include <noui.h>
1717
#include <shutdown.h>
1818
#include <ui_interface.h>
19+
#include <util/ref.h>
1920
#include <util/strencodings.h>
2021
#include <util/system.h>
2122
#include <util/threadnames.h>
@@ -77,6 +78,7 @@ static bool AppInit(int argc, char* argv[])
7778
return true;
7879
}
7980

81+
util::Ref context{node};
8082
try
8183
{
8284
if (!CheckDataDirOption()) {
@@ -145,7 +147,7 @@ static bool AppInit(int argc, char* argv[])
145147
// If locking the data directory failed, exit immediately
146148
return false;
147149
}
148-
fRet = AppInitMain(node);
150+
fRet = AppInitMain(context, node);
149151
}
150152
catch (const std::exception& e) {
151153
PrintExceptionContinue(&e, "AppInit()");

‎src/httprpc.cpp

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@ static bool RPCAuthorized(const std::string& strAuth, std::string& strAuthUserna
150150
return multiUserAuthorized(strUserPass);
151151
}
152152

153-
static bool HTTPReq_JSONRPC(HTTPRequest* req, const std::string &)
153+
static bool HTTPReq_JSONRPC(const util::Ref& context, HTTPRequest* req)
154154
{
155155
// JSONRPC handles only POST
156156
if (req->GetRequestMethod() != HTTPRequest::POST) {
@@ -165,7 +165,7 @@ static bool HTTPReq_JSONRPC(HTTPRequest* req, const std::string &)
165165
return false;
166166
}
167167

168-
JSONRPCRequest jreq;
168+
JSONRPCRequest jreq(context);
169169
jreq.peerAddr = req->GetPeer().ToString();
170170
if (!RPCAuthorized(authHeader.second, jreq.authUser)) {
171171
LogPrintf("ThreadRPCServer incorrect password attempt from %s\n", jreq.peerAddr);
@@ -284,15 +284,16 @@ static bool InitRPCAuthentication()
284284
return true;
285285
}
286286

287-
bool StartHTTPRPC()
287+
bool StartHTTPRPC(const util::Ref& context)
288288
{
289289
LogPrint(BCLog::RPC, "Starting HTTP RPC server\n");
290290
if (!InitRPCAuthentication())
291291
return false;
292292

293-
RegisterHTTPHandler("/", true, HTTPReq_JSONRPC);
293+
auto handle_rpc = [&context](HTTPRequest* req, const std::string&) { return HTTPReq_JSONRPC(context, req); };
294+
RegisterHTTPHandler("/", true, handle_rpc);
294295
if (g_wallet_init_interface.HasWalletSupport()) {
295-
RegisterHTTPHandler("/wallet/", false, HTTPReq_JSONRPC);
296+
RegisterHTTPHandler("/wallet/", false, handle_rpc);
296297
}
297298
struct event_base* eventBase = EventBase();
298299
assert(eventBase);

‎src/httprpc.h

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,14 @@
55
#ifndef BITCOIN_HTTPRPC_H
66
#define BITCOIN_HTTPRPC_H
77

8+
namespace util {
9+
class Ref;
10+
} // namespace util
811

912
/** Start HTTP RPC subsystem.
1013
* Precondition; HTTP and RPC has been started.
1114
*/
12-
bool StartHTTPRPC();
15+
bool StartHTTPRPC(const util::Ref& context);
1316
/** Interrupt HTTP RPC subsystem.
1417
*/
1518
void InterruptHTTPRPC();
@@ -21,7 +24,7 @@ void StopHTTPRPC();
2124
/** Start HTTP REST subsystem.
2225
* Precondition; HTTP and RPC has been started.
2326
*/
24-
void StartREST();
27+
void StartREST(const util::Ref& context);
2528
/** Interrupt RPC REST subsystem.
2629
*/
2730
void InterruptREST();

‎src/init.cpp

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -784,16 +784,16 @@ static bool InitSanityCheck()
784784
return true;
785785
}
786786

787-
static bool AppInitServers()
787+
static bool AppInitServers(const util::Ref& context)
788788
{
789789
RPCServer::OnStarted(&OnRPCStarted);
790790
RPCServer::OnStopped(&OnRPCStopped);
791791
if (!InitHTTPServer())
792792
return false;
793793
StartRPC();
794-
if (!StartHTTPRPC())
794+
if (!StartHTTPRPC(context))
795795
return false;
796-
if (gArgs.GetBoolArg("-rest", DEFAULT_REST_ENABLE)) StartREST();
796+
if (gArgs.GetBoolArg("-rest", DEFAULT_REST_ENABLE)) StartREST(context);
797797
StartHTTPServer();
798798
return true;
799799
}
@@ -1238,7 +1238,7 @@ bool AppInitLockDataDirectory()
12381238
return true;
12391239
}
12401240

1241-
bool AppInitMain(NodeContext& node)
1241+
bool AppInitMain(const util::Ref& context, NodeContext& node)
12421242
{
12431243
const CChainParams& chainparams = Params();
12441244
// ********************************************************* Step 4a: application initialization
@@ -1340,7 +1340,6 @@ bool AppInitMain(NodeContext& node)
13401340
for (const auto& client : node.chain_clients) {
13411341
client->registerRpcs();
13421342
}
1343-
g_rpc_node = &node;
13441343
#if ENABLE_ZMQ
13451344
RegisterZMQRPCCommands(tableRPC);
13461345
#endif
@@ -1353,7 +1352,7 @@ bool AppInitMain(NodeContext& node)
13531352
if (gArgs.GetBoolArg("-server", false))
13541353
{
13551354
uiInterface.InitMessage_connect(SetRPCWarmupStatus);
1356-
if (!AppInitServers())
1355+
if (!AppInitServers(context))
13571356
return InitError(_("Unable to start HTTP server. See debug log for details."));
13581357
}
13591358

‎src/init.h

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,9 @@ struct NodeContext;
1414
namespace boost {
1515
class thread_group;
1616
} // namespace boost
17+
namespace util {
18+
class Ref;
19+
} // namespace util
1720

1821
/** Interrupt threads */
1922
void Interrupt(NodeContext& node);
@@ -51,7 +54,7 @@ bool AppInitLockDataDirectory();
5154
* @note This should only be done after daemonization. Call Shutdown() if this function fails.
5255
* @pre Parameters should be parsed and config file should be read, AppInitLockDataDirectory should have been called.
5356
*/
54-
bool AppInitMain(NodeContext& node);
57+
bool AppInitMain(const util::Ref& context, NodeContext& node);
5558

5659
/**
5760
* Register all arguments with the ArgsManager

‎src/interfaces/node.cpp

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
#include <sync.h>
2828
#include <txmempool.h>
2929
#include <ui_interface.h>
30+
#include <util/ref.h>
3031
#include <util/system.h>
3132
#include <util/translation.h>
3233
#include <validation.h>
@@ -80,7 +81,7 @@ class NodeImpl : public Node
8081
bool appInitMain() override
8182
{
8283
m_context.chain = MakeChain(m_context);
83-
return AppInitMain(m_context);
84+
return AppInitMain(m_context_ref, m_context);
8485
}
8586
void appShutdown() override
8687
{
@@ -225,7 +226,7 @@ class NodeImpl : public Node
225226
CFeeRate getDustRelayFee() override { return ::dustRelayFee; }
226227
UniValue executeRpc(const std::string& command, const UniValue& params, const std::string& uri) override
227228
{
228-
JSONRPCRequest req;
229+
JSONRPCRequest req(m_context_ref);
229230
req.params = params;
230231
req.strMethod = command;
231232
req.URI = uri;
@@ -323,6 +324,7 @@ class NodeImpl : public Node
323324
}
324325
NodeContext* context() override { return &m_context; }
325326
NodeContext m_context;
327+
util::Ref m_context_ref{m_context};
326328
};
327329

328330
} // namespace

‎src/rest.cpp

Lines changed: 25 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
#include <sync.h>
1919
#include <txmempool.h>
2020
#include <util/check.h>
21+
#include <util/ref.h>
2122
#include <util/strencodings.h>
2223
#include <validation.h>
2324
#include <version.h>
@@ -75,13 +76,14 @@ static bool RESTERR(HTTPRequest* req, enum HTTPStatusCode status, std::string me
7576
* @param[in] req the HTTP request
7677
* return pointer to the mempool or nullptr if no mempool found
7778
*/
78-
static CTxMemPool* GetMemPool(HTTPRequest* req)
79+
static CTxMemPool* GetMemPool(const util::Ref& context, HTTPRequest* req)
7980
{
80-
if (!g_rpc_node || !g_rpc_node->mempool) {
81+
NodeContext* node = context.Has<NodeContext>() ? &context.Get<NodeContext>() : nullptr;
82+
if (!node || !node->mempool) {
8183
RESTERR(req, HTTP_NOT_FOUND, "Mempool disabled or instance not found");
8284
return nullptr;
8385
}
84-
return g_rpc_node->mempool;
86+
return node->mempool;
8587
}
8688

8789
static RetFormat ParseDataFormat(std::string& param, const std::string& strReq)
@@ -129,7 +131,8 @@ static bool CheckWarmup(HTTPRequest* req)
129131
return true;
130132
}
131133

132-
static bool rest_headers(HTTPRequest* req,
134+
static bool rest_headers(const util::Ref& context,
135+
HTTPRequest* req,
133136
const std::string& strURIPart)
134137
{
135138
if (!CheckWarmup(req))
@@ -270,20 +273,20 @@ static bool rest_block(HTTPRequest* req,
270273
}
271274
}
272275

273-
static bool rest_block_extended(HTTPRequest* req, const std::string& strURIPart)
276+
static bool rest_block_extended(const util::Ref& context, HTTPRequest* req, const std::string& strURIPart)
274277
{
275278
return rest_block(req, strURIPart, true);
276279
}
277280

278-
static bool rest_block_notxdetails(HTTPRequest* req, const std::string& strURIPart)
281+
static bool rest_block_notxdetails(const util::Ref& context, HTTPRequest* req, const std::string& strURIPart)
279282
{
280283
return rest_block(req, strURIPart, false);
281284
}
282285

283286
// A bit of a hack - dependency on a function defined in rpc/blockchain.cpp
284287
UniValue getblockchaininfo(const JSONRPCRequest& request);
285288

286-
static bool rest_chaininfo(HTTPRequest* req, const std::string& strURIPart)
289+
static bool rest_chaininfo(const util::Ref& context, HTTPRequest* req, const std::string& strURIPart)
287290
{
288291
if (!CheckWarmup(req))
289292
return false;
@@ -292,7 +295,7 @@ static bool rest_chaininfo(HTTPRequest* req, const std::string& strURIPart)
292295

293296
switch (rf) {
294297
case RetFormat::JSON: {
295-
JSONRPCRequest jsonRequest;
298+
JSONRPCRequest jsonRequest(context);
296299
jsonRequest.params = UniValue(UniValue::VARR);
297300
UniValue chainInfoObject = getblockchaininfo(jsonRequest);
298301
std::string strJSON = chainInfoObject.write() + "\n";
@@ -306,11 +309,11 @@ static bool rest_chaininfo(HTTPRequest* req, const std::string& strURIPart)
306309
}
307310
}
308311

309-
static bool rest_mempool_info(HTTPRequest* req, const std::string& strURIPart)
312+
static bool rest_mempool_info(const util::Ref& context, HTTPRequest* req, const std::string& strURIPart)
310313
{
311314
if (!CheckWarmup(req))
312315
return false;
313-
const CTxMemPool* mempool = GetMemPool(req);
316+
const CTxMemPool* mempool = GetMemPool(context, req);
314317
if (!mempool) return false;
315318
std::string param;
316319
const RetFormat rf = ParseDataFormat(param, strURIPart);
@@ -330,10 +333,10 @@ static bool rest_mempool_info(HTTPRequest* req, const std::string& strURIPart)
330333
}
331334
}
332335

333-
static bool rest_mempool_contents(HTTPRequest* req, const std::string& strURIPart)
336+
static bool rest_mempool_contents(const util::Ref& context, HTTPRequest* req, const std::string& strURIPart)
334337
{
335338
if (!CheckWarmup(req)) return false;
336-
const CTxMemPool* mempool = GetMemPool(req);
339+
const CTxMemPool* mempool = GetMemPool(context, req);
337340
if (!mempool) return false;
338341
std::string param;
339342
const RetFormat rf = ParseDataFormat(param, strURIPart);
@@ -353,7 +356,7 @@ static bool rest_mempool_contents(HTTPRequest* req, const std::string& strURIPar
353356
}
354357
}
355358

356-
static bool rest_tx(HTTPRequest* req, const std::string& strURIPart)
359+
static bool rest_tx(const util::Ref& context, HTTPRequest* req, const std::string& strURIPart)
357360
{
358361
if (!CheckWarmup(req))
359362
return false;
@@ -409,7 +412,7 @@ static bool rest_tx(HTTPRequest* req, const std::string& strURIPart)
409412
}
410413
}
411414

412-
static bool rest_getutxos(HTTPRequest* req, const std::string& strURIPart)
415+
static bool rest_getutxos(const util::Ref& context, HTTPRequest* req, const std::string& strURIPart)
413416
{
414417
if (!CheckWarmup(req))
415418
return false;
@@ -518,7 +521,7 @@ static bool rest_getutxos(HTTPRequest* req, const std::string& strURIPart)
518521
};
519522

520523
if (fCheckMemPool) {
521-
const CTxMemPool* mempool = GetMemPool(req);
524+
const CTxMemPool* mempool = GetMemPool(context, req);
522525
if (!mempool) return false;
523526
// use db+mempool as cache backend in case user likes to query mempool
524527
LOCK2(cs_main, mempool->cs);
@@ -595,7 +598,7 @@ static bool rest_getutxos(HTTPRequest* req, const std::string& strURIPart)
595598
}
596599
}
597600

598-
static bool rest_blockhash_by_height(HTTPRequest* req,
601+
static bool rest_blockhash_by_height(const util::Ref& context, HTTPRequest* req,
599602
const std::string& str_uri_part)
600603
{
601604
if (!CheckWarmup(req)) return false;
@@ -643,7 +646,7 @@ static bool rest_blockhash_by_height(HTTPRequest* req,
643646

644647
static const struct {
645648
const char* prefix;
646-
bool (*handler)(HTTPRequest* req, const std::string& strReq);
649+
bool (*handler)(const util::Ref& context, HTTPRequest* req, const std::string& strReq);
647650
} uri_prefixes[] = {
648651
{"/rest/tx/", rest_tx},
649652
{"/rest/block/notxdetails/", rest_block_notxdetails},
@@ -656,10 +659,12 @@ static const struct {
656659
{"/rest/blockhashbyheight/", rest_blockhash_by_height},
657660
};
658661

659-
void StartREST()
662+
void StartREST(const util::Ref& context)
660663
{
661-
for (unsigned int i = 0; i < ARRAYLEN(uri_prefixes); i++)
662-
RegisterHTTPHandler(uri_prefixes[i].prefix, false, uri_prefixes[i].handler);
664+
for (const auto& up : uri_prefixes) {
665+
auto handler = [&context, up](HTTPRequest* req, const std::string& prefix) { return up.handler(context, req, prefix); };
666+
RegisterHTTPHandler(up.prefix, false, handler);
667+
}
663668
}
664669

665670
void InterruptREST()

0 commit comments

Comments
 (0)