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

Addrman offline attempts #8065

Merged
merged 2 commits into from Jun 8, 2016

Conversation

Projects
None yet
6 participants
@gmaxwell
Member

gmaxwell commented May 17, 2016

If a node is offline failed outbound connection attempts will crank up
the addrman counter and effectively blow away our state.

This change reduces the problem by only counting attempts made while
the node believes it has outbound connections to at least two
netgroups.

Connect and addnode connections are also not counted, as there is no
reason to unequally penalize them for their more frequent
connections -- though there should be no real effect from this
unless their addnode configureation is later removed.

Wasteful repeated connection attempts while only a few connections are
up are avoided via nLastTry.

This is still somewhat incomplete protection because our outbound
peers could be down but not timed out or might all be on 'local'
networks (although the requirement for multiple netgroups helps).

( reopen of #6030 )

@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem May 17, 2016

Contributor

This should probably be checking for connections to at least two netgroups outside of RFC1918 IP space, but this is certainly an improvement.

Contributor

pstratem commented May 17, 2016

This should probably be checking for connections to at least two netgroups outside of RFC1918 IP space, but this is certainly an improvement.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell May 18, 2016

Member

@pstratem checking connections is better than no check at all. This PR is from April 2015, so I'm not inclined to expand the scope any further right now. If this is accepted I'll happily open a follow on PR to make it stronger.

Member

gmaxwell commented May 18, 2016

@pstratem checking connections is better than no check at all. This PR is from April 2015, so I'm not inclined to expand the scope any further right now. If this is accepted I'll happily open a follow on PR to make it stronger.

@arowser

This comment has been minimized.

Show comment
Hide comment
@arowser

arowser May 25, 2016

Contributor

Can one of the admins verify this patch?

Contributor

arowser commented May 25, 2016

Can one of the admins verify this patch?

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa May 25, 2016

Member

Needs rebase.

Member

sipa commented May 25, 2016

Needs rebase.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa May 26, 2016

Member

Did you accidentally include #7992?

Member

sipa commented May 26, 2016

Did you accidentally include #7992?

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa May 26, 2016

Member

utACK 66cefd47194a8aa7478dbea9a1b96640febeeda7

Member

sipa commented May 26, 2016

utACK 66cefd47194a8aa7478dbea9a1b96640febeeda7

gmaxwell added some commits Apr 19, 2015

Avoid counting failed connect attempts when probably offline.
If a node is offline failed outbound connection attempts will crank up
 the addrman counter and effectively blow away our state.

This change reduces the problem by only counting attempts made while
 the node believes it has outbound connections to at least two
 netgroups.

Connect and addnode connections are also not counted, as there is no
 reason to unequally penalize them for their more frequent
 connections -- though there should be no real effect from this
 unless their addnode configureation is later removed.

Wasteful repeated connection attempts while only a few connections are
 up are avoided via nLastTry.

This is still somewhat incomplete protection because our outbound
 peers could be down but not timed out or might all be on 'local'
 networks (although the requirement for multiple netgroups helps).
Do not increment nAttempts by more than one for every Good connection.
This slows the increase of the nAttempts in addrman while partitioned,
 even if the node hasn't yet noticed the partitioning.
@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa May 26, 2016

Member

Running a public node with #7960 #8007 #8065 #8080 #8082 #8084 #8086 for testing.

Member

sipa commented May 26, 2016

Running a public node with #7960 #8007 #8065 #8080 #8082 #8084 #8086 for testing.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 7, 2016

Member

This should be easy to test manually: add a debug log entry above the info.nAttempts increment in CAddrMan::Attempt_(), and see how often it triggers with network connected and disconnected.

Member

sipa commented Jun 7, 2016

This should be easy to test manually: add a debug log entry above the info.nAttempts increment in CAddrMan::Attempt_(), and see how often it triggers with network connected and disconnected.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 8, 2016

Member

utACK 6182d10

Member

laanwj commented Jun 8, 2016

utACK 6182d10

@laanwj laanwj merged commit 6182d10 into bitcoin:master Jun 8, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jun 8, 2016

Merge #8065: Addrman offline attempts
6182d10 Do not increment nAttempts by more than one for every Good connection. (Gregory Maxwell)
c769c4a Avoid counting failed connect attempts when probably offline. (Gregory Maxwell)

sickpig referenced this pull request in sickpig/BitcoinUnlimited Apr 14, 2017

Merge #8065: Addrman offline attempts
6182d10 Do not increment nAttempts by more than one for every Good connection. (Gregory Maxwell)
c769c4a Avoid counting failed connect attempts when probably offline. (Gregory Maxwell)

sickpig referenced this pull request in sickpig/BitcoinUnlimited Apr 14, 2017

Adapt OpenNetworkConnection to the new semantic
Since we merged core PR #8065 a new bool parameter has been added
to such function. if false we avoid the book keeping of connections
failed attempt. This will happen in the followin cases:

- node is offline
- node connect to less than 2 netgroups via outound conns
- connect(s) and addnode(s)

sickpig referenced this pull request in sickpig/BitcoinUnlimited Apr 14, 2017

Merge #8065: Addrman offline attempts
6182d10 Do not increment nAttempts by more than one for every Good connection. (Gregory Maxwell)
c769c4a Avoid counting failed connect attempts when probably offline. (Gregory Maxwell)

sickpig referenced this pull request in sickpig/BitcoinUnlimited Apr 14, 2017

Adapt OpenNetworkConnection to the new semantic
Since we merged core PR #8065 a new bool parameter has been added
to such function. if false we avoid the book keeping of connections
failed attempt. This will happen in the following cases:

- node is offline
- node connect to less than 2 netgroups via outbound conns
- connect(s) and addnode(s)

@dgenr8 dgenr8 referenced this pull request May 18, 2017

Merged

Networking improvements #203

codablock added a commit to codablock/dash that referenced this pull request Sep 16, 2017

Merge #8065: Addrman offline attempts
6182d10 Do not increment nAttempts by more than one for every Good connection. (Gregory Maxwell)
c769c4a Avoid counting failed connect attempts when probably offline. (Gregory Maxwell)

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge #8065: Addrman offline attempts
6182d10 Do not increment nAttempts by more than one for every Good connection. (Gregory Maxwell)
c769c4a Avoid counting failed connect attempts when probably offline. (Gregory Maxwell)

codablock added a commit to codablock/dash that referenced this pull request Dec 22, 2017

Merge #8065: Addrman offline attempts
6182d10 Do not increment nAttempts by more than one for every Good connection. (Gregory Maxwell)
c769c4a Avoid counting failed connect attempts when probably offline. (Gregory Maxwell)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment