Ignore getaddr messages on Outbound connections. #5442

pull ivanpustogarov wants to merge 1 commits into bitcoin:master from ivanpustogarov:noreply_getaddr_on_outbound changing 1 files +6 −1

ivanpustogarov commented at 10:40 PM on December 7, 2014: contributor

The only time when a client sends a "getaddr" message is when he esatblishes an Outbound connection (see ProcessMessage() in src/main.cpp). Another bitcoin client is expected to receive a "getaddr" message only on Inbound connection. Ignoring "gettaddr" requests on Outbound connections can resolve potential privacy issues (and as was said such request normally do not happen anyway).
gmaxwell commented at 6:50 AM on December 8, 2014: contributor

Hm. Breaking the symmetry here is odd but may be prudent, resulting in one less (of many) fingerprinting vectors. I'll need to consider some if this is tying our hands or would create other problems.
laanwj added the label P2P on Dec 8, 2014
laanwj commented at 9:05 AM on December 8, 2014: member

This change makes sense from a minimalism point of view. If we don't send "getaddr" to incoming connections, and have no reason to consider doing that, it doesn't have to be handled.
sipa commented at 10:16 PM on December 8, 2014: member

As we only use explicit 'getaddr' message to get more addresses in case of a nearly-empty addrman, this shouldn't have much impact. ACK
jgarzik commented at 10:51 PM on December 8, 2014: contributor

A nearly-empty addrman is what you have on testnet and other less populated networks. addrman still has some pathological behavior in that area, such as trying the same few addresses over and over again. It would be a shame to increase the pathlogical behavior.

Not completely convinced increasing the level of assemetry here has value from a Sybil perspective, but it might.
gmaxwell commented at 11:03 PM on December 8, 2014: contributor

@jgarzik yea, but we don't do it towards inbound peers and probably don't want to due to self-selecting sybil risk.
sipa commented at 4:16 PM on December 10, 2014: member

@jgarzik Between Bitcoin Core clients, this change should have 0 impact.
jgarzik commented at 4:18 PM on December 10, 2014: contributor

Today, yes.

Do we want to cut off this avenue for obtaining addresses in the future?
gmaxwell commented at 7:09 PM on December 10, 2014: contributor

On reflection, I think it's not that concerning for the future: You're free to try, the other side might not respond.

Plus, we're probably due to create a new address message type (e.g. supports more service information, supports sending pubkeys, supports longer host identifiers (i2p needs 512 bits, tor's upgraded hs will need 256 bits I think)). Another address type could have different rules.

So I think if we regret this change we wouldn't regret it enormously.
laanwj commented at 10:59 AM on January 8, 2015: member

Let's move forward with this or close it, @gmaxwell @jgarzik ACK or NACK?
gmaxwell commented at 3:29 PM on January 8, 2015: contributor

ACK.
jgarzik commented at 3:31 PM on January 8, 2015: contributor

reluctant ut ACK

in src/main.cpp:None in 19d6093bab outdated

3922 | @@ -3923,7 +3923,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
3923 |      }
3924 |  
3925 |  
3926 | -    else if (strCommand == "getaddr")
3927 | +    else if ((strCommand == "getaddr") && (pfrom->fInbound))

laanwj commented at 3:44 PM on January 8, 2015:

Thinking of it, this really needs a comment in the source code why this asymmetric behavior was introduced. Someone reading the code is bound to wonder why.

ivanpustogarov force-pushed on Feb 6, 2015

Ignore getaddr messages on Outbound connections.

The only time when a client sends a "getaddr" message is when he
esatblishes an Outbound connection (see ProcessMessage() in
src/main.cpp).  Another bitcoin client is expected to receive a
"getaddr" message only on Inbound connection. Ignoring "gettaddr"
requests on Outbound connections can resolve potential privacy issues
(and as was said such request normally do not happen anyway).

dca799e1db

ivanpustogarov force-pushed on Feb 6, 2015
ivanpustogarov commented at 9:09 PM on February 6, 2015: contributor

Added a comment in the source code why this asymmetric behavior was introduced.
sipa commented at 11:54 AM on March 1, 2015: member

re-ACK
laanwj merged this on Mar 9, 2015
laanwj closed this on Mar 9, 2015
laanwj referenced this in commit c1b723c30a on Mar 9, 2015
laanwj commented at 11:31 AM on March 9, 2015: member

Backported to 0.10 as 200f293
laanwj referenced this in commit 200f29363b on Mar 9, 2015
Astrych referenced this in commit e4d56683a8 on Jan 16, 2019
reddink referenced this in commit 8b05a144fc on May 27, 2020
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#6344 Avoid "Unknown command" messages when receiving getaddr on outbound c…