Duplicate getheaders requests #6755

During the initial headers download, a client may start to request the same headers multiple times from its syncing peer. This happens if a new block is found while the peer is syncing, for the following reason:

For the ordinary headers syncing procedure, the client starts by asking for an initial set of headers, and then whenever it receives a message with 2,000 headers, it follows up with a request for more. However, if a new block is found, its peer sends a corresponding inv message, which prompts the syncing client to also ask for the headers preceding the new block. This requests the same headers again, and when they are received a second time, it continues requesting more. Depending on how many new blocks are found and how long the header syncing lasts, this may lead to a situation where the same set of headers is requested many times.

This obviously puts unnecessary strain on both the client that syncs and the peer that sends the data. This is a problem particularly for altcoins with either of a high block frequency or merge-mining (much larger header messages), but it happens with Bitcoin itself as well.

To fix it, we could keep (in-memory only) for each peer the highest "from" height for which we requested headers from it. Whenever a new request would be sent with a smaller number, we drop it instead. Alternatively, one could probably also make the logic for requesting headers following an inv message smarter, so that it does not request when we are in the initial headers sync. What do you think? I'm willing to implement a patch for whatever solution is suggested.

Here's a more concrete proposal: We already have fSyncStarted in the node state of main.cpp. This is set to true when we start requesting headers from a particular node, so we can use it to not request headers from this node also following an inv message. In addition, we have to reset the flag to false once we receive a headers message that does not have the maximum size - currently, it is never reset.

Are there any problems with this approach or is it ok? This is straight-forward to implement and fixes, as far as my preliminary testing goes, the issue.

My proposed patch is at #6821. I tried various things to avoid the issue and in the end, the approach implemented in the patch seemed to be the best to me.

@domob1812 I've just read through this issue and the 2 PRs you made and I've also seen that #8054 was later reverted again. Did you do any further investigation into this issue after the revert?

I'm currently facing the problem of duplicate headers download while working on Dash. The amount of received data while doing the initial headers-only sync is approx. 10 times as much as I would expect, resulting in a pretty bad delay until actual block download starts. My debugging sessions have shown that there are alot of duplicate headers received, so it must be this same issue.

@codablock No, I've not spent any more effort on this after my commit got reverted. For Huntercoin, where this issue was most severe in the projects I'm interested in, I just didn't revert the upstream commit and it works fine for that purpose so far.

But if you have any ideas for how to properly fix the issue or would like to give it a shot, I'd still be very happy to have this fixed properly in upstream Bitcoin.

I found 76f74811c44b5119d5c19364b0594d423248ac0e which was merged in the mean time. It adds a timeout to headers sync. As I understood your first attempt (https://github.com/bitcoin/bitcoin/pull/6821) to fix the duplicate headers issues, it was rejected because at that time, a node did not get another chance to do a full sync in case he got a bad node for the initial sync. Now that timeouts are implemented for headers sync, shouldn't your first PR be fine today?

Linking the comment on #8306 from @DeckerSU here for those interested in continuing the discussion on this issue.

Linking comment on #6861 with a possible idea on how to mitigate.

The problem with multiple getheaders with the same peer was recently fixed by #25454, so maybe this can be closed now?

It is still possible to do multiple getheaders with different peers if a block is found during headerssync, and this is desirable to some degree (the original peer might be very slow or on an old chain), but the phenomenon where we'd start syncing headers with all of our peers when a block is found will be mitigated by #25720 - but If I understand this issue correctly, it's not really about that anyway.