Is the InitialBlockDownload() check in ‘getheaders’ too strict? #6971

1. 
   laanwj commented at 9:39 am on November 9, 2015: member

   I was trying to sync a new node from an almost-synced node, and somehow it was ignoring the other node.

   Some debugging traced the problem to: https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L4307

   I understand the rationale, of course, it makes no sense to share blocks if we’re at the beginning of the chain. But we define ‘initial block download’ as ‘hasn’t received a block in the last 24 hours’. So it is also on while catching up.

   Nodes which are almost-synced could in very well partake in serving old blocks, or am I missing something?

2. laanwj added the label P2P on Nov 9, 2015
3. laanwj added the label Priority Low on Nov 9, 2015
4. 
   laanwj commented at 9:42 am on November 9, 2015: member

   Another option, which just solves my immediate problem, would be to override this check for whitelisted peers.
5. laanwj referenced this in commit 40b77d450d on Nov 9, 2015
6. 
   sdaftuar commented at 2:34 pm on November 9, 2015: member

   The motivation for this restriction was to avoid responding to a getheaders when your chain might deviate from the checkpointed chain, because your peers that have the checkpointed chain will disconnect you. (After changing the download logic to call getheaders on inbound peers, I wanted to avoid a situation where a newly syncing node could be partitioned from the network by a malicious peer that served it a bogus headers chain.)

   This check is more restrictive than is needed for that – I think I did it this way because I didn’t want to introduce yet another function for determining “synced-ness” (or a new place in the code that uses checkpoints). Also, I think block relay only happens if you’re out of InitialBlockDownload, so it didn’t seem unreasonable to use it here as well, but we could loosen things up if we wanted to.

   I assume the use case you’re going for is to have two nodes be able to sync even if disconnected from the rest of the network? If so the whitelisting approach seems reasonable to me. As a minor nit, I think the whitelisting test gets the property backwards – really what you’d want is to eliminate the restriction if your peer has whitelisted you, rather than the other way around, since then you could respond without fear of being disconnected – but since it’s likely both nodes are under common control anyway, it seems a minor issue.

   Will review the PR.

7. 
   laanwj commented at 2:51 pm on November 9, 2015: member

   I assume the use case you’re going for is to have two nodes be able to sync even if disconnected from the rest of the network?

   Right. I have a node A that isn’t catched up fully, and node B that I want to sync from node A. B only connects to A. A is currently not connected to the internet, to make sure B gets all available I/O bandwidth.

8. 
   laanwj commented at 9:31 am on November 10, 2015: member

   Apart from my own issue, I’m also somewhat concerned that this check will reduce the number of nodes that are willing to serve blocks. But that is probably misguided. Any node that usefully contributes to the network will be synced up to within 24 hours from the tip, I suppose…

   So - other nodes will disconnect you if you give headers that deviate from the checkpointed chain? Isn’t that wrong behavior? Should we enforce the checkpoints that strictly? (is it always, or usually, an attack when this happens?)

9. 
   sdaftuar commented at 4:09 pm on November 10, 2015: member

   Yeah, I can’t think of a common scenario where you’d start up a node and find an outbound peer that wasn’t already close to caught up, and therefore unable to sync. But if you wanted to loosen this up somewhat I think it’d be fine to do so.

   Regarding the checkpoints, yes presently once we have the checkpointed headers, we will fail (see CheckIndexAgainstCheckpoint, called from AcceptBlockHeader) when trying to validate a header that forks prior to the last checkpoint, and we assign DoS points.

   I think we’re in the process of slowly loosening this up (eg with #5927 which will be new for 0.12), but I don’t have a clear picture of the roadmap for doing so. I do think that if you get invalid headers before the checkpointed chain (block height 295000 is the last checkpoint) that the node is either trying to attack you, or – maybe more likely – it has been attacked itself. It’s this latter point that worries me somewhat about not merging some kind of fix for 0.12 to prevent an attacker from partitioning the network now that #5927 is in (see #5927 (comment)).

10. laanwj closed this on Nov 11, 2015

    ---

11. 
    luke-jr commented at 7:30 am on November 15, 2015: member

    This also breaks rpc-tests/script_test.py, but is “worked around” by the whitelisted hack. IMO, that’s pretty ugly. :(
12. luke-jr referenced this in commit 8766acdb0d on Nov 18, 2015
13. 
    rebroad commented at 10:17 am on October 12, 2016: contributor

    I think this issue should be re-opened, as it’s still an issue for non-whitelisted peers. Why not make it only return if the active tip is below the last checkpoint?
14. rebroad referenced this in commit 22a4a4fd5c on Nov 2, 2016
15. rebroad referenced this in commit f5b82fe0bb on Nov 2, 2016
16. rebroad referenced this in commit f032d43aa0 on Nov 2, 2016
17. rebroad referenced this in commit 7f68d44c58 on Nov 2, 2016
18. rebroad referenced this in commit 83d40a910f on Nov 2, 2016
19. rebroad referenced this in commit cc61ff61ea on Nov 4, 2016
20. rebroad referenced this in commit e58dcdd7ce on Nov 5, 2016
21. rebroad referenced this in commit 6c0f1ba60f on Nov 5, 2016
22. rebroad referenced this in commit 0ec255a812 on Nov 6, 2016
23. rebroad referenced this in commit 65a43abf3a on Nov 6, 2016
24. rebroad referenced this in commit ec7fac9a23 on Nov 6, 2016
25. DrahtBot locked this on Sep 8, 2021

Contributors
laanwj sdaftuar luke-jr rebroad

Labels
P2P