doc: Change documentation for =0 for non-boolean options #14100

laanwj · 2018-08-29T09:12:00Z

PR #12713 changed the interpretation for negation of non-boolean options (e.g. -noconnect) to no longer set the option to 0, but to remove it from the options.

I think this is better because it gets rid of the special meaning of '0'.

However it needs to be documented. I attempt to do so in this PR.
Addresses #14064.

There might be options I missed, please help check:

-connect
-proxy
-onion
-debug
-debuglogfile

Needs a manpage update.

laanwj · 2018-08-29T09:19:57Z

~~Wait, I think I'm misunderstanding this,~~ -connect does still check for "0" explicitly:

        if (connect.size() != 1 || connect[0] != "0") {
            connOptions.m_specified_outgoing = connect;
        }

Is this only for backwards compatibility?
I checked that at least both -noconnect and -connect=0 still work.

Whereas -debuglogfile uses IsArgNegated:

g_logger->m_print_to_file = !gArgs.IsArgNegated("-debuglogfile");

Tested: -nodebuglogfile works, but -debuglogfile=0 doesn't. This is the only argument that uses IsArgNegated.

(all in all, this change is still correct, I think)

DrahtBot · 2018-08-29T12:09:24Z

Note to reviewers: This pull request conflicts with the following ones:

Add p2p layer encryption with ECDH/ChaCha20Poly1305 #14032 (Add p2p layer encryption with ECDH/ChaCha20Poly1305 by jonasschnelli)
Explain that mempool memory is added to -dbcache #13676 (Explain that mempool memory is added to -dbcache during IBD by Sjors)
Add fee_est tool for debugging fee estimation code #10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)

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.

practicalswift · 2018-08-29T15:20:18Z

ACK 9e7ddf97d581b5d7e04193567c60cb4e0cc19401

maflcko · 2018-08-29T19:00:31Z

For all options that are not a file, the existing way of -opt=0 should not be deprecated. You don't deprecate these here, just mention the other alternative for consistency with -nodebuglogfile, so I guess it it fine.

utACK 9e7ddf97d581b5d7e04193567c60cb4e0cc19401

maflcko · 2018-08-29T19:01:21Z

Note the related commit d5690f1

maflcko · 2018-08-29T19:03:08Z

Note that manpages would have to be regenerated on 0.17 after backport

laanwj · 2018-08-30T10:03:18Z

Note the related commit d5690f1

AHHHHHH
okay, really, I don't know anymore
I thought I had puzzled together a consistent picture of what was the idea here
but it seems to be moving in all directions at once
but yes I forgot about #10085

-noconnect works fine and I use it a lot for internal tests. It's a little bit more consistent to not mention it explicitly in the help I guess but please don't say that things don't work if they do!

I'm even confused at my own behavior there

Edit: as for bitcoin.conf, FWIW it looks like the only way to specify "no debug log" in bitcoin.conf is by adding nodebuglogfile=1, simply nodebuglogfile doesn't work and is ignored -- would be better to throw an error that suggests the syntax to be used, it's kindof hard to guess (done in #14105)

ken2812221 · 2018-08-31T19:17:08Z

So I found a funny result of -nodebuglogfile=0, it would log to file 1.

PR bitcoin#12713 changed the interpretation for negation of non-boolean options (e.g. -noconnect) to no longer set the option to 0, but to remove it from the options. I think this is better because it gets rid of the special meaning of '0'. However it needs to be documented. I attempt to do so in this PR. Addreses bitcoin#14064.

laanwj · 2018-09-05T12:18:38Z

So I found a funny result of -nodebuglogfile=0, it would log to file 1.

That's pretty cute, though I don't think we want to document that.

Rebased.

maflcko

re-utACK with some questions

maflcko · 2018-09-05T12:31:39Z

src/init.cpp

@@ -400,7 +400,7 @@ void SetupServerArgs()
    gArgs.AddArg("-banscore=<n>", strprintf("Threshold for disconnecting misbehaving peers (default: %u)", DEFAULT_BANSCORE_THRESHOLD), false, OptionsCategory::CONNECTION);
    gArgs.AddArg("-bantime=<n>", strprintf("Number of seconds to keep misbehaving peers from reconnecting (default: %u)", DEFAULT_MISBEHAVING_BANTIME), false, OptionsCategory::CONNECTION);
    gArgs.AddArg("-bind=<addr>", "Bind to given address and always listen on it. Use [host]:port notation for IPv6", false, OptionsCategory::CONNECTION);
-    gArgs.AddArg("-connect=<ip>", "Connect only to the specified node; -connect=0 disables automatic connections (the rules for this peer are the same as for -addnode). This option can be specified multiple times to connect to multiple nodes.", false, OptionsCategory::CONNECTION);
+    gArgs.AddArg("-connect=<ip>", "Connect only to the specified node; -noconnect disables automatic connections (the rules for this peer are the same as for -addnode). This option can be specified multiple times to connect to multiple nodes.", false, OptionsCategory::CONNECTION);


Could say -noconnect or -connect=0 to document that for this option both variants are supposed to work?

nah, I'd really like to encourage only the 'proper' syntax, there's probably no point in deprecating connect=0 but still I think it's good to be consistent in encouraging use of -noX for non-boolean options, not X=0.

maflcko · 2018-09-05T12:32:27Z

src/init.cpp

@@ -414,12 +414,12 @@ void SetupServerArgs()
    gArgs.AddArg("-maxsendbuffer=<n>", strprintf("Maximum per-connection send buffer, <n>*1000 bytes (default: %u)", DEFAULT_MAXSENDBUFFER), false, OptionsCategory::CONNECTION);
    gArgs.AddArg("-maxtimeadjustment", strprintf("Maximum allowed median peer time offset adjustment. Local perspective of time may be influenced by peers forward or backward by this amount. (default: %u seconds)", DEFAULT_MAX_TIME_ADJUSTMENT), false, OptionsCategory::CONNECTION);
    gArgs.AddArg("-maxuploadtarget=<n>", strprintf("Tries to keep outbound traffic under the given target (in MiB per 24h), 0 = no limit (default: %d)", DEFAULT_MAX_UPLOAD_TARGET), false, OptionsCategory::CONNECTION);
-    gArgs.AddArg("-onion=<ip:port>", "Use separate SOCKS5 proxy to reach peers via Tor hidden services (default: -proxy)", false, OptionsCategory::CONNECTION);
+    gArgs.AddArg("-onion=<ip:port>", "Use separate SOCKS5 proxy to reach peers via Tor hidden services, set -noonion to disable (default: -proxy)", false, OptionsCategory::CONNECTION);


maflcko · 2018-09-05T12:33:26Z

src/init.cpp

    gArgs.AddArg("-onlynet=<net>", "Make outgoing connections only through network <net> (ipv4, ipv6 or onion). Incoming connections are not affected by this option. This option can be specified multiple times to allow multiple networks.", false, OptionsCategory::CONNECTION);
    gArgs.AddArg("-peerbloomfilters", strprintf("Support filtering of blocks and transaction with bloom filters (default: %u)", DEFAULT_PEERBLOOMFILTERS), false, OptionsCategory::CONNECTION);
    gArgs.AddArg("-permitbaremultisig", strprintf("Relay non-P2SH multisig (default: %u)", DEFAULT_PERMIT_BAREMULTISIG), false, OptionsCategory::CONNECTION);
    gArgs.AddArg("-port=<port>", strprintf("Listen for connections on <port> (default: %u, testnet: %u, regtest: %u)", defaultChainParams->GetDefaultPort(), testnetChainParams->GetDefaultPort(), regtestChainParams->GetDefaultPort()), false, OptionsCategory::CONNECTION);
-    gArgs.AddArg("-proxy=<ip:port>", "Connect through SOCKS5 proxy", false, OptionsCategory::CONNECTION);
+    gArgs.AddArg("-proxy=<ip:port>", "Connect through SOCKS5 proxy, set -noproxy to disable (default: disabled)", false, OptionsCategory::CONNECTION);


maflcko · 2018-09-05T12:34:04Z

src/init.cpp

@@ -467,7 +467,7 @@ void SetupServerArgs()
    gArgs.AddArg("-limitdescendantsize=<n>", strprintf("Do not accept transactions if any ancestor would have more than <n> kilobytes of in-mempool descendants (default: %u).", DEFAULT_DESCENDANT_SIZE_LIMIT), true, OptionsCategory::DEBUG_TEST);
    gArgs.AddArg("-vbparams=deployment:start:end", "Use given start/end times for specified version bits deployment (regtest-only)", true, OptionsCategory::DEBUG_TEST);
    gArgs.AddArg("-addrmantest", "Allows to test address relay on localhost", true, OptionsCategory::DEBUG_TEST);
-    gArgs.AddArg("-debug=<category>", strprintf("Output debugging information (default: %u, supplying <category> is optional)", 0) + ". " +
+    gArgs.AddArg("-debug=<category>", "Output debugging information (default: -nodebug, supplying <category> is optional). "


Could say -nodebug or -debug=0 to document that for this option both variants are supposed to work?

if the point is supposed to work then that needs a test, not documentation
I don't want to make this overly verbose
documenting one method that works is good enough from the perspective of a user !

… options e9a78e9 doc: Change documentation for =0 for non-boolean options (Wladimir J. van der Laan) Pull request description: PR bitcoin#12713 changed the interpretation for negation of non-boolean options (e.g. -noconnect) to no longer set the option to `0`, but to remove it from the options. I think this is better because it gets rid of the special meaning of `'0'`. However it needs to be documented. I attempt to do so in this PR. Addresses bitcoin#14064. There might be options I missed, please help check: - `-connect` - `-proxy` - `-onion` - `-debug` - `-debuglogfile` Needs a manpage update. Tree-SHA512: a69e63e04a6c8bf6f61a58ddc0f5ebd4b9af7a2e0ea5174e668f2e3edd31a541910a125605ca4bfccf3aca6e59267d98a66de9d1e73650f48c8a1828d315d35d

PR bitcoin#12713 changed the interpretation for negation of non-boolean options (e.g. -noconnect) to no longer set the option to 0, but to remove it from the options. I think this is better because it gets rid of the special meaning of '0'. However it needs to be documented. I attempt to do so in this PR. Addreses bitcoin#14064. Github-Pull: bitcoin#14100 Rebased-From: e9a78e9

… options 6bfee8a doc: Update v0.17.0.0 manpages (MarcoFalke) 2936dbc doc: Change documentation for =0 for non-boolean options (Wladimir J. van der Laan) Pull request description: Github-Pull: #14100 Rebased-From: e9a78e9 Includes the bumped manpages. Tree-SHA512: 73d2dadb45418882122313975c0ab0e9f58310697d996dd2edeb400ebe73b3a45f1157c8a7fe65ae1f53d9ce68a88aae7c701f3e82e0b4db4c9417b36ddfecc0

ed2332a test: Add test for config file parsing errors (MarcoFalke) a66c0f7 util: Report parse errors in configuration file (Wladimir J. van der Laan) Pull request description: Report errors while parsing the configuration file, instead of silently ignoring them. $ src/bitcoind -regtest Error reading configuration file: parse error on line 22: nodebuglogfile, if you intended to specify a negated option, use nodebuglogfile=1 instead $ src/bitcoind -regtest Error reading configuration file: parse error on line 22: sdafsdfafs $ src/bitcoind -regtest Error reading configuration file: parse error on line 24: -nodebuglogfile=1, options in the configuration file must be specified without leading - (inspired by #14100 (comment)) Tree-SHA512: d516342b65db2969edf200390994bbbda23654c648f85dcc99f9f2d217d3d59a72e0f58227be7b4746529dcfa54ba26d8188ba9f14a57c9ab00015d7283fade2

fanquake · 2018-09-06T11:02:04Z

Backported in #14152.

PR bitcoin#12713 changed the interpretation for negation of non-boolean options (e.g. -noconnect) to no longer set the option to 0, but to remove it from the options. I think this is better because it gets rid of the special meaning of '0'. However it needs to be documented. I attempt to do so in this PR. Addreses bitcoin#14064. Github-Pull: bitcoin#14100 Rebased-From: e9a78e9

Summary: ed2332aeffb071a3404be9cff8f9fb8a81a9fbfb test: Add test for config file parsing errors (MarcoFalke) a66c0f78a941968340f030911765a84219908c4d util: Report parse errors in configuration file (Wladimir J. van der Laan) Pull request description: Report errors while parsing the configuration file, instead of silently ignoring them. $ src/bitcoind -regtest Error reading configuration file: parse error on line 22: nodebuglogfile, if you intended to specify a negated option, use nodebuglogfile=1 instead $ src/bitcoind -regtest Error reading configuration file: parse error on line 22: sdafsdfafs $ src/bitcoind -regtest Error reading configuration file: parse error on line 24: -nodebuglogfile=1, options in the configuration file must be specified without leading - (inspired by bitcoin/bitcoin#14100 (comment)) Tree-SHA512: d516342b65db2969edf200390994bbbda23654c648f85dcc99f9f2d217d3d59a72e0f58227be7b4746529dcfa54ba26d8188ba9f14a57c9ab00015d7283fade2 Backport of Core PR14105 https://github.com/bitcoin/bitcoin/pull/14105/files Test Plan: ``` ninja check test_runner.py feature_config_args ``` Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D5009

… options e9a78e9 doc: Change documentation for =0 for non-boolean options (Wladimir J. van der Laan) Pull request description: PR bitcoin#12713 changed the interpretation for negation of non-boolean options (e.g. -noconnect) to no longer set the option to `0`, but to remove it from the options. I think this is better because it gets rid of the special meaning of `'0'`. However it needs to be documented. I attempt to do so in this PR. Addresses bitcoin#14064. There might be options I missed, please help check: - `-connect` - `-proxy` - `-onion` - `-debug` - `-debuglogfile` Needs a manpage update. Tree-SHA512: a69e63e04a6c8bf6f61a58ddc0f5ebd4b9af7a2e0ea5174e668f2e3edd31a541910a125605ca4bfccf3aca6e59267d98a66de9d1e73650f48c8a1828d315d35d # Conflicts: # src/init.cpp

ed2332a test: Add test for config file parsing errors (MarcoFalke) a66c0f7 util: Report parse errors in configuration file (Wladimir J. van der Laan) Pull request description: Report errors while parsing the configuration file, instead of silently ignoring them. $ src/bitcoind -regtest Error reading configuration file: parse error on line 22: nodebuglogfile, if you intended to specify a negated option, use nodebuglogfile=1 instead $ src/bitcoind -regtest Error reading configuration file: parse error on line 22: sdafsdfafs $ src/bitcoind -regtest Error reading configuration file: parse error on line 24: -nodebuglogfile=1, options in the configuration file must be specified without leading - (inspired by bitcoin#14100 (comment)) Tree-SHA512: d516342b65db2969edf200390994bbbda23654c648f85dcc99f9f2d217d3d59a72e0f58227be7b4746529dcfa54ba26d8188ba9f14a57c9ab00015d7283fade2

laanwj added Docs Backport labels Aug 29, 2018

laanwj added this to the 0.17.0 milestone Aug 29, 2018

laanwj force-pushed the 2018_08_nodoc branch from 728e8c4 to 2739cff Compare August 29, 2018 09:34

laanwj mentioned this pull request Aug 29, 2018

-debuglogfile=0 does not disable logging to a file #14064

Closed

laanwj force-pushed the 2018_08_nodoc branch from 2739cff to 9e7ddf9 Compare August 29, 2018 12:15

DrahtBot mentioned this pull request Aug 29, 2018

[doc] Add new regtest ports in man following #10825 ports reattributions #14013

Merged

laanwj added Needs backport and removed Backport labels Aug 29, 2018

DrahtBot mentioned this pull request Aug 29, 2018

Explain that mempool memory is added to -dbcache #13676

Merged

DrahtBot mentioned this pull request Aug 29, 2018

Add p2p layer encryption with ECDH/ChaCha20Poly1305 #14032

Closed

1 task

laanwj mentioned this pull request Aug 30, 2018

util: Report parse errors in configuration file #14105

Merged

DrahtBot added the Needs rebase label Sep 4, 2018

laanwj force-pushed the 2018_08_nodoc branch from 9e7ddf9 to e9a78e9 Compare September 5, 2018 12:18

maflcko reviewed Sep 5, 2018

View reviewed changes

DrahtBot removed the Needs rebase label Sep 5, 2018

maflcko merged commit e9a78e9 into bitcoin:master Sep 5, 2018

maflcko mentioned this pull request Sep 5, 2018

[0.17] doc: Change documentation for =0 for non-boolean options #14152

Merged

fanquake removed the Needs backport label Sep 6, 2018

HashUnlimited mentioned this pull request Sep 14, 2018

doc: Change documentation for =0 for non-boolean options chaincoin/chaincoin#667

Merged

Bushstar mentioned this pull request Oct 11, 2018

Updates from bitcoin/master FeatherCoin/Feathercoin#366

Merged

ryanofsky mentioned this pull request Jan 7, 2019

Add fee_est tool for debugging fee estimation code #10443

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

doc: Change documentation for =0 for non-boolean options #14100

doc: Change documentation for =0 for non-boolean options #14100

Uh oh!

Conversation

laanwj commented Aug 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Aug 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Aug 29, 2018

Uh oh!

maflcko commented Aug 29, 2018

Uh oh!

maflcko commented Aug 29, 2018

Uh oh!

maflcko commented Aug 29, 2018

Uh oh!

laanwj commented Aug 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ken2812221 commented Aug 31, 2018

Uh oh!

laanwj commented Sep 5, 2018

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Sep 5, 2018

Choose a reason for hiding this comment

Uh oh!

laanwj Sep 5, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko Sep 5, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko Sep 5, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko Sep 5, 2018

Choose a reason for hiding this comment

Uh oh!

laanwj Sep 5, 2018

Choose a reason for hiding this comment

Uh oh!

fanquake commented Sep 6, 2018

Uh oh!

Uh oh!

laanwj commented Aug 29, 2018 •

edited

Loading

laanwj commented Aug 29, 2018 •

edited

Loading

DrahtBot commented Aug 29, 2018 •

edited

Loading

laanwj commented Aug 30, 2018 •

edited

Loading