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

brunoerg · 2024-05-08T12:10:14Z

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 · 2024-05-08T12:10:17Z

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.

brunoerg · 2024-05-08T12:10:31Z

ping @0xB10C

With bitcoin/bitcoin#30062, the mapped_as and source_mapped_as fields are introduced. These are handled here as optional, since older Bitcoin Core versions might not have them yet and it might never be merged.

0xB10C · 2024-05-08T14:20:13Z

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.

src/rpc/net.cpp

brunoerg · 2024-05-09T16:18:21Z

cc: @virtu @fjahr

This information can be used to check bucketing logic. Github-Pull: bitcoin#30062 Rebased-From: 3733f6b

virtu

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

src/rpc/net.cpp

fjahr

Concept ACK

src/rpc/net.cpp

brunoerg · 2024-05-22T09:14:18Z

Force-pushed:

Changed mapped_as and source_mapped_as to optional.
Changed description of the fields as suggested by @fjahr.
Reordered these new fields (net: add ASMap info in getrawaddrman RPC #30062 (comment))

fjahr

Code review ACK 2de765c

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

src/rpc/net.cpp

test/functional/feature_asmap.py

This information can be used to check bucketing logic.

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.

fjahr · 2024-05-22T12:34:48Z

Code review ACK 1e54d61

0xB10C

ACK 1e54d61

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 · 2024-05-23T09:02:30Z

ACK 1e54d61

Successfully re-ran all previously mentioned tests.

glozow

ACK 1e54d61

glozow · 2024-05-23T11:03:27Z

test/functional/feature_asmap.py

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


nit: this could have been a set?

jonatack · 2024-05-23T20:36:03Z

src/rpc/net.cpp

@@ -1093,20 +1093,28 @@ static RPCHelpMan getaddrmaninfo()
    };
 }

-UniValue AddrmanEntryToJSON(const AddrInfo& info)
+UniValue AddrmanEntryToJSON(const AddrInfo& info, CConnman& connman)


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

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

jonatack · 2024-05-23T20:36:27Z

src/rpc/net.cpp

    return ret;
 }

-UniValue AddrmanTableToJSON(const std::vector<std::pair<AddrInfo, AddressPosition>>& tableInfos)
+UniValue AddrmanTableToJSON(const std::vector<std::pair<AddrInfo, AddressPosition>>& tableInfos, CConnman& connman)


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

jonatack · 2024-05-23T20:39:05Z

src/rpc/net.cpp

 {
    UniValue ret(UniValue::VOBJ);
    ret.pushKV("address", info.ToStringAddr());
+    const auto mapped_as{connman.GetMappedAS(info)};
+    if (mapped_as != 0) {


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.

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

jonatack · 2024-05-23T20:39:33Z

src/rpc/net.cpp

    ret.pushKV("port", info.GetPort());
    ret.pushKV("services", (uint64_t)info.nServices);
    ret.pushKV("time", int64_t{TicksSinceEpoch<std::chrono::seconds>(info.nTime)});
    ret.pushKV("network", GetNetworkName(info.GetNetClass()));
    ret.pushKV("source", info.source.ToStringAddr());
    ret.pushKV("source_network", GetNetworkName(info.source.GetNetClass()));
+    const auto source_mapped_as{connman.GetMappedAS(info.source)};
+    if (source_mapped_as != 0) {


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

jonatack · 2024-05-23T20:42:18Z

src/rpc/net.cpp

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


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).

Suggested change

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

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

brunoerg · 2024-05-23T21:07:22Z

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

DrahtBot added the P2P label May 8, 2024

This was referenced May 8, 2024

add: show & highlight AS and source AS in frontend 0xB10C/addrman-observer#14

Merged

backend support for asmap fields 0xB10C/addrman-observer#15

Open

0xB10C reviewed May 8, 2024

View reviewed changes

src/rpc/net.cpp Outdated Show resolved Hide resolved

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request May 13, 2024

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

df45867

This information can be used to check bucketing logic. Github-Pull: bitcoin#30062 Rebased-From: 3733f6b

virtu reviewed May 21, 2024

View reviewed changes

src/rpc/net.cpp Outdated Show resolved Hide resolved

fjahr reviewed May 21, 2024

View reviewed changes

src/rpc/net.cpp Outdated Show resolved Hide resolved

brunoerg force-pushed the 2024-04-asmap-getrawaddrman branch from f80c47c to 2de765c Compare May 22, 2024 09:11

fjahr reviewed May 22, 2024

View reviewed changes

src/rpc/net.cpp Outdated Show resolved Hide resolved

test/functional/feature_asmap.py Outdated Show resolved Hide resolved

DrahtBot requested review from 0xB10C and virtu May 22, 2024 09:23

brunoerg added 2 commits May 22, 2024 07:57

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

8c27149

This information can be used to check bucketing logic.

test: add coverage for mapped_as from getrawaddrman

1e54d61

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 the 2024-04-asmap-getrawaddrman branch from 2de765c to 1e54d61 Compare May 22, 2024 10:58

0xB10C approved these changes May 22, 2024

View reviewed changes

glozow reviewed May 23, 2024

View reviewed changes

glozow merged commit 83ae1ba into bitcoin:master May 23, 2024
16 checks passed

jonatack reviewed May 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

brunoerg commented May 8, 2024

DrahtBot commented May 8, 2024 •

edited

brunoerg commented May 8, 2024

0xB10C commented May 8, 2024

brunoerg commented May 9, 2024

virtu left a comment

fjahr left a comment

brunoerg commented May 22, 2024

fjahr left a comment

fjahr commented May 22, 2024

0xB10C left a comment

virtu commented May 23, 2024

glozow left a comment

glozow May 23, 2024

jonatack May 23, 2024

brunoerg May 23, 2024

jonatack May 23, 2024

jonatack May 23, 2024 •

edited

jonatack May 23, 2024

jonatack May 23, 2024 •

edited

brunoerg commented May 23, 2024

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

net: add ASMap info in getrawaddrman RPC #30062

net: add ASMap info in getrawaddrman RPC #30062

Conversation

brunoerg commented May 8, 2024

DrahtBot commented May 8, 2024 • edited

Code Coverage

Reviews

brunoerg commented May 8, 2024

0xB10C commented May 8, 2024

brunoerg commented May 9, 2024

virtu left a comment

Choose a reason for hiding this comment

fjahr left a comment

Choose a reason for hiding this comment

brunoerg commented May 22, 2024

fjahr left a comment

Choose a reason for hiding this comment

fjahr commented May 22, 2024

0xB10C left a comment

Choose a reason for hiding this comment

virtu commented May 23, 2024

glozow left a comment

Choose a reason for hiding this comment

glozow May 23, 2024

Choose a reason for hiding this comment

jonatack May 23, 2024

Choose a reason for hiding this comment

brunoerg May 23, 2024

Choose a reason for hiding this comment

jonatack May 23, 2024

Choose a reason for hiding this comment

jonatack May 23, 2024 • edited

Choose a reason for hiding this comment

jonatack May 23, 2024

Choose a reason for hiding this comment

jonatack May 23, 2024 • edited

Choose a reason for hiding this comment

brunoerg commented May 23, 2024

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

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

DrahtBot commented May 8, 2024 •

edited

jonatack May 23, 2024 •

edited

jonatack May 23, 2024 •

edited