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

Fixed sync target detection#11817

Merged
paritytech-processbot[bot] merged 5 commits into
masterfrom
a-sync-best-block-metric
Jul 15, 2022
Merged

Fixed sync target detection#11817
paritytech-processbot[bot] merged 5 commits into
masterfrom
a-sync-best-block-metric

Conversation

@arkpar

@arkpar arkpar commented Jul 11, 2022

Copy link
Copy Markdown
Member

This fixes sync target detection. With the current mean selection method it is possible for this function to return a block that's lower than our best block. This PR changes the mean selection to only consider peers that are ahead.

See #11547

Fixes https://github.com/paritytech/devops/issues/1767

@arkpar arkpar requested review from bkchr and cheme July 11, 2022 20:12
@arkpar arkpar added 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. labels Jul 11, 2022

@cheme cheme left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh I thought #11705 was merged. So this PR does the same with best_queued_number instead of actual best. Sounds correct.

@nazar-pc

Copy link
Copy Markdown
Contributor

But queued blocks aren't guaranteed to import successfully, I'm wondering if #11705 that checks just the block successfully imported is a better alternative.

And yeah, #11705 was open for over 3 weeks now 😞

@arkpar

arkpar commented Jul 12, 2022

Copy link
Copy Markdown
Member Author

I also forgot #11705 exists. From sync POV all blocks added to the import queue are considered to be "synced". If there's a error with a block import at later stage, the sync process is reset. So I think using best_queued_number is better here for consistency.

@chevdor

chevdor commented Jul 13, 2022

Copy link
Copy Markdown
Contributor

related issue: paritytech/polkadot#5771

Comment thread client/network/sync/src/lib.rs Outdated
.peers
.values()
.filter_map(|p| {
if p.best_number > self.best_queued_number {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Doesn't this again gives another node the possibility to force you into major sync mode?

We should move this to the end of the function and do a max() with between the best number and the selected number in the range.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Or rather return None if the median is in the past.

@arkpar

arkpar commented Jul 15, 2022

Copy link
Copy Markdown
Member Author

bot merge

@paritytech-processbot

Copy link
Copy Markdown

This PR cannot be merged at the moment due to: 5 of 12 required status checks are expected.

processbot expects that the problem will be solved automatically later and so the auto-merge process will be started. You can simply wait for now.

@nazar-pc

Copy link
Copy Markdown
Contributor

master needs to be merged in after #11829

@paritytech-processbot

Copy link
Copy Markdown

Merge cancelled due to error. Error: Head SHA changed from 53f1825 to 9ccbc0f

@arkpar

arkpar commented Jul 15, 2022

Copy link
Copy Markdown
Member Author

bot merge

@paritytech-processbot

Copy link
Copy Markdown

Waiting for commit status.

@paritytech-processbot paritytech-processbot Bot merged commit 4a19d40 into master Jul 15, 2022
@paritytech-processbot paritytech-processbot Bot deleted the a-sync-best-block-metric branch July 15, 2022 11:55
DaviRain-Su pushed a commit to octopus-network/substrate that referenced this pull request Aug 23, 2022
* Fixed sync target detection

* Always report sync_target metric

* Clamp median across all peers
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* Fixed sync target detection

* Always report sync_target metric

* Clamp median across all peers
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants