net: add ASMap info in `getrawaddrman` RPC

brunoerg commented at 12:10 pm on May 8, 2024: contributor

This PR adds two new fields in getrawaddrman RPC: “mapped_as” and “source_mapped_as”. These fields are used to return the ASN (Autonomous System Number) mapped to the peer and its source. With these informations we can have a better view of the bucketing logic with ASMap specially in projects like addrman-observer.

DrahtBot commented at 12:10 pm on May 8, 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, 0xB10C, virtu, glozow

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 P2P on May 8, 2024

brunoerg commented at 12:10 pm on May 8, 2024: contributor

ping @0xB10C

0xB10C referenced this in commit a455ae900f on May 8, 2024

0xB10C referenced this in commit bad32e30b3 on May 8, 2024

0xB10C commented at 2:20 pm on May 8, 2024: contributor

Cool! Concept ACK.

I’ve added prelimarly support for this in the static HTML page on https://addrman.observer. You can now load the new getrawaddrman dumps and it will show you the AS and source AS. You can also mouse-over highlight by AS and source ASN.

in src/rpc/net.cpp:1107 in f80c47c3e8 outdated

1102@@ -1103,10 +1103,12 @@ UniValue AddrmanEntryToJSON(const AddrInfo& info)
1103     ret.pushKV("network", GetNetworkName(info.GetNetClass()));
1104     ret.pushKV("source", info.source.ToStringAddr());
1105     ret.pushKV("source_network", GetNetworkName(info.source.GetNetClass()));
1106+    ret.pushKV("mapped_as", connman.GetMappedAS(info));
1107+    ret.pushKV("source_mapped_as", connman.GetMappedAS(info.source));

0xB10C commented at 2:23 pm on May 8, 2024:

nit: I always found this confusing as I read mapped as <something> and not mapped *A*utonomous*S*ystem - don’t have a concrete idea for a name and I guess it makes sense to use the same as in getpeerinfo.

brunoerg commented at 5:54 pm on May 8, 2024:

I wanted to keep same name as in getpeerinfo but I agree with you about the confusion, I do believe mapped_asn would be better because we show the AS number.

brunoerg commented at 3:21 pm on May 9, 2024:

I will leave as is for now.

virtu commented at 9:11 am on May 21, 2024:

nit: Looking at my node’s getrawaddrman output, it felt odd to have mapped_as appear after source. IMO, the fields become more intuitively understandable when put right after or at least close to the address they refer to.

brunoerg commented at 9:28 pm on May 21, 2024:

I agree, will address it.

brunoerg commented at 4:18 pm on May 9, 2024: contributor

cc: @virtu @fjahr

luke-jr referenced this in commit df458672e7 on May 13, 2024

virtu commented at 9:39 am on May 21, 2024: contributor

Concept ACK

Also did some light testing:

Some sanity checks on getrawaddrman output (everything mapped to zero when asmap is disabled; almost all addresses mapped to non-zero when enabled)
Functional tests

in src/rpc/net.cpp:1144 in 3733f6b811 outdated

1140@@ -1139,6 +1141,8 @@ static RPCHelpMan getrawaddrman()
1141                         {RPCResult::Type::NUM_TIME, "time", "The " + UNIX_EPOCH_TIME + " when the node was last seen"},
1142                         {RPCResult::Type::STR, "source", "The address that relayed the address to us"},
1143                         {RPCResult::Type::STR, "source_network", "The network (" + Join(GetNetworkNames(), ", ") + ") of the source address"},
1144+                        {RPCResult::Type::NUM, "mapped_as", "The AS in the BGP route mapped to the node (0 if not mapped)"},

fjahr commented at 12:46 pm on May 21, 2024:

I would prefer these to be optional results rather than a magic 0 value.

fjahr commented at 12:50 pm on May 21, 2024:

nit, I think this wording (or something like it) would be more clear: “The ASN mapped to the IP of this peer per our current ASMap.” (and something corresponding to this below).

fjahr commented at 12:56 pm on May 21, 2024:

I would prefer these to be optional results rather than a magic 0 value.

(if there is no mapping of course)

brunoerg commented at 9:28 pm on May 21, 2024:

I agree, I will address it.

brunoerg commented at 9:14 am on May 22, 2024:

Done

fjahr commented at 12:54 pm on May 21, 2024: contributor

Concept ACK

brunoerg force-pushed on May 22, 2024

brunoerg commented at 9:14 am on May 22, 2024: contributor

Force-pushed:

Changed mapped_as and source_mapped_as to optional.
Changed description of the fields as suggested by @fjahr.
Reordered these new fields (https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1607941908)

in src/rpc/net.cpp:1151 in 2de765c250 outdated

1146                         {RPCResult::Type::STR, "network", "The network (" + Join(GetNetworkNames(), ", ") + ") of the address"},
1147                         {RPCResult::Type::NUM, "services", "The services offered by the node"},
1148                         {RPCResult::Type::NUM_TIME, "time", "The " + UNIX_EPOCH_TIME + " when the node was last seen"},
1149                         {RPCResult::Type::STR, "source", "The address that relayed the address to us"},
1150                         {RPCResult::Type::STR, "source_network", "The network (" + Join(GetNetworkNames(), ", ") + ") of the source address"},
1151+                        {RPCResult::Type::NUM, "source_mapped_as", /*optional=*/true, "The ASN mapped to the IP of this peer's source per our current ASMap."}

fjahr commented at 9:16 am on May 22, 2024:

nit: This comment has a dot at the end while our convention seems to be to not add a dot.

brunoerg commented at 10:58 am on May 22, 2024:

done

in test/functional/feature_asmap.py:29 in 2de765c250 outdated

25@@ -26,6 +26,7 @@
26 import os
27 import shutil
28 
29+from test_framework.util import assert_equal

fjahr commented at 9:20 am on May 22, 2024:

nit: By alphabetical ordering this would go below the following line

brunoerg commented at 10:58 am on May 22, 2024:

done

fjahr commented at 9:23 am on May 22, 2024: contributor

Code review ACK 2de765c25017e566777d927d86452d96082592a1

Feel free to ignore the nits but I can re-review quickly if you address them.

DrahtBot requested review from 0xB10C on May 22, 2024

DrahtBot requested review from virtu on May 22, 2024

net: rpc: return peer's mapped AS in getrawaddrman

This information can be used to check bucketing
logic.

8c2714907d

test: add coverage for `mapped_as` from `getrawaddrman`

Test addresses are being mapped according to the ASMap
file provided properly. Compare the result of the `getrawaddrman`
RPC with the result from the ASMap Health Check.

1e54d61c46

brunoerg force-pushed on May 22, 2024

fjahr commented at 12:34 pm on May 22, 2024: contributor

Code review ACK 1e54d61c4698debf3329d1960e06078ccbf8063c

0xB10C approved

0xB10C commented at 4:20 pm on May 22, 2024: contributor

ACK 1e54d61c4698debf3329d1960e06078ccbf8063c

I’ve played around with the getrawaddrman RPC with and without an asmap file loaded. Also loaded a json dump into addrman-observer. Works as expected.

virtu commented at 9:02 am on May 23, 2024: contributor

ACK 1e54d61

Successfully re-ran all previously mentioned tests.

in test/functional/feature_asmap.py:123 in 1e54d61c46

118@@ -118,6 +119,14 @@ def test_asmap_health_check(self):
119         msg = "ASMap Health Check: 4 clearnet peers are mapped to 3 ASNs with 0 peers being unmapped"
120         with self.node.assert_debug_log(expected_msgs=[msg]):
121             self.start_node(0, extra_args=['-asmap'])
122+        raw_addrman = self.node.getrawaddrman()
123+        asns = []

glozow commented at 11:03 am on May 23, 2024:

nit: this could have been a set?

glozow commented at 11:06 am on May 23, 2024: member

ACK 1e54d61c4698debf3329d1960e06078ccbf8063c

glozow merged this on May 23, 2024

glozow closed this on May 23, 2024

in src/rpc/net.cpp:1096 in 1e54d61c46

1092@@ -1093,20 +1093,28 @@ static RPCHelpMan getaddrmaninfo()
1093     };
1094 }
1095 
1096-UniValue AddrmanEntryToJSON(const AddrInfo& info)
1097+UniValue AddrmanEntryToJSON(const AddrInfo& info, CConnman& connman)

jonatack commented at 8:36 pm on May 23, 2024:

Why is connman passed by reference, and not reference to const? It isn’t modified.

brunoerg commented at 9:00 pm on May 23, 2024:

Makes sense, I will address it in a follow-up PR.

in src/rpc/net.cpp:1117 in 1e54d61c46

1114+    }
1115     return ret;
1116 }
1117 
1118-UniValue AddrmanTableToJSON(const std::vector<std::pair<AddrInfo, AddressPosition>>& tableInfos)
1119+UniValue AddrmanTableToJSON(const std::vector<std::pair<AddrInfo, AddressPosition>>& tableInfos, CConnman& connman)

jonatack commented at 8:36 pm on May 23, 2024:

Same comment here as https://github.com/bitcoin/bitcoin/pull/30062/files#r1612272793

in src/rpc/net.cpp:1101 in 1e54d61c46

1097+UniValue AddrmanEntryToJSON(const AddrInfo& info, CConnman& connman)
1098 {
1099     UniValue ret(UniValue::VOBJ);
1100     ret.pushKV("address", info.ToStringAddr());
1101+    const auto mapped_as{connman.GetMappedAS(info)};
1102+    if (mapped_as != 0) {

jonatack commented at 8:39 pm on May 23, 2024:

Using auto here requires looking up GetMappedAS to know the type (at least, for people using simple editors and not fancier IDEs). Using the actual type in this case would have been clearer to the reader. The conditional could also have been simpler, though that’s arguably a style nit.

0-    const auto mapped_as{connman.GetMappedAS(info)};
1-    if (mapped_as != 0) {
2+    const uint32_t mapped_as{connman.GetMappedAS(info)};
3+    if (mapped_as) {

in src/rpc/net.cpp:1111 in 1e54d61c46

1107     ret.pushKV("time", int64_t{TicksSinceEpoch<std::chrono::seconds>(info.nTime)});
1108     ret.pushKV("network", GetNetworkName(info.GetNetClass()));
1109     ret.pushKV("source", info.source.ToStringAddr());
1110     ret.pushKV("source_network", GetNetworkName(info.source.GetNetClass()));
1111+    const auto source_mapped_as{connman.GetMappedAS(info.source)};
1112+    if (source_mapped_as != 0) {

jonatack commented at 8:39 pm on May 23, 2024:

Same comment here as https://github.com/bitcoin/bitcoin/pull/30062/files#r1612276581.

in src/rpc/net.cpp:1144 in 1e54d61c46

1140@@ -1133,12 +1141,14 @@ static RPCHelpMan getrawaddrman()
1141                 {RPCResult::Type::OBJ_DYN, "table", "buckets with addresses in the address manager table ( new, tried )", {
1142                     {RPCResult::Type::OBJ, "bucket/position", "the location in the address manager table (<bucket>/<position>)", {
1143                         {RPCResult::Type::STR, "address", "The address of the node"},
1144+                        {RPCResult::Type::NUM, "mapped_as", /*optional=*/true, "The ASN mapped to the IP of this peer per our current ASMap"},

jonatack commented at 8:42 pm on May 23, 2024:

Here and in the other new help below, the reason for the optional nature of this field could perhaps be mentioned (as simply as , if present, or perhaps in more detail).

0                        {RPCResult::Type::NUM, "mapped_as", /*optional=*/true, "The ASN mapped to the IP of this peer per our current ASMap, if present"},

brunoerg commented at 9:07 pm on May 23, 2024: contributor

Thanks for reviewing, @jonatack. I’m working on a follow-up.

net: add ASMap info in getrawaddrman RPC #30062

Code Coverage

Reviews

net: add ASMap info in `getrawaddrman` RPC #30062