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
  1. 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.
  2. 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.

  3. DrahtBot added the label P2P on May 8, 2024
  4. brunoerg commented at 12:10 pm on May 8, 2024: contributor
    ping @0xB10C
  5. 0xB10C referenced this in commit a455ae900f on May 8, 2024
  6. 0xB10C referenced this in commit bad32e30b3 on May 8, 2024
  7. 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.

    image

  8. 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.
  9. brunoerg commented at 4:18 pm on May 9, 2024: contributor
  10. luke-jr referenced this in commit df458672e7 on May 13, 2024
  11. 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
  12. 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
  13. fjahr commented at 12:54 pm on May 21, 2024: contributor
    Concept ACK
  14. brunoerg force-pushed on May 22, 2024
  15. brunoerg commented at 9:14 am on May 22, 2024: contributor

    Force-pushed:

  16. 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
  17. 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
  18. 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.

  19. DrahtBot requested review from 0xB10C on May 22, 2024
  20. DrahtBot requested review from virtu on May 22, 2024
  21. net: rpc: return peer's mapped AS in getrawaddrman
    This information can be used to check bucketing
    logic.
    8c2714907d
  22. 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
  23. brunoerg force-pushed on May 22, 2024
  24. fjahr commented at 12:34 pm on May 22, 2024: contributor
    Code review ACK 1e54d61c4698debf3329d1960e06078ccbf8063c
  25. 0xB10C approved
  26. 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.

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

    ACK 1e54d61

    Successfully re-ran all previously mentioned tests.

  28. 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?
  29. glozow commented at 11:06 am on May 23, 2024: member
    ACK 1e54d61c4698debf3329d1960e06078ccbf8063c
  30. glozow merged this on May 23, 2024
  31. glozow closed this on May 23, 2024

  32. 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.
  33. 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:
  34. 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) {
    
  35. 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:
  36. 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"},
    
  37. brunoerg commented at 9:07 pm on May 23, 2024: contributor
    Thanks for reviewing, @jonatack. I’m working on a follow-up.

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