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.
878@@ -875,8 +879,13 @@ static RPCHelpMan getnodeaddresses()
879 const int count{request.params[0].isNull() ? 1 : request.params[0].get_int()};
880 if (count < 0) throw JSONRPCError(RPC_INVALID_PARAMETER, "Address count out of range");
881
882+ const Network network{request.params[1].isNull() ? Network::NET_MAX : ParseNetwork(request.params[1].get_str())};
883+ if (network == NET_UNROUTABLE) {
884+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid network (not recognized)");
Maybe print the unrecognized string:
0 throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Network not recognized: %s", request.params[1].get_str());
715@@ -716,13 +716,13 @@ friend class CAddrManTest;
716 }
717
718 //! Return a bunch of addresses, selected at random.
719- std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct)
720+ std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct, Network net = Network::NET_MAX)
std::optional would fit better than using the special value NET_MAX? I plan to remove NET_MAX some day.
std::optional would be better here. Thanks!
278@@ -279,7 +279,7 @@ friend class CAddrManTest;
279 #endif
280
281 //! Select several addresses at once.
282- void GetAddr_(std::vector<CAddress> &vAddr, size_t max_addresses, size_t max_pct) EXCLUSIVE_LOCKS_REQUIRED(cs);
283+ void GetAddr_(std::vector<CAddress> &vAddr, size_t max_addresses, size_t max_pct, Network net = Network::NET_MAX) EXCLUSIVE_LOCKS_REQUIRED(cs);
The default value can be removed since all callers of this method always provide the argument.
0 void GetAddr_(std::vector<CAddress> &vAddr, size_t max_addresses, size_t max_pct, Network net) EXCLUSIVE_LOCKS_REQUIRED(cs);
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”.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
730+ *
731+ * @param[in] max_addresses Maximum number of addresses to return (0 = all).
732+ * @param[in] max_pct Maximum percentage of addresses to return (0 = all).
733+ * @param[in] network Select only addresses of this network (nullopt = all).
734+ */
735+ std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network = std::nullopt)
It just occurred to me that the only (non test) caller of GetAddr() is GetAddresses() which will always pass 3 arguments. Maybe consider removing the default from here since it is not necessary.
0 std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network)
248
249- # addrman's size cannot be known reliably after insertion, as hash collisions may occur
250- # so only test that requesting a large number of addresses returns less than that
251- LARGE_REQUEST_COUNT = 10000
252- node_addresses = self.nodes[0].getnodeaddresses(LARGE_REQUEST_COUNT)
253- assert_greater_than(LARGE_REQUEST_COUNT, len(node_addresses))
135@@ -130,9 +136,6 @@ Changes to Wallet or GUI related settings can be found in the GUI or Wallet sect
136
137 - Passing an invalid `-rpcauth` argument now cause bitcoind to fail to start. (#20461)
138
139-- The `getnodeaddresses` RPC now returns a "network" field indicating the
140- network type (ipv4, ipv6, onion, or i2p) for each address. (#21594)
ACK ac01ec096bc5ff437db5c4797e3d98b7a6efad23
(would be happy to re-ACK if the default value of the 3rd arg of GetAddr() is removed)
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.
502@@ -501,8 +503,10 @@ void CAddrMan::GetAddr_(std::vector<CAddress>& vAddr, size_t max_addresses, size
503 assert(mapInfo.count(vRandom[n]) == 1);
504
505 const CAddrInfo& ai = mapInfo[vRandom[n]];
506- if (!ai.IsTerrible())
507+ // Filter for quality, and if a network is passed, filter also by network.
508+ if (!ai.IsTerrible() && (network == std::nullopt || ai.GetNetClass() == network)) {
8d4b14e001ec026f346c6b4b00509adae7769415
I’d use a different approach to not mix different conditions:
0// Filter by network (optional)
1if (network != std::nullopt && ai.GetNetClass() != network) continue;
2
3// Filter for quality
4if (ai.IsTerrible()) continue;
5
6vAddr.push_back(ai);
Also note that it first filters by network, which I think is slightly better.
I also want to point that IsTerrible default argument is computed in each iteration, I’d suggest:
0 const int64_t now = GetAdjustedTime();
1
2 // gather a list of random nodes, skipping those of low quality
3 for (unsigned int n = 0; n < vRandom.size(); n++) {
4 ...
5 if (ai.IsTerrible(now)) continue;
730+ *
731+ * @param[in] max_addresses Maximum number of addresses to return (0 = all).
732+ * @param[in] max_pct Maximum percentage of addresses to return (0 = all).
733+ * @param[in] network Select only addresses of this network (nullopt = all).
734+ */
735+ std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network)
8942494e8c5eff8a3e268fe830868faf8ce19f9f
Why not network = std::nullopt? It would avoid changes in call sites.
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
Only change is @promag’s suggestion: git diff bdc57fc 9d17f30
0diff --git a/src/addrman.cpp b/src/addrman.cpp
1index 473ac21ad0..ceab1689d7 100644
2--- a/src/addrman.cpp
3+++ b/src/addrman.cpp
4@@ -494,6 +494,7 @@ void CAddrMan::GetAddr_(std::vector<CAddress>& vAddr, size_t max_addresses, size
5+ const int64_t now{GetAdjustedTime()};
6 for (unsigned int n = 0; n < vRandom.size(); n++) {
7 if (vAddr.size() >= nNodes)
8 break;
9@@ -503,10 +504,14 @@ void CAddrMan::GetAddr_(std::vector<CAddress>& vAddr, size_t max_addresses, size
10 assert(mapInfo.count(vRandom[n]) == 1);
11
12 const CAddrInfo& ai = mapInfo[vRandom[n]];
13- // Filter for quality, and if a network is passed, filter also by network.
14- if (!ai.IsTerrible() && (network == std::nullopt || ai.GetNetClass() == network)) {
15- vAddr.push_back(ai);
16- }
17+
18+ // Filter by network (optional)
19+ if (network != std::nullopt && ai.GetNetClass() != network) continue;
20+
21+ // Filter for quality
22+ if (ai.IsTerrible(now)) continue;
23+
24+ vAddr.push_back(ai);
25 }
26 }
rebased after merge of #21506, no other change
git range-diff 4da26fb 9d17f30 ce6bca8
0 @@ src/net_processing.cpp: void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
1 pfrom.vAddrToSend.clear();
2 std::vector<CAddress> vAddr;
3- if (pfrom.HasPermission(PF_ADDR)) {
4+ if (pfrom.HasPermission(NetPermissionFlags::Addr)) {
5 vAddr = m_connman.GetAddresses(MAX_ADDR_TO_SEND, MAX_PCT_ADDR_TO_SEND, /* network */ std::nullopt);
503@@ -501,8 +504,14 @@ void CAddrMan::GetAddr_(std::vector<CAddress>& vAddr, size_t max_addresses, size
504 assert(mapInfo.count(vRandom[n]) == 1);
505
506 const CAddrInfo& ai = mapInfo[vRandom[n]];
507- if (!ai.IsTerrible())
508- vAddr.push_back(ai);
509+
510+ // Filter by network (optional)
511+ if (network != std::nullopt && ai.GetNetClass() != network) continue;
Won’t this result in infnite loops, if there are not enough of the requested net class? I think doing this correctly requires keeping track of the counts per network.
EDIT: nevermind, it’s bounded by the size of vRandom as pointed out below.
getnodeaddresses 10000 i2p) the loop terminates after it has iterated through vRandom.