Merge #19096: Remove g_rpc_chain global

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

Author: MarcoFalke

src/Makefile.am

@@ -241,6 +241,7 @@ BITCOIN_CORE_H = \
   versionbitsinfo.h \
   walletinitinterface.h \
   wallet/coincontrol.h \
+  wallet/context.h \
   wallet/crypter.h \
   wallet/db.h \
   wallet/feebumper.h \
@@ -350,6 +351,7 @@ libbitcoin_wallet_a_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
 libbitcoin_wallet_a_SOURCES = \
   interfaces/wallet.cpp \
   wallet/coincontrol.cpp \
+  wallet/context.cpp \
   wallet/crypter.cpp \
   wallet/db.cpp \
   wallet/feebumper.cpp \

src/interfaces/wallet.cpp

@@ -9,13 +9,16 @@
 #include <interfaces/handler.h>
 #include <policy/fees.h>
 #include <primitives/transaction.h>
+#include <rpc/server.h>
 #include <script/standard.h>
 #include <support/allocators/secure.h>
 #include <sync.h>
 #include <ui_interface.h>
 #include <uint256.h>
 #include <util/check.h>
+#include <util/ref.h>
 #include <util/system.h>
+#include <wallet/context.h>
 #include <wallet/feebumper.h>
 #include <wallet/fees.h>
 #include <wallet/ismine.h>
@@ -481,16 +484,21 @@ class WalletClientImpl : public ChainClient
 {
 public:
     WalletClientImpl(Chain& chain, std::vector<std::string> wallet_filenames)
-        : m_chain(chain), m_wallet_filenames(std::move(wallet_filenames))
+        : m_wallet_filenames(std::move(wallet_filenames))
     {
+        m_context.chain = &chain;
     }
     void registerRpcs() override
     {
-        g_rpc_chain = &m_chain;
-        return RegisterWalletRPCCommands(m_chain, m_rpc_handlers);
+        for (const CRPCCommand& command : GetWalletRPCCommands()) {
+            m_rpc_commands.emplace_back(command.category, command.name, [this, &command](const JSONRPCRequest& request, UniValue& result, bool last_handler) {
+                return command.actor({request, m_context}, result, last_handler);
+            }, command.argNames, command.unique_id);
+            m_rpc_handlers.emplace_back(m_context.chain->handleRpc(m_rpc_commands.back()));
+        }
     }
-    bool verify() override { return VerifyWallets(m_chain, m_wallet_filenames); }
-    bool load() override { return LoadWallets(m_chain, m_wallet_filenames); }
+    bool verify() override { return VerifyWallets(*m_context.chain, m_wallet_filenames); }
+    bool load() override { return LoadWallets(*m_context.chain, m_wallet_filenames); }
     void start(CScheduler& scheduler) override { return StartWallets(scheduler); }
     void flush() override { return FlushWallets(); }
     void stop() override { return StopWallets(); }
@@ -505,9 +513,10 @@ class WalletClientImpl : public ChainClient
     }
     ~WalletClientImpl() override { UnloadWallets(); }
 
-    Chain& m_chain;
+    WalletContext m_context;
     std::vector<std::string> m_wallet_filenames;
     std::vector<std::unique_ptr<Handler>> m_rpc_handlers;
+    std::list<CRPCCommand> m_rpc_commands;
 };
 
 } // namespace

src/rpc/request.h

@@ -41,6 +41,16 @@ class JSONRPCRequest
     const util::Ref& context;
 
     JSONRPCRequest(const util::Ref& context) : id(NullUniValue), params(NullUniValue), fHelp(false), context(context) {}
+
+    //! Initializes request information from another request object and the
+    //! given context. The implementation should be updated if any members are
+    //! added or removed above.
+    JSONRPCRequest(const JSONRPCRequest& other, const util::Ref& context)
+        : id(other.id), strMethod(other.strMethod), params(other.params), fHelp(other.fHelp), URI(other.URI),
+          authUser(other.authUser), peerAddr(other.peerAddr), context(context)
+    {
+    }
+
     void parse(const UniValue& valRequest);
 };
 

src/wallet/context.cpp

@@ -0,0 +1,8 @@
+// Copyright (c) 2020 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <wallet/context.h>
+
+WalletContext::WalletContext() {}
+WalletContext::~WalletContext() {}

src/wallet/context.h

@@ -0,0 +1,32 @@
+// Copyright (c) 2020 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef BITCOIN_WALLET_CONTEXT_H
+#define BITCOIN_WALLET_CONTEXT_H
+
+namespace interfaces {
+class Chain;
+} // namespace interfaces
+
+//! WalletContext struct containing references to state shared between CWallet
+//! instances, like the reference to the chain interface, and the list of opened
+//! wallets.
+//!
+//! Future shared state can be added here as an alternative to adding global
+//! variables.
+//!
+//! The struct isn't intended to have any member functions. It should just be a
+//! collection of state pointers that doesn't pull in dependencies or implement
+//! behavior.
+struct WalletContext {
+    interfaces::Chain* chain{nullptr};
+
+    //! Declare default constructor and destructor that are not inline, so code
+    //! instantiating the WalletContext struct doesn't need to #include class
+    //! definitions for smart pointer and container members.
+    WalletContext();
+    ~WalletContext();
+};
+
+#endif // BITCOIN_WALLET_CONTEXT_H

src/wallet/rpcwallet.cpp

@@ -21,12 +21,14 @@
 #include <util/fees.h>
 #include <util/message.h> // For MessageSign()
 #include <util/moneystr.h>
+#include <util/ref.h>
 #include <util/string.h>
 #include <util/system.h>
 #include <util/translation.h>
 #include <util/url.h>
 #include <util/vector.h>
 #include <wallet/coincontrol.h>
+#include <wallet/context.h>
 #include <wallet/feebumper.h>
 #include <wallet/rpcwallet.h>
 #include <wallet/wallet.h>
@@ -121,6 +123,14 @@ void EnsureWalletIsUnlocked(const CWallet* pwallet)
     }
 }
 
+WalletContext& EnsureWalletContext(const util::Ref& context)
+{
+    if (!context.Has<WalletContext>()) {
+        throw JSONRPCError(RPC_INTERNAL_ERROR, "Wallet context not found");
+    }
+    return context.Get<WalletContext>();
+}
+
 // also_create should only be set to true only when the RPC is expected to add things to a blank wallet and make it no longer blank
 LegacyScriptPubKeyMan& EnsureLegacyScriptPubKeyMan(CWallet& wallet, bool also_create)
 {
@@ -2584,6 +2594,7 @@ static UniValue loadwallet(const JSONRPCRequest& request)
                 },
             }.Check(request);
 
+    WalletContext& context = EnsureWalletContext(request.context);
     WalletLocation location(request.params[0].get_str());
 
     if (!location.Exists()) {
@@ -2598,7 +2609,7 @@ static UniValue loadwallet(const JSONRPCRequest& request)
 
     bilingual_str error;
     std::vector<bilingual_str> warnings;
-    std::shared_ptr<CWallet> const wallet = LoadWallet(*g_rpc_chain, location, error, warnings);
+    std::shared_ptr<CWallet> const wallet = LoadWallet(*context.chain, location, error, warnings);
     if (!wallet) throw JSONRPCError(RPC_WALLET_ERROR, error.original);
 
     UniValue obj(UniValue::VOBJ);
@@ -2702,6 +2713,7 @@ static UniValue createwallet(const JSONRPCRequest& request)
         },
     }.Check(request);
 
+    WalletContext& context = EnsureWalletContext(request.context);
     uint64_t flags = 0;
     if (!request.params[1].isNull() && request.params[1].get_bool()) {
         flags |= WALLET_FLAG_DISABLE_PRIVATE_KEYS;
@@ -2731,7 +2743,7 @@ static UniValue createwallet(const JSONRPCRequest& request)
 
     bilingual_str error;
     std::shared_ptr<CWallet> wallet;
-    WalletCreationStatus status = CreateWallet(*g_rpc_chain, passphrase, flags, request.params[0].get_str(), error, warnings, wallet);
+    WalletCreationStatus status = CreateWallet(*context.chain, passphrase, flags, request.params[0].get_str(), error, warnings, wallet);
     switch (status) {
         case WalletCreationStatus::CREATION_FAILED:
             throw JSONRPCError(RPC_WALLET_ERROR, error.original);
@@ -4263,7 +4275,7 @@ UniValue removeprunedfunds(const JSONRPCRequest& request);
 UniValue importmulti(const JSONRPCRequest& request);
 UniValue importdescriptors(const JSONRPCRequest& request);
 
-void RegisterWalletRPCCommands(interfaces::Chain& chain, std::vector<std::unique_ptr<interfaces::Handler>>& handlers)
+Span<const CRPCCommand> GetWalletRPCCommands()
 {
 // clang-format off
 static const CRPCCommand commands[] =
@@ -4329,9 +4341,5 @@ static const CRPCCommand commands[] =
     { "wallet",             "walletprocesspsbt",                &walletprocesspsbt,             {"psbt","sign","sighashtype","bip32derivs"} },
 };
 // clang-format on
-
-    for (unsigned int vcidx = 0; vcidx < ARRAYLEN(commands); vcidx++)
-        handlers.emplace_back(chain.handleRpc(commands[vcidx]));
+    return MakeSpan(commands);
 }
-
-interfaces::Chain* g_rpc_chain = nullptr;

src/wallet/rpcwallet.h

@@ -5,30 +5,22 @@
 #ifndef BITCOIN_WALLET_RPCWALLET_H
 #define BITCOIN_WALLET_RPCWALLET_H
 
+#include <span.h>
+
 #include <memory>
 #include <string>
 #include <vector>
 
-class CRPCTable;
+class CRPCCommand;
 class CWallet;
 class JSONRPCRequest;
 class LegacyScriptPubKeyMan;
 class UniValue;
-struct PartiallySignedTransaction;
 class CTransaction;
+struct PartiallySignedTransaction;
+struct WalletContext;
 
-namespace interfaces {
-class Chain;
-class Handler;
-}
-
-//! Pointer to chain interface that needs to be declared as a global to be
-//! accessible loadwallet and createwallet methods. Due to limitations of the
-//! RPC framework, there's currently no direct way to pass in state to RPC
-//! methods without globals.
-extern interfaces::Chain* g_rpc_chain;
-
-void RegisterWalletRPCCommands(interfaces::Chain& chain, std::vector<std::unique_ptr<interfaces::Handler>>& handlers);
+Span<const CRPCCommand> GetWalletRPCCommands();
 
 /**
  * Figures out what wallet, if any, to use for a JSONRPCRequest.
@@ -40,6 +32,7 @@ std::shared_ptr<CWallet> GetWalletForJSONRPCRequest(const JSONRPCRequest& reques
 
 void EnsureWalletIsUnlocked(const CWallet*);
 bool EnsureWalletIsAvailable(const CWallet*, bool avoidException);
+WalletContext& EnsureWalletContext(const util::Ref& context);
 LegacyScriptPubKeyMan& EnsureLegacyScriptPubKeyMan(CWallet& wallet, bool also_create = false);
 
 UniValue getaddressinfo(const JSONRPCRequest& request);