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	fjahr, laanwj, 0xB10C
Stale 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

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

achow101 requested review from fjahr on Oct 15, 2024

achow101 requested review from 0xB10C on Oct 15, 2024

achow101 requested review from laanwj on Oct 15, 2024

achow101 requested review from sr-gi on Oct 15, 2024

laanwj commented at 9:43 am on October 16, 2024: member

Code and doc review ACK b33eb137e39c434a7be69e1453a708b0c52553c4

laanwj approved

in src/rpc/net.cpp:132 in b33eb137e3 outdated

128@@ -129,8 +129,8 @@ static RPCHelpMan getpeerinfo()
129                     {RPCResult::Type::STR, "addrbind", /*optional=*/true, "(ip:port) Bind address of the connection to the peer"},
130                     {RPCResult::Type::STR, "addrlocal", /*optional=*/true, "(ip:port) Local address as reported by the peer"},
131                     {RPCResult::Type::STR, "network", "Network (" + Join(GetNetworkNames(/*append_unroutable=*/true), ", ") + ")"},
132-                    {RPCResult::Type::NUM, "mapped_as", /*optional=*/true, "The AS in the BGP route to the peer used for diversifying\n"
133-                                                        "peer selection (only available if the asmap config flag is set)"},
134+                    {RPCResult::Type::NUM, "mapped_as", /*optional=*/true, "Mapped AS (Autonomous System) number in the BGP route to the peer, used for diversifying\n"

fjahr commented at 1:02 pm on October 16, 2024:

nit: I am not super happy with the wording that the help text here is converged to but we can keep it since I think everyone who understands asmap and BGP knows what is meant by this. The part I don’t like is “in the BGP route to the peer” because a BGP route in a routing table includes several ASNs (the actual route), just the last one is the ASN which controls the IP and that is the mapping that we have in ASMap. Better wordings would be “at the end of the BGP route” or (closer to the old text in the other RPCs) “the ASN controlling the IP”.

0xB10C commented at 12:24 pm on October 17, 2024:

(I agree that the wording isn’t perfect, but think this can be merged as is)

jonatack commented at 2:32 pm on October 17, 2024:

“at the end of the BGP route to the peer” in all of them (including -netinfo as well) seems good

laanwj commented at 0:37 am on October 21, 2024:

agree that that’s clearer; it may not always be (due to ASmap being an approximation, as well as incomplete information) but the AS number is supposed ot be the end-point AS, not just one along the route

brunoerg commented at 10:00 am on October 21, 2024:

I agree, but since we have some ACKs here, I can leave it for another PR or in case I need to touch it again.

laanwj commented at 11:35 am on October 21, 2024:

yes i am a bit divided about this, i understand you don’t want to invalidate ACKs last-minute, but on the other hand re-reviewing after a trivial change is super easy, and if we’re touching this anyway it’s good to make sure it’s the best possible documentation

brunoerg commented at 11:40 am on October 21, 2024:

No problem, so I’m gonna address it here.

brunoerg commented at 1:17 pm on October 21, 2024:

Done.

fjahr commented at 1:04 pm on October 16, 2024: contributor

utACK b33eb137e39c434a7be69e1453a708b0c52553c4

0xB10C approved

0xB10C commented at 12:23 pm on October 17, 2024: contributor

Code Review ACK b33eb137e39c434a7be69e1453a708b0c52553c4

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

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>

a16917fb59

brunoerg force-pushed on Oct 21, 2024

brunoerg commented at 1:50 pm on October 21, 2024: contributor

Force-pushed addressing #30183 (review)

fjahr commented at 3:05 pm on October 21, 2024: contributor

re-ACK a16917fb5981d1465ffd4c036586f8729e683b73

Thanks for addressing it!

DrahtBot requested review from jonatack on Oct 21, 2024

DrahtBot requested review from 0xB10C on Oct 21, 2024

DrahtBot requested review from laanwj on Oct 21, 2024

jonatack commented at 3:14 pm on October 21, 2024: member

LGTM but the commit now needs renaming (perhaps “Improve mapped AS documentation”) and can touch up this doc in the same commit:

 0--- a/src/netgroup.h
 1+++ b/src/netgroup.h
 2@@ -35,9 +35,9 @@ public:
 3     std::vector<unsigned char> GetGroup(const CNetAddr& address) const;
 4 
 5     /**
 6-     *  Get the autonomous system on the BGP path to address.
 7+     *  Get the autonomous system number at the end of the BGP path to the IP address.
 8      *
 9-     *  The ip->AS mapping depends on how asmap is constructed.
10+     *  The IP->AS mapping depends on how the AS map is constructed.
11      */
12     uint32_t GetMappedAS(const CNetAddr& address) const;

DrahtBot requested review from jonatack on Oct 21, 2024

laanwj approved

laanwj commented at 6:31 pm on October 21, 2024: member

thanks for updating ! re-ACK a16917fb5981d1465ffd4c036586f8729e683b73

0xB10C commented at 8:10 am on October 22, 2024: contributor

re-ACK a16917fb5981d1465ffd4c036586f8729e683b73

fanquake merged this on Oct 22, 2024

fanquake closed this on Oct 22, 2024

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

Code Coverage

Reviews