net, rpc: return NET_UNROUTABLE as not_publicly_routable, automate helps by jonatack · Pull Request #20965 · bitcoin/bitcoin

jonatack · 2021-01-19T14:43:46Z

per the IRC discussion today at http://www.erisian.com.au/bitcoin-core-dev/log-2021-01-19.html#l-87

return a more helpful string name for Network::NET_UNROUTABLE: "not_publicly_routable" instead of "unroutable"
update the RPC getpeerinfo "network" help, and automate it and the getnetworkinfo "network#name" and the -onlynet help doc generation

theStack

ACK d9ca41964dae2bc58ed8f9639847951beb0f3453

src/rpc/net.cpp

DrahtBot · 2021-01-29T02:30:11Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Add I2P support using I2P SAM #20685 (Add I2P support using I2P SAM by vasild)
net: Make DNS lookup mockable, add fuzzing harness #19415 (net: Make DNS lookup mockable, add fuzzing harness by practicalswift)

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/netbase.cpp

jonatack · 2021-01-29T15:07:34Z

Updated per review feedback by @MarcoFalke and @theStack and added an optional commit to use named casts (can be dropped if preferred).

theStack

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(), ", ") + ")"},

jonatack · 2021-01-30T20:58:20Z

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

maflcko · 2021-01-31T08:24:05Z

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.

theStack · 2021-01-31T12:34:50Z

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.

maflcko

ok, fair enough on the static_cast, but changing existing code (especially one that is about to get removed) might not be worth it.

src/rpc/net.cpp

jonatack · 2021-01-31T19:56:16Z

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!

theStack

re-ACK 23299def6acda0c07d14274c605dbd4c9d23fe56 🍏

maflcko

left some feedback on 23299def6acda0c07d14274c605dbd4c9d23fe56 🔌

Show signature and timestamp

Signature:

Timestamp of file with hash 27f463ade22a48024739db23d1039da56bed75cd5631f55b35e5df1c8ebcbea7 -

src/rpc/net.cpp

jonatack · 2021-02-01T11:26:55Z

Updated per git diff 23299de bbb07d1 for @MarcoFalke's feedback (thanks!)

jonatack · 2021-02-01T23:21:50Z

Realized that -onlynet in src/init.cpp should use GetNetworkNames() too. Rebased, as -onlynet was just changed, and updated.

theStack

re-ACK 96635e6 🌳
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.

jonatack · 2021-02-12T23:08:12Z

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));

maflcko

review ACK 96635e6 🐗

Show signature and timestamp

Signature:

Timestamp of file with hash 946105e795bca254ed011a4ffce2cf299168c362a6c99aed2702782f5738dc7f -

jonatack · 2021-02-15T14:45:35Z

src/netbase.cpp

+    std::vector<std::string> names;
+    for (int n = 0; n < NET_MAX; ++n) {
+        const enum Network network{static_cast<Network>(n)};
+        if (network == NET_UNROUTABLE || network == NET_I2P || network == NET_CJDNS || network == NET_INTERNAL) continue;


@vasild you may want to update this line to remove NET_I2P (and add it to the rpc_net.py tests) in #20685 (if you're inclined to do it there)

Done in 433d9a9f1e416c5518f832b4ca9402c8807877bc net: Do not skip the I2P network from GetNetworkNames(), thanks for the notice!

…routable, automate helps 96635e6 init: use GetNetworkNames() in -onlynet help (Jon Atack) 0dbde70 rpc: use GetNetworkNames() in getnetworkinfo and getpeerinfo helps (Jon Atack) 1c3af37 net: create GetNetworkNames() (Jon Atack) b45eae4 net: update NET_UNROUTABLE to not_publicly_routable in GetNetworkName() (Jon Atack) Pull request description: per the IRC discussion today at http://www.erisian.com.au/bitcoin-core-dev/log-2021-01-19.html#l-87 - return a more helpful string name for `Network::NET_UNROUTABLE`: "not_publicly_routable" instead of "unroutable" - update the RPC getpeerinfo "network" help, and automate it and the getnetworkinfo "network#name" and the -onlynet help doc generation ACKs for top commit: theStack: re-ACK 96635e6 🌳 MarcoFalke: review ACK 96635e6 🐗 Tree-SHA512: 511a7d987126b48a7a090739aa7c4964b6186a3ff8f5f7eec9233dd816c6b7a6dc91b3ea6b824aa68f218a8a3ebdc6ffd214e9a88af38f2bf23f3257c4284c3a

jonatack · 2021-02-15T17:51:39Z

@shesek done, per your original IRC issue getpeerinfo's network field returns (and documents in the help) the Network::NET_UNROUTABLE type as "not_publicly_routable" (master/v22.0)

trongham

Tr

Github-Pull: bitcoin#20965 Rebased-From: 1c3af37

Summary: > net: update NET_UNROUTABLE to not_publicly_routable in GetNetworkName() > net: create GetNetworkNames() > rpc: use GetNetworkNames() in getnetworkinfo and getpeerinfo helps > init: use GetNetworkNames() in -onlynet help This is a backport of [[bitcoin/bitcoin#20965 | core#20965]] Depends on D11030 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D11036

jonatack force-pushed the update-network-info branch from 2c25e6d to d9ca419 Compare January 19, 2021 15:05

DrahtBot added P2P RPC/REST/ZMQ labels Jan 19, 2021

theStack approved these changes Jan 28, 2021

View reviewed changes

maflcko reviewed Jan 28, 2021

View reviewed changes

src/rpc/net.cpp Outdated Show resolved Hide resolved

jonatack force-pushed the update-network-info branch from d9ca419 to 181fe06 Compare January 29, 2021 01:20

DrahtBot mentioned this pull request Jan 29, 2021

net: Make DNS lookup mockable, add fuzzing harness #19415

Merged

jonatack force-pushed the update-network-info branch from 181fe06 to b0ed5b1 Compare January 29, 2021 12:06

theStack reviewed Jan 29, 2021

View reviewed changes

src/netbase.cpp Outdated Show resolved Hide resolved

jonatack force-pushed the update-network-info branch from b0ed5b1 to 355ca86 Compare January 29, 2021 15:05

jonatack force-pushed the update-network-info branch from 355ca86 to 4c97ff2 Compare January 29, 2021 15:16

This was referenced Jan 29, 2021

Make all of net_processing (and some of net) use std::chrono types #21015

Merged

Net: Move ping data to net_processing #20721

Merged

Add I2P support using I2P SAM #20685

Merged

theStack approved these changes Jan 30, 2021

View reviewed changes

maflcko reviewed Jan 31, 2021

View reviewed changes

src/rpc/net.cpp Outdated Show resolved Hide resolved

jonatack force-pushed the update-network-info branch from 4c97ff2 to 23299de Compare January 31, 2021 19:54

theStack approved these changes Jan 31, 2021

View reviewed changes

maflcko reviewed Feb 1, 2021

View reviewed changes

src/rpc/net.cpp Outdated Show resolved Hide resolved

src/rpc/net.cpp Outdated Show resolved Hide resolved

jonatack force-pushed the update-network-info branch 2 times, most recently from fbc694a to bbb07d1 Compare February 1, 2021 11:23

jonatack added 2 commits February 2, 2021 00:00

net: update NET_UNROUTABLE to not_publicly_routable in GetNetworkName()

b45eae4

net: create GetNetworkNames()

1c3af37

jonatack added 2 commits February 2, 2021 00:16

rpc: use GetNetworkNames() in getnetworkinfo and getpeerinfo helps

0dbde70

init: use GetNetworkNames() in -onlynet help

96635e6

jonatack force-pushed the update-network-info branch from bbb07d1 to 96635e6 Compare February 1, 2021 23:19

jonatack changed the title ~~net, rpc: return NET_UNROUTABLE as not_publicly_routable, update rpc helps~~ net, rpc: return NET_UNROUTABLE as not_publicly_routable, automate helps Feb 1, 2021

theStack approved these changes Feb 12, 2021

View reviewed changes

maflcko reviewed Feb 15, 2021

View reviewed changes

maflcko merged commit 489030f into bitcoin:master Feb 15, 2021

jonatack deleted the update-network-info branch February 15, 2021 14:39

jonatack commented Feb 15, 2021

View reviewed changes

trongham reviewed Feb 15, 2021

View reviewed changes

jonatack mentioned this pull request Apr 6, 2021

rpc: add network field to getnodeaddresses #21594

Merged

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 27, 2021

net: create GetNetworkNames()

4c35f03

Github-Pull: bitcoin#20965 Rebased-From: 1c3af37

jonatack mentioned this pull request Nov 24, 2021

refactor: Make CWalletTx sync state type-safe #21206

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Conversation

jonatack commented Jan 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Jan 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

jonatack commented Jan 29, 2021

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack commented Jan 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jan 31, 2021

Uh oh!

theStack commented Jan 31, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonatack commented Jan 31, 2021

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jonatack commented Feb 1, 2021

Uh oh!

jonatack commented Feb 1, 2021

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack commented Feb 12, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack Feb 15, 2021

Choose a reason for hiding this comment

Uh oh!

vasild Feb 15, 2021

Choose a reason for hiding this comment

Uh oh!

jonatack commented Feb 15, 2021

Uh oh!

trongham left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jonatack commented Jan 19, 2021 •

edited

Loading

DrahtBot commented Jan 29, 2021 •

edited

Loading

jonatack commented Jan 30, 2021 •

edited

Loading