argsman: Prevent duplicate option registration across categories #35470

pull pablomartin4btc wants to merge 1 commits into bitcoin:master from pablomartin4btc:argsman-prevent-option-duplication-acroos-categories changing 1 files +5 −0

pablomartin4btc commented at 8:40 PM on June 5, 2026: member
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.
DrahtBot commented at 8:40 PM on June 5, 2026: contributor



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35470.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK sedited, ryanofsky

Concept ACK w0xlt

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.
sedited commented at 7:37 AM on June 6, 2026: contributor

Concept ACK
sedited requested review from ryanofsky on Jun 9, 2026
w0xlt commented at 8:08 AM on June 14, 2026: contributor

Concept ACK
in src/common/args.cpp:673 in 568afb4390
```
 667 | @@ -668,6 +668,11 @@ void ArgsManager::AddArg(const std::string& name, const std::string& help, unsig
 668 |      std::string arg_name = name.substr(0, eq_index);
 669 |  
 670 |      LOCK(cs_args);
 671 | +
 672 | +    for (const auto& [existing_cat, available_args] : m_available_args) {
 673 | +        Assert(existing_cat == cat || !available_args.contains(arg_name));
```
ryanofsky commented at 10:10 PM on June 15, 2026:

In commit "argsman: Prevent duplicate option registration across categories" (568afb4390eecac9bd8c344d22dce4069a147b98)

I think this existing_cat == cat || condition is unnecessarily confusing and should be removed. It seems like it should be an error to register the same argument more than once regardless of category. Removing this condition should not change behavior because there will already be an error in assert(ret.second) below if the argument is registered in the same category.

Possible I'm missing something though, and if so, it might be good to add a little comment explaining what this code is checking for and why.

pablomartin4btc commented at 2:01 AM on June 16, 2026:

You are correct and I do agree, the existing_cat == cat || condition is unnecessary. The existing assert(ret.second) would already catch same-category duplicates, but checking all categories directly makes the invariant clearer: the option name must not already be registered anywhere before insertion. I’ll simplify it with your suggestion. Thank you!
ryanofsky approved
ryanofsky commented at 10:11 PM on June 15, 2026: contributor

Code review ACK 568afb4390eecac9bd8c344d22dce4069a147b98, but did have a question / suggestion below
DrahtBot requested review from sedited on Jun 15, 2026
pablomartin4btc force-pushed on Jun 16, 2026
pablomartin4btc commented at 2:09 AM on June 16, 2026: member
-<ins>Updates</ins>:
- Simplified the invariant enforcing unique option names across categories, addressing @ryanofsky's feedback.
in src/common/args.cpp:672 in 6b3e8fc295
```
 667 | @@ -668,6 +668,11 @@ void ArgsManager::AddArg(const std::string& name, const std::string& help, unsig
 668 |      std::string arg_name = name.substr(0, eq_index);
 669 |  
 670 |      LOCK(cs_args);
 671 | +
 672 | +    for (const auto& [existing_cat, available_args] : m_available_args) {
```
ryanofsky commented at 7:02 PM on June 16, 2026:

In commit "argsman: Prevent duplicate option registration across categories" (6b3e8fc29543ef6c3112286da7547011ac70b61a)

Not important but loop variable names seem a little off. existing_ prefix seems redundant and available_args uses the name of the container to refer to the element. Might suggest [category, category_args] instead

pablomartin4btc commented at 7:12 PM on June 16, 2026:

Yeah, I'll remove it. Thanks!
ryanofsky approved
ryanofsky commented at 7:05 PM on June 16, 2026: contributor

Code review ACK 6b3e8fc29543ef6c3112286da7547011ac70b61a just simplifying assert since last review

Type	Reviewers
ACK	sedited, ryanofsky
Concept ACK	w0xlt

argsman: Prevent duplicate option registration across categories

Added a validation in AddArg() preventing the same option name from
being registered across different categories, avoiding ambiguous option
resolution and make the distinction between global and command-specific
options explicit.

32df86f1d8

pablomartin4btc force-pushed on Jun 16, 2026
pablomartin4btc commented at 7:33 PM on June 16, 2026: member
-<ins>Updates</ins>:
- Removed the now-unused category loop variable noticed while addressing @ryanoskys' latest feedback.
sedited approved
sedited commented at 7:57 AM on June 17, 2026: contributor

ACK 32df86f1d80dc88bdde96e03ff021a196b4b4ea3
DrahtBot requested review from ryanofsky on Jun 17, 2026
ryanofsky approved
ryanofsky commented at 12:25 PM on June 17, 2026: contributor

Code review ACK 32df86f1d80dc88bdde96e03ff021a196b4b4ea3. Thanks for the simplifications!
fanquake merged this on Jun 17, 2026
fanquake closed this on Jun 17, 2026

Contributors

Linked (view graph)

#28802 ArgsManager: support command-specific options #33540 argsman, cli: GNU-style command-line option parsing (allows options after non-option arguments)#35549 argsman: Fix duplicate option assertion to allow HIDDEN category registration

argsman: Prevent duplicate option registration across categories #35470

Code Coverage & Benchmarks

Reviews