Concept ACK on fixing #17548

Thanks for tackling this issue!

Few thoughts:

• This changes rpc method signature every every method from UniValue(const JSONRPCRequest&) by adding NodeContext parameter, but Marco's rpc: remove deprecated CRPCCommand constructor #18531 is already changing method signatures a different way. You should work with Marco to come up with signature for rpc methods that doesn't conflict
• This PR doesn't actually get rid of the g_rpc_node variable, just a handful of uses. Maybe that is temporary (you could mark this PR a draft), but otherwise title is kind of misleading.
• NodeContext& rpc_node arguments should be renamed NodeContext& node, RPC prefix is misleading and not doing anything
• NodeContext* pnode_ctx followed by CHECK_NONFATAL(pnode_ctx) should probably be NodeContext& node and not using a pointer. Also current coding convention doesn't prefix pointers variable with p.
• 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 rpc: Removing g_rpc_node #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.
• This appears to be adding NodeContext& parameters to many RPC methods that don't use it. Again this may be another reason to just make it accessible through JSONRPCRequest

#18531 Doesn't change how RPC methods are dispatched, but it wraps them into an RPCMan. Assuming my pull makes it in first, it is going to reduce the diff of this pull significantly. Though, when JSONRPCRequest is used to pass in context, the two pulls should not conflict at all.

Hi @ryanofsky, and many thanks!

Few thoughts:

• This changes rpc method signature every every method from UniValue(const JSONRPCRequest&) by adding NodeContext parameter, but Marco's rpc: remove deprecated CRPCCommand constructor #18531 is already changing method signatures a different way. You should work with Marco to come up with signature for rpc methods that doesn't conflict

Didn't know about that. Could not find anything in the original issue. Anyway, thanks for pointing this out!

• This PR doesn't actually get rid of the g_rpc_node variable, just a handful of uses. Maybe that is temporary (you could mark this PR a draft), but otherwise title is kind of misleading.

I never wanted to get rid of it, only make it:

• const ref
• forwarded to actors directly and in one place (and not scattered around as it currently is)
• while keeping JSONRPCRequest structure as it is. Why should JSONRPCRequest know anything about NodeContext? I see no reason to let a passive object (a request) that's going back and forth knowing anything about its environment. But maybe there are other things I know nothing about that would mandate such behavior.

And yes, there are still a few places where we still need g_rpc_node, but those aren't directly related to this PR, so I deliberately didn't change them.

--EDIT: My sentence "never wanted to get rid of it" might have been misleading. Actually, I want to get rid of it as a global, but wanted to keep the NodeContext itself available to all RPC methods by forwarding it to Actors, of course.

--EDIT 2: Now the g_rpc_node is completely gone.

• NodeContext& rpc_node arguments should be renamed NodeContext& node, RPC prefix is misleading and not doing anything

Ok.

--EDIT: Was changed and force-pushed.

• NodeContext* pnode_ctx followed by CHECK_NONFATAL(pnode_ctx) should probably be NodeContext& node and not using a pointer. Also current coding convention doesn't prefix pointers variable with p.

Ok. Will check it again.

--EDIT: Have checked it. Sadly, not possible because copy assignment operator of NodeContext is implicitly deleted. So, will keep it a pointer there, and the check, unless someone comes with a better idea. Any inputs that help me get rid of raw pointers are highly welcome :)

• Wallet RPC methods shouldn't have access to NodeContext.

I didn't know about his. I thought it's the right place to put the context in, because those methods are being executed within a NodeContext. It's like "self" variable in python, for example. It's simply there, be it used or not, because at no point in time is an RPC method being used outside NodeContext. But I might be mistaken.

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.

Again, why should a passive object, like a request, know anything about the environment that's using it? Why not create some kind of MetaContext that holds both Node- and WalletContext and forwards it directly to actors? This way one could easily extend contexts, if there might be a need to do so in future (I'm only speculating here)

--EDIT: Another option would be to create a MetaContext that's based on aliasing shared_ptr. Something like this:

struct MetaContext {
    NodeContext node;
    WalletContext wallet;
};

We then initialize the meta-context first:

auto meta_ctx = std::shared_ptr<MetaContext>(new MetaContext());

Now, when we want to forward the "correct" context to a particular actor, we would do this:

auto subctx_node = std::shared_ptr<NodeContext>(meta_ctx, &meta_ctx->node);  

For functions that should only deal with Wallet, we then go this way:

auto subctx_wallet = std::shared_ptr<WalletContext>(meta_ctx, &meta_ctx->wallet);  

With this strategy we could put any number of different contexts in the same place, but only reveal those that are of use to functions which will consume them.

• This appears to be adding NodeContext& parameters to many RPC methods that don't use it. Again this may be another reason to just make it accessible through JSONRPCRequest

I see it like the "self" or "this" pointers. They're always there, regardless of usage. "Some" context is always there.

Anyway, thanks again!

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• wallet: Avoid translating RPC errors #18699 (wallet: Avoid translating RPC errors by MarcoFalke)
• Make g_chainman internal to validation #18698 (Make g_chainman internal to validation by MarcoFalke)
• wallet: Add BlockUntilSyncedToCurrentChain to dumpwallet #18671 (wallet: Add BlockUntilSyncedToCurrentChain to dumpwallet by MarcoFalke)
• rpc: separate bumpfee's psbt creation function into psbtbumpfee #18654 (rpc: separate bumpfee's psbt creation function into psbtbumpfee by achow101)
• test: checks that bitcoin-cli autocomplete is in sync #18606 (test: checks that bitcoin-cli autocomplete is in sync by pierreN)
• rpc, cli: add multiwallet balances rpc and use it in -getinfo #18453 (rpc, cli: add multiwallet balances rpc and use it in -getinfo by jonatack)
• RPC: Internal named params #17356 (RPC: Internal named params by luke-jr)
• Native Descriptor Wallets using DescriptorScriptPubKeyMan #16528 (Native Descriptor Wallets using DescriptorScriptPubKeyMan by achow101)
• Log RPC parameters (arguments) if -debug=rpcparams #16365 (Log RPC parameters (arguments) if -debug=rpcparams by LarryRuane)
• gui: Bilingual GUI error messages #16224 (gui: Bilingual GUI error messages by hebasto)
• [RPC] Include coinbase transactions in receivedby RPCs #14707 ([RPC] Include coinbase transactions in receivedby RPCs by andrewtoth)
• RPC: Support addnode onetry without making the connection priviliged #12674 (RPC: Support addnode onetry without making the connection priviliged by luke-jr)
• [wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option #11413 ([wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option by kallewoof)

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.

I never wanted to get rid of it, only make it:

• const ref

I don't think the the const part is good. RPC methods can start/stop the node, initiate connections, load wallets, so I don't think they should be prevented from changing all context members that exist now and all new context members we may add in the future.

• forwarded to actors directly and in one place (and not scattered around as it currently is)

This seems good. Maybe list it as a benefit in PR description. If you think of examples where it could help practically, those are good to mention too.

• while keeping JSONRPCRequest structure as it is. Why should JSONRPCRequest know anything about NodeContext? I see no reason to let a passive object (a request) that's going back and forth knowing anything about its environment.

Maybe my suggestion here and in #17548 wasn't clear, but I'm not saying add a NodeContext member to JSONRPCRequest. I'm saying add a generic context member so it can be a vehicle for information about the environment without knowing about the environment, the same way it is vehicle for params without knowing about the meaning of params

I never wanted to get rid of it, only make it:

• const ref

I don't think the the const part is good. RPC methods can start/stop the node, initiate connections, load wallets, so I don't think they should be prevented from changing all context members that exist now and all new context members we may add in the future.

Well, then I think we should have different variants of contexts at our disposal. Maybe something similar to the example with aliasing shared_ptrs (in the EDIT-ed part of my previous posting). Read-only contexts, writeable ones etc.

• forwarded to actors directly and in one place (and not scattered around as it currently is)

This seems good. Maybe list it as a benefit in PR description. If you think of examples where it could help practically, those are good to mention too.

Thanks for pointing this out. I will update the PR description.

• while keeping JSONRPCRequest structure as it is. Why should JSONRPCRequest know anything about NodeContext? I see no reason to let a passive object (a request) that's going back and forth knowing anything about its environment.

Maybe my suggestion here and in #17548 wasn't clear, but I'm not saying add a NodeContext member to JSONRPCRequest. I'm saying add a generic context member so it can be a vehicle for information about the environment without knowing about the environment, the same way it is vehicle for params without knowing about the meaning of params

I would not extend JSONRPCRequest in any way. It should be a message carrier in the purest KISS sense.

Well, then I think we should need different variant of contexts.

It would be good to elaborate on the problem being solved in the PR description. It seems to have something to do with data data hiding, but specifics are unclear.

I think the goal of #17548 is remove the global variable, though, so I wouldn't say this fixes that issue, even though it could help

Well, then I think we should need different variant of contexts.

It would be good to elaborate on the problem being solved in the PR description. It seems to have something to do with data data hiding, but specifics are unclear.

I think the goal of #17548 is remove the global variable, though, so I wouldn't say this fixes that issue, even though it could help

Yes, of course. I am very glad that my PR found much attention within such a short time frame. The learning process I went through while thinking about "this damn global NodeContext and how to make it less exposed" is already a win for me.

We will surely find a way.

Instead of changing all RPC handlers I think you could override CRPCCommand constructor to take
a UniValue (*rpcfn_type)(const JSONRPCRequest& jsonRequest). The same approach can be done in Marco PR, RPCMan can take different method signatures.

Yes, this approach would be less invasive. Maybe I could rebase after Marco's PR gets merged?

I really appreciate the effort here, but I don't think this PR is teneble, and I have a simpler alternative in #18740. This PR is too wide in scope changing things that don't need to be changed, and is accessing node context in places that will make changes we do want more difficult. Specifically this PR is:

• Making spurious changes to httpserver which aren't necessary because it already supports context passing. These changes also have potential to cause bugs given fragility of libevent.
• Using node type in httprpc rpc/server layers that don't otherwise contain node specific code (or even bitcoin specific code). These changes will make multiprocess and offline wallet work more difficult, and get in the way of any features that want to accept RPC requests outside of the node process.
• Adding NodeContext parameter to every single RPC method when most of these methods don't need it and some of them (wallet methods) should not have access to it. This makes the change bigger and harder to review, and will potentially cause confusion and accidental changes that will again make multiprocess and offline wallet work more difficult.
• Making unit test initialization inconsistent with application initialization. Passing context through tableRPC.addNodeContext in one case and AppInitServers in the other case.
• Not safely or consistently handling errors. Removing null checks in rest and rpc code and segfaulting when the context isn't available instead of returning HTTP errors
• Making spurious changes that complicate review, adding consts, removing consts, changing indentation, making manual changes that would be safer as scripted-diffs
• Not using recommended style, using pointers instead of references, using hungarian notation, using #include where a forward declaration would be more efficient
• Making NodeContext a more ubiquitous presence in the codebase than it was ever meant to be (increasing from 66 to 266 usages).

It was good to see this PR opened and have progress on #17548, and be useful as an experiment, but I think this is not the best way forward and would recommend not doing more work on this PR

Hi @ryanofsky

I really appreciate the effort here, but I don't think this PR is teneble, and I have a simpler alternative in #18740.

Simpler is very often better!

This PR is too wide in scope changing things that don't need to be changed, and is accessing > node context in places that will make changes we do want more difficult.

This indeed is the case, but mostly because g_rpc_node, and later g_rpc_chain, were basically "everywhere". This of course should not serve as an excuse to write an even bigger "solution" for the original problem.

Specifically this PR is:

• Making spurious changes to httpserver which aren't necessary because it already supports context passing. These changes also have potential to cause bugs given fragility of libevent.

As I don't know much about libevent, I will simply agree on that.

• Using node type in httprpc rpc/server layers that don't otherwise contain node specific code (or even bitcoin specific code). These changes will make multiprocess and offline wallet work more difficult, and get in the way of any features that want to accept RPC requests outside of the node process.

The NodeContext structure is actually even bigger. It contains various things, so you are right. But also: maybe NodeContext itself should be split into smaller parts? To me, it looks like NodeContext carries many heavy things. I actually never needed NodeContext as a whole but merely a pointer/ref to one of its members. However, this is not important here, imo, so I'll leave it for another discussion.

• Adding NodeContext parameter to every single RPC method when most of these methods don't need it and some of them (wallet methods) should not have access to it.

Yes, it's not the cutest thing to do, but a bit less ugly than a global pointer. And as I said, I never needed NodeContext itself, only a pointer/ref from it.

This makes the change bigger and harder to review,

To understand why g_rpc_node is everywhere and how to deal with it correctly, was hard to me. :)

Later then, g_rpc_chain came to the party.

and will potentially cause confusion and
accidental changes that will again make multiprocess and offline wallet work more difficult.

As I still lack enough knowledge about these areas, I can only agree with you.

• Making unit test initialization inconsistent with application initialization. Passing context through tableRPC.addNodeContext in one case and AppInitServers in the other case.

Ok.

• Not safely or consistently handling errors. Removing null checks in rest and rpc code and segfaulting when the context isn't available instead of returning HTTP errors

I was actually checking for nullptr at the very beginning of the initialization. But, this is not important here, so the code as it is, is not stable enough. Accepted.

• Making spurious changes that complicate review, adding consts, removing consts, changing indentation, making manual changes that would be safer as scripted-diffs

I use clang format as stated in Bitcoin docs.

git diff -U0 HEAD~1.. | ./contrib/devtools/clang-format-diff.py -p1 -i -v

Regarding const/non-costs: it was because the NodeContext has an implicitly deleted copy assignment constructor, because one of its members is a unique_ptr. So, I had to find a way around this impasse. This too is one of the things that make me believe, that NodeContext itself should be split into smaller parts. But, this is not that important here, I think.

• Not using recommended style, using pointers instead of references, using hungarian notation, using #include where a forward declaration would be more efficient

I would like to know more about the bitcoinic coding style. If you have any doc/link please, let me know. I could not find anything.

• Making NodeContext a more ubiquitous presence in the codebase than it was ever meant to be (increasing from 66 to 266 usages).

NodeContext should maybe be made even smaller, imo. So, yes, I agree with you.

It was good to see this PR opened and have progress on #17548, and be useful as an experiment,

Yes, that was the initial motivation. The original issue #17548 was hanging around for some time, so I thought, let's do something about it.

but I think this is not the best way forward and would recommend not doing more work on this PR

In my opinion, it was the best way to forward for me, as it helped me a lot to understand some interesting parts of Bitcoin. And I thank you for that! However, you are right, it's not the best way to forward for the project, so I will close my PR, and will study your code and try it out.

Thanks again!