[net] Minor improvements to addr caching #25096

dergoegge · 2022-05-09T15:46:27Z

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

bitcoin/src/net.cpp

Lines 2800 to 2804 in a8098f2

    
           auto local_socket_bytes = requestor.addrBind.GetAddrBytes(); 
        
           uint64_t cache_id = GetDeterministicRandomizer(RANDOMIZER_ID_ADDRCACHE) 
        
               .Write(requestor.addr.GetNetwork()) 
        
               .Write(local_socket_bytes.data(), local_socket_bytes.size()) 
        
               .Finalize();

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.

…ddr cache randomizer

jonatack · 2022-05-09T20:27:35Z

Concept ACK

theStack · 2022-05-10T10:36:56Z

Concept ACK

src/net.cpp

DrahtBot · 2022-05-10T19:45:42Z

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.

src/net.cpp

naumenkogs · 2022-05-17T07:22:45Z

Concept ACK

test/functional/p2p_getaddr_caching.py

naumenkogs · 2022-05-23T07:55:43Z

utACK f5e1269fbeaaf37fdb3945381231d5ff9834e42e

mzumsande

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.

src/net.cpp

test/functional/p2p_getaddr_caching.py

dergoegge · 2022-06-02T17:21:26Z

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

sipa · 2022-06-02T17:21:59Z

utACK 292828c

mzumsande · 2022-06-03T13:57:01Z

Code Review ACK 292828c

naumenkogs · 2022-06-08T10:17:26Z

utACK 292828c

maflcko · 2022-06-08T15:51:27Z

test/functional/p2p_getaddr_caching.py

    def set_test_params(self):
        self.num_nodes = 1
+        # Start onion ports after p2p and rpc ports.
+        port = PORT_MIN + 2 * PORT_RANGE


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

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

…getaddr_caching.py ea54ba2 [test] Fix port collisions caused by p2p_getaddr_caching.py (dergoegge) f9682e7 [test_framework] Set PortSeed.n directly after initialising params (dergoegge) Pull request description: This PR fixes the issue mentioned [here](bitcoin/bitcoin#25096 (comment)), to avoid port collisions between nodes spun up by the test framework. Top commit has no ACKs. Tree-SHA512: ec9159f0af90db636f7889d664c24e1430cf2bcb3c02a9ab2dcfe531b2a4d18f6e3a0f8ba73071bdf2f7db518df9d5d86a9cd06695e67644d20fe4515fac32b7

fa7a711 test: Fix out-of-range port collisions (MacroFake) Pull request description: Otherwise the test will fail if two tests running in parallel use the same port. See bitcoin/bitcoin#25096 (comment) and bitcoin/bitcoin#25312 ACKs for top commit: dergoegge: ACK fa7a711 - This gets rid of some rather arbitrary choices for ports in some of our functional tests that can cause port collisions across test runs, resulting in intermittent failures. Tree-SHA512: ac73da8a498230b992ab12e1ee3c4ff3d868cd63c00d2c71537d156cb7c8f8be8598ec574646b17c5a44ae3ac5bb54bf29d300f054a36cec6f6ce8054a0da0a4

292828c [test] Test addr cache for multiple onion binds (dergoegge) 3382905 [net] Seed addr cache randomizer with port from binding address (dergoegge) f10e80b [net] Use ConnectedThroughNetwork() instead of GetNetwork() to seed addr cache randomizer (dergoegge) Pull request description: 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`. ACKs for top commit: sipa: utACK 292828c mzumsande: Code Review ACK 292828c naumenkogs: utACK 292828c Tree-SHA512: d0be13bab6bc121c2926d4b168687f6c2ed4ce0c9dd19be71eb4886adeba8afc3daacdc4e232a0ba3b03a89d69b618abc5595b69abd1ad0c476d825bc6ea1f9f

…caching.py ea54ba2 [test] Fix port collisions caused by p2p_getaddr_caching.py (dergoegge) f9682e7 [test_framework] Set PortSeed.n directly after initialising params (dergoegge) Pull request description: This PR fixes the issue mentioned [here](bitcoin#25096 (comment)), to avoid port collisions between nodes spun up by the test framework. Top commit has no ACKs. Tree-SHA512: ec9159f0af90db636f7889d664c24e1430cf2bcb3c02a9ab2dcfe531b2a4d18f6e3a0f8ba73071bdf2f7db518df9d5d86a9cd06695e67644d20fe4515fac32b7

[net] Use ConnectedThroughNetwork() instead of GetNetwork() to seed a…

f10e80b

…ddr cache randomizer

fanquake added the P2P label May 9, 2022

dergoegge force-pushed the 2022-05-improve-addr-cache branch 2 times, most recently from 1968c8a to 4df29c9 Compare May 10, 2022 10:33

sipa reviewed May 10, 2022

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

dergoegge force-pushed the 2022-05-improve-addr-cache branch from 4df29c9 to 224d765 Compare May 10, 2022 18:57

DrahtBot mentioned this pull request May 11, 2022

tests: Use test framework utils where possible #23127

Closed

naumenkogs reviewed May 12, 2022

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

mzumsande reviewed May 16, 2022

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

naumenkogs reviewed May 17, 2022

View reviewed changes

test/functional/p2p_getaddr_caching.py Outdated Show resolved Hide resolved

dergoegge force-pushed the 2022-05-improve-addr-cache branch from 224d765 to f5e1269 Compare May 18, 2022 12:11

DrahtBot mentioned this pull request May 18, 2022

p2p: Prevent block index fingerprinting by sending additional getheaders messages #24571

Closed

fanquake requested review from jnewbery and mzumsande May 25, 2022 08:51

mzumsande reviewed Jun 2, 2022

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

test/functional/p2p_getaddr_caching.py Outdated Show resolved Hide resolved

dergoegge added 2 commits June 2, 2022 19:03

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

3382905

[test] Test addr cache for multiple onion binds

292828c

dergoegge force-pushed the 2022-05-improve-addr-cache branch from f5e1269 to 292828c Compare June 2, 2022 17:16

fanquake merged commit b9416c3 into bitcoin:master Jun 8, 2022

maflcko reviewed Jun 8, 2022

View reviewed changes

dergoegge mentioned this pull request Jun 8, 2022

test: Fix port collisions caused by p2p_getaddr_caching.py #25312

Merged

maflcko mentioned this pull request Jun 10, 2022

test: Fix out-of-range port collisions #25333

Merged

bitcoin locked and limited conversation to collaborators Jun 8, 2023

	auto local_socket_bytes = requestor.addrBind.GetAddrBytes();
	uint64_t cache_id = GetDeterministicRandomizer(RANDOMIZER_ID_ADDRCACHE)
	.Write(requestor.addr.GetNetwork())
	.Write(local_socket_bytes.data(), local_socket_bytes.size())
	.Finalize();

[net] Minor improvements to addr caching #25096

[net] Minor improvements to addr caching #25096

Uh oh!

Conversation

dergoegge commented May 9, 2022

Uh oh!

jonatack commented May 9, 2022

Uh oh!

theStack commented May 10, 2022

Uh oh!

Uh oh!

DrahtBot commented May 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

Uh oh!

naumenkogs commented May 17, 2022

Uh oh!

Uh oh!

naumenkogs commented May 23, 2022

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dergoegge commented Jun 2, 2022

Uh oh!

sipa commented Jun 2, 2022

Uh oh!

mzumsande commented Jun 3, 2022

Uh oh!

naumenkogs commented Jun 8, 2022

Uh oh!

maflcko Jun 8, 2022

Choose a reason for hiding this comment

Uh oh!

dergoegge Jun 8, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

DrahtBot commented May 10, 2022 •

edited

Loading