Merge #19099: refactor: Move wallet methods out of chain.h and node.h

24bf176 gui refactor: Inline SplashScreen::ConnectWallet (Russell Yanofsky)
e4f4350 refactor: Move wallet methods out of chain.h and node.h (Russell Yanofsky)
b266b3e refactor: Create interfaces earlier during initialization (Russell Yanofsky)

Pull request description:

  Add WalletClient interface so node interface is cleaner and don't need wallet-specific methods.

  The new NodeContext::wallet_client pointer will also be needed to eliminate global wallet variables like ::vpwallets in #19101, because createWallet(), loadWallet(), getWallets(), etc methods called by the GUI need a way to get a reference to the list of open wallets if it is no longer a global variable.

ACKs for top commit:
  promag:
    Code review ACK 24bf176.
  MarcoFalke:
    ACK 24bf176 🐚

Tree-SHA512: a70d3776cd6723093db8912028c50075ec5fa0a48b961cb1a945f922658f5363754f8380dbb8378ed128c8c858913024f8264740905b8121a35c0d63bfaed7cf

Author: MarcoFalke

src/bitcoind.cpp

@@ -44,7 +44,6 @@ static void WaitForShutdown(NodeContext& node)
 static bool AppInit(int argc, char* argv[])
 {
     NodeContext node;
-    node.chain = interfaces::MakeChain(node);
 
     bool fRet = false;
 
@@ -144,7 +143,7 @@ static bool AppInit(int argc, char* argv[])
             // If locking the data directory failed, exit immediately
             return false;
         }
-        fRet = AppInitMain(context, node);
+        fRet = AppInitInterfaces(node) && AppInitMain(context, node);
     }
     catch (const std::exception& e) {
         PrintExceptionContinue(&e, "AppInit()");

src/dummywallet.cpp

@@ -4,11 +4,8 @@
 
 #include <util/system.h>
 #include <walletinitinterface.h>
-#include <support/allocators/secure.h>
 
 class CWallet;
-enum class WalletCreationStatus;
-struct bilingual_str;
 
 namespace interfaces {
 class Chain;
@@ -59,37 +56,6 @@ void DummyWalletInit::AddWalletOptions(ArgsManager& argsman) const
 
 const WalletInitInterface& g_wallet_init_interface = DummyWalletInit();
 
-fs::path GetWalletDir()
-{
-    throw std::logic_error("Wallet function called in non-wallet build.");
-}
-
-std::vector<fs::path> ListWalletDir()
-{
-    throw std::logic_error("Wallet function called in non-wallet build.");
-}
-
-std::vector<std::shared_ptr<CWallet>> GetWallets()
-{
-    throw std::logic_error("Wallet function called in non-wallet build.");
-}
-
-std::shared_ptr<CWallet> LoadWallet(interfaces::Chain& chain, const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings)
-{
-    throw std::logic_error("Wallet function called in non-wallet build.");
-}
-
-WalletCreationStatus CreateWallet(interfaces::Chain& chain, const SecureString& passphrase, uint64_t wallet_creation_flags, const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings, std::shared_ptr<CWallet>& result)
-{
-    throw std::logic_error("Wallet function called in non-wallet build.");
-}
-
-using LoadWalletFn = std::function<void(std::unique_ptr<interfaces::Wallet> wallet)>;
-std::unique_ptr<interfaces::Handler> HandleLoadWallet(LoadWalletFn load_wallet)
-{
-    throw std::logic_error("Wallet function called in non-wallet build.");
-}
-
 namespace interfaces {
 
 std::unique_ptr<Wallet> MakeWallet(const std::shared_ptr<CWallet>& wallet)

src/init.cpp

@@ -1229,6 +1229,17 @@ bool AppInitLockDataDirectory()
     return true;
 }
 
+bool AppInitInterfaces(NodeContext& node)
+{
+    node.chain = interfaces::MakeChain(node);
+    // Create client interfaces for wallets that are supposed to be loaded
+    // according to -wallet and -disablewallet options. This only constructs
+    // the interfaces, it doesn't load wallet data. Wallets actually get loaded
+    // when load() and start() interface methods are called below.
+    g_wallet_init_interface.Construct(node);
+    return true;
+}
+
 bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
 {
     const ArgsManager& args = *Assert(node.args);
@@ -1318,12 +1329,6 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
 
     GetMainSignals().RegisterBackgroundSignalScheduler(*node.scheduler);
 
-    // Create client interfaces for wallets that are supposed to be loaded
-    // according to -wallet and -disablewallet options. This only constructs
-    // the interfaces, it doesn't load wallet data. Wallets actually get loaded
-    // when load() and start() interface methods are called below.
-    g_wallet_init_interface.Construct(node);
-
     /* Register RPC commands regardless of -server setting so they will be
      * available in the GUI RPC console even if external calls are disabled.
      */

src/init.h

@@ -52,6 +52,10 @@ bool AppInitSanityChecks();
  * @pre Parameters should be parsed and config file should be read, AppInitSanityChecks should have been called.
  */
 bool AppInitLockDataDirectory();
+/**
+ * Initialize node and wallet interface pointers. Has no prerequisites or side effects besides allocating memory.
+ */
+bool AppInitInterfaces(NodeContext& node);
 /**
  * Bitcoin core main initialization.
  * @note This should only be done after daemonization. Call Shutdown() if this function fails.

src/interfaces/chain.h

@@ -314,24 +314,11 @@ class ChainClient
 
     //! Set mock time.
     virtual void setMockTime(int64_t time) = 0;
-
-    //! Return interfaces for accessing wallets (if any).
-    virtual std::vector<std::unique_ptr<Wallet>> getWallets() = 0;
 };
 
 //! Return implementation of Chain interface.
 std::unique_ptr<Chain> MakeChain(NodeContext& node);
 
-//! Return implementation of ChainClient interface for a wallet client. This
-//! function will be undefined in builds where ENABLE_WALLET is false.
-//!
-//! Currently, wallets are the only chain clients. But in the future, other
-//! types of chain clients could be added, such as tools for monitoring,
-//! analysis, or fee estimation. These clients need to expose their own
-//! MakeXXXClient functions returning their implementations of the ChainClient
-//! interface.
-std::unique_ptr<ChainClient> MakeWalletClient(Chain& chain, ArgsManager& args, std::vector<std::string> wallet_filenames);
-
 } // namespace interfaces
 
 #endif // BITCOIN_INTERFACES_CHAIN_H

src/interfaces/node.cpp

@@ -27,6 +27,7 @@
 #include <support/allocators/secure.h>
 #include <sync.h>
 #include <txmempool.h>
+#include <util/check.h>
 #include <util/ref.h>
 #include <util/system.h>
 #include <util/translation.h>
@@ -41,16 +42,7 @@
 
 #include <boost/signals2/signal.hpp>
 
-class CWallet;
-fs::path GetWalletDir();
-std::vector<fs::path> ListWalletDir();
-std::vector<std::shared_ptr<CWallet>> GetWallets();
-std::shared_ptr<CWallet> LoadWallet(interfaces::Chain& chain, const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings);
-WalletCreationStatus CreateWallet(interfaces::Chain& chain, const SecureString& passphrase, uint64_t wallet_creation_flags, const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings, std::shared_ptr<CWallet>& result);
-std::unique_ptr<interfaces::Handler> HandleLoadWallet(interfaces::Node::LoadWalletFn load_wallet);
-
 namespace interfaces {
-
 namespace {
 
 class NodeImpl : public Node
@@ -64,11 +56,10 @@ class NodeImpl : public Node
     bool baseInitialize() override
     {
         return AppInitBasicSetup(gArgs) && AppInitParameterInteraction(gArgs) && AppInitSanityChecks() &&
-               AppInitLockDataDirectory();
+               AppInitLockDataDirectory() && AppInitInterfaces(*m_context);
     }
     bool appInitMain(interfaces::BlockAndHeaderTipInfo* tip_info) override
     {
-        m_context->chain = MakeChain(*m_context);
         return AppInitMain(m_context_ref, *m_context, tip_info);
     }
     void appShutdown() override
@@ -240,36 +231,9 @@ class NodeImpl : public Node
         LOCK(::cs_main);
         return ::ChainstateActive().CoinsTip().GetCoin(output, coin);
     }
-    std::string getWalletDir() override
-    {
-        return GetWalletDir().string();
-    }
-    std::vector<std::string> listWalletDir() override
-    {
-        std::vector<std::string> paths;
-        for (auto& path : ListWalletDir()) {
-            paths.push_back(path.string());
-        }
-        return paths;
-    }
-    std::vector<std::unique_ptr<Wallet>> getWallets() override
+    WalletClient& walletClient() override
     {
-        std::vector<std::unique_ptr<Wallet>> wallets;
-        for (auto& client : m_context->chain_clients) {
-            auto client_wallets = client->getWallets();
-            std::move(client_wallets.begin(), client_wallets.end(), std::back_inserter(wallets));
-        }
-        return wallets;
-    }
-    std::unique_ptr<Wallet> loadWallet(const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings) override
-    {
-        return MakeWallet(LoadWallet(*m_context->chain, name, error, warnings));
-    }
-    std::unique_ptr<Wallet> createWallet(const SecureString& passphrase, uint64_t wallet_creation_flags, const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings, WalletCreationStatus& status) override
-    {
-        std::shared_ptr<CWallet> wallet;
-        status = CreateWallet(*m_context->chain, passphrase, wallet_creation_flags, name, error, warnings, wallet);
-        return MakeWallet(wallet);
+        return *Assert(m_context->wallet_client);
     }
     std::unique_ptr<Handler> handleInitMessage(InitMessageFn fn) override
     {
@@ -287,10 +251,6 @@ class NodeImpl : public Node
     {
         return MakeHandler(::uiInterface.ShowProgress_connect(fn));
     }
-    std::unique_ptr<Handler> handleLoadWallet(LoadWalletFn fn) override
-    {
-        return HandleLoadWallet(std::move(fn));
-    }
     std::unique_ptr<Handler> handleNotifyNumConnectionsChanged(NotifyNumConnectionsChangedFn fn) override
     {
         return MakeHandler(::uiInterface.NotifyNumConnectionsChanged_connect(fn));

src/interfaces/node.h

@@ -29,14 +29,13 @@ class RPCTimerInterface;
 class UniValue;
 class proxyType;
 enum class SynchronizationState;
-enum class WalletCreationStatus;
 struct CNodeStateStats;
 struct NodeContext;
 struct bilingual_str;
 
 namespace interfaces {
 class Handler;
-class Wallet;
+class WalletClient;
 struct BlockTip;
 
 //! Block and header tip information
@@ -173,22 +172,8 @@ class Node
     //! Get unspent outputs associated with a transaction.
     virtual bool getUnspentOutput(const COutPoint& output, Coin& coin) = 0;
 
-    //! Return default wallet directory.
-    virtual std::string getWalletDir() = 0;
-
-    //! Return available wallets in wallet directory.
-    virtual std::vector<std::string> listWalletDir() = 0;
-
-    //! Return interfaces for accessing wallets (if any).
-    virtual std::vector<std::unique_ptr<Wallet>> getWallets() = 0;
-
-    //! Attempts to load a wallet from file or directory.
-    //! The loaded wallet is also notified to handlers previously registered
-    //! with handleLoadWallet.
-    virtual std::unique_ptr<Wallet> loadWallet(const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings) = 0;
-
-    //! Create a wallet from file
-    virtual std::unique_ptr<Wallet> createWallet(const SecureString& passphrase, uint64_t wallet_creation_flags, const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings, WalletCreationStatus& status) = 0;
+    //! Get wallet client.
+    virtual WalletClient& walletClient() = 0;
 
     //! Register handler for init messages.
     using InitMessageFn = std::function<void(const std::string& message)>;
@@ -210,10 +195,6 @@ class Node
     using ShowProgressFn = std::function<void(const std::string& title, int progress, bool resume_possible)>;
     virtual std::unique_ptr<Handler> handleShowProgress(ShowProgressFn fn) = 0;
 
-    //! Register handler for load wallet messages.
-    using LoadWalletFn = std::function<void(std::unique_ptr<Wallet> wallet)>;
-    virtual std::unique_ptr<Handler> handleLoadWallet(LoadWalletFn fn) = 0;
-
     //! Register handler for number of connections changed messages.
     using NotifyNumConnectionsChangedFn = std::function<void(int new_num_connections)>;
     virtual std::unique_ptr<Handler> handleNotifyNumConnectionsChanged(NotifyNumConnectionsChangedFn fn) = 0;

src/interfaces/wallet.cpp

@@ -485,7 +485,7 @@ class WalletImpl : public Wallet
     std::shared_ptr<CWallet> m_wallet;
 };
 
-class WalletClientImpl : public ChainClient
+class WalletClientImpl : public WalletClient
 {
 public:
     WalletClientImpl(Chain& chain, ArgsManager& args, std::vector<std::string> wallet_filenames)
@@ -494,6 +494,9 @@ class WalletClientImpl : public ChainClient
         m_context.chain = &chain;
         m_context.args = &args;
     }
+    ~WalletClientImpl() override { UnloadWallets(); }
+
+    //! ChainClient methods
     void registerRpcs() override
     {
         for (const CRPCCommand& command : GetWalletRPCCommands()) {
@@ -509,6 +512,30 @@ class WalletClientImpl : public ChainClient
     void flush() override { return FlushWallets(); }
     void stop() override { return StopWallets(); }
     void setMockTime(int64_t time) override { return SetMockTime(time); }
+
+    //! WalletClient methods
+    std::unique_ptr<Wallet> createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags, WalletCreationStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings) override
+    {
+        std::shared_ptr<CWallet> wallet;
+        status = CreateWallet(*m_context.chain, passphrase, wallet_creation_flags, name, error, warnings, wallet);
+        return MakeWallet(std::move(wallet));
+    }
+    std::unique_ptr<Wallet> loadWallet(const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings) override
+    {
+        return MakeWallet(LoadWallet(*m_context.chain, WalletLocation(name), error, warnings));
+    }
+    std::string getWalletDir() override
+    {
+        return GetWalletDir().string();
+    }
+    std::vector<std::string> listWalletDir() override
+    {
+        std::vector<std::string> paths;
+        for (auto& path : ListWalletDir()) {
+            paths.push_back(path.string());
+        }
+        return paths;
+    }
     std::vector<std::unique_ptr<Wallet>> getWallets() override
     {
         std::vector<std::unique_ptr<Wallet>> wallets;
@@ -517,7 +544,10 @@ class WalletClientImpl : public ChainClient
         }
         return wallets;
     }
-    ~WalletClientImpl() override { UnloadWallets(); }
+    std::unique_ptr<Handler> handleLoadWallet(LoadWalletFn fn) override
+    {
+        return HandleLoadWallet(std::move(fn));
+    }
 
     WalletContext m_context;
     const std::vector<std::string> m_wallet_filenames;
@@ -529,7 +559,7 @@ class WalletClientImpl : public ChainClient
 
 std::unique_ptr<Wallet> MakeWallet(const std::shared_ptr<CWallet>& wallet) { return wallet ? MakeUnique<WalletImpl>(wallet) : nullptr; }
 
-std::unique_ptr<ChainClient> MakeWalletClient(Chain& chain, ArgsManager& args, std::vector<std::string> wallet_filenames)
+std::unique_ptr<WalletClient> MakeWalletClient(Chain& chain, ArgsManager& args, std::vector<std::string> wallet_filenames)
 {
     return MakeUnique<WalletClientImpl>(chain, args, std::move(wallet_filenames));
 }