Track block download times per individual block #7804

sipa · 2016-04-03T20:23:59Z

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.

btcdrak · 2016-04-03T20:44:46Z

Concept ACK

sipa · 2016-04-04T12:34:06Z

Updated to add a simple self consistency check.

dcousens · 2016-04-05T00:55:35Z

src/main.cpp

-                queuedBlock.nTimeDisconnect = nTimeoutIfRequestedNow;
-            }
-            if (queuedBlock.nTimeDisconnect < nNow) {
+            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.

sipa · 2016-04-06T15:47:06Z

Rebased, and introduced constants for the timeouts.

dcousens · 2016-04-07T03:11:05Z

utACK b18c125

gmaxwell · 2016-04-07T09:14:09Z

Seems like an obvious improvement. Concept ACK.

paveljanik · 2016-04-07T09:24:19Z

src/main.h

@@ -106,6 +106,10 @@ static const unsigned int AVG_INVENTORY_BROADCAST_INTERVAL = 5;
 static const unsigned int AVG_FEEFILTER_BROADCAST_INTERVAL = 10 * 60;
 /** Maximum feefilter broadcast delay after significant change. */
 static const unsigned int MAX_FEEFILTER_CHANGE_DELAY = 5 * 60;
+/** Block download timeout base, expressed in millionths of the block interval */
+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.

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.

0e24bbf Self check after the last peer is removed (Pieter Wuille) 2d1d658 Track block download times per individual block (Pieter Wuille)

laanwj · 2016-04-07T11:07:37Z

utACK 0e24bbf

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. Conflicts: src/main.cpp src/main.h Self check after the last peer is removed Github-Pull: #7804 Rebased-From: 2d1d658 0e24bbf

Now that bitcoin#7804 fixed the timeout handling, reduce the block timeout from 20 minutes to 10 minutes. 20 minutes is overkill.

Now that #7804 fixed the timeout handling, reduce the block timeout from 20 minutes to 10 minutes. 20 minutes is overkill. Conflicts: src/main.h Github-Pull: #7832 Rebased-From: 62b9a55

sdaftuar · 2016-04-11T15:58:14Z

post-merge utACK

Now that bitcoin#7804 fixed the timeout handling, reduce the block timeout from 20 minutes to 10 minutes. 20 minutes is overkill.

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. Conflicts: src/main.cpp src/main.h Self check after the last peer is removed Github-Pull: bitcoin#7804 Rebased-From: 2d1d658 0e24bbf

Now that bitcoin#7804 fixed the timeout handling, reduce the block timeout from 20 minutes to 10 minutes. 20 minutes is overkill. Conflicts: src/main.h Github-Pull: bitcoin#7832 Rebased-From: 62b9a55

rebroad · 2016-12-07T03:27:26Z

src/main.cpp

@@ -212,6 +207,9 @@ namespace {

    /** Dirty block file entries. */
    set<int> setDirtyFileInfo;
+
+    /** Number of peers from which we're downloading blocks. */
+    int nPeersWithValidatedDownloads = 0;


why not simply nPeersProvidingBlocks? The use of the word 'validated' seems to imply it does more than the comment states.

rebroad · 2016-12-07T03:31:53Z

src/main.cpp

+        }
+        if (state->vBlocksInFlight.begin() == itInFlight->second.second) {
+            // First block on the queue was received, update the start download time for the next one
+            state->nDownloadingSince = std::max(state->nDownloadingSince, GetTimeMicros());


When and why would it need to be a date in the future?

rebroad · 2016-12-07T03:38:49Z

src/main.h

@@ -106,6 +106,10 @@ static const unsigned int AVG_INVENTORY_BROADCAST_INTERVAL = 5;
 static const unsigned int AVG_FEEFILTER_BROADCAST_INTERVAL = 10 * 60;
 /** Maximum feefilter broadcast delay after significant change. */
 static const unsigned int MAX_FEEFILTER_CHANGE_DELAY = 5 * 60;
+/** Block download timeout base, expressed in millionths of the block interval (i.e. 20 min) */
+static const int64_t BLOCK_DOWNLOAD_TIMEOUT_BASE = 2000000;


Why 20 minutes?

laanwj added the P2P label Apr 4, 2016

sipa force-pushed the betterkicktimeout branch from 5f359ed to 5bc71fc Compare April 4, 2016 11:46

dcousens reviewed Apr 5, 2016
View reviewed changes

sipa force-pushed the betterkicktimeout branch from 5bc71fc to b18c125 Compare April 6, 2016 15:46

paveljanik reviewed Apr 7, 2016
View reviewed changes

sipa added 2 commits April 7, 2016 12:13

Self check after the last peer is removed

0e24bbf

sipa force-pushed the betterkicktimeout branch from b18c125 to 0e24bbf Compare April 7, 2016 10:13

laanwj merged commit 0e24bbf into bitcoin:master Apr 7, 2016

laanwj added a commit that referenced this pull request Apr 7, 2016

Merge #7804: Track block download times per individual block

1ddf0ce

0e24bbf Self check after the last peer is removed (Pieter Wuille) 2d1d658 Track block download times per individual block (Pieter Wuille)

laanwj mentioned this pull request Apr 7, 2016

Reduce block timeout to 10 minutes #7832

Merged

laanwj added a commit to laanwj/bitcoin that referenced this pull request Apr 7, 2016

Reduce block timeout to 10 minutes

62b9a55

Now that bitcoin#7804 fixed the timeout handling, reduce the block timeout from 20 minutes to 10 minutes. 20 minutes is overkill.

zander pushed a commit to bitcoinclassic/bitcoinclassic that referenced this pull request May 23, 2016

Reduce block timeout to 10 minutes

c7dfbad

Now that bitcoin#7804 fixed the timeout handling, reduce the block timeout from 20 minutes to 10 minutes. 20 minutes is overkill.

rebroad reviewed Dec 7, 2016

View reviewed changes

codablock mentioned this pull request Dec 11, 2017

Backport missing PRs from Bitcoin 0.13 - Part 1 dashpay/dash#1770

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Track block download times per individual block #7804

Track block download times per individual block #7804

Uh oh!

sipa commented Apr 3, 2016

Uh oh!

btcdrak commented Apr 3, 2016

Uh oh!

sipa commented Apr 4, 2016

Uh oh!

dcousens Apr 5, 2016

Uh oh!

sipa commented Apr 6, 2016

Uh oh!

dcousens commented Apr 7, 2016

Uh oh!

gmaxwell commented Apr 7, 2016

Uh oh!

paveljanik Apr 7, 2016

Uh oh!

sipa Apr 7, 2016

Uh oh!

laanwj commented Apr 7, 2016

Uh oh!

sdaftuar commented Apr 11, 2016

Uh oh!

rebroad Dec 7, 2016

Uh oh!

rebroad Dec 7, 2016

Uh oh!

rebroad Dec 7, 2016

Uh oh!

Uh oh!

Track block download times per individual block #7804

Track block download times per individual block #7804

Uh oh!

Conversation

sipa commented Apr 3, 2016

Uh oh!

btcdrak commented Apr 3, 2016

Uh oh!

sipa commented Apr 4, 2016

Uh oh!

dcousens Apr 5, 2016

Choose a reason for hiding this comment

Uh oh!

sipa commented Apr 6, 2016

Uh oh!

dcousens commented Apr 7, 2016

Uh oh!

gmaxwell commented Apr 7, 2016

Uh oh!

paveljanik Apr 7, 2016

Choose a reason for hiding this comment

Uh oh!

sipa Apr 7, 2016

Choose a reason for hiding this comment

Uh oh!

laanwj commented Apr 7, 2016

Uh oh!

sdaftuar commented Apr 11, 2016

Uh oh!

rebroad Dec 7, 2016

Choose a reason for hiding this comment

Uh oh!

rebroad Dec 7, 2016

Choose a reason for hiding this comment

Uh oh!

rebroad Dec 7, 2016

Choose a reason for hiding this comment

Uh oh!

Uh oh!