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

pull kevkevinpal wants to merge 1 commits into bitcoin:master from kevkevinpal:torcontrolHelp changing 3 files +5 −4
  1. 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

  2. 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.

  3. 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
  4. kevkevinpal force-pushed on Jul 19, 2023
  5. DrahtBot added the label CI failed on Jul 19, 2023
  6. DrahtBot removed the label CI failed on Jul 19, 2023
  7. 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).”

  8. luke-jr approved
  9. luke-jr commented at 3:06 pm on July 25, 2023: member
    utACK
  10. kevkevinpal force-pushed on Jul 26, 2023
  11. kevkevinpal force-pushed on Jul 26, 2023
  12. 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)."

  13. kevkevinpal force-pushed on Jul 28, 2023
  14. DrahtBot added the label Needs rebase on Aug 15, 2023
  15. kevkevinpal commented at 4:36 pm on August 17, 2023: contributor
    rebased to 5fa09d0
  16. MarcoFalke 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
  17. 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
  18. 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

  19. kevkevinpal force-pushed on Aug 17, 2023
  20. DrahtBot added the label CI failed on Aug 17, 2023
  21. 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

  22. 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
  23. jonatack commented at 5:33 pm on August 17, 2023: contributor
    Approach ACK modulo #28101 (review)
  24. DrahtBot removed the label Needs rebase on Aug 17, 2023
  25. kevkevinpal force-pushed on Aug 17, 2023
  26. 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
  27. jonatack commented at 9:33 pm on August 17, 2023: contributor

    ACK e5f20cb26848c97d48c6a479c4680f6823f2ec1e

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

  28. 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
  29. 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
  30. kevkevinpal force-pushed on Aug 18, 2023
  31. DrahtBot removed the label CI failed on Aug 18, 2023
  32. kevkevinpal requested review from luke-jr on Aug 24, 2023
  33. kevkevinpal requested review from jonatack on Aug 24, 2023
  34. jonatack commented at 10:50 pm on August 25, 2023: contributor
    re-ACK 9a84200cfc994eebf38c46919b20e0c0261799ae
  35. DrahtBot removed review request from jonatack on Aug 25, 2023
  36. jonatack commented at 7:24 pm on September 7, 2023: contributor
    Anything more needed here?
  37. MarnixCroes commented at 8:56 pm on September 7, 2023: contributor
    utACK 9a84200cfc994eebf38c46919b20e0c0261799ae
  38. kristapsk approved
  39. kristapsk commented at 9:42 pm on September 7, 2023: contributor
    utACK 9a84200cfc994eebf38c46919b20e0c0261799ae
  40. achow101 commented at 4:41 pm on September 12, 2023: member
    ACK 9a84200cfc994eebf38c46919b20e0c0261799ae
  41. achow101 merged this on Sep 12, 2023
  42. achow101 closed this on Sep 12, 2023


kevkevinpal DrahtBot luke-jr MarcoFalke jonatack MarnixCroes kristapsk achow101


luke-jr

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-03 10:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me