net, rpc, test, bugfix: update GetNetworkName, GetNetworksInfo, regression tests #20120

jonatack commented at 10:42 AM on October 11, 2020: member

Following up on the BIP155 addrv2 changes, and starting with 7be6ff6 in #19845, RPC getnetworkinfo began returning networks with empty names.

<details><summary><code>getnetworkinfo</code> on current master</summary><p>

  "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": false,
      "reachable": true,
      "proxy": "127.0.0.1:9050",
      "proxy_randomize_credentials": true
    },
    {
      "name": "",
      "limited": false,
      "reachable": true,
      "proxy": "",
      "proxy_randomize_credentials": false
    },
    {
      "name": "",
      "limited": false,
      "reachable": true,
      "proxy": "",
      "proxy_randomize_credentials": false
    }
  ],

</p></details>

<details><summary><code>getnetworkinfo</code> on this branch</summary><p>

  "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": false,
      "reachable": true,
      "proxy": "127.0.0.1:9050",
      "proxy_randomize_credentials": true
    }
  ],

</p></details>

This patch:

updates GetNetworkName() to the current Network enum
updates getNetworksInfo() to ignore as-yet unsupported networks
adds regression tests

fanquake added the label RPC/REST/ZMQ on Oct 11, 2020

jonatack force-pushed on Oct 11, 2020

DrahtBot commented at 8:26 PM on October 11, 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:

#20254 (Add I2P support using statically configured destinations by vasild)

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.

promag commented at 12:27 AM on October 12, 2020: member

Concept ACK.

How about:

--- a/src/netbase.cpp
+++ b/src/netbase.cpp
@@ -58,7 +58,7 @@ std::string GetNetworkName(enum Network net) {
     case NET_IPV4: return "ipv4";
     case NET_IPV6: return "ipv6";
     case NET_ONION: return "onion";
-    default: return "";
+    default: assert(false);
     }
 }

This is the only place GetNetworkName is called.

jonatack force-pushed on Oct 12, 2020

jonatack commented at 7:57 AM on October 12, 2020: member

Thanks @promag. This aborts, but good idea--I've updated this patch to fix the issue at the netbase level as well. (I'm proposing calling GetNetworkName here as well for getpeerinfo.)

jonatack renamed this:
~~rpc, bugfix: filter networks with empty names from getnetworkinfo~~
net, rpc, test, bugfix: update GetNetworkName, GetNetworksInfo, regression tests
on Oct 12, 2020

in test/functional/feature_proxy.py:225 in 2320bc09d8 outdated

 200 |  
 201 |          if self.have_ipv6:
 202 |              n3 = networks_dict(self.nodes[3].getnetworkinfo())
 203 | -            for net in ['ipv4','ipv6']:
 204 | +            assert_equal(NETWORKS, n3.keys())
 205 | +            for net in NETWORKS:

vasild commented at 11:52 AM on October 12, 2020:

This used to run for just 'ipv4','ipv6' before and now will also run for 'onion' in addition. I guess it is ok as long as the test passes.

jonatack commented at 12:11 PM on October 12, 2020:

Thanks. Yes, that was my thinking: if it passes with the additional coverage, we'll take it.

vasild approved

vasild commented at 11:52 AM on October 12, 2020: member

ACK 2320bc09d

promag commented at 5:27 PM on October 12, 2020: member

Code review ACK 2320bc09d829e6efe26cc07ef388599ffdc99b9e.

Note that #20002 adds another call to GetNetworkName.

fanquake deleted a comment on Oct 12, 2020

hebasto commented at 8:53 AM on October 14, 2020: member

Concept ACK.

Following up on the BIP155 addrv2 changes, and starting with #19991, RPC getnetworkinfo began returning networks with empty names.

From the git log of the master branch it follows that behavior was changed in 7be6ff61875a8d5d2335bff5d1f16ba40557adb0 from #19845. @jonatack if you could confirm this, mind updating the OP?

in src/netbase.cpp:68 in 49f7be8396 outdated

  63 | +    case NET_I2P: return "i2p";
  64 | +    case NET_CJDNS: return "cjdns";
  65 | +    case NET_INTERNAL: return "internal";
  66 | +    case NET_MAX: assert(false);
  67 |      }
  68 | +    assert(false);

hebasto commented at 9:03 AM on October 14, 2020:

49f7be8396d85b1795c65ca5ebf6069c6c1e047e

Mind following Developer Notes -- C++ data structures:

    case NET_MAX: assert(false);
    } // no default case, so the compiler can warn about missing cases
    assert(false);

?

hebasto commented at 9:23 AM on October 14, 2020:

ofc, it's not a blocker :)

jonatack commented at 9:24 AM on October 14, 2020:

Thanks for reviewing--I don't mind, but I hesitate to invalidate 3 reviews for a non-essential code comment. Can add it in another PR.

jonatack commented at 5:13 PM on October 15, 2020:

Had to rebase, so updating with your feedback. Thanks!

hebasto approved

hebasto commented at 9:13 AM on October 14, 2020: member

ACK 2320bc09d829e6efe26cc07ef388599ffdc99b9e, tested on Linux Mint 20 (x86_64).

jonatack commented at 9:56 AM on October 14, 2020: member

From the git log of the master branch it follows that behavior was changed in 7be6ff6 from #19845. @jonatack if you could confirm this, mind updating the OP?

Updated, thanks. This makes sense. I likely tested incorrectly.

jonatack force-pushed on Oct 15, 2020

net: update GetNetworkName() with all enum Network cases ba8997fb2e

rpc: update GetNetworksInfo() to not return unsupported networks 9a75e1e569

test: add getnetworkinfo network name regression tests 7b5bd3102e

jonatack force-pushed on Oct 15, 2020

jonatack commented at 5:24 PM on October 15, 2020: member

Rebased and updated per git range-diff 0d22482 2320bc0 7b5bd31 after merge of #20002.

hebasto approved

hebasto commented at 7:00 PM on October 15, 2020: member

re-ACK 7b5bd3102e06f7ff34b5d0f1d45a005560f265a5

laanwj added this to the milestone 0.21.0 on Oct 15, 2020

luke-jr approved

luke-jr commented at 6:29 PM on October 24, 2020: member

utACK

vasild approved

vasild commented at 10:24 AM on October 26, 2020: member

ACK 7b5bd3102

in test/functional/feature_proxy.py:48 in 7b5bd3102e

  42 | @@ -41,12 +43,16 @@
  43 |  from test_framework.netutil import test_ipv6_local
  44 |  
  45 |  RANGE_BEGIN = PORT_MIN + 2 * PORT_RANGE  # Start after p2p and rpc ports
  46 | -# From GetNetworkName() in netbase.cpp:
  47 | -NET_UNROUTABLE = ""
  48 | +
  49 | +# Networks returned by RPC getpeerinfo, defined in src/netbase.cpp::GetNetworkName()
  50 | +NET_UNROUTABLE = "unroutable"

promag commented at 12:13 PM on November 7, 2020:

This is not used?

jonatack commented at 4:16 PM on November 9, 2020:

It's used below, line 171: self.network_test(node, addr, network=NET_UNROUTABLE)

laanwj merged this on Nov 9, 2020

laanwj closed this on Nov 9, 2020

jonatack deleted the branch on Nov 9, 2020

sidhujag referenced this in commit 55fb188668 on Nov 9, 2020

random-zebra referenced this in commit b4751e10ce on Aug 11, 2021

Fabcien referenced this in commit c50d2ac071 on Dec 22, 2021

DeckerSU referenced this in commit c26f71f1ac on Feb 12, 2022

DrahtBot locked this on Feb 15, 2022