Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net: Add -networkactive option #19473

Merged
merged 3 commits into from Jul 23, 2020
Merged

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Jul 9, 2020

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.

@MarcoFalke
Copy link
Member

MarcoFalke commented Jul 9, 2020

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

No opinion on that, though.

Concept ACK on adding the functionality.

Copy link
Contributor

@LarryRuane LarryRuane left a comment

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.

src/init.cpp Outdated Show resolved Hide resolved
@hebasto
Copy link
Member Author

hebasto commented Jul 14, 2020

@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 :)

@LarryRuane
Copy link
Contributor

LarryRuane commented Jul 15, 2020

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.

Copy link
Member

@MarcoFalke MarcoFalke left a comment

Tested in the gui:

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

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

ACK e9a9c97 🎈

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK e9a9c9712f 🎈
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUiw8wwAhcByYIf/MRTpljoUpnyyQOh2M2qMLBv5ECseJmHeC9Tq1n/04FTVnguB
C6AjlPujp1jmIpyA1QLMp5j7hMjjBeRSE9t/xH5Tt1Lc5NpzmYucK6/JXU8+P7jN
EB/FpW2QAsAIBc7Hqyh+Cjq4aJegeNQn/j0zBgGKoiEjZtw2wSNgeOZ4QfezXGFT
iUJrlszz+FYQY4lPijAjIJcyVtWwMNuNG4N1T/gC9fi7vF8sZZCtyDdgZgl4VmDe
0IRKXKRz3HCcSe4o8lkB4AnEzqhhUwyORziMcxqkj8eNDYNhzDCfnHiRXZAnQYbk
whHxEHq68P6eRbnbPy+9MSs3HsE9Gu9jLZavZ6sjzdhHkqQXz06J6Irku/2hvEPa
oGygTwLyv2cvpP1kOW8FoE6ddpCCrCFWC9TkQZyJRhnTsoShcxAix5cp38VEVz9c
0KZNkNQZNZC1xsjN8RKkYKbXi72OziL9dPr2oX1ISTdbrzz+c2u7cP6cuo5a5oty
HclCb1NB
=2rnx
-----END PGP SIGNATURE-----

Timestamp of file with hash 289aaa99e8660b4dd3573b6eabcfd374fdeaf85e793ebd854168b6684124745b -

Though, I used the following diff:

diff --git a/src/init.cpp b/src/init.cpp
index a726c5e062..615f3929d4 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -455,7 +455,7 @@ void SetupServerArgs(NodeContext& node)
     gArgs.AddArg("-proxy=<ip:port>", "Connect through SOCKS5 proxy, set -noproxy to disable (default: disabled)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     gArgs.AddArg("-proxyrandomize", strprintf("Randomize credentials for every proxy connection. This enables Tor stream isolation (default: %u)", DEFAULT_PROXYRANDOMIZE), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     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);
-    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);
+    gArgs.AddArg("-networkactive", "Enable all p2p network activity (default: 1). Could be changed by the setnetworkactive RPC command", ArgsManager::ALLOW_BOOL, OptionsCategory::CONNECTION);
     gArgs.AddArg("-timeout=<n>", strprintf("Specify connection timeout in milliseconds (minimum: 1, default: %d)", DEFAULT_CONNECT_TIMEOUT), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     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);
     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);
@@ -1373,7 +1373,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node)
     assert(!node.banman);
     node.banman = MakeUnique<BanMan>(GetDataDir() / "banlist.dat", &uiInterface, gArgs.GetArg("-bantime", DEFAULT_MISBEHAVING_BANTIME));
     assert(!node.connman);
-    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)));
+    node.connman = MakeUnique<CConnman>(GetRand(std::numeric_limits<uint64_t>::max()), GetRand(std::numeric_limits<uint64_t>::max()), gArgs.GetBoolArg("-networkactive", true));
     // Make mempool generally available in the node context. For example the connection manager, wallet, or RPC threads,
     // which are all started after this, may use it from the node context.
     assert(!node.mempool);

src/init.cpp Outdated Show resolved Hide resolved
src/init.cpp Outdated Show resolved Hide resolved
@jonatack
Copy link
Member

jonatack commented Jul 18, 2020

Mind adding test coverage?

@hebasto
Copy link
Member Author

hebasto commented Jul 18, 2020

Updated e9a9c97 -> a323249 (pr19473.01 -> pr19473.02, diff):

  • addressed all comments

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 18, 2020

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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.

src/init.cpp Outdated
@@ -456,6 +456,7 @@ void SetupServerArgs(NodeContext& node)
gArgs.AddArg("-proxy=<ip:port>", "Connect through SOCKS5 proxy, set -noproxy to disable (default: disabled)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
gArgs.AddArg("-proxyrandomize", strprintf("Randomize credentials for every proxy connection. This enables Tor stream isolation (default: %u)", DEFAULT_PROXYRANDOMIZE), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
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);
gArgs.AddArg("-networkactive", "Enable all p2p network activity (default: 1). Can be changed by the setnetworkactive RPC command", ArgsManager::ALLOW_BOOL, OptionsCategory::CONNECTION);
Copy link
Member

@laanwj laanwj Jul 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please capitalize P2P

Copy link
Member Author

@hebasto hebasto Jul 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/net.cpp Outdated Show resolved Hide resolved
@laanwj
Copy link
Member

laanwj commented Jul 22, 2020

utACK otherwise

hebasto added 3 commits Jul 22, 2020
The `setnetworkactive' RPC command is already present.
This new option allows to start the client with disabled p2p network
activity for testing or reindexing.
@hebasto
Copy link
Member Author

hebasto commented Jul 22, 2020

Updated a323249 -> 2aac093 (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?

Copy link
Contributor

@LarryRuane LarryRuane left a comment

ACK 2aac093

@MarcoFalke
Copy link
Member

MarcoFalke commented Jul 23, 2020

re-ACK 2aac093 🏠

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

re-ACK 2aac093a3d60e446b85eebdf170ea6bed77bec92 🏠
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgzggv8Cu5uiAMd9wd9hW4BXGop+YSDO/jU8KwnxV9CWvYJ9DRLfI8ke5DtsY9x
yJFXnsihcsHmfXuIwfUrXTwkDQJkMY6iULwXCNPApEBjVPgbzpp1oDUU1YXVgQTW
cZehgGiH5K3O/AGr4PgGQ5+RssYw4l9Q5OO9jB1p9kjnAsKOuwGh5ArWLsLeM/84
vkBXs21DuyzqQpJBGHrqRqNM2kMaoBPmkb7gI5C5sW30ZuoAwN4x2NeqMc0UCsGt
gx3MnOHt1lwe4I75NGHO6VajdKJgYxGiRDCKgBJSFUaaRqrai6u27PBqiuJOBm1z
Cw4emfaTovnLM09LHuBYvDnpXd9wLySRpDe254aKMqett5uI4mKwobs521v40zeA
ETHn/8m6aARqgQKK3H6a61Ts8FBfL3rVFIakAIgC/ncUTEEWa9lhkj4OUTU1Y2fO
k2A9l0TQPm41tFegIGdG0ff9NHDj5urBD/1DXWpVCV0FBxJFxfCof+GfcykYoQLa
RD3CPEnA
=Vohr
-----END PGP SIGNATURE-----

Timestamp of file with hash 79e733808b8f76828a454a01b6ad120aa8619b7cef190ba12821d040e4881d01 -

@MarcoFalke MarcoFalke merged commit 6ee36a2 into bitcoin:master Jul 23, 2020
3 of 4 checks passed
@hebasto hebasto deleted the 200709-setnet branch Jul 23, 2020
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Aug 15, 2020
The `setnetworkactive' RPC command is already present.
This new option allows to start the client with disabled p2p network
activity for testing or reindexing.

Github-Pull: bitcoin#19473
Rebased-From: aabc6af
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Aug 15, 2020
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jul 1, 2021
Summary:
Some Bitcoin 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`.

Commit message #1:
> 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.

Commit message #2:
>net: Log network activity status change unconditionally

Commit message #3:
> test: Add test coverage for -networkactive option

This is a backport of [[bitcoin/bitcoin#19473 | core#19473]]

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D9713
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants