Avoid counting failed connect attempts when probably offline. #6030
Conversation
|
I'm less sure if this one should be tagged for 0.10, PR #6029's penalty cap stops most of the bleeding (and prevents the addrman updates from being a regression); though the behavior this addresses is pretty bad. |
|
On the second commit, using the acceptance of a block might be a better measure of onlineness; I didn't because its likely even slower (more conservative) than Good, and because addrman has no knowledge of the blockchain. If there is a preference, I could add a call to addrman for letting it know that it's probably online as of a particular time. If I did that I could probably drop the first commit entirely-- incrementing once per block is probably strong enough to not need the other test. Opinions welcome. |
|
Re: checking if online, In my opinion (successful) network activity is the best measure. Accepting blocks is a different concern of the application, and can also happen through other sources (such as from disk, from RPC). Needs rebase. |
|
Needs rebase. |
|
Related #5886 |
|
Rebased version: https://github.com/sipa/bitcoin/commits/addrman_offline_attemps |
|
Needs rebase. |
|
oh wow, totally forgot about this. |
|
This likely fixes #6903, I'll test. |
|
I'll rebase. |
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).
This slows the increase of the nAttempts in addrman while partitioned, even if the node hasn't yet noticed the partitioning.
|
Rebased. |
|
@mcelrath any testing results yet? |
|
Needs rebase. |
|
@sipa rebased at https://github.com/gmaxwell/bitcoin/tree/addrman_offline_attempts but github hasn't picked it up because I'd previously removed the branch in my repository. ... should I just close this? it seems to be going nowhere. |
|
concept ACK |
|
@gmaxwell Just close and reopen? |
|
I tried to close and reopen but it's not letting me reopen: "The repository that submitted this pull request has been deleted"
Either it's getting the wrong information or you've deleted more than just the branch |
|
The branch must be at the same commit as it was at the time of closing, so this can not work anyway... You can use https://github.com/bitcoin/bitcoin/compare/master...gmaxwell:addrman_offline_attempts?expand=1 to open a fresh pull and link to this one in the pull body. |
|
Reopened as #8065 -- sorry, didn't see the closure and comments until now. |
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).