net: Add -networkactive option #19473

pull hebasto wants to merge 3 commits into bitcoin:master from hebasto:200709-setnet changing 5 files +38 −6
  1. hebasto commented at 7:33 am on July 9, 2020: member

    Some Bitcoin Core activity is completely local (offline), e.g., reindexing.

    The setnetworkactive RPC command is already present. This PR adds the corresponding command-line argument / config option, and allows to start the client with disabled p2p network by providing -networkactive=0 or -nonetworkactive.

    This was done while reviewing #16981.

  2. fanquake added the label P2P on Jul 9, 2020
  3. MarcoFalke added the label Settings on Jul 9, 2020
  4. MarcoFalke commented at 8:53 am on July 9, 2020: member

    There was an idea to overload -noconnect with this. #15900 (comment)

    No opinion on that, though.

    Concept ACK on adding the functionality.

  5. in src/init.cpp:459 in e9a9c9712f outdated 
    455@@ -456,6 +456,7 @@ void SetupServerArgs(NodeContext& node)
456     gArgs.AddArg("-proxy=<ip:port>", "Connect through SOCKS5 proxy, set -noproxy to disable (default: disabled)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
457     gArgs.AddArg("-proxyrandomize", strprintf("Randomize credentials for every proxy connection. This enables Tor stream isolation (default: %u)", DEFAULT_PROXYRANDOMIZE), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
458     gArgs.AddArg("-seednode=<ip>", "Connect to a node to retrieve peer addresses, and disconnect. This option can be specified multiple times to connect to multiple nodes.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
459+    gArgs.AddArg("-networkactive", "Enable all p2p network activity (default: 1). Could be changed by the `setnetworkactive' RPC command", ArgsManager::ALLOW_BOOL | ArgsManager::DEBUG_ONLY, OptionsCategory::CONNECTION);
    LarryRuane commented at 1:05 am on July 13, 2020:
    The open-quote character before setnetworkactive is a back-quote, but the close-quote character is a regular single-quote. There is one other place in this file that uses back-quote characters (addnode), but there they are balanced (back-quote for both open-quote and close-quote).
    hebasto commented at 10:24 am on July 17, 2020:

    https://www.gnu.org/prep/standards/html_node/Quote-Characters.html

    TBH, I’m opposite to using back-quote characters for both opening and closing quotes. If you believe that quotes should be balanced, could they be '?

    jonatack commented at 10:37 am on July 17, 2020:
    ISTM the most common practice ATM is neither quotes nor backticks for RPC commands in the bitcoind help, e.g. bitcoind -help | grep "pruneblockchain\|getrawtransaction"
    MarcoFalke commented at 6:21 am on July 18, 2020:
    why is it debug_only?
    jonatack commented at 7:07 am on July 18, 2020:
    Also: s/Could/Can/
    hebasto commented at 9:08 am on July 18, 2020:
    Updated.
    hebasto commented at 9:08 am on July 18, 2020:
    Updated.
  6. LarryRuane commented at 1:47 am on July 13, 2020: contributor

    Nice PR! I tested various combinations:

    • bitcoind -networkactive
    • bitcoind -networkactive=1
    • bitcoind -networkactive=0
    • bitcoind -nonetworkactive

    and the behavior was as expected (the first two enabled and the second two disabled networking). I also verified that after disabling networking on the command line, I could change it in both directions using the setnetworkactive RPC.

    I think it’s okay that there’s no unit test, because this is test code anyway, and there’s no easy place to add one. The only thing bad that could happen is networking isn’t enabled by default, and that would be noticed right away!

    If you can fix that tiny quote problem, I’d be happy to approve.

  7. hebasto commented at 4:14 pm on July 14, 2020: member

    @LarryRuane

    The only thing bad that could happen is networking isn’t enabled by default, and that would be noticed right away!

    I think it is enabled by default :)

  8. LarryRuane commented at 3:25 am on July 15, 2020: contributor

    I think it is enabled by default :)

    Yes, sorry, my comment wasn’t clear; I just meant that without a test, a bug in this PR could conceivably make networking disabled by default. But, as I said, that would be pretty obvious, so I’m not worried! Plus, now that I think about it, there are numerous existing tests that would fail. So never mind. I was just trying to support your decision not to include a test.

  9. in src/init.cpp:1376 in e9a9c9712f outdated 
    1372@@ -1372,7 +1373,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node)
1373     assert(!node.banman);
1374     node.banman = MakeUnique<BanMan>(GetDataDir() / "banlist.dat", &uiInterface, gArgs.GetArg("-bantime", DEFAULT_MISBEHAVING_BANTIME));
1375     assert(!node.connman);
1376-    node.connman = std::unique_ptr<CConnman>(new CConnman(GetRand(std::numeric_limits<uint64_t>::max()), GetRand(std::numeric_limits<uint64_t>::max())));
1377+    node.connman = std::unique_ptr<CConnman>(new CConnman(GetRand(std::numeric_limits<uint64_t>::max()), GetRand(std::numeric_limits<uint64_t>::max()), gArgs.GetBoolArg("-networkactive", true)));
    MarcoFalke commented at 6:15 am on July 18, 2020:
    unrelated style-nit: Can’t this use MakeUnique?
    hebasto commented at 9:08 am on July 18, 2020:
    Updated.
  10. MarcoFalke approved
  11. MarcoFalke commented at 6:27 am on July 18, 2020: member

    Tested in the gui:

    0$ ./src/qt/bitcoin-qt -? |grep -A1 networka
1  -networkactive
2       Enable all p2p network activity (default: 1). Could be changed by the
3       setnetworkactive RPC command

    Screenshot from 2020-07-18 08-25-05

    ACK e9a9c9712f 🎈

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK e9a9c9712f 🎈
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUiw8wwAhcByYIf/MRTpljoUpnyyQOh2M2qMLBv5ECseJmHeC9Tq1n/04FTVnguB
 8C6AjlPujp1jmIpyA1QLMp5j7hMjjBeRSE9t/xH5Tt1Lc5NpzmYucK6/JXU8+P7jN
 9EB/FpW2QAsAIBc7Hqyh+Cjq4aJegeNQn/j0zBgGKoiEjZtw2wSNgeOZ4QfezXGFT
10iUJrlszz+FYQY4lPijAjIJcyVtWwMNuNG4N1T/gC9fi7vF8sZZCtyDdgZgl4VmDe
110IRKXKRz3HCcSe4o8lkB4AnEzqhhUwyORziMcxqkj8eNDYNhzDCfnHiRXZAnQYbk
12whHxEHq68P6eRbnbPy+9MSs3HsE9Gu9jLZavZ6sjzdhHkqQXz06J6Irku/2hvEPa
13oGygTwLyv2cvpP1kOW8FoE6ddpCCrCFWC9TkQZyJRhnTsoShcxAix5cp38VEVz9c
140KZNkNQZNZC1xsjN8RKkYKbXi72OziL9dPr2oX1ISTdbrzz+c2u7cP6cuo5a5oty
15HclCb1NB
16=2rnx
17-----END PGP SIGNATURE-----

    Timestamp of file with hash 289aaa99e8660b4dd3573b6eabcfd374fdeaf85e793ebd854168b6684124745b -

    Though, I used the following diff:

     0diff --git a/src/init.cpp b/src/init.cpp
 1index a726c5e062..615f3929d4 100644
 2--- a/src/init.cpp
 3+++ b/src/init.cpp
 4@@ -455,7 +455,7 @@ void SetupServerArgs(NodeContext& node)
 5     gArgs.AddArg("-proxy=<ip:port>", "Connect through SOCKS5 proxy, set -noproxy to disable (default: disabled)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 6     gArgs.AddArg("-proxyrandomize", strprintf("Randomize credentials for every proxy connection. This enables Tor stream isolation (default: %u)", DEFAULT_PROXYRANDOMIZE), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 7     gArgs.AddArg("-seednode=<ip>", "Connect to a node to retrieve peer addresses, and disconnect. This option can be specified multiple times to connect to multiple nodes.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 8-    gArgs.AddArg("-networkactive", "Enable all p2p network activity (default: 1). Could be changed by the `setnetworkactive' RPC command", ArgsManager::ALLOW_BOOL | ArgsManager::DEBUG_ONLY, OptionsCategory::CONNECTION);
 9+    gArgs.AddArg("-networkactive", "Enable all p2p network activity (default: 1). Could be changed by the setnetworkactive RPC command", ArgsManager::ALLOW_BOOL, OptionsCategory::CONNECTION);
10     gArgs.AddArg("-timeout=<n>", strprintf("Specify connection timeout in milliseconds (minimum: 1, default: %d)", DEFAULT_CONNECT_TIMEOUT), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
11     gArgs.AddArg("-peertimeout=<n>", strprintf("Specify p2p connection timeout in seconds. This option determines the amount of time a peer may be inactive before the connection to it is dropped. (minimum: 1, default: %d)", DEFAULT_PEER_CONNECT_TIMEOUT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CONNECTION);
12     gArgs.AddArg("-torcontrol=<ip>:<port>", strprintf("Tor control port to use if onion listening enabled (default: %s)", DEFAULT_TOR_CONTROL), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
13@@ -1373,7 +1373,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node)
14     assert(!node.banman);
15     node.banman = MakeUnique<BanMan>(GetDataDir() / "banlist.dat", &uiInterface, gArgs.GetArg("-bantime", DEFAULT_MISBEHAVING_BANTIME));
16     assert(!node.connman);
17-    node.connman = std::unique_ptr<CConnman>(new CConnman(GetRand(std::numeric_limits<uint64_t>::max()), GetRand(std::numeric_limits<uint64_t>::max()), gArgs.GetBoolArg("-networkactive", true)));
18+    node.connman = MakeUnique<CConnman>(GetRand(std::numeric_limits<uint64_t>::max()), GetRand(std::numeric_limits<uint64_t>::max()), gArgs.GetBoolArg("-networkactive", true));
19     // Make mempool generally available in the node context. For example the connection manager, wallet, or RPC threads,
20     // which are all started after this, may use it from the node context.
21     assert(!node.mempool);
  12. jonatack commented at 7:08 am on July 18, 2020: member
    Mind adding test coverage?
  13. hebasto force-pushed on Jul 18, 2020
  14. hebasto commented at 9:07 am on July 18, 2020: member

    Updated e9a9c9712f8a9f22d90057f095676a29a2d0b77f -> a3232493378d87821a3e3b1c11f9070329cb6c5e (pr19473.01 -> pr19473.02, diff):

    • addressed all comments
  15. DrahtBot commented at 9:06 pm on July 18, 2020: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #19561 (refactor: Pass ArgsManager into functions that register args by S3RK)
    • #19405 (rpc, cli: add network in/out connections to getnetworkinfo and -getinfo by jonatack)

    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.

  16. in src/init.cpp:459 in a323249337 outdated 
    455@@ -456,6 +456,7 @@ void SetupServerArgs(NodeContext& node)
456     gArgs.AddArg("-proxy=<ip:port>", "Connect through SOCKS5 proxy, set -noproxy to disable (default: disabled)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
457     gArgs.AddArg("-proxyrandomize", strprintf("Randomize credentials for every proxy connection. This enables Tor stream isolation (default: %u)", DEFAULT_PROXYRANDOMIZE), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
458     gArgs.AddArg("-seednode=<ip>", "Connect to a node to retrieve peer addresses, and disconnect. This option can be specified multiple times to connect to multiple nodes.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
459+    gArgs.AddArg("-networkactive", "Enable all p2p network activity (default: 1). Can be changed by the setnetworkactive RPC command", ArgsManager::ALLOW_BOOL, OptionsCategory::CONNECTION);
    laanwj commented at 7:19 pm on July 22, 2020:
    Please capitalize P2P
    hebasto commented at 8:03 pm on July 22, 2020:

    Indeed!

    Updated.

  17. in src/net.cpp:2268 in a323249337 outdated 
    2261@@ -2262,8 +2262,10 @@ void CConnman::SetNetworkActive(bool active)
2262     uiInterface.NotifyNetworkActiveChanged(fNetworkActive);
2263 }
2264 
2265-CConnman::CConnman(uint64_t nSeed0In, uint64_t nSeed1In) : nSeed0(nSeed0In), nSeed1(nSeed1In)
2266+CConnman::CConnman(uint64_t nSeed0In, uint64_t nSeed1In, bool network_active)
2267+    : nSeed0(nSeed0In), nSeed1(nSeed1In)
2268 {
2269+    SetNetworkActive(network_active);
    laanwj commented at 7:25 pm on July 22, 2020:
    I don’t think it makes any difference in practice in this case, but isn’t the general convention to call functions like this on the object only after Init?
    hebasto commented at 8:03 pm on July 22, 2020:
    Updated.
  18. laanwj commented at 7:27 pm on July 22, 2020: member
    utACK otherwise
  19. net: Add -networkactive option 
    The `setnetworkactive' RPC command is already present.
This new option allows to start the client with disabled p2p network
activity for testing or reindexing.
    62fe6aa87e
  20. net: Log network activity status change unconditionally 3c58129b12
  21. test: Add test coverage for -networkactive option 2aac093a3d
  22. hebasto force-pushed on Jul 22, 2020
  23. hebasto commented at 8:01 pm on July 22, 2020: member

    Updated a3232493378d87821a3e3b1c11f9070329cb6c5e -> 2aac093a3d60e446b85eebdf170ea6bed77bec92 (pr19473.02 -> pr19473.03, diff):

    Please capitalize P2P

    I don’t think it makes any difference in practice in this case, but isn’t the general convention to call functions like this on the object only after Init?

  24. LarryRuane approved
  25. LarryRuane commented at 3:42 pm on July 23, 2020: contributor
    ACK 2aac093a3d60e446b85eebdf170ea6bed77bec92
  26. MarcoFalke commented at 4:32 pm on July 23, 2020: member

    re-ACK 2aac093a3d60e446b85eebdf170ea6bed77bec92 🏠

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK 2aac093a3d60e446b85eebdf170ea6bed77bec92 🏠
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgzggv8Cu5uiAMd9wd9hW4BXGop+YSDO/jU8KwnxV9CWvYJ9DRLfI8ke5DtsY9x
 8yJFXnsihcsHmfXuIwfUrXTwkDQJkMY6iULwXCNPApEBjVPgbzpp1oDUU1YXVgQTW
 9cZehgGiH5K3O/AGr4PgGQ5+RssYw4l9Q5OO9jB1p9kjnAsKOuwGh5ArWLsLeM/84
10vkBXs21DuyzqQpJBGHrqRqNM2kMaoBPmkb7gI5C5sW30ZuoAwN4x2NeqMc0UCsGt
11gx3MnOHt1lwe4I75NGHO6VajdKJgYxGiRDCKgBJSFUaaRqrai6u27PBqiuJOBm1z
12Cw4emfaTovnLM09LHuBYvDnpXd9wLySRpDe254aKMqett5uI4mKwobs521v40zeA
13ETHn/8m6aARqgQKK3H6a61Ts8FBfL3rVFIakAIgC/ncUTEEWa9lhkj4OUTU1Y2fO
14k2A9l0TQPm41tFegIGdG0ff9NHDj5urBD/1DXWpVCV0FBxJFxfCof+GfcykYoQLa
15RD3CPEnA
16=Vohr
17-----END PGP SIGNATURE-----

    Timestamp of file with hash 79e733808b8f76828a454a01b6ad120aa8619b7cef190ba12821d040e4881d01 -

  27. MarcoFalke merged this on Jul 23, 2020
  28. MarcoFalke closed this on Jul 23, 2020
  29. hebasto deleted the branch on Jul 23, 2020
  30. luke-jr referenced this in commit 3b8f0e16db on Aug 15, 2020
  31. luke-jr referenced this in commit a76ee98603 on Aug 15, 2020
  32. Fabcien referenced this in commit 9f9dcac429 on Jul 1, 2021
  33. DrahtBot locked this on Feb 15, 2022


hebasto MarcoFalke LarryRuane jonatack DrahtBot laanwj

Labels
P2P Settings

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-12-21 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me