refactor: Remove redundant and confusing calls to IsArgSet #31896

1. 
   maflcko commented at 3:25 pm on February 18, 2025: member

   IsArgSet is problematic:

   • It returns whether an arg has been set, even if it has been negated. IsArgSet is sometimes used to check for a truthy value, which is wrong, but usually harmless. Cleanup of those cases may or may not be done in a follow-up.
   • In most other cases, calling it is redundant, because the immediately following Get*Arg calls can already return an std::optional nullopt value to indicate an unset arg.

   So relieve both issues by removing all IsArgSet that are redundant.

2. refactor: Remove IsArgSet guard when fallback value is provided

   Checking for IsArgSet before calling GetArg while providing the args
   default value as fallback is both confusing and fragile.
   
   It is confusing, because the provided fallback is dead code. So it would
   be better to just call GetArg without a fallback.
   
   However, ignoring the fallback value is fragile, because it would not be
   sanitized.
   
   Fix all issues by sanitizing the fallback value.

   fa41685f43
3. refactor: Remove redundant call to IsArgSet

   Checking for IsArgSet before calling GetArg while providing an arbitrary
   default value as fallback is both confusing and fragile.
   
   It is confusing, because the provided fallback is dead code. So it would
   be better to just call GetArg without a fallback.
   
   Even better would be to provide the true fallback value and sanitize it
   as if it were user-input, but this can be done in a follow-up.
   
   Removing the redundant call to IsArgSet will have to be done either way,
   so do it now.

   fa3fb3c23f
4. 
   DrahtBot commented at 3:25 pm on February 18, 2025: 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/31896.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	ryanofsky
   Concept ACK	yancyribbens

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #31260 (scripted-diff: Type-safe settings retrieval by ryanofsky)
   • #29278 (Wallet: Add maxfeerate wallet startup option by ismaelsadeeq)

   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.

5. DrahtBot added the label Refactoring on Feb 18, 2025
6. ryanofsky approved
7. 
   ryanofsky commented at 9:13 pm on February 18, 2025: contributor

   Code review ACK fa3fb3c23fae287b161112b9b04cf0937a1051c6. Nice refactoring to get rid of pointless complexity and repetition in this code.
8. 
   yancyribbens commented at 10:34 pm on February 18, 2025: contributor

   It is confusing, because the provided fallback is dead code. So it would be better to just call GetArg without a fallback.

   I’m not 100% sure when reading the commit what is meant by dead code. Is it that the arg -blockmintxfee is a no longer used switch here? I ask because I notice some other calls to GetArg with fallbacks such as:

   0src/init.cpp:879:    if (args.IsArgSet("-upnp")) {
   

9. in src/init.cpp:1014 in fa3fb3c23f

   1011@@ -1012,20 +1012,20 @@ bool AppInitParameterInteraction(const ArgsManager& args)
   1012 
   1013     // Sanity check argument for min fee for including tx in block
   1014     // TODO: Harmonize which arguments need sanity checking and where that happens
   

   ---

   ---

   

   yancyribbens commented at 10:35 pm on February 18, 2025:

   Is this TODO still accurate? Not sure what arguments need Harmonizing.

   ---

   

   maflcko commented at 7:43 am on February 19, 2025:

   Not sure. It is not immediately actionable and probably opinionated or unclear, so best to remove. Though, I don’t want to do it here. Maybe a follow-up pull could do it?

   ---

   

   yancyribbens commented at 3:26 pm on February 19, 2025:

   I agree the TODO isn’t very clear and possibly no longer applicable. Maybe it could be rounded up with other unclear comments and TODOs in a separate PR. A PR to just remove this one TODO line seems silly.
10. 
    yancyribbens commented at 10:37 pm on February 18, 2025: contributor

    Looks like a worthwhile refactor. Better than an LLM could do ;)
11. 
    ryanofsky commented at 11:40 pm on February 18, 2025: contributor

    It is confusing, because the provided fallback is dead code. So it would be better to just call GetArg without a fallback.

    I’m not 100% sure when reading the commit what is meant by dead code.

    It is just saying the fallback argument is unused. For example in:

    0   if (gArgs.IsArgSet("-color")) {
    1   {
    2        const std::string color{gArgs.GetArg("-color", DEFAULT_COLOR_SETTING)};
    3        ...
    4   }
    

    The DEFAULT_COLOR_SETTING argument is unused, and any fallback value that is passed would be ignored because IsArgSet returned true.

12. 
    yancyribbens commented at 1:37 am on February 19, 2025: contributor

    Concept ACK https://github.com/bitcoin/bitcoin/pull/31896/commits/fa3fb3c23fae287b161112b9b04cf0937a1051c6

Contributors
maflcko DrahtBot ryanofsky yancyribbens

Labels
Refactoring