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

remove connected disconnected state, 3rd attempt #3898

Merged
merged 20 commits into from
Sep 28, 2021
Merged

remove connected disconnected state, 3rd attempt #3898

merged 20 commits into from
Sep 28, 2021

Conversation

drahnr
Copy link
Contributor

@drahnr drahnr commented Sep 20, 2021 •
edited
Loading

Split commits into tiny bit-sized chunks.

Re-introduces functionality of #3868 without the previous issue.

@drahnr drahnr added A3-in_progress Pull request is in progress. No review needed at this stage. 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 Sep 20, 2021
@drahnr drahnr self-assigned this Sep 20, 2021
@drahnr
Copy link
Contributor Author

drahnr commented Sep 20, 2021

Commit 9ac71f5 verified to work.

@drahnr drahnr added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Sep 20, 2021
ordian
ordian approved these changes Sep 21, 2021
View reviewed changes
Copy link
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Looks good, but I suggest we confirm it works on a validator as well as a full node before merging. And CI needs fixing.

node/service/src/lib.rs Outdated Show resolved Hide resolved
@drahnr drahnr added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Sep 22, 2021
@drahnr drahnr added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Sep 22, 2021
@drahnr
Copy link
Contributor Author

drahnr commented Sep 22, 2021

Checked dfe7044

eskimor
eskimor previously approved these changes Sep 28, 2021
View reviewed changes
@eskimor eskimor dismissed their stale review September 28, 2021 09:45

Did not mean to yet.

eskimor
eskimor approved these changes Sep 28, 2021
View reviewed changes
Copy link
Member

@eskimor eskimor left a comment

Choose a reason for hiding this comment

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

Just to recap - why is it not possible to not construct the overseer in the first place, if we find that we are not a relay chain? And make the handle so that it cannot exist without an overseer - so chain selection cannot be wrong?

@drahnr
Copy link
Contributor Author

drahnr commented Sep 28, 2021 •
edited
Loading

Just to recap - why is it not possible to not construct the overseer in the first place, if we find that we are not a relay chain? And make the handle so that it cannot exist without an overseer - so chain selection cannot be wrong?

It's not that simple. There is a rather large dependency loop, with chain selection, grandpa, substrate, and the overseer handle closing the loop. Now to resolve this the OverseerConnector was created. An alternative way of fixing it alltogether, is indeed splitting the code into two distinct paths, but that itself is a rather invasive thing to do and we would still have the circular dependency in one case and we have to maintain two code paths for init or cut the existing setup routines in a more granular way (which is also started in this PR). Out of scope of this PR, created paritytech/polkadot-sdk#939 to track this in for the future™.

@eskimor
Copy link
Member

eskimor commented Sep 28, 2021

@ordian if there were no issues in testing, I would say - let's merge.

@ordian
Copy link
Member

ordian commented Sep 28, 2021

Just to recap - why is it not possible to not construct the overseer in the first place, if we find that we are not a relay chain? And make the handle so that it cannot exist without an overseer - so chain selection cannot be wrong?

We were worried that disabling overseer on polkadot would interfere with block production when implementing #3321. So went with enabling overseer, but disabling approval-voting finality rule instead.

@ordian ordian merged commit e80340c into master Sep 28, 2021
@ordian ordian deleted the bernhard-thin-slices branch September 28, 2021 13:01
@ordian ordian mentioned this pull request Sep 28, 2021
Closed
ordian added a commit that referenced this pull request Sep 29, 2021
@ordian
Merge branch 'master' into temporary-malus-2
f320c20 
* master:
  feat: measured oneshots (#3902)
  remove `AllSubsystems` and `AllSubsystemsGen` types (#3874)
  Companion for Substrate#9867 (#3938)
  Substrate Companion for #9552 (#3834)
  CI: run disputes tests (#3962)
  Bump parity-scale-codec from 2.3.0 to 2.3.1 (#3959)
  approval-voting: populate session cache in advance (#3954)
  Bump libc from 0.2.102 to 0.2.103 (#3950)
  fix master (#3955)
  Docker files chore (#3880)
  Bump nix from 0.19.1 to 0.20.0 (#3587)
  remove connected disconnected state, 3rd attempt (#3898)
  fix flaky chain-selection tests (#3948)
  Add benchmarking for parachain runtime initializer pallet (#3913)
  minor chore changes (#3944)
  disputes: reject single-sided disputes (#3903)
ordian added a commit that referenced this pull request Sep 30, 2021
@ordian
Merge branch 'master' into ladi-logging-disputes
cac79f1 
* master: (52 commits)
  Companion for substrate PR#9890 (#3961)
  Bump version, tx_version and spec_version in prep for v0.9.11 (#3970)
  Fix master compilation (#3977)
  Make most XCM APIs accept an Into<MultiLocation> where MultiLocation is accepted (#3627)
  fix disputes tests (#3974)
  Drop availability only for candidates that lose disputes (#3973)
  revert +1 change to be on the safer side (#3972)
  paras_inherent: reject only candidates with concluded disputes (#3969)
  feat: measured oneshots (#3902)
  remove `AllSubsystems` and `AllSubsystemsGen` types (#3874)
  Companion for Substrate#9867 (#3938)
  Substrate Companion for #9552 (#3834)
  CI: run disputes tests (#3962)
  Bump parity-scale-codec from 2.3.0 to 2.3.1 (#3959)
  approval-voting: populate session cache in advance (#3954)
  Bump libc from 0.2.102 to 0.2.103 (#3950)
  fix master (#3955)
  Docker files chore (#3880)
  Bump nix from 0.19.1 to 0.20.0 (#3587)
  remove connected disconnected state, 3rd attempt (#3898)
  ...
@Dengjianping Dengjianping mentioned this pull request Oct 13, 2021
Merged
17 tasks
@ghzlatarev ghzlatarev mentioned this pull request Nov 2, 2021
Merged
19 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ordian ordian ordian approved these changes

@eskimor eskimor eskimor approved these changes

@Lldenaurois Lldenaurois Awaiting requested review from Lldenaurois

Assignees

@drahnr drahnr

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@drahnr @eskimor @ordian