Improvements on ADDR caching #19697

naumenkogs · 2020-08-11T10:45:51Z

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 Cache responses to GETADDR to prevent topology leaks #18991

src/net_processing.cpp

promag

Concept ACK. Nice explanation in 3a09c7b.

src/net.cpp

luke-jr · 2020-08-11T22:05:20Z

src/net.cpp

@@ -2539,15 +2539,44 @@ std::vector<CAddress> CConnman::GetAddresses()
    return addresses;
 }

-std::vector<CAddress> CConnman::GetAddresses(Network requestor_network)
+const std::vector<CAddress>& CConnman::GetAddresses(Network requestor_network, CAddress local_socket_address)


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

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.

luke-jr · 2020-08-11T22:07:19Z

src/net.cpp

 {
+    auto local_socket_bytes = local_socket_address.GetAddrBytes();
+    uint64_t cache_id = GetDeterministicRandomizer(RANDOMIZER_ID_LOCALHOSTNONCE)


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

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?

src/net.cpp

src/net_processing.cpp

src/net.h

practicalswift · 2020-08-14T15:17:43Z

Concept ACK

naumenkogs · 2020-08-23T15:43:02Z

Ready for re-review :)

DrahtBot · 2020-08-24T02:03:55Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

p2p: Try to preserve outbound block-relay-only connections during restart #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.

src/net.cpp

src/net.h

src/net.cpp

src/net.h

test/functional/p2p_getaddr_caching.py

jnewbery

Looks good. Just a few minor style comments.

test/functional/p2p_getaddr_caching.py

src/net.cpp

test/functional/p2p_getaddr_caching.py

jnewbery · 2020-08-25T11:52:52Z

utACK a3f1c2f but needs rebase

vasild

ACK f680ae4

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

src/net.h

naumenkogs · 2020-09-02T07:33:59Z

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

vasild

ACK 0d04784

jnewbery · 2020-09-02T07:56:11Z

utACK 0d04784

promag · 2020-09-20T20:42:33Z

src/net.cpp

 {
+    SOCKET socket;
+    WITH_LOCK(requestor.cs_hSocket, socket = requestor.hSocket);


81b00f8

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

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

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.

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

jonatack

Code review ACK 0d04784

jonatack · 2020-09-21T08:35:23Z

test/functional/p2p_getaddr_caching.py

@@ -21,21 +16,9 @@
    assert_equal,
 )

+# As defined in net_processing.


0d04784

-# As defined in net_processing. -MAX_ADDR_TO_SEND = 1000 -MAX_PCT_ADDR_TO_SEND = 23 +MAX_ADDR_TO_SEND = 1000 # As defined in net.h +MAX_PCT_ADDR_TO_SEND = 23 # As defined in net_processing.cpp

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

hebasto · 2020-09-21T10:00:09Z

Concept ACK.

0d04784 Refactor the functional test (Gleb Naumenko) 83ad65f Address nits in ADDR caching (Gleb Naumenko) 81b00f8 Add indexing ADDR cache by local socket addr (Gleb Naumenko) 42ec558 Justify the choice of ADDR cache lifetime (Gleb Naumenko) Pull request description: This is a follow-up on bitcoin#18991 which does 3 things: - improves privacy of a node listening to multiple addresses via adding cache index by local socket addr (suggested [here](bitcoin#18991 (comment))) - documents on the choice of 24h cache lifetime - addresses nits from bitcoin#18991 ACKs for top commit: jnewbery: utACK 0d04784 vasild: ACK 0d04784 jonatack: Code review ACK 0d04784 Tree-SHA512: bb65a34dd1ce2811186d3e4469bc33e8399cebaaa494ce13041c7cff23275870e4176a719f7a72f8d779c49f8b2344bf4fa1aeb3ea4e2626d5ae76514f00a750

vasild · 2020-11-24T14:34:50Z

src/net.cpp

+    SOCKET socket;
+    WITH_LOCK(requestor.cs_hSocket, socket = requestor.hSocket);
+    auto local_socket_bytes = GetBindAddress(socket).GetAddrBytes();


Can this be simplified like:

Suggested change

SOCKET socket;

WITH_LOCK(requestor.cs_hSocket, socket = requestor.hSocket);

auto local_socket_bytes = GetBindAddress(socket).GetAddrBytes();

auto local_socket_bytes = requestor.addrBind.GetAddrBytes();

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

I think you're right?

Ok, will include this tiny change in another PR:

net: avoid unnecessary GetBindAddress() call

commit 15ba60e9a Parent: 25dcd4ad9 Author: Vasil Dimov <vd@FreeBSD.org> AuthorDate: Tue Nov 24 15:36:27 2020 +0100 Commit: Vasil Dimov <vd@FreeBSD.org> CommitDate: Tue Nov 24 15:36:27 2020 +0100 net: avoid unnecessary GetBindAddress() call Our local (bind) address is already saved in `CNode::addrBind` and there is no need to re-retrieve it again with `GetBindAddress()`. Also, for I2P connections `CNode::addrBind` would contain our I2P address, but `GetBindAddress()` would return something like `127.0.0.1:RANDOM_PORT`. diff --git a/src/net.cpp b/src/net.cpp index a811732e2..94a424ca3 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2601,15 +2601,13 @@ std::vector<CAddress> CConnman::GetAddresses(size_t max_addresses, size_t max_pc } return addresses; } std::vector<CAddress> CConnman::GetAddresses(CNode& requestor, size_t max_addresses, size_t max_pct) { - SOCKET socket; - WITH_LOCK(requestor.cs_hSocket, socket = requestor.hSocket); - auto local_socket_bytes = GetBindAddress(socket).GetAddrBytes(); + 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(); const auto current_time = GetTime<std::chrono::microseconds>(); auto r = m_addr_response_caches.emplace(cache_id, CachedAddrResponse{});

You just reviewed that! 🥳

Summary: PR description > 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 "Cache responses to GETADDR to prevent topology leaks" #18991 This is a backport of [[bitcoin/bitcoin#19697 | core#19697]] [1/4] bitcoin/bitcoin@42ec558 Test Plan: `ninja` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10290

Summary: This is a backport of [[bitcoin/bitcoin#19697 | core#19697]] [2/4] bitcoin/bitcoin@81b00f8 Depends on D10290 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10291

Summary: This is a backport of [[bitcoin/bitcoin#19697 | core#19697]] [3/4] bitcoin/bitcoin@83ad65f Depends on D10291 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien, baby636 Reviewed By: #bitcoin_abc, Fabien, baby636 Subscribers: baby636, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10292

Summary: This is a backport of [[bitcoin/bitcoin#19697 | core#19697]] [4/4] bitcoin/bitcoin@0d04784 Depends on D10292 Test Plan: `test/functional/test_runner.py p2p_getaddr_caching.py` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10293

naumenkogs changed the title ~~Minor improvements on ADDR caching~~ Improvements on ADDR caching Aug 11, 2020

fanquake added the P2P label Aug 11, 2020

jnewbery reviewed Aug 11, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

promag reviewed Aug 11, 2020

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

luke-jr suggested changes Aug 11, 2020

View reviewed changes

vasild suggested changes Aug 12, 2020

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net.h Show resolved Hide resolved

vasild reviewed Aug 12, 2020

View reviewed changes

src/net.h Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Aug 12, 2020

naumenkogs force-pushed the 2020-08-addr-cache-follow-up branch 2 times, most recently from 2e356dc to 213bd3b Compare August 23, 2020 15:38

DrahtBot removed the Needs rebase label Aug 23, 2020

This was referenced Aug 24, 2020

test: Remove confusing mininode terminology #19760

Merged

Net processing: move ProcessMessage() to PeerLogicValidation #19704

Merged

This was referenced Aug 24, 2020

Add BIP324 encrypted p2p transport de-/serializer (only used in tests) #18242

Closed

p2p: Try to preserve outbound block-relay-only connections during restart #17428

Merged

naumenkogs closed this Aug 24, 2020

naumenkogs reopened this Aug 24, 2020

jnewbery reviewed Aug 24, 2020

View reviewed changes

DrahtBot added the Needs rebase label Aug 24, 2020

naumenkogs force-pushed the 2020-08-addr-cache-follow-up branch from 213bd3b to 49ce876 Compare August 25, 2020 09:57

jnewbery reviewed Aug 25, 2020

View reviewed changes

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

src/net.cpp Outdated Show resolved Hide resolved

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

naumenkogs force-pushed the 2020-08-addr-cache-follow-up branch from 49ce876 to a3f1c2f Compare August 25, 2020 11:09

naumenkogs force-pushed the 2020-08-addr-cache-follow-up branch 2 times, most recently from 51bdb93 to da184b0 Compare August 26, 2020 06:30

DrahtBot added Needs rebase and removed Needs rebase labels Aug 26, 2020

naumenkogs added 2 commits August 27, 2020 10:51

Justify the choice of ADDR cache lifetime

42ec558

Add indexing ADDR cache by local socket addr

81b00f8

naumenkogs force-pushed the 2020-08-addr-cache-follow-up branch from da184b0 to f680ae4 Compare August 27, 2020 07:54

DrahtBot removed the Needs rebase label Aug 27, 2020

vasild approved these changes Sep 1, 2020

View reviewed changes

src/net.h Outdated Show resolved Hide resolved

naumenkogs added 2 commits September 2, 2020 10:33

Address nits in ADDR caching

83ad65f

Refactor the functional test

0d04784

naumenkogs force-pushed the 2020-08-addr-cache-follow-up branch from f680ae4 to 0d04784 Compare September 2, 2020 07:34

vasild approved these changes Sep 2, 2020

View reviewed changes

naumenkogs closed this Sep 8, 2020

naumenkogs reopened this Sep 8, 2020

promag reviewed Sep 20, 2020

View reviewed changes

jonatack reviewed Sep 21, 2020

View reviewed changes

laanwj merged commit c0c409d into bitcoin:master Sep 21, 2020

vasild reviewed Nov 24, 2020

View reviewed changes

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Improvements on ADDR caching #19697

Improvements on ADDR caching #19697

Uh oh!

Conversation

naumenkogs commented Aug 11, 2020

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

practicalswift commented Aug 14, 2020

Uh oh!

naumenkogs commented Aug 23, 2020

Uh oh!

DrahtBot commented Aug 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jnewbery commented Aug 25, 2020

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

naumenkogs commented Sep 2, 2020

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Sep 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Sep 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

DrahtBot commented Aug 24, 2020 •

edited

Loading