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

Add BEEFY capabilities to Westend and Kusama #7591

Merged
merged 17 commits into from
Aug 22, 2023

Conversation

acatangiu
Copy link
Contributor

@acatangiu acatangiu commented Aug 8, 2023

This enables BEEFY capabilities on Westend and Kusama.

Governance/sudo call is later required to enable/start consensus.

Part of paritytech/parity-bridges-common#2462

Notes:

Validators can still opt-out of running BEEFY using --no-beefy flag, but this should be used in case of stability issues only. Ideally validators should run BEEFY consensus.

Westend and Kusama validators (or at least archive nodes), should also run with --enable-offchain-indexing=true so that MMR proof generation is available for submitting to light-clients.

cumulus companion: paritytech/cumulus#3021

Signed-off-by: Adrian Catangiu <adrian@parity.io>
Signed-off-by: Adrian Catangiu <adrian@parity.io>
Signed-off-by: Adrian Catangiu <adrian@parity.io>
@acatangiu acatangiu added A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. B1-note_worthy Changes should be noted in the release notes labels Aug 8, 2023
@acatangiu acatangiu requested a review from andresilva August 8, 2023 15:29
@acatangiu acatangiu self-assigned this Aug 8, 2023
@paritytech-ci paritytech-ci requested review from a team August 8, 2023 15:29
…ative

Since these keys are only used for development/local chains, also publish
the secret seeds used to generate the public keys, so that developers can
recover/generate the private key pairs if needed.

Signed-off-by: Adrian Catangiu <adrian@parity.io>
@acatangiu acatangiu added the T0-node This PR/Issue is related to the topic “node”. label Aug 8, 2023
Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

I haven't reviewed the entire PR yet, was mainly looking at session key handling (since it's something that could break the existing runtime).

runtime/westend/src/lib.rs Show resolved Hide resolved
runtime/kusama/src/lib.rs Show resolved Hide resolved
@paritytech-ci paritytech-ci requested review from a team August 9, 2023 13:18
@andresilva
Copy link
Contributor

Also regarding session keys, when this gets deployed we'll have to ask validators to rotate their session keys so that BEEFY keys are generated and set on-chain. We'll need to monitor this before triggering the activation.

@Polkadot-Forum
Copy link

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

https://forum.polkadot.network/t/polkadot-kusama-bridge/2971/4

@acatangiu acatangiu changed the title Enable BEEFY on Westend and Kusama Add BEEFY capabilities to Westend and Kusama Aug 10, 2023
Copy link
Contributor Author

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

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

@ggwpez or @kianenigma this is my first runtime migration, how do I properly test it? 😄

runtime/kusama/src/lib.rs Show resolved Hide resolved
runtime/westend/src/lib.rs Show resolved Hide resolved
@acatangiu
Copy link
Contributor Author

acatangiu commented Aug 15, 2023

@ggwpez or @kianenigma this is my first runtime migration, how do I properly test it? 😄

I tested the migration locally on Westend using sudo and it (now) works fine.

@andresilva PTAL

Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

lgtm. just minor nits

runtime/kusama/src/lib.rs Outdated Show resolved Hide resolved
runtime/kusama/src/lib.rs Show resolved Hide resolved
runtime/kusama/src/lib.rs Outdated Show resolved Hide resolved
runtime/kusama/src/lib.rs Outdated Show resolved Hide resolved
@@ -1385,6 +1498,14 @@ construct_runtime! {
Staking: pallet_staking::{Pallet, Call, Storage, Config<T>, Event<T>} = 6,
Offences: pallet_offences::{Pallet, Storage, Event} = 7,
Historical: session_historical::{Pallet} = 34,

// BEEFY Bridges support.
Beefy: pallet_beefy::{Pallet, Call, Storage, Config<T>, ValidateUnsigned} = 200,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: not sure about choice of pallet index, but looking at the list there doesn't seem to be any real rule to it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, couldn't find an exact rule either, other than pallet indices are somewhat grouped by functionality with XCM for example starting at 100, so I think bridge-related ones could start from 200 as we shouldn't need 50 of them to reach 255 😛

runtime/westend/src/lib.rs Show resolved Hide resolved
runtime/westend/src/lib.rs Outdated Show resolved Hide resolved
runtime/westend/src/lib.rs Outdated Show resolved Hide resolved
runtime/westend/src/lib.rs Show resolved Hide resolved
node/service/src/chain_spec.rs Show resolved Hide resolved
@paritytech-ci paritytech-ci requested a review from a team August 17, 2023 15:31
@acatangiu
Copy link
Contributor Author

bot rebase

@paritytech-processbot
Copy link

Rebased

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.

Only nitpicks. Look good otherwise :)

runtime/kusama/src/lib.rs Outdated Show resolved Hide resolved
runtime/kusama/src/lib.rs Outdated Show resolved Hide resolved
impl frame_support::traits::OnRuntimeUpgrade for UpgradeSessionKeys {
fn on_runtime_upgrade() -> Weight {
Session::upgrade_keys::<OldSessionKeys, _>(transform_session_keys);
Perbill::from_percent(50) * BlockWeights::get().max_block
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Perbill::from_percent(50) * BlockWeights::get().max_block
BlockWeights::get().max_block

50% sounds rather random. Just let's take all the weight.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not familiar with block production code, if we take full Weight::max_block, do we run the risk of unable to produce block because of it being "overweight"?

I saw all past session key migrations "used" 50% max_weight, which seems safer to the untrained eye.

runtime/westend/src/lib.rs Outdated Show resolved Hide resolved
runtime/westend/src/lib.rs Outdated Show resolved Hide resolved
runtime/westend/src/lib.rs Outdated Show resolved Hide resolved
runtime/westend/src/lib.rs Outdated Show resolved Hide resolved
runtime/kusama/src/lib.rs Outdated Show resolved Hide resolved
acatangiu and others added 2 commits August 21, 2023 19:05
@acatangiu
Copy link
Contributor Author

bot merge

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. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. T0-node This PR/Issue is related to the topic “node”.
Projects
Development

Successfully merging this pull request may close these issues.

4 participants