per the IRC discussion today at http://www.erisian.com.au/bitcoin-core-dev/log-2021-01-19.html#l-87
Network::NET_UNROUTABLE: "not_publicly_routable" instead of "unroutable"
ACK d9ca41964dae2bc58ed8f9639847951beb0f3453
586 | @@ -587,7 +587,7 @@ static RPCHelpMan getnetworkinfo() 587 | { 588 | {RPCResult::Type::OBJ, "", "", 589 | { 590 | - {RPCResult::Type::STR, "name", "network (ipv4, ipv6 or onion)"}, 591 | + {RPCResult::Type::STR, "name", "network (ipv4, ipv6, onion, i2p, cjdns, or not_publicly_routable)"},
unrelated nit: Could return a list of all networks in netbase and call Join(...) here? Would simplify future review if a network is added or the list added to another place.
done, it works but not sure if it's kosher to iterate over the enum instead of using a static, manually updated array
added regression coverage for the generated help
(it seems ok to iterate over the enum per the doc in src/netaddress.h)
* Keep these sequential starting from 0 and `NET_MAX` as the last entry.
* We have loops like `for (int i = 0; i < NET_MAX; i++)` that expect to iterate
* over all enum values and also `GetExtNetwork()` "extends" this enum by
* introducing standalone constants starting from `NET_MAX`.
*/
enum Network
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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.
67 | @@ -68,6 +68,16 @@ std::string GetNetworkName(enum Network net) 68 | assert(false); 69 | } 70 | 71 | +std::vector<std::string> GetNetworkNames() 72 | +{ 73 | + std::vector<std::string> names; 74 | + for (int n = Network::NET_IPV4; n != Network::NET_INTERNAL; ++n) { 75 | + names.emplace_back(GetNetworkName(static_cast<Network>(n))); 76 | + }
nit: I'd slightly prefer a more generic approach here for the loop that doesn't make any assumptions about the position of concrete enums:
for (int n = 0; n < Network::NET_MAX; ++n) {
enum Network network = static_cast<enum Network>(n);
if (network == Network::NET_UNROUTABLE || network == Network::NET_INTERNAL) continue;
names.emplace_back(GetNetworkName(network));
}
(untested)
Yes, that's better. Thanks!
Done, and moved it next to GetNetworksInfo() as the 2 functions are similar.
Updated per review feedback by @MarcoFalke and @theStack and added an optional commit to use named casts (can be dropped if preferred).
light re-ACK 4c97ff24eef77bde3c27258f93e8d216caac9cc8 🦉
Reviewed the second commit via --color-moved=dimmed-zebra to verify that the changes involving GetNetworksInfo were move-only (apart from taking advantage of C++11 initializer list at one place) .
Changes LGTM, also replacing C-style casts with named casts is a good idea to enable compile time checks. My reason for only "light"-ACKing: I'm not 100% sure how dynamic the generation of RPC help texts is allowed to be, other uses of Join(...) in RPC help texts seem only to use constants:
$ git grep RPC.*Join
src/rpc/net.cpp: {RPCResult::Type::STR, "network", "Network (" + Join(GetNetworkNames(), ", ") + ")"},
src/rpc/net.cpp: {RPCResult::Type::STR, "permission_type", Join(NET_PERMISSIONS_DOC, ",\n") + ".\n"},
src/rpc/net.cpp: {RPCResult::Type::STR, "connection_type", "Type of connection: \n" + Join(CONNECTION_TYPE_DOC, ",\n") + ".\n"
src/rpc/net.cpp: {RPCResult::Type::STR, "name", "network (" + Join(GetNetworkNames(), ", ") + ")"},
@theStack thanks for having a look--I asked myself the same question and decided to start with proposing dynamic generation because returning the help doc isn't a perf hotspot and, unlike the two existing cases, this one can be generated automatically (the functional tests enforce a sanity check on any upstream updates).
replacing C-style casts with named casts is a good idea to enable compile time checks
What is an example for a compile time check that is enabled when using those for numeric types? I am only aware of compile time checks that are enabled for pointers. Static casts for numeric values are just overly verbose for no reason and I'd prefer not to change existing code.
What is an example for a compile time check that is enabled when using those for numeric types?
If the destination type is numeric, the named cast asserts at compile time that also the source type is numeric, while a c-style cast doesn't in all cases:
long foo = (long)some_pointer; // compiles happily
long bar = static_cast<long>some_pointer; // "error: invalid ‘static_cast’ from type ‘char*’ to type ‘long int’"
Also note that modern C++ coding guidelines discourage the use of C casts: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-casts-named https://google.github.io/styleguide/cppguide.html#Casting
That said, I'm happy to re-ACK this PR also without the third commit.
244 | - obj.pushKV("minping", ((double)stats.m_min_ping_usec) / 1e6); 245 | + obj.pushKV("minping", static_cast<double>(stats.m_min_ping_usec) / 1e6); 246 | } 247 | if (stats.m_ping_wait_usec > 0) { 248 | - obj.pushKV("pingwait", ((double)stats.m_ping_wait_usec) / 1e6); 249 | + obj.pushKV("pingwait", static_cast<double>(stats.m_ping_wait_usec) / 1e6);
Still not convinced that it is worth it to change existing code that is being removed in #21015
Generally, it might be better to use C++11 non-narrowing casts, or ideally no cast at all.
ok, fair enough on the static_cast, but changing existing code (especially one that is about to get removed) might not be worth it.
Yup, dropped the last commit, it's not related to this change and can be done later when the relevant lines are touched. Thanks for reviewing!
re-ACK 23299def6acda0c07d14274c605dbd4c9d23fe56 🍏
600 | @@ -587,7 +601,7 @@ static RPCHelpMan getnetworkinfo() 601 | { 602 | {RPCResult::Type::OBJ, "", "", 603 | { 604 | - {RPCResult::Type::STR, "name", "network (ipv4, ipv6 or onion)"}, 605 | + {RPCResult::Type::STR, "name", "network (" + Join(GetNetworkNames(), ", ") + ")"},
I don't think getnetworkinfo returns an object for unroutable networks? Also, i2p, etc aren't implemented yet.
Good catch about getnetworkinfo! (and I should have remembered that, too). Fixed both.
574 | @@ -542,25 +575,6 @@ static RPCHelpMan getnettotals() 575 | }; 576 | } 577 | 578 | -static UniValue GetNetworksInfo() 579 | -{ 580 | - UniValue networks(UniValue::VARR); 581 | - for (int n = 0; n < NET_MAX; ++n) { 582 | - enum Network network = static_cast<enum Network>(n);
Any reason to add enum in this move-only? The compiler, as well as the dev already know this is an enum
Oh, nvm you added const, not enum.
left some feedback on 23299def6acda0c07d14274c605dbd4c9d23fe56 🔌
<details><summary>Show signature and timestamp</summary>
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
left some feedback on 23299def6acda0c07d14274c605dbd4c9d23fe56 🔌
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUipnAv/YQrIj8eF8B2E2U0kCmbK1ftlsNowtsHgXUIHRB+1Ow8i3c7ZsjYnQp7Z
CfdGVWgiQ/4UkWIXlimM+8IBkQnkggupIQN4w4/3Gm7i9R3HlC4AMQV+Qov7Jhpn
qPRf2jbt9sUJCYt0tW7sH/TvwZTjJrOkM9wy6PseK8u06bTTWdVWRGjl/bZ7cCt5
RZ0sJ01AQ4y7dDCMgbKPx/t120RdzPwFT/blSLXi0/SIWZNtfX7oXQ+nlKEJ5bIa
jeg/z5P88QkfWsbMbSERycZW3WbFA13o7Um39P/tSdzBzOa8ykXA/fHdJo7g8oC/
SOQk8oqSJuDsL7jEkM78JrBt4AIiIOSuIXnafb0nvZ1ZA9tDoRrxsK7bNV2HZ9nr
fKuy++V7LJsMwRzn6jLeZARkHGDT8RNpTnpDYOiCC5dGfCgxJhwQ/hZ0mcVDrTZd
nGRo63Fe7O3zKnnenNZRHB5m+yfuq0rslIveDgNCeRdmusGrc+UnGAYcxQO++z5J
8PopnI7e
=tod6
-----END PGP SIGNATURE-----
Timestamp of file with hash 27f463ade22a48024739db23d1039da56bed75cd5631f55b35e5df1c8ebcbea7 -
</details>
Updated per git diff 23299de bbb07d1 for @MarcoFalke's feedback (thanks!)
Realized that -onlynet in src/init.cpp should use GetNetworkNames() too. Rebased, as -onlynet was just changed, and updated.
re-ACK 96635e61777add29b6a34d47767a63f43b2919af 🌳 Note that the exact string listing in parantheses changes slightly now, as the "or" part is not there anymore, but I think we can perfectly live with that.
Note that the exact string listing in parantheses changes slightly now, as the "or" part is not there anymore, but I think we can perfectly live with that.
Indeed. I hesitated to do something like
names.emplace_back("or " + GetNetworkName(NET_UNROUTABLE));
review ACK 96635e61777add29b6a34d47767a63f43b2919af 🐗
<details><summary>Show signature and timestamp</summary>
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
review ACK 96635e61777add29b6a34d47767a63f43b2919af 🐗
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUi1nAv8Co9dfz7Kuxi/uUXJ3ob+y0mhUj0d023QFAyW/2Wh/C+QKZu6HRs8Q8Qh
PG+LNvgT+xfvEZb3bYPUUalSmODqU4OkgkulK1YPCr4zxquExcSFkSFkJL8gZzRu
CyduWAzAfGf2I0lGopqgsHMpfs6Q7SYeoEyXRHjztEzZu2FWZMQEuU5APep5e4q9
tw9Qkw1cfScc0rhROs+GbRj6N8AxA0B+GvvKWvc7Y16RfCIaWlP4vKzwN0s18Lzb
JPKRTTKmasfGHkIgnPCKmR2nlu5z8PekfGtvo5wMrRC5fVsFxedgTrtVuaRBk/Jc
IOM7qdVHvp/ClMvbu78Dm9kTwPTs2yFvg9/XcN57vTNQb4Qme9DJiMzOtELvwgaE
xM3xbwskCTldH6CPwMU7QJtmJxHnsNuO0VmtD7BX4QdCpyGXVXwjoYVpN6yn+/BJ
Ad9EKVDN/OCiC+jXiidgQvy6aiUoev1CEH23zvk3RRLXp+HbqNQfLzJfk6deQ5uF
GAJxcJlJ
=v5MP
-----END PGP SIGNATURE-----
Timestamp of file with hash 946105e795bca254ed011a4ffce2cf299168c362a6c99aed2702782f5738dc7f -
</details>
67 | @@ -68,6 +68,20 @@ std::string GetNetworkName(enum Network net) 68 | assert(false); 69 | } 70 | 71 | +std::vector<std::string> GetNetworkNames(bool append_unroutable) 72 | +{ 73 | + std::vector<std::string> names; 74 | + for (int n = 0; n < NET_MAX; ++n) { 75 | + const enum Network network{static_cast<Network>(n)}; 76 | + if (network == NET_UNROUTABLE || network == NET_I2P || network == NET_CJDNS || network == NET_INTERNAL) continue;
Done in 433d9a9f1e416c5518f832b4ca9402c8807877bc net: Do not skip the I2P network from GetNetworkNames(), thanks for the notice!
Tr