doc, refactor: changing -torcontrol help to specify that a default port is used

kevkevinpal commented at 3:07 am on July 19, 2023: contributor

Right now when we get the help for -torcontrol it says that there is a default ip and port we dont specify if there is a specified ip that we would also use port 9051 as default

Also I create a new const instead of using 9051 directly in the function

linking this PR because this was discussed here https://github.com/bitcoin/bitcoin/pull/28018

DrahtBot commented at 3:07 am on July 19, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	jonatack, MarnixCroes, kristapsk, achow101
Concept ACK	luke-jr

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

Conflicts

No conflicts as of last run.

kevkevinpal renamed this:
~~Changing -torcontrol help to specify that a default port is used~~
init: changing -torcontrol help to specify that a default port is used
on Jul 19, 2023

kevkevinpal force-pushed on Jul 19, 2023

DrahtBot added the label CI failed on Jul 19, 2023

DrahtBot removed the label CI failed on Jul 19, 2023

in src/init.cpp:511 in ea9da2ffd8 outdated

507@@ -508,7 +508,7 @@ void SetupServerArgs(ArgsManager& argsman)
508     argsman.AddArg("-networkactive", "Enable all P2P network activity (default: 1). Can be changed by the setnetworkactive RPC command", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
509     argsman.AddArg("-timeout=<n>", strprintf("Specify socket connection timeout in milliseconds. If an initial attempt to connect is unsuccessful after this amount of time, drop it (minimum: 1, default: %d)", DEFAULT_CONNECT_TIMEOUT), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
510     argsman.AddArg("-peertimeout=<n>", strprintf("Specify a p2p connection timeout delay in seconds. After connecting to a peer, wait this amount of time before considering disconnection based on inactivity (minimum: 1, default: %d)", DEFAULT_PEER_CONNECT_TIMEOUT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CONNECTION);
511-    argsman.AddArg("-torcontrol=<ip>:<port>", strprintf("Tor control port to use if onion listening enabled (default: %s)", DEFAULT_TOR_CONTROL), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
512+    argsman.AddArg("-torcontrol=<ip>:<port>", strprintf("Tor control host and port to use if onion listening enabled (default: %s) If no port specified default port %s is used", DEFAULT_TOR_CONTROL, ToString(DEFAULT_TOR_CONTROL_PORT)), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);

luke-jr commented at 3:06 pm on July 25, 2023:

nit: I feel like this could be formatted better. Might need a comma after “specified”.

kevkevinpal commented at 5:49 pm on July 26, 2023:

Does this look good? I used chatgpt to refine the sentence a bit more to this

“Specify the Tor control host and optional port to use when onion listening is enabled (default: %s). If no port is specified, the default port will be used (default: %s).”

luke-jr approved

luke-jr commented at 3:06 pm on July 25, 2023: member

utACK

kevkevinpal force-pushed on Jul 26, 2023

in src/init.cpp:511 in e70ee8be8b outdated

507@@ -508,7 +508,7 @@ void SetupServerArgs(ArgsManager& argsman)
508     argsman.AddArg("-networkactive", "Enable all P2P network activity (default: 1). Can be changed by the setnetworkactive RPC command", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
509     argsman.AddArg("-timeout=<n>", strprintf("Specify socket connection timeout in milliseconds. If an initial attempt to connect is unsuccessful after this amount of time, drop it (minimum: 1, default: %d)", DEFAULT_CONNECT_TIMEOUT), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
510     argsman.AddArg("-peertimeout=<n>", strprintf("Specify a p2p connection timeout delay in seconds. After connecting to a peer, wait this amount of time before considering disconnection based on inactivity (minimum: 1, default: %d)", DEFAULT_PEER_CONNECT_TIMEOUT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CONNECTION);
511-    argsman.AddArg("-torcontrol=<ip>:<port>", strprintf("Tor control port to use if onion listening enabled (default: %s)", DEFAULT_TOR_CONTROL), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
512+    argsman.AddArg("-torcontrol=<ip>:<port>", strprintf("Specify the Tor control host and optional port to use when onion listening is enabled (default: %s). If no port is specified, the default port will be used (default: %s).", DEFAULT_TOR_CONTROL, ToString(DEFAULT_TOR_CONTROL_PORT)), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);

luke-jr commented at 10:57 pm on July 27, 2023:

I used chatgpt to refine the sentence a bit more to this

Please redo this without ChatGPT involved. LLMs do not have a clear copyright status.

kevkevinpal commented at 3:16 am on July 28, 2023:

Valid point will change to the following

"Tor control host and port to use if onion listening enabled (default: %s). If no port is specified, the default port will be used (default: %s)."

kevkevinpal force-pushed on Jul 28, 2023

DrahtBot added the label Needs rebase on Aug 15, 2023

kevkevinpal commented at 4:36 pm on August 17, 2023: contributor

rebased to 5fa09d0

maflcko commented at 5:09 pm on August 17, 2023: member

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

in src/torcontrol.cpp:26 in 5fa09d039f outdated

22@@ -23,6 +23,7 @@
23 #include <util/string.h>
24 #include <util/thread.h>
25 #include <util/time.h>
26+#include <util/string.h>

jonatack commented at 5:13 pm on August 17, 2023:

This is already included 3 lines higher.

kevkevinpal commented at 5:25 pm on August 17, 2023:

thanks, addressed in 5d94b1f

in src/torcontrol.h:22 in 5fa09d039f outdated

18@@ -19,6 +19,7 @@
19 #include <string>
20 #include <vector>
21 
22+static const int DEFAULT_TOR_CONTROL_PORT = 9051;

jonatack commented at 5:14 pm on August 17, 2023:

ISTM static does nothing here, and this can be constexpr.

0constexpr int DEFAULT_TOR_CONTROL_PORT = 9051;

kevkevinpal commented at 5:26 pm on August 17, 2023:

thanks didnt know that fixed in

5d94b1f

kevkevinpal force-pushed on Aug 17, 2023

DrahtBot added the label CI failed on Aug 17, 2023

kevkevinpal commented at 5:25 pm on August 17, 2023: contributor

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

rebased properly in 5d94b1f

in src/init.cpp:512 in 5d94b1faec outdated

508@@ -509,7 +509,7 @@ void SetupServerArgs(ArgsManager& argsman)
509     argsman.AddArg("-networkactive", "Enable all P2P network activity (default: 1). Can be changed by the setnetworkactive RPC command", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
510     argsman.AddArg("-timeout=<n>", strprintf("Specify socket connection timeout in milliseconds. If an initial attempt to connect is unsuccessful after this amount of time, drop it (minimum: 1, default: %d)", DEFAULT_CONNECT_TIMEOUT), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
511     argsman.AddArg("-peertimeout=<n>", strprintf("Specify a p2p connection timeout delay in seconds. After connecting to a peer, wait this amount of time before considering disconnection based on inactivity (minimum: 1, default: %d)", DEFAULT_PEER_CONNECT_TIMEOUT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CONNECTION);
512-    argsman.AddArg("-torcontrol=<ip>:<port>", strprintf("Tor control port to use if onion listening enabled (default: %s)", DEFAULT_TOR_CONTROL), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
513+    argsman.AddArg("-torcontrol=<ip>:<port>", strprintf("Tor control host and port to use if onion listening enabled (default: %s). If no port is specified, the default port will be used (default: %s).", DEFAULT_TOR_CONTROL, ToString(DEFAULT_TOR_CONTROL_PORT)), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);

jonatack commented at 5:30 pm on August 17, 2023:

In the new sentence, duplicate “default” seems redundant:

0$ ./src/bitcoind -h | grep -A3 torcontrol 
1  -torcontrol=<ip>:<port>
2       Tor control host and port to use if onion listening enabled (default:
3       127.0.0.1:9051). If no port is specified, the default port will
4       be used (default: 9051).

0    argsman.AddArg("-torcontrol=<ip>:<port>", strprintf("Tor control host and port to use if onion listening enabled (default: %s). If no port is specified, the default port of %i will be used.", DEFAULT_TOR_CONTROL, ToString(DEFAULT_TOR_CONTROL_PORT)), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);

0  -torcontrol=<ip>:<port>
1       Tor control host and port to use if onion listening enabled (default:
2       127.0.0.1:9051). If no port is specified, the default port of
3       9051 will be used.

kevkevinpal commented at 9:14 pm on August 17, 2023:

Update in e5f20cb26848c97d48c6a479c4680f6823f2ec1e

jonatack commented at 5:33 pm on August 17, 2023: member

Approach ACK modulo #28101 (review)

DrahtBot removed the label Needs rebase on Aug 17, 2023

kevkevinpal force-pushed on Aug 17, 2023

in src/torcontrol.h:22 in e5f20cb268 outdated

18@@ -19,6 +19,8 @@
19 #include <string>
20 #include <vector>
21 
22+

jonatack commented at 9:31 pm on August 17, 2023:

If you retouch, can remove this added newline.

kevkevinpal commented at 4:59 am on August 18, 2023:

removed new line and updated title in 9a84200

jonatack commented at 9:33 pm on August 17, 2023: member

ACK e5f20cb26848c97d48c6a479c4680f6823f2ec1e

It might be good to prefix the pull title with doc, refactor: instead of init.

kevkevinpal renamed this:
~~init: changing -torcontrol help to specify that a default port is used~~
doc, refactor: changing -torcontrol help to specify that a default port is used
on Aug 18, 2023

doc, refactor: Changing -torcontrol help to specify that a default port is used

Right now when we get the help for -torcontrol it says that there is a
default ip and port we dont specify if there is a specified ip that we
would also use port 9051 as default

9a84200cfc

kevkevinpal force-pushed on Aug 18, 2023

DrahtBot removed the label CI failed on Aug 18, 2023

kevkevinpal requested review from luke-jr on Aug 24, 2023

kevkevinpal requested review from jonatack on Aug 24, 2023

jonatack commented at 10:50 pm on August 25, 2023: member

re-ACK 9a84200cfc994eebf38c46919b20e0c0261799ae

DrahtBot removed review request from jonatack on Aug 25, 2023

jonatack commented at 7:24 pm on September 7, 2023: member

Anything more needed here?

MarnixCroes commented at 8:56 pm on September 7, 2023: contributor

utACK 9a84200cfc994eebf38c46919b20e0c0261799ae

kristapsk approved

kristapsk commented at 9:42 pm on September 7, 2023: contributor

utACK 9a84200cfc994eebf38c46919b20e0c0261799ae

achow101 commented at 4:41 pm on September 12, 2023: member

ACK 9a84200cfc994eebf38c46919b20e0c0261799ae

achow101 merged this on Sep 12, 2023

achow101 closed this on Sep 12, 2023

luke-jr referenced this in commit 2a3756d2ef on Sep 16, 2023

Frank-GER referenced this in commit e98d6b2b06 on Sep 19, 2023

bitcoin locked this on Sep 11, 2024

doc, refactor: changing -torcontrol help to specify that a default port is used #28101

Reviews

Conflicts