rpc: net: follow-ups for #30062

brunoerg commented at 1:03 pm on May 27, 2024: contributor

Change addrman to reference to const since it isn’t modified (https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1612272793).
Improve documentation of mapped_as/source_mapped_as in getrawaddrman RPC by mentioning that both fields will be only available if asmap flag is set. It is the same message for mapped_as field in getpeerinfo.

DrahtBot commented at 1:03 pm on May 27, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	jonatack

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label RPC/REST/ZMQ on May 27, 2024

in src/rpc/net.cpp:1150 in eeaaac49a9 outdated

1146@@ -1147,14 +1147,14 @@ static RPCHelpMan getrawaddrman()
1147                 {RPCResult::Type::OBJ_DYN, "table", "buckets with addresses in the address manager table ( new, tried )", {
1148                     {RPCResult::Type::OBJ, "bucket/position", "the location in the address manager table (<bucket>/<position>)", {
1149                         {RPCResult::Type::STR, "address", "The address of the node"},
1150-                        {RPCResult::Type::NUM, "mapped_as", /*optional=*/true, "The ASN mapped to the IP of this peer per our current ASMap"},
1151+                        {RPCResult::Type::NUM, "mapped_as", /*optional=*/true, "The ASN mapped to the IP of this peer per our current ASMap (only available if the asmap config flag is set)"},

luke-jr commented at 11:05 pm on May 28, 2024:

Last I checked, we require more than just a flag?

brunoerg commented at 11:27 pm on May 28, 2024:

Just need to set the flag with a valid asmap file.

jonatack commented at 4:16 am on May 29, 2024:

Perhaps use the -netinfo help doc (here and line 1157 below):

0Mapped AS (Autonomous System) number in the BGP route to the peer, used for diversifying
1peer selection (only displayed if the -asmap config option is set)

(the getpeerinfo help below is similar but the -netinfo one seems more complete)

0The AS in the BGP route to the peer used for diversifying
1peer selection (only available if the asmap config flag is set)

brunoerg commented at 11:14 am on May 29, 2024:

Much better, I will update it. I think it’s worth updating getpeerinfo too.

brunoerg commented at 6:09 pm on May 29, 2024:

done

jonatack commented at 4:20 am on May 29, 2024: member

Concept ACK, perhaps take #30062 (review) too if you agree. These could be one commit, i.e. rpc: getrawaddrman "mapped_as" follow-ups

brunoerg force-pushed on May 29, 2024

brunoerg commented at 6:10 pm on May 29, 2024: contributor

Force-pushed addressing #30183 (review) and #30183#pullrequestreview-2084228631.

luke-jr commented at 6:40 pm on May 29, 2024: member

Not sure the asmap feature cares if your map is based on BGP or something else?

DrahtBot added the label CI failed on May 31, 2024

DrahtBot removed the label CI failed on May 31, 2024

brunoerg commented at 6:58 pm on June 4, 2024: contributor

Not sure the asmap feature cares if your map is based on BGP or something else?

AFAIK I think it doesn’t care, but it’s the expected. Which alternatives do you see?

luke-jr referenced this in commit 81566fa584 on Jun 13, 2024

luke-jr referenced this in commit 2c73e88c96 on Jun 13, 2024

DrahtBot added the label CI failed on Jul 2, 2024

DrahtBot removed the label CI failed on Jul 3, 2024

brunoerg requested review from jonatack on Jul 12, 2024

jonatack commented at 2:57 pm on July 12, 2024: member

@brunoerg thanks for the ping! Reviewing soon.

DrahtBot added the label CI failed on Aug 5, 2024

DrahtBot removed the label CI failed on Aug 10, 2024

in src/rpc/net.cpp:1157 in 53f38f6fce outdated

1154                         {RPCResult::Type::NUM, "services", "The services offered by the node"},
1155                         {RPCResult::Type::NUM_TIME, "time", "The " + UNIX_EPOCH_TIME + " when the node was last seen"},
1156                         {RPCResult::Type::STR, "source", "The address that relayed the address to us"},
1157                         {RPCResult::Type::STR, "source_network", "The network (" + Join(GetNetworkNames(), ", ") + ") of the source address"},
1158-                        {RPCResult::Type::NUM, "source_mapped_as", /*optional=*/true, "The ASN mapped to the IP of this peer's source per our current ASMap"}
1159+                        {RPCResult::Type::NUM, "source_mapped_as", /*optional=*/true, "Mapped AS (Autonomous System) number in the BGP route to the peer's source, used for diversifying peer selection (only displayed if the -asmap config option is set)"}

jonatack commented at 7:11 pm on August 28, 2024:

53f38f6fceec3581943 nit, “peer” is slightly confusing to me here, as the getrawaddrman help uses the word “node” instead. Just “source” as used in the preceding 2 fields might be clearer.

0                        {RPCResult::Type::NUM, "source_mapped_as", /*optional=*/true, "Mapped AS (Autonomous System) number in the BGP route to the source, used for diversifying peer selection (only displayed if the -asmap config option is set)"}

brunoerg commented at 9:25 pm on August 28, 2024:

Sounds good. I will address it.

jonatack commented at 7:12 pm on August 28, 2024: member

ACK 53f38f6fceec35819439e4b8491c4d7b700bf33

Sorry for the delay. Happy to re-review if you prefer the suggestion.

rpc, net: getrawaddrman "mapped_as" follow-ups bdad0243be

rpc, net: improve `mapped_as` doc for getrawaddrman/getpeerinfo

Use same text from `-netinfo` help doc which is more
complete and clear. Before, we did not explicity say that
both fields `{source_}mapped_as` (that are optional in
getrawaddrman) will be only available if the asmap config
flag is set.

Co-authored-by: Jon Atack <jon@atack.com>

b33eb137e3

brunoerg force-pushed on Aug 28, 2024

brunoerg commented at 9:29 pm on August 28, 2024: contributor

Force-pushed addressing #30183 (review)

jonatack commented at 9:46 pm on August 28, 2024: member

Thanks for updating!

re-ACK b33eb137e39c434a7be69e1453a708b0c52553c4

rpc: net: follow-ups for #30062 #30183

Code Coverage

Reviews