Description
Currently, when a node sends a HeaderListRequest to the peer, it always creates a Locator based on the current main chain tip. The peer then responds with a list of block headers up to msg_header_count_limit in size; after that the node downloads all those blocks and then makes another HeaderListRequest. The issue here is that in theory adding those blocks to chainstate may not change the main chain tip (if the node and the peer have diverged more that msg_header_count_limit blocks ago), so on the second iteration the node will create exactly the same locator as it did initially, the peer will respond with exactly the same header list, and this loop may go on forever.
At this moment such a situation is only hypothetical, because msg_header_count_limit (2000) is bigger than the max reorg depth (1000). So, any stale-chain block deeper than msg_header_count_limit will be rejected by chainstate and the diverged peer will eventually be banned.
But:
- Technically, we can decide to change the limits in the future.
- Even if they stay the same, it's not nice for p2p to assume any particular behavior of chainstate (i.e. the presence of a reorg depth limit or the fact that chainstate will produce an error with a non-zero ban score if such a block is received).
So, it'd be better to construct locators for subsequent HeaderListRequests taking into account the previously received stale-chain blocks. Or at least to detect the lack of progress in syncing and ban such a peer explicitly.