From 986495c3eb35c76e32e465f9425824c9720dd4b7 Mon Sep 17 00:00:00 2001 From: akildemir Date: Wed, 4 Oct 2023 12:13:02 +0300 Subject: [PATCH] fix some pr comments --- substrate/client/tests/validator_sets.rs | 2 +- substrate/node/src/chain_spec.rs | 2 +- substrate/primitives/src/networks.rs | 2 +- substrate/runtime/src/lib.rs | 2 +- substrate/staking/pallet/LICENSE | 2 +- substrate/staking/pallet/src/lib.rs | 52 +++------ substrate/validator-sets/pallet/Cargo.toml | 2 +- substrate/validator-sets/pallet/src/lib.rs | 116 +++++++++++---------- 8 files changed, 79 insertions(+), 101 deletions(-) diff --git a/substrate/client/tests/validator_sets.rs b/substrate/client/tests/validator_sets.rs index 79558d682..8e4fa3534 100644 --- a/substrate/client/tests/validator_sets.rs +++ b/substrate/client/tests/validator_sets.rs @@ -38,7 +38,7 @@ serai_test!( .get_new_set_events(serai.get_block_by_number(0).await.unwrap().unwrap().hash()) .await .unwrap(), - [NetworkId::Bitcoin, NetworkId::Ethereum, NetworkId::Monero, NetworkId::Serai] + [NetworkId::Serai, NetworkId::Bitcoin, NetworkId::Ethereum, NetworkId::Monero] .iter() .copied() .map(|network| ValidatorSetsEvent::NewSet { diff --git a/substrate/node/src/chain_spec.rs b/substrate/node/src/chain_spec.rs index 8f8883a55..fae8b462e 100644 --- a/substrate/node/src/chain_spec.rs +++ b/substrate/node/src/chain_spec.rs @@ -56,10 +56,10 @@ fn testnet_genesis( validator_sets: ValidatorSetsConfig { bond: Amount(1_000_000 * 10_u64.pow(8)), networks: vec![ + (NetworkId::Serai, NETWORKS[&NetworkId::Serai].clone()), (NetworkId::Bitcoin, NETWORKS[&NetworkId::Bitcoin].clone()), (NetworkId::Ethereum, NETWORKS[&NetworkId::Ethereum].clone()), (NetworkId::Monero, NETWORKS[&NetworkId::Monero].clone()), - (NetworkId::Serai, NETWORKS[&NetworkId::Serai].clone()), ], participants: validators.iter().map(|name| account_from_name(name)).collect(), }, diff --git a/substrate/primitives/src/networks.rs b/substrate/primitives/src/networks.rs index 10efb6571..0eb89b0e2 100644 --- a/substrate/primitives/src/networks.rs +++ b/substrate/primitives/src/networks.rs @@ -124,9 +124,9 @@ impl Network { #[cfg(feature = "std")] lazy_static::lazy_static! { pub static ref NETWORKS: HashMap = HashMap::from([ + (NetworkId::Serai, Network::new(vec![Coin::Serai]).unwrap()), (NetworkId::Bitcoin, Network::new(vec![Coin::Bitcoin]).unwrap()), (NetworkId::Ethereum, Network::new(vec![Coin::Ether, Coin::Dai]).unwrap()), (NetworkId::Monero, Network::new(vec![Coin::Monero]).unwrap()), - (NetworkId::Serai, Network::new(vec![Coin::Serai]).unwrap()), ]); } diff --git a/substrate/runtime/src/lib.rs b/substrate/runtime/src/lib.rs index 3bb405d88..b61ebe7f4 100644 --- a/substrate/runtime/src/lib.rs +++ b/substrate/runtime/src/lib.rs @@ -189,7 +189,7 @@ impl Contains for CallFilter { } if let RuntimeCall::Session(call) = call { - return matches!(call, session::Call::set_keys { .. } | session::Call::purge_keys {}); + return matches!(call, session::Call::set_keys { .. }); } false diff --git a/substrate/staking/pallet/LICENSE b/substrate/staking/pallet/LICENSE index d6e1814a2..c425427c8 100644 --- a/substrate/staking/pallet/LICENSE +++ b/substrate/staking/pallet/LICENSE @@ -1,6 +1,6 @@ AGPL-3.0-only license -Copyright (c) 2022 Luke Parker +Copyright (c) 2022-2023 Luke Parker This program is free software: you can redistribute it and/or modify it under the terms of the GNU Affero General Public License Version 3 as diff --git a/substrate/staking/pallet/src/lib.rs b/substrate/staking/pallet/src/lib.rs index b4bd9ed1e..dace32c83 100644 --- a/substrate/staking/pallet/src/lib.rs +++ b/substrate/staking/pallet/src/lib.rs @@ -13,7 +13,6 @@ pub mod pallet { use serai_primitives::{NetworkId, Amount, PublicKey}; use serai_validator_sets_primitives::{ValidatorSet, Session}; - use staking_primitives::AllocatedStaking; use validator_sets_pallet::{Config as VsConfig, Pallet as VsPallet}; use pallet_session::{Config as SessionConfig, SessionManager, Pallet as SessionPallet}; @@ -21,7 +20,7 @@ pub mod pallet { #[pallet::error] pub enum Error { BondUnavailable, - InSufficientAllocation, + InsufficientAllocation, } // TODO: Event @@ -86,7 +85,7 @@ pub mod pallet { fn deallocate_internal(account: &T::AccountId, amount: u64) -> Result<(), Error> { Allocated::::try_mutate(account, |allocated| { if *allocated < amount { - Err(Error::::InSufficientAllocation)?; + Err(Error::::InsufficientAllocation)?; } *allocated -= amount; Ok(()) @@ -109,8 +108,7 @@ pub mod pallet { Ok(()) } - /// Unstake funds from this account. Only unallocated funds may be - /// unstaked. + /// Unstake funds from this account. Only unallocated funds may be unstaked. #[pallet::call_index(1)] #[pallet::weight((0, DispatchClass::Operational))] // TODO pub fn unstake(origin: OriginFor, #[pallet::compact] amount: u64) -> DispatchResult { @@ -135,17 +133,11 @@ pub mod pallet { // add to amount bonded Self::allocate_internal(&account, amount)?; - // add to participants list for the network - let result = VsPallet::::add_participant(account, Amount(amount), network); - if result.is_err() { - Self::deallocate_internal(&account, amount).unwrap(); - return result; - } - - Ok(()) + // increase allocation for participant or add to participants list if new. + VsPallet::::increase_allocation(account, Amount(amount), network) } - /// Allocate `amount` to a given validator set. + /// Deallocate `amount` from a given validator set. #[pallet::call_index(3)] #[pallet::weight((0, DispatchClass::Operational))] // TODO pub fn deallocate( @@ -155,27 +147,20 @@ pub mod pallet { ) -> DispatchResult { let account = ensure_signed(origin)?; - // remove the participant if necessary. + // decrease allocation and remove the participant if necessary. // we can't directly deallocate here, since the leaving validator // will be removed after the next session. We only deallocate then // on `end_session` for the right index. - VsPallet::::maybe_remove_participant(account, Amount(amount), network) + VsPallet::::decrease_allocation(account, Amount(amount), network) } } - /// Call order is end_session(i - 1) -> start_session(i) -> new_session(i + 1) - /// new_session(i + 1) is called immediately after start_session(i) returns then - /// we wait until the session ends then get a call to end_session(i) and so on. + // Call order is end_session(i - 1) -> start_session(i) -> new_session(i + 1) + // new_session(i + 1) is called immediately after start_session(i) returns, + // then we wait until the session ends then get a call to end_session(i) and so on. impl SessionManager for Pallet { fn new_session(new_index: u32) -> Option> { - let next_validators = VsPallet::::next_validator_set(new_index, NetworkId::Serai); - - // Returning None will keep the previous set going. - if next_validators.is_empty() { - return None; - } - - Some(next_validators) + Some(VsPallet::::next_validator_set(new_index, NetworkId::Serai)) } fn new_session_genesis(_: u32) -> Option> { @@ -192,7 +177,7 @@ pub mod pallet { let deallocating_validators = VsPallet::::deallocating_validators(key); for (account, amount, _) in deallocating_validators { // we can unwrap because we are not deallocating more than allocated. - >::deallocate(&account, amount.0).unwrap(); + Self::deallocate_internal(&account, amount.0).unwrap(); } VsPallet::::end_session(end_index, NetworkId::Serai); @@ -203,17 +188,6 @@ pub mod pallet { VsPallet::::start_session(start_index, NetworkId::Serai, validators) } } - - impl AllocatedStaking for Pallet { - type Error = Error; - - fn allocate(account: &T::AccountId, amount: u64) -> Result<(), Error> { - Self::allocate_internal(account, amount) - } - fn deallocate(account: &T::AccountId, amount: u64) -> Result<(), Error> { - Self::deallocate_internal(account, amount) - } - } } pub use pallet::*; diff --git a/substrate/validator-sets/pallet/Cargo.toml b/substrate/validator-sets/pallet/Cargo.toml index ed3d17bce..cc78a809d 100644 --- a/substrate/validator-sets/pallet/Cargo.toml +++ b/substrate/validator-sets/pallet/Cargo.toml @@ -51,7 +51,7 @@ runtime-benchmarks = [ "frame-system/runtime-benchmarks", "frame-support/runtime-benchmarks", - "pallet-babe/runtime-benchmarks", + "pallet-babe/runtime-benchmarks", ] default = ["std"] diff --git a/substrate/validator-sets/pallet/src/lib.rs b/substrate/validator-sets/pallet/src/lib.rs index 22960ccc7..2ff1a8fe3 100644 --- a/substrate/validator-sets/pallet/src/lib.rs +++ b/substrate/validator-sets/pallet/src/lib.rs @@ -70,18 +70,15 @@ pub mod pallet { #[pallet::getter(fn keys)] pub type Keys = StorageMap<_, Twox64Concat, ValidatorSet, KeyPair, OptionQuery>; - /// The Validators that has enough bond allocated - /// and set to be joining to the validator set - /// in the next session. + /// The Validators that has enough bond allocated and set to be joining to the validator set + /// in the next session, or just allocating more bond. #[pallet::storage] - #[pallet::getter(fn joining_validators)] + #[pallet::getter(fn allocating_validators)] #[pallet::unbounded] - pub type JoiningValidators = + pub type AllocatingValidators = StorageMap<_, Twox64Concat, ValidatorSet, Vec<(T::AccountId, Amount)>, ValueQuery>; - /// The Validators that has enough bond deallocated - /// to still remain in the validator set but freed - /// some funds. + /// The Validators that want to deallocate some bond. #[pallet::storage] #[pallet::getter(fn deallocating_validators)] #[pallet::unbounded] @@ -112,7 +109,10 @@ pub mod pallet { /// Validator wasn't registered or active. NonExistentValidator, /// Trying to deallocate more than allocated. - InSufficientAllocation, + InsufficientAllocation, + /// Deallocation would remove the participant from the sent + /// since their bond would not enough. + DeallocationWouldRemoveParticipant, } #[pallet::genesis_build] @@ -153,18 +153,19 @@ pub mod pallet { impl Hooks> for Pallet { /// Called when a block is initialized. fn on_initialize(n: BlockNumberFor) -> Weight { - // fire new_set 100 blocks prior to session change. - // we can "predict" next session start by getting the - // next epoch time from babe since sessions rotate on epoch change. + // fire new_set 100 blocks prior to session change. we can "predict" next session start by + // getting the next epoch time from babe since sessions rotate on epoch change. // This is only approximation since slots != blocks. let next_start: u64 = Babe::::next_epoch().start_slot.into(); if next_start - 100 == n.saturated_into::() { - // TODO: only for serai or for all networks? - let set = ValidatorSet { - session: Session(Self::current_session().0 + 1), - network: NetworkId::Serai, - }; - Pallet::::deposit_event(Event::NewSet { set }) + // TODO: somehow primitives::networks::NETWORKS isn't visible here + let networks = + [NetworkId::Serai, NetworkId::Bitcoin, NetworkId::Ethereum, NetworkId::Monero]; + for network in networks.iter() { + let set = + ValidatorSet { session: Session(Self::current_session().0 + 1), network: *network }; + Pallet::::deposit_event(Event::NewSet { set }) + } } Weight::zero() // TODO } @@ -189,7 +190,7 @@ pub mod pallet { } /// Will set the given account to validator list if the allocated bond is enough. - pub fn add_participant( + pub fn increase_allocation( account: T::AccountId, amount: Amount, network: NetworkId, @@ -203,7 +204,7 @@ pub mod pallet { // where it will be active. let participant_set = ValidatorSet { session: Session(Self::current_session().0 + 2), network }; - JoiningValidators::::try_mutate_exists(participant_set, |existing| { + AllocatingValidators::::try_mutate_exists(participant_set, |existing| { if existing.is_some() { existing.as_mut().unwrap().push((account, amount)); } else { @@ -219,7 +220,7 @@ pub mod pallet { /// This function will set the participant for leaving if remaining bond /// falls short for the bond requirement of the set after the deallocation. /// Otherwise it will just deallocate the given `amount`. - pub fn maybe_remove_participant( + pub fn decrease_allocation( account: T::AccountId, amount: Amount, network: NetworkId, @@ -231,20 +232,24 @@ pub mod pallet { let pair = match current_set.participants.iter().find(|p| p.0 == account) { Some(p) => *p, None => { - // check whether it is still in the joining set before getting active - let joining_set = JoiningValidators::::get(key); - *joining_set.iter().find(|p| p.0 == account).ok_or(Error::::NonExistentValidator)? + // check whether it is still in the allocating set before getting active + let allocating_set = Self::allocating_validators(key); + *allocating_set.iter().find(|p| p.0 == account).ok_or(Error::::NonExistentValidator)? } }; // check validator has enough to deallocate if amount > pair.1 { - Err(Error::::InSufficientAllocation)?; + Err(Error::::InsufficientAllocation)?; } - // if the remaining bond is not enough remove from the validator set and - // deallocate all bond instead of just "amount" since they will be leaving. - let deallocation = if pair.1 - amount < current_set.bond { + // Either deallocate `amount` much or full. + let remaining = pair.1 - amount; + let deallocation = if remaining < current_set.bond { + // user didn't request to deallocate full amount but deallocation would cause so. + if remaining > Amount(0) { + Err(Error::::DeallocationWouldRemoveParticipant)?; + } (account, pair.1, true) } else { (account, amount, false) @@ -264,6 +269,7 @@ pub mod pallet { Ok(()) } + /// Returns the genesis validator set. pub fn genesis_validator_set(network: NetworkId) -> Vec { let key = ValidatorSet { session: Session(0), network }; Self::validator_set(key) @@ -274,10 +280,13 @@ pub mod pallet { .collect::>() } + /// Returns the participant set that will be active at `new_index`. pub fn next_validator_set(new_index: u32, network: NetworkId) -> Vec { let mut key = ValidatorSet { session: Session(new_index), network }; - let mut joining = - Self::joining_validators(key).iter().map(|(id, _)| *id).collect::>(); + let mut allocating = Self::allocating_validators(key) + .iter() + .map(|(id, _)| *id) + .collect::>(); // get only the ones who is leaving because of insufficient bond. let leaving = Self::deallocating_validators(key) @@ -297,17 +306,17 @@ pub mod pallet { // add new ones // - We have to add new ones before filtering out the leaving ones. - // Since some of the leaving ones are still only in the joining set. + // Since some of the leaving ones are still only in the allocating set. // These are the ones who allocated to a set and then // immediately deallocated before getting into an active set when - // they are still only in the joining set. If they are to be removed from - // joining set as a result of their deallocation, we remove them here. + // they are still only in the allocating set. If they are to be removed from + // allocating set as a result of their deallocation, we remove them here. // We can't remove at the time of their public call, because we still // need to deallocate the funds. // // - Alternatively we can just block the deallocation if you are not // in an active set currently. - current.append(&mut joining); + current.append(&mut allocating); // remove the validators who wanted to leave current.retain(|id| !leaving.contains(id)); @@ -317,9 +326,8 @@ pub mod pallet { /// Makes a new validator set for the given session index with the given `validators` /// as participants. All validators in `validators`, must be present either - /// in `JoiningValidators` or the current active validator set. - /// Does not fire the `Event::NewSet` for Serai network, since this expected to fired - /// prior to this call for a given session index. + /// in `AllocatingValidators` or the current active validator set. Does not fire + /// `Event::NewSet`, since this expected to fired prior to this call for a given session index. pub fn start_session(new_index: u32, network: NetworkId, validators: Vec) { // validator sets for index 0 is already set in the genesis build. if new_index == 0 { @@ -329,8 +337,8 @@ pub mod pallet { // get the bond of the validators, prepare participants. let mut participants = Vec::new(); - let mut joining = JoiningValidators::::get(key); - let deallocating = DeallocatingValidators::::get(key) + let mut allocating = Self::allocating_validators(key); + let deallocating = Self::deallocating_validators(key) .iter() .filter(|(_, _, leaving)| !leaving) .map(|(id, amount, _)| (*id, *amount)) @@ -339,13 +347,13 @@ pub mod pallet { Self::validator_set(ValidatorSet { session: Session(new_index - 1), network }).unwrap(); // append them all together - joining.extend(prev.participants); - let last_add_index = joining.len() - 1; - joining.extend(deallocating); + allocating.extend(prev.participants); + let last_add_index = allocating.len() - 1; + allocating.extend(deallocating); // calculate the amounts let mut amounts = BTreeMap::::new(); - for (i, (id, amount)) in joining.into_iter().enumerate() { + for (i, (id, amount)) in allocating.into_iter().enumerate() { amounts .entry(id) .and_modify(|bond| { @@ -369,6 +377,11 @@ pub mod pallet { participants.push((v, bond)); } + // check whether the Keys set for an external network. + if network != NetworkId::Serai && Keys::::get(key).is_none() { + todo!() + } + // insert the new set for the session let new_set = ValidatorSet { session: Session(new_index), network }; let new_data = ValidatorSetData { @@ -382,15 +395,6 @@ pub mod pallet { CurrentSessionIndex::::mutate(|v| { *v = Session(new_index); }); - - // TODO: This new_set fire is to be removed if we fire - // 100 block ago for other networks too like for serai net. - if network != NetworkId::Serai { - if Keys::::get(key).is_none() { - todo!() - } - Pallet::::deposit_event(Event::NewSet { set: new_set }) - } } pub fn end_session(end_index: u32, network: NetworkId) { @@ -399,10 +403,9 @@ pub mod pallet { } // delete old keys - // we don't delete the end_index itself since we still need it - // to start the next session. + // we don't delete the end_index itself since we still need it to start the next session. let key = ValidatorSet { session: Session(end_index - 1), network }; - JoiningValidators::::remove(key); + AllocatingValidators::::remove(key); DeallocatingValidators::::remove(key); Keys::::remove(key); MuSigKeys::::remove(key); @@ -455,7 +458,8 @@ pub mod pallet { Err(Error::NonExistentValidatorSet) | Err(Error::BadSignature) | Err(Error::NonExistentValidator) | - Err(Error::InSufficientAllocation) => Err(InvalidTransaction::BadProof)?, + Err(Error::InsufficientAllocation) | + Err(Error::DeallocationWouldRemoveParticipant) => Err(InvalidTransaction::BadProof)?, Err(Error::__Ignore(_, _)) | Err(Error::InSufficientBond) => unreachable!(), Ok(()) => (), }