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

collator-protocol: short-term fixes for connectivity #4640

Merged
merged 9 commits into from
Jan 4, 2022

Conversation

ordian
Copy link
Member

@ordian ordian commented Dec 29, 2021

Fixes #4435.

@ordian ordian 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 Dec 29, 2021
@ordian ordian requested a review from eskimor December 29, 2021 18:03
node/network/collator-protocol/src/collator_side/mod.rs Outdated Show resolved Hide resolved
);

// Add the current validator group to the reserved peers
connect_to_validators(ctx, validators).await;
Copy link
Member

Choose a reason for hiding this comment

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

That might cause issues. new_leaves is an Option at the moment, but that does not really fix the issue: We are not only collating on the very latest leaf. If we get a new leaves update for forks, we will only be able to collate with validators on the fork which had the most recent leaf update (Assuming other forks have different block numbers and thus might have other validators assigned.)

We could collect validators of all currently active leaves. Which in turn means we would stay connected to validators on stale forks, until it's block height gets finalized - so it is not perfect either.

What can happen with this implementation is, that collators will not be able to collate on the "right" fork at rotation boundaries sometimes. Considering that rotation boundaries are tough anyways as of now, this should actually be fine for the interim solution this is. ... so disregard.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a valid concern which I also have considered, but as you pointed out this is an interim solution.

Copy link
Member Author

Choose a reason for hiding this comment

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

new_leaves is an Option at the moment, but that does not really fix the issue: We are not only collating on the very latest leaf

note that we're using OurView instead of ActiveLeavesUpdate, so new_leaves can be a few leaves

Copy link
Member Author

@ordian ordian Dec 30, 2021

Choose a reason for hiding this comment

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

@eskimor
Copy link
Member

eskimor commented Dec 30, 2021

Looks good!

@ordian ordian force-pushed the ao-lower-collator-inactivity-period branch 3 times, most recently from a6d046c to 554ebe3 Compare December 30, 2021 16:44
@ordian ordian force-pushed the ao-lower-collator-inactivity-period branch from 554ebe3 to 503e467 Compare December 30, 2021 16:51
* master:
  Set CurrentCodeHash before running some dispatchable benchmarks (#4645)
  paras: split tests (#4636)
  Bump quote from 1.0.10 to 1.0.14 (#4632)
  Bump pin-project from 1.0.8 to 1.0.9 (#4606)
  chore: fix copy&paste and tidy comments (#4646)
  derive Copy and Clone for Upgrade signals (#4637) (#4647)
  paras: fix upgrade restriction signal (#4603)
  configuration: Rename validation_upgrade_{frequency -> cooldown} (#4635)
  Bump lru from 0.7.1 to 0.7.2 (#4633)
  paras: add governance control dispatchables (#4575)
@eskimor eskimor merged commit 5b74841 into master Jan 4, 2022
@eskimor eskimor deleted the ao-lower-collator-inactivity-period branch January 4, 2022 11:58
Wizdave97 pushed a commit to ComposableFi/polkadot that referenced this pull request Feb 3, 2022
* collator-protocol: add to reserved peers on every relay parent

* bump collator slots from 25 to 100

* collator-protocol: reduce inactivity timeout from 24s to 5s

* try to satisfy spellcheck

* add connection log

* fmt

* bring a warn back

* gather validators across all active leaves
para_id = ?id,
"Connecting to validators.",
);
connect_to_validators(ctx, validators).await;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is maybe a bug - collators immediately connect to validators but the validators still preserve the behavior of disconnecting collators that haven't declared within 1 second.

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -58,7 +58,7 @@ pub struct CollatorEvictionPolicy {
impl Default for CollatorEvictionPolicy {
fn default() -> Self {
CollatorEvictionPolicy {
inactive_collator: Duration::from_secs(24),
inactive_collator: Duration::from_secs(5),
undeclared: Duration::from_secs(1),
Copy link
Contributor

Choose a reason for hiding this comment

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

rphmeier added a commit that referenced this pull request Feb 13, 2022
ordian added a commit that referenced this pull request Feb 13, 2022
gilescope pushed a commit that referenced this pull request Feb 15, 2022
eskimor added a commit that referenced this pull request Feb 18, 2022
eskimor pushed a commit that referenced this pull request Feb 18, 2022
…#4914)

* Revert "collator-protocol: fix wrong warning (#4909)"

This reverts commit c02c24d.

* Revert "collator-protocol: short-term fixes for connectivity (#4640)"

This reverts commit 5b74841.

* make the slots great again

Co-authored-by: Andronik <write@reusable.software>
eskimor pushed a commit that referenced this pull request Feb 18, 2022
ordian added a commit that referenced this pull request Feb 18, 2022
coderobe pushed a commit that referenced this pull request Feb 18, 2022
* Revert "collator-protocol: short-term fixes for connectivity (#4640)"

This reverts commit 5b74841.

* make the slots great again

Co-authored-by: Andronik <write@reusable.software>
ordian added a commit that referenced this pull request Feb 22, 2022
* master: (27 commits)
  Bump `tokio` to 1.17.0 (#4965)
  bump transaction_version (#4956)
  Bump tracing-subscriber from 0.3.8 to 0.3.9 (#4954)
  staking miner: reuse ws conn for remote-ext (#4849)
  Revert "collator-protocol: short-term fixes for connectivity (#4640)" (#4914)
  Bump tracing from 0.1.30 to 0.1.31 (#4941)
  Better spam slots handling (#4845)
  Bump proc-macro-crate from 1.1.0 to 1.1.2 (#4936)
  corrected paras code validation event comments (#4932)
  Companion for refactor election score #10834 (#4927)
  Companion CI: Make sure to pass the update crates properly (#4928)
  update digest to v0.10.2 (#4907)
  Companion for #10832 (#4918)
  Companion for `Remove u32_trait` (#4920)
  Bump serde_json from 1.0.78 to 1.0.79 (#4916)
  Bump rand from 0.8.4 to 0.8.5 (#4917)
  Remove stale migrations post 9.16 release (#4848)
  Add proxy type for Kappa Sigma Mu (#4851)
  Baseline weights for `force_apply_min_commission` (#4896)
  Allow two Parachains to swap (#4772)
  ...
@coderobe coderobe mentioned this pull request Mar 17, 2022
13 tasks
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.

collator-protocol: short-term fixes for connectivity
3 participants