Avoid counting failed connect attempts when probably offline. #6030

1. 
   gmaxwell commented at 7:50 pm on April 19, 2015: contributor

   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).

2. 
   gmaxwell commented at 8:43 pm on April 19, 2015: contributor

   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.
3. 
   gmaxwell commented at 8:46 pm on April 19, 2015: contributor

   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.

4. laanwj added the label P2P on Apr 20, 2015
5. 
   laanwj commented at 6:03 am on May 18, 2015: member

   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.

6. 
   sipa commented at 2:14 pm on June 14, 2015: member

   Needs rebase.
7. 
   ghost commented at 3:05 pm on June 14, 2015: none

   Related #5886
8. 
   sipa commented at 2:04 pm on June 16, 2015: member

   Rebased version: https://github.com/sipa/bitcoin/commits/addrman_offline_attemps
9. 
   sipa commented at 0:14 am on November 13, 2015: member

   Needs rebase.
10. 
    gmaxwell commented at 0:45 am on November 13, 2015: contributor

    oh wow, totally forgot about this.
11. 
    mcelrath commented at 10:28 pm on November 13, 2015: none

    This likely fixes #6903, I’ll test.
12. 
    gmaxwell commented at 8:05 am on November 14, 2015: contributor

    I’ll rebase.
13. 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).

    d40154bf1c
14. 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.

    801ffd89f4
15. 
    gmaxwell commented at 0:14 am on November 23, 2015: contributor

    Rebased.
16. 
    laanwj commented at 9:52 am on November 27, 2015: member

    @mcelrath any testing results yet?
17. 
    sipa commented at 3:04 pm on April 10, 2016: member

    Needs rebase.
18. 
    gmaxwell commented at 4:41 pm on April 10, 2016: contributor

    @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.
19. 
    dcousens commented at 6:51 am on April 11, 2016: contributor

    concept ACK
20. 
    sipa commented at 10:43 am on April 11, 2016: member

    @gmaxwell Just close and reopen?
21. laanwj closed this on Apr 15, 2016

    ---

22. 
    laanwj commented at 6:03 am on April 15, 2016: member

    I tried to close and reopen but it’s not letting me reopen: “The repository that submitted this pull request has been deleted”

    gmaxwell wants to merge 2 commits into bitcoin:master from unknown repository

    Either it’s getting the wrong information or you’ve deleted more than just the branch

23. 
    MarcoFalke commented at 8:10 am on April 15, 2016: member

    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.

24. 
    gmaxwell commented at 7:27 am on May 17, 2016: contributor

    Reopened as #8065 – sorry, didn’t see the closure and comments until now.
25. MarcoFalke locked this on Sep 8, 2021

Contributors
gmaxwell laanwj sipa ghost mcelrath dcousens MarcoFalke

Labels
P2P