Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore getaddr messages on Outbound connections. #5442

Merged
merged 1 commit into from Mar 9, 2015

Conversation

@ivanpustogarov
Copy link
Contributor

ivanpustogarov commented Dec 7, 2014

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

This comment has been minimized.

Copy link
Contributor

gmaxwell commented Dec 8, 2014

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 laanwj added the P2P label Dec 8, 2014
@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Dec 8, 2014

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

This comment has been minimized.

Copy link
Member

sipa commented Dec 8, 2014

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

This comment has been minimized.

Copy link
Contributor

jgarzik commented Dec 8, 2014

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

This comment has been minimized.

Copy link
Contributor

gmaxwell commented Dec 8, 2014

@jgarzik yea, but we don't do it towards inbound peers and probably don't want to due to self-selecting sybil risk.

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Dec 10, 2014

@jgarzik Between Bitcoin Core clients, this change should have 0 impact.

@jgarzik

This comment has been minimized.

Copy link
Contributor

jgarzik commented Dec 10, 2014

Today, yes.

Do we want to cut off this avenue for obtaining addresses in the future?

@gmaxwell

This comment has been minimized.

Copy link
Contributor

gmaxwell commented Dec 10, 2014

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

This comment has been minimized.

Copy link
Member

laanwj commented Jan 8, 2015

Let's move forward with this or close it, @gmaxwell @jgarzik ACK or NACK?

@gmaxwell

This comment has been minimized.

Copy link
Contributor

gmaxwell commented Jan 8, 2015

ACK.

@jgarzik

This comment has been minimized.

Copy link
Contributor

jgarzik commented Jan 8, 2015

reluctant ut ACK

@laanwj
laanwj reviewed Jan 8, 2015
View changes
src/main.cpp Outdated
@@ -3923,7 +3923,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
}


else if (strCommand == "getaddr")
else if ((strCommand == "getaddr") && (pfrom->fInbound))

This comment has been minimized.

Copy link
@laanwj

laanwj Jan 8, 2015

Member

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 ivanpustogarov force-pushed the ivanpustogarov:noreply_getaddr_on_outbound branch Feb 6, 2015
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).
@ivanpustogarov ivanpustogarov force-pushed the ivanpustogarov:noreply_getaddr_on_outbound branch to dca799e Feb 6, 2015
@ivanpustogarov

This comment has been minimized.

Copy link
Contributor Author

ivanpustogarov commented Feb 6, 2015

Added a comment in the source code why this asymmetric behavior was introduced.

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Mar 1, 2015

re-ACK

@laanwj laanwj merged commit dca799e into bitcoin:master Mar 9, 2015
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Mar 9, 2015
dca799e Ignore getaddr messages on Outbound connections. (Ivan Pustogarov)
@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Mar 9, 2015

Backported to 0.10 as 200f293

laanwj added a commit that referenced this pull request Mar 9, 2015
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).

Rebased-From: dca799e
Github-Pull: #5442
Astrych added a commit to Astrych/Pink2 that referenced this pull request Jan 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.