Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove usage of the pallet::getter macro from pallet-grandpa #4529

Merged
merged 25 commits into from
Jan 13, 2025
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
ae29e4b
to turbo
PolkadotDom May 20, 2024
a5f423e
Remove getters and usage
PolkadotDom May 20, 2024
533b9bf
Implement explicit getters to replace lost visibility from getter mac…
PolkadotDom May 20, 2024
ccf7a20
Fix getters to be explicitly what the macro had generated, remove unu…
PolkadotDom May 20, 2024
5e7db56
Fix syntax bug
PolkadotDom May 20, 2024
ab1e44f
Remove unused import
PolkadotDom May 20, 2024
7e311ad
fmt
PolkadotDom May 20, 2024
db2f3b2
Create pr_9999.prdoc
PolkadotDom May 20, 2024
12cccec
Update prdoc number
PolkadotDom May 20, 2024
c54a534
Remove explicit getters, make storage items public, bump to major
PolkadotDom May 23, 2024
ca029b3
Remove added line
PolkadotDom May 23, 2024
84196e7
Remove added comment
PolkadotDom May 23, 2024
ecd0aaa
Fix visibility warning
PolkadotDom May 23, 2024
74ee8d6
Update pallet external references
PolkadotDom May 23, 2024
b817682
Update pr_4529.prdoc
PolkadotDom May 23, 2024
9407991
Merge branch 'master' into dom/remove-getters-pallet-grandpa
PolkadotDom Jun 22, 2024
61151fc
Merge remote-tracking branch 'upstream/master' into dom/remove-getter…
PolkadotDom Jul 25, 2024
e6776ee
Add removed getters back explicitly
PolkadotDom Jul 25, 2024
51dbc92
Set values to option<> where appropriate
PolkadotDom Jul 25, 2024
46c3ca5
fmt
PolkadotDom Jul 25, 2024
5cfd188
Update prdoc
PolkadotDom Jul 25, 2024
8392342
Merge branch 'master' into dom/remove-getters-pallet-grandpa
PolkadotDom Aug 16, 2024
20aadd7
Merge remote-tracking branch 'upstream/master' into dom/remove-getter…
PolkadotDom Jan 13, 2025
b488ac4
Update substrate/frame/grandpa/src/lib.rs
bkchr Jan 13, 2025
979507a
Merge branch 'master' into dom/remove-getters-pallet-grandpa
bkchr Jan 13, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion polkadot/runtime/rococo/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2276,7 +2276,7 @@ sp_api::impl_runtime_apis! {
}

fn current_set_id() -> fg_primitives::SetId {
Grandpa::current_set_id()
pallet_grandpa::CurrentSetId::<Runtime>::get()
}

fn submit_report_equivocation_unsigned_extrinsic(
Expand Down
2 changes: 1 addition & 1 deletion polkadot/runtime/test-runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1186,7 +1186,7 @@ sp_api::impl_runtime_apis! {
}

fn current_set_id() -> fg_primitives::SetId {
Grandpa::current_set_id()
pallet_grandpa::CurrentSetId::<Runtime>::get()
}

fn submit_report_equivocation_unsigned_extrinsic(
Expand Down
2 changes: 1 addition & 1 deletion polkadot/runtime/westend/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2300,7 +2300,7 @@ sp_api::impl_runtime_apis! {
}

fn current_set_id() -> fg_primitives::SetId {
Grandpa::current_set_id()
pallet_grandpa::CurrentSetId::<Runtime>::get()
}

fn submit_report_equivocation_unsigned_extrinsic(
Expand Down
22 changes: 22 additions & 0 deletions prdoc/pr_4529.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json

title: Removed `pallet::getter` usage from pallet-grandpa

doc:
- audience: Runtime Dev
description: |
This PR removed the `pallet::getter`s from `pallet-grandpa`.
The syntax `StorageItem::<T, I>::get()` should be used instead

crates:
- name: pallet-grandpa
bump: minor
- name: kitchensink-runtime
bump: none
- name: westend-runtime
bump: none
- name: polkadot-test-runtime
bump: none
- name: rococo-runtime
bump: none
2 changes: 1 addition & 1 deletion substrate/bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2978,7 +2978,7 @@ impl_runtime_apis! {
}

fn current_set_id() -> sp_consensus_grandpa::SetId {
Grandpa::current_set_id()
pallet_grandpa::CurrentSetId::<Runtime>::get()
}

fn submit_report_equivocation_unsigned_extrinsic(
Expand Down
4 changes: 2 additions & 2 deletions substrate/frame/grandpa/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

//! Benchmarks for the GRANDPA pallet.

use super::{Pallet as Grandpa, *};
use super::*;
use frame_benchmarking::v2::*;
use frame_system::RawOrigin;
use sp_core::H256;
Expand Down Expand Up @@ -69,7 +69,7 @@ mod benchmarks {
#[extrinsic_call]
_(RawOrigin::Root, delay, best_finalized_block_number);

assert!(Grandpa::<T>::stalled().is_some());
assert!(Stalled::<T>::get().is_some());
}

impl_benchmark_test_suite!(
Expand Down
2 changes: 1 addition & 1 deletion substrate/frame/grandpa/src/equivocation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ where
evidence: (EquivocationProof<T::Hash, BlockNumberFor<T>>, T::KeyOwnerProof),
) -> Result<(), DispatchError> {
let (equivocation_proof, key_owner_proof) = evidence;
let reporter = reporter.or_else(|| <pallet_authorship::Pallet<T>>::author());
let reporter = reporter.or_else(|| pallet_authorship::Pallet::<T>::author());
let offender = equivocation_proof.offender().clone();

// We check the equivocation within the context of its set id (and
Expand Down
108 changes: 71 additions & 37 deletions substrate/frame/grandpa/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ pub mod pallet {
impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
fn on_finalize(block_number: BlockNumberFor<T>) {
// check for scheduled pending authority set changes
if let Some(pending_change) = <PendingChange<T>>::get() {
if let Some(pending_change) = PendingChange::<T>::get() {
// emit signal if we're at the block that scheduled the change
if block_number == pending_change.scheduled_at {
let next_authorities = pending_change.next_authorities.to_vec();
Expand All @@ -150,12 +150,12 @@ pub mod pallet {
Self::deposit_event(Event::NewAuthorities {
authority_set: pending_change.next_authorities.into_inner(),
});
<PendingChange<T>>::kill();
PendingChange::<T>::kill();
}
}

// check for scheduled pending state changes
match <State<T>>::get() {
match State::<T>::get() {
StoredState::PendingPause { scheduled_at, delay } => {
// signal change to pause
if block_number == scheduled_at {
Expand All @@ -164,7 +164,7 @@ pub mod pallet {

// enact change to paused state
if block_number == scheduled_at + delay {
<State<T>>::put(StoredState::Paused);
State::<T>::put(StoredState::Paused);
Self::deposit_event(Event::Paused);
}
},
Expand All @@ -176,7 +176,7 @@ pub mod pallet {

// enact change to live state
if block_number == scheduled_at + delay {
<State<T>>::put(StoredState::Live);
State::<T>::put(StoredState::Live);
Self::deposit_event(Event::Resumed);
}
},
Expand Down Expand Up @@ -297,37 +297,32 @@ pub mod pallet {
}

#[pallet::type_value]
pub(super) fn DefaultForState<T: Config>() -> StoredState<BlockNumberFor<T>> {
pub fn DefaultForState<T: Config>() -> StoredState<BlockNumberFor<T>> {
StoredState::Live
}

/// State of the current authority set.
#[pallet::storage]
#[pallet::getter(fn state)]
pub(super) type State<T: Config> =
pub type State<T: Config> =
StorageValue<_, StoredState<BlockNumberFor<T>>, ValueQuery, DefaultForState<T>>;

/// Pending change: (signaled at, scheduled change).
#[pallet::storage]
#[pallet::getter(fn pending_change)]
pub(super) type PendingChange<T: Config> =
pub type PendingChange<T: Config> =
StorageValue<_, StoredPendingChange<BlockNumberFor<T>, T::MaxAuthorities>>;

/// next block number where we can force a change.
#[pallet::storage]
#[pallet::getter(fn next_forced)]
pub(super) type NextForced<T: Config> = StorageValue<_, BlockNumberFor<T>>;
pub type NextForced<T: Config> = StorageValue<_, BlockNumberFor<T>>;

/// `true` if we are currently stalled.
#[pallet::storage]
#[pallet::getter(fn stalled)]
pub(super) type Stalled<T: Config> = StorageValue<_, (BlockNumberFor<T>, BlockNumberFor<T>)>;
pub type Stalled<T: Config> = StorageValue<_, (BlockNumberFor<T>, BlockNumberFor<T>)>;

/// The number of changes (both in terms of keys and underlying economic responsibilities)
/// in the "set" of Grandpa validators from genesis.
#[pallet::storage]
#[pallet::getter(fn current_set_id)]
pub(super) type CurrentSetId<T: Config> = StorageValue<_, SetId, ValueQuery>;
pub type CurrentSetId<T: Config> = StorageValue<_, SetId, ValueQuery>;

/// A mapping from grandpa set ID to the index of the *most recent* session for which its
/// members were responsible.
Expand All @@ -340,12 +335,11 @@ pub mod pallet {
///
/// TWOX-NOTE: `SetId` is not under user control.
#[pallet::storage]
#[pallet::getter(fn session_for_set)]
pub(super) type SetIdSession<T: Config> = StorageMap<_, Twox64Concat, SetId, SessionIndex>;
pub type SetIdSession<T: Config> = StorageMap<_, Twox64Concat, SetId, SessionIndex>;

/// The current list of authorities.
#[pallet::storage]
pub(crate) type Authorities<T: Config> =
pub type Authorities<T: Config> =
StorageValue<_, BoundedAuthorityList<T::MaxAuthorities>, ValueQuery>;

#[derive(frame_support::DefaultNoBound)]
Expand Down Expand Up @@ -432,6 +426,46 @@ pub enum StoredState<N> {
}

impl<T: Config> Pallet<T> {
/// State of the current authority set.
pub fn state() -> StoredState<BlockNumberFor<T>> {
PolkadotDom marked this conversation as resolved.
Show resolved Hide resolved
State::<T>::get()
}

/// Pending change: (signaled at, scheduled change).
pub fn pending_change() -> Option<StoredPendingChange<BlockNumberFor<T>, T::MaxAuthorities>> {
PendingChange::<T>::get()
}

/// next block number where we can force a change.
pub fn next_forced() -> Option<BlockNumberFor<T>> {
NextForced::<T>::get()
}

/// `true` if we are currently stalled.
pub fn stalled() -> Option<(BlockNumberFor<T>, BlockNumberFor<T>)> {
Stalled::<T>::get()
}

/// The number of changes (both in terms of keys and underlying economic responsibilities)
/// in the "set" of Grandpa validators from genesis.
pub fn current_set_id() -> SetId {
CurrentSetId::<T>::get()
}

/// A mapping from grandpa set ID to the index of the *most recent* session for which its
/// members were responsible.
///
/// This is only used for validating equivocation proofs. An equivocation proof must
/// contains a key-ownership proof for a given session, therefore we need a way to tie
/// together sessions and GRANDPA set ids, i.e. we need to validate that a validator
/// was the owner of a given key on a given session, and what the active set ID was
/// during that session.
///
/// TWOX-NOTE: `SetId` is not under user control.
bkchr marked this conversation as resolved.
Show resolved Hide resolved
pub fn session_for_set(set_id: SetId) -> Option<SessionIndex> {
SetIdSession::<T>::get(set_id)
}

/// Get the current set of authorities, along with their respective weights.
pub fn grandpa_authorities() -> AuthorityList {
Authorities::<T>::get().into_inner()
Expand All @@ -440,9 +474,9 @@ impl<T: Config> Pallet<T> {
/// Schedule GRANDPA to pause starting in the given number of blocks.
/// Cannot be done when already paused.
pub fn schedule_pause(in_blocks: BlockNumberFor<T>) -> DispatchResult {
if let StoredState::Live = <State<T>>::get() {
let scheduled_at = <frame_system::Pallet<T>>::block_number();
<State<T>>::put(StoredState::PendingPause { delay: in_blocks, scheduled_at });
if let StoredState::Live = State::<T>::get() {
let scheduled_at = frame_system::Pallet::<T>::block_number();
State::<T>::put(StoredState::PendingPause { delay: in_blocks, scheduled_at });

Ok(())
} else {
Expand All @@ -452,9 +486,9 @@ impl<T: Config> Pallet<T> {

/// Schedule a resume of GRANDPA after pausing.
pub fn schedule_resume(in_blocks: BlockNumberFor<T>) -> DispatchResult {
if let StoredState::Paused = <State<T>>::get() {
let scheduled_at = <frame_system::Pallet<T>>::block_number();
<State<T>>::put(StoredState::PendingResume { delay: in_blocks, scheduled_at });
if let StoredState::Paused = State::<T>::get() {
let scheduled_at = frame_system::Pallet::<T>::block_number();
State::<T>::put(StoredState::PendingResume { delay: in_blocks, scheduled_at });

Ok(())
} else {
Expand All @@ -481,17 +515,17 @@ impl<T: Config> Pallet<T> {
in_blocks: BlockNumberFor<T>,
forced: Option<BlockNumberFor<T>>,
) -> DispatchResult {
if !<PendingChange<T>>::exists() {
let scheduled_at = <frame_system::Pallet<T>>::block_number();
if !PendingChange::<T>::exists() {
let scheduled_at = frame_system::Pallet::<T>::block_number();

if forced.is_some() {
if Self::next_forced().map_or(false, |next| next > scheduled_at) {
if NextForced::<T>::get().map_or(false, |next| next > scheduled_at) {
return Err(Error::<T>::TooSoon.into())
}

// only allow the next forced change when twice the window has passed since
// this one.
<NextForced<T>>::put(scheduled_at + in_blocks * 2u32.into());
NextForced::<T>::put(scheduled_at + in_blocks * 2u32.into());
}

let next_authorities = WeakBoundedVec::<_, T::MaxAuthorities>::force_from(
Expand All @@ -502,7 +536,7 @@ impl<T: Config> Pallet<T> {
),
);

<PendingChange<T>>::put(StoredPendingChange {
PendingChange::<T>::put(StoredPendingChange {
delay: in_blocks,
scheduled_at,
next_authorities,
Expand All @@ -518,7 +552,7 @@ impl<T: Config> Pallet<T> {
/// Deposit one of this module's logs.
fn deposit_log(log: ConsensusLog<BlockNumberFor<T>>) {
let log = DigestItem::Consensus(GRANDPA_ENGINE_ID, log.encode());
<frame_system::Pallet<T>>::deposit_log(log);
frame_system::Pallet::<T>::deposit_log(log);
}

// Perform module initialization, abstracted so that it can be called either through genesis
Expand Down Expand Up @@ -554,7 +588,7 @@ impl<T: Config> Pallet<T> {
// when we record old authority sets we could try to figure out _who_
// failed. until then, we can't meaningfully guard against
// `next == last` the way that normal session changes do.
<Stalled<T>>::put((further_wait, median));
Stalled::<T>::put((further_wait, median));
}
}

Expand Down Expand Up @@ -583,10 +617,10 @@ where
// Always issue a change if `session` says that the validators have changed.
// Even if their session keys are the same as before, the underlying economic
// identities have changed.
let current_set_id = if changed || <Stalled<T>>::exists() {
let current_set_id = if changed || Stalled::<T>::exists() {
let next_authorities = validators.map(|(_, k)| (k, 1)).collect::<Vec<_>>();

let res = if let Some((further_wait, median)) = <Stalled<T>>::take() {
let res = if let Some((further_wait, median)) = Stalled::<T>::take() {
Self::schedule_change(next_authorities, further_wait, Some(median))
} else {
Self::schedule_change(next_authorities, Zero::zero(), None)
Expand All @@ -608,17 +642,17 @@ where
// either the session module signalled that the validators have changed
// or the set was stalled. but since we didn't successfully schedule
// an authority set change we do not increment the set id.
Self::current_set_id()
CurrentSetId::<T>::get()
}
} else {
// nothing's changed, neither economic conditions nor session keys. update the pointer
// of the current set.
Self::current_set_id()
CurrentSetId::<T>::get()
};

// update the mapping to note that the current set corresponds to the
// latest equivalent session (i.e. now).
let session_index = <pallet_session::Pallet<T>>::current_index();
let session_index = pallet_session::Pallet::<T>::current_index();
SetIdSession::<T>::insert(current_set_id, &session_index);
}

Expand Down
Loading
Loading