Do not download transactions during initial blockchain sync

ptschip commented at 2:10 pm on December 3, 2015: contributor

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.

in src/main.cpp: in bdfdce1f2a outdated

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

laanwj commented at 2:18 pm on December 3, 2015:

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

ptschip commented at 2:51 pm on December 3, 2015:

made the change and pushed.

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

In src/main.cpp #7164 (review):
@@ -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 added the label P2P on Dec 3, 2015

ptschip force-pushed on Dec 3, 2015

laanwj commented at 2:50 pm on December 3, 2015: member

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 commented at 3:10 pm on December 3, 2015: contributor

utACK

petertodd commented at 5:10 pm on December 3, 2015: contributor

Concept ACK

dcousens commented at 8:13 pm on December 3, 2015: contributor

conceptACK/utACK

ptschip commented at 8:33 pm on December 3, 2015: contributor

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 force-pushed on Dec 3, 2015

ptschip commented at 11:10 pm on December 3, 2015: contributor

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:

0   -block_time = 1388534400
1   +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).

in qa/rpc-tests/test_framework/util.py: in 3c1546242b outdated

153@@ -154,9 +154,9 @@ def initialize_chain(test_dir):
154 
155         # Create a 200-block-long chain; each of the 4 nodes
156         # gets 25 mature blocks and 25 immature.
157-        # blocks are created with timestamps 10 minutes apart, starting
158-        # at 1 Jan 2014
159-        block_time = 1388534400
160+        # blocks are created with timestamps 10 minutes apart
161+        # going backwards in time from now

MarcoFalke commented at 11:34 pm on December 3, 2015:

Nit: “, starting 2000 minutes in the past”?

ptschip commented at 0:57 am on December 4, 2015:

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 (review):
@@ -154,9 +154,9 @@ def initialize_chain(test_dir):
0     # Create a 200-block-long chain; each of the 4 nodes
1     # 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 commented at 1:52 am on December 4, 2015:

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 (review):
@@ -154,9 +154,9 @@ def initialize_chain(test_dir):
0     # Create a 200-block-long chain; each of the 4 nodes
1     # 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 force-pushed on Dec 4, 2015

ptschip commented at 6:13 am on December 4, 2015: contributor

@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 commented at 8:02 am on December 4, 2015: member

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 force-pushed on Dec 4, 2015

ptschip commented at 8:48 pm on December 4, 2015: contributor

@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 commented at 10:23 pm on December 4, 2015: contributor

re-utACK

ptschip commented at 1:25 am on December 5, 2015: contributor

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 commented at 9:49 am on December 5, 2015: contributor

Rebase needed.

sdaftuar commented at 2:39 pm on December 5, 2015: member

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 commented at 4:07 pm on December 5, 2015: contributor

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 force-pushed on Dec 5, 2015

ptschip commented at 6:06 pm on December 5, 2015: contributor

@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.

ptschip commented at 6:31 pm on December 5, 2015: contributor

@sdaftuar And then if we go with a more complex mocktime it probably needs to be in a separate PR. I think I’m going to go back to the way the original mocktime was and see if I can get that nodehandling script to work. That would be much simpler and more within the scope of this PR.

ptschip force-pushed on Dec 5, 2015

ptschip force-pushed on Dec 6, 2015

ptschip commented at 1:40 am on December 6, 2015: contributor

@laanwj @sdaftuar I reverted back to the original mocktime which doesn’t increment GetTime(). Instead, I made edits to a few of the python scripts to get them to work with mocktime and checked both the regular and extended scripts to make sure they all work. Pushed and Travis is happy.

Perhaps in the future we should have a mocktime that is both deterministic as well as one that allows GetTime() to increment. I think having a more realistic test scenario is always best but I agree with @sdaftuar that we need to keep the current deterministic behavior of mocktime as well.

jtimon commented at 2:26 am on December 6, 2015: contributor

It seems reasonable to have 2 different setmocktime functions if they’re both useful for testing.

MarcoFalke commented at 9:52 am on December 6, 2015: member

utACK 233fe56

sdaftuar commented at 10:54 am on December 6, 2015: member

@ptschip I agree that an advancing mocktime can be useful too, and yeah probably makes sense to add that support in a separate PR.

I’m not sure it’s a good idea for the default test-framework behavior to be to enable mocktime for all tests (which changes the behavior of the code being tested). I guess the issue is that if you’re using initialize_chain which can give you an old cached datadir, then you probably want a mocktime that goes with it? Many of the tests now use initialize_chain_clean to avoid the issue of an old blockchain, and I think for tests that don’t need to use a mocktime (because of the way they are written) it’d be best to not change the behavior they’re testing by introducing use of mocktime.

It looks like running bitcoind with a mocktime of 0 is a no-op, so could we do something where the default mocktime used by start_node is only set to a non-zero value if the code that tries to generate a cached datadir is invoked?

sdaftuar commented at 11:01 am on December 6, 2015: member

I think another option would be to just have the tests that don’t need to use mocktime change their invocations of start_node to include -mocktime=0 as one of the arguments to bitcoind. That should override the argument that’s being added to util.py, and the test writer is likely aware if they’re not using the cached directory.

If others are fine with an extra argument being needed to avoid invoking mocktime that is fine with me, but I do think we should go back and make sure that we’re only using non-zero mocktimes on the tests that need them.

jtimon commented at 5:18 pm on December 6, 2015: contributor

As said, having 2 mocktime functions may be useful, but I completely agree with not using it in the tests that don’t need it. I think being explicit in the tests you need it may be more verbose than having it set implicitly for all tests, but is also clearer and more easily modifiable (if “explicit is better than implicit” is not enough).

ptschip commented at 6:10 pm on December 6, 2015: contributor

On 06/12/2015 3:02 AM, Suhas Daftuar wrote:

I think another option would be to just have the tests that don’t need to use mocktime change their invocations of start_node to include |-mocktime=0| as one of the arguments to bitcoind. That should override the argument that’s being added to |util.py|,

Yes, you can override mocktime in that way. In fact I had to do that in the maxuploadtarget.py to get it to work, although in that case I had to start_node with mocktime set to the current time, but it does work if one wants to use it that way. So for now I would think that would be sufficient for those that don’t want to use mocktime in their scripting?

ptschip commented at 6:31 pm on December 6, 2015: contributor

Another simple solution for those that don’t want to use or need to use mocktime in their scripts is simply to set MOCKTIME=0 at the very beginning.

On 06/12/2015 3:02 AM, Suhas Daftuar wrote:

I think another option would be to just have the tests that don’t need to use mocktime change their invocations of start_node to include |-mocktime=0| as one of the arguments to bitcoind. That should override the argument that’s being added to |util.py|, and the test writer is likely aware if they’re not using the cached directory.

If others are fine with an extra argument being needed to avoid invoking mocktime that is fine with me, but I do think we should go back and make sure that we’re only using non-zero mocktimes on the tests that need them.

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

laanwj commented at 4:34 pm on December 7, 2015: member

Changing nmaxtipage certainly is an easy fix but I never like the idea of changing the behaviour of an app before regression testing

I tend to agree. I also think the max tip age should never have been a chain parameter in the first place, but an option separately specified.

I like the new approach.

Edit: simulated time in tests is great, if it avoids sleep()s it speeds up the tests!

pstratem commented at 0:26 am on December 8, 2015: contributor

@ptschip please do the mocktime fix as a separate commit possibly even a separate pr? (someone argue with me on that))

ptschip commented at 4:32 am on December 8, 2015: contributor

@pstratem I already had taken the changes out of GetTime() and mocktime , reverting back to the current master branch version, and only updated the py scripts.instead so that they use mocktime correctly. The py scripts need those changes otherwise this PR will break them.

I have no additional changes to this PR, I think it’s complete.

dcousens commented at 11:56 pm on December 8, 2015: contributor

Tested ACK.

No transactions received during IBD
Transactions received after IBD

(that was the extent of my testing)

in qa/rpc-tests/maxuploadtarget.py: in 233fe563b5 outdated

246@@ -248,7 +247,7 @@ def run_test(self):
247         #stop and start node 0 with 1MB maxuploadtarget, whitelist 127.0.0.1
248         print "Restarting nodes with -whitelist=127.0.0.1"
249         stop_node(self.nodes[0], 0)
250-        self.nodes[0] = start_node(0, self.options.tmpdir, ["-debug", "-whitelist=127.0.0.1", "-maxuploadtarget=1", "-blockmaxsize=999000"])
251+        self.nodes[0] = start_node(0, self.options.tmpdir, ["-debug", "-whitelist=127.0.0.1", "-maxuploadtarget=1", "-blockmaxsize=999000" , "-mocktime="+str(int(time.time()))])

MarcoFalke commented at 0:06 am on December 9, 2015:

Nit: Shouldn’t this say MOCKTIME + 14*60*60*24?

ptschip commented at 2:23 am on December 9, 2015:

@MarcoFalke It works either way but I suppose for consistency it makes sense, I pushed up the change to maxuploadtarget.

sdaftuar commented at 0:45 am on December 9, 2015: member

I really don’t think we should change any tests to be invoking mocktime unless we actually need to (ie because the test only makes sense in such a context). If we go with the approach this PR suggests, so that we’re defaulting to use a mocktime unless the test explicitly sets MOCKTIME=0 or invokes start_nodes with a “-mocktime=0” command line argument, then I think we should go back through all the tests and explicitly turn off mocktime, unless the test’s behavior actually needs changing as a result of this PR.

It doesn’t seem like good practice to make a blanket change to the code path being exercised across all the tests without careful consideration of each test to ensure that the new test behavior is correct. That level of review seems out of scope for what this PR is trying to accomplish, so I think it makes more sense to be conservative and not change the behavior of any tests, except if there are any specific ones that need changing and can be called out for review as part of this PR.

I started to go through the list of tests alphabetically and the first two I looked at, bip65-cltv-p2p.py and bip65-cltv.py can both be made to work simply by disabling mocktime (I tried passing -mocktime=0 as an argument to start_nodes), which is consistent with my understanding of what those tests are doing.

jtimon commented at 1:47 am on December 9, 2015: contributor

No thoughts on having two different SetMockTime functions for now? We can discuss their unification in another PR.

ptschip commented at 2:35 am on December 9, 2015: contributor

@sdaftuar I agree with you. I didn’t realize at the beginning of all this where it was going and now with the realization that it’s very easy to turn off mocktime just by setting MOCKTIME=0 at the beginning of a script then it does make sense to turn it off where we don’t need it. I’ll go through the scripts tomorrow and turn off mocktime where it makes sense to do so.

dcousens commented at 2:44 am on December 9, 2015: contributor

@ptschip I’m not fully aware of the context around MOCKTIME, however, is it possible to ‘opt-in’ rather than out?

ptschip commented at 2:42 pm on December 9, 2015: contributor

@dcousens Now that we’ve gone through this whole exercise in mocktime, I think it’s becoming more clear as far as what we need in the py scripts. I suppose, Yes, we should be able opt-in rather than out and maybe that’s in the end the cleanest and most intuitive approach.

Hopefully we’re getting to the light at the end of this tunnel. I’ll go ahead and make make mocktime an opt-in and also use @sdaftuar suggestion and only apply it to scripts that need it while keeping the current deterministic behaviour of mocktime. I think this is starting to make sense.

ptschip force-pushed on Dec 9, 2015

ptschip commented at 9:16 pm on December 9, 2015: contributor

@sdaftuar @dcousens I Made these changes. Mocktime stays deterministic as it always has been, mocktime is opt in now for the py scripts, and only 5 scripts needed to be changed. I tested all scripts including the extended ones.

Pushed and Travis is happy. Comments?

in qa/rpc-tests/test_framework/util.py: in aa55bb29e1 outdated

134@@ -117,6 +135,7 @@ def initialize_chain(test_dir):
135     bitcoind and bitcoin-cli must be in search path.
136     """
137 
138+    #Refresh the cache if cache files are missing or are > 12 hours old

sdaftuar commented at 11:27 am on December 10, 2015:

I think this comment is extraneous now?

in qa/rpc-tests/bip65-cltv-p2p.py: in aa55bb29e1 outdated

41@@ -42,6 +42,9 @@ def __init__(self):
42         self.num_nodes = 1
43 
44     def setup_network(self):
45+        #This test requires mocktime

sdaftuar commented at 11:39 am on December 10, 2015:

I don’t understand why this test requires mocktime – I just tested locally and it runs fine if I remove the changes you made to this file. And logically I don’t understand how this test can fail based on the code change in this PR? I don’t believe any transactions are sent over the network in this test (only put into blocks).

in qa/rpc-tests/bipdersig-p2p.py: in aa55bb29e1 outdated

49@@ -50,6 +50,9 @@ def __init__(self):
50         self.num_nodes = 1
51 
52     def setup_network(self):
53+        #This test requires mocktime

sdaftuar commented at 11:41 am on December 10, 2015:

Same comments as above apply to this test; it works fine for me without any changes.

in qa/rpc-tests/invalidblockrequest.py: in aa55bb29e1 outdated

24@@ -25,9 +25,12 @@
25 # Use the ComparisonTestFramework with 1 node: only use --testbinary.
26 class InvalidBlockRequestTest(ComparisonTestFramework):
27 
28+
29     ''' Can either run this test as 1 node with expected answers, or two and compare them. 
30         Change the "outcome" variable from each TestInstance object to only do the comparison. '''
31     def __init__(self):
32+        #This test requires mocktime
33+        enable_mocktime()

sdaftuar commented at 11:42 am on December 10, 2015:

This test also doesn’t seem to do anything with transactions and works fine for me locally without any changes.

in qa/rpc-tests/p2p-fullblocktest.py: in aa55bb29e1 outdated

34@@ -35,12 +35,15 @@ class FullBlockTest(ComparisonTestFramework):
35     ''' Can either run this test as 1 node with expected answers, or two and compare them. 
36         Change the "outcome" variable from each TestInstance object to only do the comparison. '''
37     def __init__(self):
38+        #This test requires mocktime
39+        enable_mocktime()

sdaftuar commented at 11:43 am on December 10, 2015:

Same for this test, no changes needed.

in qa/rpc-tests/listtransactions.py: in aa55bb29e1 outdated

31@@ -32,6 +32,11 @@ def check_array_result(object_array, to_match, expected):
32 
33 class ListTransactionsTest(BitcoinTestFramework):
34 
35+    def setup_nodes(self):
36+        #This test requires mocktime
37+        enable_mocktime()
38+        return start_nodes(4, self.options.tmpdir)
39+

sdaftuar commented at 11:48 am on December 10, 2015:

For this test I agree that something is needed to fix the test to work with this PR. Enabling mocktime as you do does the trick; alternatively, it would also be sufficient to insert self.nodes[0].generate(1) at line 36 below, as that would cause the nodes to leave IBD.

I don’t feel strongly about which workaround is better (mocktime or generating a block to leave IBD), just mentioning it in case anyone else has an opinion.

sdaftuar commented at 11:55 am on December 10, 2015: member

@ptschip Thanks for incorporating the feedback and updating the PR. I left a few comments on the individual tests, I think several don’t need to be updated (looks to me that of the tests you changed, just listtransactions.py and receivedby.py require a fix, because the first thing they do is send a transaction).

ptschip force-pushed on Dec 10, 2015

ptschip commented at 6:55 pm on December 10, 2015: contributor

@sdaftuar You were right, thanks for doing a thorough job of testing. I dont remember why I put those changes in there, probably when I was reverting to opt in, at any rate it’s pushed and travis is finished. It looks much cleaner and with minimal code changes.

On 10/12/2015 3:56 AM, Suhas Daftuar wrote:

@ptschip https://github.com/ptschip Thanks for incorporating the feedback and updating the PR. I left a few comments on the individual tests, I think several don’t need to be updated (looks to me that of the tests you changed, just |listtransactions.py| and |receivedby.py| require a fix, because the first thing they do is send a transaction).

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

ptschip force-pushed on Dec 11, 2015

sdaftuar commented at 6:47 pm on December 11, 2015: member

ACK a06f106c8a942b0b2a2aeaa3d39d673feb432c69

dcousens commented at 0:30 am on December 14, 2015: contributor

ACK a06f106

laanwj commented at 12:11 pm on December 14, 2015: member

Needs rebase

laanwj referenced this in commit 64360f1304 on Dec 14, 2015

ptschip force-pushed on Dec 14, 2015

ptschip commented at 2:27 pm on December 14, 2015: contributor

Rebase Done.

On 14/12/2015 4:12 AM, Wladimir J. van der Laan wrote:

Needs rebase

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

MarcoFalke commented at 2:47 pm on December 14, 2015: member

utACK cee6674

Do not download transactions during inital sync 39a525c21f

in src/main.cpp: in cee667418f outdated

5653@@ -5654,24 +5654,26 @@ bool SendMessages(CNode* pto)
5654         //
5655         // Message: getdata (non-blocks)
5656         //
5657-        while (!pto->fDisconnect && !pto->mapAskFor.empty() && (*pto->mapAskFor.begin()).first <= nNow)
5658+        if(!IsInitialBlockDownload())

sdaftuar commented at 4:31 pm on December 14, 2015:

Seems like we don’t actually need this guard here, do we?

jtimon commented at 7:53 pm on December 14, 2015:

What do you mean? adding this condition is the whol point of the PR, isn’t it?

sdaftuar commented at 7:55 pm on December 14, 2015:

I think the guard on line 4566 is sufficient, from my reading of the code and a quick test. Is there a case I’m missing?

ptschip commented at 3:25 pm on December 15, 2015:

Yes, you’re right. I made the change and tested it out.

I pushed it, if you want to take a look before I squash.

On 14/12/2015 11:56 AM, Suhas Daftuar wrote:

In src/main.cpp #7164 (review):
@@ -5654,24 +5654,26 @@ bool SendMessages(CNode* pto) // // Message: getdata (non-blocks) //
   while (!pto->fDisconnect && !pto->mapAskFor.empty() && (*pto->mapAskFor.begin()).first <= nNow)
   if(!IsInitialBlockDownload())
I think the guard on line 4566 is sufficient, from my reading of the code and a quick test. Is there a case I’m missing?

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

sdaftuar commented at 3:37 pm on December 15, 2015:

Looks good, feel free to squash

ptschip commented at 3:44 pm on December 15, 2015:

Squashed.

Well there you go, just one line of code :)

On 15/12/2015 7:38 AM, Suhas Daftuar wrote:

In src/main.cpp #7164 (review):
@@ -5654,24 +5654,26 @@ bool SendMessages(CNode* pto) // // Message: getdata (non-blocks) //
   while (!pto->fDisconnect && !pto->mapAskFor.empty() && (*pto->mapAskFor.begin()).first <= nNow)
   if(!IsInitialBlockDownload())
Looks good, feel free to squash

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

ptschip force-pushed on Dec 15, 2015

sdaftuar commented at 4:01 pm on December 15, 2015: member

ACK 39a525c21fd1b34df63ab30868423b97b708ee49

jtimon commented at 6:53 pm on December 15, 2015: contributor

re-utACK 39a525c21fd1b34df63ab30868423b97b708ee49

ptschip commented at 0:43 am on December 18, 2015: contributor

@laanwj I think this should be ready to be merged.

dcousens commented at 0:36 am on January 15, 2016: contributor

@laanwj any reason not to merge? re-tested ACK 39a525c

laanwj merged this on Jan 19, 2016

laanwj closed this on Jan 19, 2016

laanwj referenced this in commit 3b43cad9d0 on Jan 19, 2016

laanwj added the label Needs backport on Feb 11, 2016

ptschip deleted the branch on Apr 5, 2016

MarcoFalke referenced this in commit 797036bd83 on Apr 25, 2016

MarcoFalke referenced this in commit 90955940d5 on Apr 27, 2016

MarcoFalke commented at 10:55 am on June 9, 2016: member

Backported as part of #7938. Removing label ‘Needs backport’.

MarcoFalke removed the label Needs backport on Jun 9, 2016

thokon00 referenced this in commit 28bd4753a7 on Jun 28, 2016

nomnombtc referenced this in commit f78a69476e on Nov 12, 2016

nomnombtc referenced this in commit 22e7c02db5 on Nov 12, 2016

nomnombtc referenced this in commit b712652094 on Nov 13, 2016

sickpig referenced this in commit 5edccdeacf on Nov 14, 2016

deadalnix referenced this in commit d9ec1a7db1 on Jan 11, 2017

deadalnix referenced this in commit 27d238fdb1 on Jan 11, 2017

deadalnix referenced this in commit a193ba35df on Jan 15, 2017

deadalnix referenced this in commit 890e1c1dda on Jan 16, 2017

deadalnix referenced this in commit 398d02958b on Jan 17, 2017

deadalnix referenced this in commit 3015c86761 on Jan 19, 2017

deadalnix referenced this in commit e442224c4b on Jan 19, 2017

deadalnix referenced this in commit 36cf0f2266 on Jan 19, 2017

deadalnix referenced this in commit 1164b55292 on Jan 26, 2017

deadalnix referenced this in commit 85a832d6a6 on Feb 12, 2017

sickpig referenced this in commit 52db63687f on Feb 25, 2017

deadalnix referenced this in commit c2a143452c on Feb 27, 2017

deadalnix referenced this in commit fa68b4bcd3 on Feb 27, 2017

zkbot referenced this in commit 8c000ae281 on Jan 6, 2021

DrahtBot locked this on Sep 8, 2021

Do not download transactions during initial blockchain sync #7164