There are certain statistics we can collect by running all our known clearnet addresses against the ASMap file. This could show issues with a maliciously manipulated file or with an old file that has decayed with time.
This is just a proof of concept for now. My idea currently is to run the analysis once per day and print the results to logs if an ASMap file is used.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | mzumsande, brunoerg, achow101 |
| Concept ACK | pablomartin4btc, virtu |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
forceinbound to evict a random unprotected connection if all slots are otherwise full by pinheadmz)If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
1967+ std::transform(v4_addrs.begin(), v4_addrs.end(), std::back_inserter(clearnet_addrs),
1968+ [](const CAddress& addr) { return static_cast<CNetAddr>(addr); });
1969+ std::transform(v6_addrs.begin(), v6_addrs.end(), std::back_inserter(clearnet_addrs),
1970+ [](const CAddress& addr) { return static_cast<CNetAddr>(addr); });
1971+ m_connman.ASMapHealthCheck(clearnet_addrs);
1972+ }
net.cpp and shouldn’t use the block height.
I wasn’t happy with the location myself but I need more time to think about where it would fit in best, so happy to take suggestions. I just tried to put it somewhere where it requires the least amount of new dependencies and shipped it to get conceptual feedback.
and shouldn’t use the block height.
What do you suggest to use instead of block height?
happy to take suggestions.
I think it would make sense for the opencon thread (ThreadOpenConnections) to handle this.
What do you suggest to use instead of block height?
0if (m_last_asmap_health_check + 24h < NodeClock::now()) {
1 // Do health check
2}
109@@ -109,3 +110,23 @@ uint32_t NetGroupManager::GetMappedAS(const CNetAddr& address) const
110 uint32_t mapped_as = Interpret(m_asmap, ip_bits);
111 return mapped_as;
112 }
113+
114+void NetGroupManager::ASMapHealthCheck(const std::vector<CNetAddr> clearnet_addrs) const {
115+ std::set<uint32_t> clearnet_asns{};
116+ int unmapped_count{0};
117+
118+ for (auto addr : clearnet_addrs) {
I think you need to declare addr as const (tidyCI test is failing due to that).
0 for (const auto &addr : clearnet_addrs) {
1641@@ -1642,6 +1642,20 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
1642 const bool dnsseed = gArgs.GetBoolArg("-dnsseed", DEFAULT_DNSSEED);
1643 bool add_fixed_seeds = gArgs.GetBoolArg("-fixedseeds", DEFAULT_FIXEDSEEDS);
1644
1645+ // Run the ASMap Health check and log results once every 24h.
1646+ if (m_netgroupman.UsingASMap() && m_last_asmap_health_check + 24h < NodeClock::now()) {
1647+ m_last_asmap_health_check = NodeClock::now();
1648+ const std::vector<CAddress> v4_addrs{GetAddresses(0, 0, Network::NET_IPV4)};
nit:
0 const std::vector<CAddress> v4_addrs{GetAddresses(/*max_addresses=*/0, /*max_pct=*/0, Network::NET_IPV4)};
Concept ACK, but I’m not conviced about the approach.
In 9b1d32e203435a62caef7079fe8c49f755b43e3d “Net: Add continuous ASMap health check logging”: You’re basically fetching all addresses (ipv4/ipv6) from addrman and doing a sanity check every 24hs. Considering we don’t change asmap file frequently (we need to restart the node to do it), do we really need to do it for all addresses every 24hs?
Perhaps we could do it when:
OpenThreadConnection) - this is the only situation where asmap file can be changed
My idea currently is to run the analysis once per day
Instead of creating m_last_asmap_health_check and putting the logic into ThreadOpenConnection. Why not creating ASMapHealthCheck in CConnman:
0void CConnman::ASMapHealthCheck()
1{
2 const std::vector<CAddress> v4_addrs{GetAddresses(0, 0, Network::NET_IPV4)};
3 const std::vector<CAddress> v6_addrs{GetAddresses(0, 0, Network::NET_IPV6)};
4 std::vector<CNetAddr> clearnet_addrs;
5 clearnet_addrs.reserve(v4_addrs.size() + v6_addrs.size());
6 std::transform(v4_addrs.begin(), v4_addrs.end(), std::back_inserter(clearnet_addrs),
7 [](const CAddress& addr) { return static_cast<CNetAddr>(addr); });
8 std::transform(v6_addrs.begin(), v6_addrs.end(), std::back_inserter(clearnet_addrs),
9 [](const CAddress& addr) { return static_cast<CNetAddr>(addr); });
10 m_netgroupman.ASMapHealthCheck(clearnet_addrs);
11}
and then you could do in CConnman::Start something like:
0if (m_netgroupman.UsingASMap()) {
1 scheduler.scheduleFromNow([this] { ASMapHealthCheck(); }, ASMAP_HEALTH_CHECK);
2}
being ASMAP_HEALTH_CHECK the period, e.g.:
0static constexpr std::chrono::hours ASMAP_HEALTH_CHECK{24};
Obs: I didn’t test it, just a suggestion that came to my mind.
119+ uint32_t asn = GetMappedAS(addr);
120+ if (asn == 0) {
121+ ++unmapped_count;
122+ continue;
123+ }
124+ clearnet_asns.insert(GetMappedAS(addr));
To avoid calling GetMappedAS again
0 clearnet_asns.insert(asn);
37@@ -38,8 +38,12 @@ def expected_messages(filename):
38
39 class AsmapTest(BitcoinTestFramework):
40 def set_test_params(self):
41- self.num_nodes = 1
42- self.extra_args = [["-checkaddrman=1"]] # Do addrman checks on all operations.
43+ self.num_nodes = 2
So we would still need to track the elapsed time to not run the check more often than necessary. So I think just using the 24h scheduler is cleaner and just as good.
I agree.
114@@ -111,6 +115,15 @@ def test_empty_asmap(self):
115 self.node.assert_start_raises_init_error(extra_args=['-asmap'], expected_msg=msg)
116 os.remove(self.default_asmap)
117
118+ def test_asmap_health_check(self):
119+ self.log.info('Test bitcoind -asmap logs ASMap Health Check with basic stats')
120+ self.stop_node(0)
test_asmapp_health_check is called after test_empty_asmap, node0 is not running, so we don’t need to stop it here again.
40@@ -41,6 +41,16 @@ class NetGroupManager {
41 */
42 uint32_t GetMappedAS(const CNetAddr& address) const;
43
44+ /**
45+ * Analyze and print current health of ASMap based buckets.
46+ */
47+ void ASMapHealthCheck(const std::vector<CNetAddr> clearnet_addrs) const;
0 void ASMapHealthCheck(const std::vector<CNetAddr>& clearnet_addrs) const;
109@@ -109,3 +110,23 @@ uint32_t NetGroupManager::GetMappedAS(const CNetAddr& address) const
110 uint32_t mapped_as = Interpret(m_asmap, ip_bits);
111 return mapped_as;
112 }
113+
114+void NetGroupManager::ASMapHealthCheck(const std::vector<CNetAddr> clearnet_addrs) const {
115+ std::set<uint32_t> clearnet_asns{};
116+ int unmapped_count{0};
117+
118+ for (const auto &addr : clearnet_addrs) {
nit:
0 for (const auto& addr : clearnet_addrs) {
40@@ -41,6 +41,16 @@ class NetGroupManager {
41 */
42 uint32_t GetMappedAS(const CNetAddr& address) const;
43
44+ /**
45+ * Analyze and print current health of ASMap based buckets.
nit:
0 * Analyze and log current health of ASMap based buckets.
122+ continue;
123+ }
124+ clearnet_asns.insert(asn);
125+ }
126+
127+ LogPrint(BCLog::NET, "ASMap Health Check: %i clearnet peers are mapped to %i ASNs with %i peers being unmapped\n", clearnet_addrs.size(), clearnet_asns.size(), unmapped_count);
BCLog::NET for extended periods of time for most users, considering how spammy it is.
2921@@ -2916,6 +2922,19 @@ uint64_t CConnman::CalculateKeyedNetGroup(const CAddress& address) const
2922 return GetDeterministicRandomizer(RANDOMIZER_ID_NETGROUP).Write(vchNetGroup.data(), vchNetGroup.size()).Finalize();
2923 }
2924
2925+void CConnman::ASMapHealthCheck()
2926+{
2927+ const std::vector<CAddress> v4_addrs{GetAddresses(/*max_addresses=*/ 0, /*max_pct=*/ 0, Network::NET_IPV4)};
GetAddr() in all cases? I don’t see that mentioned in GetAddr() header file doc comment and GetAddr_() just says “Filter for quality”. Git blaming my way back to the original point it was added seems tedious at first look due to the many refactors on this code…
GetAddr() was initially only used to answer GETADDR messages, and only later used for reporting purposes such as getnodeaddresses. #27511 has some more discussion on this. I don’t think it is strictly necessary to do anything about it here, but one way to deal with it would be to just add a bool to GetAddr() that lets callers pick whether to filter out Terrible addresses or not, and use it to include all addresses in the ASMap check.
GetAddr() unfiltered, i.e. with isTerrible() = true addresses. Otherwise, the results are not as expressive as I would have liked but happy to bikeshed this if people have different opinions :)
CI times out
Doesn’t really look related? One failed after 120m, one after 16s and most succeeded. I also don’t see any relation to test changes here. Pushed a new rebase to see if the issues persist…
3304@@ -3305,6 +3305,12 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)
3305 // Dump network addresses
3306 scheduler.scheduleEvery([this] { DumpAddresses(); }, DUMP_PEERS_INTERVAL);
3307
3308+ // Run the ASMap Health check once and then schedule it to run every 24h.
3309+ if (m_netgroupman.UsingASMap()) {
3310+ ASMapHealthCheck();
3311+ scheduler.scheduleFromNow([this] { ASMapHealthCheck(); }, ASMAP_HEALTH_CHECK_INTERVAL);
scheduleEvery.
crACK 3ea54e5db7d53da5afa321e1800c29aa269dd3b3
code lgtm, will test in practice soon!
