Be even stricter in processing unrequested blocks #6224

1. 
   sdaftuar commented at 2:04 am on June 3, 2015: member

   As reported in issue #6184, pruning doesn’t currently work well during initial block download if connected to a peer (like the relay client) which can deliver unrequested blocks at the tip before the node is synced. When that happens, blocks with a very large height are written to early block files, thus preventing pruning from taking place on those early files (due to the restriction against deleting block files containing blocks with a height greater than chainActive.Tip() - 288). #6221 attempts to deal with this better by allowing block files to at least not be contiguous, so that we can still prune later files even if an earlier one ends up containing a large-height block. However, even with that behavior change, during the several hours that initial download could take, it’s possible for dozens of block files to end up containing a block that is relatively recent, potentially causing the pruning target to be exceeded substantially for 2 days (~288 blocks) after starting up.

   This pull goes further to improve behavior when connected to a peer that is serving unrequested blocks. The first commit tries to prevent block files from becoming too out-of-order by not processing an unrequested block if its height is more than 288 blocks past our tip.

   • Regarding the choice of constant: there were two natural choices to consider, one was the constant used by pruning (288 == MIN_BLOCKS_TO_KEEP), the other is the BLOCK_DOWNLOAD_WINDOW (1024) which is used for a similar purpose when doing parallel fetching of blocks. The smaller MIN_BLOCKS_TO_KEEP value means that we’d be more likely to achieve our pruning target, and it seemed reasonable to me that we’d use it similarly for managing out-of-order-ness in block files to make pruning behave better. But I’m open to other suggestions for this value.
   • While this change was motivated by pruning nodes, for simplicity’s sake I didn’t restrict this behavior change to pruning nodes.

   The second commit changes the behavior when receiving unrequested blocks from whitelisted peers, so that those blocks are not automatically processed during initial block download. Instead, during IBD they are subject to the same conditions as when received from non-whitelisted peers.

   • This change is to accommodate relay network users who whitelist the relay peer; without the second commit the first commit would have no effect. Alternatively, if it’s reasonable for users to not whitelist the relay peer, then perhaps we could drop this commit altogether.
   • Before settling on using IsInitialBlockDownload(), I considered using the guard that prevents direct-fetching of inv’ed blocks, but that test was in my opinion worse: chainActive.Tip()->GetBlockTime() > GetAdjustedTime() - chainparams.GetConsensus().nPowTargetSpacing * 20

   If this pull looks okay, I think it should be included in 0.11.

2. sdaftuar force-pushed on Jun 3, 2015
3. laanwj added the label P2P on Jun 3, 2015
4. Ignore unrequested blocks too far ahead of tip bfc30b3437
5. sdaftuar force-pushed on Jun 3, 2015
6. sdaftuar force-pushed on Jun 3, 2015
7. 
   sdaftuar commented at 8:57 pm on June 3, 2015: member

   Rebased on top of master now that #5875 has been merged.
8. 
   sipa commented at 2:00 pm on June 14, 2015: member

   Concept ACK
9. 
   sipa commented at 1:22 pm on June 21, 2015: member

   Been running on bitcoin.sipa.be for a week, no problems.
10. 
    morcos commented at 8:06 pm on June 25, 2015: member

    utACK
11. in src/main.cpp: in 543f69dc34 outdated

    4477@@ -4471,8 +4478,12 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
    4478         pfrom->AddInventoryKnown(inv);
    4479 
    4480         CValidationState state;
    4481-        // Process all blocks from whitelisted peers, even if not requested.
    4482-        ProcessNewBlock(state, pfrom, &block, pfrom->fWhitelisted, NULL);
    4483+        // Process all blocks from whitelisted peers, even if not requested,
    4484+        // unless we're far still syncing with the network.
    

    ---

    ---

    

    petertodd commented at 2:18 am on June 30, 2015:

    s/far//
12. in src/main.cpp: in 543f69dc34 outdated

    2856@@ -2851,6 +2857,7 @@ bool AcceptBlock(CBlock& block, CValidationState& state, CBlockIndex** ppindex,
    2857     if (!fRequested) {  // If we didn't ask for it:
    2858         if (pindex->nTx != 0) return true;  // This is a previously-processed block that was pruned
    2859         if (!fHasMoreWork) return true;     // Don't process less-work chains
    2860+        if (fTooFarAhead) return true;      // Block height is too high
    

    ---

    ---

    

    petertodd commented at 2:21 am on June 30, 2015:

    Really odd how we have this flag-based coding style here; I don’t see why we can’t just put the various tests in this if block.

    That said, you might as well follow the style right now and fix it up later in another cleanup pull-req.

13. 
    petertodd commented at 2:22 am on June 30, 2015: contributor

    Aside from the extra word nit, mildly-tested ACK.
14. Ignore whitelisting during IBD for unrequested blocks. 6b1066fab4
15. Replace sleep with syncing using pings 04b5d235f1
16. sdaftuar force-pushed on Jun 30, 2015
17. 
    sdaftuar commented at 1:44 pm on June 30, 2015: member

    @petertodd Thanks for reviewing; I’ve fixed the comment and updated this PR.
18. 
    petertodd commented at 2:13 pm on June 30, 2015: contributor

    Looks good now, ACK.
19. 
    sipa commented at 1:44 pm on July 9, 2015: member

    @laanwj Any chance for getting this in 0.11? It should only have an effect when using both pruning and having a peer that sends unrequested blocks (such as @TheBlueMatt’s relay client) - a use case that is currently broken in 0.11.
20. in src/main.cpp: in 04b5d235f1 outdated

    2848+    // Blocks that are too out-of-order needlessly limit the effectiveness of
    2849+    // pruning, because pruning will not delete block files that contain any
    2850+    // blocks which are too close in height to the tip.  Apply this test
    2851+    // regardless of whether pruning is enabled; it should generally be safe to
    2852+    // not process unrequested blocks.
    2853+    bool fTooFarAhead = (pindex->nHeight - chainActive.Height()) > MIN_BLOCKS_TO_KEEP;
    

    ---

    ---

    

    luke-jr commented at 11:14 pm on July 10, 2015:

    warning: comparison between signed and unsigned integer expressions
21. sdaftuar force-pushed on Jul 11, 2015
22. Eliminate signed/unsigned comparison warning 59b49cd074
23. sdaftuar force-pushed on Jul 11, 2015
24. 
    sdaftuar commented at 11:12 am on July 11, 2015: member

    @luke-jr Thanks for catching the comparison issue; fix pushed.
25. 
    laanwj commented at 5:58 pm on July 29, 2015: member

    ACK- nice tests, too
26. laanwj merged this on Jul 29, 2015
27. laanwj closed this on Jul 29, 2015

    ---

28. laanwj referenced this in commit 675d2feffa on Jul 29, 2015
29. laanwj referenced this in commit 93b606aee4 on Jul 29, 2015
30. 
    laanwj commented at 6:34 pm on July 29, 2015: member

    Backported to 0.11 as 93b606aee41c3ff37980e73e8dc1c6c06f3b11f9
31. 
    dcousens commented at 5:54 am on July 31, 2015: contributor

    Out of interest, why do we allow processing of blocks higher than chainActive.Height() + 1 anyway? If the idea is that we can synchronise faster because we don’t wait for the blocks in order, then why not just put them in a temporary cache and process them in order anyway? If they drop out of that cache as being stale, then, all is well?
32. MarcoFalke locked this on Sep 8, 2021

Contributors
sdaftuar sipa morcos petertodd luke-jr laanwj dcousens

Labels
P2P