[net] Minor improvements to addr caching #25096

dergoegge commented at 3:46 PM on May 9, 2022: member

The addr cache id randomizer is currently supposed to be seeded with the network of the inbound connection and the local socket (only the address is used not the port): https://github.com/bitcoin/bitcoin/blob/a8098f2cef53ec003edae91100afce564e9c6f23/src/net.cpp#L2800-L2804

For inbound onion connections CNode::addr.GetNetwork() returns NET_UNROUTABLE and CNode::addrBind is set to 127.0.0.1:<onion bind port>. This results in the same addr cache for all inbound connections on 127.0.0.1 binds.

To avoid the same addr cache across all onion and other 127.0.0.1 binds, we should seed the addr cache randomizer with the correct network for inbound onion connections (using CNode::ConnectedThroughNetwork()) as well as the port of CNode::addrBind.

[net] Use ConnectedThroughNetwork() instead of GetNetwork() to seed addr cache randomizer f10e80b6e4

fanquake added the label P2P on May 9, 2022

jonatack commented at 8:27 PM on May 9, 2022: member

Concept ACK

dergoegge force-pushed on May 10, 2022

theStack commented at 10:36 AM on May 10, 2022: member

Concept ACK

in src/net.cpp:2804 in 3404b26848 outdated

2800 | @@ -2801,6 +2801,7 @@ std::vector<CAddress> CConnman::GetAddresses(CNode& requestor, size_t max_addres
2801 |      uint64_t cache_id = GetDeterministicRandomizer(RANDOMIZER_ID_ADDRCACHE)
2802 |          .Write(requestor.ConnectedThroughNetwork())
2803 |          .Write(local_socket_bytes.data(), local_socket_bytes.size())
2804 | +        .Write(requestor.addrBind.GetPort())

sipa commented at 1:53 PM on May 10, 2022:

This GetAddresses function is only called for inbound connections, AFAICT, so this is safe, but if it were ever invoked for outbound ones, requestor.addrBind.GetPort() would not return a useful number (outbound connections have a OS randomly-assigned local port, I believe). Perhaps add a comment to explain, or explicitly use 0 if the connection is outbound?

dergoegge commented at 7:00 PM on May 10, 2022:

Good point! Changed it to pass 0 for outbound connections and added a comment.

dergoegge force-pushed on May 10, 2022

DrahtBot commented at 7:45 PM on May 10, 2022: 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:

#24571 (p2p: Prevent block index fingerprinting by sending additional getheaders messages by dergoegge)
#23127 (tests: Use test framework utils where possible by vincenzopalazzo)

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.

in src/net.cpp:2806 in 15727b2824 outdated

2800 | @@ -2801,6 +2801,9 @@ std::vector<CAddress> CConnman::GetAddresses(CNode& requestor, size_t max_addres
2801 |      uint64_t cache_id = GetDeterministicRandomizer(RANDOMIZER_ID_ADDRCACHE)
2802 |          .Write(requestor.ConnectedThroughNetwork())
2803 |          .Write(local_socket_bytes.data(), local_socket_bytes.size())
2804 | +        // For outbound connections, the port of the bound address is randomly
2805 | +        // assigend by the OS and would therefore not be useful for seeding.
2806 | +        .Write(requestor.IsInboundConn() ? requestor.addrBind.GetPort() : 0)

naumenkogs commented at 7:06 AM on May 12, 2022:

Before this change, all inbound onion connections get the same cache_id. After this change, every different onion would generate a new cache_id. Am i right?

dergoegge commented at 2:31 PM on May 12, 2022:

Before this change, all inbound onion connections get the same cache_id.

Yes and all other incoming connections to 127.0.0.1:* also get the same cache_id.

After this change, every different onion would generate a new cache_id.

After this change all incoming connections to 127.0.0.1 binds get the same cache_id if the bound port is the same. e.g.: All connections to 127.0.0.1:8333 will get the same cache_id "a" and all connections to 127.0.0.1:8334 will get the same cache_id "b".

naumenkogs commented at 5:03 AM on May 16, 2022:

I assume the bound port would be the same for all Tor connections, right?

dergoegge commented at 4:07 PM on May 16, 2022:

You can have more than one onion bind (not the default) by using -bind=<addr>=onion multiple times. In that case not all Tor connections would have the same bound port.

example:

-bind=127.0.0.1:5555=onion
-bind=127.0.0.1:6666=onion

With this config any inbound Tor connection will have addrBind set to 127.0.0.1:5555 or 127.0.0.1:6666.

in src/net.cpp:2802 in 224d765d0c outdated

2798 | @@ -2799,8 +2799,11 @@ std::vector<CAddress> CConnman::GetAddresses(CNode& requestor, size_t max_addres
2799 |  {
2800 |      auto local_socket_bytes = requestor.addrBind.GetAddrBytes();
2801 |      uint64_t cache_id = GetDeterministicRandomizer(RANDOMIZER_ID_ADDRCACHE)
2802 | -        .Write(requestor.addr.GetNetwork())
2803 | +        .Write(requestor.ConnectedThroughNetwork())

mzumsande commented at 5:16 PM on May 16, 2022:

I'm not sure if this is matters a lot, but I think that aside from the intended change for inbound onion connections, this also changes the behavior for addrs for which HasLinkedIPv4() ist true, because ConnectedThroughNetwork() calls CNetAddr::GetNetClass() instead of CNetAddr::GetNetwork().

dergoegge commented at 10:59 AM on May 18, 2022:

Good catch, I did not notice this. I don't know enough about these IPv6 addresses that contain IPv4 addresses but it does not seem to matter here. It might be that inbound connections on IPv6 that come from these special IPv6 addresses get their own addr cache but that does not seem bad to me. It is mildly confusing that we have 3 different functions that return the network type for an address. @naumenkogs It looks like you were the one that introduced CNetAddr::GetNetClass(), what do you think about this?

naumenkogs commented at 7:44 AM on May 23, 2022:

I also don't think this HasLinkedIPv4 difference matters. I don't have a preference here.

naumenkogs commented at 7:22 AM on May 17, 2022: member

Concept ACK

in test/functional/p2p_getaddr_caching.py:90 in 224d765d0c outdated

  92 | +            responses.append(addr_receiver_local.get_received_addrs())
  93 | +            responses.append(addr_receiver_onion1.get_received_addrs())
  94 | +            responses.append(addr_receiver_onion2.get_received_addrs())
  95 | +
  96 | +        for i in range(0, len(responses) - 3):
  97 | +            assert(responses[i] != responses[i+1])

naumenkogs commented at 7:26 AM on May 17, 2022:

This indexing is hard to read. Perhaps you could try using named variables instead?

dergoegge commented at 12:15 PM on May 18, 2022:

Got rid of the responses list entirely in favor of simply comparing to the last received response.

dergoegge force-pushed on May 18, 2022

naumenkogs commented at 7:55 AM on May 23, 2022: member

utACK f5e1269fbeaaf37fdb3945381231d5ff9834e42e

fanquake requested review from jnewbery on May 25, 2022

fanquake requested review from mzumsande on May 25, 2022

in src/net.cpp:2805 in 15727b2824 outdated

2800 | @@ -2801,6 +2801,9 @@ std::vector<CAddress> CConnman::GetAddresses(CNode& requestor, size_t max_addres
2801 |      uint64_t cache_id = GetDeterministicRandomizer(RANDOMIZER_ID_ADDRCACHE)
2802 |          .Write(requestor.ConnectedThroughNetwork())
2803 |          .Write(local_socket_bytes.data(), local_socket_bytes.size())
2804 | +        // For outbound connections, the port of the bound address is randomly
2805 | +        // assigend by the OS and would therefore not be useful for seeding.

mzumsande commented at 3:48 PM on June 1, 2022:

typo: assigned

in test/functional/p2p_getaddr_caching.py:116 in f5e1269fbe outdated

 112 | @@ -82,8 +113,7 @@ def run_test(self):
 113 |          self.nodes[0].setmocktime(cur_mock_time)
 114 |          last_addr_receiver.wait_until(last_addr_receiver.addr_received)
 115 |          # new response is different
 116 | -        assert(set(responses[0]) != set(last_addr_receiver.get_received_addrs()))
 117 | -
 118 | +        assert(set(last_response_on_local_bind) != set(last_addr_receiver.get_received_addrs()))

mzumsande commented at 4:15 PM on June 2, 2022:

nit: for completeness maybe also check that the onion networks have different caches after the mocktime bump.

mzumsande commented at 4:23 PM on June 2, 2022: member

Code Review ACK f5e1269fbeaaf37fdb3945381231d5ff9834e42e

If it was somehow possible for inbound peers to make us generate additional bind addresses, this could be abused to make us create a lot of caches, but I don't think this is possible in any way.

[net] Seed addr cache randomizer with port from binding address 3382905bef

[test] Test addr cache for multiple onion binds 292828cd77

dergoegge force-pushed on Jun 2, 2022

dergoegge commented at 5:21 PM on June 2, 2022: member

Thanks @mzumsande for the review, I took both of your suggestions.

sipa commented at 5:21 PM on June 2, 2022: member

utACK 292828cd7744ec7eadede4ad54aa2117087c5435

mzumsande commented at 1:57 PM on June 3, 2022: member

Code Review ACK 292828cd7744ec7eadede4ad54aa2117087c5435

naumenkogs commented at 10:17 AM on June 8, 2022: member

utACK 292828cd7744ec7eadede4ad54aa2117087c5435

fanquake merged this on Jun 8, 2022

fanquake closed this on Jun 8, 2022

in test/functional/p2p_getaddr_caching.py:48 in 292828cd77

  43 | @@ -42,6 +44,13 @@ def addr_received(self):
  44 |  class AddrTest(BitcoinTestFramework):
  45 |      def set_test_params(self):
  46 |          self.num_nodes = 1
  47 | +        # Start onion ports after p2p and rpc ports.
  48 | +        port = PORT_MIN + 2 * PORT_RANGE

MarcoFalke commented at 3:51 PM on June 8, 2022:

pretty sure the tests only pass by accident? Locally they fail for me:

./test/functional/test_runner.py p2p_getaddr_caching feature_proxy feature_bind_extra p2p_add_connections 

p2p_add_connections.py | ✓ Passed  | 7 s
feature_bind_extra.py  | ✖ Failed  | 1 s
feature_proxy.py       | ✖ Failed  | 1 s
p2p_getaddr_caching.py | ✖ Failed  | 20 s

The basic idea is that all port ranges for all tests are fully assigned once the test_runner starts. Using the same range for different tests is asking for collisions.

One option to fix this would be to start a third range for "other" ports or just use one of the 12 ports assigned to each test (initially intended to be used by the test nodes itself).

dergoegge commented at 4:09 PM on June 8, 2022:

Oops sorry about that, will have a PR up shortly...

MarcoFalke referenced this in commit c3daa321f9 on Jun 10, 2022

MarcoFalke referenced this in commit b71d37da2c on Jun 10, 2022

sidhujag referenced this in commit b4e8ccf82a on Jun 13, 2022

sidhujag referenced this in commit 7baafa5dc4 on Jun 13, 2022

DrahtBot locked this on Jun 8, 2023