Currently, we're keeping a timeout for each requested block, starting from when it is requested, with a correction factor for the number of blocks in the queue.
That's unnecessarily complicated and inaccurate.
As peers process block requests in order, we can make the timeout for each block start counting only when all previous ones have been received, and have a correction based on the number of peers, rather than the total number of blocks.
Concept ACK
Updated to add a simple self consistency check.
5844 | - if (queuedBlock.nTimeDisconnect > nTimeoutIfRequestedNow) { 5845 | - LogPrint("net", "Reducing block download timeout for peer=%d block=%s, orig=%d new=%d\n", pto->id, queuedBlock.hash.ToString(), queuedBlock.nTimeDisconnect, nTimeoutIfRequestedNow); 5846 | - queuedBlock.nTimeDisconnect = nTimeoutIfRequestedNow; 5847 | - } 5848 | - if (queuedBlock.nTimeDisconnect < nNow) { 5849 | + if (nNow > state.nDownloadingSince + 500000 * consensusParams.nPowTargetSpacing * (4 + nPeersWithValidatedDownloads)) {
state.nDownloadingSince + 500000 * consensusParams.nPowTargetSpacing * (4 + nPeersWithValidatedDownloads)
Could we name this constant variable? As what it represents isn't exactly clear IMHO. Comment, variable, constant, whatever.
194 | @@ -195,15 +195,10 @@ namespace { 195 | struct QueuedBlock { 196 | uint256 hash; 197 | CBlockIndex *pindex; //! Optional. 198 | - int64_t nTime; //! Time of "getdata" request in microseconds.
Needs merge conflict solved in this line, I guess.
Rebased, and introduced constants for the timeouts.
utACK b18c125
Seems like an obvious improvement. Concept ACK.
Currently, we're keeping a timeout for each requested block, starting
from when it is requested, with a correction factor for the number of
blocks in the queue.
That's unnecessarily complicated and inaccurate.
As peers process block requests in order, we can make the timeout for each
block start counting only when all previous ones have been received, and
have a correction based on the number of peers, rather than the total number
of blocks.
105 | @@ -106,6 +106,10 @@ static const unsigned int AVG_INVENTORY_BROADCAST_INTERVAL = 5; 106 | static const unsigned int AVG_FEEFILTER_BROADCAST_INTERVAL = 10 * 60; 107 | /** Maximum feefilter broadcast delay after significant change. */ 108 | static const unsigned int MAX_FEEFILTER_CHANGE_DELAY = 5 * 60; 109 | +/** Block download timeout base, expressed in millionths of the block interval */ 110 | +static const int64_t BLOCK_DOWNLOAD_TIMEOUT_BASE = 2000000;
Do I understand this correctly that BLOCK_DOWNLOAD_TIMEOUT_BASE is thus 2000000*(10min/1000000), ie. 20minutes? In either case, it is worth more expressive comment IMO.
Added the actual clock times to the comment.
utACK 0e24bbf
post-merge utACK
206 | @@ -212,6 +207,9 @@ namespace { 207 | 208 | /** Dirty block file entries. */ 209 | set<int> setDirtyFileInfo; 210 | + 211 | + /** Number of peers from which we're downloading blocks. */ 212 | + int nPeersWithValidatedDownloads = 0;
why not simply nPeersProvidingBlocks? The use of the word 'validated' seems to imply it does more than the comment states.
359 | + // Last validated block on the queue was received. 360 | + nPeersWithValidatedDownloads--; 361 | + } 362 | + if (state->vBlocksInFlight.begin() == itInFlight->second.second) { 363 | + // First block on the queue was received, update the start download time for the next one 364 | + state->nDownloadingSince = std::max(state->nDownloadingSince, GetTimeMicros());
When and why would it need to be a date in the future?
105 | @@ -106,6 +106,10 @@ static const unsigned int AVG_INVENTORY_BROADCAST_INTERVAL = 5; 106 | static const unsigned int AVG_FEEFILTER_BROADCAST_INTERVAL = 10 * 60; 107 | /** Maximum feefilter broadcast delay after significant change. */ 108 | static const unsigned int MAX_FEEFILTER_CHANGE_DELAY = 5 * 60; 109 | +/** Block download timeout base, expressed in millionths of the block interval (i.e. 20 min) */ 110 | +static const int64_t BLOCK_DOWNLOAD_TIMEOUT_BASE = 2000000;
Why 20 minutes?