[p2p] Reduce addr blackholes #21528

amitiuttarwar · 2021-03-25T04:18:07Z

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 [p2p] Reduce addr blackholes #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

src/net_processing.cpp

DrahtBot · 2021-03-25T08:54:46Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Erlay: bandwidth-efficient transaction relay protocol #21515 by naumenkogs
assumeutxo #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.

jnewbery

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 😁

src/net.cpp

src/net_processing.cpp

jnewbery · 2021-03-25T10:36:09Z

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 · 2021-03-30T04:53:07Z

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:

breadwallet	✅	Sends `getaddr` & only accepts `addr` messages afterwards (link)
btcd	✅ 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.
libbitcoin	✅ Confirmed by @evoskuil (issue)	When connecting to an outbound peer, sends a `getaddr` message.
bitcoin knots	✅	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)
bcoin	✅ Confirmed by @pinheadmz (issue).	Upon opening a connection, node will self-advertise & usually send a `getaddr` message (link).
gocoin	✅ Confirmed by @piotrnar (issue)	Upon receipt of a `version` message, node will self-advertise. Node will usually also send a `getaddr` message to peers.
bitcore	✅ Confirmed by @micahriggan (issue)	Bitcore nodes typically have dedicated bitcoin node(s) and the addresses are passed in via config.
bitcoinj	✅ (issue)	As part of connecting to a peer, the node will send a `getaddr` message on the link.

src/net_processing.cpp

test/functional/p2p_addr_relay.py

glozow

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?

src/net.h

test/functional/p2p_addr_relay.py

glozow · 2021-03-30T22:41:05Z

src/net_processing.cpp

@@ -3569,6 +3584,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
        }
        pfrom.fSentAddr = true;

+        SetupAddressRelay(pfrom);
+
        pfrom.vAddrToSend.clear();


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

sipa · 2021-03-31T05:01:50Z

@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 · 2021-03-31T16:06:07Z

Concept ACK

src/net.h

GeneFerneau · 2021-03-31T16:49:07Z

Concept ACK

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

a732ee3 [test] Add tests for addr relay in -blocksonly mode (Amiti Uttarwar) a6694ea [test] Add address relay tests involving outbound peers (Martin Zumsande) 8188b77 [test] Add tests for getaddr behavior (Martin Zumsande) d2dbfe6 [test] Extract sending an addr message into a helper (Amiti Uttarwar) c991943 [test] Refactor the addr relay test to prepare for new tests (Amiti Uttarwar) Pull request description: This extends the functional test `p2p_addr_relay.py`. It adds test coverage for address relay involving outbound peers, tests for both outgoing and incoming `GETADDR` requests and tests for `-blocksonly` mode. The initial refactors and some of the new tests were taken from Amiti Uttarwar's PR bitcoin#21528 - they are general test improvements not directly tied to the change proposed there. ACKs for top commit: amitiuttarwar: re-ACK a732ee3, small diff based on code review MarcoFalke: Concept ACK a732ee3 🌊 Tree-SHA512: e80d52683808ddd6b948a5134239f002f3fecf61b60e187877b07be6251721fde847104e495c75a1a5133a09c0b41a9255a0bec82932c0b304b516fa89bce33e

jnewbery · 2021-06-09T10:49:28Z

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 · 2021-06-09T11:39:24Z

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 bitpay/bitcore#3140) which now looks like it's now resolved.

amitiuttarwar · 2021-06-12T00:58:26Z

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

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.

…bled

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

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.

amitiuttarwar · 2021-07-30T00:50:15Z

thanks @MarcoFalke, addressed your comments

jnewbery · 2021-07-30T09:17:56Z

reACK 3f7250b

Only changes are addressing review comments from Marco.

glozow · 2021-07-30T09:37:00Z

ACK 3f7250b

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

ajtowns

utACK 3f7250b

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.

src/net_processing.cpp

fanquake · 2021-08-03T01:47:41Z

test/functional/p2p_addr_relay.py

@@ -167,6 +179,9 @@ def relay_tests(self):
        # of the outbound peer which is often sent before the GETADDR response.
        assert_equal(inbound_peer.num_ipv4_received, 0)

+        # Send an empty ADDR message to intialize address relay on this connection.


s/intialize/initialize

maflcko · 2021-08-03T08:44:58Z

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?

maflcko · 2021-08-03T08:58:08Z

test/functional/p2p_addr_relay.py

@@ -11,14 +11,15 @@
    NODE_NETWORK,
    NODE_WITNESS,
    msg_addr,
-    msg_getaddr
+    msg_getaddr,
+    msg_verack


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 · 2021-08-03T16:51:52Z

@MarcoFalke

Maybe add a line to the release notes

Added in #22618, thanks :)

And fix a typo in the test.

9778b0f [net_processing] Provide debug error if code assumptions change. (Amiti Uttarwar) aa79c91 [docs] Add release notes for #21528 (Amiti Uttarwar) Pull request description: Adds a release note & addresses [this](bitcoin/bitcoin#21528 (comment)) review comment to make expectations more explicit. ACKs for top commit: Zero-1729: re-ACK 9778b0f jonatack: ACK 9778b0f Tree-SHA512: 9507df5f2746d05c6df8c86b7a19364610ebfafc81af7650be7e68d7536a0685cce9fd2e5f287ef92b6245c584f8875b24a958109ba5bd8acf3c8fc9fd19eef2

Summary: ``` 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. ``` Backport of [[bitcoin/bitcoin#21528 | core#21528]]. Ref T1696. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, tyler-smith Reviewed By: #bitcoin_abc, tyler-smith Subscribers: tyler-smith Maniphest Tasks: T1696 Differential Revision: https://reviews.bitcoinabc.org/D10933

fanquake added the P2P label Mar 25, 2021

ajtowns reviewed Mar 25, 2021

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

jnewbery reviewed Mar 25, 2021

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Mar 25, 2021

net processing: Extract addr send functionality into MaybeSendAddr() #21236

Merged

ajtowns mentioned this pull request Mar 25, 2021

net_processing: lock clean up #21527

Closed

amitiuttarwar force-pushed the 2021-03-addr-defer2 branch 2 times, most recently from bcb89d9 to 8e2bcc0 Compare March 30, 2021 00:40

amitiuttarwar changed the title ~~[net_processing] Reduce addr blackholes~~ [p2p] Reduce addr blackholes Mar 30, 2021

ccdle12 reviewed Mar 30, 2021

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

ccdle12 reviewed Mar 30, 2021

View reviewed changes

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

amitiuttarwar force-pushed the 2021-03-addr-defer2 branch from 8e2bcc0 to 0c194ff Compare March 30, 2021 21:03

glozow reviewed Mar 30, 2021

View reviewed changes

GeneFerneau reviewed Mar 31, 2021

View reviewed changes

src/net.h Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Apr 1, 2021

net: Restrict period when cs_vNodes mutex is locked #21563

Merged

mzumsande mentioned this pull request Apr 16, 2021

test: Extend functional tests for addr relay #21707

Merged

amitiuttarwar mentioned this pull request Jun 14, 2021

[p2p] Stop sending SENDADDRV2 message to block-relay-only peers #22245

Closed

amitiuttarwar added 6 commits July 29, 2021 17:40

[net_processing] Introduce new field to indicate if addr relay is ena…

201e496

…bled

[net_processing] Remove RelayAddrsWithPeer function

c061599

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

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

0980ca7

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.

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

3893da0

[test] Use the new endpoint to improve tests

3f7250b

amitiuttarwar force-pushed the 2021-03-addr-defer2 branch from 00a54cb to 3f7250b Compare July 30, 2021 00:48

fanquake requested a review from ajtowns August 2, 2021 02:21

ajtowns approved these changes Aug 2, 2021

View reviewed changes

src/net_processing.cpp Show resolved Hide resolved

fanquake reviewed Aug 3, 2021

View reviewed changes

fanquake merged commit 06788c6 into bitcoin:master Aug 3, 2021

amitiuttarwar deleted the 2021-03-addr-defer2 branch August 3, 2021 01:50

maflcko reviewed Aug 3, 2021

View reviewed changes

amitiuttarwar added a commit to amitiuttarwar/bitcoin that referenced this pull request Aug 3, 2021

[docs] Add release notes for bitcoin#21528

5da1ca2

amitiuttarwar mentioned this pull request Aug 3, 2021

[p2p] Small follow-ups to 21528 #22618

Merged

amitiuttarwar added a commit to amitiuttarwar/bitcoin that referenced this pull request Aug 3, 2021

[docs] Add release notes for bitcoin#21528

dddd053

And fix a typo in the test.

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 4, 2021

Merge bitcoin#21528: [p2p] Reduce addr blackholes

9045bd0

amitiuttarwar added a commit to amitiuttarwar/bitcoin that referenced this pull request Aug 4, 2021

[docs] Add release notes for bitcoin#21528

aa79c91

And fix a typo in the test.

jnewbery mentioned this pull request Aug 10, 2021

Newsletters: add 161 (2021-08-11) bitcoinops/bitcoinops.github.io#625

Merged

9 tasks

jonatack mentioned this pull request Aug 27, 2021

test: Test failed when running p2p_addr_relay.py #22819

Closed

naumenkogs mentioned this pull request Sep 15, 2021

p2p: Avoid forwarding ADDR messages to SPV nodes #17194

Closed

sdaftuar mentioned this pull request Jan 5, 2022

p2p: Add DISABLETX message for negotiating block-relay-only connections #20726

Closed

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[p2p] Reduce addr blackholes #21528

[p2p] Reduce addr blackholes #21528

amitiuttarwar commented Mar 25, 2021 •

edited

DrahtBot commented Mar 25, 2021 •

edited

jnewbery left a comment

jnewbery commented Mar 25, 2021

amitiuttarwar commented Mar 30, 2021 •

edited

glozow left a comment

glozow Mar 30, 2021

sipa commented Mar 31, 2021

ivanacostarubio commented Mar 31, 2021

GeneFerneau commented Mar 31, 2021

jnewbery commented Jun 9, 2021

ajtowns commented Jun 9, 2021

amitiuttarwar commented Jun 12, 2021

amitiuttarwar commented Jul 30, 2021

jnewbery commented Jul 30, 2021

glozow commented Jul 30, 2021

ajtowns left a comment

fanquake Aug 3, 2021

maflcko commented Aug 3, 2021

maflcko Aug 3, 2021

amitiuttarwar Aug 3, 2021

amitiuttarwar commented Aug 3, 2021 •

edited

[p2p] Reduce addr blackholes #21528

[p2p] Reduce addr blackholes #21528

Conversation

amitiuttarwar commented Mar 25, 2021 • edited

DrahtBot commented Mar 25, 2021 • edited

Conflicts

jnewbery left a comment

Choose a reason for hiding this comment

jnewbery commented Mar 25, 2021

amitiuttarwar commented Mar 30, 2021 • edited

glozow left a comment

Choose a reason for hiding this comment

glozow Mar 30, 2021

Choose a reason for hiding this comment

sipa commented Mar 31, 2021

ivanacostarubio commented Mar 31, 2021

GeneFerneau commented Mar 31, 2021

jnewbery commented Jun 9, 2021

ajtowns commented Jun 9, 2021

amitiuttarwar commented Jun 12, 2021

amitiuttarwar commented Jul 30, 2021

jnewbery commented Jul 30, 2021

glozow commented Jul 30, 2021

ajtowns left a comment

Choose a reason for hiding this comment

fanquake Aug 3, 2021

Choose a reason for hiding this comment

maflcko commented Aug 3, 2021

maflcko Aug 3, 2021

Choose a reason for hiding this comment

amitiuttarwar Aug 3, 2021

Choose a reason for hiding this comment

amitiuttarwar commented Aug 3, 2021 • edited

amitiuttarwar commented Mar 25, 2021 •

edited

DrahtBot commented Mar 25, 2021 •

edited

amitiuttarwar commented Mar 30, 2021 •

edited

amitiuttarwar commented Aug 3, 2021 •

edited