rpc, cli: add network in/out connections to `getnetworkinfo` and `-getinfo` #19405

jonatack commented at 9:00 AM on June 29, 2020: member

This is basic info that is present in the GUI that I've been wishing to have exposed via the RPC and CLI without needing a bash workaround or script. For human users it would also be useful to have it in -getinfo.

bitcoin-cli getnetworkinfo

  "connections": 15,
  "connections_in": 6,
  "connections_out": 9,

bitcoin-cli -getinfo

  "connections": {
    "in": 6,
    "out": 9,
    "total": 15
  },

Update the tests, RPC help, and release notes for the changes. Also fixup the getnettotals timemillis help while touching rpc/net.cpp.

Reviewers can manually test this PR by building from source, launching bitcoind, and then running bitcoin-cli -getinfo, bitcoin-cli getnetworkinfo, bitcoin-cli help getnetworkinfo, and bitcoin-cli help getnettotals (for the UNIX epoch time change).

fanquake added the label RPC/REST/ZMQ on Jun 29, 2020

in src/rpc/net.cpp:486 in e7c7809a09 outdated

 483 | @@ -484,6 +484,8 @@ static UniValue getnetworkinfo(const JSONRPCRequest& request)
 484 |                          {RPCResult::Type::BOOL, "localrelay", "true if transaction relay is requested from peers"},
 485 |                          {RPCResult::Type::NUM, "timeoffset", "the time offset"},
 486 |                          {RPCResult::Type::NUM, "connections", "the number of connections"},

jonatack commented at 11:01 AM on June 29, 2020:

Possibly change this line to "the total number of connections".

laanwj commented at 1:52 PM on June 29, 2020: member

This is info I've been wishing to have for some time -- probably there is an RPC I'm overlooking? But for human users it seems good to have it in -getinfo.

Yeah, I've personally always used a stats script that goes through getpeerinfo and categorizes them by in/out × ipv4/ipv6/tor.

in:  ipv4 139 ipv6  20 tor   0
out: ipv4  10 ipv6   0 tor   0

Seems like something that could easily be done client-side in bitcoin-cli itself. But no objections to adding this server-side, it's a small code change.

practicalswift commented at 2:00 PM on June 29, 2020: contributor

Concept ACK

jonatack force-pushed on Jun 29, 2020

jonatack commented at 3:14 PM on June 29, 2020: member

Re-pushed with release notes.

Yeah, I've personally always used a stats script that goes through getpeerinfo and categorizes them by in/out × ipv4/ipv6/tor.
in:  ipv4 139 ipv6  20 tor   0
out: ipv4  10 ipv6   0 tor   0
Seems like something that could easily be done client-side in bitcoin-cli itself. But no objections to adding this server-side, it's a small code change.

That's pretty nice. In terms of vertical space for human use, something like that might be better as string values

  "connections": 149,
  "in": "ipv4 139 ipv6 20 tor 0",
  "out": "ipv4 10 ipv6  0 tor 0",

rather than a JSON object

  "connections": {
    "in": {
      "ipv4": 139,
      "ipv6": 20,
      "tor": 0
    },
    "out": {
      "ipv4": 10,
      "ipv6": 0,
      "tor": 0
    },
    "total": 149
  },

We could perhaps create a new CLI -netinfo option to provide a more detailed report of the network peer connections.

in src/rpc/net.cpp:410 in 9290a98843 outdated

 406 | @@ -407,7 +407,7 @@ static UniValue getnettotals(const JSONRPCRequest& request)
 407 |                     {
 408 |                         {RPCResult::Type::NUM, "totalbytesrecv", "Total bytes received"},
 409 |                         {RPCResult::Type::NUM, "totalbytessent", "Total bytes sent"},
 410 | -                       {RPCResult::Type::NUM_TIME, "timemillis", "Current UNIX time in milliseconds"},
 411 | +                       {RPCResult::Type::NUM_TIME, "timemillis", UNIX_EPOCH_TIME + " in milliseconds"},

MarcoFalke commented at 5:27 PM on June 29, 2020:

nit: Any reason to remove "Current "?

jonatack commented at 5:44 PM on June 29, 2020:

nit: Any reason to remove "Current "?

Without any other qualifier I thought it was self-understood but don't mind adding back "Current" if reviewers think it's unclear.

jonatack commented at 6:25 PM on June 29, 2020:

Re-added while retouching to fix the extra line.

in src/rpc/net.cpp:539 in 9290a98843 outdated

 532 | @@ -531,7 +533,10 @@ static UniValue getnetworkinfo(const JSONRPCRequest& request)
 533 |      obj.pushKV("timeoffset",    GetTimeOffset());
 534 |      if (node.connman) {
 535 |          obj.pushKV("networkactive", node.connman->GetNetworkActive());
 536 | -        obj.pushKV("connections",   (int)node.connman->GetNodeCount(CConnman::CONNECTIONS_ALL));
 537 | +        obj.pushKV("connections", (int)node.connman->GetNodeCount(CConnman::CONNECTIONS_ALL));
 538 | +        obj.pushKV("connections_in", (int)node.connman->GetNodeCount(CConnman::CONNECTIONS_IN));
 539 | +        obj.pushKV("connections_out", (int)node.connman->GetNodeCount(CConnman::CONNECTIONS_OUT));
 540 | +

MarcoFalke commented at 5:27 PM on June 29, 2020:

nit: Any reason to add an empty line?

jonatack commented at 6:10 PM on June 29, 2020:

That's strange and inadvertant. Will remove.

jonatack commented at 6:26 PM on June 29, 2020:

Thanks -- removed.

MarcoFalke commented at 5:28 PM on June 29, 2020: member

Concept ACK, but I am wondering if instead the connection type (#19316) should be returned?

jonatack commented at 6:01 PM on June 29, 2020: member

Concept ACK, but I am wondering if instead the connection type (#19316) should be returned?

That PR seems orthogonal, as it proposes to detail the outbounds into various subtypes, but the basic distinction remains between inbound and outbound connections.

I reckon that exposing to users and RPC clients the counts of the outbound subtypes in addition to in/out/total would be enough detail and screen space to warrant a separate client-side -netinfo option in a future PR that could also provide @laanwj's ip4/6/tor breakdown.

jonatack force-pushed on Jun 29, 2020

jonatack commented at 6:27 PM on June 29, 2020: member

Addressed @MarcoFalke's suggestions. Thanks everyone. Rebased per git diff 9290a98 94a792cc.

jnewbery commented at 1:35 PM on July 1, 2020: member

I have no objection to adding this to getnetworkinfo, but just noting that jq is your friend here:

bitcoin-cli getpeerinfo | jq "group_by(.inbound) | map({Inbound: .[0].inbound, Count: length}) | .[]"
{
  "Inbound": false,
  "Count": 9
}
{
  "Inbound": true,
  "Count": 6
}

willcl-ark commented at 2:25 PM on July 2, 2020: member

tACK.

Changes seem simple-enough and are working (and passing tests) on MacOS 10.15.

Also pleased to learn jnewbery's advanced jq pipe from above :)

eriknylund commented at 9:22 AM on July 15, 2020: contributor

ACK 94a792cc19f26754ac5d60a1b733d4b520f7d0c7

Ran unit tests and extended functional tests, all pass. Did not run fuzz tests. Started bitcoind on a clean datadir and ran the bitcoin-cli commands to verify the new output:

$ ./bitcoind -datadir=/tmp/pr19405

Tested -getinfo

$ ./bitcoin-cli -datadir=/tmp/pr19405 -getinfo
{
  "version": 209900,
  "blocks": 0,
  "headers": 192000,
  "verificationprogress": 1.827923226519457e-09,
  "timeoffset": 0,
  "connections": {
    "in": 0,
    "out": 6,
    "total": 6
  },
  "proxy": "",
  "difficulty": 1,
  "chain": "main",
  "keypoolsize": 1000,
  "paytxfee": 0.00000000,
  "balance": 0.00000000,
  "relayfee": 0.00001000,
  "warnings": "This is a pre-release test build - use at your own risk - do not use for mining or merchant applications"
}

Tested getnetworkinfo:

$ ./bitcoin-cli -datadir=/tmp/pr19405 getnetworkinfo
{
  "version": 209900,
  "subversion": "/Satoshi:0.20.99/",
  "protocolversion": 70015,
  "localservices": "0000000000000409",
  "localservicesnames": [
    "NETWORK",
    "WITNESS",
    "NETWORK_LIMITED"
  ],
  "localrelay": true,
  "timeoffset": 0,
  "networkactive": true,
  "connections": 7,
  "connections_in": 0,
  "connections_out": 7,
  "networks": [
    {
      "name": "ipv4",
      "limited": false,
      "reachable": true,
      "proxy": "",
      "proxy_randomize_credentials": false
    },
    {
      "name": "ipv6",
      "limited": false,
      "reachable": true,
      "proxy": "",
      "proxy_randomize_credentials": false
    },
    {
      "name": "onion",
      "limited": true,
      "reachable": false,
      "proxy": "",
      "proxy_randomize_credentials": false
    }
  ],
  "relayfee": 0.00001000,
  "incrementalfee": 0.00001000,
  "localaddresses": [
  ],
  "warnings": "This is a pre-release test build - use at your own risk - do not use for mining or merchant applications"
}

Verified that the help contains the new fields as well:

$ ./bitcoin-cli -datadir=/tmp/pr19405 help getnetworkinfo
getnetworkinfo
Returns an object containing various state info regarding P2P networking.

Result:
{                                                    (json object)
  "version" : n,                                     (numeric) the server version
  "subversion" : "str",                              (string) the server subversion string
  "protocolversion" : n,                             (numeric) the protocol version
  "localservices" : "hex",                           (string) the services we offer to the network
  "localservicesnames" : [                           (json array) the services we offer to the network, in human-readable form
    "str",                                           (string) the service name
    ...
  ],
  "localrelay" : true|false,                         (boolean) true if transaction relay is requested from peers
  "timeoffset" : n,                                  (numeric) the time offset
  "connections" : n,                                 (numeric) the total number of connections
  "connections_in" : n,                              (numeric) the number of inbound connections
  "connections_out" : n,                             (numeric) the number of outbound connections
  "networkactive" : true|false,                      (boolean) whether p2p networking is enabled
  "networks" : [                                     (json array) information per network
    {                                                (json object)
      "name" : "str",                                (string) network (ipv4, ipv6 or onion)
      "limited" : true|false,                        (boolean) is the network limited using -onlynet?
      "reachable" : true|false,                      (boolean) is the network reachable?
      "proxy" : "str",                               (string) ("host:port") the proxy that is used for this network, or empty if none
      "proxy_randomize_credentials" : true|false     (boolean) Whether randomized credentials are used
    },
    ...
  ],
  "relayfee" : n,                                    (numeric) minimum relay fee for transactions in BTC/kB
  "incrementalfee" : n,                              (numeric) minimum fee increment for mempool limiting or BIP 125 replacement in BTC/kB
  "localaddresses" : [                               (json array) list of local addresses
    {                                                (json object)
      "address" : "str",                             (string) network address
      "port" : n,                                    (numeric) network port
      "score" : n                                    (numeric) relative score
    },
    ...
  ],
  "warnings" : "str"                                 (string) any network and blockchain warnings
}
`

in test/functional/rpc_net.py:105 in 94a792cc19 outdated

  96 | @@ -97,21 +97,27 @@ def _test_getnettotals(self):
  97 |              assert_greater_than_or_equal(after['bytessent_per_msg'].get('ping', 0), before['bytessent_per_msg'].get('ping', 0) + 32)
  98 |  
  99 |      def _test_getnetworkinfo(self):
 100 | -        assert_equal(self.nodes[0].getnetworkinfo()['networkactive'], True)
 101 | -        assert_equal(self.nodes[0].getnetworkinfo()['connections'], 2)
 102 | +        self.log.info("Test getnetworkinfo")

eriknylund commented at 9:36 AM on July 15, 2020:

I'm unsure about what this part is doing, if we just close all connection right after? Could this part be left out?

jonatack commented at 11:03 AM on July 15, 2020:

Thanks for reviewing, @eriknylund -- much appreciated. This code at line 100 adds informational logging to the test output at the start of this _test_getnetworkinfo() test function (e.g. what you see when you run test/functional/rpc_net.py), then updates the rest of the test function to cover the two fields added in this PR while making one less call to getnetworkinfo().

jonatack commented at 11:04 AM on July 15, 2020:

(If I misunderstood your question, let me know.)

eriknylund commented at 12:21 PM on July 15, 2020:

Sorry if my question was unclear I'll try to rephrase it. I realize I should have used the comment function better to hilight all the lines and not only the beginning of the block which was misleading - sorry about that. I'm trying to figure out why the asserts on line 102-105. I think the ones on lines 116-120 are enough to test, as line 110 sets the precondition for this test. Perhaps I'm missing something obvious?

jonatack commented at 1:12 PM on July 15, 2020:

Oh ok. I agree that the test could be marginally improved or refactored, but it is also fine as-is -- if anything, faster than before -- and not worth invalidating review for.

eriknylund commented at 1:36 PM on July 15, 2020:

Thanks for taking your time to explain. I agree it's probably not worth fixing, just wanted to make sure I understood your intent with those lines asserting the entry of this test method - if there's something obvious I'm missing or could learn with regards to how tests are structured and setup here.

eriknylund approved

DrahtBot commented at 9:23 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:

#19771 (net: Replace enum CConnMan::NumConnections with enum class ConnectionDirection by luke-jr)
#17167 (Allow whitelisting outgoing connections by luke-jr)

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.

DrahtBot added the label Needs rebase on Jul 23, 2020

jonatack commented at 5:01 PM on July 23, 2020: member

Current status:

1 Concept ACK
2 "No objections"
2 Tested ACKs

Is anything else needed? The changes are simple and have test coverage.

jonatack closed this on Jul 23, 2020

luke-jr referenced this in commit 2709f14877 on Aug 15, 2020

luke-jr referenced this in commit f97fe2b116 on Aug 15, 2020

jonatack reopened this on Aug 19, 2020

darosior commented at 8:38 AM on August 20, 2020: member

Concept ACK

jonatack force-pushed on Aug 20, 2020

jonatack commented at 9:35 AM on August 20, 2020: member

Rebased and re-opened per IRC discussion yesterday with shesek at http://www.erisian.com.au/bitcoin-core-dev/log-2020-08-19.html#l-375. Proposing with both getnetworkinfo and -getinfo, open to adjust per review feedback.

DrahtBot removed the label Needs rebase on Aug 20, 2020

DrahtBot added the label Needs rebase on Aug 24, 2020

Add in/out connections to rpc getnetworkinfo 1ab49b81cf

UNIX_EPOCH_TIME fixup in rpc getnettotals d9cc13e88d

Add in/out connections to cli -getinfo 581b343d5b

jonatack force-pushed on Aug 24, 2020

jonatack commented at 4:44 PM on August 24, 2020: member

Rebased. Testing and review welcome! :smiling_face_with_three_hearts:

DrahtBot removed the label Needs rebase on Aug 24, 2020

eriknylund commented at 7:24 PM on August 24, 2020: contributor

ACK 581b343d5bf517510ab0236583ca96628751177d

Repeated steps from #19405 (comment) on the re-opened and rebased PR. Tests and extended functional tests pass.

Tested bitcoin-cli -getinfo, bitcoin-cli getnetworkinfo and bitcoin-cli help getnetworkinfo and the output is unchanged from the previous test. Also tested the UNIX epoch time fixup in bitcoin-cli help getnettotals this looks alright as well.

eriknylund commented at 5:57 AM on August 27, 2020: contributor

@shesek have you had a chance to test and review to see if this works for your purposes?

benthecarman commented at 6:33 AM on August 27, 2020: contributor

tACK 581b343

willcl-ark commented at 11:05 AM on August 28, 2020: member

tACK for 581b343d5bf517510ab0236583ca96628751177d, this time rebased onto master at 862fde88be706adb20a211178253636442c3ae00.

shesek commented at 2:42 PM on August 28, 2020: contributor

tACK 581b343. This provides what I needed, thanks!

eriknylund commented at 2:47 PM on August 28, 2020: contributor

tACK 581b343. This provides what I needed, thanks!

Thanks @shesek!

n-thumann commented at 10:35 AM on September 4, 2020: contributor

tACK 581b343 on master at a0a422c, ran unit & functional tests and and confirmed changes on an existing datadir ✌️

n-thumann approved

eriknylund commented at 10:38 AM on September 4, 2020: contributor

tACK 581b343 on master at a0a422c, ran unit & functional tests and and confirmed changes on an existing datadir ✌️

Thanks for reviewing @n-thumann and happy to hear that you tested on an existing datadir, which covers the other side of my test! 👍

laanwj merged this on Sep 4, 2020

laanwj closed this on Sep 4, 2020

jonatack deleted the branch on Sep 4, 2020

sidhujag referenced this in commit 2eceb2b906 on Sep 9, 2020

fanquake referenced this in commit a33651866c on Sep 15, 2020

sidhujag referenced this in commit 7c9738a42a on Sep 15, 2020

Fabcien referenced this in commit fa47e97498 on Sep 23, 2021

DrahtBot locked this on Aug 16, 2022