Improvements on ADDR caching

naumenkogs commented at 10:45 am on August 11, 2020: member

This is a follow-up on #18991 which does 3 things:

improves privacy of a node listening to multiple addresses via adding cache index by local socket addr (suggested here)
documents on the choice of 24h cache lifetime
addresses nits from #18991

naumenkogs renamed this:
~~Minor improvements on ADDR caching~~
Improvements on ADDR caching
on Aug 11, 2020

fanquake added the label P2P on Aug 11, 2020

in src/net_processing.cpp:3534 in 96cd22cd32 outdated

3530@@ -3531,7 +3531,9 @@ void ProcessMessage(
3531         if (pfrom.HasPermission(PF_ADDR)) {
3532             vAddr = connman.GetAddresses();
3533         } else {
3534-            vAddr = connman.GetAddresses(pfrom.addr.GetNetwork());
3535+            LOCK(pfrom.cs_hSocket);

jnewbery commented at 11:06 am on August 11, 2020:

This seems like a slight layer violation, with network and socket details bleeding up into net_processing. Did you consider just passing a CNode& reference down to CConnMan and allowing it to get the bind address and network?

in src/net.cpp:2545 in e3ec66f132 outdated

2538@@ -2539,12 +2539,17 @@ std::vector<CAddress> CConnman::GetAddresses()
2539     return addresses;
2540 }
2541 
2542-std::vector<CAddress> CConnman::GetAddresses(Network requestor_network)
2543+std::vector<CAddress> CConnman::GetAddresses(Network requestor_network, CAddress local_socket_address)
2544 {
2545+    auto local_socket_bytes = local_socket_address.GetAddrBytes();
2546+    uint64_t cache_id = GetDeterministicRandomizer(RANDOMIZER_ID_LOCALHOSTNONCE)

promag commented at 12:01 pm on August 11, 2020:

e3ec66f1322154e84176ac06311cc0c570f38e23

nit, const.

in src/net.cpp:2550 in 96cd22cd32 outdated

2548         .Write(local_socket_bytes.data(), local_socket_bytes.size())
2549         .Finalize();
2550     const auto current_time = GetTime<std::chrono::microseconds>();
2551-    if (m_addr_response_caches.find(cache_id) == m_addr_response_caches.end() ||
2552-        m_addr_response_caches[cache_id].m_update_addr_response < current_time) {
2553+    if (m_addr_response_caches.count(cache_id) == 0 || m_addr_response_caches[cache_id].m_cache_expiration_time < current_time) {

promag commented at 12:09 pm on August 11, 2020:

96cd22cd3204b35b9f863ada93a99dcfac178d13

You have 4 m_addr_response_caches lookups. How about (untested):

0auto r = m_addr_response_caches.emplace(cache_id, {}};
1if (e.second || e.first->m_cache_expiration_time < current_time) {
2    r.first->m_addrs_response_cache = GetAddresses());
3    r.first->m_cache_expiration_time = ...;
4}
5return r.first->m_addrs_response_cache;

vasild commented at 9:16 am on August 12, 2020:

 0diff --git i/src/net.cpp w/src/net.cpp
 1index eb462d108..169c4cd1c 100644
 2--- i/src/net.cpp
 3+++ w/src/net.cpp
 4@@ -2544,14 +2544,16 @@ const std::vector<CAddress>& CConnman::GetAddresses(Network requestor_network, C
 5     auto local_socket_bytes = local_socket_address.GetAddrBytes();
 6     uint64_t cache_id = GetDeterministicRandomizer(RANDOMIZER_ID_LOCALHOSTNONCE)
 7         .Write(requestor_network)
 8         .Write(local_socket_bytes.data(), local_socket_bytes.size())
 9         .Finalize();
10     const auto current_time = GetTime<std::chrono::microseconds>();
11-    if (m_addr_response_caches.count(cache_id) == 0 || m_addr_response_caches[cache_id].m_cache_expiration_time < current_time) {
12-        m_addr_response_caches[cache_id].m_addrs_response_cache = GetAddresses();
13+    auto r = m_addr_response_caches.emplace(cache_id, CachedAddrResponse{});
14+    auto& cache = r.first->second;
15+    if (cache.m_cache_expiration_time < current_time) { // If emplace() added new one it has expiration 0.
16+        cache.m_addrs_response_cache = GetAddresses();
17 
18         // Choosing a proper cache lifetime a trade-off between the privacy leak minimization
19         // and the usefulness of ADDR responses to honest users.
20         //
21         // Longer cache lifetime makes it more difficult for an attacker to scrape
22         // enough AddrMan data to maliciously infer something useful.
23@@ -2571,15 +2573,15 @@ const std::vector<CAddress>& CConnman::GetAddresses(Network requestor_network, C
24         // in the ADDR response are no longer active.
25         //
26         // However, the churn in the network is known to be rather low. Since we consider
27         // nodes to be "terrible" (see IsTerrible()) if the timestamps are older than 30 days,
28         // max. 24 hours of "penalty" due to cache shouldn't make any meaningful difference
29         // in terms of the freshness of the response.
30-        m_addr_response_caches[cache_id].m_cache_expiration_time = current_time + std::chrono::hours(21) + GetRandMillis(std::chrono::hours(6));
31+        cache.m_cache_expiration_time = current_time + std::chrono::hours(21) + GetRandMillis(std::chrono::hours(6));
32     }
33-    return m_addr_response_caches[cache_id].m_addrs_response_cache;
34+    return cache.m_addrs_response_cache;
35 }
36 
37 bool CConnman::AddNode(const std::string& strNode)
38 {
39     LOCK(cs_vAddedNodes);
40     for (const std::string& it : vAddedNodes) {

promag commented at 12:11 pm on August 11, 2020: member

Concept ACK. Nice explanation in 3a09c7b050d102509e732c31a1e9d84ed2b324f4.

in src/net.cpp:2542 in 96cd22cd32 outdated

2538@@ -2539,15 +2539,44 @@ std::vector<CAddress> CConnman::GetAddresses()
2539     return addresses;
2540 }
2541 
2542-std::vector<CAddress> CConnman::GetAddresses(Network requestor_network)
2543+const std::vector<CAddress>& CConnman::GetAddresses(Network requestor_network, CAddress local_socket_address)

luke-jr commented at 10:05 pm on August 11, 2020:

Is there any scenario where CAddress does not imply a Network?

vasild commented at 2:27 pm on August 12, 2020:

With Tor we may be seeing an IPv4 connection between Bitcoin Core and the Tor proxy (with IPv4 addresses) but the “requestor network” should be Tor, not IPv4.

in src/net.cpp:2545 in 96cd22cd32 outdated

2538@@ -2539,15 +2539,44 @@ std::vector<CAddress> CConnman::GetAddresses()
2539     return addresses;
2540 }
2541 
2542-std::vector<CAddress> CConnman::GetAddresses(Network requestor_network)
2543+const std::vector<CAddress>& CConnman::GetAddresses(Network requestor_network, CAddress local_socket_address)
2544 {
2545+    auto local_socket_bytes = local_socket_address.GetAddrBytes();
2546+    uint64_t cache_id = GetDeterministicRandomizer(RANDOMIZER_ID_LOCALHOSTNONCE)

luke-jr commented at 10:07 pm on August 11, 2020:

Ever overlapping caches sounds dangerous. Why not just index a unique cache per bind address?

naumenkogs commented at 3:40 pm on August 23, 2020:

I can’t grasp your concern, could you rephrase? I believe the caches won’t overlap across nodes because seeds inside GetDeterministicRandomizer are unique. Or perhaps you are concerned with something else?

luke-jr changes_requested

in src/net.cpp:2553 in 96cd22cd32 outdated

2553-        m_addr_response_caches[requestor_network].m_addrs_response_cache = GetAddresses();
2554-        m_addr_response_caches[requestor_network].m_update_addr_response = current_time + std::chrono::hours(21) + GetRandMillis(std::chrono::hours(6));
2555+    if (m_addr_response_caches.count(cache_id) == 0 || m_addr_response_caches[cache_id].m_cache_expiration_time < current_time) {
2556+        m_addr_response_caches[cache_id].m_addrs_response_cache = GetAddresses();
2557+
2558+        // Choosing a proper cache lifetime a trade-off between the privacy leak minimization

vasild commented at 7:51 am on August 12, 2020:

nit: s/lifetime a/lifetime is a/

in src/net.cpp:2562 in 96cd22cd32 outdated

2562+        // enough AddrMan data to maliciously infer something useful.
2563+        // By the time an attacker scraped enough AddrMan records, most of
2564+        // the records should be old enough to not leak topology info by
2565+        // e.g. analyzing real-time changes in timestamps.
2566+        //
2567+        // It takes ~100 of requests to scrape everything from an AddrMan containing 100,000 nodes,

vasild commented at 8:00 am on August 12, 2020:

nit: s/of requests/requests/

in src/net_processing.cpp:3536 in 96cd22cd32 outdated

3530@@ -3531,7 +3531,9 @@ void ProcessMessage(
3531         if (pfrom.HasPermission(PF_ADDR)) {
3532             vAddr = connman.GetAddresses();
3533         } else {
3534-            vAddr = connman.GetAddresses(pfrom.addr.GetNetwork());
3535+            LOCK(pfrom.cs_hSocket);
3536+            CAddress local_socket_address = CConnman::GetBindAddress(pfrom.hSocket);
3537+            vAddr = connman.GetAddresses(pfrom.addr.GetNetwork(), local_socket_address);

vasild commented at 8:30 am on August 12, 2020:

I am not sure if it would make any practical difference in this case, but in general it is a good practice to minimize the time the mutex is being held (may also be relevant if you decide to move the code to connman as per @jnewbery):

0            SOCKET socket;
1            WITH_LOCK(pfrom.cs_hSocket, socket = pfrom.hSocket);
2            CAddress local_socket_address = CConnman::GetBindAddress(socket);
3            vAddr = connman.GetAddresses(pfrom.addr.GetNetwork(), local_socket_address);

in src/net.h:462 in 96cd22cd32 outdated

455-     * resulting in at most ~160 KB.
456+     * resulting in at most ~160 KB. Every separate local socket may
457+     * add up to ~160 KB extra.
458      */
459-    std::map<Network, CachedAddrResponse> m_addr_response_caches;
460+    std::map<uint64_t, CachedAddrResponse> m_addr_response_caches;

vasild commented at 8:45 am on August 12, 2020:

nit: there is no need for the elements in this container to be ordered. Change to std::unordered_map to make lookup and insertion O(1).

Edit: changed to “nit” because I realized we are going to have about dozen entries here, map vs unordered_map will not make any practical difference.

vasild changes_requested

in src/net.h:451 in 96cd22cd32 outdated

448      * a single cache (or no cache at all) would let an attacker
449      * to easily detect that it is the same node by comparing responses.
450+     * Indexing by local socket prevents leakage when a node has multiple
451+     * listening addresses on the same network.
452+     *
453      * The used memory equals to 1000 CAddress records (or around 32 bytes) per

vasild commented at 2:31 pm on August 12, 2020:

CAddress is actually 48 bytes (or 40 if #19705 gets merged).

DrahtBot added the label Needs rebase on Aug 12, 2020

practicalswift commented at 3:17 pm on August 14, 2020: contributor

Concept ACK

naumenkogs force-pushed on Aug 23, 2020

naumenkogs commented at 3:43 pm on August 23, 2020: member

Ready for re-review :)

DrahtBot removed the label Needs rebase on Aug 23, 2020

DrahtBot commented at 2:03 am on August 24, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17428 (p2p: Try to preserve outbound block-relay-only connections during restart by hebasto)

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.

naumenkogs closed this on Aug 24, 2020

naumenkogs reopened this on Aug 24, 2020

in src/net.cpp:2564 in 213bd3b214 outdated

2564+        // enough AddrMan data to maliciously infer something useful.
2565+        // By the time an attacker scraped enough AddrMan records, most of
2566+        // the records should be old enough to not leak topology info by
2567+        // e.g. analyzing real-time changes in timestamps.
2568+        //
2569+        // It takes ~100 requests to scrape everything from an AddrMan containing 100,000 nodes,

jnewbery commented at 7:49 am on August 24, 2020:

This isn’t true. Addresses are chosen at random, so each subsequent getaddr request yields fewer new records for the spy (eg a spy who’s already scraped 50,000 records will only learn 500 new records on average from a getaddr request).

naumenkogs commented at 9:04 am on August 25, 2020:

Oh, true. I was originally intending to put “several hundreds” there. Unless you have an exact (probabilistic) formula, I would put “several hundreds”.

jnewbery commented at 10:17 am on August 25, 2020:

“several hundred” sounds fine to me

in src/net.cpp:357 in 213bd3b214 outdated

353@@ -353,7 +354,7 @@ bool CConnman::CheckIncomingNonce(uint64_t nonce)
354 }
355 
356 /** Get the bind address for a socket as CAddress */
357-static CAddress GetBindAddress(SOCKET sock)
358+CAddress CConnman::GetBindAddress(SOCKET sock)

jnewbery commented at 7:53 am on August 24, 2020:

This doesn’t need to move into CConnman

in src/net.h:272 in 213bd3b214 outdated

268@@ -269,7 +269,7 @@ class CConnman
269      * A non-malicious call (from RPC or a peer with addr permission) should
270      * call the function without a parameter to avoid using the cache.
271      */
272-    std::vector<CAddress> GetAddresses(Network requestor_network, size_t max_addresses, size_t max_pct);
273+    const std::vector<CAddress>& GetAddresses(CNode& requestor, size_t max_addresses, size_t max_pct);

jnewbery commented at 8:26 am on August 24, 2020:

Changing the return type to a const reference doesn’t actually make any difference here. The caller assigns a local variable vAddr to this function’s return, so the vector needs to be copied in the calling function. Compare that to the current behavior of returning by value, where the vector is copied inside this function, but can be move-assigned in the caller.

This would make a difference if we directly iterated over the return value, eg:

0for (const CAddress &addr : connman.GetAddresses(pfrom, MAX_ADDR_TO_SEND, MAX_PCT_ADDR_TO_SEND)) {
1    ...

or assigned the return of this function to a const reference and then iterated over it:

0const std::vector<CAddress>& vAddr{connman.GetAddresses(pfrom, MAX_ADDR_TO_SEND, MAX_PCT_ADDR_TO_SEND)};
1FastRandomContext insecure_rand;
2for (const CAddress &addr : vAddr) {
3    ...

but because vAddr can come from one of two places, I don’t think it’s worth doing that. Let’s just keep this as return by value for simplicity.

naumenkogs commented at 9:42 am on August 25, 2020:

Thanks for the thorough explanation :) I’m fine with keeping this as return by value for simplicity.

in src/net.cpp:2552 in 213bd3b214 outdated

2552-    if (m_addr_response_caches.find(requestor_network) == m_addr_response_caches.end() ||
2553-        m_addr_response_caches[requestor_network].m_update_addr_response < current_time) {
2554-        m_addr_response_caches[requestor_network].m_addrs_response_cache = GetAddresses(max_addresses, max_pct);
2555-        m_addr_response_caches[requestor_network].m_update_addr_response = current_time + std::chrono::hours(21) + GetRandMillis(std::chrono::hours(6));
2556+    auto r = m_addr_response_caches.emplace(cache_id, CachedAddrResponse{});
2557+    auto& cache = r.first->second;

jnewbery commented at 8:27 am on August 24, 2020:

I think this is a place where auto makes things less clear. Can you change this to explicitly name the CachedAddrResponse type? Also, cached_response or cache_entry would be a more accurate name than cache.

in src/net.h:447 in 213bd3b214 outdated

443@@ -442,20 +444,24 @@ class CConnman
444      */
445     struct CachedAddrResponse {
446         std::vector<CAddress> m_addrs_response_cache;
447-        std::chrono::microseconds m_update_addr_response{0};
448+        std::chrono::microseconds m_cache_expiration_time{0};

jnewbery commented at 8:30 am on August 24, 2020:

m_cache_entry_expiration ?

in test/functional/p2p_getaddr_caching.py:47 in 213bd3b214 outdated

40@@ -63,17 +41,16 @@ def set_test_params(self):
41 
42     def run_test(self):
43         self.log.info('Create connection that sends and requests addr messages')
44-        addr_source = self.nodes[0].add_p2p_connection(P2PInterface())
45+        self.nodes[0].add_p2p_connection(P2PInterface())
46 
47-        msg_send_addrs = msg_addr()
48         self.log.info('Fill peer AddrMan with a lot of records')
49         # Since these addrs are sent from the same source, not all of them will be stored,

jnewbery commented at 8:31 am on August 24, 2020:

This comment is no longer true.

in test/functional/p2p_getaddr_caching.py:69 in 213bd3b214 outdated

65@@ -89,7 +66,7 @@ def run_test(self):
66             responses.append(addr_receiver.get_received_addrs())
67         for response in responses[1:]:
68             assert_equal(response, responses[0])
69-        assert(len(response) < MAX_ADDR_TO_SEND)
70+        assert(len(response) <= 1000)

jnewbery commented at 8:38 am on August 24, 2020:

You can assert that response is exactly 1000 here.

in test/functional/p2p_getaddr_caching.py:53 in 213bd3b214 outdated

55-            addr_source.send_and_ping(msg_send_addrs)
56+        for i in range(10000):
57+            first_octet = i >> 8
58+            second_octet = i % 256
59+            a = "{}.{}.1.1".format(first_octet, second_octet)
60+            self.nodes[0].addpeeraddress(a, 8333)

jnewbery commented at 8:39 am on August 24, 2020:

Perhaps assert that len(self.nodes[0].getnodeaddresses(0)) is > 4000 here (to ensure that a getaddr response has at least 1000 records in it)

DrahtBot added the label Needs rebase on Aug 24, 2020

naumenkogs force-pushed on Aug 25, 2020

in test/functional/p2p_getaddr_caching.py:55 in 49ce8761c8 outdated

59+            a = "{}.{}.1.1".format(first_octet, second_octet)
60+            self.nodes[0].addpeeraddress(a, 8333)
61+
62+        # Need to make sure we hit 1000 records in the addr response later because
63+        # only a fraction of all known addresses can be cached and returned.
64+        assert(len(self.nodes[0].getnodeaddresses(0)) > int(1000 / 0.23))

jnewbery commented at 10:37 am on August 25, 2020:

Very minor style: consider defining MAX_ADDR_TO_SEND and MAX_PCT_ADDR_TO_SEND as constants rather than these magic numbers (if someone changes those values in the bitcoind code, it’ll be easier to see where to change them here at the same time).

in src/net.cpp:2564 in 49ce8761c8 outdated

2564+        // enough AddrMan data to maliciously infer something useful.
2565+        // By the time an attacker scraped enough AddrMan records, most of
2566+        // the records should be old enough to not leak topology info by
2567+        // e.g. analyzing real-time changes in timestamps.
2568+        //
2569+        // It takes only several hundreds of requests to scrape everything from an AddrMan containing 100,000 nodes,

jnewbery commented at 10:38 am on August 25, 2020:

grammar: s/several hundreds of requests/several hundred requests/

in test/functional/p2p_getaddr_caching.py:44 in 49ce8761c8 outdated

40@@ -63,17 +41,18 @@ def set_test_params(self):
41 
42     def run_test(self):
43         self.log.info('Create connection that sends and requests addr messages')
44-        addr_source = self.nodes[0].add_p2p_connection(P2PInterface())
45+        self.nodes[0].add_p2p_connection(P2PInterface())

jnewbery commented at 10:43 am on August 25, 2020:

This line (and the log line above) can be removed entirely.

jnewbery commented at 10:47 am on August 25, 2020: member

Looks good. Just a few minor style comments.

naumenkogs force-pushed on Aug 25, 2020

jnewbery commented at 11:52 am on August 25, 2020: member

utACK a3f1c2f56e6425f64cb05a4bcba6cf1465bf128f but needs rebase

naumenkogs force-pushed on Aug 26, 2020

DrahtBot removed the label Needs rebase on Aug 26, 2020

DrahtBot added the label Needs rebase on Aug 26, 2020

Justify the choice of ADDR cache lifetime 42ec558542

Add indexing ADDR cache by local socket addr 81b00f8780

naumenkogs force-pushed on Aug 27, 2020

DrahtBot removed the label Needs rebase on Aug 27, 2020

in src/net.h:457 in 4208cf8c41 outdated

453@@ -454,10 +454,10 @@ class CConnman
454      * Indexing by local socket prevents leakage when a node has multiple
455      * listening addresses on the same network.
456      *
457-     * The used memory equals to 1000 CAddress records (or around 32 bytes) per
458+     * The used memory equals to 1000 CAddress records (or around  bytes) per

vasild commented at 1:44 pm on September 1, 2020:

s/around bytes/around 40 bytes/

vasild approved

vasild commented at 1:51 pm on September 1, 2020: member

ACK f680ae4f0

Perhaps the commit Address nits in ADDR caching should be squashed into the previous commit Add indexing ADDR cache by local socket addr?

Address nits in ADDR caching 83ad65f31b

Refactor the functional test 0d04784af1

naumenkogs commented at 7:33 am on September 2, 2020: member

@vasild Fixed the comment issue you pointed out!

Perhaps the commit Address nits in ADDR caching should be squashed into the previous commit Add indexing ADDR cache by local socket addr?

I would leave it as is, that stuff doesn’t have to do with indexing by local socket…

naumenkogs force-pushed on Sep 2, 2020

vasild approved

vasild commented at 7:52 am on September 2, 2020: member

ACK 0d04784

jnewbery commented at 7:56 am on September 2, 2020: member

utACK 0d04784af151de249bbbcbad51e6e8ad9af8f5a3

laanwj added this to the "Blockers" column in a project

naumenkogs closed this on Sep 8, 2020

naumenkogs reopened this on Sep 8, 2020

in src/net.cpp:2546 in 81b00f8780 outdated

2539@@ -2539,12 +2540,19 @@ std::vector<CAddress> CConnman::GetAddresses(size_t max_addresses, size_t max_pc
2540     return addresses;
2541 }
2542 
2543-std::vector<CAddress> CConnman::GetAddresses(Network requestor_network, size_t max_addresses, size_t max_pct)
2544+std::vector<CAddress> CConnman::GetAddresses(CNode& requestor, size_t max_addresses, size_t max_pct)
2545 {
2546+    SOCKET socket;
2547+    WITH_LOCK(requestor.cs_hSocket, socket = requestor.hSocket);

promag commented at 8:42 pm on September 20, 2020:

81b00f87800f40cb14f2131ff27668bd2bb9e551

Not really sure what is the purpose of cs_hSocket, but looks like this should be

0vector<unsigned char> local_socket_bytes = WITH_LOCK(requestor.cs_hSocket, return GetBindAddress(requestor.hSocket).GetAddrBytes())

naumenkogs commented at 6:28 am on September 21, 2020:

True, this looks better. I’d keep it the way it is for now to save the review energy if you don’t mind. I will update these lines if I have other reasons to update the PR.

promag commented at 6:57 am on September 21, 2020:

It’s fine, can come next too if it’s the only thing that needs to be addressed.

in test/functional/p2p_getaddr_caching.py:19 in 0d04784af1

15@@ -21,21 +16,9 @@
16     assert_equal,
17 )
18 
19+# As defined in net_processing.

jonatack commented at 8:35 am on September 21, 2020:

0d04784af

0-# As defined in net_processing.
1-MAX_ADDR_TO_SEND = 1000
2-MAX_PCT_ADDR_TO_SEND = 23
3+MAX_ADDR_TO_SEND = 1000  # As defined in net.h
4+MAX_PCT_ADDR_TO_SEND = 23  # As defined in net_processing.cpp

laanwj commented at 5:44 pm on September 21, 2020:

Looks like a good suggestion,, but we can add more documentation later, didn’t want to hold up the PR on this

jonatack commented at 8:54 am on September 21, 2020: member

Code review ACK 0d04784

hebasto commented at 10:00 am on September 21, 2020: member

Concept ACK.

laanwj merged this on Sep 21, 2020

laanwj closed this on Sep 21, 2020

sidhujag referenced this in commit eec709c2bf on Sep 21, 2020

laanwj removed this from the "Blockers" column in a project

in src/net.cpp:2547 in 0d04784af1

2539@@ -2539,15 +2540,47 @@ std::vector<CAddress> CConnman::GetAddresses(size_t max_addresses, size_t max_pc
2540     return addresses;
2541 }
2542 
2543-std::vector<CAddress> CConnman::GetAddresses(Network requestor_network, size_t max_addresses, size_t max_pct)
2544+std::vector<CAddress> CConnman::GetAddresses(CNode& requestor, size_t max_addresses, size_t max_pct)
2545 {
2546+    SOCKET socket;
2547+    WITH_LOCK(requestor.cs_hSocket, socket = requestor.hSocket);
2548+    auto local_socket_bytes = GetBindAddress(socket).GetAddrBytes();

vasild commented at 2:34 pm on November 24, 2020:

Can this be simplified like:

0    auto local_socket_bytes = requestor.addrBind.GetAddrBytes();

Our local address is already saved in CNode::addrBind.

naumenkogs commented at 2:42 pm on November 24, 2020:

I think you’re right?

vasild commented at 2:50 pm on November 24, 2020:

Ok, will include this tiny change in another PR:

 0commit 15ba60e9a
 1Parent: 25dcd4ad9
 2Author:     Vasil Dimov <vd@FreeBSD.org>
 3AuthorDate: Tue Nov 24 15:36:27 2020 +0100
 4Commit:     Vasil Dimov <vd@FreeBSD.org>
 5CommitDate: Tue Nov 24 15:36:27 2020 +0100
 6
 7    net: avoid unnecessary GetBindAddress() call
 8    
 9    Our local (bind) address is already saved in `CNode::addrBind` and there
10    is no need to re-retrieve it again with `GetBindAddress()`.
11    
12    Also, for I2P connections `CNode::addrBind` would contain our I2P
13    address, but `GetBindAddress()` would return something like
14    `127.0.0.1:RANDOM_PORT`.
15
16diff --git a/src/net.cpp b/src/net.cpp
17index a811732e2..94a424ca3 100644
18--- a/src/net.cpp
19+++ b/src/net.cpp
20@@ -2601,15 +2601,13 @@ std::vector<CAddress> CConnman::GetAddresses(size_t max_addresses, size_t max_pc
21     }
22     return addresses;
23 }
24 
25 std::vector<CAddress> CConnman::GetAddresses(CNode& requestor, size_t max_addresses, size_t max_pct)
26 {
27-    SOCKET socket;
28-    WITH_LOCK(requestor.cs_hSocket, socket = requestor.hSocket);
29-    auto local_socket_bytes = GetBindAddress(socket).GetAddrBytes();
30+    auto local_socket_bytes = requestor.addrBind.GetAddrBytes();
31     uint64_t cache_id = GetDeterministicRandomizer(RANDOMIZER_ID_ADDRCACHE)
32         .Write(requestor.addr.GetNetwork())
33         .Write(local_socket_bytes.data(), local_socket_bytes.size())
34         .Finalize();
35     const auto current_time = GetTime<std::chrono::microseconds>();
36     auto r = m_addr_response_caches.emplace(cache_id, CachedAddrResponse{});

You just reviewed that! :partying_face:

Fabcien referenced this in commit deb75e9f51 on Oct 11, 2021

Fabcien referenced this in commit 00084affeb on Oct 11, 2021

Fabcien referenced this in commit 74c406fc16 on Oct 11, 2021

Fabcien referenced this in commit b6a9cbeec9 on Oct 11, 2021

DrahtBot locked this on Feb 15, 2022

Improvements on ADDR caching #19697

Conflicts