Sanity-check command line parameters format

kostaz commented at 2:48 pm on May 17, 2014: contributor

Sanity-check command line parameters format 6a248d5158

BitcoinPullTester commented at 3:12 pm on May 17, 2014: none

Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/6a248d515831109fc3044f6901a548cf385a592a for binaries and test log.

This could happen for one of several reasons:

It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts (please tweak those patches in qa/pull-tester)
It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
It does not build on either Linux i386 or Win32 (via MinGW cross compile)
The test suite fails on either Linux i386 or Win32
The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)

If you believe this to be in error, please ping BlueMatt on freenode or TheBlueMatt here.

This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

laanwj commented at 2:00 pm on May 19, 2014: member

What kind of command-line mistakes is this trying to protect against? Can we avoid needing to import boost::regex just for this?

sipa commented at 2:41 pm on May 19, 2014: member

I’ve for a long time been wanting to write a simple utility module that would allow command-line options to be written as global objects (initialized with defaults, range information, description), that can be used as variables (and queried for being set or not), and with extra methods to request a list of all, … to build –help output. It would make things like this trivial.

laanwj commented at 8:06 am on May 21, 2014: member

@sipa I agree with that approach. Though we also have to take care not to overdesign, the current argument handling is really ad-hoc and error prone (and also not thread-safe I think). A new solution should also make it possible to query the currently effective values of all settings. That would be a useful RPC.

(Though, if you spend time coding on bitcoin pleease work on the headers first stuff :-) Someone else can handle this)

kostaz commented at 10:35 am on May 22, 2014: contributor

This regex fix was just ad-hoc to fix a bug when I used (unintentionally) wrong parameters. I did saw that the whole parameter handling code is the ad-hoc code, but I just wanted to contribute my penny into the development effort. What do you mean by “headers first stuff”?

leofidus commented at 3:11 pm on May 22, 2014: none

@kostaz I think the headers first part was still directed at sipa. He coded a more efficient block download algorithm that works by downloading the headers of all blocks before downloading the blocks. That allows to download from multiple peers simultaniously and allows for much smarter block download in general. He’s submitting that piece for piece because it is so much code to review.

sipa commented at 3:55 pm on May 23, 2014: member

Yeah, ignore that comment.

Is there any reason not to immediately die if an unparseable command-line options appears?

kostaz commented at 4:35 pm on May 28, 2014: contributor

IMHO, it is better to die immediately. It is much worse to run with wrong parameters and, potentially, corrupt anything. Do you think I should update it?

laanwj commented at 4:53 pm on May 28, 2014: member

@kostaz I agree that more thorough argument checking would be useful, but I don’t think your current regex-based check is useful. It doesn’t protect against the most common errors which seem to be:

Mistyped parameter names
Invalid values for parameters
Using -parameter value instead of -parameter=value

kostaz commented at 8:57 pm on May 28, 2014: contributor

Fully agree. It was just a quick fix. I’d like to redo it in a proper/fuller way. Is there anybody working on this stuff already? Just trying to avoid effort duplication…

laanwj commented at 6:22 am on May 29, 2014: member

I don’t think anyone is working on it. See also #1044 for previous discussion. It would be a larger change that would need a refactoring of the option system.

Currently the complete list of options, let alone how their values are to be parsed, is not specified in advance. A possible design would be to use some table-based structure like CRPCCommand in rpcserver.cpp, with ‘actors’ that parse/verify the values. The GUI, CLI and daemon then register their own lists of options before the parsing starts.

There are quite some challenges though;

Handling option defaults is non-trivial. Some option settings affect other options defaults (in AppInit2).
For the GUI there is a third options source: the QSettings. Both command line options and bitcoind.conf options take precedence over this.
-conf, -datadir, -testnet/-regtest are difficult to handle properly (as it is possible to specify datadir= in the config file, as well as testnet, which changes the datadir which is respectively cached…), there have been many commits in the past reverting unwanted behavior after changes to argument handling: c61fe44, c52c4e5 (pull #3935, #3844)

It would be best to make a test framework first, so we can be more sure that there are no regressions.

sipa commented at 11:09 am on May 29, 2014: member

Based on some experience with another config framework, I would suggest something like this:

Have data types CIntOption, CStringOption, … that on construction take a name, a description, a default value.
Constructing them automatically registers them in a table of known options.
Internally they retain their value, and whether or not they were explicitly set.
They can be implicitly cast to their “value” (so a variable of type CIntOption can be used as an int).
They have a Set and SetDefault method, which take a string and convert it to their native data type (which can fail). SetDefault does nothing in case the option is already explicitly set (perhaps by a previous SetDefault).
There’s a lookup function that finds options with arbitrary names at runtime, by looking them up in the table.

This means you can replace existing definitions like

0int nTransactionFee = DEFAULT_TRANSACTION_FEE;

to

0CIntOption nTransactionFee("paytxfee", DEFAULT_TRANSACTION_FEE, "The relay fee used, blah blah")

The startup sequence can just be:

Iterate over command-line options, look the options up, and apply SetDefault to them.
Open the config file (whose location may have been influenced by the command-line options), look them up, and apply SetDefault to them.
Run custom logic for defaults, looking values of some options, and use it to set defaults for others (assuming they haven’t been set yet).

I believe this should be safe, as the explicit set/default flag in each option instance corresponds to the current logic associated with an option being present in mapArgs/mapMultiArgs, and the overriding of defaults would work similarly.

laanwj commented at 1:41 pm on June 3, 2014: member

Closing this pull as a larger redesign is needed to improve argument error reporting than this specific implementation. I’ve referred to the discussion in this pull from #1044 because of the useful information in case anyone is going to redesign the argument handling.

laanwj closed this on Jun 3, 2014

DrahtBot locked this on Sep 8, 2021

Sanity-check command line parameters format #4194