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.
net: add ASMap info in getrawaddrman
RPC
#30062
pull
brunoerg
wants to merge
2
commits into
bitcoin:master
from
brunoerg:2024-04-asmap-getrawaddrman
changing
2
files
+26 −5
-
brunoerg commented at 12:10 pm on May 8, 2024: contributorThis PR adds two new fields in
-
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.
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
-
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 readmapped as <something>
and notmapped *A*utonomous*S*ystem
- don’t have a concrete idea for a name and I guess it makes sense to use the same as ingetpeerinfo
.
brunoerg commented at 5:54 pm on May 8, 2024:I wanted to keep same name as ingetpeerinfo
but I agree with you about the confusion, I do believemapped_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’sgetrawaddrman
output, it felt odd to havemapped_as
appear aftersource
. 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.luke-jr referenced this in commit df458672e7 on May 13, 2024virtu commented at 9:39 am on May 21, 2024: contributorConcept 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:Donefjahr commented at 12:54 pm on May 21, 2024: contributorConcept ACKbrunoerg force-pushed on May 22, 2024brunoerg commented at 9:14 am on May 22, 2024: contributorForce-pushed:
- Changed
mapped_as
andsource_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:donein 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:donefjahr commented at 9:23 am on May 22, 2024: contributorCode 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, 2024DrahtBot requested review from virtu on May 22, 2024net: rpc: return peer's mapped AS in getrawaddrman
This information can be used to check bucketing logic.
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.
brunoerg force-pushed on May 22, 2024fjahr commented at 12:34 pm on May 22, 2024: contributorCode review ACK 1e54d61c4698debf3329d1960e06078ccbf8063c0xB10C approved0xB10C commented at 4:20 pm on May 22, 2024: contributorACK 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.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: memberACK 1e54d61c4698debf3329d1960e06078ccbf8063cglozow merged this on May 23, 2024glozow 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 isconnman
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#r1612272793in 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 upGetMappedAS
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"},
github-metadata-mirror
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-11 06:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me