highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-09-01 00:50:58 +02:00
Code Issues Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#31375: multiprocess: Add bitcoin wrapper executable

Browse Source 
a5ac43d98d doc: Add release notes describing bitcoin wrapper executable (Ryan Ofsky)
258bda80c0 doc: Mention bitcoin wrapper executable in documentation (Ryan Ofsky)
d2739d75c9 build: add bitcoin.exe to windows installer (Sjors Provoost)
ba649c0006 ci: Run multiprocess tests through wrapper executable (Ryan Ofsky)
29bdd743bb test: Support BITCOIN_CMD environment variable (Ryan Ofsky)
9c8c68891b multiprocess: Add bitcoin wrapper executable (Ryan Ofsky)
5076d20fdb util: Add cross-platform ExecVp and GetExePath functions (Ryan Ofsky)

Pull request description:

  Intended to make bitcoin command line features more discoverable and allow installing new multiprocess binaries in libexec/ instead of bin/ so they don't cause confusion.

  Idea and implementation of this were discussed in https://github.com/bitcoin/bitcoin/issues/30983.

  ---

  Initial implementation of this feature is deliberately minimal so the UX can evolve in response to feedback and there are not too many details to debate and discuss in a single PR. But many improvements are possible or planned:

  - Adding manpage and bash completions.
  - Showing nicer error messages that detect if an executable isn't installed and suggest how to fix [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2073194474)
  - Showing wrapper command lines in subcommand in help output [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077800405). This could be done conditionally as suggested in the comment or be unconditional.
  - Showing wrapper command lines in subcommand error output. There is a bitcoin-cli error pointed out in [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2091152243) that is needlessly confusing.
  - Integrating help so `bitcoin help subcommand` invokes `bitcoin subcommand -h`. `bitcoin -h subcommand` should also be supported and be equivalent [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2093116725)
  - Adding support for `bitcoin-util` subcommands. Ideal interface would probably be more like `bitcoin grind` not `bitcoin util grind` but this has been punted for now. Supporting subcommands directly would require some ArgsManager modifications
  - Adding a dedicated python functional test for the wrapper. Right now there is some CI coverage by setting the `BITCOIN_CMD` variable, but this doesn't cover things like the help output and version output, and support for different directory layouts.
  - Better `--multiprocess` (`-m`) / `--monolithic` (`-M`) default selection. Right now, default is monolithic but it probably makes sense to chose more intelligently depending on whether -ipc options are enabled and what binaries are available.
  - Maybe parsing `bitcoin.conf` and supporting options to control wrapper behavior like custom locations or preferences or aliases.
  - Better command command line usability. Allow combining short options like (`-ah`). Allow fuzzy matching of subcommands or suggestions if you misspell. (suggested by stickies in review club)
  - Not directly related to this PR but `bitcoin-cli named` implementation used by the wrapper should do a better job disambiguating named arguments from base64 arguments ending in = as pointed out in [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2091886628)

  ---

  This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722). A review club meeting for it took place in https://bitcoincore.reviews/31375

ACKs for top commit:
  Sjors:
    utACK a5ac43d98d
  achow101:
    ACK a5ac43d98d
  vasild:
    ACK a5ac43d98d
  theStack:
    ACK a5ac43d98d
  ismaelsadeeq:
    fwiw my last review implied an ACK a5ac43d98d
  hodlinator:
    ACK a5ac43d98d

Tree-SHA512: 570e6a4ff8bd79ef6554da3d01f36c0a7c6d2dd7dace8f8732eca98f4a8bc2284474a9beadeba783114fe2f3dd08b2041b3da7753bae0b7f881ec50668cb821f
This commit is contained in:
Ava Chow
2025-05-27 12:38:19 -07:00
parent 38ad8027a2 a5ac43d98d
commit 012f347685
26 changed files with 392 additions and 21 deletions
Download Patch File Download Diff File Expand all files Collapse all files

32
test/functional/test_framework/test_framework.py
View File

@@ -13,6 +13,7 @@ import platform
import pdb
import random
import re
import shlex
import shutil
import subprocess
import sys
@@ -73,33 +74,37 @@ class Binaries:
self.paths = paths
self.bin_dir = bin_dir
def daemon_argv(self):
def node_argv(self):
"Return argv array that should be used to invoke bitcoind"
return self._argv(self.paths.bitcoind)
return self._argv("node", self.paths.bitcoind)
def rpc_argv(self):
"Return argv array that should be used to invoke bitcoin-cli"
return self._argv(self.paths.bitcoincli)
# Add -nonamed because "bitcoin rpc" enables -named by default, but bitcoin-cli doesn't
return self._argv("rpc", self.paths.bitcoincli) + ["-nonamed"]
def util_argv(self):
"Return argv array that should be used to invoke bitcoin-util"
return self._argv(self.paths.bitcoinutil)
return self._argv("util", self.paths.bitcoinutil)
def wallet_argv(self):
"Return argv array that should be used to invoke bitcoin-wallet"
return self._argv(self.paths.bitcoinwallet)
return self._argv("wallet", self.paths.bitcoinwallet)
def chainstate_argv(self):
"Return argv array that should be used to invoke bitcoin-chainstate"
return self._argv(self.paths.bitcoinchainstate)
return self._argv("chainstate", self.paths.bitcoinchainstate)
def _argv(self, bin_path):
"""Return argv array that should be used to invoke the command.
Normally this will return binary paths directly from the paths object,
but when bin_dir is set (by tests calling binaries from previous
releases) it will return paths relative to bin_dir instead."""
def _argv(self, command, bin_path):
"""Return argv array that should be used to invoke the command. It
either uses the bitcoin wrapper executable (if BITCOIN_CMD is set), 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."""
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:
return self.paths.bitcoin_cmd + [command]
else:
return [bin_path]
@@ -293,6 +298,9 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
binary + self.config["environment"]["EXEEXT"],
)
setattr(paths, attribute_name, 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):
@@ -538,7 +546,7 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
bins_missing = False
for bin_path in (argv[0] for bin_dir in bin_dirs
for binaries in (self.get_binaries(bin_dir),)
for argv in (binaries.daemon_argv(), binaries.rpc_argv())):
for argv in (binaries.node_argv(), binaries.rpc_argv())):
if shutil.which(bin_path) is None:
self.log.error(f"Binary not found: {bin_path}")
bins_missing = True

2
test/functional/test_framework/test_node.py
View File

@@ -107,7 +107,7 @@ class TestNode():
# Configuration for logging is set as command-line args rather than in the bitcoin.conf file.
# This means that starting a bitcoind using the temp dir to debug a failed test won't
# spam debug.log.
self.args = self.binaries.daemon_argv() + [
self.args = self.binaries.node_argv() + [
f"-datadir={self.datadir_path}",
"-logtimemicros",
"-debug",