Should warn on unrecognised command line options

[rnicoll]

If unrecognised command line options are provided, we should at least warn the user so they know they're not going to be applied.

[ReverseControl]

@rnicoll Was this ever addressed? It appears it hasn't, at least up to 1.14.4.

[slightlyskepticalpotat]

It looks like bitcoin core implemented this functionality in https://github.com/bitcoin/bitcoin/commit/4f8704d57f8fb2958a43534779b20201b77eecae with v0.17.0rc1, and litecoin core ported it over at https://github.com/litecoin-project/litecoin/commit/4f8704d57f8fb2958a43534779b20201b77eecae.

[ReverseControl]

@patricklodder We should include this in 1.14.5.

[patricklodder]

👍 Re-milestoned it & added to 1.14.5 project.

[chromatic]

Bitcoin core started down this path with an ArgsManager class, though I see references later in the code that a single shared gArgs object made certain kinds of testing and fuzzing awkward.

Refactoring argument handling to a single manager class seems like a good first step though.

I'll start on this and see where it goes.

[patricklodder]

@rnicoll Could you elaborate on where/how is this implemented?

[rnicoll]

@patricklodder Oops I'd missed this had activity on it, I'd thought it was in 1.14.5 by accident. Re-opened!

[patricklodder]

moved to 1.14.6

[ZachOlivier]

Refactoring argument handling to a single manager class seems like a good first step though.

I'll start on this and see where it goes.

@chromatic did you make any progress on this? I don't see a branch associated with this issue.

[jiamsu]

哈哈啊哈

[envisiondata]

Is this issue still open to be worked on? I would like to implement it.

[patricklodder]

@envisiondata - It definitely still is open - we have no open Pull Request right now. I think it would be good to discuss with @chromatic what was done/found on their end before putting in weeks of work.

[chromatic]

I've only done research on this; I haven't started porting, cherry-picking, or rewriting code.

Based on my findings, the ArgsManager approach Bitcoin took seems like a good and workable design. This gets complicated because there are two phases to Bitcoin's design:

• the initial introduction of the class and pattern
• a subsequent revision to make the argument manager instance a global

We can pull in the former without too much effort, but the latter is spread across multiple releases that would span our 1.14.x series and our 1.21.x series, so there's overlap with any other code porting or cherry-picking as we develop both families.

I think that leaves a few possibilities:

• start this in 1.14.x and follow the Bitcoin approach, with the testing drawbacks of the singleton
• use the Bitcoin approach as a guide but port both phases as once
• start this in 1.21.x and get both phases at once

To me, the biggest risk of any approach is making other code more difficult. Argument handling shows up in lots of places, and I foresee some complicated merge conflicts around this behavior. Targeting 1.21 seems like it will reduce that risk.

I don't hold these opinions too strongly, however, because I stopped at reading code and didn't try porting or cherry-picking.

[envisiondata]

@chromatic @patricklodder If you want to wait till 1.21.x that is fine. I looked at the PR from bitcoin and I figured this enhancement would give me the most exposure to the code base.

I am new here and would like something to work on to learn the system. Any suggestions?

[patricklodder]

@envisiondata the risk of starting with a high-impact PR that exposes you to everything is that it will expose the entire codebase to you too. It will also be extremely hard to review, intensive to test, and you risk getting stuck. Smart and capable people have become stuck and demotivated on much smaller exposure that still brought tons of problems.

If you want to start learning the codebase, I'd suggest you start small: the cleanest issue, with a solution suggestion is probably #3041 for bug fixing. If you're looking more to build an enhancement, implementing (part of) #2908 would be suitable.

[patricklodder]

there are two phases to Bitcoin's design

Is it possible at all to shortcut and for 1.14.7 backport the end result - or even re-implement the same concept of Bitcoin's end result - rather than going through the motions of multiple iterations?

[chromatic]

Is it possible at all to shortcut and for 1.14.7 backport the end result - or even re-implement the same concept of Bitcoin's end result - rather than going through the motions of multiple iterations?

Yes, that's the right way to do it.

The risk is that a lot of the changes are tied to other feature work, so disentangling only the argument handling will be some effort and any subsequent backporting or cherry-picking of the unrelated feature work is going to make rebasing difficult.

I could be wrong; if someone wants to start the process for 1.14.7, we can evaluate the results while they're underway. For me personally, getting 1.14.6 and 1.21 shipped seemed more valuable than solving this for 1.14.6 (or, now, 1.14.7).

[patricklodder]

I do think that we can do it in chunks as long as we have a good basis to work off:

1. Introduce the arg manager in a technically correct format (i.e. at least as awesome as Bitcoin Core's current implementation), skip the warning part, just log to a debug descriptor. Take a small set of expert-only args for a first integration.
2. Iterate through the params and integrate, but do it in small batches. Maintain a qa test for every newly integrated arg (this will also help us identify breaking changes in config and command line args cross-version), merge whatever is ready and of high quality. If we need to do a release, it's no problem.
3. When we're ready, we turn on "grumble mode" and warn/disallow unrecognized args. If it takes 3 releases to be ready, it is fine.

What do you think?

[chromatic]

Yes, a piecemeal approach across releases would reduce a lot of risk. We'll need a careful design that can support the current approach (functions working on global data) and the desired approach (methods on a semi-global instance of a class) simultaneously, and we'll still have to be cautious about merge/rebase/cherry-pick conflicts.

For point 3, let's call it "grumble mode" and put that in the scaffolding too. We could use that to figure out the options we haven't yet ported to the new design, both "I've processed this CLI option I don't know about" and "Code somewhere has used the old mechanism for accessing CLI options".

[patricklodder]

_Originally posted by @chromatic in https://github.com/dogecoin/dogecoin/pull/3466#discussion_r1545864357_

[..] we'll want to remember we once supported this [-enable-bip70] flag when we move to 1.21 or reimplement/cherry-pick the code that checks for unsupported CLI arguments.