Revert "net: Avoid duplicate getheaders requests." PR #8054 #8306

This reverts commit f93c2a1b7ee912f0651ebb4c8a5eca220e434f4a.

This can cause synchronization to get stuck.

I observed a testnet node persistently stuck (even across restarts) in a case where its best header chain was invalid and the public network has a best valid chain with much more work than the invalid best header chain that I have, but it took more than one headers message to connect it.

Suhas identified this PR as the probable cause, and on revert the node immediately became unstuck. Considering how near we are to release, I think simply reverting this is the right action currently. The issue it was fixing should have been rare and largely inconsequential on the Bitcoin network.

utACK. We should tag this for 0.13.0.

I ran into this problem, this fixed it.

I've run into at least two people on IRC with this issue (in addition to Patrick).

Even if the issue my original patch fixed is rare in practice, I would like to see it fixed. I understand that it is best to roll the change back after finding the issue now before the upcoming release, though. Can I open an issue to track a "fixed fix" for 0.14? Also, I do not yet fully understand how a node could become stuck with the patch even if it is on an invalid chain. Does anyone have a good explanation for what the issue is exactly?

@domob1812 One example: before reverting this patch, if there are two competing forks with tips A and B, and a node is at tip A and the fork point C between A and B is more than 2000 blocks in the past, and a node already has the first 2000 headers from C to B but no later ones, then it's possible that the hasHeaders check added by #8054 would prevent the node from ever learning about tip B, causing chain sync to fail.

Thanks @sdaftuar, makes sense. I'll think about it.

@sdaftuar i know it's been a long time since initial patch, but how do you think, if we limit initial @domob1812 fix only to IBD, like:

bool hasNewHeaders = true;
if (IsInitialBlockDownload()) {
    hasNewHeaders = (mapBlockIndex.count(headers.back().GetHash()) == 0);
}
...
if (nCount == MAX_HEADERS_RESULTS && pindexLast && hasNewHeaders) {
...
    pfrom->PushMessage("getheaders", chainActive.GetLocator(pindexLast), uint256());
}

Will it cause sync stuck case described here #8306 (comment) ?

I'm trying to solve duplicate getheaders requests issue in ZCash and Komodo, bcz in these chains duplicate getheaders requests causes really huge overhead (additional traffic download), bcz of bigger blockheader size (1488 size). Just an example:

2020-04-16 02:03:29 more getheaders (1641598) to end to peer=1 (startheight:1836680)
2020-04-16 02:03:29 sending: getheaders (1061 bytes) peer=1
2020-04-16 02:03:29 received: headers (238081 bytes) peer=2
2020-04-16 02:03:29 more getheaders (1641598) to end to peer=2 (startheight:1836680)
2020-04-16 02:03:29 sending: getheaders (1061 bytes) peer=2
2020-04-16 02:03:29 sending: getheaders (1061 bytes) peer=3
2020-04-16 02:03:29 getheaders (1641598) 08685ebc864678e16e9094fafce2119a9c63aae7872dc6fcc974f29cfc201e1e to peer=3
...
2020-04-16 02:03:29 more getheaders (1641598) to end to peer=5 (startheight:1836680)
2020-04-16 02:03:29 sending: getheaders (1061 bytes) peer=5
2020-04-16 02:03:29 received: headers (238081 bytes) peer=5

As we are see here same getheaders request sent to peer 1, 2 and 5. And we got 3 replies (MAX_HEADERS_RESULTS x size(CBlockHeader) = 160 x 1488 = 238081 bytes each). With ~1.8M blocks in chain it can cause even ~500 Gb of download traffic during IBD and initial headers sync stage, while the size of entire blockchain is only ~25-30 Gb. I did small test-lab experiment with syncing from scratch from 2 peers in local 1 Gbps network - it downloads 1 Gb of duplicate headers just in few minutes.

So, any advice is appreciated.

p.s. Limiting initial fix with IsInitialBlockDownload() seems working, trafic download during initial headers sync reducing significantly, but I just want to make sure that there are no any pitfalls.