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

Do not set an addr time penalty when a peer advertises itself. #8661

Merged
merged 1 commit into from Sep 23, 2016

Conversation

@gmaxwell
Copy link
Contributor

commented Sep 3, 2016

Claims a peer makes about itself are inherently more credible.

Do not set an addr time penalty when a peer advertises itself.
Claims a peer makes about itself are inherently more credible.
@gmaxwell

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2016

@sipa @EthanHeilman Is this sensible in your view?

@jonasschnelli jonasschnelli added the P2P label Sep 5, 2016

@sipa

This comment has been minimized.

Copy link
Member

commented Sep 5, 2016

utACK

@EthanHeilman

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2016

@gmaxwell utACK. Feeler connections don't currently send addr messages so they would not benefit from this change. I think that is ok for now.

I plan on investigating the benefits of having feeler connections either send an addr message containing only themselves (if they accept incoming connections) or having feeler connections send a full addr message but that goes beyond the scope of this PR and should be discussed later.

@gmaxwell

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2016

I think there would be a good argument for feeler connections going as far as trying to determine if the peers have distinct chaintips-- and attempting to sync them if they do-- before moving along. But right, orthogonal to this. Thanks for taking a look.

@rebroad

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2016

@gmaxwell Can you summarise what the impact of this is please? (as if talking to someone who is unfamiliar with addrman)

@gmaxwell

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2016

@rebroad The node keeps track of when it last heard from a peer and uses this information when trying to figure out who to connect to (esp when connected to no one). Advertisements rumored through third parties have that time artificially increased, because we're getting the information second hand and it might not be correct. But when a peer advertises themselves this information is first hand.

@btcdrak

This comment has been minimized.

Copy link
Member

commented Sep 22, 2016

utACK 6d0ced1

@laanwj

This comment has been minimized.

Copy link
Member

commented Sep 23, 2016

Yes, this makes sense.
utACK 6d0ced1

@laanwj laanwj merged commit 6d0ced1 into bitcoin:master Sep 23, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Sep 23, 2016
Merge #8661: Do not set an addr time penalty when a peer advertises i…
…tself.

6d0ced1 Do not set an addr time penalty when a peer advertises itself. (Gregory Maxwell)
@rebroad

This comment has been minimized.

Copy link
Contributor

commented Sep 23, 2016

@gmaxwell thanks for the explanation. I notice that the addresses advertised via 3rd parties go into the same database as the "last heard from a peer database" with fake times ranging from 3 to 7 days... why the need to fake anything - why not simply keep a database of nodes actually heard from, and a database of rumored nodes?

@EthanHeilman

This comment has been minimized.

Copy link
Contributor

commented Sep 23, 2016

@rebroad The new table is a database of rumored nodes, the tried table is a database of nodes that we have heard from or nodes that we have made an outgoing connection to.

The PR #8594 changes the tried table to just be a database of nodes which we have connected to.

One idea could be to have three tables: new, tried, heard. I think this is a good idea but it requires careful design to insure it could not be manipulated by an attacker attempting to partition or eclipse a node.

@sipa sipa referenced this pull request Jan 10, 2017
16 of 18 tasks complete
codablock added a commit to codablock/dash that referenced this pull request Jan 11, 2018
Merge bitcoin#8661: Do not set an addr time penalty when a peer adver…
…tises itself.

6d0ced1 Do not set an addr time penalty when a peer advertises itself. (Gregory Maxwell)
andvgal added a commit to energicryptocurrency/energi that referenced this pull request Jan 6, 2019
Merge bitcoin#8661: Do not set an addr time penalty when a peer adver…
…tises itself.

6d0ced1 Do not set an addr time penalty when a peer advertises itself. (Gregory Maxwell)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.