Do not download transactions during initial blockchain sync #7164

ptschip · 2015-12-03T14:10:01Z

What I've noticed now that transaction rates are getting much higher is that every time a block is downloaded during the initial sync, sometimes 5, 10 or more new transactions are being downloaded as well and is slowing down the initial sync process. There is no reason to do a "getdata" and download these transactions since they are not getting accepted into the mempool anyway.

laanwj · 2015-12-03T14:18:43Z

src/main.cpp

@@ -5653,7 +5653,7 @@ bool SendMessages(CNode* pto, bool fSendTrickle)
        //
        // Message: getdata (non-blocks)
        //
-        while (!pto->fDisconnect && !pto->mapAskFor.empty() && (*pto->mapAskFor.begin()).first <= nNow)
+        while (!pto->fDisconnect && !pto->mapAskFor.empty() && (*pto->mapAskFor.begin()).first <= nNow && !IsInitialBlockDownload())


No need to check IsInitialBlockDownload() every iteration of the while loop.

made the change and pushed.

On 03/12/2015 6:19 AM, Wladimir J. van der Laan wrote:

In src/main.cpp
#7164 (comment):

@@ -5653,7 +5653,7 @@ bool SendMessages(CNode* pto, bool fSendTrickle)
//
// Message: getdata (non-blocks)
//

while (!pto->fDisconnect && !pto->mapAskFor.empty() && (*pto->mapAskFor.begin()).first <= nNow)

while (!pto->fDisconnect && !pto->mapAskFor.empty() && (*pto->mapAskFor.begin()).first <= nNow && !IsInitialBlockDownload())

No need to check IsInitialBlockDownload() every iteration of the while
loop.

—
Reply to this email directly or view it on GitHub
https://github.com/bitcoin/bitcoin/pull/7164/files#r46555484.

laanwj · 2015-12-03T14:50:50Z

Despite not liking how more and more depends on IsInitialBlockDownload(), I think this makes sense. Downloading transactions during initial sync just gets a long list of "Rejected: Non-final" and wastes bandwidth and verification overhead.

jtimon · 2015-12-03T15:10:38Z

utACK

petertodd · 2015-12-03T17:10:40Z

Concept ACK

dcousens · 2015-12-03T20:13:27Z

conceptACK/utACK

ptschip · 2015-12-03T20:33:26Z

acutally please don't merge this yet, there is a problem with the
regression tests, i think something to do with mocktime...it appears
that the tests always think that we are syncing the blockchain...i'm
investigating

On 03/12/2015 12:14 PM, Daniel Cousens wrote:

ACK

—
Reply to this email directly or view it on GitHub
#7164 (comment).

ptschip · 2015-12-03T23:10:12Z

Travis is happy now...during running the regression tests, the block
timestamps in the cache were too old and so the memory pools would not
sync thinking that an initial block download was happening. The last
block needs to be within the last 24 hours. But, I think I need to add
one more thing, if the cache folder is more than 1 day old it needs to
automatically be rebuilt otherwise the regression tests will fail again.

in util.py:

   -block_time = 1388534400
   +block_time = int(round(time.time() - (200 * 10 * 60) ))

On 03/12/2015 12:14 PM, Daniel Cousens wrote:

ACK

—
Reply to this email directly or view it on GitHub
#7164 (comment).

maflcko · 2015-12-03T23:34:13Z

qa/rpc-tests/test_framework/util.py

-        # at 1 Jan 2014
-        block_time = 1388534400
+        # blocks are created with timestamps 10 minutes apart
+        # going backwards in time from now


Nit: ", starting 2000 minutes in the past"?

sure, that sounds better

(and it's 2000 minutes in the past).

I'll push it up with the changes for updating the cache every 24hours.

On 03/12/2015 3:34 PM, MarcoFalke wrote:

In qa/rpc-tests/test_framework/util.py
#7164 (comment):

@@ -154,9 +154,9 @@ def initialize_chain(test_dir):

# Create a 200-block-long chain; each of the 4 nodes # gets 25 mature blocks and 25 immature.

# blocks are created with timestamps 10 minutes apart, starting

# at 1 Jan 2014

block_time = 1388534400

# blocks are created with timestamps 10 minutes apart

# going backwards in time from now

Nit: ", starting 200 minutes in the past"?

—
Reply to this email directly or view it on GitHub
https://github.com/bitcoin/bitcoin/pull/7164/files#r46629519.

Ok that should do it,

pushed and travis is good

have a look when you can.

On 03/12/2015 3:34 PM, MarcoFalke wrote:

In qa/rpc-tests/test_framework/util.py
#7164 (comment):

@@ -154,9 +154,9 @@ def initialize_chain(test_dir):

# Create a 200-block-long chain; each of the 4 nodes # gets 25 mature blocks and 25 immature.

# blocks are created with timestamps 10 minutes apart, starting

# at 1 Jan 2014

block_time = 1388534400

# blocks are created with timestamps 10 minutes apart

# going backwards in time from now

Nit: ", starting 200 minutes in the past"?

—
Reply to this email directly or view it on GitHub
https://github.com/bitcoin/bitcoin/pull/7164/files#r46629519.

ptschip · 2015-12-04T06:13:46Z

@MarcoFalke Fixed that nit. Also added refreshing the cache every twelve hours which will prevent any very very long running regression tests from going over the 24hr limit for the age of the cache and subsequently preventing the mempools from syncing.

laanwj · 2015-12-04T08:02:14Z

Ugh that "maximum 24 hours" check has tripped us up multiple times.

Another option to avoid the cache invalid issue would be to change nMaxTipAge to 0x7fffffff, as it is also for regtest, as it is for testnet (see #5987)

https://github.com/bitcoin/bitcoin/blob/master/src/chainparams.cpp#L172

I'd prefer that myself to having to expire the cache every 12 hours just to avoid this check. Although there's something to be said for testing IsInitialBlockDownload itself as well... so not sure

Yet another option would be to set a mock time, but this isn't easy to do consistently.

ptschip · 2015-12-04T20:48:43Z

@laanwj Changing nmaxtipage certainly is an easy fix but I never like the idea of changing the behaviour of an app before regression testing, it sometimes bites down the road. I used mocktime instead but then had issues with nodehandling because GetTime() doesn't increment properly once you set mocktime. Made some edits there to make GetTime() increment correctly if mocktime is set.

Also needed a small edit to p2pfullblocktest to set the time base to MOCKTIME.

Pushed and travis is happy...

jtimon · 2015-12-04T22:23:50Z

re-utACK

ptschip · 2015-12-05T01:25:30Z

Oops there is one last thing, there are a few of extended tests that might need the mocktime. I can't execute them because of ndbm which doesn't run on windows so I'll have to try things out on Travis. I think about 4 scripts that might need fixing up. Shouldn't take too long...I'll let you know when it's done.

paveljanik · 2015-12-05T09:49:40Z

Rebase needed.

sdaftuar · 2015-12-05T14:39:56Z

I like the overall goal here, but I'm not sure I follow the reasoning for changing the behavior of SetMockTime. One advantage of the way mocktime works now is that for the places in the code that call GetTime(), the behavior is deterministic -- the time will always be whatever the last mocktime call set it to. I think this determinism can be a useful feature for testing, and making it an offset of the current system time will eliminate the determinism.

Can we work around this problem differently in the tests? I don't actually understand why the time would need to advance but I haven't yet poked into the problems you ran into. But for instance, one workaround I've used in tests I've previously written is to just mine a single block at the beginning of the test to ensure that the nodes are out of IBD (I recognize this workaround may not easily apply to all testing scenarios though).

ptschip · 2015-12-05T16:07:48Z

Mocktime needs to advance otherwise banning and unbanning doesn't work
so scripts such as nodehandling end up failing. I think having mocktime
advance, gives us a more realisitic functionality for testing as it
behaves in the same way as the application does in the real world.

On 05/12/2015 6:40 AM, Suhas Daftuar wrote:

I like the overall goal here, but I'm not sure I follow the reasoning
for changing the behavior of |SetMockTime|. One advantage of the way
mocktime works now is that for the places in the code that call
|GetTime()|, the behavior is deterministic -- the time will always be
whatever the last mocktime call set it to. I think this determinism
can be a useful feature for testing, and making it an offset of the
current system time will eliminate the determinism.

Can we work around this problem differently in the tests? I don't
actually understand why the time would need to advance but I haven't
yet poked into the problems you ran into. But for instance, one
workaround I've used in tests I've previously written is to just mine
a single block at the beginning of the test to ensure that the nodes
are out of IBD (I recognize this workaround may not easily apply to
all testing scenarios though).

—
Reply to this email directly or view it on GitHub
#7164 (comment).

ptschip · 2015-12-05T18:06:48Z

@sdaftuar Thinking about this some more perhaps you are right, but maybe we can have both. We can have the mocktime we have now and also have a mocktime that will advance for those that need it. It would certainly be easier than having to fix up these python scripts.

Github-Pull: bitcoin#7164 Rebased-From: 39a525c

maflcko · 2016-06-09T10:55:18Z

Backported as part of #7938. Removing label 'Needs backport'.

Github-Pull: bitcoin#7164 Rebased-From: 39a525c

After discussion in bitcoin#7164 I think this is better. Max tip age was introduced in bitcoin#5987 to make it possible to run testnet-in-a-box. But associating this behavior with the testnet chain is wrong conceptually, as it is not needed in normal usage. Should aim to make testnet test the software as-is. Replace it with a (debug) option `-maxtipage`, which can be specified only in the specific case.

Skip "tx" messages during initial block download. We avoid requesting these messages by not request non-block inventory during IBD. This is equivalent to bitcoin/bitcoin#7164, but we also ignore unsolicited `tx` messages (which `zcashd` nodes will never send). Fixes #4943.

laanwj reviewed Dec 3, 2015
View reviewed changes

laanwj added the P2P label Dec 3, 2015

ptschip force-pushed the tx_getdata branch from bdfdce1 to ad6e1f3 Compare December 3, 2015 14:44

ptschip force-pushed the tx_getdata branch from ad6e1f3 to 3c15462 Compare December 3, 2015 22:46

maflcko reviewed Dec 3, 2015
View reviewed changes

ptschip force-pushed the tx_getdata branch 3 times, most recently from 3fb49df to 725835b Compare December 4, 2015 05:50

ptschip force-pushed the tx_getdata branch 5 times, most recently from 2acebaa to 030c552 Compare December 4, 2015 20:24

ptschip force-pushed the tx_getdata branch 2 times, most recently from ea8e6d6 to b51b7bf Compare December 5, 2015 17:15

laanwj mentioned this pull request Feb 9, 2016

Client receives and stores transactions while syncing #2303

Closed

laanwj added the Needs backport label Feb 11, 2016

ptschip deleted the tx_getdata branch April 5, 2016 02:18

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Apr 27, 2016

Do not download transactions during inital sync

9095594

Github-Pull: bitcoin#7164 Rebased-From: 39a525c

maflcko removed the Needs backport label Jun 9, 2016

thokon00 pushed a commit to faircoin/faircoin that referenced this pull request Jun 28, 2016

Do not download transactions during inital sync

28bd475

Github-Pull: bitcoin#7164 Rebased-From: 39a525c

dgenr8 mentioned this pull request Jan 8, 2017

Cherries bitcoinxt/bitcoinxt#184

Merged

deadalnix mentioned this pull request Feb 27, 2017

Make max tip age an option instead of chainparam BitcoinUnlimited/BitcoinUnlimited#321

Merged

str4d mentioned this pull request Jan 5, 2021

Skip "tx" messages during initial block download. zcash/zcash#4944

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

Do not download transactions during initial blockchain sync #7164

Do not download transactions during initial blockchain sync #7164

ptschip commented Dec 3, 2015

laanwj Dec 3, 2015

ptschip Dec 3, 2015

laanwj commented Dec 3, 2015

jtimon commented Dec 3, 2015

petertodd commented Dec 3, 2015

dcousens commented Dec 3, 2015

ptschip commented Dec 3, 2015

ptschip commented Dec 3, 2015

maflcko Dec 3, 2015

ptschip Dec 4, 2015

ptschip Dec 4, 2015

ptschip commented Dec 4, 2015

laanwj commented Dec 4, 2015

ptschip commented Dec 4, 2015

jtimon commented Dec 4, 2015

ptschip commented Dec 5, 2015

paveljanik commented Dec 5, 2015

sdaftuar commented Dec 5, 2015

ptschip commented Dec 5, 2015

ptschip commented Dec 5, 2015

maflcko commented Jun 9, 2016

Do not download transactions during initial blockchain sync #7164

Do not download transactions during initial blockchain sync #7164

Conversation

ptschip commented Dec 3, 2015

laanwj Dec 3, 2015

Choose a reason for hiding this comment

ptschip Dec 3, 2015

Choose a reason for hiding this comment

laanwj commented Dec 3, 2015

jtimon commented Dec 3, 2015

petertodd commented Dec 3, 2015

dcousens commented Dec 3, 2015

ptschip commented Dec 3, 2015

ptschip commented Dec 3, 2015

maflcko Dec 3, 2015

Choose a reason for hiding this comment

ptschip Dec 4, 2015

Choose a reason for hiding this comment

ptschip Dec 4, 2015

Choose a reason for hiding this comment

ptschip commented Dec 4, 2015

laanwj commented Dec 4, 2015

ptschip commented Dec 4, 2015

jtimon commented Dec 4, 2015

ptschip commented Dec 5, 2015

paveljanik commented Dec 5, 2015

sdaftuar commented Dec 5, 2015

ptschip commented Dec 5, 2015

ptschip commented Dec 5, 2015

maflcko commented Jun 9, 2016