From 476f183d00cd23d36ccda54bf7b001d018b0e310 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Sat, 4 Nov 2023 23:44:23 +0100 Subject: [PATCH 1/7] pallet-grandpa: Remove `GRANDPA_AUTHORITIES_KEY` Remove the `GRANDPA_AUTHORITIES_KEY` key and its usage. Apparently this was used in the early days to communicate the grandpa authorities to the node. However, we have now a runtime api that does this for us. So, this pull request is moving from the custom managed storage item to a FRAME managed storage item. This pr also includes a migration for doing the switch on a running chain. --- polkadot/runtime/rococo/src/lib.rs | 2 + polkadot/runtime/westend/src/lib.rs | 1 + substrate/frame/grandpa/src/lib.rs | 34 ++++--- substrate/frame/grandpa/src/migrations.rs | 3 + substrate/frame/grandpa/src/migrations/v5.rs | 92 +++++++++++++++++++ .../primitives/consensus/grandpa/src/lib.rs | 60 +----------- 6 files changed, 121 insertions(+), 71 deletions(-) create mode 100644 substrate/frame/grandpa/src/migrations/v5.rs diff --git a/polkadot/runtime/rococo/src/lib.rs b/polkadot/runtime/rococo/src/lib.rs index 257f364dca01..5acfe6f9da3c 100644 --- a/polkadot/runtime/rococo/src/lib.rs +++ b/polkadot/runtime/rococo/src/lib.rs @@ -1489,6 +1489,8 @@ pub mod migrations { frame_support::migrations::RemovePallet::DbWeight>, frame_support::migrations::RemovePallet::DbWeight>, frame_support::migrations::RemovePallet::DbWeight>, + + pallet_grandpa::migrations::MigrateV4ToV5, ); } diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index ec94973af4f3..23b342f6743b 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -1558,6 +1558,7 @@ pub mod migrations { pallet_nomination_pools::migration::versioned_migrations::V5toV6, pallet_referenda::migration::v1::MigrateV0ToV1, pallet_nomination_pools::migration::versioned_migrations::V6ToV7, + pallet_grandpa::migrations::MigrateV4ToV5, ); } diff --git a/substrate/frame/grandpa/src/lib.rs b/substrate/frame/grandpa/src/lib.rs index 95d1c8aa6094..952bba3f4ca1 100644 --- a/substrate/frame/grandpa/src/lib.rs +++ b/substrate/frame/grandpa/src/lib.rs @@ -30,14 +30,13 @@ // Re-export since this is necessary for `impl_apis` in runtime. pub use sp_consensus_grandpa::{ - self as fg_primitives, AuthorityId, AuthorityList, AuthorityWeight, VersionedAuthorityList, + self as fg_primitives, AuthorityId, AuthorityList, AuthorityWeight, }; -use codec::{self as codec, Decode, Encode, MaxEncodedLen}; +use codec::{Decode, Encode, MaxEncodedLen}; use frame_support::{ dispatch::{DispatchResultWithPostInfo, Pays}, pallet_prelude::Get, - storage, traits::OneSessionHandler, weights::Weight, WeakBoundedVec, @@ -45,8 +44,8 @@ use frame_support::{ use frame_system::pallet_prelude::BlockNumberFor; use scale_info::TypeInfo; use sp_consensus_grandpa::{ - ConsensusLog, EquivocationProof, ScheduledChange, SetId, GRANDPA_AUTHORITIES_KEY, - GRANDPA_ENGINE_ID, RUNTIME_LOG_TARGET as LOG_TARGET, + ConsensusLog, EquivocationProof, ScheduledChange, SetId, GRANDPA_ENGINE_ID, + RUNTIME_LOG_TARGET as LOG_TARGET, }; use sp_runtime::{generic::DigestItem, traits::Zero, DispatchResult}; use sp_session::{GetSessionNumber, GetValidatorCount}; @@ -75,7 +74,7 @@ pub mod pallet { use frame_system::pallet_prelude::*; /// The current storage version. - const STORAGE_VERSION: StorageVersion = StorageVersion::new(4); + const STORAGE_VERSION: StorageVersion = StorageVersion::new(5); #[pallet::pallet] #[pallet::storage_version(STORAGE_VERSION)] @@ -342,6 +341,11 @@ pub mod pallet { #[pallet::getter(fn session_for_set)] pub(super) type SetIdSession = StorageMap<_, Twox64Concat, SetId, SessionIndex>; + /// The current list of authorities. + #[pallet::storage] + pub(super) type Authorities = + StorageValue<_, BoundedAuthorityList, ValueQuery>; + #[derive(frame_support::DefaultNoBound)] #[pallet::genesis_config] pub struct GenesisConfig { @@ -354,7 +358,7 @@ pub mod pallet { impl BuildGenesisConfig for GenesisConfig { fn build(&self) { CurrentSetId::::put(SetId::default()); - Pallet::::initialize(&self.authorities) + Pallet::::initialize(self.authorities.clone()) } } @@ -428,12 +432,12 @@ pub enum StoredState { impl Pallet { /// Get the current set of authorities, along with their respective weights. pub fn grandpa_authorities() -> AuthorityList { - storage::unhashed::get_or_default::(GRANDPA_AUTHORITIES_KEY).into() + Authorities::::get().to_vec() } /// Set the current set of authorities, along with their respective weights. - fn set_grandpa_authorities(authorities: &AuthorityList) { - storage::unhashed::put(GRANDPA_AUTHORITIES_KEY, &VersionedAuthorityList::from(authorities)); + pub(crate) fn set_grandpa_authorities(authorities: &BoundedAuthorityList) { + Authorities::::put(authorities); } /// Schedule GRANDPA to pause starting in the given number of blocks. @@ -522,10 +526,14 @@ impl Pallet { // Perform module initialization, abstracted so that it can be called either through genesis // config builder or through `on_genesis_session`. - fn initialize(authorities: &AuthorityList) { + fn initialize(authorities: AuthorityList) { if !authorities.is_empty() { assert!(Self::grandpa_authorities().is_empty(), "Authorities are already initialized!"); - Self::set_grandpa_authorities(authorities); + Self::set_grandpa_authorities( + &BoundedAuthorityList::::try_from(authorities).expect( + "Granpa: `Config::MaxAuthorities` is smaller than the number of genesis authorities!", + ), + ); } // NOTE: initialize first session of first set. this is necessary for @@ -568,7 +576,7 @@ where I: Iterator, { let authorities = validators.map(|(_, k)| (k, 1)).collect::>(); - Self::initialize(&authorities); + Self::initialize(authorities); } fn on_new_session<'a, I: 'a>(changed: bool, validators: I, _queued_validators: I) diff --git a/substrate/frame/grandpa/src/migrations.rs b/substrate/frame/grandpa/src/migrations.rs index 6307cbdd3b05..3a484eb60d28 100644 --- a/substrate/frame/grandpa/src/migrations.rs +++ b/substrate/frame/grandpa/src/migrations.rs @@ -22,8 +22,11 @@ use frame_support::{ use crate::{Config, CurrentSetId, SetIdSession, LOG_TARGET}; +pub use v5::MigrateV4ToV5; + /// Version 4. pub mod v4; +mod v5; /// This migration will clean up all stale set id -> session entries from the /// `SetIdSession` storage map, only the latest `max_set_id_session_entries` diff --git a/substrate/frame/grandpa/src/migrations/v5.rs b/substrate/frame/grandpa/src/migrations/v5.rs new file mode 100644 index 000000000000..8317837ca0a1 --- /dev/null +++ b/substrate/frame/grandpa/src/migrations/v5.rs @@ -0,0 +1,92 @@ +// This file is part of Substrate. + +// Copyright (C) Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use crate::{BoundedAuthorityList, Pallet}; +use codec::Decode; +use frame_support::{ + migrations::VersionedMigration, + storage, + traits::{Get, OnRuntimeUpgrade}, + weights::Weight, +}; +use sp_consensus_grandpa::AuthorityList; +use sp_std::{marker::PhantomData, vec::Vec}; + +const GRANDPA_AUTHORITIES_KEY: &[u8] = b":grandpa_authorities"; + +fn load_authority_list() -> AuthorityList { + storage::unhashed::get_raw(GRANDPA_AUTHORITIES_KEY).map_or_else( + || Vec::new(), + |l| <(u8, AuthorityList)>::decode(&mut &l[..]).unwrap_or_default().1, + ) +} + +/// Actual implementation of [`MigrateV4ToV5`]. +pub struct MigrateImpl(PhantomData); + +impl OnRuntimeUpgrade for MigrateImpl { + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result, sp_runtime::TryRuntimeError> { + use codec::Encode; + + let authority_list_len = load_authority_list().len() as u32; + + if authority_list_len > T::MaxAuthorities::get() { + return Err( + "Grandpa: `Config::MaxAuthorities` is smaller than the actual number of authorities.".into() + ) + } + + Ok(authority_list_len.encode()) + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(state: Vec) -> Result<(), sp_runtime::TryRuntimeError> { + let len = u32::decode(&mut &state[..]).unwrap(); + + frame_support::ensure!( + len == crate::Pallet::::grandpa_authorities().len() as u32, + "Grandpa: pre-migrated and post-migrated list should have the same length" + ); + + frame_support::ensure!( + load_authority_list().is_empty(), + "Old authority list shouldn't exist anymore" + ); + + Ok(()) + } + + fn on_runtime_upgrade() -> Weight { + crate::Pallet::::set_grandpa_authorities( + &BoundedAuthorityList::::force_from( + load_authority_list(), + Some("Grandpa: `Config::MaxAuthorities` is smaller than the actual number of authorities.") + ) + ); + + storage::unhashed::kill(GRANDPA_AUTHORITIES_KEY); + + T::DbWeight::get().reads_writes(1, 1) + } +} + +/// Migrate the storage from V4 to V5. +/// +/// Switches from `GRANDPA_AUTHORITIES_KEY` to a normal FRAME storage item. +pub type MigrateV4ToV5 = + VersionedMigration<4, 5, MigrateImpl, Pallet, ::DbWeight>; diff --git a/substrate/primitives/consensus/grandpa/src/lib.rs b/substrate/primitives/consensus/grandpa/src/lib.rs index baeaee4738e4..b51695435fb3 100644 --- a/substrate/primitives/consensus/grandpa/src/lib.rs +++ b/substrate/primitives/consensus/grandpa/src/lib.rs @@ -19,13 +19,10 @@ #![cfg_attr(not(feature = "std"), no_std)] -#[cfg(not(feature = "std"))] -extern crate alloc; - #[cfg(feature = "serde")] use serde::Serialize; -use codec::{Codec, Decode, Encode, Input}; +use codec::{Codec, Decode, Encode}; use scale_info::TypeInfo; #[cfg(feature = "std")] use sp_keystore::KeystorePtr; @@ -33,7 +30,7 @@ use sp_runtime::{ traits::{Header as HeaderT, NumberFor}, ConsensusEngineId, RuntimeDebug, }; -use sp_std::{borrow::Cow, vec::Vec}; +use sp_std::vec::Vec; /// The log target to be used by client code. pub const CLIENT_LOG_TARGET: &str = "grandpa"; @@ -62,10 +59,6 @@ pub type AuthoritySignature = app::Signature; /// The `ConsensusEngineId` of GRANDPA. pub const GRANDPA_ENGINE_ID: ConsensusEngineId = *b"FRNK"; -/// The storage key for the current set of weighted Grandpa authorities. -/// The value stored is an encoded VersionedAuthorityList. -pub const GRANDPA_AUTHORITIES_KEY: &[u8] = b":grandpa_authorities"; - /// The weight of an authority. pub type AuthorityWeight = u64; @@ -469,55 +462,6 @@ pub const PENDING_CHANGE_CALL: &str = "grandpa_pending_change"; /// WASM function call to get current GRANDPA authorities. pub const AUTHORITIES_CALL: &str = "grandpa_authorities"; -/// The current version of the stored AuthorityList type. The encoding version MUST be updated any -/// time the AuthorityList type changes. -const AUTHORITIES_VERSION: u8 = 1; - -/// An AuthorityList that is encoded with a version specifier. The encoding version is updated any -/// time the AuthorityList type changes. This ensures that encodings of different versions of an -/// AuthorityList are differentiable. Attempting to decode an authority list with an unknown -/// version will fail. -#[derive(Default)] -pub struct VersionedAuthorityList<'a>(Cow<'a, AuthorityList>); - -impl<'a> From for VersionedAuthorityList<'a> { - fn from(authorities: AuthorityList) -> Self { - VersionedAuthorityList(Cow::Owned(authorities)) - } -} - -impl<'a> From<&'a AuthorityList> for VersionedAuthorityList<'a> { - fn from(authorities: &'a AuthorityList) -> Self { - VersionedAuthorityList(Cow::Borrowed(authorities)) - } -} - -impl<'a> Into for VersionedAuthorityList<'a> { - fn into(self) -> AuthorityList { - self.0.into_owned() - } -} - -impl<'a> Encode for VersionedAuthorityList<'a> { - fn size_hint(&self) -> usize { - (AUTHORITIES_VERSION, self.0.as_ref()).size_hint() - } - - fn using_encoded R>(&self, f: F) -> R { - (AUTHORITIES_VERSION, self.0.as_ref()).using_encoded(f) - } -} - -impl<'a> Decode for VersionedAuthorityList<'a> { - fn decode(value: &mut I) -> Result { - let (version, authorities): (u8, AuthorityList) = Decode::decode(value)?; - if version != AUTHORITIES_VERSION { - return Err("unknown Grandpa authorities version".into()) - } - Ok(authorities.into()) - } -} - /// An opaque type used to represent the key ownership proof at the runtime API /// boundary. The inner value is an encoded representation of the actual key /// ownership proof which will be parameterized when defining the runtime. At From 845efeb0f47e705f12c91a99e3252b2d53dd35a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Mon, 6 Nov 2023 18:05:58 +0100 Subject: [PATCH 2/7] Remove comment --- substrate/client/consensus/grandpa/src/lib.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/substrate/client/consensus/grandpa/src/lib.rs b/substrate/client/consensus/grandpa/src/lib.rs index da621abd254c..a4584e6fc807 100644 --- a/substrate/client/consensus/grandpa/src/lib.rs +++ b/substrate/client/consensus/grandpa/src/lib.rs @@ -471,9 +471,6 @@ where Client: ExecutorProvider + HeaderBackend, { fn get(&self) -> Result { - // This implementation uses the Grandpa runtime API instead of reading directly from the - // `GRANDPA_AUTHORITIES_KEY` as the data may have been migrated since the genesis block of - // the chain, whereas the runtime API is backwards compatible. self.executor() .call( self.expect_block_hash_from_id(&BlockId::Number(Zero::zero()))?, From 67c9f030dda97f313932853b5a069ebd35d0881f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Thu, 9 Nov 2023 12:47:52 +0100 Subject: [PATCH 3/7] Update substrate/frame/grandpa/src/lib.rs Co-authored-by: Davide Galassi --- substrate/frame/grandpa/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/grandpa/src/lib.rs b/substrate/frame/grandpa/src/lib.rs index 952bba3f4ca1..c366228cd3f4 100644 --- a/substrate/frame/grandpa/src/lib.rs +++ b/substrate/frame/grandpa/src/lib.rs @@ -432,7 +432,7 @@ pub enum StoredState { impl Pallet { /// Get the current set of authorities, along with their respective weights. pub fn grandpa_authorities() -> AuthorityList { - Authorities::::get().to_vec() + Authorities::::get().into_inner() } /// Set the current set of authorities, along with their respective weights. From 3849737562aafc0cd7be72965ca3f989632c6ff6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Thu, 9 Nov 2023 12:47:58 +0100 Subject: [PATCH 4/7] Update substrate/frame/grandpa/src/lib.rs Co-authored-by: Davide Galassi --- substrate/frame/grandpa/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/grandpa/src/lib.rs b/substrate/frame/grandpa/src/lib.rs index c366228cd3f4..a9e6fc7443e8 100644 --- a/substrate/frame/grandpa/src/lib.rs +++ b/substrate/frame/grandpa/src/lib.rs @@ -531,7 +531,7 @@ impl Pallet { assert!(Self::grandpa_authorities().is_empty(), "Authorities are already initialized!"); Self::set_grandpa_authorities( &BoundedAuthorityList::::try_from(authorities).expect( - "Granpa: `Config::MaxAuthorities` is smaller than the number of genesis authorities!", + "Grandpa: `Config::MaxAuthorities` is smaller than the number of genesis authorities!", ), ); } From 6f119a4f3a27d4c7f5ba8afafc3b3ce435043e32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Thu, 9 Nov 2023 12:49:20 +0100 Subject: [PATCH 5/7] Remove moare --- substrate/primitives/consensus/grandpa/src/lib.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/substrate/primitives/consensus/grandpa/src/lib.rs b/substrate/primitives/consensus/grandpa/src/lib.rs index b51695435fb3..1cf5504c5e7d 100644 --- a/substrate/primitives/consensus/grandpa/src/lib.rs +++ b/substrate/primitives/consensus/grandpa/src/lib.rs @@ -457,11 +457,6 @@ where Some(grandpa::SignedMessage { message, signature, id: public }) } -/// WASM function call to check for pending changes. -pub const PENDING_CHANGE_CALL: &str = "grandpa_pending_change"; -/// WASM function call to get current GRANDPA authorities. -pub const AUTHORITIES_CALL: &str = "grandpa_authorities"; - /// An opaque type used to represent the key ownership proof at the runtime API /// boundary. The inner value is an encoded representation of the actual key /// ownership proof which will be parameterized when defining the runtime. At From 6a7459db72e49d44991530c77b68ebbef6f04423 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Thu, 9 Nov 2023 13:03:44 +0100 Subject: [PATCH 6/7] Review comments --- substrate/frame/grandpa/src/lib.rs | 11 +++-------- substrate/frame/grandpa/src/migrations/v5.rs | 8 ++++++-- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/substrate/frame/grandpa/src/lib.rs b/substrate/frame/grandpa/src/lib.rs index 952bba3f4ca1..4be485aaa16e 100644 --- a/substrate/frame/grandpa/src/lib.rs +++ b/substrate/frame/grandpa/src/lib.rs @@ -144,7 +144,7 @@ pub mod pallet { // enact the change if we've reached the enacting block if block_number == pending_change.scheduled_at + pending_change.delay { - Self::set_grandpa_authorities(&pending_change.next_authorities); + Authorities::::put(&pending_change.next_authorities); Self::deposit_event(Event::NewAuthorities { authority_set: pending_change.next_authorities.into_inner(), }); @@ -343,7 +343,7 @@ pub mod pallet { /// The current list of authorities. #[pallet::storage] - pub(super) type Authorities = + pub(crate) type Authorities = StorageValue<_, BoundedAuthorityList, ValueQuery>; #[derive(frame_support::DefaultNoBound)] @@ -435,11 +435,6 @@ impl Pallet { Authorities::::get().to_vec() } - /// Set the current set of authorities, along with their respective weights. - pub(crate) fn set_grandpa_authorities(authorities: &BoundedAuthorityList) { - Authorities::::put(authorities); - } - /// Schedule GRANDPA to pause starting in the given number of blocks. /// Cannot be done when already paused. pub fn schedule_pause(in_blocks: BlockNumberFor) -> DispatchResult { @@ -529,7 +524,7 @@ impl Pallet { fn initialize(authorities: AuthorityList) { if !authorities.is_empty() { assert!(Self::grandpa_authorities().is_empty(), "Authorities are already initialized!"); - Self::set_grandpa_authorities( + Authorities::::put( &BoundedAuthorityList::::try_from(authorities).expect( "Granpa: `Config::MaxAuthorities` is smaller than the number of genesis authorities!", ), diff --git a/substrate/frame/grandpa/src/migrations/v5.rs b/substrate/frame/grandpa/src/migrations/v5.rs index 8317837ca0a1..b9662a2f66bc 100644 --- a/substrate/frame/grandpa/src/migrations/v5.rs +++ b/substrate/frame/grandpa/src/migrations/v5.rs @@ -51,6 +51,10 @@ impl OnRuntimeUpgrade for MigrateImpl { ) } + if authority_list_len() == 0 { + return Err("Grandpa: Authority list is empty!".into()) + } + Ok(authority_list_len.encode()) } @@ -72,7 +76,7 @@ impl OnRuntimeUpgrade for MigrateImpl { } fn on_runtime_upgrade() -> Weight { - crate::Pallet::::set_grandpa_authorities( + crate::Authorities::::put( &BoundedAuthorityList::::force_from( load_authority_list(), Some("Grandpa: `Config::MaxAuthorities` is smaller than the actual number of authorities.") @@ -81,7 +85,7 @@ impl OnRuntimeUpgrade for MigrateImpl { storage::unhashed::kill(GRANDPA_AUTHORITIES_KEY); - T::DbWeight::get().reads_writes(1, 1) + T::DbWeight::get().reads_writes(1, 2) } } From cd88a2cb8ab9365aba3df6f5127bb02948c4fd0d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Thu, 9 Nov 2023 13:29:48 +0100 Subject: [PATCH 7/7] :facepalm: --- substrate/frame/grandpa/src/migrations/v5.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/grandpa/src/migrations/v5.rs b/substrate/frame/grandpa/src/migrations/v5.rs index b9662a2f66bc..24cfc34104b5 100644 --- a/substrate/frame/grandpa/src/migrations/v5.rs +++ b/substrate/frame/grandpa/src/migrations/v5.rs @@ -51,7 +51,7 @@ impl OnRuntimeUpgrade for MigrateImpl { ) } - if authority_list_len() == 0 { + if authority_list_len == 0 { return Err("Grandpa: Authority list is empty!".into()) }