p2p, rpc: enable GetAddr, GetAddresses, and getnodeaddresses by network

[jonatack]

This patch allows passing a network argument to CAddrMan::GetAddr(), CConnman::GetAddresses(), and rpc getnodeaddresses to return only addresses of that network.

It also contains a performance optimisation by promag to no longer call gettime for each vRandom entry inside the CAddrMan::GetAddr_() loop.

[vasild]

Approach ACK.

Filtering this "server side" would help reduce network traffic especially if all addresses are requested (matters if the RPC client is not running on the same machine as bitcoind). It does not add any (measurable) computational overhead on the "server side".

[jonatack]

Thanks for the early feedback @vasild! Updated and ready for review.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• fuzz: Call const member functions in addrman fuzz test only once #21941 (fuzz: Call const member functions in addrman fuzz test only once by MarcoFalke)
• refactor: Mark CAddrMan::Select and GetAddr const #21940 (refactor: Mark CAddrMan::Select const by MarcoFalke)
• rpc/validation: enable packages through testmempoolaccept #20833 (rpc/validation: enable packages through testmempoolaccept by glozow)
• addrman: improve performance by using more suitable containers #18722 (addrman: improve performance by using more suitable containers by vasild)

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.

[vasild]

ACK ac01ec0

(would be happy to re-ACK if the default value of the 3rd arg of GetAddr() is removed)

[jonatack]

Thank you @vasild. Removed the defaults and improved the fuzzing per git diff ac01ec0 09f2c20.

[jonatack]

Rebased for CI fix on master.

[vasild]

The latest push removed the default value for the network argument from CAddrMan::GetAddr() and from CConnMan::GetAddresses() too. The latter deserves a default value because now there are a lot of call sites like connman.GetAddresses(..., std::nullopt), no?

[jonatack]

GetAddresses(..., std::nullopt)`

Only in two places outside of tests, so it seemed better to pass it explicitly.

[promag]

Concept ACK.

I think this change could be simplified by setting the default argument of the network filter.

[vasild]

ACK bdc57fc

Whether to have a default value for the argument of CAddrMan::GetAddr() and CConnMan::GetAddresses() is not a showstopper for this PR, IMO.

Now there are two non-test callers of GetAddresses() that explicitly pass nullopt:

bitcoin/src/net.cpp

Line 2691 in bdc57fc

cache_entry.m_addrs_response_cache = GetAddresses(max_addresses, max_pct, /* network */ std::nullopt);

bitcoin/src/net_processing.cpp

Line 3580 in bdc57fc

vAddr = m_connman.GetAddresses(MAX_ADDR_TO_SEND, MAX_PCT_ADDR_TO_SEND, /* network */ std::nullopt);

[jonatack]

Thanks @vasild, agreed. Making them explicit was more work, but I reckon sooner or later someone is going to make them explicit so may as well do it now.

[jonatack]

Only change is @promag's suggestion: git diff bdc57fc 9d17f30

diff --git a/src/addrman.cpp b/src/addrman.cpp
index 473ac21ad0..ceab1689d7 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -494,6 +494,7 @@ void CAddrMan::GetAddr_(std::vector<CAddress>& vAddr, size_t max_addresses, size
+    const int64_t now{GetAdjustedTime()};
     for (unsigned int n = 0; n < vRandom.size(); n++) {
         if (vAddr.size() >= nNodes)
             break;
@@ -503,10 +504,14 @@ void CAddrMan::GetAddr_(std::vector<CAddress>& vAddr, size_t max_addresses, size
         assert(mapInfo.count(vRandom[n]) == 1);
 
         const CAddrInfo& ai = mapInfo[vRandom[n]];
-        // Filter for quality, and if a network is passed, filter also by network.
-        if (!ai.IsTerrible() && (network == std::nullopt || ai.GetNetClass() == network)) {
-            vAddr.push_back(ai);
-        }
+
+        // Filter by network (optional)
+        if (network != std::nullopt && ai.GetNetClass() != network) continue;
+
+        // Filter for quality
+        if (ai.IsTerrible(now)) continue;
+
+        vAddr.push_back(ai);
     }
 }

[promag]

Code review ACK 9d17f30.

[vasild]

ACK 9d17f30

[jonatack]

rebased after merge of #21506, no other change

git range-diff 4da26fb 9d17f30 ce6bca8

     @@ src/net_processing.cpp: void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
              pfrom.vAddrToSend.clear();
              std::vector<CAddress> vAddr;
-         if (pfrom.HasPermission(PF_ADDR)) {
+         if (pfrom.HasPermission(NetPermissionFlags::Addr)) {
              vAddr = m_connman.GetAddresses(MAX_ADDR_TO_SEND, MAX_PCT_ADDR_TO_SEND, /* network */ std::nullopt);

[vasild]

ACK ce6bca8

[laanwj]

Code review and lightly tested ACK ce6bca8

[jarolrod]

post-merge ACK 👍