Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Drain blocks on peer disconnect #8553

Merged
merged 4 commits into from
Apr 7, 2021
Merged

Drain blocks on peer disconnect #8553

merged 4 commits into from
Apr 7, 2021

Conversation

arkpar
Copy link
Member

@arkpar arkpar commented Apr 7, 2021

Two issues fixed here:

  1. After Update common block in sync after importing blocks of a peer, please read UPDATE #7733 peers may enter common block/ancestry search mode when common block is detected to be too far behind. However this should not be happening when major sync is in progress. I've added a check for that.
  2. Peers starting common block search in the middle of the major sync resulted in common block being a bit behind of the import queue, which leads to duplicate requests and bans.
  3. Since we only drain downloaded blocks on new block data, a canceled duplicate request at the front of the download range may result in stalemate situation, where we don't issue new download requests because the block container is already full and we don't drain the block container because there are no incoming blocks. To fix that I've added a check for blocks that may be imported after a peer disconnects and their download request is cleared.

@arkpar arkpar requested review from bkchr and andresilva April 7, 2021 10:28
@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Apr 7, 2021
@arkpar arkpar requested a review from tomaka April 7, 2021 10:29
@arkpar arkpar added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Apr 7, 2021
Copy link
Contributor

@tomaka tomaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not an expert in the sync code, but looks good to me

client/network/src/protocol/sync.rs Outdated Show resolved Hide resolved
@@ -1542,11 +1557,33 @@ impl<B: BlockT> ChainSync<B> {
}

/// Call when a peer has disconnected.
pub fn peer_disconnected(&mut self, who: &PeerId) {
/// Canceled block request may result in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfinished comment.

@bkchr
Copy link
Member

bkchr commented Apr 7, 2021

Does this means we are waiting until all blocks are downloaded, even if we have send the same request multiple times to different peers?

No, in case of parallel requests, as soon as we download a block all other requests are made obsolete. Normally when doing major sync, we don't issue parallel requests though. Duplicate requests come from issues 1 and 2. We try to download blocks that are already queued, and while the request was active, import queue made some progress. So a call to drain would discard the block that are already imported.

arkpar and others added 3 commits April 7, 2021 14:10
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
@arkpar arkpar merged commit f296656 into master Apr 7, 2021
@arkpar arkpar deleted the a-fix-sync3 branch April 7, 2021 12:43
hirschenberger pushed a commit to hirschenberger/substrate that referenced this pull request Apr 14, 2021
* Drain blocks on peer disconnect

* Finish comment

* Fixed test

* Update client/network/src/protocol/sync.rs

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
jordy25519 pushed a commit to cennznet/substrate that referenced this pull request Sep 17, 2021
* Drain blocks on peer disconnect

* Finish comment

* Fixed test

* Update client/network/src/protocol/sync.rs

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants