From b7253a24ef66202d2958f41d6ef0dda3640c934d Mon Sep 17 00:00:00 2001 From: krayt78 <ludovic.domingues96@gmail.com> Date: Tue, 18 Mar 2025 09:44:37 +0100 Subject: [PATCH 1/5] First draft --- substrate/frame/session/README.md | 10 +- substrate/frame/session/src/lib.rs | 48 +++++++++- substrate/frame/session/src/mock.rs | 133 ++++++++++++++++++++++++++- substrate/frame/session/src/tests.rs | 42 ++++++++- 4 files changed, 226 insertions(+), 7 deletions(-) diff --git a/substrate/frame/session/README.md b/substrate/frame/session/README.md index 5a063bffee0b1..6faecb6504d16 100644 --- a/substrate/frame/session/README.md +++ b/substrate/frame/session/README.md @@ -23,10 +23,12 @@ same as the account ID. For staking systems using a stash/controller model, the ID of the controller. - **Session key configuration process:** Session keys are set using `set_keys` for use not in the next session, but the session after next. They are stored in `NextKeys`, a mapping between the caller's `ValidatorId` and the session keys -provided. `set_keys` allows users to set their session key prior to being selected as validator. It is a public call -since it uses `ensure_signed`, which checks that the origin is a signed account. As such, the account ID of the origin -stored in `NextKeys` may not necessarily be associated with a block author or a validator. The session keys of accounts -are removed once their account balance is zero. +provided. `set_keys` allows users to set their session key prior to being selected as validator. When setting keys, a deposit +is required and reserved from the account. The account must have sufficient funds available for this deposit, or the +operation will fail. This deposit is returned when the keys are purged. `set_keys` is a public call since it uses +`ensure_signed`, which checks that the origin is a signed account. As such, the account ID of the origin stored in +`NextKeys` may not necessarily be associated with a block author or a validator. The session keys of accounts are removed +once their account balance is zero. - **Session length:** This pallet does not assume anything about the length of each session. Rather, it relies on an implementation of `ShouldEndSession` to dictate a new session's start. This pallet provides the `PeriodicSessions` struct for simple periodic sessions. diff --git a/substrate/frame/session/src/lib.rs b/substrate/frame/session/src/lib.rs index a80a2b235757b..7ae70ade200d0 100644 --- a/substrate/frame/session/src/lib.rs +++ b/substrate/frame/session/src/lib.rs @@ -130,7 +130,7 @@ use frame_support::{ ensure, traits::{ Defensive, EstimateNextNewSession, EstimateNextSessionRotation, FindAuthor, Get, - OneSessionHandler, ValidatorRegistration, ValidatorSet, + OneSessionHandler, ValidatorRegistration, ValidatorSet, Currency, ReservableCurrency, }, weights::Weight, Parameter, @@ -436,6 +436,13 @@ pub mod pallet { /// Weight information for extrinsics in this pallet. type WeightInfo: WeightInfo; + + /// The currency type to reserve when setting keys. + type Currency: ReservableCurrency<Self::AccountId>; + + /// The amount to be reserved when setting keys. + #[pallet::constant] + type KeyDeposit: Get<<Self::Currency as Currency<Self::AccountId>>::Balance>; } #[pallet::genesis_config] @@ -558,6 +565,10 @@ pub mod pallet { ValidatorDisabled { validator: T::ValidatorId }, /// Validator has been re-enabled. ValidatorReenabled { validator: T::ValidatorId }, + /// Funds have been reserved for setting keys. + KeysFundsReserved { account: T::AccountId, amount: <<T as Config>::Currency as Currency<T::AccountId>>::Balance }, + /// Funds have been unreserved when purging keys. + KeysFundsUnreserved { account: T::AccountId, amount: <<T as Config>::Currency as Currency<T::AccountId>>::Balance }, } /// Error for the session pallet. @@ -573,6 +584,8 @@ pub mod pallet { NoKeys, /// Key setting account is not live, so it's impossible to associate keys. NoAccount, + /// Insufficient funds for setting keys. + InsufficientFunds, } #[pallet::hooks] @@ -836,10 +849,32 @@ impl<T: Config> Pallet<T> { .ok_or(Error::<T>::NoAssociatedValidatorId)?; ensure!(frame_system::Pallet::<T>::can_inc_consumer(account), Error::<T>::NoAccount); + + // Check if this will be a new registration + let is_new_registration = Self::load_keys(&who).is_none(); + + // For new registrations, ensure the account has enough funds for the deposit + if is_new_registration { + let deposit = T::KeyDeposit::get(); + ensure!(T::Currency::can_reserve(account, deposit), Error::<T>::InsufficientFunds); + } + + // Now that we've checked funds, proceed with setting keys let old_keys = Self::inner_set_keys(&who, keys)?; + + // Reserve deposit if this is a new registration (no old keys) if old_keys.is_none() { + let deposit = T::KeyDeposit::get(); + T::Currency::reserve(account, deposit)?; + let assertion = frame_system::Pallet::<T>::inc_consumers(account).is_ok(); debug_assert!(assertion, "can_inc_consumer() returned true; no change since; qed"); + + // Emit an event for the reserved funds + Self::deposit_event(Event::<T>::KeysFundsReserved { + account: account.clone(), + amount: deposit, + }); } Ok(()) @@ -898,6 +933,17 @@ impl<T: Config> Pallet<T> { let key_data = old_keys.get_raw(*id); Self::clear_key_owner(*id, key_data); } + + // Unreserve the deposit + let deposit = T::KeyDeposit::get(); + let _ = T::Currency::unreserve(account, deposit); + + // Emit an event for the unreserved funds + Self::deposit_event(Event::<T>::KeysFundsUnreserved { + account: account.clone(), + amount: deposit, + }); + frame_system::Pallet::<T>::dec_consumers(account); Ok(()) diff --git a/substrate/frame/session/src/mock.rs b/substrate/frame/session/src/mock.rs index ac8f9d320d75a..5fe53c861c7ee 100644 --- a/substrate/frame/session/src/mock.rs +++ b/substrate/frame/session/src/mock.rs @@ -29,7 +29,9 @@ use sp_runtime::{impl_opaque_keys, testing::UintAuthorityId, BuildStorage}; use sp_staking::SessionIndex; use sp_state_machine::BasicExternalities; -use frame_support::{derive_impl, parameter_types, traits::ConstU64}; +use frame_support::{ + derive_impl, parameter_types, traits::{ConstU64, WithdrawReasons, Currency, ReservableCurrency, SignedImbalance} +}; impl_opaque_keys! { pub struct MockSessionKeys { @@ -102,6 +104,8 @@ parameter_types! { // Stores if `on_before_session_end` was called pub static BeforeSessionEndCalled: bool = false; pub static ValidatorAccounts: BTreeMap<u64, u64> = BTreeMap::new(); + pub static CurrencyBalance: u64 = 100; + pub const KeyDeposit: u64 = 10; } pub struct TestShouldEndSession; @@ -267,6 +271,8 @@ impl Config for Test { type DisablingStrategy = disabling::UpToLimitWithReEnablingDisablingStrategy<DISABLING_LIMIT_FACTOR>; type WeightInfo = (); + type Currency = pallet_balances::Pallet<Test>; + type KeyDeposit = KeyDeposit; } #[cfg(feature = "historical")] @@ -274,3 +280,128 @@ impl crate::historical::Config for Test { type FullIdentification = u64; type FullIdentificationOf = sp_runtime::traits::ConvertInto; } + +mod pallet_balances { + use super::*; + use frame_support::pallet_prelude::*; + + pub struct Pallet<T>(core::marker::PhantomData<T>); + + impl<T> Currency<u64> for Pallet<T> { + type Balance = u64; + type PositiveImbalance = (); + type NegativeImbalance = (); + + fn total_balance(_: &u64) -> Self::Balance { + CurrencyBalance::get() + } + + fn can_slash(_: &u64, _: Self::Balance) -> bool { + true + } + + fn total_issuance() -> Self::Balance { + 0 + } + + fn minimum_balance() -> Self::Balance { + 0 + } + + fn burn(_: Self::Balance) -> Self::PositiveImbalance { + () + } + + fn issue(_: Self::Balance) -> Self::NegativeImbalance { + () + } + + fn free_balance(_: &u64) -> Self::Balance { + CurrencyBalance::get() + } + + fn ensure_can_withdraw( + _: &u64, + _: Self::Balance, + _: WithdrawReasons, + _: Self::Balance, + ) -> DispatchResult { + Ok(()) + } + + fn transfer( + _: &u64, + _: &u64, + _: Self::Balance, + _: frame_support::traits::ExistenceRequirement, + ) -> DispatchResult { + Ok(()) + } + + fn slash(_: &u64, _: Self::Balance) -> (Self::NegativeImbalance, Self::Balance) { + ((), 0) + } + + fn deposit_into_existing(_: &u64, _: Self::Balance) -> Result<Self::PositiveImbalance, DispatchError> { + Ok(()) + } + + fn deposit_creating(_: &u64, _: Self::Balance) -> Self::PositiveImbalance { + () + } + + fn withdraw( + _: &u64, + _: Self::Balance, + _: WithdrawReasons, + _: frame_support::traits::ExistenceRequirement, + ) -> Result<Self::NegativeImbalance, DispatchError> { + Ok(()) + } + + fn make_free_balance_be( + _: &u64, + _: Self::Balance, + ) -> SignedImbalance<Self::Balance, Self::PositiveImbalance> { + frame_support::traits::SignedImbalance::Positive(()) + } + } + + impl<T> ReservableCurrency<u64> for Pallet<T> { + fn can_reserve(who: &u64, amount: Self::Balance) -> bool { + // Account 999 is special and always has insufficient funds for testing + if *who == 999 { + return false + } + CurrencyBalance::get() >= amount + } + + fn reserved_balance(_: &u64) -> Self::Balance { + 0 + } + + fn reserve(who: &u64, amount: Self::Balance) -> DispatchResult { + if !Self::can_reserve(who, amount) { + return Err(DispatchError::Other("InsufficientBalance")) + } + Ok(()) + } + + fn unreserve(_: &u64, _: Self::Balance) -> Self::Balance { + 0 + } + + fn slash_reserved(_: &u64, _: Self::Balance) -> (Self::NegativeImbalance, Self::Balance) { + ((), 0) + } + + fn repatriate_reserved( + _: &u64, + _: &u64, + _: Self::Balance, + _: frame_support::traits::BalanceStatus, + ) -> Result<Self::Balance, DispatchError> { + Ok(0) + } + } +} diff --git a/substrate/frame/session/src/tests.rs b/substrate/frame/session/src/tests.rs index 42aeb8e14c364..79db2564b395a 100644 --- a/substrate/frame/session/src/tests.rs +++ b/substrate/frame/session/src/tests.rs @@ -22,7 +22,7 @@ use crate::mock::{ authorities, before_session_end_called, force_new_session, new_test_ext, reset_before_session_end_called, session_changed, set_next_validators, set_session_length, PreUpgradeMockSessionKeys, RuntimeOrigin, Session, SessionChanged, System, Test, - TestSessionChanged, TestValidatorIdOf, + TestSessionChanged, TestValidatorIdOf, MockSessionKeys, ValidatorAccounts, }; use codec::Decode; @@ -482,3 +482,43 @@ fn test_migration_v1() { crate::migrations::historical::post_migrate::<Test, Historical>(); }); } + +#[test] +fn set_keys_should_fail_with_insufficient_funds() { + new_test_ext().execute_with(|| { + // Account 999 is mocked to always have insufficient funds + let account_id = 999; + let keys = MockSessionKeys { dummy: UintAuthorityId(account_id).into() }; + frame_system::Pallet::<Test>::inc_providers(&account_id); + // Make sure we have a validator ID + ValidatorAccounts::mutate(|m| { + m.insert(account_id, account_id); + }); + + // Attempt to set keys with an account that has insufficient funds + let res = Session::set_keys(RuntimeOrigin::signed(account_id), keys, vec![]); + // Should fail with InsufficientFunds error + assert_noop!(res, Error::<Test>::InsufficientFunds); + }); +} + +#[test] +fn set_keys_should_reserve_funds() { + new_test_ext().execute_with(|| { + // Account 1000 is mocked to have sufficient funds + let account_id = 1000; + let keys = MockSessionKeys { dummy: UintAuthorityId(account_id).into() }; + + // Make sure we have a validator ID + ValidatorAccounts::mutate(|m| { + m.insert(account_id, account_id); + }); + + // Attempt to set keys + let res = Session::set_keys(RuntimeOrigin::signed(account_id), keys, vec![]); + assert_ok!(res); + + // Check that the funds were reserved + assert_eq!(frame_system::Pallet::<Test>::providers(&account_id), Some(1)); + }); +} From da7274627461045573302ad1be06624e95160a84 Mon Sep 17 00:00:00 2001 From: krayt78 <ludovic.domingues96@gmail.com> Date: Tue, 18 Mar 2025 10:39:54 +0100 Subject: [PATCH 2/5] Implement reserved balance tracking for test accounts in the session pallet. Update KeyDeposit type to use BalanceOf. Add tests for setting and purging keys to verify fund reservation and unreservation. --- substrate/frame/session/src/lib.rs | 5 ++- substrate/frame/session/src/mock.rs | 32 +++++++++++++++--- substrate/frame/session/src/tests.rs | 50 +++++++++++++++++++++++++--- 3 files changed, 77 insertions(+), 10 deletions(-) diff --git a/substrate/frame/session/src/lib.rs b/substrate/frame/session/src/lib.rs index 7ae70ade200d0..36e2c5773d0c5 100644 --- a/substrate/frame/session/src/lib.rs +++ b/substrate/frame/session/src/lib.rs @@ -442,9 +442,12 @@ pub mod pallet { /// The amount to be reserved when setting keys. #[pallet::constant] - type KeyDeposit: Get<<Self::Currency as Currency<Self::AccountId>>::Balance>; + type KeyDeposit: Get<BalanceOf<Self>>; } + // Add a type alias for the balance + type BalanceOf<T> = <<T as Config>::Currency as Currency<<T as frame_system::Config>::AccountId>>::Balance; + #[pallet::genesis_config] #[derive(frame_support::DefaultNoBound)] pub struct GenesisConfig<T: Config> { diff --git a/substrate/frame/session/src/mock.rs b/substrate/frame/session/src/mock.rs index 5fe53c861c7ee..e81a59f2c4fd9 100644 --- a/substrate/frame/session/src/mock.rs +++ b/substrate/frame/session/src/mock.rs @@ -106,6 +106,8 @@ parameter_types! { pub static ValidatorAccounts: BTreeMap<u64, u64> = BTreeMap::new(); pub static CurrencyBalance: u64 = 100; pub const KeyDeposit: u64 = 10; + // Track reserved balances for test accounts + pub static ReservedBalances: BTreeMap<u64, u64> = BTreeMap::new(); } pub struct TestShouldEndSession; @@ -281,7 +283,7 @@ impl crate::historical::Config for Test { type FullIdentificationOf = sp_runtime::traits::ConvertInto; } -mod pallet_balances { +pub mod pallet_balances { use super::*; use frame_support::pallet_prelude::*; @@ -376,19 +378,39 @@ mod pallet_balances { CurrencyBalance::get() >= amount } - fn reserved_balance(_: &u64) -> Self::Balance { - 0 + fn reserved_balance(who: &u64) -> Self::Balance { + ReservedBalances::get().get(who).cloned().unwrap_or(0) } fn reserve(who: &u64, amount: Self::Balance) -> DispatchResult { if !Self::can_reserve(who, amount) { return Err(DispatchError::Other("InsufficientBalance")) } + + // Update the reserved balance + ReservedBalances::mutate(|balances| { + let reserved = balances.entry(*who).or_insert(0); + *reserved += amount; + }); + Ok(()) } - fn unreserve(_: &u64, _: Self::Balance) -> Self::Balance { - 0 + fn unreserve(who: &u64, amount: Self::Balance) -> Self::Balance { + // Get the current reserved amount + let mut remaining = amount; + ReservedBalances::mutate(|balances| { + let reserved = balances.entry(*who).or_insert(0); + if *reserved >= amount { + *reserved -= amount; + remaining = 0; + } else { + remaining = amount - *reserved; + *reserved = 0; + } + }); + + remaining } fn slash_reserved(_: &u64, _: Self::Balance) -> (Self::NegativeImbalance, Self::Balance) { diff --git a/substrate/frame/session/src/tests.rs b/substrate/frame/session/src/tests.rs index 79db2564b395a..0807cf8ace363 100644 --- a/substrate/frame/session/src/tests.rs +++ b/substrate/frame/session/src/tests.rs @@ -22,7 +22,7 @@ use crate::mock::{ authorities, before_session_end_called, force_new_session, new_test_ext, reset_before_session_end_called, session_changed, set_next_validators, set_session_length, PreUpgradeMockSessionKeys, RuntimeOrigin, Session, SessionChanged, System, Test, - TestSessionChanged, TestValidatorIdOf, MockSessionKeys, ValidatorAccounts, + TestSessionChanged, TestValidatorIdOf, MockSessionKeys, ValidatorAccounts, KeyDeposit, }; use codec::Decode; @@ -31,7 +31,7 @@ use sp_runtime::testing::UintAuthorityId; use frame_support::{ assert_noop, assert_ok, - traits::{ConstU64, OnInitialize}, + traits::{ConstU64, OnInitialize, ReservableCurrency}, }; fn initialize_block(block: u64) { @@ -508,17 +508,59 @@ fn set_keys_should_reserve_funds() { // Account 1000 is mocked to have sufficient funds let account_id = 1000; let keys = MockSessionKeys { dummy: UintAuthorityId(account_id).into() }; + let deposit = KeyDeposit::get(); // Make sure we have a validator ID ValidatorAccounts::mutate(|m| { m.insert(account_id, account_id); }); - // Attempt to set keys + // Check the reserved balance before setting keys + let reserved_balance_before = crate::mock::pallet_balances::Pallet::<Test>::reserved_balance(&account_id); + + // Ensure system providers are properly set for the test account + frame_system::Pallet::<Test>::inc_providers(&account_id); + + // Set keys and check the operation succeeds let res = Session::set_keys(RuntimeOrigin::signed(account_id), keys, vec![]); assert_ok!(res); // Check that the funds were reserved - assert_eq!(frame_system::Pallet::<Test>::providers(&account_id), Some(1)); + let reserved_balance_after = crate::mock::pallet_balances::Pallet::<Test>::reserved_balance(&account_id); + assert_eq!(reserved_balance_after, reserved_balance_before + deposit); + }); +} + +#[test] +fn purge_keys_should_unreserve_funds() { + new_test_ext().execute_with(|| { + // Account 1000 is mocked to have sufficient funds + let account_id = 1000; + let keys = MockSessionKeys { dummy: UintAuthorityId(account_id).into() }; + let deposit = KeyDeposit::get(); + + // Make sure we have a validator ID + ValidatorAccounts::mutate(|m| { + m.insert(account_id, account_id); + }); + + // Ensure system providers are properly set for the test account + frame_system::Pallet::<Test>::inc_providers(&account_id); + + // First set the keys to reserve the deposit + let res = Session::set_keys(RuntimeOrigin::signed(account_id), keys, vec![]); + assert_ok!(res); + + // Check the reserved balance after setting keys + let reserved_balance_before_purge = crate::mock::pallet_balances::Pallet::<Test>::reserved_balance(&account_id); + assert!(reserved_balance_before_purge >= deposit, "Deposit should be reserved after setting keys"); + + // Now purge the keys + let res = Session::purge_keys(RuntimeOrigin::signed(account_id)); + assert_ok!(res); + + // Check that the funds were unreserved + let reserved_balance_after_purge = crate::mock::pallet_balances::Pallet::<Test>::reserved_balance(&account_id); + assert_eq!(reserved_balance_after_purge, reserved_balance_before_purge - deposit); }); } From 691e38d0ec8c79f1657662d58486190b431354b8 Mon Sep 17 00:00:00 2001 From: krayt78 <ludovic.domingues96@gmail.com> Date: Tue, 18 Mar 2025 11:18:19 +0100 Subject: [PATCH 3/5] Update inner_set_keys to accept old keys as an argument to reduce redundant storage reads and improve efficiency in the session pallet. --- substrate/frame/session/src/lib.rs | 40 +++++++++++++++--------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/substrate/frame/session/src/lib.rs b/substrate/frame/session/src/lib.rs index 36e2c5773d0c5..699f715d93255 100644 --- a/substrate/frame/session/src/lib.rs +++ b/substrate/frame/session/src/lib.rs @@ -484,7 +484,7 @@ pub mod pallet { for (account, val, keys) in self.keys.iter().chain(self.non_authority_keys.iter()).cloned() { - Pallet::<T>::inner_set_keys(&val, keys) + Pallet::<T>::inner_set_keys(&val, keys, None) .expect("genesis config must not contain duplicates; qed"); if frame_system::Pallet::<T>::inc_consumers_without_limit(&account).is_err() { // This will leak a provider reference, however it only happens once (at @@ -853,8 +853,9 @@ impl<T: Config> Pallet<T> { ensure!(frame_system::Pallet::<T>::can_inc_consumer(account), Error::<T>::NoAccount); - // Check if this will be a new registration - let is_new_registration = Self::load_keys(&who).is_none(); + // Load keys once to avoid redundant storage reads + let old_keys = Self::load_keys(&who); + let is_new_registration = old_keys.is_none(); // For new registrations, ensure the account has enough funds for the deposit if is_new_registration { @@ -863,10 +864,11 @@ impl<T: Config> Pallet<T> { } // Now that we've checked funds, proceed with setting keys - let old_keys = Self::inner_set_keys(&who, keys)?; + // Pass old_keys to avoid another storage read + Self::inner_set_keys(&who, keys, old_keys)?; - // Reserve deposit if this is a new registration (no old keys) - if old_keys.is_none() { + // Reserve deposit if this is a new registration + if is_new_registration { let deposit = T::KeyDeposit::get(); T::Currency::reserve(account, deposit)?; @@ -892,35 +894,33 @@ impl<T: Config> Pallet<T> { fn inner_set_keys( who: &T::ValidatorId, keys: T::Keys, + old_keys_opt: Option<T::Keys>, ) -> Result<Option<T::Keys>, DispatchError> { - let old_keys = Self::load_keys(who); - + // First, check for duplicates without modifying storage for id in T::Keys::key_ids() { let key = keys.get_raw(*id); - - // ensure keys are without duplication. ensure!( Self::key_owner(*id, key).map_or(true, |owner| &owner == who), Error::<T>::DuplicatedKey, ); } - + + // After all duplicate checks have passed, update storage for id in T::Keys::key_ids() { let key = keys.get_raw(*id); - - if let Some(old) = old_keys.as_ref().map(|k| k.get_raw(*id)) { - if key == old { - continue + + if let Some(old_key) = old_keys_opt.as_ref().map(|k| k.get_raw(*id)) { + if key != old_key { + Self::clear_key_owner(*id, old_key); + Self::put_key_owner(*id, key, who); } - - Self::clear_key_owner(*id, old); + } else { + Self::put_key_owner(*id, key, who); } - - Self::put_key_owner(*id, key, who); } Self::put_keys(who, &keys); - Ok(old_keys) + Ok(old_keys_opt) } fn do_purge_keys(account: &T::AccountId) -> DispatchResult { From 734ffc67cbe8f79c47cdacd88b8698e4cc86a993 Mon Sep 17 00:00:00 2001 From: krayt78 <ludovic.domingues96@gmail.com> Date: Tue, 18 Mar 2025 12:18:00 +0100 Subject: [PATCH 4/5] Update session pallet to use NamedReservableCurrency for key reservations --- substrate/frame/session/src/lib.rs | 30 ++++-- substrate/frame/session/src/mock.rs | 155 +++++++++++++++++++++++++--- 2 files changed, 166 insertions(+), 19 deletions(-) diff --git a/substrate/frame/session/src/lib.rs b/substrate/frame/session/src/lib.rs index 699f715d93255..5224fcb8cd0a1 100644 --- a/substrate/frame/session/src/lib.rs +++ b/substrate/frame/session/src/lib.rs @@ -130,7 +130,8 @@ use frame_support::{ ensure, traits::{ Defensive, EstimateNextNewSession, EstimateNextSessionRotation, FindAuthor, Get, - OneSessionHandler, ValidatorRegistration, ValidatorSet, Currency, ReservableCurrency, + OneSessionHandler, ValidatorRegistration, ValidatorSet, Currency, NamedReservableCurrency, + ReservableCurrency, }, weights::Weight, Parameter, @@ -397,6 +398,10 @@ pub mod pallet { #[pallet::without_storage_info] pub struct Pallet<T>(_); + /// A simple identifier for session keys. + #[derive(codec::Encode, codec::Decode, codec::MaxEncodedLen, scale_info::TypeInfo, Debug, PartialEq, Eq, Clone)] + pub struct SessionKeysReserveId; + #[pallet::config] pub trait Config: frame_system::Config { /// The overarching event type. @@ -437,8 +442,11 @@ pub mod pallet { /// Weight information for extrinsics in this pallet. type WeightInfo: WeightInfo; - /// The currency type to reserve when setting keys. - type Currency: ReservableCurrency<Self::AccountId>; + /// The currency type for reserving when setting keys. + type Currency: NamedReservableCurrency<Self::AccountId>; + + /// The reserve identifier type. + type ReserveIdentifier: Get<<Self::Currency as NamedReservableCurrency<Self::AccountId>>::ReserveIdentifier> + TypeInfo + 'static; /// The amount to be reserved when setting keys. #[pallet::constant] @@ -860,7 +868,13 @@ impl<T: Config> Pallet<T> { // For new registrations, ensure the account has enough funds for the deposit if is_new_registration { let deposit = T::KeyDeposit::get(); - ensure!(T::Currency::can_reserve(account, deposit), Error::<T>::InsufficientFunds); + let _= T::ReserveIdentifier::get(); + // Since NamedReservableCurrency doesn't directly expose can_reserve, + // we need to use the normal reserve method for checking + ensure!( + <T::Currency as ReservableCurrency<_>>::can_reserve(account, deposit), + Error::<T>::InsufficientFunds + ); } // Now that we've checked funds, proceed with setting keys @@ -870,7 +884,8 @@ impl<T: Config> Pallet<T> { // Reserve deposit if this is a new registration if is_new_registration { let deposit = T::KeyDeposit::get(); - T::Currency::reserve(account, deposit)?; + let id = T::ReserveIdentifier::get(); + T::Currency::reserve_named(&id, account, deposit)?; let assertion = frame_system::Pallet::<T>::inc_consumers(account).is_ok(); debug_assert!(assertion, "can_inc_consumer() returned true; no change since; qed"); @@ -937,9 +952,10 @@ impl<T: Config> Pallet<T> { Self::clear_key_owner(*id, key_data); } - // Unreserve the deposit + // Unreserve the deposit using the named reserve let deposit = T::KeyDeposit::get(); - let _ = T::Currency::unreserve(account, deposit); + let id = T::ReserveIdentifier::get(); + let _ = T::Currency::unreserve_named(&id, account, deposit); // Emit an event for the unreserved funds Self::deposit_event(Event::<T>::KeysFundsUnreserved { diff --git a/substrate/frame/session/src/mock.rs b/substrate/frame/session/src/mock.rs index e81a59f2c4fd9..e2c7133e28959 100644 --- a/substrate/frame/session/src/mock.rs +++ b/substrate/frame/session/src/mock.rs @@ -30,7 +30,8 @@ use sp_staking::SessionIndex; use sp_state_machine::BasicExternalities; use frame_support::{ - derive_impl, parameter_types, traits::{ConstU64, WithdrawReasons, Currency, ReservableCurrency, SignedImbalance} + derive_impl, parameter_types, traits::{ConstU64, WithdrawReasons, Currency, ReservableCurrency, + SignedImbalance, NamedReservableCurrency, BalanceStatus} }; impl_opaque_keys! { @@ -106,8 +107,8 @@ parameter_types! { pub static ValidatorAccounts: BTreeMap<u64, u64> = BTreeMap::new(); pub static CurrencyBalance: u64 = 100; pub const KeyDeposit: u64 = 10; - // Track reserved balances for test accounts - pub static ReservedBalances: BTreeMap<u64, u64> = BTreeMap::new(); + // Track reserved balances for test accounts - use Vecs for simplicity + pub static ReservedBalances: BTreeMap<u64, BTreeMap<Vec<u8>, u64>> = BTreeMap::new(); } pub struct TestShouldEndSession; @@ -258,6 +259,19 @@ impl Convert<u64, Option<u64>> for TestValidatorIdOf { // `UpToLimitWithReEnablingDisablingStrategy`` pub(crate) const DISABLING_LIMIT_FACTOR: usize = 3; +// Type to represent session keys in the test +pub type SessionKeysId = [u8; 12]; +pub const TEST_SESSION_KEYS_ID: SessionKeysId = *b"session_keys"; + +// Define TestReserveIdentifier with the necessary traits +#[derive(Debug, Clone, PartialEq, Eq, codec::Encode, codec::Decode, scale_info::TypeInfo)] +pub struct TestReserveIdentifier; +impl Get<SessionKeysId> for TestReserveIdentifier { + fn get() -> SessionKeysId { + TEST_SESSION_KEYS_ID + } +} + impl Config for Test { type ShouldEndSession = TestShouldEndSession; #[cfg(feature = "historical")] @@ -274,6 +288,7 @@ impl Config for Test { disabling::UpToLimitWithReEnablingDisablingStrategy<DISABLING_LIMIT_FACTOR>; type WeightInfo = (); type Currency = pallet_balances::Pallet<Test>; + type ReserveIdentifier = TestReserveIdentifier; type KeyDeposit = KeyDeposit; } @@ -379,7 +394,11 @@ pub mod pallet_balances { } fn reserved_balance(who: &u64) -> Self::Balance { - ReservedBalances::get().get(who).cloned().unwrap_or(0) + // Sum up all reserved balances for the account + ReservedBalances::get() + .get(who) + .map(|reserves| reserves.values().sum()) + .unwrap_or(0) } fn reserve(who: &u64, amount: Self::Balance) -> DispatchResult { @@ -387,9 +406,13 @@ pub mod pallet_balances { return Err(DispatchError::Other("InsufficientBalance")) } + // Use an empty ID for anonymous reserves + let id = Vec::new(); + // Update the reserved balance ReservedBalances::mutate(|balances| { - let reserved = balances.entry(*who).or_insert(0); + let account_reserves = balances.entry(*who).or_insert_with(BTreeMap::new); + let reserved = account_reserves.entry(id).or_insert(0); *reserved += amount; }); @@ -397,16 +420,32 @@ pub mod pallet_balances { } fn unreserve(who: &u64, amount: Self::Balance) -> Self::Balance { + // Use an empty ID for anonymous reserves + let id = Vec::new(); + // Get the current reserved amount let mut remaining = amount; ReservedBalances::mutate(|balances| { - let reserved = balances.entry(*who).or_insert(0); - if *reserved >= amount { - *reserved -= amount; - remaining = 0; - } else { - remaining = amount - *reserved; - *reserved = 0; + if let Some(account_reserves) = balances.get_mut(who) { + if let Some(reserved) = account_reserves.get_mut(&id) { + if *reserved >= amount { + *reserved -= amount; + remaining = 0; + } else { + remaining = amount - *reserved; + *reserved = 0; + } + + // Clean up empty reserves + if *reserved == 0 { + account_reserves.remove(&id); + } + } + + // Clean up empty accounts + if account_reserves.is_empty() { + balances.remove(who); + } } }); @@ -426,4 +465,96 @@ pub mod pallet_balances { Ok(0) } } + + impl<T> NamedReservableCurrency<u64> for Pallet<T> { + type ReserveIdentifier = [u8; 12]; + + fn reserved_balance_named(id: &Self::ReserveIdentifier, who: &u64) -> Self::Balance { + // Convert fixed array to Vec for lookup + let id_vec = id.to_vec(); + + ReservedBalances::get() + .get(who) + .and_then(|reserves| reserves.get(&id_vec)) + .cloned() + .unwrap_or(0) + } + + fn slash_reserved_named( + _id: &Self::ReserveIdentifier, + _who: &u64, + _amount: Self::Balance, + ) -> (Self::NegativeImbalance, Self::Balance) { + ((), 0) + } + + fn reserve_named( + id: &Self::ReserveIdentifier, + who: &u64, + amount: Self::Balance, + ) -> DispatchResult { + if !Self::can_reserve(who, amount) { + return Err(DispatchError::Other("InsufficientBalance")) + } + + // Convert fixed array to Vec for storage + let id_vec = id.to_vec(); + + // Update the reserved balance + ReservedBalances::mutate(|balances| { + let account_reserves = balances.entry(*who).or_insert_with(BTreeMap::new); + let reserved = account_reserves.entry(id_vec).or_insert(0); + *reserved += amount; + }); + + Ok(()) + } + + fn unreserve_named( + id: &Self::ReserveIdentifier, + who: &u64, + amount: Self::Balance, + ) -> Self::Balance { + // Convert fixed array to Vec for lookup/storage + let id_vec = id.to_vec(); + + // Get the current reserved amount + let mut remaining = amount; + ReservedBalances::mutate(|balances| { + if let Some(account_reserves) = balances.get_mut(who) { + if let Some(reserved) = account_reserves.get_mut(&id_vec) { + if *reserved >= amount { + *reserved -= amount; + remaining = 0; + } else { + remaining = amount - *reserved; + *reserved = 0; + } + + // Clean up empty reserves + if *reserved == 0 { + account_reserves.remove(&id_vec); + } + } + + // Clean up empty accounts + if account_reserves.is_empty() { + balances.remove(who); + } + } + }); + + remaining + } + + fn repatriate_reserved_named( + _id: &Self::ReserveIdentifier, + _slashed: &u64, + _beneficiary: &u64, + _amount: Self::Balance, + _status: BalanceStatus, + ) -> Result<Self::Balance, DispatchError> { + Ok(0) + } + } } From 5843e53217123d1ee7d46f83dd21efc10fc3a684 Mon Sep 17 00:00:00 2001 From: krayt78 <ludovic.domingues96@gmail.com> Date: Tue, 18 Mar 2025 18:01:23 +0100 Subject: [PATCH 5/5] Refactor session pallet to replace NamedReservableCurrency with hold functionality for key management. Update related types and events to reflect changes from reservation to holding of funds. --- substrate/frame/session/src/lib.rs | 63 +++++----- substrate/frame/session/src/mock.rs | 178 ++++++++++++++++------------ 2 files changed, 136 insertions(+), 105 deletions(-) diff --git a/substrate/frame/session/src/lib.rs b/substrate/frame/session/src/lib.rs index 5224fcb8cd0a1..e7ea76d778383 100644 --- a/substrate/frame/session/src/lib.rs +++ b/substrate/frame/session/src/lib.rs @@ -130,8 +130,8 @@ use frame_support::{ ensure, traits::{ Defensive, EstimateNextNewSession, EstimateNextSessionRotation, FindAuthor, Get, - OneSessionHandler, ValidatorRegistration, ValidatorSet, Currency, NamedReservableCurrency, - ReservableCurrency, + OneSessionHandler, ValidatorRegistration, ValidatorSet, + fungible::{hold::Mutate as HoldMutate, hold::Inspect as HoldInspect, Inspect}, }, weights::Weight, Parameter, @@ -398,9 +398,9 @@ pub mod pallet { #[pallet::without_storage_info] pub struct Pallet<T>(_); - /// A simple identifier for session keys. + /// A simple identifier for session keys hold reason. #[derive(codec::Encode, codec::Decode, codec::MaxEncodedLen, scale_info::TypeInfo, Debug, PartialEq, Eq, Clone)] - pub struct SessionKeysReserveId; + pub struct SessionKeysHoldReason; #[pallet::config] pub trait Config: frame_system::Config { @@ -442,19 +442,19 @@ pub mod pallet { /// Weight information for extrinsics in this pallet. type WeightInfo: WeightInfo; - /// The currency type for reserving when setting keys. - type Currency: NamedReservableCurrency<Self::AccountId>; + /// The currency type for placing holds when setting keys. + type Currency: HoldMutate<Self::AccountId> + Inspect<Self::AccountId>; - /// The reserve identifier type. - type ReserveIdentifier: Get<<Self::Currency as NamedReservableCurrency<Self::AccountId>>::ReserveIdentifier> + TypeInfo + 'static; + /// The hold reason type. + type HoldReason: Get<<Self::Currency as HoldInspect<Self::AccountId>>::Reason> + TypeInfo + 'static; - /// The amount to be reserved when setting keys. + /// The amount to be held when setting keys. #[pallet::constant] type KeyDeposit: Get<BalanceOf<Self>>; } // Add a type alias for the balance - type BalanceOf<T> = <<T as Config>::Currency as Currency<<T as frame_system::Config>::AccountId>>::Balance; + type BalanceOf<T> = <<T as Config>::Currency as Inspect<<T as frame_system::Config>::AccountId>>::Balance; #[pallet::genesis_config] #[derive(frame_support::DefaultNoBound)] @@ -576,10 +576,10 @@ pub mod pallet { ValidatorDisabled { validator: T::ValidatorId }, /// Validator has been re-enabled. ValidatorReenabled { validator: T::ValidatorId }, - /// Funds have been reserved for setting keys. - KeysFundsReserved { account: T::AccountId, amount: <<T as Config>::Currency as Currency<T::AccountId>>::Balance }, - /// Funds have been unreserved when purging keys. - KeysFundsUnreserved { account: T::AccountId, amount: <<T as Config>::Currency as Currency<T::AccountId>>::Balance }, + /// Funds have been held for setting keys. + KeysFundsHeld { account: T::AccountId, amount: BalanceOf<T> }, + /// Funds have been released when purging keys. + KeysFundsReleased { account: T::AccountId, amount: BalanceOf<T> }, } /// Error for the session pallet. @@ -868,11 +868,9 @@ impl<T: Config> Pallet<T> { // For new registrations, ensure the account has enough funds for the deposit if is_new_registration { let deposit = T::KeyDeposit::get(); - let _= T::ReserveIdentifier::get(); - // Since NamedReservableCurrency doesn't directly expose can_reserve, - // we need to use the normal reserve method for checking + let reason = T::HoldReason::get(); ensure!( - <T::Currency as ReservableCurrency<_>>::can_reserve(account, deposit), + <T::Currency as HoldInspect<_>>::can_hold(&reason, account, deposit), Error::<T>::InsufficientFunds ); } @@ -881,17 +879,17 @@ impl<T: Config> Pallet<T> { // Pass old_keys to avoid another storage read Self::inner_set_keys(&who, keys, old_keys)?; - // Reserve deposit if this is a new registration + // Place deposit on hold if this is a new registration if is_new_registration { let deposit = T::KeyDeposit::get(); - let id = T::ReserveIdentifier::get(); - T::Currency::reserve_named(&id, account, deposit)?; + let reason = T::HoldReason::get(); + T::Currency::hold(&reason, account, deposit)?; let assertion = frame_system::Pallet::<T>::inc_consumers(account).is_ok(); debug_assert!(assertion, "can_inc_consumer() returned true; no change since; qed"); - // Emit an event for the reserved funds - Self::deposit_event(Event::<T>::KeysFundsReserved { + // Emit an event for the held funds + Self::deposit_event(Event::<T>::KeysFundsHeld { account: account.clone(), amount: deposit, }); @@ -952,16 +950,17 @@ impl<T: Config> Pallet<T> { Self::clear_key_owner(*id, key_data); } - // Unreserve the deposit using the named reserve - let deposit = T::KeyDeposit::get(); - let id = T::ReserveIdentifier::get(); - let _ = T::Currency::unreserve_named(&id, account, deposit); + // Release the deposit from hold + let reason = T::HoldReason::get(); - // Emit an event for the unreserved funds - Self::deposit_event(Event::<T>::KeysFundsUnreserved { - account: account.clone(), - amount: deposit, - }); + // Use release_all to handle the case where the exact amount might not be available + if let Ok(released) = T::Currency::release_all(&reason, account, frame_support::traits::tokens::Precision::BestEffort) { + // Emit an event for the released funds + Self::deposit_event(Event::<T>::KeysFundsReleased { + account: account.clone(), + amount: released, + }); + } frame_system::Pallet::<T>::dec_consumers(account); diff --git a/substrate/frame/session/src/mock.rs b/substrate/frame/session/src/mock.rs index e2c7133e28959..77863764b8447 100644 --- a/substrate/frame/session/src/mock.rs +++ b/substrate/frame/session/src/mock.rs @@ -31,7 +31,10 @@ use sp_state_machine::BasicExternalities; use frame_support::{ derive_impl, parameter_types, traits::{ConstU64, WithdrawReasons, Currency, ReservableCurrency, - SignedImbalance, NamedReservableCurrency, BalanceStatus} + SignedImbalance, tokens::{fungible::{ + hold::{Mutate as HoldMutate, Inspect as HoldInspect, Unbalanced as UnbalancedHold}, + Inspect as FungibleInspect, Unbalanced as FungibleUnbalanced, Dust + }, Preservation, Fortitude}}, }; impl_opaque_keys! { @@ -263,10 +266,10 @@ pub(crate) const DISABLING_LIMIT_FACTOR: usize = 3; pub type SessionKeysId = [u8; 12]; pub const TEST_SESSION_KEYS_ID: SessionKeysId = *b"session_keys"; -// Define TestReserveIdentifier with the necessary traits +// Define TestHoldReason with the necessary traits #[derive(Debug, Clone, PartialEq, Eq, codec::Encode, codec::Decode, scale_info::TypeInfo)] -pub struct TestReserveIdentifier; -impl Get<SessionKeysId> for TestReserveIdentifier { +pub struct TestHoldReason; +impl Get<SessionKeysId> for TestHoldReason { fn get() -> SessionKeysId { TEST_SESSION_KEYS_ID } @@ -288,7 +291,7 @@ impl Config for Test { disabling::UpToLimitWithReEnablingDisablingStrategy<DISABLING_LIMIT_FACTOR>; type WeightInfo = (); type Currency = pallet_balances::Pallet<Test>; - type ReserveIdentifier = TestReserveIdentifier; + type HoldReason = TestHoldReason; type KeyDeposit = KeyDeposit; } @@ -301,6 +304,10 @@ impl crate::historical::Config for Test { pub mod pallet_balances { use super::*; use frame_support::pallet_prelude::*; + use frame_support::traits::{ + tokens::{WithdrawConsequence, Provenance, DepositConsequence}, + fungible::Dust, + }; pub struct Pallet<T>(core::marker::PhantomData<T>); @@ -466,12 +473,76 @@ pub mod pallet_balances { } } - impl<T> NamedReservableCurrency<u64> for Pallet<T> { - type ReserveIdentifier = [u8; 12]; + impl<T> FungibleInspect<u64> for Pallet<T> { + type Balance = u64; + + fn total_issuance() -> Self::Balance { + CurrencyBalance::get() + } + + fn minimum_balance() -> Self::Balance { + 0 + } + + fn balance(_who: &u64) -> Self::Balance { + CurrencyBalance::get() + } + + fn total_balance(_who: &u64) -> Self::Balance { + CurrencyBalance::get() + } + + fn reducible_balance( + _who: &u64, + _keep_alive: Preservation, + _force: Fortitude, + ) -> Self::Balance { + CurrencyBalance::get() + } + + fn can_deposit( + _who: &u64, + _amount: Self::Balance, + _provenance: Provenance, + ) -> DepositConsequence { + DepositConsequence::Success + } + + fn can_withdraw( + who: &u64, + amount: Self::Balance, + ) -> WithdrawConsequence<Self::Balance> { + if *who == 999 || amount > CurrencyBalance::get() { + WithdrawConsequence::BalanceLow + } else { + WithdrawConsequence::Success + } + } + } + + impl<T> FungibleUnbalanced<u64> for Pallet<T> { + fn handle_dust(_dust: Dust<u64, Self>) { + // No-op in mock + } + + fn write_balance( + _who: &u64, + _amount: Self::Balance + ) -> Result<Option<Self::Balance>, DispatchError> { + Ok(None) + } + + fn set_total_issuance(_amount: Self::Balance) { + // No-op in mock + } + } + + impl<T> HoldInspect<u64> for Pallet<T> { + type Reason = SessionKeysId; - fn reserved_balance_named(id: &Self::ReserveIdentifier, who: &u64) -> Self::Balance { + fn balance_on_hold(reason: &Self::Reason, who: &u64) -> Self::Balance { // Convert fixed array to Vec for lookup - let id_vec = id.to_vec(); + let id_vec = reason.to_vec(); ReservedBalances::get() .get(who) @@ -480,81 +551,42 @@ pub mod pallet_balances { .unwrap_or(0) } - fn slash_reserved_named( - _id: &Self::ReserveIdentifier, - _who: &u64, - _amount: Self::Balance, - ) -> (Self::NegativeImbalance, Self::Balance) { - ((), 0) + fn total_balance_on_hold(who: &u64) -> Self::Balance { + // Sum up all held balances for the account + ReservedBalances::get() + .get(who) + .map(|holds| holds.values().sum()) + .unwrap_or(0) } - fn reserve_named( - id: &Self::ReserveIdentifier, + fn can_hold(_reason: &Self::Reason, who: &u64, amount: Self::Balance) -> bool { + // Account 999 is special and always has insufficient funds for testing + if *who == 999 { + return false + } + CurrencyBalance::get() >= amount + } + } + + impl<T> UnbalancedHold<u64> for Pallet<T> { + fn set_balance_on_hold( + reason: &Self::Reason, who: &u64, amount: Self::Balance, ) -> DispatchResult { - if !Self::can_reserve(who, amount) { - return Err(DispatchError::Other("InsufficientBalance")) - } - // Convert fixed array to Vec for storage - let id_vec = id.to_vec(); + let id_vec = reason.to_vec(); - // Update the reserved balance + // Update the held balance ReservedBalances::mutate(|balances| { - let account_reserves = balances.entry(*who).or_insert_with(BTreeMap::new); - let reserved = account_reserves.entry(id_vec).or_insert(0); - *reserved += amount; + let account_holds = balances.entry(*who).or_insert_with(BTreeMap::new); + account_holds.insert(id_vec, amount); }); Ok(()) } - - fn unreserve_named( - id: &Self::ReserveIdentifier, - who: &u64, - amount: Self::Balance, - ) -> Self::Balance { - // Convert fixed array to Vec for lookup/storage - let id_vec = id.to_vec(); - - // Get the current reserved amount - let mut remaining = amount; - ReservedBalances::mutate(|balances| { - if let Some(account_reserves) = balances.get_mut(who) { - if let Some(reserved) = account_reserves.get_mut(&id_vec) { - if *reserved >= amount { - *reserved -= amount; - remaining = 0; - } else { - remaining = amount - *reserved; - *reserved = 0; - } - - // Clean up empty reserves - if *reserved == 0 { - account_reserves.remove(&id_vec); - } - } - - // Clean up empty accounts - if account_reserves.is_empty() { - balances.remove(who); - } - } - }); - - remaining - } - - fn repatriate_reserved_named( - _id: &Self::ReserveIdentifier, - _slashed: &u64, - _beneficiary: &u64, - _amount: Self::Balance, - _status: BalanceStatus, - ) -> Result<Self::Balance, DispatchError> { - Ok(0) - } } + + impl<T> HoldMutate<u64> for Pallet<T> {} } +