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.
Replace with RPC request reference to new WalletContext struct similar to the
existing NodeContext struct and reference.
This PR is a followup to 25ad2c623af30056ffb36dcd203a52edda2b170f
https://github.com/bitcoin/bitcoin/pull/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 <joao.paulo.barbosa@gmail.com>
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
496+ for (const CRPCCommand& command : GetWalletRPCCommands()) {
497+ m_rpc_commands.emplace_back(command.category, command.name, [this, &command](const JSONRPCRequest& request, UniValue& result, bool last_handler) {
498+ util::Ref context{m_context};
499+ JSONRPCRequest wallet_request{context};
500+ wallet_request = request;
501+ return command.actor(wallet_request, result, last_handler);
operator= (which doesn’t copy m_context) maybe overload constructor so that here would be return command.actor({request, m_context}, result, last_handler). I’ve pushed 5e433dcb6fc3106281d14330879acf0ef1160b62 with this suggestion.
re: #19096 (review)
Instead of adding the
operator=(which doesn’t copy m_context) maybe overload constructor so that here would bereturn command.actor({request, m_context}, result, last_handler). I’ve pushed 5e433dc with this suggestion.
Thanks, that’s a better approach. Merged the two commits
122@@ -121,6 +123,14 @@ void EnsureWalletIsUnlocked(const CWallet* pwallet)
123 }
124 }
125
126+WalletContext& EnsureWalletContext(const util::Ref& context)
127+{
128+ if (!context.Has<WalletContext>()) {
assert(context.Has<WalletContext>()) for now?
re: #19096 (review)
Why not
assert(context.Has<WalletContext>())for now?
Not sure what the advantages of asserting would be. I do think not aborting would be a little better but mostly just want EnsureWalletContext to be consistent with EnsureNodeContext and other Ensure methods.
assert can not be used in rpc code. See
0test/lint/lint-assertions.sh:# Macro CHECK_NONFATAL(condition) should be used instead of assert for RPC code, where it
51+ , params(other.params)
52+ , fHelp(other.fHelp)
53+ , URI(other.URI)
54+ , authUser(other.authUser)
55+ , peerAddr(other.peerAddr)
56+ , context(context) {}
re: #19096 (review)
style-nit: I don’t think this is valid clang-formatted code hides
Ran clang-format
4340@@ -4329,9 +4341,5 @@ static const CRPCCommand commands[] =
4341 { "wallet", "walletprocesspsbt", &walletprocesspsbt, {"psbt","sign","sighashtype","bip32derivs"} },
4342 };
4343 // clang-format on
4344-
4345- for (unsigned int vcidx = 0; vcidx < ARRAYLEN(commands); vcidx++)
4346- handlers.emplace_back(chain.handleRpc(commands[vcidx]));
4347+ return MakeSpan(commands);
re: #19096 (review)
Would it be possible to do just this change in the first commit and all other changes in the second commit? If yes, that should simplify review. If no, I will take another look.
Moved to new commit
7+
8+namespace interfaces {
9+class Chain;
10+} // namespace interfaces
11+
12+//! WalletContext struct containing references to state shared between CWallet
WalletsContext ? We have one node but maybe multiple wallets connected to it.
re: #19096 (review)
If it has to contain references for multiple wallet why not call it
WalletsContext? We have one node but maybe multiple wallets connected to it.
I think using “wallets” instead of “wallet” is good where it can prevent ambiguity and misuse (https://github.com/bitcoin/bitcoin/pull/12221), but using plural where a name might apply to more than one object doesn’t always sound right (hairscut, sailsboat) and can create confusion on its on (-disablewallets, -walletsnotify) because it can imply some instead of all. In this case, I think wallets is more distracting and doesn’t sound right.
Code Review ACK 4a7253a, feel free to ignore comment it’s super nit.
Thanks to removing global in RPC, I think it would make it simpler to remove RPC methods from the Chain interface, and have the wallet listen on its own ports?
ACK 4a7253ab6c3bb323581cea54573529c2f823f035 🎋
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3ACK 4a7253ab6c3bb323581cea54573529c2f823f035 🎋
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUjJGQwAmXh+3GnuU8VGHIDajsI/sbNPuX1JJO4IAE5er+oHyDEebO2WCTwzymi7
8GswRu7Iol7P57sF5FtVTO1ozDSppq2qOrUEMU7p6CUrBq132GaA08uf9YbE0mx34
9B+dCmhW0qE320G/iZY/pevTiBnhKJagq/MHb31eRCGGDxpcskdJUUY5LPTZVuH33
10bwnMlVHjaO7Mm+JsrGNx4VpGUwh5LAR8XI1NqsDQu5Rbp18et5h1oc++AR9+qvhn
11Cf2lC6r9A3me5r9GO+766hZ52hgBVGXIG3UWV6xQ1tHA4j+Kdptwag3wbG+w49Zh
127VEk2Zt88SOxLI/qdg7andf2j5EmUFvBWbgZLYx83+/IvXOcZ/cyZFFB67tA2SVp
13vMCsbbFS3IDlB0Cfo09RkJwK3dn9EV0iGlr3noPUYr1JvGemVEYNgthF7t+hrCYk
14goyZSPcM30KJ4WJeG0TenWdK7eD5Y27Xj4yqBic/86zij2ixSUEP/jqPUSrVSkIG
15CsoSWkO0
16=tBE9
17-----END PGP SIGNATURE-----
Timestamp of file with hash 4ff2c313852eb5872f3d1eec1aa996276e07d1a8f4efe878195311834e130a3c -
re: MarcoFalke #19096 (comment)
@ryanofsky What is the status of this? I am currently undecided on whether to send this into rebase hell or merge.
Status is fine to merge as far as I know. It doesn’t seem to conflict with very much #19096 (comment):
re: ariard #19096#pullrequestreview-422249745
Thanks to removing global in RPC, I think it would make it simpler to remove RPC methods from the Chain interface, and have the wallet listen on its own ports?
Adding listening ports is a good idea and has been suggested before. It’s true that if it were implemented and backwards compatibility were dropped this PR would be slightly simpler. But I wouldn’t want to implement a new feature and break existing usage cases just to remove a global variable.