Merge bitcoin/bitcoin#33229: multiprocess: Don't require bitcoin -m argument when IPC options are used

453b0fa286 bitcoin: Make wrapper not require -m (Ryan Ofsky)
29e836fae6 test: add tool_bitcoin to test bitcoin wrapper behavior (Ryan Ofsky)
0972f55040 init: add exe name to bitcoind, bitcoin-node -version output to be able to distinguish these in tests (Ryan Ofsky)

Pull request description:

  This change makes the `bitcoin` command respect IPC command line options and _bitcoin.conf_ settings, so IPC listening can be enabled by just running `bitcoin node -ipcbind=unix` or `bitcoin node` with `ipcbind=unix` in the configuration file, and there is no longer a need to specify a multiprocess `-m` option like `bitcoin -m node [...]`

  sipa and theuni in #31802 pointed out that users shouldn't be exposed to multiprocess implementation details just to use IPC features, so current need to specify the `bitcoin -m` option in conjunction with `-ipcbind` could be seen as a design mistake and not just a usage inconvenience.

  This PR also adds a dedicated functional test for the `bitcoin` wrapper command and to make sure it calls the right binaries and test the new functionality.

  ---

  This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722).

ACKs for top commit:
  Sjors:
    re-ACK 453b0fa286
  achow101:
    ACK 453b0fa286
  TheCharlatan:
    Re-ACK 453b0fa286

Tree-SHA512: 9e49cb7e183fd220fa7a4e8ac68cef55f3cb2ccec40ad2a9d3e3f31db64c4953db8337f8caf7fce877bc97002ae97568dcf47ee269a06ca1f503f119bfe392c1

src/CMakeLists.txt

@@ -292,7 +292,7 @@ if(BUILD_BITCOIN_BIN)
   add_executable(bitcoin bitcoin.cpp)
   add_windows_resources(bitcoin bitcoin-res.rc)
   add_windows_application_manifest(bitcoin)
-  target_link_libraries(bitcoin core_interface bitcoin_util)
+  target_link_libraries(bitcoin core_interface bitcoin_common bitcoin_util)
   install_binary_component(bitcoin HAS_MANPAGE)
 endif()
 

src/bitcoin.cpp

@@ -5,6 +5,7 @@
 #include <bitcoin-build-config.h> // IWYU pragma: keep
 
 #include <clientversion.h>
+#include <common/args.h>
 #include <util/fs.h>
 #include <util/exec.h>
 #include <util/strencodings.h>
@@ -47,7 +48,7 @@ Run '%s help' to see additional commands (e.g. for testing and debugging).
 )";
 
 struct CommandLine {
-    bool use_multiprocess{false};
+    std::optional<bool> use_multiprocess;
     bool show_version{false};
     bool show_help{false};
     std::string_view command;
@@ -55,6 +56,7 @@ struct CommandLine {
 };
 
 CommandLine ParseCommandLine(int argc, char* argv[]);
+bool UseMultiprocess(const CommandLine& cmd);
 static void ExecCommand(const std::vector<const char*>& args, std::string_view argv0);
 
 int main(int argc, char* argv[])
@@ -78,9 +80,9 @@ int main(int argc, char* argv[])
                 return EXIT_FAILURE;
             }
         } else if (cmd.command == "gui") {
-            args.emplace_back(cmd.use_multiprocess ? "bitcoin-gui" : "bitcoin-qt");
+            args.emplace_back(UseMultiprocess(cmd) ? "bitcoin-gui" : "bitcoin-qt");
         } else if (cmd.command == "node") {
-            args.emplace_back(cmd.use_multiprocess ? "bitcoin-node" : "bitcoind");
+            args.emplace_back(UseMultiprocess(cmd) ? "bitcoin-node" : "bitcoind");
         } else if (cmd.command == "rpc") {
             args.emplace_back("bitcoin-cli");
             // Since "bitcoin rpc" is a new interface that doesn't need to be
@@ -143,6 +145,30 @@ CommandLine ParseCommandLine(int argc, char* argv[])
     return cmd;
 }
 
+bool UseMultiprocess(const CommandLine& cmd)
+{
+    // If -m or -M options were explicitly specified, there is no need to
+    // further parse arguments to determine which to use.
+    if (cmd.use_multiprocess) return *cmd.use_multiprocess;
+
+    ArgsManager args;
+    args.SetDefaultFlags(ArgsManager::ALLOW_ANY);
+    std::string error_message;
+    auto argv{cmd.args};
+    argv.insert(argv.begin(), nullptr);
+    if (!args.ParseParameters(argv.size(), argv.data(), error_message)) {
+        tfm::format(std::cerr, "Warning: failed to parse subcommand command line options: %s\n", error_message);
+    }
+    if (!args.ReadConfigFiles(error_message, true)) {
+        tfm::format(std::cerr, "Warning: failed to parse subcommand config: %s\n", error_message);
+    }
+    args.SelectConfigNetwork(args.GetChainTypeString());
+
+    // If any -ipc* options are set these need to be processed by a
+    // multiprocess-capable binary.
+    return args.IsArgSet("-ipcbind") || args.IsArgSet("-ipcconnect") || args.IsArgSet("-ipcfd");
+}
+
 //! Execute the specified bitcoind, bitcoin-qt or other command line in `args`
 //! using src, bin and libexec directory paths relative to this executable, where
 //! the path to this executable is specified in `wrapper_argv0`.

src/bitcoind.cpp

@@ -132,11 +132,16 @@ static bool ParseArgs(NodeContext& node, int argc, char* argv[])
     return true;
 }
 
-static bool ProcessInitCommands(ArgsManager& args)
+static bool ProcessInitCommands(interfaces::Init& init, ArgsManager& args)
 {
     // Process help and version before taking care about datadir
     if (HelpRequested(args) || args.GetBoolArg("-version", false)) {
-        std::string strUsage = CLIENT_NAME " daemon version " + FormatFullVersion() + "\n";
+        std::string strUsage = CLIENT_NAME " daemon version " + FormatFullVersion();
+        if (const char* exe_name{init.exeName()}) {
+            strUsage += " ";
+            strUsage += exe_name;
+        }
+        strUsage += "\n";
 
         if (args.GetBoolArg("-version", false)) {
             strUsage += FormatParagraph(LicenseInfo());
@@ -277,7 +282,7 @@ MAIN_FUNCTION
     ArgsManager& args = *Assert(node.args);
     if (!ParseArgs(node, argc, argv)) return EXIT_FAILURE;
     // Process early info return commands such as -help or -version
-    if (ProcessInitCommands(args)) return EXIT_SUCCESS;
+    if (ProcessInitCommands(*init, args)) return EXIT_SUCCESS;
 
     // Start application
     if (!AppInit(node) || !Assert(node.shutdown_signal)->wait()) {

src/common/args.cpp

@@ -266,7 +266,13 @@ std::optional<unsigned int> ArgsManager::GetArgFlags(const std::string& name) co
             return search->second.m_flags;
         }
     }
-    return std::nullopt;
+    return m_default_flags;
+}
+
+void ArgsManager::SetDefaultFlags(std::optional<unsigned int> flags)
+{
+    LOCK(cs_args);
+    m_default_flags = flags;
 }
 
 fs::path ArgsManager::GetPathArg(std::string arg, const fs::path& default_value) const

src/common/args.h

@@ -137,6 +137,7 @@ protected:
     std::string m_network GUARDED_BY(cs_args);
     std::set<std::string> m_network_only_args GUARDED_BY(cs_args);
     std::map<OptionsCategory, std::map<std::string, Arg>> m_available_args GUARDED_BY(cs_args);
+    std::optional<unsigned int> m_default_flags GUARDED_BY(cs_args){};
     bool m_accept_any_command GUARDED_BY(cs_args){true};
     std::list<SectionInfo> m_config_sections GUARDED_BY(cs_args);
     std::optional<fs::path> m_config_path GUARDED_BY(cs_args);
@@ -375,10 +376,15 @@ protected:
 
     /**
      * Return Flags for known arg.
-     * Return nullopt for unknown arg.
+     * Return default flags for unknown arg.
      */
     std::optional<unsigned int> GetArgFlags(const std::string& name) const;
 
+    /**
+     * Set default flags to return for an unknown arg.
+     */
+    void SetDefaultFlags(std::optional<unsigned int>);
+
     /**
      * Get settings file path, or return false if read-write settings were
      * disabled with -nosettings.

src/init/bitcoin-gui.cpp

@@ -39,6 +39,7 @@ public:
     // bitcoin-node accepts the option, and bitcoin-gui accepts all bitcoin-node
     // options and will start the node with those options.
     bool canListenIpc() override { return true; }
+    const char* exeName() override { return EXE_NAME; }
     node::NodeContext m_node;
     std::unique_ptr<interfaces::Ipc> m_ipc;
 };

src/init/bitcoin-node.cpp

@@ -38,6 +38,7 @@ public:
     std::unique_ptr<interfaces::Echo> makeEcho() override { return interfaces::MakeEcho(); }
     interfaces::Ipc* ipc() override { return m_ipc.get(); }
     bool canListenIpc() override { return true; }
+    const char* exeName() override { return EXE_NAME; }
     node::NodeContext& m_node;
     std::unique_ptr<interfaces::Ipc> m_ipc;
 };

src/init/bitcoin-qt.cpp

@@ -16,6 +16,8 @@
 
 namespace init {
 namespace {
+const char* EXE_NAME = "bitcoin-qt";
+
 class BitcoinQtInit : public interfaces::Init
 {
 public:
@@ -32,6 +34,7 @@ public:
         return MakeWalletLoader(chain, *Assert(m_node.args));
     }
     std::unique_ptr<interfaces::Echo> makeEcho() override { return interfaces::MakeEcho(); }
+    const char* exeName() override { return EXE_NAME; }
     node::NodeContext m_node;
 };
 } // namespace

src/init/bitcoind.cpp

@@ -18,6 +18,8 @@ using node::NodeContext;
 
 namespace init {
 namespace {
+const char* EXE_NAME = "bitcoind";
+
 class BitcoindInit : public interfaces::Init
 {
 public:
@@ -34,6 +36,7 @@ public:
         return MakeWalletLoader(chain, *Assert(m_node.args));
     }
     std::unique_ptr<interfaces::Echo> makeEcho() override { return interfaces::MakeEcho(); }
+    const char* exeName() override { return EXE_NAME; }
     NodeContext& m_node;
 };
 } // namespace

src/interfaces/init.h

@@ -38,6 +38,7 @@ public:
     virtual std::unique_ptr<Echo> makeEcho() { return nullptr; }
     virtual Ipc* ipc() { return nullptr; }
     virtual bool canListenIpc() { return false; }
+    virtual const char* exeName() { return nullptr; }
 };
 
 //! Return implementation of Init interface for the node process. If the argv

test/functional/test_runner.py

@@ -337,6 +337,7 @@ BASE_SCRIPTS = [
     'p2p_tx_privacy.py',
     'rpc_getdescriptoractivity.py',
     'rpc_scanblocks.py',
+    'tool_bitcoin.py',
     'p2p_sendtxrcncl.py',
     'rpc_scantxoutset.py',
     'feature_unsupported_utxo_db.py',

test/functional/tool_bitcoin.py

@@ -0,0 +1,111 @@
+#!/usr/bin/env python3
+# Copyright (c) The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Test the bitcoin wrapper tool."""
+from test_framework.test_framework import (
+    BitcoinTestFramework,
+    SkipTest,
+)
+from test_framework.util import (
+    append_config,
+    assert_equal,
+)
+
+import platform
+import re
+
+
+class ToolBitcoinTest(BitcoinTestFramework):
+    def set_test_params(self):
+        self.setup_clean_chain = True
+        self.num_nodes = 1
+
+    def skip_test_if_missing_module(self):
+        # Skip test on windows because currently when `bitcoin node -version` is
+        # run on windows, python doesn't capture output from the child
+        # `bitcoind` and `bitcoin-node` process started with _wexecvp, and
+        # stdout/stderr are always empty. See
+        # https://github.com/bitcoin/bitcoin/pull/33229#issuecomment-3265524908
+        if platform.system() == "Windows":
+            raise SkipTest("Test does not currently work on windows")
+
+    def setup_network(self):
+        """Set up nodes normally, but save a copy of their arguments before starting them."""
+        self.add_nodes(self.num_nodes, self.extra_args)
+        node_argv = self.get_binaries().node_argv()
+        self.node_options = [node.args[len(node_argv):] for node in self.nodes]
+        assert all(node.args[:len(node_argv)] == node_argv for node in self.nodes)
+
+    def set_cmd_args(self, node, args):
+        """Set up node so it will be started through bitcoin wrapper command with specified arguments."""
+        node.args = [self.binary_paths.bitcoin_bin] + args + ["node"] + self.node_options[node.index]
+
+    def test_args(self, cmd_args, node_args, expect_exe=None, expect_error=None):
+        node = self.nodes[0]
+        self.set_cmd_args(node, cmd_args)
+        extra_args = node_args + ["-version"]
+        if expect_error is not None:
+            node.assert_start_raises_init_error(expected_msg=expect_error, extra_args=extra_args)
+        else:
+            assert expect_exe
+            node.start(extra_args=extra_args)
+            ret, out, err = get_node_output(node)
+            try:
+                assert_equal(get_exe_name(out), expect_exe.encode())
+                assert_equal(err, b"")
+            except Exception as e:
+                raise RuntimeError(f"Unexpected output from {node.args + extra_args}: {out=!r} {err=!r} {ret=!r}") from e
+
+    def run_test(self):
+        node = self.nodes[0]
+
+        self.log.info("Ensure bitcoin node command invokes bitcoind by default")
+        self.test_args([], [], expect_exe="bitcoind")
+
+        self.log.info("Ensure bitcoin -M invokes bitcoind")
+        self.test_args(["-M"], [], expect_exe="bitcoind")
+
+        self.log.info("Ensure bitcoin -M does not accept -ipcbind")
+        self.test_args(["-M"], ["-ipcbind=unix"], expect_error='Error: Error parsing command line arguments: Invalid parameter -ipcbind=unix')
+
+        if self.is_ipc_compiled():
+            self.log.info("Ensure bitcoin -m invokes bitcoin-node")
+            self.test_args(["-m"], [], expect_exe="bitcoin-node")
+
+            self.log.info("Ensure bitcoin -m does accept -ipcbind")
+            self.test_args(["-m"], ["-ipcbind=unix"], expect_exe="bitcoin-node")
+
+            self.log.info("Ensure bitcoin accepts -ipcbind by default")
+            self.test_args([], ["-ipcbind=unix"], expect_exe="bitcoin-node")
+
+            self.log.info("Ensure bitcoin recognizes -ipcbind in config file")
+            append_config(node.datadir_path, ["ipcbind=unix"])
+            self.test_args([], [], expect_exe="bitcoin-node")
+
+
+def get_node_output(node):
+    ret = node.process.wait(timeout=60)
+    node.stdout.seek(0)
+    node.stderr.seek(0)
+    out = node.stdout.read()
+    err = node.stderr.read()
+    node.stdout.close()
+    node.stderr.close()
+
+    # Clean up TestNode state
+    node.running = False
+    node.process = None
+    node.rpc_connected = False
+    node.rpc = None
+
+    return ret, out, err
+
+
+def get_exe_name(version_str):
+    """Get exe name from last word of first line of version string."""
+    return re.match(rb".*?(\S+)\s*?(?:\n|$)", version_str.strip()).group(1)
+
+
+if __name__ == '__main__':
+    ToolBitcoinTest(__file__).main()

test/lint/check-doc.py

@@ -23,7 +23,7 @@ CMD_GREP_WALLET_ARGS = r"git grep --function-context 'void WalletInit::AddWallet
 CMD_GREP_WALLET_HIDDEN_ARGS = r"git grep --function-context 'void DummyWalletInit::AddWalletOptions' -- {}".format(CMD_ROOT_DIR)
 CMD_GREP_DOCS = r"git grep --perl-regexp '{}' {}".format(REGEX_DOC, CMD_ROOT_DIR)
 # list unsupported, deprecated and duplicate args as they need no documentation
-SET_DOC_OPTIONAL = set(['-h', '-?', '-dbcrashratio', '-forcecompactdb'])
+SET_DOC_OPTIONAL = set(['-h', '-?', '-dbcrashratio', '-forcecompactdb', '-ipcconnect', '-ipcfd'])
 
 
 def lint_missing_argument_documentation():