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>
Concept ACK
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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);
Instead of adding the 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>()) {
Why not 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.
Is it possible to hit this exception?
assert can not be used in rpc code. See
test/lint/lint-assertions.sh:# Macro CHECK_NONFATAL(condition) should be used instead of assert for RPC code, where it
Approach ACK, just two comments before ACK code change.
ACK e1cfcd4cf82bd7c62e95a4e80bead3cac08afdf0.
51 | + , params(other.params) 52 | + , fHelp(other.fHelp) 53 | + , URI(other.URI) 54 | + , authUser(other.authUser) 55 | + , peerAddr(other.peerAddr) 56 | + , context(context) {}
style-nit: I don't think this is valid clang-formatted code hides
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);
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.
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
Thanks for splitting up. This made the review easier for me.
Concept ACK e1cfcd4cf82bd7c62e95a4e80bead3cac08afdf0
7 | + 8 | +namespace interfaces { 9 | +class Chain; 10 | +} // namespace interfaces 11 | + 12 | +//! WalletContext struct containing references to state shared between CWallet
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.
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 🎋
<details><summary>Show signature and timestamp</summary>
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
ACK 4a7253ab6c3bb323581cea54573529c2f823f035 🎋
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjJGQwAmXh+3GnuU8VGHIDajsI/sbNPuX1JJO4IAE5er+oHyDEebO2WCTwzymi7
GswRu7Iol7P57sF5FtVTO1ozDSppq2qOrUEMU7p6CUrBq132GaA08uf9YbE0mx34
B+dCmhW0qE320G/iZY/pevTiBnhKJagq/MHb31eRCGGDxpcskdJUUY5LPTZVuH33
bwnMlVHjaO7Mm+JsrGNx4VpGUwh5LAR8XI1NqsDQu5Rbp18et5h1oc++AR9+qvhn
Cf2lC6r9A3me5r9GO+766hZ52hgBVGXIG3UWV6xQ1tHA4j+Kdptwag3wbG+w49Zh
7VEk2Zt88SOxLI/qdg7andf2j5EmUFvBWbgZLYx83+/IvXOcZ/cyZFFB67tA2SVp
vMCsbbFS3IDlB0Cfo09RkJwK3dn9EV0iGlr3noPUYr1JvGemVEYNgthF7t+hrCYk
goyZSPcM30KJ4WJeG0TenWdK7eD5Y27Xj4yqBic/86zij2ixSUEP/jqPUSrVSkIG
CsoSWkO0
=tBE9
-----END PGP SIGNATURE-----
Timestamp of file with hash 4ff2c313852eb5872f3d1eec1aa996276e07d1a8f4efe878195311834e130a3c -
</details>
@ryanofsky What is the status of this? I am currently undecided on whether to send this into rebase hell or merge.
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.