test: automatically pick bitcoind or bitcoin-node

ci/test/00_setup_env_mac_native.sh

@@ -16,4 +16,3 @@ export BITCOIN_CONFIG="-DBUILD_GUI=ON -DWITH_ZMQ=ON -DREDUCE_EXPORTS=ON -DCMAKE_
 export CI_OS_NAME="macos"
 export NO_DEPENDS=1
 export OSX_SDK=""
-export BITCOIN_CMD="bitcoin -m" # Used in functional tests

ci/test/00_setup_env_native_centos.sh

@@ -18,4 +18,3 @@ export BITCOIN_CONFIG="\
  -DREDUCE_EXPORTS=ON \
  -DCMAKE_BUILD_TYPE=Debug \
 "
-export BITCOIN_CMD="bitcoin -m" # Used in functional tests

doc/multiprocess.md

@@ -26,7 +26,6 @@ HOST_PLATFORM="x86_64-pc-linux-gnu"
 cmake -B build --toolchain=depends/$HOST_PLATFORM/toolchain.cmake
 cmake --build build
 build/bin/bitcoin -m node -regtest -printtoconsole -debug=ipc
-BITCOIN_CMD="bitcoin -m" build/test/functional/test_runner.py
 ```
 
 The `cmake` build will pick up settings and library locations from the depends directory, so there is no need to pass `-DENABLE_IPC=ON` as a separate flag when using the depends system (it's controlled by the `NO_IPC=1` option).

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)
 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,14 +48,15 @@ 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;
     std::vector<const char*> args;
 };
 
 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 @@ class ArgsManager
     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 @@ class ArgsManager
 
     /**
      * 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 @@ class BitcoinGuiInit : public interfaces::Init
     // 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 @@ class BitcoinNodeInit : public interfaces::Init
     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 @@ class BitcoinQtInit : public interfaces::Init
         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 @@ class BitcoindInit : public interfaces::Init
         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 @@ class Init
     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_framework/test_framework.py

@@ -15,7 +15,6 @@
 import pdb
 import random
 import re
-import shlex
 import shutil
 import subprocess
 import sys
@@ -103,20 +102,21 @@ def chainstate_argv(self):
 
     def _argv(self, command, bin_path, need_ipc=False):
         """Return argv array that should be used to invoke the command. It
-        either uses the bitcoin wrapper executable (if BITCOIN_CMD is set or
-        need_ipc is True), or the direct binary path (bitcoind, etc). When
-        bin_dir is set (by tests calling binaries from previous releases) it
-        always uses the direct path."""
+        uses the bitcoin wrapper executable need_ipc is True or any command
+        other than 'node' (e.g. 'rpc'). Otherwise it uses bitcoind.
+        When bin_dir is set (by tests calling binaries from previous releases)
+        it always uses the direct binary path (bitcoind, etc)."""
         if self.bin_dir is not None:
             return [os.path.join(self.bin_dir, os.path.basename(bin_path))]
-        elif self.paths.bitcoin_cmd is not None or need_ipc:
+        elif need_ipc or command != "node":
             # If the current test needs IPC functionality, use the bitcoin
-            # wrapper binary and append -m so it calls multiprocess binaries.
-            bitcoin_cmd = self.paths.bitcoin_cmd or [self.paths.bitcoin_bin]
-            return bitcoin_cmd + (["-m"] if need_ipc else []) + [command]
+            # wrapper binary. It will decide based on arguments whether to
+            # launch the bitcoind or bitcoin-node binary.
+            # The wrapper binary is also used for RPC calls (bitcoin-cli).
+            return [self.paths.bitcoin_bin, command]
         else:
-            return [bin_path]
-
+            # Use bitcoind otherwise to retain coverage.
+            return [self.paths.bitcoind]
 
 class BitcoinTestMetaClass(type):
     """Metaclass for BitcoinTestFramework.
@@ -299,9 +299,6 @@ def get_binary_paths(self):
                 binary + self.config["environment"]["EXEEXT"],
             )
             setattr(paths, env_variable_name.lower(), os.getenv(env_variable_name, default=default_filename))
-        # BITCOIN_CMD environment variable can be specified to invoke bitcoin
-        # wrapper binary instead of other executables.
-        paths.bitcoin_cmd = shlex.split(os.getenv("BITCOIN_CMD", "")) or None
         return paths
 
     def get_binaries(self, bin_dir=None):

test/functional/test_runner.py

@@ -335,6 +335,7 @@
     '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_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():