Add better usage information for -discover.

[paveljanik]

The usage description (--help) for -discover didn't mention an argument.

[laanwj]

ACK for 0.9 and master

[BitcoinPullTester]

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/8af4bf5f2bfd916fc05dd21c15eb25a0739de329 for binaries and test log.
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.

[cozz]

From my understanding "-discover" is a boolean argument. In this case the proposed change would not make sense.
=<n> is meant to be for parameters that take a numeric value.

Though I agree that -discover=0 to disable a boolean option is a little bit confusing. So maybe we should avoid boolean options with default=1 in general, and always provide the opposite: "-nodiscover" in this case. We already have implemented InterpretNegativeSetting in util.cpp to actually support "-nodiscover". So I suggest considering this as a general issue with boolean arguments defaulting to 1 and rework them in general in a separate pull request.

[laanwj]

Booleans are also numeric values in our case, though, just restricted to 0 and 1.

Edit: It's possible to use -option=0 or -option=1 as well as -nooption and -option. These are equivalent and both can make sense in some settings, for example in the configuration file option=0 is slightly clearer.

[paveljanik]

I was tempted to put there, but there was no other option using it.

[laanwj]

Though @cozz is right in that the other boolean-valued options don't have a =<n> either in the documentation. Not sure how to word this better in the defaults.

[cozz]

I would be fine with this "-discover=<0|1>"
<n> means any numeric value, not only zero and one.

In any case all the other boolean options have to be changed as well, not only "-discover".

[paveljanik]

If the default is true/1, isn't it better to document only the "no" variant?

[sipa]

Or just list the opposite of the default.

If the default is discover=1, list -nodiscover as an option. If the default is discover=0, list -discover ?

[paveljanik]

Ie.
-nodiscover Disable own IP address discovery (default is to discover when listening and no -externalip)

[laanwj]

Reasonable idea but the default is not always straightforward. For some options it depends on other options (ie "default: 1 if listening", "default: 1 unless -connect"), or compile-time settings.

[paveljanik]

OK, I spend a bit of time on this and I found out, that the concept of external IP address discovery is only applicable in very special cases (why it was added at all? You can listen on all local IPs or selected IP specified with -externalip).

If you are on a masqueraded network, behind transparent proxy, have more external IPs/your outgoing IP is different than the IP you want it to autodiscover etc. In theses cases, you asks for your external IP, make a connection to the network (if allowed by your firewall at all!?) to some external services which are not controlled by us anyway.

There is a comment like this in net.cpp:

    // We should be phasing out our use of sites like these. If we need
    // replacements, we should ask for volunteers to put this simple
    // php file on their web server that prints the client IP:
    //  <?php echo $_SERVER["REMOTE_ADDR"]; ?>

I think the whole concept of external IP discovery should be removed. One thread less, code simplified, better security (no external connection, plain HTTP can be MiTM easily, bad IP could be returned/daemon not starting or listening on other IP/DoS).

But for now, I'd go with cozz's <0|1> which is better than generic n, I think.

[gmaxwell]

Funny that you think discovery is "very special cases" when it's likely the majority of hosts. Discovery doesn't just cover the external IP service, it also covers UPNP discovery (which also covers the firewall cases). There are incomplete patches floating around which eliminate the service usage and instead use other Bitcoin nodes to accomplish discovery.

I have no opinion on the parameter description nitpicking. I'm just commenting to clear up confusion about what the behavior is used for...

[paveljanik]

Looks like I forgot about people who are on the wide open network behind wide open UPnP-capable router which allows anyone to tunnel anything into the internal network.

Ups, yes.

I was talking only about the case when you do not have upnp libs/router (but even in this case, most of the things still apply - but in such case, we can't talk about security at all anyway ;-).

[laanwj]

@paveljanik See #3088 and rebased version #3461 (needs rebase again).

[laanwj]

Closing this pull for now. None of the bool options have an argument in the help, changing this for one option doesn't make it clearer. To change this, it should be changed consistently.