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

[AHM] Replace Validator FullIdentification from Exposure to Existence #7936

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
4 changes: 2 additions & 2 deletions polkadot/runtime/test-runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -323,8 +323,8 @@ impl pallet_session::Config for Runtime {
}

impl pallet_session::historical::Config for Runtime {
type FullIdentification = ();
type FullIdentificationOf = pallet_staking::NullIdentity;
type FullIdentification = pallet_staking::Existence;
type FullIdentificationOf = pallet_staking::ExistenceOf<Runtime>;
}

pallet_staking_reward_curve::build! {
Expand Down
4 changes: 2 additions & 2 deletions polkadot/runtime/westend/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -537,8 +537,8 @@ impl pallet_session::Config for Runtime {
}

impl pallet_session::historical::Config for Runtime {
type FullIdentification = pallet_staking::Exposure<AccountId, Balance>;
type FullIdentificationOf = pallet_staking::ExposureOf<Runtime>;
type FullIdentification = pallet_staking::ExistenceOrLegacyExposure<AccountId, Balance>;
type FullIdentificationOf = pallet_staking::ExistenceOrLegacyExposureOf<Runtime>;
}

pub struct MaybeSignedPhase;
Expand Down
27 changes: 27 additions & 0 deletions prdoc/pr_7936.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
title: '[AHM] Replace Validator FullIdentification from `Exposure` to `Existence`'
doc:
- audience: Todo
description: |-
closes https://github.com/paritytech/polkadot-sdk/issues/6344.

## TODO
- [ ] TryStateCheck to see existing offence report decodes correctly.
crates:
- name: pallet-babe
bump: major
- name: pallet-beefy
bump: major
- name: pallet-grandpa
bump: major
- name: pallet-offences-benchmarking
bump: major
- name: pallet-root-offences
bump: major
- name: pallet-session-benchmarking
bump: major
- name: pallet-staking
bump: major
- name: westend-runtime
bump: major
- name: pallet-staking-ah-client
bump: major
4 changes: 2 additions & 2 deletions substrate/bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -776,8 +776,8 @@ impl pallet_session::Config for Runtime {
}

impl pallet_session::historical::Config for Runtime {
type FullIdentification = ();
type FullIdentificationOf = pallet_staking::NullIdentity;
type FullIdentification = pallet_staking::Existence;
type FullIdentificationOf = pallet_staking::ExistenceOf<Runtime>;
}

pallet_staking_reward_curve::build! {
Expand Down
4 changes: 2 additions & 2 deletions substrate/frame/babe/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,8 @@ impl pallet_session::Config for Test {
}

impl pallet_session::historical::Config for Test {
type FullIdentification = ();
type FullIdentificationOf = pallet_staking::NullIdentity;
type FullIdentification = pallet_staking::Existence;
type FullIdentificationOf = pallet_staking::ExistenceOf<Test>;
}

impl pallet_authorship::Config for Test {
Expand Down
4 changes: 2 additions & 2 deletions substrate/frame/beefy/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -189,8 +189,8 @@ impl pallet_session::Config for Test {
}

impl pallet_session::historical::Config for Test {
type FullIdentification = ();
type FullIdentificationOf = pallet_staking::NullIdentity;
type FullIdentification = pallet_staking::Existence;
type FullIdentificationOf = pallet_staking::ExistenceOf<Test>;
}

impl pallet_authorship::Config for Test {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,8 +147,8 @@ impl pallet_session::Config for Runtime {
type WeightInfo = ();
}
impl pallet_session::historical::Config for Runtime {
type FullIdentification = ();
type FullIdentificationOf = pallet_staking::NullIdentity;
type FullIdentification = pallet_staking::Existence;
type FullIdentificationOf = pallet_staking::ExistenceOf<Runtime>;
}

frame_election_provider_support::generate_solution_type!(
Expand Down
4 changes: 2 additions & 2 deletions substrate/frame/grandpa/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,8 @@ impl pallet_session::Config for Test {
}

impl pallet_session::historical::Config for Test {
type FullIdentification = ();
type FullIdentificationOf = pallet_staking::NullIdentity;
type FullIdentification = pallet_staking::Existence;
type FullIdentificationOf = pallet_staking::ExistenceOf<Test>;
}

impl pallet_authorship::Config for Test {
Expand Down
4 changes: 2 additions & 2 deletions substrate/frame/offences/benchmarking/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,8 @@ impl pallet_timestamp::Config for Test {
type WeightInfo = ();
}
impl pallet_session::historical::Config for Test {
type FullIdentification = ();
type FullIdentificationOf = pallet_staking::NullIdentity;
type FullIdentification = pallet_staking::Existence;
type FullIdentificationOf = pallet_staking::ExistenceOf<Test>;
}

sp_runtime::impl_opaque_keys! {
Expand Down
4 changes: 2 additions & 2 deletions substrate/frame/root-offences/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ pub mod pallet {
+ pallet_staking::Config
+ pallet_session::Config<ValidatorId = <Self as frame_system::Config>::AccountId>
+ pallet_session::historical::Config<
FullIdentification = (),
FullIdentificationOf = pallet_staking::NullIdentity,
FullIdentification = pallet_staking::Existence,
FullIdentificationOf = pallet_staking::ExistenceOf<Self>,
>
{
type RuntimeEvent: From<Event<Self>> + IsType<<Self as frame_system::Config>::RuntimeEvent>;
Expand Down
4 changes: 2 additions & 2 deletions substrate/frame/root-offences/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,8 +148,8 @@ impl pallet_staking::Config for Test {
}

impl pallet_session::historical::Config for Test {
type FullIdentification = ();
type FullIdentificationOf = pallet_staking::NullIdentity;
type FullIdentification = pallet_staking::Existence;
type FullIdentificationOf = pallet_staking::ExistenceOf<Test>;
}

sp_runtime::impl_opaque_keys! {
Expand Down
5 changes: 2 additions & 3 deletions substrate/frame/session/benchmarking/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ use frame_support::{
derive_impl, parameter_types,
traits::{ConstU32, ConstU64},
};
use pallet_staking::NullIdentity;
use sp_runtime::{traits::IdentityLookup, BuildStorage, KeyTypeId};

type AccountId = u64;
Expand Down Expand Up @@ -68,8 +67,8 @@ impl pallet_timestamp::Config for Test {
type WeightInfo = ();
}
impl pallet_session::historical::Config for Test {
type FullIdentification = ();
type FullIdentificationOf = NullIdentity;
type FullIdentification = pallet_staking::Existence;
type FullIdentificationOf = pallet_staking::ExistenceOf<Test>;
}

sp_runtime::impl_opaque_keys! {
Expand Down
9 changes: 6 additions & 3 deletions substrate/frame/staking/ah-client/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ pub mod pallet {
use core::result;
use frame_system::pallet_prelude::*;
use pallet_session::historical;
use pallet_staking::ExposureOf;
use pallet_staking::{ExistenceOrLegacyExposure, ExistenceOrLegacyExposureOf};
use polkadot_primitives::Id as ParaId;
use polkadot_runtime_parachains::origin::{ensure_parachain, Origin};
use sp_runtime::Perbill;
Expand Down Expand Up @@ -238,8 +238,11 @@ pub mod pallet {
where
T: pallet_session::Config<ValidatorId = <T as frame_system::Config>::AccountId>,
T: pallet_session::historical::Config<
FullIdentification = Exposure<<T as frame_system::Config>::AccountId, BalanceOf<T>>,
FullIdentificationOf = ExposureOf<T>,
FullIdentification = ExistenceOrLegacyExposure<
<T as frame_system::Config>::AccountId,
BalanceOf<T>,
>,
FullIdentificationOf = ExistenceOrLegacyExposureOf<T>,
>,
T::SessionHandler: pallet_session::SessionHandler<<T as frame_system::Config>::AccountId>,
T::SessionManager: pallet_session::SessionManager<<T as frame_system::Config>::AccountId>,
Expand Down
134 changes: 122 additions & 12 deletions substrate/frame/staking/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,9 @@ mod pallet;
extern crate alloc;

use alloc::{collections::btree_map::BTreeMap, vec, vec::Vec};
use codec::{Decode, DecodeWithMemTracking, Encode, HasCompact, MaxEncodedLen};
use codec::{
Decode, DecodeWithMemTracking, Encode, EncodeLike, HasCompact, Input, MaxEncodedLen, Output,
};
use frame_election_provider_support::ElectionProvider;
use frame_support::{
defensive, defensive_assert,
Expand Down Expand Up @@ -653,7 +655,7 @@ impl<T: Config> StakingLedger<T> {
// first we try to remove stake from active
if self.active >= to_withdraw {
self.active -= to_withdraw;
return self
return self;
} else {
withdrawn += self.active;
self.active = BalanceOf::<T>::zero();
Expand All @@ -671,7 +673,7 @@ impl<T: Config> StakingLedger<T> {
}

if withdrawn >= to_withdraw {
break
break;
}
}

Expand All @@ -698,7 +700,7 @@ impl<T: Config> StakingLedger<T> {
}

if unlocking_balance >= value {
break
break;
}
}

Expand Down Expand Up @@ -735,7 +737,7 @@ impl<T: Config> StakingLedger<T> {
slash_era: EraIndex,
) -> BalanceOf<T> {
if slash_amount.is_zero() {
return Zero::zero()
return Zero::zero();
}

use sp_runtime::PerThing as _;
Expand Down Expand Up @@ -828,15 +830,15 @@ impl<T: Config> StakingLedger<T> {
let mut slashed_unlocking = BTreeMap::<_, _>::new();
for i in slash_chunks_priority {
if remaining_slash.is_zero() {
break
break;
}

if let Some(chunk) = self.unlocking.get_mut(i).defensive() {
slash_out_of(&mut chunk.value, &mut remaining_slash);
// write the new slashed value of this chunk to the map.
slashed_unlocking.insert(chunk.era, chunk.value);
} else {
break
break;
}
}

Expand Down Expand Up @@ -1116,8 +1118,10 @@ impl<T: Config> Convert<T::AccountId, Option<T::AccountId>> for StashOf<T> {
///
/// Active exposure is the exposure of the validator set currently validating, i.e. in
/// `active_era`. It can differ from the latest planned exposure in `current_era`.
#[deprecated(note = "Use `ExistenceOf` or `ExistenceOrLegacyExposureOf` instead")]
pub struct ExposureOf<T>(core::marker::PhantomData<T>);

#[allow(deprecated)]
impl<T: Config> Convert<T::AccountId, Option<Exposure<T::AccountId, BalanceOf<T>>>>
for ExposureOf<T>
{
Expand All @@ -1127,10 +1131,64 @@ impl<T: Config> Convert<T::AccountId, Option<Exposure<T::AccountId, BalanceOf<T>
}
}

pub struct NullIdentity;
impl<T> Convert<T, Option<()>> for NullIdentity {
fn convert(_: T) -> Option<()> {
Some(())
/// A marker type representing the presence of a validator. Encodes as a unit type.
pub type Existence = ();
Copy link
Contributor

@kianenigma kianenigma Mar 16, 2025

Choose a reason for hiding this comment

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

fwiw this is a type alias to (), what we usually call a marker type is actually a different type, which doesn't inherit any of the impls of the original type, such as pub struct Existence


/// A converter type that returns `Some(())` if the validator exists in the current active era,
/// otherwise `None`. This serves as a lightweight presence check for validators.
pub struct ExistenceOf<T>(core::marker::PhantomData<T>);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should move this to stkaing-next branch, right?

staking-classic only needs ExistenceOrLegacyExposureOf.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need them only in staking-classic, right? Staking-next does not interact with historical sessions atleast directly?

We don't strictly need ExistenceOf. But seemed a nice thing to have. New solo chains can choose ExistenceOf instead of ExistenceOrLegacyExposureOf .

impl<T: Config> Convert<T::AccountId, Option<Existence>> for ExistenceOf<T> {
fn convert(validator: T::AccountId) -> Option<Existence> {
Validators::<T>::contains_key(&validator).then_some(())
}
}

/// A compatibility wrapper type used to represent the presence of a validator in the current era.
/// Encodes as type [`Existence`] but can decode from legacy [`Exposure`] values for backward
/// compatibility.
#[derive(PartialEq, Eq, PartialOrd, Ord, Clone, RuntimeDebug, TypeInfo, DecodeWithMemTracking)]
pub enum ExistenceOrLegacyExposure<A, B: HasCompact> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could have just used Option but this is more readable.

/// Validator exists in the current era.
Exists,
/// Legacy `Exposure` data, retained for decoding compatibility.
Exposure(Exposure<A, B>),
}

/// Converts a validator account ID to a Some([`ExistenceOrLegacyExposure::Exists`]) if the
/// validator exists in the current era, otherwise `None`.
pub struct ExistenceOrLegacyExposureOf<T>(core::marker::PhantomData<T>);

impl<T: Config> Convert<T::AccountId, Option<ExistenceOrLegacyExposure<T::AccountId, BalanceOf<T>>>>
for ExistenceOrLegacyExposureOf<T>
{
fn convert(
validator: T::AccountId,
) -> Option<ExistenceOrLegacyExposure<T::AccountId, BalanceOf<T>>> {
Validators::<T>::contains_key(&validator).then_some(ExistenceOrLegacyExposure::Exists)
}
}

impl<A, B: HasCompact> Encode for ExistenceOrLegacyExposure<A, B> {
fn encode_to<T: Output + ?Sized>(&self, _: &mut T) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should encode it the same as Exposure<A, B> right?

}

impl<A, B: HasCompact> MaxEncodedLen for ExistenceOrLegacyExposure<A, B> {
fn max_encoded_len() -> usize {
0
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
0
Exposure::<A, B>::max_encoded_len()

}
}

impl<A, B: HasCompact> EncodeLike for ExistenceOrLegacyExposure<A, B> {}

impl<A, B: HasCompact> Decode for ExistenceOrLegacyExposure<A, B>
where
Exposure<A, B>: Decode,
{
fn decode<I: Input>(input: &mut I) -> Result<Self, codec::Error> {
match input.remaining_len() {
Ok(Some(x)) if x > 0 => Ok(ExistenceOrLegacyExposure::Exposure(Decode::decode(input)?)),
_ => Ok(ExistenceOrLegacyExposure::Exists),
}
}
}

Expand Down Expand Up @@ -1278,7 +1336,7 @@ impl<T: Config> EraInfo<T> {
if claimed_pages.contains(&page) {
defensive!("Trying to set an already claimed reward");
// nevertheless don't do anything since the page already exist in claimed rewards.
return
return;
}

// add page to claimed entries
Expand Down Expand Up @@ -1418,3 +1476,55 @@ impl BenchmarkingConfig for TestBenchmarkingConfig {
type MaxValidators = frame_support::traits::ConstU32<100>;
type MaxNominators = frame_support::traits::ConstU32<100>;
}

#[cfg(test)]
mod test {
use crate::ExistenceOrLegacyExposure;
use codec::{Decode, Encode};
use sp_staking::{Exposure, IndividualExposure};

#[test]
fn existence_encodes_decodes_correctly() {
let encoded_existence = ExistenceOrLegacyExposure::<u32, u32>::Exists.encode();
assert!(encoded_existence.is_empty());

// try decoding the existence
let decoded_existence =
ExistenceOrLegacyExposure::<u32, u32>::decode(&mut encoded_existence.as_slice())
.unwrap();
assert!(matches!(decoded_existence, ExistenceOrLegacyExposure::Exists));

// check that round-trip encoding works
assert_eq!(encoded_existence, decoded_existence.encode());
}

#[test]
fn legacy_existence_encodes_decodes_correctly() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Also you are missing a test to make sure () can be decoded as ExistenceOrLegacyExposure

let legacy_exposure = Exposure::<u32, u32> {
total: 1,
own: 2,
others: vec![IndividualExposure { who: 3, value: 4 }],
};

let encoded_legacy_exposure = legacy_exposure.encode();

// try decoding the legacy exposure
let decoded_legacy_exposure =
ExistenceOrLegacyExposure::<u32, u32>::decode(&mut encoded_legacy_exposure.as_slice())
.unwrap();
assert!(matches!(
decoded_legacy_exposure,
ExistenceOrLegacyExposure::Exposure(Exposure {
total: 1,
own: 2,
others: ref i
}) if *i == vec![IndividualExposure { who: 3, value: 4 }]
));
Comment on lines +1515 to +1522
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assert!(matches!(
decoded_legacy_exposure,
ExistenceOrLegacyExposure::Exposure(Exposure {
total: 1,
own: 2,
others: ref i
}) if *i == vec![IndividualExposure { who: 3, value: 4 }]
));
assert_eq!(
decoded_legacy_exposure,
ExistenceOrLegacyExposure::Exposure(Exposure {
total: 1,
own: 2,
others: vec![IndividualExposure { who: 3, value: 4 }]
}
));

nit: I think marches! gives worst errors messages + is harder to write and read, unless if you want to skip some fields, which makes it worthwhile.


// encoding again removes the exposure.
assert_eq!(
ExistenceOrLegacyExposure::<u32, u32>::Exists.encode(),
decoded_legacy_exposure.encode()
);
}
}
Loading
Loading