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

approval-voting improvement: include all tranche0 assignments in one certificate #6782

Open
wants to merge 118 commits into
base: master
Choose a base branch
from

Conversation

sandreim
Copy link
Contributor

@sandreim sandreim commented Feb 24, 2023

fixes paritytech/polkadot-sdk#729

This PR will upgrade the network protocol to version 2 -> VStaging which will later be renamed to V2. This version introduces a new kind of assignment certificate that will be used for tranche0 assignments. Instead of issuing/importing one tranche0 assignment per candidate, there will be just one certificate per relay chain block per validator. However, we will not be sending out the new assignment certificates, yet. So everything should work exactly as before. Once the majority of the validators have been upgraded to the new protocol version we will enable the new certificates (starting at a specific relay chain block) with a new client update.

There are still a few things that need to be done:

Signed-off-by: Andrei Sandu <[email protected]>
- tuned test params to test single and multiple assignments

Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
@sandreim sandreim added C1-low PR touches the given topic and has a low impact on builders. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited. B1-note_worthy Changes should be noted in the release notes A0-pleasereview labels Feb 24, 2023
Signed-off-by: Andrei Sandu <[email protected]>
@sandreim sandreim requested a review from rphmeier August 2, 2023 15:36
@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/dynamic-backing-groups-preparing-cores-for-agile-scheduling/3629/3

Signed-off-by: Andrei Sandu <[email protected]>
…/vrf_modulo_comapct_assignment

Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
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.

Going to give it an approval to signal readiness for an audit.

Just to be clear, before merging this, I expect to see the following addressed:

  • merge conflicts resolved after async backing is merged
  • Versioned::VStaging is stabilized into Versioned::V2
  • Another Versi burn-in after these changes
  • audit (when v2 is actived)

node/service/src/parachains_db/upgrade.rs Show resolved Hide resolved
@@ -1753,12 +2033,23 @@ impl ApprovalDistribution {
ApprovalDistributionMessage::NewBlocks(metas) => {
state.handle_new_blocks(ctx, metrics, metas, rng).await;
},
ApprovalDistributionMessage::DistributeAssignment(cert, candidate_index) => {
ApprovalDistributionMessage::DistributeAssignment(cert, candidate_indices) => {
// TODO: Fix warning: `Importing locally an already known assignment` for multiple candidate assignments.
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to addressed before merge? If not, don't forget to create an issue :)

node/network/approval-distribution/src/lib.rs Show resolved Hide resolved
node/network/approval-distribution/src/lib.rs Outdated Show resolved Hide resolved
Cargo.lock Outdated Show resolved Hide resolved
@sandreim
Copy link
Contributor Author

Going to give it an approval to signal readiness for an audit.

Thanks !

Just to be clear, before merging this, I expect to see the following addressed:

  • merge conflicts resolved after async backing is merged

Yup, this is the next step.

  • Versioned::VStaging is stabilized into Versioned::V2

My plan is to merge in master and piggy back on the network upgrade for async backing. Currently master code has the same naming with same semantics. But yeah, if we remove the feature flag until I merge, then it will have to be V3.

  • Another Versi burn-in after these changes

Yes, this is in progress implicitly as we burn it along in #7554 , but I will do a separate test before the merge.

  • audit (when v2 is actived)

I am assuming we want to do the audit before the merge, then we are more flexible to when we enable the new code.

Signed-off-by: Andrei Sandu <[email protected]>
…/vrf_modulo_comapct_assignment

Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
@paritytech-ci paritytech-ci requested a review from a team August 21, 2023 21:20
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/3424355

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. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited. E1-database_migration PR introduces code that does a one-way migration of the database. T0-node This PR/Issue is related to the topic “node”.
Projects
Development

Successfully merging this pull request may close these issues.

approval-voting: merge tranche 0 assignments