rpc: Removing g_rpc_node #17548

1. 
   MarcoFalke commented at 3:47 pm on November 21, 2019: member

   g_rpc_node is an alias for the current node context. It would be nice to pass in the node context into each rpc method somehow and remove the g_rpc_node global.

   Not sure how to do this, but here is a hint: #17407 (review)

   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.

2. MarcoFalke added the label Feature on Nov 21, 2019
3. MarcoFalke added the label Brainstorming on Nov 21, 2019
4. MarcoFalke added the label RPC/REST/ZMQ on Nov 21, 2019
5. 
   ryanofsky commented at 4:51 pm on November 21, 2019: member

   Adding a std::any or void* or similar context member to struct JSONRPCRequest is probably the simplest way to pass context to RPC methods and get rid of RPC globals without having to change the way RPC methods are written. But other ways would be possible like making RPC methods into objects: #16839 (review)

   But I wouldn’t put much too much priority on getting rid of g_rpc_node and g_rpc_chain globals. It’d be nice to add EnsureConnman EnsureBanman functions similar to EnsureMempool so the globals could be referenced in fewer places, but I don’t think much would be gained practically by completely getting rid of them.

6. 
   MarcoFalke commented at 6:17 pm on November 21, 2019: member

   Yeah, I am mostly leaving this open to not forget about it.

   But other ways would be possible like making RPC methods into objects

   I think I wanted to make them objects anyway for some RPCHelpMan work.

7. 
   laanwj commented at 8:53 pm on November 22, 2019: member

   Concept ACK.

   Putting the context in a thread-local variable would also work, but I prefer a context pointer on JSONRPCRequest to that kind of action at a distance.

   Not convinced that JSONRPC methods should be objects. They’re conceptually functions, defined as functions. Making them classes seems over-OOP to me.

8. 
   brakmic commented at 7:54 pm on April 16, 2020: contributor

   I have a question regarding g_txindex.

   After I have removed g_rpc_node and g_rpc_chain in my PR #18647 I went searching for similar globals, that maybe could be removed the same way. And I found one: g_txindex which too is being used in two of the RPCs: getrawtransaction and rest_tx. That is, they rely on GetTransaction which actually uses g_txindex.

   And as I have adapted RPCs to be called with NodeContext as their second argument, I would like to hear your opinions on expanding NodeContext by adding a unique_ptr TxIndex member to it.

   So that getrawtransaction would receive g_txindex from NodeContext and then forward it to GetTransaction.

   With this change we could also get rid of this third global variable (this of course depends on the fate of my PR).

   Regards,

9. 
   MarcoFalke commented at 11:29 pm on May 8, 2020: member

   There is a pull for this, and this issue is low prio, so closing.
10. MarcoFalke closed this on May 8, 2020

    ---

11. MarcoFalke referenced this in commit 25ad2c623a on May 21, 2020
12. sidhujag referenced this in commit a1f0893177 on May 21, 2020
13. DrahtBot locked this on Feb 15, 2022

Contributors
MarcoFalke ryanofsky laanwj brakmic

Labels
Feature Brainstorming RPC/REST/ZMQ