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

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.

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

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

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #21941 (fuzz: Call const member functions in addrman fuzz test only once by MarcoFalke)
• #21940 (refactor: Mark CAddrMan::Select const by MarcoFalke)
• #20833 (rpc/validation: enable packages through testmempoolaccept by glozow)
• #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.

ACK ac01ec096bc5ff437db5c4797e3d98b7a6efad23

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

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

Rebased for CI fix on master.

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?

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

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

Concept ACK.

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

ACK bdc57fc120affcd255b5b79b505433f5ed4dc843

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:

https://github.com/bitcoin/bitcoin/blob/bdc57fc120affcd255b5b79b505433f5ed4dc843/src/net.cpp#L2691

https://github.com/bitcoin/bitcoin/blob/bdc57fc120affcd255b5b79b505433f5ed4dc843/src/net_processing.cpp#L3580

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.

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);
     }
 }

Code review ACK 9d17f30c7e281d0f424eb45129beb6a6e62e37b9.

ACK 9d17f30c7e281d0f424eb45129beb6a6e62e37b9

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

ACK ce6bca88e8c685c69686e0b8dc095ffc3e2ac34d

Code review and lightly tested ACK ce6bca88e8c685c69686e0b8dc095ffc3e2ac34d

post-merge ACK 👍