[p2p] Reduce addr blackholes

amitiuttarwar commented at 4:18 am on March 25, 2021: contributor

This PR builds on the test refactors extracted into #22306 (first 5 commits).

This PR aims to reduce addr blackholes. When we receive an addr message that contains 10 or less addresses, we forward them to 1-2 peers. This is the main technique we use for self advertisements, so sending to peers that wouldn’t relay would effectively “blackhole” the trickle. Although we cannot prevent this in a malicious case, we can improve it for the normal, honest cases, and reduce the overall likelihood of occurrence. Two known cases where peers would not participate in addr relay are if they have connected to you as a block-relay-only connection, or if they are a light client.

This implementation defers initialization of m_addr_known until it is needed, then uses its presence to decide if the peer is participating in addr relay. For outbound (not block-relay-only) peers, we initialize the filter before sending the initial self announcement when processing their version message. For inbound peers, we initialize the filter if/when we get an addr related message (ADDR, ADDRV2, GETADDR). We do NOT initialize the filter based on a SENDADDRV2 message.

To communicate about these changes beyond bitcoin core & to (try to) ensure that no other software would be disrupted, I have:

Posted to the mailing list
Researched other open source clients to confirm compatibility, opened issues in all the projects & documented in #21528 (comment). Many have confirmed that this change would not be problematic.
Raised as topic during bitcoin-core-dev meeting
Raised as topic during bitcoin p2p meeting

fanquake added the label P2P on Mar 25, 2021

in src/net_processing.cpp:2751 in cd55b53800 outdated

2624@@ -2625,7 +2625,11 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
2625             pfrom.fDisconnect = true;
2626             return;
2627         }
2628-        pfrom.m_wants_addrv2 = true;
2629+
2630+        if (SetupAddressRelay(pfrom)) {

ajtowns commented at 5:32 am on March 25, 2021:

I don’t think there’s any need to SetupAddressRelay here, and doing so would cause you to continue choosing inbounds that are treating you as block-relay-only instead of ignoring them as a black hole.

jnewbery commented at 10:30 am on March 25, 2021:

I don’t think it matters much either way. The only peers this affects are inbound block-relay-only connections from v0.21 peers, which (incorrectly) send sendaddrv2 to us. Inbound block-relay-only peers on v22 won’t send sendaddrv2 because of the change in commit Stop sending SENDADDRV2 message to block relay only peers.

amitiuttarwar commented at 1:05 am on March 30, 2021:

ergh, its annoying that v0.21 peers will send us the sendaddrv2 message, but I’m not sure if this warrants removing this conditional. some thoughts:

in terms of protocol & logical coherence, I’d expect sendaddrv2 to indicate desire that the node wants to participate in addr relay
in this patch, calling SetupAddressRelay for the node mimics the behavior of current master. so I think of calling as the default (rather than the other way around)
in terms of safety, it seems better to have a blackhole than exclude a peer from address relay. I think… a blackhole means an address, or set of addresses will not propagate as well this time, but excluding a peer from address relay (esp if programmatically / many peers exclude) means that peer could be at a higher risk of being eclipsed?

I thought I was on the fence, but writing out this reasoning makes me realize I’d like to leave as is.

thoughts?

amitiuttarwar commented at 1:34 am on June 15, 2021:

Leaving this as is - I find it most logically consistent for sendaddrv2 to indicate “interested in addresses”. Opened #22245 to change the behavior for inbound block-relay-only v22.0 peers.

DrahtBot commented at 8:54 am on March 25, 2021: 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:

#21515 by naumenkogs
#15606 by jamesob

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:615 in ea30030216 outdated

610@@ -611,6 +611,8 @@ void CNode::copyStats(CNodeStats &stats, const std::vector<bool> &m_asmap)
611     stats.addrLocal = addrLocalUnlocked.IsValid() ? addrLocalUnlocked.ToString() : "";
612 
613     X(m_conn_type);
614+
615+    stats.addr_relay_enabled = RelayAddrsWithConn();

jnewbery commented at 10:21 am on March 25, 2021:

This isn’t thread safe. By accessing m_addr_known from outside the message handler thread (eg an rpc worker thread or qt), this opens race conditions where m_addr_known is being set by the message handler thread and read by an rpc thread concurrently. In such cases, the thread reading the value could see the old value, the new value or garbage.

amitiuttarwar commented at 1:09 am on March 30, 2021:

ayo, good catch. it feels like overkill to introduce a lock just to expose a boolean, so I’ve dropped this commit for now. will some of your addr-in-net-processing refactor will make it so we can do this safely?

jnewbery commented at 7:56 am on March 30, 2021:

will some of your addr-in-net-processing refactor will make it so we can do this safely?

Hopefully, yes!

in src/net_processing.cpp:4285 in ea30030216 outdated

4278@@ -4243,12 +4279,17 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
4279                     }
4280                 }
4281             }
4282+
4283             pto->vAddrToSend.clear();
4284-            if (!vAddr.empty())
4285+
4286+            if (!vAddr.empty()) {

jnewbery commented at 10:31 am on March 25, 2021:

I suggest you drop these tidy-up changes in Add some documentation and small style fixes. They don’t seem very related to the rest of the PR :see_no_evil:

amitiuttarwar commented at 1:13 am on March 30, 2021:

sorry I’m not sorry!! I abhor two line if statements, they are great footguns. these changes are pretty trivial, I don’t think they would make sense as a stand-alone PR, or that they increase the complexity of review, so I’d rather leave them in.

jnewbery commented at 8:05 am on March 30, 2021:

I agree that two line if statements are undesirable. If you were already touching these lines, then I’d agree that you should improve them. However, these are the only changes you make to SendMessages() and are unrelated to the rest of the changes in this PR.

I refactor this entire chunk of logic in #21236. Without this tidy-up commit, the two PRs don’t conflict and can be merged in either order. If you include these changes, then the PRs conflict and one will have to be rebased when the other is merged.

amitiuttarwar commented at 9:04 pm on March 30, 2021:

ok! reverted :)

jnewbery commented at 10:32 am on March 25, 2021: member

Concept ACK. Thanks for implementing this!

You’ve introduced a tiny thread safety issue by accessing m_addr_known from multiple threads without a lock. Y’know, this’d be much tidier if the addr data was all contained in net_processing :grin:

jnewbery commented at 10:36 am on March 25, 2021: member

I think that an approach like this was first suggested by AJ in #15759 (comment):

I think since we AdvertiseLocal regularly to our non-block-relay peers, we could just set a flag on the peer if we’ve ever seen an ADDR message from them, and only choose nodes with that flag in RelayAddress?

I think it’s a good idea, since it stops us relaying addrs to both inbound block-relay-only peers and other kinds of black holes.

amitiuttarwar force-pushed on Mar 29, 2021

amitiuttarwar force-pushed on Mar 30, 2021

amitiuttarwar renamed this:
~~[net_processing] Reduce addr blackholes~~
[p2p] Reduce addr blackholes
on Mar 30, 2021

amitiuttarwar commented at 4:53 am on March 30, 2021: contributor

fixed tests & added a test. patch is ready, but leaving as a draft until I gain more confidence that this would not break any other software on the network.

compiling a list of other clients & compatibility with this patch:

Sends getaddr & only accepts addr messages afterwards (link)

✅ Confirmed by @roasbeef (issue)

After connecting to a new outbound peer, sends addr self advertisement, and sometimes sends getaddr. Relevant code linked in the issue.

✅ Confirmed by @evoskuil (issue)

When connecting to an outbound peer, sends a getaddr message.

Same behavior as Bitcoin Core, upon receiving a version message, sends a getaddr to outbound connections unless they are block-relay only, usually also self-advertises. (link)

✅ Confirmed by @pinheadmz (issue).

Upon opening a connection, node will self-advertise & usually send a getaddr message (link).

✅ Confirmed by @piotrnar (issue)

Upon receipt of a version message, node will self-advertise. Node will usually also send a getaddr message to peers.

✅ Confirmed by @micahriggan (issue)

Bitcore nodes typically have dedicated bitcoin node(s) and the addresses are passed in via config.

✅ (issue)

in src/net_processing.cpp:2439 in 9db8ab25b0 outdated

2436@@ -2437,6 +2437,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
2437         }
2438 
2439         if (!pfrom.IsInboundConn() && !pfrom.IsBlockOnlyConn()) {

ccdle12 commented at 10:27 am on March 30, 2021:

nit: is it worth replacing if (... && !pfrom.IsBlockOnlyConn()) with if (... && SetupAddrssRelay(pfrom)) since it already checks if the peer is block-relay-only?

amitiuttarwar commented at 9:05 pm on March 30, 2021:

nice idea, done!

in test/functional/p2p_addr_relay.py:118 in 8e2bcc0cb6 outdated

114@@ -101,6 +115,25 @@ def relay_tests(self):
115 
116         self.nodes[0].disconnect_p2ps()
117 
118+    def blackhole_tests(self):

ccdle12 commented at 10:27 am on March 30, 2021:

nice tests, would it would be overkill to also test for the outbound case?

amitiuttarwar commented at 9:08 pm on March 30, 2021:

definitely not overkill, the inbound vs outbound behavior is different- inbounds we wait to hear an addr message before considering the peer eligible for relay, but with outbounds we setup relay, send getaddr & self-announce (unless its a block-relay-only connection). I added some tests to check these behaviors.

amitiuttarwar force-pushed on Mar 30, 2021

in src/net.h:558 in 0c194ffafd outdated

553+     *  We use the presence of this filter to decide whether a peer is eligible
554+     *  for trickle relay of addr messages. This avoids relaying to peers that
555+     *  are unlikely to forward them, effectively blackholing self
556+     *  announcements. Reasons peers might not support addr relay on the link
557+     *  is if they connected to us as a block-relay-only peer or as a light
558+     *  client.

glozow commented at 9:19 pm on March 30, 2021:

0     *  announcements. Reasons peers might not support addr relay on the link
1     *  include that they connected to us as a block-relay-only peer or they are
2     *  a light client.

amitiuttarwar commented at 1:49 am on June 15, 2021:

updated language

in test/functional/p2p_addr_relay.py:251 in 0c194ffafd outdated

143+        self.send_addr_msg(addr_source, msg, [receiver_peer, blackhole_peer])
144+
145+        assert_equal(receiver_peer.num_ipv4_received, 2)
146+        assert_equal(blackhole_peer.num_ipv4_received, 0)
147+
148+        self.nodes[0].disconnect_p2ps()

glozow commented at 9:41 pm on March 30, 2021:

Since peers are chosen randomly, does it make sense to run self.inbound_blackhole_tests() a few extra times, just to assert it’s not because the node didn’t pick blackhole_peer by chance?

glozow commented at 9:53 pm on March 30, 2021:

I think this test could also be improved by asserting that, after blackhole_peer does send an addr/addrv2/getaddrs/sendaddrv2 message, it can get selected for addr propagation.

glozow commented at 9:57 pm on March 30, 2021:

Another thought - for the blackhole peer, I think could be helpful to assert bytes received for addr-related messages = 0 using getpeerinfo()

amitiuttarwar commented at 11:35 pm on March 30, 2021:

Since peers are chosen randomly..

yes, peers are chosen randomly, but this test is set up to not be random- ipv4 addresses will be relayed to 2 peers that are not the source of the addr message. so both receiver_peer and blackhole_peer would receive the addr if they were eligible. definitely wouldn’t want a flaky test there :)

I think this test could also be improved..

thats a great idea! I’ve added locally, will include in next push.

assert bytes received for addr-related messages = 0

hm, blackhole_peer is a p2p connection, so we can’t call getpeerinfo() on it to get the bytes received directly. that said, we could check bytessent_per_msg on the node to see that no bytes have been sent for an addr message. however, I its a bit brittle & the p2p connection is capturing directly receiving the addr message, so I don’t think that’s adding much value to the test. let me know if you think it’d add test coverage :)

glozow commented at 0:39 am on March 31, 2021:

yes, peers are chosen randomly, but this test is set up to not be random- ipv4 addresses will be relayed to 2 peers that are not the source of the addr message. so both receiver_peer and blackhole_peer would receive the addr if they were eligible. definitely wouldn’t want a flaky test there :)

Oh right it’s always going to be the same one. That makes sense then, that the node has 2 options but doesn’t send it to a peer that hasn’t established addr relay. But wait I’m confused why it would be flaky, I thought blackhole_peer would never get the addr because it hasn’t sent any addr-related messages?

hm, blackhole_peer is a p2p connection, so we can’t call getpeerinfo() on it to get the bytes received directly.

Er I mean, call getpeerinfo() on the node to see bytes received, kinda like this:

 0def sum_addr_messages(msgs):
 1    return msgs['addr'] + msgs['addrv2'] + msgs['getaddr'] + msgs['sendaddrv2']
 2...
 3
 4peerinfo = self.nodes[0].getpeerinfo()
 5# receiver_peer sent us its own addr and a getaddr after the VERSION message, so we relay
 6assert sum_addr_messages(peerinfo[1]['bytesrecv_per_msg']) > 0
 7assert_equal(receiver_peer.num_ipv4_received, 2)
 8
 9# blackhole_peer should not have sent us any addr-related messages, so we don't relay
10assert sum_addr_messages(peerinfo[2]['bytesrecv_per_msg']) == 0
11assert_equal(blackhole_peer.num_ipv4_received, 0)

I just thought it’d be a more precise test, asserting that the addrs aren’t relayed because no addr messages have been received. 😅 nbd

amitiuttarwar commented at 1:06 am on March 31, 2021:

But wait I’m confused why it would be flaky, I thought blackhole_peer would never get the addr because it hasn’t sent any addr-related messages?

yes yes, it would be flaky if it wasn’t deterministic / blackhole_peer was being skipped by chance. I think we’re on the same page, just confusion of my language now.

Er I mean, call getpeerinfo() on the node to see bytes received, kinda like this

oh I get it now, you’re suggesting additional coverage to test the setup, not another way to test the addr relayed. that’s a cool idea! sure, I’ll add it :)

smol note:

receiver_peer sent us its own addr and a getaddr after the VERSION message, so we relay

since receiver_peer is essentially a P2PInterface object, it actually won’t self announce upon receiving version message. I added that it will send out a getaddr so tests wouldn’t unexpectedly break in this patch. but I think that highlights why this suggested coverage is useful, at the very least it communicates the relevant setup that’s otherwise “invisible” from the POV of this test file.

amitiuttarwar commented at 1:51 am on June 15, 2021:

based on this convo, I’ve updated the tests to assert that blackhole_peer starts receiving addr messages after sending one. I’ve also added getpeerinfo for the node to verify the test setup of which peers have sent addr related messages.

thanks!

in src/net_processing.cpp:3589 in 0c194ffafd outdated

3583@@ -3569,6 +3584,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
3584         }
3585         pfrom.fSentAddr = true;
3586 
3587+        SetupAddressRelay(pfrom);
3588+
3589         pfrom.vAddrToSend.clear();

glozow commented at 10:41 pm on March 30, 2021:

Side question: why do we clear this list here instead of appending to it, given that PushAddress and SendMessages filter for duplicates?

glozow commented at 10:57 pm on March 30, 2021: member

Concept ACK to making an effort not to send addrs to peers that will likely ignore them. By “disrupt other software on the network,” is the concern that some nodes who aren’t immediately active in addr-related dialogue might be skipped over and not hear about addrs?

At first I was confused because I figured a lot of nodes don’t care about advertising their own addr (e.g. bootstrapping, in IBD, maybe not accepting inbounds). But it seems like a node making a full relay outbound will always send its own addr and a getaddr, and should thus immediately be a candidate for getting addrs forwarded to it (is this accurate?). Was there a version of Bitcoin Core that didn’t send addr stuff after getting VERSION from outbound?

sipa commented at 5:01 am on March 31, 2021: member

@glozow The concern would be other Bitcoin node implementations which don’t send addr/getaddr, but do care about receiving them. I don’t think that’s particularly likely, but it’s also hard to be sure there are none.

ivanacostarubio commented at 4:06 pm on March 31, 2021: none

Concept ACK

in src/net.h:560 in 0c194ffafd outdated

555+     *  are unlikely to forward them, effectively blackholing self
556+     *  announcements. Reasons peers might not support addr relay on the link
557+     *  is if they connected to us as a block-relay-only peer or as a light
558+     *  client.
559+     * */
560     std::unique_ptr<CRollingBloomFilter> m_addr_known{nullptr};

GeneFerneau commented at 4:46 pm on March 31, 2021:

0    std::unique_ptr<CRollingBloomFilter> m_addr_known GUARDED_BY(cs_addrKnown){nullptr};

and add cs_addrKnown as a private member:

0mutable RecursiveMutex cs_addrKnown;

to address the point about race conditions mentioned by @jnewbery

amitiuttarwar commented at 1:53 am on June 15, 2021:

Holding off on this to try to figure out if there’s a better way I can expose this value through the getpeerinfo RPC than having to introduce a mutex for this one hyper-specific use case. Ideas welcome!

amitiuttarwar commented at 8:42 pm on July 26, 2021:

added addr_relay_enabled as an atomic to address this issue.

GeneFerneau commented at 4:49 pm on March 31, 2021: none

Concept ACK

Need to review surrounding code more, and run the tests. Looks good so far!

MarcoFalke referenced this in commit 0a0a95b9d6 on Apr 26, 2021

jnewbery commented at 10:49 am on June 9, 2021: member

Reiterating my concept ACK on this. Reducing the number of addr black holes seems like a good win for addr gossipping across the network.

This branch needs rebase now that the addr data has been moved up into net_processing. I’d be very happy to help with that if you need a hand.

ajtowns commented at 11:39 am on June 9, 2021: member

Reiterating my concept ACK on this. Reducing the number of addr black holes seems like a good win for addr gossipping across the network.

I think this was somewhat pending being sure it wouldn’t break bitcore (see https://github.com/bitpay/bitcore/issues/3140) which now looks like it’s now resolved.

amitiuttarwar commented at 0:58 am on June 12, 2021: contributor

Reiterating my concept ACK on this

Thanks!

I think this was somewhat pending being sure it wouldn’t break bitcore (see bitpay/bitcore#3140) which now looks like it’s now resolved.

Indeed. I’m now working to rebase & bring this PR out of draft. I’ve rebased locally but am working through some test failures, I hope to have this ready for review by early next week.

PS: I also investigated bitcoinj & opened an issue on their repo, they also seem compatible 🎉.

amitiuttarwar force-pushed on Jun 15, 2021

amitiuttarwar commented at 2:04 am on June 15, 2021: contributor

My research indicates that these changes would not break any clients, I’ve opened issues trying to confirm my understand on all the repos, and I have not heard any opposition from the mailing list or on IRC. This seems like the extent of what I can do to make sure these changes are compatible with alternative clients 😛

I’ve also broken out the commit to “stop sending sendaddrv2 messages to block-relay-only peers” into #22245.

So I’m bringing this PR out of draft. I have rebased it on top of #21236 and #21707. They were both pretty involved, so I recommend reviewing from scratch. The p2p_addr_relay.py functional test is getting pretty big & hits a fair amount of behind-the-scenes p2p mechanisms. I tried to improve the clarity & document relevant mechanisms, but open to suggestions if anything is still unclear.

amitiuttarwar marked this as ready for review on Jun 15, 2021

michaelfolkson commented at 11:35 am on June 16, 2021: contributor

Concept ACK. Impressively thorough investigation of impact on alternative clients :)

vasild commented at 11:48 am on June 16, 2021: member

For inbound peers, we initialize the filter if/when we get an addr related message (ADDR, ADDRV2, GETADDR, SENDADDRV2).

So it may happen that a light client who does not participate in address relay connects to us and sends us getaddr in order to learn some peers and we (mis)interpret that and start sending unrequested addresses to that peer in the hopes of further relay. Black hole again.

Or a peer that does not participate in address relay replies to our getaddr with addr and we (mis)interpret that as “the peer participates in address relay”?

Do we have any estimates of how severe the black holes problem is? I think that currently address relay works, so maybe it is not a “big” problem, or not a problem at all?

What if we relay to 3 instead of 2 random peers? Has that been considered? I would not require protocol changes. Would it solve the black holes problem or diminish it to such extend that it can be ignored?

amitiuttarwar commented at 5:50 am on June 18, 2021: contributor

@vasild

So it may happen that a light client who does not participate in address relay connects to us and sends us getaddr in order to learn some peers and we (mis)interpret that and start sending unrequested addresses to that peer in the hopes of further relay. Black hole again. Or a peer that does not participate in address relay replies to our getaddr with addr and we (mis)interpret that as “the peer participates in address relay”?

I believe this question is addressed in the OP- “Although we cannot prevent this in a malicious case, we can improve it for the normal, honest cases, and reduce the overall likelihood of occurrence.”

Incase that is unclear, let me try to provide some additional context- the goal of this PR is not to ensure perfect accuracy of whether or not the receiving peer would forward the addresses. In fact, I don’t think we could ever ensure such accuracy- even if we had a p2p message that said “I will forward addresses you send me”, that could easily be abused by a peer who doesn’t actually follow through.

The goal of this PR is to reduce the frequency of sending addresses to nodes who have no interest in them & will certainly not forward those along. For example, we know that an honest node who is running the latest version of bitcoin core will have block-relay-only connections where it will drop any addresses sent over that connection.

Do we have any estimates of how severe the black holes problem is? I think that currently address relay works, so maybe it is not a “big” problem, or not a problem at all?

While I do think this PR offers an improvement to address relay, the end goal is to increase the number of block-relay-only connections created by a bitcoin node (also goal of the DISABLETX work in #20726). Even if the number of black holes are not currently a problem, fixing them are a prerequisite to being able to increase the number of block-relay-only connections. For more context on this conversation, please see #15759 (comment) & the surrounding discussion.

What if we relay to 3 instead of 2 random peers? Has that been considered? I would not require protocol changes. Would it solve the black holes problem or diminish it to such extend that it can be ignored?

Relaying addresses to 3 rather than 2 random peers could be possible, but I think it would require in-depth research to ensure there wouldn’t be any negative bandwidth or privacy impact. One of the alternative approaches that has been considered is to mark “potential blackholes” and then increase redundancy if they have been selected in RelayAddress. A downside of that approach is it’s hard to build confidence that such approach wouldn’t lead to network-wide biases of certain addresses or certain peers.

I agree with what @sdaftuar said in #22245 (comment):

I don’t think anyone believes that implicit behavior (that is undocumented in any BIP) is the right way for protocol development to proceed if starting from a clean slate — so eventually I think we’ll rewrite things.

The current state of addr propagation relies heavily on assumptions & is poorly understood and poorly documented. In the long term, I hope to work towards a shared set of design goals & more explicit expectations that are well documented. However, this PR is coming from the hope that we don’t have to gate making progress towards increasing block-relay-only connections on a fundamental redesign of addr relay.

vasild commented at 10:40 am on June 18, 2021: member

If the focus is on avoiding gossip to peers that connected to us in block-only mode, then isn’t the absence of getaddr enough to recognize that? That is, a peer who connected to us and did not send us getaddr must be in blocks-only mode / is a black hole? No need to involve addr and addrv2? I.e. only for inbound connections, delay initializing Peer::m_addr_known for until after getaddr is received.

Given that “when” and “to whom” to relay addresses is not mandated by any specification and is up to each peer to decide, and one may as well decide to relay only on full moon on odd months or if the peer’s address ends in 7, then deciding to skip relay to peers that did not send getaddr is not a protocol change.

I see the assumption “the peer did not send us messageX => they must be a black hole” as flawed. More like a hack, which might be acceptable in the current situation.

Still sending to some black holes is ok (we currently do). But could this PR have the adverse effect of starting to treat as black holes peers that are actually not?

As for relaying to more peers: currently we relay reachable addresses to 2 peers. So (assuming everybody relays) every node relays to two others (it spreads like a virus). And an average spread rate of <1 is required to stop an address from propagating. It means that if more than half of the peers are black holes then propagation will not work (will stop). I think we are far from 50% black holes but I could be wrong. For unreachable peers (e.g. a Tor address relayed via a peer that does not have Tor connectivity) the number is 1.5 and so, 33% or more black holes are needed to brick propagation.

amitiuttarwar commented at 6:17 pm on June 18, 2021: contributor

@vasild

That is, a peer who connected to us and did not send us getaddr must be in blocks-only mode / is a black hole?

This is false. If you see the notes I’ve taken on alternative clients here: #21528 (comment), you can see that several of them will always self-advertise, but only occasionally send getaddr messages.

Still sending to some black holes is ok (we currently do). But could this PR have the adverse effect of starting to treat as black holes peers that are actually not?

Yes, exactly. This is why I have taken great care to communicate these changes & research clients to ensure they would not be harmed. If we only used getaddr to indicate “wants to participate in addr relay”, this exact adverse effect would occur.

As for relaying to more peers: currently we relay reachable addresses to 2 peers. So (assuming everybody relays) every node relays to two others (it spreads like a virus). And an average spread rate of <1 is required to stop an address from propagating. It means that if more than half of the peers are black holes then propagation will not work (will stop). I think we are far from 50% black holes but I could be wrong. For unreachable peers (e.g. a Tor address relayed via a peer that does not have Tor connectivity) the number is 1.5 and so, 33% or more black holes are needed to brick propagation.

As I said in my previous post, “Relaying addresses to 3 rather than 2 random peers could be possible, but I think it would require in-depth research to ensure there wouldn’t be any negative bandwidth or privacy impact.” There is no doubt that relaying addresses to more peers could ensure they saturate the network. Heck, if that was all we cared about, why not forward addresses to every node?

The concern is if there would be any other adverse impact, and that is difficult to reason about. We could model the bandwidth increase by estimating: number of nodes on network, expected frequency of initiating self-announcements, number of anticipated blackholes & multiplier of forwarding addresses. This could help build confidence, but a lot of these numbers would have to be estimates. Even harder would be to guarantee that there are no significant security concerns. For example, addr relay can reveal node topology which can ease a partition or eclipse attack. I believe it would be significantly more complex to usefully model how changing the relay policy would impact potential information leaks. And that wouldn’t be the only security concern about changing addr relay policy, just one example.

mzumsande commented at 0:08 am on June 21, 2021: member

Concept ACK.

As for relaying to more peers: currently we relay reachable addresses to 2 peers. So (assuming everybody relays) every node relays to two others (it spreads like a virus). And an average spread rate of <1 is required to stop an address from propagating. It means that if more than half of the peers are black holes then propagation will not work (will stop). I think we are far from 50% black holes but I could be wrong.

I think that there are at least two limitations to a virus-like spread:

Each full node relays a given addr only once to two peers, and becomes a black hole for it after that - if it receives the addr again from another peer, it won’t relay it to different peers within 24h because of how RelayAddress() works. This makes addr gossip sensitive to network topology (hubs with many peers are bottlenecks) and makes it pretty much impossible for an addr to percolate through the entire network via gossip relay. Even with zero black holes, an addr would only reach ~35% of the nodes in my simulation.
Each node in the network will stop relaying an addr 10 minutes after the announcement (that’s 20 hops on average with AVG_ADDRESS_BROADCAST_INTERVAL = 30s), see first condition here

My impression is that the introduction of 20% block-relay-only connections could have effected addr gossip relay quite clearly. I did some quick simulations similar to @naumenkogs’ work from #17194 but adjusted for points 1 and 2 from above: The simulations (code) suggest that the percentage of nodes reached by an initial self-announcement of a new non-listening node falls from 35% (0% black holes) to 21% (20% black holes) to 2% (35% black holes).

I think that currently address relay works, so maybe it is not a “big” problem, or not a problem at all?

I think that whether addr gossip relay “works” is not so easy to judge because other means of addr propagation (DNS seeds; getaddr mechanism) also contribute to addr propagation and introduce some degree of redundancy - maybe the best measure is how fast a new node on the network receives inbound connections? But getaddr does not adapt quickly (older addrs, 24 hour cache) to changes in the network, so it makes sense to me to improve the state of gossip relay as suggested by this PR.

ajtowns commented at 5:30 am on June 21, 2021: member

The simulations (code) suggest that the percentage of nodes reached by an initial self-announcement of a new non-listening node falls from 35% (0% black holes) to 21% (20% black holes) to 2% (35% black holes).

I think that’s simulating having 8 outbounds of which 20% (or 30%,35%,40%) are blackholes, rather than 8 full outbounds and an additional 2 blackholes.

Might be worth distinguishing between the percentage of listening nodes that see an address versus total nodes – I think with 8 outbounds, 0% blackholes it’s 92% of listening nodes and 35% of all nodes that see an address, and 20% blackholes (6.4 full outbound 1.6 block relay only) gives 65% of listening nodes and 21% of all nodes see an address.

I tried writing my own sim which only deals with listening peers; the numbers I got were 8 full-relay 0 block-relay gives ~80% of nodes seeing an address on average, while 8 full-relay 2 block-relay gives ~70% coverage, and 8 full-relay 5 block-relay gives ~50% coverage. Switching from 2 peers to 3 for addr relay brings those numbers up to 95%, 92% and 85% respectively.

vasild commented at 2:48 pm on June 21, 2021: member

@mzumsande, @ajtowns, wow! That gives some rough picture on the severity of the problem. I think it rules out “not a problem at all”. Thanks! @amitiuttarwar, I guess the “waiting for confirmation” for btcd and bitcoinj above should be cleared before this PR is merged?

amitiuttarwar closed this on Jun 21, 2021

amitiuttarwar reopened this on Jun 21, 2021

amitiuttarwar force-pushed on Jun 22, 2021

amitiuttarwar commented at 3:47 am on June 22, 2021: contributor

@mzumsande, thanks for putting together that awesome simulation! and thank you @ajtowns for reviewing :) very cool.

biggest changes in the recent push:

as per the conversation on #22245, I’ve removed the commit that uses sendaddrv2 as a signal to set up addr relay on the connection
I was finding the p2p_addr_relay.py refactor commit hard to parse, so I pulled it out into #22306 and broke it down into multiple commits. this PR builds on that branch.

ajtowns commented at 7:09 am on June 22, 2021: member

Had a bit more of a poke at this, I think the interesting results are (with 10k listening nodes, 50k private nodes with only outbounds):

0Conn: 8+2 Spread: 2 Limit: 10000/600 Avg/dev: 78.5%±9.7% 26.1%±3.5%
1Conn: 8+0 Spread: 2 Limit: 10000/600 Avg/dev: 90.5%±1.7% 33.5%±1.0%
2Conn: 8+5 Spread: 2 Limit: 10000/600 Avg/dev: 44.5%±17.0% 13.0%±5.1%
3
4Conn: 8+5 Spread: 3 Limit: 10000/600 Avg/dev: 96.5%±9.7% 35.8%±3.6%
5Conn: 8+0 Spread: 3 Limit: 10000/600 Avg/dev: 99.6%±0.1% 46.1%±0.2%

That is, with 8 regular outbounds, sending addresses to 2 peers gives ~78% coverage of listening peers (and ~26% coverage of total peers) with 2 additional block-relay-only/blackhole peers, or ~90% coverage with this patch, or ~44% coverage without this patch and 5 additional block-relay-only peers instead of 2.

With a spread of 3 instead, then 5 block-relay-only peers gets to 96% coverage even without this patch (though with a large stddev), and with this patch as well gets to 99% coverage with a small standard deviation. So this seems like a “both” rather than an “either/or” situation to me.

Addr relay is pretty low bandwidth, so I think the extra 50% of data for going from 2 to 3 is probably pretty fine (maybe make it 3 peers of which at most 2 are outbounds so that private peers don’t increase traffic to listening nodes?); I’m not seeing what the privacy concerns should really be.

I think these numbers are a bit more robust and better match @mzumsande’s results. I switched the limit to 600s with poisson timing rather than 20 hops; provided there were enough samples the results between the two methods seemed pretty consistent.

So in summary, Concept ACK?

vasild commented at 12:06 pm on June 22, 2021: member

Addr relay is pretty low bandwidth, so I think the extra 50% of data for going from 2 to 3 is probably pretty fine…

I agree.

I’m not seeing what the privacy concerns should really be.

Me neither.

I reproduced the above results and ran one more simulation: just increase spread from 2 to 3:

0# current (copy from above)
1Conn: 8+2 Spread: 2 Limit: 10000/600 Avg/dev: 78.5%±9.7% 26.1%±3.5%
2
3# just increase spread from 2 to 3
4Conn: 8+2 Spread: 3 Limit: 10000/600 Avg/dev: 99.1%/0.1% 41.4%/0.2%
5
6# increase spread from 2 to 3 and reduce blackholes as per this PR (copy from above)
7Conn: 8+0 Spread: 3 Limit: 10000/600 Avg/dev: 99.6%±0.1% 46.1%±0.2%

So, assuming spread is increased from 2 to 3 and we are at 99.1%/41.4% then this PR will only improve it to 99.6%/46.1% :eyes:

Also to consider relay of addresses from non-reachable networks, currently with spread 1.5. The script gives an error when given a float: TypeError: can’t multiply sequence by non-int of type ‘float’.

ajtowns commented at 6:43 am on June 23, 2021: member

Also to consider relay of addresses from non-reachable networks, currently with spread 1.5.

Changing the spread to random.choice([1,2]) to simulate an address everyone considers unreachable gives about 5% coverage with 8 outbounds and 2 blackholes; but 27% with 8 outbounds and no blackholes (ie behaviour prior to block relay only nods or after this pr). If you wanted to model ipv4 vs ipv6 or tor, you’d need to model some nodes as considering those nets reachable and being peered with other nodes on the network (preferentially?) which would get complicated.

MarcoFalke referenced this in commit bfa885898a on Jun 24, 2021

DrahtBot added the label Needs rebase on Jul 19, 2021

amitiuttarwar force-pushed on Jul 26, 2021

amitiuttarwar commented at 8:41 pm on July 26, 2021: contributor

Latest push:

rebased on master
introduced a new variable, addr_relay_enabled, to the Peer struct. this is used to expose this information to getpeerinfo without causing a thread safety issue. The bool made the RelayAddrsWithPeer function redundant, so I removed that.
these commits are generally small & isolated, so I’m hoping this PR is pretty straightforward to review :)

Status of this PR:

Ready for review! 🔎
My research of alternative clients suggests these changes would not harm other software: #21528 (comment)
I believe I have addressed all of the conceptual concerns that have been expressed by reviewers.

Future thought:

If this PR gets accepted & released, we should keep an eye on the peer rate limiting introduced in #22387. MAX_ADDR_RATE_PER_SECOND is currently set to 0.1 based on network observations + some buffer. If this patch were widely adopted, it’s possible the observed rate could naturally increase.

☝️ Further explanation: based on an estimated number of listening nodes on the network & frequency of self announcements in Bitcoin Core, I was surprised to find the observed addr announcement rate / per / second to be ~0.025. I think a large part of the delta between conceptual math & observed frequency arises from blackhole rate (aka how well does a single announcement propagate) combined with the role of m_addr_known, which would fluctuate based on the node-path a specific announcement takes.

Anyway, just want to mention that since this is a network level phenomena, its hard for us to accurately predict, and thus would want to observe.

DrahtBot removed the label Needs rebase on Jul 26, 2021

in src/net_processing.cpp:1699 in d16a2296bf outdated

1695@@ -1681,7 +1696,7 @@ void PeerManagerImpl::RelayAddress(NodeId originator,
1696     LOCK(m_peer_mutex);
1697 
1698     for (auto& [id, peer] : m_peer_map) {
1699-        if (RelayAddrsWithPeer(*peer) && id != originator && IsAddrCompatible(*peer, addr)) {
1700+        if ( peer->addr_relay_enabled && id != originator && IsAddrCompatible(*peer, addr)) {

jnewbery commented at 9:53 am on July 27, 2021:

0        if (peer->addr_relay_enabled && id != originator && IsAddrCompatible(*peer, addr)) {

amitiuttarwar commented at 9:13 pm on July 27, 2021:

fixed

in test/functional/p2p_addr_relay.py:181 in d16a2296bf outdated

177@@ -166,11 +178,13 @@ def relay_tests(self):
178         # large GETADDR responses from being relayed, it now typically affects the self-announcement
179         # of the outbound peer which is often sent before the GETADDR response.
180         assert_equal(inbound_peer.num_ipv4_received, 0)
181+        inbound_peer.send_and_ping(msg_getaddr())

jnewbery commented at 9:53 am on July 27, 2021:

Is this new getaddr message to trigger addr relay from the node? What do you think about just sending an empty addr message instead?

amitiuttarwar commented at 9:15 pm on July 27, 2021:

good idea! done & added a comment

in test/functional/p2p_addr_relay.py:206 in d16a2296bf outdated

197@@ -184,7 +198,68 @@ def relay_tests(self):
198 
199         self.nodes[0].disconnect_p2ps()
200 
201+    def sum_addr_messages(self, msgs_dict):
202+        val = 0
203+        for msg in ['addr', 'addrv2', 'getaddr', 'sendaddrv2']:
204+            if msg in msgs_dict:
205+                val += msgs_dict[msg]
206+        return val

jnewbery commented at 10:01 am on July 27, 2021:

0        return sum(bytes_recvd for (msg, bytes_recvd) in msgs_dict.items() if msg in ['addr', 'addrv2', 'getaddr', 'sendaddrv2'])

jnewbery commented at 10:12 am on July 27, 2021:

Also, remove sendaddrv2 from the list?

amitiuttarwar commented at 9:16 pm on July 27, 2021:

done and done

in src/net_processing.cpp:3754 in d16a2296bf outdated

3742@@ -3725,6 +3743,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
3743         }
3744         peer->m_getaddr_recvd = true;
3745 
3746+        SetupAddressRelay(pfrom);

jnewbery commented at 10:03 am on July 27, 2021:

Perhaps move this above the m_getaddr_recvd logic (to avoid reading the unused m_getaddr_recvd value for non-addr-relay peers).

amitiuttarwar commented at 8:07 pm on July 27, 2021:

I don’t follow- there is an earlier clause that returns early if !IsInboundConn, so SetupAddressRelay should never return false. So I don’t think the order matters here?

jnewbery commented at 8:41 am on July 28, 2021:

Logically, I think it makes sense to not access any addr data members until after SetupAddressRelay() is called. You’re right that for now the ordering doesn’t matter, but I could imagine in the future we might want to collect the addr relay data members (m_addrs_to_send, m_getaddr_sent, m_addr_send_times_mutex, m_next_addr_send, etc) into a struct which is initialized by SetupAddressRelay(), and only access those members after calling SetupAddressRelay().

I’ll mark this comment as resolved.

amitiuttarwar commented at 10:10 pm on July 28, 2021:

ah gotcha, ok! moved

in src/net_processing.cpp:2590 in d16a2296bf outdated

2586@@ -2572,7 +2587,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
2587         UpdatePreferredDownload(pfrom, State(pfrom.GetId()));
2588         }
2589 
2590-        if (!pfrom.IsInboundConn() && !pfrom.IsBlockOnlyConn()) {
2591+        // Self advertisement logic

jnewbery commented at 10:07 am on July 27, 2021:

This is a bit misleading. The logic block below contains self-advertisement logic, but it also sends a getaddr to request addresses, which is not self-advertisement.

amitiuttarwar commented at 9:16 pm on July 27, 2021:

good point, updated to include

in src/net_processing.cpp:234 in d16a2296bf outdated

232+     *  We initialize this filter for outbound peers (other than
233+     *  block-relay-only connections) or when an inbound peer sends us an
234+     *  address related message (ADDR, ADDRV2, GETADDR).
235+     *
236+     *  We use the presence of this filter to decide whether a peer is eligible
237+     *  for trickle relay of addr messages. This avoids relaying to peers that

jnewbery commented at 10:11 am on July 27, 2021:

I’d very much prefer not to introduce the word ’trickle’ here. I’ve only ever heard the word ’trickle’ being used to refer to transaction propagation (eg #7125). Adding the word here would be overloading the term with a new meaning.

amitiuttarwar commented at 9:17 pm on July 27, 2021:

ah gotcha. I updated the language to use “gossiping addr messages”, does that feel more appropriate / consistent?

(also mentioned in #21528 (review), linking as a reminder to myself for potential future updates)

jnewbery commented at 9:45 pm on July 27, 2021:

“gossiping addr messages” SGTM!

in test/functional/p2p_addr_relay.py:241 in d16a2296bf outdated

236+        # Confirm node has not received addr-related messages from blackhole peer
237+        assert_equal(self.sum_addr_messages(peerinfo[2]['bytesrecv_per_msg']), 0)
238+        # And that peer did not receive addresses
239+        assert_equal(blackhole_peer.num_ipv4_received, 0)
240+
241+        self.log.info("After blackhole peer sends addr message, it becomes eligible for trickle relay")

jnewbery commented at 10:11 am on July 27, 2021:

Same comment as above about the word “trickle”

amitiuttarwar commented at 9:18 pm on July 27, 2021:

convo continued in #21528 (review), resolving this thread

in src/net_processing.cpp:238 in d16a2296bf outdated

236+     *  We use the presence of this filter to decide whether a peer is eligible
237+     *  for trickle relay of addr messages. This avoids relaying to peers that
238+     *  are unlikely to forward them, effectively blackholing self
239+     *  announcements. Reasons peers might not support addr relay on the link
240+     *  include that they connected to us as a block-relay-only peer or they
241+     *  are a light client.

jnewbery commented at 10:22 am on July 27, 2021:

This is no longer true. We use addr_relay_enabled to decide whether a peer is eligible for addr relay.

amitiuttarwar commented at 9:19 pm on July 27, 2021:

ah good point. updated the comments on both m_addr_known and m_addr_relay_enabled

in src/net_processing.cpp:243 in d16a2296bf outdated

241+     *  are a light client.
242+     **/
243+    std::unique_ptr<CRollingBloomFilter> m_addr_known;
244+    /** Whether we are participating in address relay with this connection.
245+     *  Must correlate with whether m_addr_known has been initialized. */
246+    std::atomic_bool addr_relay_enabled{false};

jnewbery commented at 10:22 am on July 27, 2021:

0    std::atomic_bool m_addr_relay_enabled{false};

amitiuttarwar commented at 9:19 pm on July 27, 2021:

done, thanks

in src/net_processing.h:34 in d16a2296bf outdated

30@@ -31,6 +31,7 @@ struct CNodeStateStats {
31     std::vector<int> vHeightInFlight;
32     uint64_t m_addr_processed = 0;
33     uint64_t m_addr_rate_limited = 0;
34+    bool addr_relay_enabled;

jnewbery commented at 10:27 am on July 27, 2021:

0    bool m_addr_relay_enabled;

amitiuttarwar commented at 9:20 pm on July 27, 2021:

done

in src/net_processing.cpp:4458 in d16a2296bf outdated

4453+    PeerRef peer = GetPeerRef(node.GetId());
4454+    if (!peer->addr_relay_enabled) {
4455+        // First addr message we have received from the peer, initialize
4456+        // m_addr_known
4457+        peer->m_addr_known = std::make_unique<CRollingBloomFilter>(5000, 0.001);
4458+        peer->addr_relay_enabled = true;

jnewbery commented at 10:29 am on July 27, 2021:

You can atomically test and set the flag as follows:

0    if (!peer->addr_relay_enabled.exchange(true)) {
1        // First addr message we have received from the peer, initialize
2        // m_addr_known
3        peer->m_addr_known = std::make_unique<CRollingBloomFilter>(5000, 0.001);

(std::atomic<T>::exchange() returns the value of the object before the call (https://en.cppreference.com/w/cpp/atomic/atomic/exchange))

amitiuttarwar commented at 9:20 pm on July 27, 2021:

nice trick! done

jnewbery commented at 10:32 am on July 27, 2021: member

Looks good.

I think the use of an atomic addr_relay_enabled is fine for now. Perhaps once #21527 has landed and we have proper locking in net_processing, m_addr_known can be guarded by the net processing lock and we can lose the additional atomic bool.

amitiuttarwar force-pushed on Jul 27, 2021

amitiuttarwar commented at 9:22 pm on July 27, 2021: contributor

thanks for the review @jnewbery! incorporated all your feedback with one or two questions.

I think the use of an atomic addr_relay_enabled is fine for now. Perhaps once #21527 has landed and we have proper locking in net_processing, m_addr_known can be guarded by the net processing lock and we can lose the additional atomic bool.

sounds good, I would definitely prefer that approach.

in src/net_processing.cpp:643 in ceb20ad575 outdated

638+     * the m_addr_known bloom filter and sets m_addr_relay_enabled to true.
639+     *
640+     *  @return   True if address relay is enabled with peer
641+     *            False if address relay is disallowed
642+     */
643+    bool SetupAddressRelay(CNode& node) const;

jnewbery commented at 8:33 am on July 28, 2021:

I’m not sure it makes sense to declare this method const. It doesn’t mutate the state of the PeerManagerImpl object itself, but it can mutate a Peer object, which is owned by PeerManagerImpl.

amitiuttarwar commented at 10:09 pm on July 28, 2021:

removed

in src/net_processing.h:34 in ceb20ad575 outdated

30@@ -31,6 +31,7 @@ struct CNodeStateStats {
31     std::vector<int> vHeightInFlight;
32     uint64_t m_addr_processed = 0;
33     uint64_t m_addr_rate_limited = 0;
34+    bool m_addr_relay_enabled;

jnewbery commented at 11:22 am on July 28, 2021:

Maybe explicitly default initialize this to false:

0    bool m_addr_relay_enabled{false};

amitiuttarwar commented at 10:09 pm on July 28, 2021:

done

jnewbery commented at 11:23 am on July 28, 2021: member

Code review ACK ceb20ad575cb2cbe971844e8a6422eca87d44bbb

In thread #21528 (review) I’ve suggested follow-up work where all of the address data members are stored in a sub structure that only gets initialized if we’re relaying addresses with the peer. I have an example implementation of that here: https://github.com/jnewbery/bitcoin/tree/pr21528.1 if you’re interested.

amitiuttarwar force-pushed on Jul 28, 2021

amitiuttarwar commented at 10:12 pm on July 28, 2021: contributor

@jnewbery

incorporated all your feedback
also took a look at your branch, it looks awesome! I think it makes a lot of sense to encapsulate the address related data into a small data structure & only initialize where needed. I’d be happy to see this sorta thing through as a follow-up, if these changes get accepted. thank you!

jnewbery commented at 11:28 am on July 29, 2021: member

Code review ACK 00a54cbddbc6bc69c30a9597f6e669b1af331d65

in src/net_processing.cpp:4441 in 4ef28624c3 outdated

4436+    // We don't participate in addr relay with outbound block-relay-only
4437+    // connections to prevent providing adversaries with the additional
4438+    // information of addr traffic to infer the link.
4439+    if (node.IsBlockOnlyConn()) return false;
4440+
4441+    PeerRef peer = GetPeerRef(node.GetId());

MarcoFalke commented at 11:43 am on July 29, 2021:

Any reason to not pass in PeerRef peer from outside and remove this line?

amitiuttarwar commented at 5:11 pm on July 29, 2021:

no particular reason not to, I’m trying to understand the differences…

current implementation: uses GetPeerRef which constructs an additional shared_ptr, and would have to update the control block used to coordinate between the pointers.

your suggestion: passing in PeerRef peer, doesn’t this increment the refcount at the beginning of the function & decrement at the end? so what difference does it make?

another alternative: passing in Peer& peer. does this mean the lifetime of the reference would just be managed in the normal program execution? so would that mean this is the most efficient?

MarcoFalke commented at 5:16 pm on July 29, 2021:

Also, you wouldn’t need to check for nullptr, as the caller already did that

jnewbery commented at 5:25 pm on July 29, 2021:

another alternative: passing in Peer& peer. does this mean the lifetime of the reference would just be managed in the normal program execution? so would that mean this is the most efficient?

This is the way to do it. The called function is not taking ownership of Peer, so just pass by reference. You only need to pass smart pointers when you’re indicating ownership semantics: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-inout (and items F26/27)

amitiuttarwar commented at 0:49 am on July 30, 2021:

👍 done, thanks!

in src/net_processing.cpp:245 in 7e4662b8a0 outdated

240+     *  block-relay-only connections), or when an inbound peer sends us an
241+     *  address related message (ADDR, ADDRV2, GETADDR).
242+     *
243+     *  We use this bool to decide whether a peer is eligible for gossiping
244+     *  addr messages. This avoids relaying to peers that are unlikely to
245+     *  forward them, effectively blaackholing self announcements. Reasons

MarcoFalke commented at 12:00 pm on July 29, 2021:

7e4662b8a0ea0428aa9eefe7b8116f85e5eda486: blackholing

amitiuttarwar commented at 0:48 am on July 30, 2021:

*** blaaaaackholing

:) fixed

[net_processing] Introduce SetupAddressRelay

Idempotent function that initializes m_addr_known for connections that support
address relay (anything other than block-relay-only). Unused until the next
commit.

2fcaec7bbb

[test] Update p2p_addr_relay test to prepare

Use an init param to make clear whether a getaddr message should be sent when
the P2PConnection receives a version message. These changes are in preparation
for upcoming commits that modify the behavior of a bitcoind node and the test
framework.

6653fa3328

[net_processing] Defer initializing m_addr_known

Use SetupAddressRelay to only initialize `m_addr_known` as needed. For outbound
peers, we initialize the filter before sending our self announcement (not
applicable for block-relay-only connections). For inbound peers, we initialize
the filter when we get an addr related message (ADDR, ADDRV2, GETADDR).

These changes intend to mitigate address blackholes. Since an inbound peer has
to send us an addr related message to become eligible as a candidate for addr
relay, this should reduce our likelihood of sending them self-announcements.

1d1ef2db7e

[net_processing] Introduce new field to indicate if addr relay is enabled 201e496481

[net_processing] Remove RelayAddrsWithPeer function

Now that we have a simple boolean stored on the field, the wrapper function is
no longer necessary.

c061599e40

[test] Test that we intentionally select addr relay peers.

This test checks that we only relay addresses with inbound peers who have sent
us an addr related message. Uses a combination of GETADDR and ADDR to verify
when peers are eligible.

0980ca78cd

[RPC] Add field to getpeerinfo to indicate if addr relay is enabled 3893da06db

[test] Use the new endpoint to improve tests 3f7250b328

amitiuttarwar force-pushed on Jul 30, 2021

amitiuttarwar commented at 0:50 am on July 30, 2021: contributor

thanks @MarcoFalke, addressed your comments

jnewbery commented at 9:17 am on July 30, 2021: member

reACK 3f7250b328b8b2f5d63f323702445ac5c989b73d

Only changes are addressing review comments from Marco.

glozow commented at 9:37 am on July 30, 2021: member

ACK 3f7250b328b8b2f5d63f323702445ac5c989b73d

Compared the SetupAddressRelay() call sites to where I thought they should be, threw in some asserts where I expected m_addr_relay_enabled to be true, did a code review looking for possibilities of m_addr_known being used before initialized. Test looks correct. Thanks for diligently checking for compatibility with other clients, seems to be a safe change. :)

fanquake requested review from ajtowns on Aug 2, 2021

in src/net_processing.cpp:3746 in 3f7250b328

3742@@ -3717,6 +3743,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
3743             return;
3744         }
3745 
3746+        SetupAddressRelay(pfrom, *peer);

ajtowns commented at 1:18 pm on August 2, 2021:

if (!pfrom.IsInboundConn() || !SetupAddressRelay(pfrom, *peer)) { error; return; } might have been better so you don’t have to keep the knowledge that SetupAddressRelay always returns true for inbound conns in your head.

amitiuttarwar commented at 2:25 pm on August 2, 2021:

makes sense, I’ll change if I have to push again

MarcoFalke commented at 8:39 am on August 3, 2021:

Assume(Setup()) would do that?

amitiuttarwar commented at 4:51 pm on August 3, 2021:

addressed in #22618

ajtowns approved

ajtowns commented at 1:39 pm on August 2, 2021: member

utACK 3f7250b328b8b2f5d63f323702445ac5c989b73d

I don’t think the potential interaction with #22387 should be an issue – if the rate limit wasn’t being hit before it was because addresses weren’t being relayed, and if they’re hit now the result is just that addresses aren’t being relayed.

in test/functional/p2p_addr_relay.py:182 in 3f7250b328

178@@ -167,6 +179,9 @@ def relay_tests(self):
179         # of the outbound peer which is often sent before the GETADDR response.
180         assert_equal(inbound_peer.num_ipv4_received, 0)
181 
182+        # Send an empty ADDR message to intialize address relay on this connection.

fanquake commented at 1:47 am on August 3, 2021:

s/intialize/initialize

fanquake merged this on Aug 3, 2021

fanquake closed this on Aug 3, 2021

amitiuttarwar deleted the branch on Aug 3, 2021

MarcoFalke commented at 8:44 am on August 3, 2021: member

Maybe add a line to the release notes saying that inbound peers will never receive address messages unless they send ADDR, ADDRV2, or GETADDR themselves?

in test/functional/p2p_addr_relay.py:15 in 3f7250b328

10@@ -11,14 +11,15 @@
11     NODE_NETWORK,
12     NODE_WITNESS,
13     msg_addr,
14-    msg_getaddr
15+    msg_getaddr,
16+    msg_verack

MarcoFalke commented at 8:58 am on August 3, 2021:

nit: Would be nice to always add a trailing comma to avoid ugly diffs and keeping the git blame depth lower in the future.

amitiuttarwar commented at 3:19 pm on August 3, 2021:

noted

amitiuttarwar referenced this in commit 5da1ca2b52 on Aug 3, 2021

amitiuttarwar commented at 4:51 pm on August 3, 2021: contributor

@MarcoFalke

Maybe add a line to the release notes

Added in #22618, thanks :)

amitiuttarwar referenced this in commit dddd05362e on Aug 3, 2021

sidhujag referenced this in commit 9045bd0a34 on Aug 4, 2021

amitiuttarwar referenced this in commit aa79c91260 on Aug 4, 2021

MarcoFalke referenced this in commit dd981b5e84 on Aug 5, 2021

Fabcien referenced this in commit d3a5c3e432 on Jan 28, 2022

DrahtBot locked this on Aug 18, 2022

[p2p] Reduce addr blackholes #21528

Conflicts

Latest push:

Status of this PR:

Future thought: