p2p: Avoid relaying ADDR messages to SPV clients

naumenkogs commented at 5:24 pm on October 16, 2019: member

Note that this PR used this code 989e85a, but then I force-pushed the branch so you can’t see it here.

We currently announce our own network address to all our peers (except block-relay-only peers) on the handshake. We also relay further other nodes’ addresses as long as the batch is less than 10 elements. We should not relay it to SPV, because they never store or relay it further.

it actually makes addr relay slower, because for the second case we pick 1 or 2 peers at random, and it’s possible that we’ll forward a message to an SPV.
it’s a waste of resources (bandwidth, also RAM after 17164).

fanquake added the label P2P on Oct 16, 2019

naumenkogs renamed this:
~~p2p: Address relay optimization~~
p2p: Address relay optimization (not relay to SPV and not allocate BloomFilter when unneeded)
on Oct 16, 2019

naumenkogs renamed this:
~~p2p: Address relay optimization (not relay to SPV and not allocate BloomFilter when unneeded)~~
p2p: Avoid relaying ADDR messages to SPV clients
on Oct 16, 2019

naumenkogs force-pushed on Oct 16, 2019

in src/net.cpp:2659 in c06b552610 outdated

2654@@ -2655,8 +2655,8 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn
2655     addrKnown(5000, 0.001),
2656     // Don't relay addr messages to peers that we connect to as block-relay-only
2657     // peers (to prevent adversaries from inferring these links from addr
2658-    // traffic).
2659-    m_addr_relay_peer(!block_relay_only),
2660+    // traffic) or SPV clients (because they don't store or relay addresses).
2661+    m_addr_relay_peer(!block_relay_only && MayHaveUsefulAddressDB(nServices)),

dongcarl commented at 7:23 pm on October 16, 2019:

I believe that since this is the constructor, nServices is always NODE_NONE here

naumenkogs commented at 9:12 pm on October 16, 2019:

Yeah, will fix soon.

dongcarl dismissed

MarcoFalke commented at 7:50 pm on October 16, 2019: member

Concept ACK. SPV nodes use dns seeders to get addresses, so they can filter for service bits.

Side-note: The memory savings aren’t substantial (https://github.com/bitcoin/bitcoin/pull/17164#issuecomment-542862195)

naumenkogs force-pushed on Oct 16, 2019

laanwj commented at 8:43 am on October 17, 2019: member

Concept ACK. I think ideally there should have been separate node / capability flags for this instead of piggybacking on NODE_NETWORK / NODE_NETWORK_LIMITED. But that bridge was already crossed with MayHaveUsefulAddressDB.

in src/net.h:738 in 685819544d outdated

734@@ -734,8 +735,8 @@ class CNode
735     int64_t nNextAddrSend GUARDED_BY(cs_sendProcessing){0};
736     int64_t nNextLocalAddrSend GUARDED_BY(cs_sendProcessing){0};
737 
738-    const bool m_addr_relay_peer;
739-    bool IsAddrRelayPeer() const { return m_addr_relay_peer; }
740+    bool IsAddrRelayPeer() { return m_addr_relay_peer; }

MarcoFalke commented at 12:37 pm on October 17, 2019:

0    bool IsAddrRelayPeer() const { return m_addr_relay_peer; }

This can be const, just as it was before.

MarcoFalke added the label Resource usage on Oct 17, 2019

naumenkogs force-pushed on Oct 17, 2019

sipa commented at 5:47 pm on October 17, 2019: member

I’m unconvinced that we shouldn’t relay IP addresses to lightweight nodes entirely; there is no reason why they can’t do their own IP address management, or shouldn’t hear about this information from the network. If it’s the case that currently none of them care because they 100% rely on DNS seeds, that sounds like a flaw of theirs, and not something to encourage further by making it harder to use real IP management.

On the other hand, I do see that we may assume that they won’t relay the IP addresses further, and we should take that into account when determining where to gossip incoming addr data to.

I don’t have a good solution, though.

naumenkogs commented at 5:54 pm on October 17, 2019: member

Now after Pieter’s comment I’m leaning towards allowing them to learn, but not choosing them to forward addr messages.

I’ll wait a bit for other opinions, and if everyone agrees there — I’ll re-implement it according to what I said above. Should be an easy clean change.

sipa commented at 5:54 pm on October 17, 2019: member

Unrelatedly, should we discard/discount addr messages coming in on incoming connections?

MarcoFalke commented at 5:54 pm on October 17, 2019: member

I think address management is resource expensive when done correctly. At the very least your SPV wallet would need to connect to the node and check the version message for service bits. Then maybe it should also ask for a block or txs to identify obvious spy nodes. This seems to not fit in well where SPV nodes are deployed (mobile wallets).

sipa commented at 6:02 pm on October 17, 2019: member

@MarcoFalke If wallets rely solely on DNS seeds, that’s a scary thing. If we’re going to go as far as codifying that as the only option, and giving up the hope that they’ll ever do something sane, I will probably stop running a DNS seed.

luke-jr commented at 6:05 pm on October 17, 2019: member

My gut response is to agree with @sipa… but light wallets are also trust-based, so they should only be used in tandem with your own node anyway. I’m not sure doing their own address management makes sense, in that context.

In other words, the “sane” thing isn’t doing their own address management nor relying solely on DNS seeds - it’s peering with the user’s own full node. And not sharing addresses with them is perfectly compatible with that.

naumenkogs commented at 6:20 pm on October 17, 2019: member

@sipa: Unrelatedly, should we discard/discount addr messages coming in on incoming connections?

If we do what you’re suggesting, then

Older nodes are very likely to forward their <10 addr messages in the blackhole (ok, that’s fixable with work-arounds)
I believe forwarding <10 addr messages would substantially degrade. Like, it’s very likely that this “stem” will end up at a private node very fast (just graph-wise), and it will drop it on the floor. Can measure.

DrahtBot commented at 6:53 pm on October 17, 2019: 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:

#17164 (p2p: Avoid allocating memory for addrKnown where we don’t need it by naumenkogs)

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.

naumenkogs closed this on Oct 18, 2019

naumenkogs force-pushed on Oct 18, 2019

naumenkogs commented at 8:23 pm on October 18, 2019: member

Closed in favor of 17194

I should have kept this code to let reviewers see the difference, but it’s too late I guess.

This PR suggested also not responding to SPV’s GETADDR. 17194 allows SPVs to request addresses, but avoids unsolicited forwarding addresses to them.

MarcoFalke commented at 8:32 pm on October 18, 2019: member

The previous branch was 989e85aecfc59ef0778d188cbcfed7c4f1878485

DrahtBot locked this on Dec 16, 2021

p2p: Avoid relaying ADDR messages to SPV clients #17163

Conflicts