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

Ignore peers that are not synced during sync status check#11705

Closed
nazar-pc wants to merge 1 commit into
paritytech:masterfrom
nazar-pc:ignore-not-synced-peers
Closed

Ignore peers that are not synced during sync status check#11705
nazar-pc wants to merge 1 commit into
paritytech:masterfrom
nazar-pc:ignore-not-synced-peers

Conversation

@nazar-pc

Copy link
Copy Markdown
Contributor

This addresses my comment from https://github.com/paritytech/substrate/pull/11547/files#r889214514

Basically there is no reason to take into account peers that have best number below ours.

@bkchr

bkchr commented Jun 19, 2022

Copy link
Copy Markdown
Member

Hmm, how often do you really have 7000 syncing nodes? I'm not sure if that maybe opens some other attack vector. @arkpar WDYT?

@nazar-pc

Copy link
Copy Markdown
Contributor Author

We're aiming for network with hundreds of thousands of nodes, so I guess the answer is... all the time? With over 25k of consensus nodes during non-incentivized testnet I can totally expect inrush of thousands of non-synced nodes into the network after some announcement and this is a potential attach vector that will cause extra forking.

@bkchr bkchr 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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Jun 19, 2022
@nazar-pc

Copy link
Copy Markdown
Contributor Author

The way I see this, there isn't any benefit in looking at peers that are obviously not synced in determining whether local peer is synced, it can't possibly add any useful context, but it may hurt in specific cases.

@arkpar

arkpar commented Jun 20, 2022

Copy link
Copy Markdown
Member

Makes sense to me

@bkchr bkchr requested a review from arkpar June 20, 2022 13:20
@nazar-pc

Copy link
Copy Markdown
Contributor Author

Closing as obsolete after #11817

@nazar-pc nazar-pc closed this Jul 15, 2022
@nazar-pc nazar-pc deleted the ignore-not-synced-peers branch July 15, 2022 12:21
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