argsman, cli: GNU-style command-line option parsing (allows options after non-option arguments)#33540
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33540. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. LLM Linter (✨ experimental)Possible typos and grammar issues:
2026-06-07 18:38:02 |
This comment has been minimized.
This comment has been minimized.
This comment does not contribute to the technical discussion. Please put in more effort in the future or you may be banned. |
|
Haven't looked at code yet, so forgive if these are dumb questions, but could be nice if PR description addressed these:
|
|
Concept ACK on the idea. I think it probably makes sense to allow options arguments to follow non-option arguments even this change is not strictly backwards compatible, as long as there is some way to escape non-option arguments beginning with |
|
Assuming we are ok with losing some backwards compatibility by starting to treating arguments that begin with For example, bitcoin-cli createwallet mywallet --load_on_startup=1as an equivalent to: bitcoin-cli -named createwallet mywallet load_on_startup=1This seems like it could provide a more convenient way of using named RPC parameters. |
At the moment, please correct me if I'm wrong, there's no or I haven't seen an RPC that would receive a valid argument that starts with Testing the scenario you mentioned in On this PR would fail because it would be an invalid option (from the I think perhaps the scenario you mention is a valid one since any a string argument for an RPC could start with The change would look like this, and we can drop the 2nd commit (bringing back alive So even the following case would be successful:
Not on this PR. I see you think we could extend it with the usage of
The difference now will be that valid options starting with
Yeah the interpretation of numeric values come later on the command execution, not during the parsing in the Argsman, but when I made the change, there was a a test that was failing: So then I checked that there is another RPC case ( Line 120 in d298710 That's the reason of the 2nd commit, reintroducing doubles parsing, but since your first question on this thread, I'm considering this change to be pushed (added a test scenario where commands could receive args starting with |
8258dc3 to
8b97f47
Compare
|
-Updates:
|
8b97f47 to
a101bbb
Compare
|
-Updates:
|
w0xlt
left a comment
There was a problem hiding this comment.
This command works on master but not in this PR:
./build/bin/bitcoin-cli --datadir=/tmp/btc1 --signet getblockhash 1000
Could it be related to GetCommandArgs()?
There was a problem hiding this comment.
Restructuring ProcessOptionKey as below seems to solve the problem of ./build/bin/bitcoin-cli --datadir=/tmp/btc1 --signet getblockhash 1000.
That seems to fix the Method not found regression while preserving the GNU-style options after the command and the new double-dash-named-params feature.
bool ArgsManager::ProcessOptionKey(std::string& key, std::optional<std::string>& val, std::string& error, const bool found_after_non_option) {
bool double_dash{false};
const std::optional<std::string> original_val{val};
std::string original_input{key};
if (val) original_input += "=" + *val;
// Normalize leading dashes
if (key.length() > 1 && key[1] == '-') {
key.erase(0, 1); // `--foo` -> `-foo`
double_dash = true;
}
key.erase(0, 1); // `-foo` -> `foo`
KeyInfo keyinfo = InterpretKey(key);
std::optional<unsigned int> flags = GetArgFlags('-' + keyinfo.name);
const bool known_option = flags.has_value() && keyinfo.section.empty();
// Handle the special "named RPC" case early ---
if (double_dash && found_after_non_option && !known_option) {
// Try to map this to `-named`, if supported by the binary.
val.reset();
KeyInfo named_keyinfo = InterpretKey("named");
std::optional<unsigned int> named_flags = GetArgFlags('-' + named_keyinfo.name);
if (named_flags && named_keyinfo.section.empty()) {
// Binary supports -named: enable it and append the stripped parameter.
std::optional<common::SettingsValue> named_value =
InterpretValue(named_keyinfo, /* val */ nullptr, *named_flags, error);
if (!named_value) return false;
{
LOCK(cs_args);
m_settings.command_line_options[named_keyinfo.name].push_back(*named_value);
// Strip the leading "--" before passing to the command as a named param
m_command.emplace_back(original_input.substr(2));
}
return true;
}
// Binary doesn’t support -named: fall back and treat `--foo` as a
// normal option (or unknown option handled below).
keyinfo = InterpretKey(key);
flags = GetArgFlags('-' + keyinfo.name);
}
// --- Normal option handling ---
if (!flags || !keyinfo.section.empty()) {
if (!found_after_non_option) {
// Unknown global option: keep legacy behaviour (error)
error = strprintf("Invalid parameter %s", original_input);
return false;
}
// Unknown option after command: pass through to the command as an arg
LOCK(cs_args);
m_command.emplace_back(original_input);
return true;
}
// Known option (including `--datadir`, `--signet`, etc.)
std::optional<common::SettingsValue> value =
InterpretValue(keyinfo, val ? &*val : nullptr, *flags, error);
if (!value) return false;
LOCK(cs_args);
m_settings.command_line_options[keyinfo.name].push_back(*value);
return true;
}
Thanks for testing it (not sure if that's valid, no test failed, perhaps we need to add one?), anyways just putting the output's diff here.
this branch: |
Checking it... Thanks for the suggestion! |
Ok, all tests pass... First checked only the affected ones by my changes: Then I ran them all including the functional Thanks again for working on this! I'm seeing if I can make the code a bit clearer otherwise I'll include your suggestion. |
5089e20 to
0486cba
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
5f0ff21 to
f6f44df
Compare
|
-Updates:
|
w0xlt
left a comment
There was a problem hiding this comment.
ArgsManager now accepts options after non-option arguments, but bitcoin-tx still walked raw argv afterward.
That means any option placed after the tx input could be parsed as an option and then incorrectly processed again as a transaction mutation command.
Suggested changes:
diff
diff --git a/src/bitcoin-tx.cpp b/src/bitcoin-tx.cpp
index f21db40f3c..7f94598d39 100644
--- a/src/bitcoin-tx.cpp
+++ b/src/bitcoin-tx.cpp
@@ -797,35 +797,42 @@ static int CommandLineRawTx(int argc, char* argv[])
std::string strPrint;
int nRet = 0;
try {
- // Skip switches; Permit common stdin convention "-"
- while (argc > 1 && IsSwitchChar(argv[1][0]) &&
- (argv[1][1] != 0)) {
- argc--;
- argv++;
+ std::vector<std::string> args;
+ if (const auto command{gArgs.GetCommand()}) {
+ args = command->args;
+ } else {
+ // Skip switches; Permit common stdin convention "-"
+ while (argc > 1 && IsSwitchChar(argv[1][0]) &&
+ (argv[1][1] != 0)) {
+ argc--;
+ argv++;
+ }
+ args.assign(argv + 1, argv + argc);
}
CMutableTransaction tx;
- int startArg;
+ size_t startArg;
if (!fCreateBlank) {
// require at least one param
- if (argc < 2)
+ if (args.empty()) {
throw std::runtime_error("too few parameters");
+ }
// param: hex-encoded bitcoin transaction
- std::string strHexTx(argv[1]);
+ std::string strHexTx(args[0]);
if (strHexTx == "-") // "-" implies standard input
strHexTx = readStdin();
if (!DecodeHexTx(tx, strHexTx, true))
throw std::runtime_error("invalid transaction encoding");
- startArg = 2;
- } else
startArg = 1;
+ } else
+ startArg = 0;
- for (int i = startArg; i < argc; i++) {
- std::string arg = argv[i];
+ for (size_t i = startArg; i < args.size(); i++) {
+ std::string arg = args[i];
std::string key, value;
size_t eqpos = arg.find('=');
if (eqpos == std::string::npos)
diff --git a/test/functional/data/util/bitcoin-util-test.json b/test/functional/data/util/bitcoin-util-test.json
index 896decc4e3..03606149bf 100644
--- a/test/functional/data/util/bitcoin-util-test.json
+++ b/test/functional/data/util/bitcoin-util-test.json
@@ -56,6 +56,11 @@
"output_cmp": "blanktxv2.json",
"description": "Creates a blank transaction when nothing is piped into bitcoin-tx (output in json)"
},
+ { "exec": "./bitcoin-tx",
+ "args": ["01000000000000000000", "-json"],
+ "output_cmp": "blanktxv1.json",
+ "description": "Parses an option after a transaction hex"
+ },
{ "exec": "./bitcoin-tx",
"args": ["-create", "nversion=+1"],
"return_code": 1,
w0xlt
left a comment
There was a problem hiding this comment.
The test can be improved by parsing --datadir=... with a fresh ArgsManager instead of using ForceSetArg(), so it exercises the real command-line path.
diff
diff --git a/src/test/argsman_tests.cpp b/src/test/argsman_tests.cpp
index cab21ad4aa..1b5cf4d862 100644
--- a/src/test/argsman_tests.cpp
+++ b/src/test/argsman_tests.cpp
@@ -36,9 +36,16 @@ BOOST_AUTO_TEST_CASE(util_datadir)
args.ClearPathCache();
BOOST_CHECK_EQUAL(dd_norm, args.GetDataDirBase());
- args.ForceSetArg("--datadir", fs::PathToString(dd_norm) + "/");
- args.ClearPathCache();
- BOOST_CHECK_EQUAL(dd_norm, args.GetDataDirBase());
+ {
+ ArgsManager parsed_args;
+ parsed_args.AddArg("-datadir=<dir>", "", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::OPTIONS);
+ const std::string datadir_arg{"--datadir=" + fs::PathToString(dd_norm) + "/"};
+ const char* argv_datadir[] = {"ignored", datadir_arg.c_str()};
+ std::string error;
+ BOOST_CHECK(parsed_args.ParseParameters(2, argv_datadir, error));
+ BOOST_CHECK_EQUAL(error, "");
+ BOOST_CHECK_EQUAL(dd_norm, parsed_args.GetDataDirBase());
+ }
args.ForceSetArg("-datadir", fs::PathToString(dd_norm) + "/.");
args.ClearPathCache();
Thanks, it makes sense. I agree |
I agree, better to have it parsed, I'll update the test as well. Thanks! |
c5baf88 to
7959cde
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
|
-Updates:
|
7959cde to
751d83f
Compare
Previously, ArgsManager stopped interpreting options once a
non-option argument was encountered (non-GNU parsing).
For example:
bitcoind -a -b param -c
would ignore `-c` because it followed the non-option argument
`param`. In cases where some 'param' command receives parameters,
'-c' is interpreted as one of them producing unexpected behavior
(e.g. empty arrays for listtransactions command).
This commit changes the behavior so that options following
non-option argument are recognized and processed (GNU parsing).
This is needed for workflows for tools like CLI where command
arguments may be followed by wallet- or RPC-specific options, e.g.:
bitcoin-cli listtransactions -rpcwallet=mywallet
bitcoin-cli getaddressinfo <address> -rpcwallet=mywallet
bitcoin-wallet create -wallet=newname
Also, invalid options passed after non-options will be treated
as RPC arguments that begin with a '-' (dash) character (so no
behavioural change against master), e.g.:
bitcoin-cli -regtest -datadir=/tmp/btc createwallet -mywallet
{
"name": "-mywallet"
}
Refactor bitcoin-cli to use command arguments provided by ArgsManager instead of manually slicing argv. - Introduce GetCommandArgs() helper. - Remove local argv/argc skipping logic. - Simplify CommandLineRPC argument collection. This improves consistency in bitcoin-cli, avoiding duplicating parsing logic. Added a new functional test passing an option after a non-option argument (GNU-style). No behavior change intended.
Treat options that begin wih double dashes as RPC named params: bitcoin-cli createwallet mywallet --load_on_startup=true as an equivalent to: bitcoin-cli -named createwallet mywallet load_on_startup=true
Improve handling of long (`--`) options passed to bitcoin-cli, such as
`--datadir` and `--signet`. Previously, some code paths in CLI didn't
properly recognize or forward double-dash options, causing unexpected
argument parsing behavior or timeouts.
This change ensures that long options are processed consistently,
matching ArgsManager behavior and CLI semantics.
Example fixed case:
/build/bin/bitcoin-cli --datadir=/tmp/btc1 --signet getblockhash 1000
Co-authored-by: w0xlt <w0xlt@users.noreply.github.com>
No behavior changed.
No behavior changed.
Allow command-specific options using '/' prefixes on Windows after a command has been encountered. Add a regression test covering '/D=windows' after a command. Co-authored-by: w0xlt <w0xlt@users.noreply.github.com>
Use the command arguments already parsed by ArgsManager instead of re-processing raw argv. This ensures options specified after non-option arguments are handled consistently. Co-authored-by: w0xlt <w0xlt@users.noreply.github.com>
751d83f to
e47c722
Compare
|
-Updates:
|
…ategories 32df86f argsman: Prevent duplicate option registration across categories (Pablo Martin) Pull request description: Follow-up to #28802 This PR enforces the invariant that option names are unique across categories. -<ins>_**Rationale**_</ins>: - While adapting the `argsman_tests.cpp` cases introduced in #28802 for the GNU-style parsing changes proposed in #33540, I noticed that the same option name can currently be registered in multiple categories. - At present in `master`, this ambiguity is largely masked by the existing command-line parsing behaviour. However, it relies on assumptions about how options are interpreted based on their position. Future changes to option parsing, such as the GNU-style parsing proposed in #33540, may expose this ambiguity and lead to unexpected option resolution. - To avoid ambiguous option resolution and make the distinction between global and command-specific options explicit, this PR adds validation in `AddArg()` preventing the same option name from being registered across different categories. ACKs for top commit: sedited: ACK 32df86f ryanofsky: Code review ACK 32df86f. Thanks for the simplifications! Tree-SHA512: 5d3cb951bd90c46cbd8205a229d49336c3a29e5100b7c50c5cba66979dd3cbd480ed42593caa272c6205eb7ac503af631a7e3531af0b726334e3448e17d5c0d5
This PR simplifies the argument parsing logic and resolves practical issues for
bitcoin-cliand similar binaries (e.g.bitcoin-wallet) making their usage more consistent and predictable, enabling valid patterns such as mixing commands with wallet- or RPC-specific options—without relying on argument ordering quirks.What's changed in ArgsManager that allows the above.
Previously,
ArgsManagerstopped interpreting options once a non-option argument was encountered (non-GNU parsing). For example:would ignore
-cbecause it followed the non-option argumentparam. In some cases '-c' is interpreted as an argument for 'param', producing unexpected behavior (eg empty arrays for listtransactions command).This PR changes the behavior so that options following non-option argument are recognized and processed (GNU parsing). This is needed for workflows for tools like CLI where command arguments may be followed by wallet- or RPC-specific
options, e.g.:
(other binaries could also benefit from this change, e.g.
bitcoin-wallet)Invalid options passed after non-options will be treated as RPC arguments that begin with a
-character (so nobehavioural change against
master), e.g.:Before and after outputs running RPC commands from
CLI/bitcoin-cli.wallet commands using
CLI:getaddressinfomasterlisttransactionsmasterany command that needs an option to be specifed, it can be added at the end directly instead of before the command
masterother binaries could benefit from it, e.g.
bitcoin-wallet:createcommandmaster(
-walletoption has to be passed before thecreatecommand in order to work)-Notes:
Backwards compatibility:
This PR is backward compatible in the sense that it allows new valid invocations that were previously rejected (failed or somehow ignored).
In
master, any valid option following a command is treated as a positional argument. This can lead to unexpected RPC errors or help messages due to mismatched argument types. With this PR, those cases are parsed as proper options when applicable.bitcoin-clicontinues to accept RPC arguments that begin with a-character as currently inmaster(checkcreatewalletexample in "What's change in ArgsManager" section above).bitcoin-clicould distinguish between options that begin with single and double dashes, and treat options after the RPC method name that begin with double dashes as RPC named parameters (check example in 4th commit message body).Refactored both
ArgsManager::ProcessOptionKey(new function added in this PR) andArgsManager::ParseParametersin separated commits making the code clearer and the PR much easier to follow and review.On Windows, command-specific options using the '/' prefix are now handled correctly when specified after a command.
Updated
bitcoin-txto consume the command arguments already parsed byArgsManager, avoiding re-processing rawargvinCommandLineRawTx().Additionally, this PR includes release notes documenting the GNU-style option parsing behaviour and the distinction between command-line options and RPC named parameters.