Reduce download timeouts as blocks arrive #5976
Conversation
|
Nice, I like the simplicity of the semantics. Will test. |
|
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. |
|
@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. |
|
Travis failed, and this doesn't look like a random failure. |
|
Fixed |
|
ACK, feel free to squash. |
sdaftuar
force-pushed
the
reduce-timeout-window
branch
from
026e855 to
d8e6ed7
Compare
|
Hmm, it seems you're not using nValidatedQueuedBefore at all anymore. Is that intentional? |
sdaftuar
force-pushed
the
reduce-timeout-window
branch
from
d8e6ed7 to
ae9c8ad
Compare
|
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. |
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.
sdaftuar
force-pushed
the
reduce-timeout-window
branch
from
ae9c8ad to
8ba7f84
Compare
|
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? |
|
Code review and lightly tested (synced a partly-synced main chain using it) ACK. |
|
utACK |
8ba7f84 Reduce download timeouts as blocks arrive (Suhas Daftuar)
|
Hi,
Removing line 4148 in mail.cpp removes the warning, but maybe there is more to do/check. |
Merge some upstream commits that add better handling of the header/block download logic. Includes: Reduce download timeouts as blocks arrive bitcoin/bitcoin#5976 Prevent peer flooding inv request queue bitcoin/bitcoin#7079
5 participants
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.)