net: Avoid duplicate getheaders requests. #8054

The current logic for syncing headers may lead to lots of duplicate getheaders requests being sent: If a new block arrives while the node is in headers sync, it will send getheaders in response to the block announcement. When the headers arrive, the message will be of maximum size and so a follow-up request will be sent---all of that in addition to the existing headers syncing. This will create a second "chain" of getheaders requests. If more blocks arrive, this may even lead to arbitrarily many parallel chains of redundant requests. (See #6755.)

This patch changes the behaviour to only request more headers after a maximum-sized message when it contained at least one unknown header. This avoids sustaining parallel chains of redundant requests.

Note that this patch avoids the issues raised in the discussion of #6821: There is no risk of the node being permanently blocked. At the latest when a new block arrives this will trigger a new getheaders request and restart syncing.

Concept ACK

This is equivalent to just testing whether the last block is already known, by the way.

Oh yes, of course - nice catch! I will update the patch accordingly.

I've now updated the patch - which further simplifies it. When the message contains no previously unknown headers (i. e., the last header is already known), simply ignore it.

utACK f93c2a1

utACK f93c2a1b7ee912f0651ebb4c8a5eca220e434f4a

utACK f93c2a1b7ee912f0651ebb4c8a5eca220e434f4a

utACK f93c2a1b7ee912f0651ebb4c8a5eca220e434f4a

Looks much better -- thanks for coming up with an alternate solution to this.

this clearly got merged with insufficient testing - I guess the testing policy is "merge first test later"?

No amount of simple testing would turn up the problems with this; if you'd like to argue that we need better test harnesses... I'd agree. Patches accepted!

@rebroad You are welcome to help with testing: https://github.com/bitcoin/bitcoin#testing

Also note, that master is not considered stable. Pull request get merged after they received some review, but it is impossible to detect all issues which might arise in the future. Whenever a pull received enough review, merging it into master is a good way to expose the patch to a larger group of developers and get feedback about tricky issues which might arise in very rare and special circumstances.

Agree with @MarcoFalke. If you can't handle things breaking here and there momentarily, you certainly shouldn't be using the master branch. These things happen, and posting here complaining months after the fact is just sad.

@laanwj Thanks for the clarification. I think it could be made clearer somewhere that the master branch is largely untested - I had assumed that some effort was made to test things before they were merged - it certainly used to be the case.

Of course things are tested before merge. But we can't test everything. That is why release candidates and releases exist: to give external people a chance to test code before we label it ready to be used by everyone.