Remove g_rpc_node global #18740

ryanofsky · 2020-04-22T16:11:24Z

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 (#18647 (comment))

maflcko · 2020-04-22T16:26:59Z

Instead [...] void*, which isn't type safe, it uses a small new util::Ref

Why not NodeContext*? That might seem like a layer violation reading the source code, but practically it does the same and the diff would be smaller.

ryanofsky · 2020-04-22T16:49:37Z

Instead [...] void*, which isn't type safe, it uses a small new util::Ref

Why not NodeContext*? That might seem like a layer violation reading the source code, but practically it does the same and the diff would be smaller.

This would allow only allow dropping the first commit. The later commits would look basically the same but with s/util::Ref/NodeContext/.

The practical reason I don't want to do this is I want to the be able to use the RPC server outside the node process. E.g. start a multiprocess wallet process and have it listen for wallet RPC requests. Maybe add more offline wallet support and be able to have wallet-tool respond to RPCs like listtransactions. In these cases, wallet code can't link against src/node/context.cpp and NodeContext is useless for passing wallet state (aside from just looking strange).

Also, RPC code in rpc/server and httprpc is mostly pretty generic and not bitcoin specific and personally I think it would be a shame to see bitcoin types leak into it.

promag · 2020-04-22T17:05:21Z

From #18647 (comment)

Wallet RPC methods shouldn't have access to NodeContext. At some point we should have WalletContext struct that will hold things like vpwallets and let us get rid of wallet globals, and it would make sense for wallet RPC methods to have access to this. That is why suggestion from #17548 was to just add a new member to JSONRPCRequest that could hold a generic context, not something tied to the wallet or node. Other advantage of using JSONRPCRequest is that you dont' have to change RPC method signatures.

How would you implement this? Make context a map<string, any>? And do context["node"].Has<NodeContext>()?

practicalswift · 2020-04-22T17:30:37Z

Concept ACK

Very happy to see g_rpc_node go!

Thanks for the great architectural work you are doing.

ryanofsky · 2020-04-22T17:48:36Z

How would you implement this? Make context a map<string, any>? And do context["node"].Has<NodeContext>()?

No need for multiple contexts in the same request. interfaces::WalletClientImpl can just pass an different context to wallet RPC methods. Passing an interfaces::Chain reference would be sufficient enough for loadwallet / createwallet. But I actually want to introduce a WalletContext struct in src/wallet/context.h analagous to the NodeContext struct in src/node/context.h which would hold things like the Chain interface pointer and vpwallets vector.

ryanofsky · 2020-04-22T17:58:05Z

Actually @promag, it is a little clumsier than I thought. I should change this PR to pass context to rpc method register calls instead of the StartHTTPRPC call to make this more future-proof (should still be an improvement as is)

DrahtBot · 2020-04-22T19:37:48Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

test: Set -logthreadnames in unit tests #19028 (test: Set -logthreadnames in unit tests by MarcoFalke)
Reduce cs_main lock accumulation during GUI startup #19011 (Reduce cs_main lock accumulation during GUI startup by jonasschnelli)
Cache responses to GETADDR to prevent topology leaks #18991 (Cache responses to GETADDR to prevent topology leaks by naumenkogs)
Make g_chainman internal to validation #18698 (Make g_chainman internal to validation by MarcoFalke)
rpc: remove deprecated CRPCCommand constructor #18531 (rpc: Assert that RPCArg names are equal to CRPCCommand ones by MarcoFalke)
RPC: Internal named params #17356 (RPC: Internal named params by luke-jr)
Utils and libraries: Fix #13371 - move umask operation earlier in AppInit() #13389 (Utils and libraries: Fix -sysperms=false (default) doesn't appear to do anything #13371 - move umask operation earlier in AppInit() by n2yen)
RPC: Support addnode onetry without making the connection priviliged #12674 (RPC: Support addnode onetry without making the connection priviliged by luke-jr)

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

Concept ACK.

promag · 2020-04-23T00:45:41Z

src/rpc/blockchain.h

-extern NodeContext* g_rpc_node;
-
-CTxMemPool& EnsureMemPool();
+NodeContext& EnsureNodeContext(const util::Ref& context);


Have you considered EnsureNodeContext(const JSONRPCRequest& req) instead? Same for EnsureMemPool. Could avoid forward declaration above.

re: #18740 (comment)

Have you considered EnsureNodeContext(const JSONRPCRequest& req) instead? Same for EnsureMemPool. Could avoid forward declaration above.

Probably considered it, but I don't like this style (along lines of c++ koan https://www.youtube.com/watch?v=Zx_Tjp9WIII&t=15m07s)

Replace with RPC request reference to new WalletContext struct similar to the existing NodeContext struct and reference. This PR is a followup to 25ad2c6 bitcoin#18740 removing the g_rpc_node global. Some later PRs will follow this up and move more wallet globals to the WalletContext struct. Co-authored-by: João Barbosa <[email protected]>

4a7253a Remove g_rpc_chain global (Russell Yanofsky) e783197 refactor: replace RegisterWalletRPCCommands with GetWalletRPCCommands (Russell Yanofsky) Pull request description: Replace with RPC request reference to new WalletContext struct similar to the existing NodeContext struct and reference. This PR is a followup to #18740 removing the g_rpc_node global. Some later PRs will follow this up and move more wallet globals to the WalletContext struct. ACKs for top commit: MarcoFalke: ACK 4a7253a 🎋 ariard: Code Review ACK 4a7253a, feel free to ignore comment it's super nit. Tree-SHA512: 5bb5561c89f81811ca5232a58bf450e230d4218e62471c03227d142395fd36131672e99cb88329b33b9680a235db01e8b9d1c1e2a18288349e57205528deabab

4a7253a Remove g_rpc_chain global (Russell Yanofsky) e783197 refactor: replace RegisterWalletRPCCommands with GetWalletRPCCommands (Russell Yanofsky) Pull request description: Replace with RPC request reference to new WalletContext struct similar to the existing NodeContext struct and reference. This PR is a followup to bitcoin#18740 removing the g_rpc_node global. Some later PRs will follow this up and move more wallet globals to the WalletContext struct. ACKs for top commit: MarcoFalke: ACK 4a7253a 🎋 ariard: Code Review ACK 4a7253a, feel free to ignore comment it's super nit. Tree-SHA512: 5bb5561c89f81811ca5232a58bf450e230d4218e62471c03227d142395fd36131672e99cb88329b33b9680a235db01e8b9d1c1e2a18288349e57205528deabab

Summary: This commit does not change behavior Partial backport of core [[bitcoin/bitcoin#18740 | PR18740]]: bitcoin/bitcoin@691c817 Test Plan: ninja check Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D7989

Summary: ``` This commit does not change behavior ``` Partial backport of core [[bitcoin/bitcoin#18740 | PR18740]]: bitcoin/bitcoin@6fca33b#diff-2ae094f9329ce9498e1b4a9cb7767945b54a33b4ee9fed4f0ba401a98e1683c1 Depends on D7988 and D7989. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, jasonbcox, deadalnix Reviewed By: #bitcoin_abc, jasonbcox, deadalnix Subscribers: jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D7990

Summary: ``` This commit does not change behavior -BEGIN VERIFY SCRIPT- git grep -l g_rpc_node | xargs sed -i 's/g_rpc_node->/node./g' -END VERIFY SCRIPT- ``` Partial backport of core [[bitcoin/bitcoin#18740 | PR18740]]: bitcoin/bitcoin@ccb5059 Depends on D7990. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7991

Summary: ``` This commit does not change behavior ``` Concludes backport of core [[bitcoin/bitcoin#18740 | PR18740]]: bitcoin/bitcoin@b3f7f37 Depends on D7991. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7992

Replace with RPC request reference to new WalletContext struct similar to the existing NodeContext struct and reference. This PR is a followup to 25ad2c623af30056ffb36dcd203a52edda2b170f bitcoin/bitcoin#18740 removing the g_rpc_node global. Some later PRs will follow this up and move more wallet globals to the WalletContext struct. Co-authored-by: João Barbosa <[email protected]>

hebasto · 2021-03-28T00:34:34Z

@ryanofsky

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.

The std::any requires that a type of a contained object must be CopyConstructible.
But std::is_copy_constructible<NodeContext>::value == false.

So, even with C++17, there are no simple ways to drop util::Ref in favor of std::any, unfortunately.

Or did I miss something?

fanquake · 2021-03-28T00:36:51Z

So, even with C++17, there are no simple ways to drop util::Ref in favor of std::any, unfortunately.

It's being dropped for std::any in #21366.

…h_by_height fcb7261 Prevent valgrind false positive in rest_blockhash_by_height (Russell Yanofsky) Pull request description: A bad interaction between valgrind and clang 6.0.0-1ubuntu2 with -O2 optimizations makes valgrind misleadingly imply C++ code is reading an uninitialized blockheight value in `rest_blockhash_by_height` just because that's what clang optimized code is doing. The C++ code looks like: ```c++ int32_t blockheight; if (!ParseInt32(height_str, &blockheight) || blockheight < 0) { ``` while the optimized code looks like: ``` 0x00000000000f97ab <+123>: callq 0x4f8860 <ParseInt32(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, int*)> 0x00000000000f97b0 <+128>: mov 0xc(%rsp),%ebx 0x00000000000f97b4 <+132>: test %ebx,%ebx 0x00000000000f97b6 <+134>: js 0xf98aa <rest_blockhash_by_height(util::Ref const&, HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)+378> 0x00000000000f97bc <+140>: xor $0x1,%al 0x00000000000f97be <+142>: jne 0xf98aa <rest_blockhash_by_height(util::Ref const&, HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)+378> ``` During the rest_interface.py test: https://github.com/bitcoin/bitcoin/blob/eef90c14ed0f559e3f6e187341009270b84f45cb/test/functional/interface_rest.py#L266 when `height_str` is empty, `ParseInt32` returns false and `blockheight` value is never assigned. The optimized code reads the uninitialized `blockheight` value in `0xc(%rsp)` before the checking the `ParseInt32` return value in `%al`, which is harmless, but triggers the following error from valgrind: ``` ==30660== Thread 13 b-httpworker.2: ==30660== Conditional jump or move depends on uninitialised value(s) ==30660== at 0x2017B6: rest_blockhash_by_height(util::Ref const&, HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (rest.cpp:614) ==30660== by 0x2041B9: operator() (rest.cpp:670) ==30660== by 0x2041B9: std::_Function_handler<bool (HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&), StartREST(util::Ref const&)::$_1>::_M_invoke(std::_Any_data const&, HTTPRequest*&&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (std_function.h:301) ==30660== by 0x3EC994: operator() (std_function.h:706) ==30660== by 0x3EC994: HTTPWorkItem::operator()() (httpserver.cpp:55) ==30660== by 0x3ED16D: WorkQueue<HTTPClosure>::Run() (httpserver.cpp:114) ==30660== by 0x3E9168: HTTPWorkQueueRun(WorkQueue<HTTPClosure>*, int) (httpserver.cpp:342) ==30660== by 0x3EDAAA: __invoke_impl<void, void (*)(WorkQueue<HTTPClosure> *, int), WorkQueue<HTTPClosure> *, int> (invoke.h:60) ==30660== by 0x3EDAAA: __invoke<void (*)(WorkQueue<HTTPClosure> *, int), WorkQueue<HTTPClosure> *, int> (invoke.h:95) ==30660== by 0x3EDAAA: _M_invoke<0, 1, 2> (thread:234) ==30660== by 0x3EDAAA: operator() (thread:243) ==30660== by 0x3EDAAA: std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (*)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int> > >::_M_run() (thread:186) ==30660== by 0x64256DE: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25) ==30660== by 0x54876DA: start_thread (pthread_create.c:463) ==30660== by 0x6DC888E: clone (clone.S:95) ==30660== Uninitialised value was created by a stack allocation ==30660== at 0x20173A: rest_blockhash_by_height(util::Ref const&, HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (rest.cpp:608) ==30660== { <insert_a_suppression_name_here> Memcheck:Cond fun:_ZL24rest_blockhash_by_heightRKN4util3RefEP11HTTPRequestRKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE fun:operator() fun:_ZNSt17_Function_handlerIFbP11HTTPRequestRKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEEZ9StartRESTRKN4util3RefEE3$_1E9_M_invokeERKSt9_Any_dataOS1_S9_ fun:operator() fun:_ZN12HTTPWorkItemclEv fun:_ZN9WorkQueueI11HTTPClosureE3RunEv fun:_ZL16HTTPWorkQueueRunP9WorkQueueI11HTTPClosureEi fun:__invoke_impl<void, void (*)(WorkQueue<HTTPClosure> *, int), WorkQueue<HTTPClosure> *, int> fun:__invoke<void (*)(WorkQueue<HTTPClosure> *, int), WorkQueue<HTTPClosure> *, int> fun:_M_invoke<0, 1, 2> fun:operator() fun:_ZNSt6thread11_State_implINS_8_InvokerISt5tupleIJPFvP9WorkQueueI11HTTPClosureEiES6_iEEEEE6_M_runEv obj:/usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25 fun:start_thread fun:clone } ``` This is a known bad interaction between clang and valgrind. The clang optimized code is correct but valgrind has no way of knowing that accessing the uninitialized value isn't a problem. Issue has been reported previously: - https://bugs.llvm.org/show_bug.cgi?id=32604#c4 - Z3Prover/z3#972 This commit just sets blockheight to -1 as a workaround. This change was originally made in 41d5d65 from bitcoin#18740 to fix the travis error there (https://travis-ci.org/github/bitcoin/bitcoin/jobs/678453061#L7157) but MarcoFalke suggested bitcoin#18740 (comment) moving to a new PR, since apparently the error's been seen on travis previously ACKs for top commit: MarcoFalke: ACK fcb7261 practicalswift: ACK fcb7261 Tree-SHA512: ec8abf45bd3d6c6e0e7e404d0b2a749efd43910619b84b0b5fe7dab22881598d1011a0f3ff2e146bf46320b63eb152bf63c62c06f1ab84c35dd640abc468f18f

backport: bitcoin#18038, bitcoin#14193, bitcoin#17564, bitcoin#17999, bitcoin#18740, bitcoin#19096, bitcoin#16426, bitcoin#17737, bitcoin#18698: deglobalization backports (part 2)

[bitcoin#14624](bitcoin/bitcoin#14624) bls refactoring to resolve clang warnings [bitcoin#16426](bitcoin/bitcoin#16426) - cs_wallet lock order refactoring and reduce cs_main locking atomic smartnode_connection (allow read/write by different threads simultaneously) cs_mnauth locks on verifiedProRegTxHash read RecursiveMutex at locking access to activeSmartNode [bitcoin#14307](bitcoin/bitcoin#14307) - Consolidate redundant implementation of ParseHashStr [bitcoin#13399](bitcoin/bitcoin#13399) - rpc: Add submitheader built-in miner deleted [bitcoin#17781](bitcoin/bitcoin#17781) - Remove mempool global from miner [bitcoin#16623](bitcoin/bitcoin#16624) - encapsulate transactions state [bitcoin#15931](bitcoin/bitcoin#15931) - Remove GetDepthInMainChain dependency on locked chain interface Pass CConnman to function against global pointer [bitcoin#16839](bitcoin/bitcoin#16839) - Replace Connman and BanMan globals with NodeContext local [bitcoin#16034](bitcoin/bitcoin#16034) - refactoring: Rename LockAnnotation to LockAssertion and add run-time check to it [bitcoin#17564](bitcoin/bitcoin#17564) - Use mempool from node context instead of global [bitcoin#18740](bitcoin/bitcoin#18740) - Remove g_rpc_node global [bitcoin#19096](bitcoin/bitcoin#19096) - Remove g_rpc_chain global [bitcoin#18338](bitcoin/bitcoin#18338) - Fix wallet unload race condition other fixes and tweaks

ryanofsky mentioned this pull request Apr 22, 2020

rpc: remove g_rpc_node & g_rpc_chain #18647

Closed

DrahtBot added Build system Mining RPC/REST/ZMQ Tests Utils/log/libs Wallet labels Apr 22, 2020

maflcko added Refactoring and removed Build system Mining Tests Utils/log/libs Wallet labels Apr 22, 2020

ryanofsky force-pushed the pr/frpc branch from 0708140 to 0d84b1a Compare April 22, 2020 16:26

ryanofsky force-pushed the pr/frpc branch from 0d84b1a to fe33374 Compare April 22, 2020 19:31

This was referenced Apr 22, 2020

wallet: Avoid translating RPC errors #18699

Merged

Make g_chainman internal to validation #18698

Merged

wallet: Add BlockUntilSyncedToCurrentChain to dumpwallet #18671

Merged

RPC: Internal named params #17356

Closed

promag reviewed Apr 23, 2020

View reviewed changes

This was referenced Apr 23, 2020

gui: Bilingual GUI error messages #16224

Merged

RPC: Support addnode onetry without making the connection priviliged #12674

Closed

Multiprocess bitcoin #10102

Draft

kwvg mentioned this pull request Oct 7, 2021

merge bitcoin#15931, #16839, #17192, #17407, #18037, #17997, partial #15639, #17989: deglobalisation and mining rpc backports dashpay/dash#4357

Closed

kwvg added a commit to kwvg/dash that referenced this pull request May 18, 2022

merge bitcoin#18740: Remove g_rpc_node global

1a65972

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Remove g_rpc_node global #18740

Remove g_rpc_node global #18740

Uh oh!

Conversation

ryanofsky commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Apr 22, 2020

Uh oh!

ryanofsky commented Apr 22, 2020

Uh oh!

promag commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Apr 22, 2020

Uh oh!

ryanofsky commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Apr 22, 2020

Uh oh!

DrahtBot commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

ryanofsky Apr 27, 2020

Choose a reason for hiding this comment

Uh oh!

hebasto commented Mar 28, 2021

Uh oh!

fanquake commented Mar 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

ryanofsky commented Apr 22, 2020 •

edited

Loading

promag commented Apr 22, 2020 •

edited

Loading

ryanofsky commented Apr 22, 2020 •

edited

Loading

DrahtBot commented Apr 22, 2020 •

edited

Loading