Fix additional edge cases in fast sync process

[liuchengxu]

During local testing for issue #5406, I encountered additional edge cases that required fixing. These should be the final adjustments before I submit part 2 at #5406 (comment).

While the code changes themselves are relatively minor, the rationale behind them is more complex and involves careful handling of specific sync scenarios. I recommend going through the PR commit by commit. Each commit includes detailed explanations in the messages to provide context for the necessity of these changes. cc @dmitry-markin

[lexnv]

LGTM! The changes are subtle indeed, I'll feel more comfortable once Dmitry has another look at this

Thanks for contributing 🙏

[dmitry-markin]

What if peer.best_number > best_number? The fix won't work and the duplicate block request will be issued?

[liuchengxu]

Good point. The fix is incomplete in this case, the block requests would include both the duplicate and new blocks, so the gap sync will still be interrupted unexpectedly. I'll need some more time to think about it.

[liuchengxu]

I'm running out of ideas on how to completely avoid duplicate block requests. The challenge lies in how duplicate block processing interferes with the gap sync state.

Why duplicate blocks pollute the gap sync state

1. Block request confusion: The main issue in the current chain sync is that multiple types of block requests are made, but the handler can't distinguish between them. As a result, blocks requested from one type of request can interfere with another sync component. For example, when duplicate blocks in the range [finalized_number, best_number] are requested via peer_block_request(), once these blocks are received and enqueued, gap_sync.best_queued_number is updated to best_number.
   

   polkadot-sdk/substrate/client/network/sync/src/strategy/chain_sync.rs

   Lines 1295 to 1299 in 03f6e42

   	if let Some(gap_sync) = &mut self.gap_sync {
   	if number > gap_sync.best_queued_number && number <= gap_sync.target {
   	gap_sync.best_queued_number = number;
   	}
   	}

   
   This causes a problem because no further blocks will be requested from peer_gap_block_request() as gap_sync.best_queued_number == peer.best_number, leading to a failure in downloading the block history.

2. Impact on gap_sync_complete detection: The detection of whether gap_sync is complete can be affected by importing duplicate blocks, as I explained in the second commit message.

The second point can be updated to let gap_sync_complete = self.gap_sync.is_some() && self.client.info().block_gap.is_none(); which is more reliable too. However, I still haven’t found a solution for the issue with gap_sync.best_queued_number outlined in the first point.

To ensure gap sync functions as expected, I suggest detecting whether a block response contains duplicate blocks in gap sync and preventing those duplicates from being processed further. This would prevent pollution of the gap sync state and allow block history to be downloaded correctly. Thoughts? @dmitry-markin