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

Don't start evicting peers right after SyncingEngine is started #14216

Merged
merged 6 commits into from
May 25, 2023

Conversation

altonen
Copy link
Contributor

@altonen altonen commented May 24, 2023

Parachain collators may need to wait to receive a relaychain block before they can start producing blocks which can cause SyncingEngine to incorrectly evict them.

When SyncingEngine is started, wait 2 minutes before the eviction is activated to give collators a chance to produce a block.

Parachain collators may need to wait to receive a relaychain block before
they can start producing blocks which can cause `SyncingEngine` to
incorrectly evict them.

When `SyncingEngine` is started, wait 2 minutes before the eviction is
activated to give collators a chance to produce a block.
@altonen altonen 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 T0-node This PR/Issue is related to the topic “node”. labels May 24, 2023
@altonen altonen requested review from pepoviola, wirednkod, skunert, bkchr and a team May 24, 2023 13:56
Copy link
Contributor

@pepoviola pepoviola left a comment

Choose a reason for hiding this comment

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

Can we also update the comment in here to be 30 seconds.
Thanks!!

@skunert
Copy link
Contributor

skunert commented May 24, 2023

For my understanding: If I see correctly, the reputation of a peer that is evicted because of inactivity is lowered. After what time will that same evicted peer be able to connect again?

@altonen
Copy link
Contributor Author

altonen commented May 24, 2023

For my understanding: If I see correctly, the reputation of a peer that is evicted because of inactivity is lowered. After what time will that same evicted peer be able to connect again?

If there are no peers with higher reputation, the peer is selected again right away and Notifications tries to reconnect but because they were disconnected, there is a 10-30 second back-off. After that back-off is done, the connection is retried. So I'd say around 30 seconds.

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

It is a hack, not super nice. So, let's hope we can remove this in the near future.

@altonen do we already have some issue to remove this eviction logic after your refactorings?

@altonen
Copy link
Contributor Author

altonen commented May 24, 2023

It is a hack, not super nice. So, let's hope we can remove this in the near future.

@altonen do we already have some issue to remove this eviction logic after your refactorings?

We can remove it soon. The notification PR is published already and here the code checks if it can accept the peer and if it fails, the handshake will fail for the remote peer and it won't have peers that rejected it. The eviction has not been removed in that PR though but I'll probably remove before it's converted from the draft state to ready state.

@altonen
Copy link
Contributor Author

altonen commented May 24, 2023

Btw I noticed that changes introduced in #13800 have made syncing tests visibly slower. If you checkout to its parent or revert the commit, they execute in around 60 seconds and after those changes the execution time is ~10 seconds slower.

@altonen
Copy link
Contributor Author

altonen commented May 25, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

👍

@altonen
Copy link
Contributor Author

altonen commented May 25, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

@altonen altonen merged commit 5a4f49b into master May 25, 2023
@altonen altonen deleted the fix-eviction branch May 25, 2023 14:23
Ank4n pushed a commit that referenced this pull request Jul 8, 2023
…4216)

* Don't start evicting peers right after `SyncingEngine` is started

Parachain collators may need to wait to receive a relaychain block before
they can start producing blocks which can cause `SyncingEngine` to
incorrectly evict them.

When `SyncingEngine` is started, wait 2 minutes before the eviction is
activated to give collators a chance to produce a block.

* fix doc

* Use `continue` instead of `break`

* Trigger CI

---------

Co-authored-by: parity-processbot <>
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
…ritytech#14216)

* Don't start evicting peers right after `SyncingEngine` is started

Parachain collators may need to wait to receive a relaychain block before
they can start producing blocks which can cause `SyncingEngine` to
incorrectly evict them.

When `SyncingEngine` is started, wait 2 minutes before the eviction is
activated to give collators a chance to produce a block.

* fix doc

* Use `continue` instead of `break`

* Trigger CI

---------

Co-authored-by: parity-processbot <>
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 T0-node This PR/Issue is related to the topic “node”.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants