Reduce download timeouts as blocks arrive #5976

1. 
   sdaftuar commented at 7:16 pm on April 6, 2015: member

   The current block download timeout logic uses the number of blocks in flight (with validated headers) to extend the time we give a peer to deliver a block before disconnecting; the idea is that we permit extra time if there are many blocks in flight, because we may be saturating our own network link.

   However, this means that if there are many blocks in flight when a block request goes out, then a peer may be able to stall for a very long time on such a block request before we disconnect and request the block elsewhere. In particular, during initial block download, we might have 128 blocks in flight at a time, meaning that the block download timeout may not trigger even if a peer doesn’t deliver a block for over 10 hours. This would be the case even if the other 127 blocks requested before a given block were all delivered very quickly.

   With this commit, we ratchet down the block download window if the same block request made now (with the current time and current number of blocks in flight) would result in an earlier timeout than the one calculated when the block request originally went out. As we approach the end of a large block download, if a handful of peers have a handful of blocks still outstanding, the download window given to each peer before we disconnect will be substantially reduced as the number of blocks in flight is reduced.

   Note that in all circumstances, a peer will have at least 20 minutes to deliver a block before we’d disconnect. Whenever this code reduces the timeout, it is reducing it to a time in the future that is at least 20 minutes from now.

   (This was motivated by #5662, which in its current form would allow inbound peers to serve blocks and result in potentially even more blocks in flight.)

2. laanwj added the label P2P on Apr 7, 2015
3. 
   sipa commented at 8:53 pm on April 7, 2015: member

   Nice, I like the simplicity of the semantics. Will test.
4. 
   sipa commented at 9:52 pm on April 7, 2015: member

   Ideally you’d only take blocks requested before this one into account for computing the allowed remaining time, but that’s much harder to implement.

   I did very light testing (parting mainnet sync), and it seems to work well so far.

5. 
   sdaftuar commented at 8:05 pm on April 9, 2015: member

   @sipa I thought you raised a good point about ignoring blocks requested after a given one, so I’ve pushed up a commit that does so (for block requests going to the same peer).

   I am not entirely sure the additional 5 lines of code (and one extra piece of state for each peer) is worth the code complexity, so if you prefer the first version I can drop this commit, but it seemed unfortunate that in the first approach, if a single peer had 16 blocks outstanding at the end of your download, that even the reduced timeout window would still be 95 minutes away – a long time compared with how fast IBD should be with headers-first. With this change, in that same situation, the peer would have to deliver the first block in 20 minutes or be disconnected, which seems like a meaningful improvement.

6. 
   sipa commented at 9:32 am on April 11, 2015: member

   Travis failed, and this doesn’t look like a random failure.
7. 
   sdaftuar commented at 10:07 am on April 11, 2015: member

   Fixed
8. 
   sipa commented at 10:28 am on April 11, 2015: member

   ACK, feel free to squash.
9. sdaftuar force-pushed on Apr 11, 2015
10. 
    sipa commented at 3:01 pm on April 12, 2015: member

    Hmm, it seems you’re not using nValidatedQueuedBefore at all anymore. Is that intentional?
11. sdaftuar force-pushed on Apr 13, 2015
12. 
    sdaftuar commented at 5:39 pm on April 13, 2015: member

    Thanks for catching that; it was intentional to not use it anymore but it was an oversight not to remove the unnecessary variable from QueuedBlock. Since the original timeout calculation was based only on information known at the time of a block request, I opted to store the disconnect time directly.

    I’ve updated the pull to remove that variable.

13. Reduce download timeouts as blocks arrive

    Compare the block download timeout to what the timeout would be if calculated
    based on current time and current value of nQueuedValidatedHeaders, but
    ignoring other in-flight blocks from the same peer. If the calculation based on
    present conditions is shorter, then set that to be the time after which we
    disconnect the peer for not delivering this block.

    8ba7f842e5
14. sdaftuar force-pushed on Apr 15, 2015
15. 
    sdaftuar commented at 4:32 pm on May 6, 2015: member

    Now that #5662 has been merged, which can increase the number of blocks in flight, I think this would be a useful behavior change. Anything else I can do here?
16. 
    gavinandresen commented at 4:23 pm on May 13, 2015: contributor

    Code review and lightly tested (synced a partly-synced main chain using it) ACK.
17. 
    laanwj commented at 11:02 am on May 26, 2015: member

    utACK
18. laanwj merged this on May 26, 2015
19. laanwj closed this on May 26, 2015

    ---

20. laanwj referenced this in commit 9f7809f6c3 on May 26, 2015
21. 
    arthurbouquet commented at 9:06 am on May 27, 2015: none

    Hi, Just to let you know that there is a warning during build since this merge:

    main.cpp: In function ‘bool SendMessages(CNode*, bool)’: main.cpp:4841:31: warning: unused variable ‘consensusParams’ [-Wunused-variable] const Consensus::Params& consensusParams = Params().GetConsensus();

    Removing line 4148 in mail.cpp removes the warning, but maybe there is more to do/check.

22. Fuzzbawls referenced this in commit 50285d0427 on Sep 26, 2016
23. MarcoFalke locked this on Sep 8, 2021

Contributors
sdaftuar sipa gavinandresen laanwj arthurbouquet

Labels
P2P