Fix double BlockRequest after announcement

[gnunicorn]

While investigating the CPU spikes at import, I noticed that we are requesting a block twice after it as announced. Turns out that we are tracking the queue_best_number twice, once in the importqueue itself and once prior to that and because of the order in items are called, they are not in sync at that particular moment after we have received the latest block and before the handover to the importqueue - but exactly at that moment do we check whether we should download further blocks against the importqueue, which doesn't yet know of the new block. And we happen to request the same block again - but only twice because right after the request we successfully import the block and the next response is actually ignored.

The fix is simple: we need to check the local value not the remote value. This PR does that and the logs show no double imports or requests anymore.

[svyatonik]

iirc, initially the reason behind two counters was:

• the counter#1 in import_queue.rs was about max block that is currently scheduled for import;
• the counter#2 in sync.rs was about max block that is already imported.
  So when block was imported by the ImportQueue, counter#2 was increased. I guess this isn't true now (after [frontport] Fixed sync stalling when import queue is full #691) && counters must be merged (not a part of this PR).

That said, if I understand correctly, the whole ::std::cmp::max(...) could be replaced back with the peer.common_number, since peer.common_number should always be lte than self.best_queued_number (best block# that we have ever seen).

[gnunicorn]

Well, this problem occurs now because, because of other constrains we had to move import_queue.import_blocks(self, protocol, (origin, new_blocks)); out of that function call and it now only takes places after maintain_sync (and subsequently download_now) is being called. Therefore, arguably a different fix could also be to move maintain_sync only into the call for block_imported (which also makes more sense to me, as something actually happened). But I am not sure whether that has unwanted consequences...

[rphmeier]

cc @arkpar

[svyatonik]

IMO maintain_sync is called where it should be (but probably better ask @arkpar about that). I'm happy with the fix, but still suggest you to remove the changed line at all, because peer.common_number should always be <= import_status.best_importing_number. Feel free to merge as-is if you're unsure.

[arkpar]

Looks correct to me

[rphmeier]

This seems to have broken syncing with fork and ancestry search tests.

[gnunicorn]

Yeah, I noticed at least test is failing with this, so we might have to find a different approach after all..

[arkpar]

Superseded by #1235