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

Ban direct indexing #369

Merged
merged 12 commits into from
Apr 30, 2024
21 changes: 15 additions & 6 deletions node/src/chain_spec.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
// Allowed since it's actually better to panic during chain setup when there is an error
#![allow(clippy::unwrap_used)]
Comment on lines +1 to +2
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, nice to allow this in tests, benchmarks, and some node stuff like this


use node_subtensor_runtime::{
AccountId, AuraConfig, BalancesConfig, GrandpaConfig, RuntimeGenesisConfig,
SenateMembersConfig, Signature, SubtensorModuleConfig, SudoConfig, SystemConfig,
Expand Down Expand Up @@ -90,14 +93,16 @@ pub fn finney_mainnet_config() -> Result<ChainSpec, String> {
Vec<(sp_runtime::AccountId32, (u64, u16))>,
)> = Vec::new();
for (coldkey_str, hotkeys) in old_state.stakes.iter() {
let coldkey = <sr25519::Public as Ss58Codec>::from_ss58check(coldkey_str).unwrap();
let coldkey = <sr25519::Public as Ss58Codec>::from_ss58check(coldkey_str)
.map_err(|e| e.to_string())?;
let coldkey_account = sp_runtime::AccountId32::from(coldkey);

let mut processed_hotkeys: Vec<(sp_runtime::AccountId32, (u64, u16))> = Vec::new();

for (hotkey_str, amount_uid) in hotkeys.iter() {
let (amount, uid) = amount_uid;
let hotkey = <sr25519::Public as Ss58Codec>::from_ss58check(hotkey_str).unwrap();
let hotkey = <sr25519::Public as Ss58Codec>::from_ss58check(hotkey_str)
.map_err(|e| e.to_string())?;
let hotkey_account = sp_runtime::AccountId32::from(hotkey);

processed_hotkeys.push((hotkey_account, (*amount, *uid)));
Expand All @@ -109,7 +114,8 @@ pub fn finney_mainnet_config() -> Result<ChainSpec, String> {
let mut balances_issuance: u64 = 0;
let mut processed_balances: Vec<(sp_runtime::AccountId32, u64)> = Vec::new();
for (key_str, amount) in old_state.balances.iter() {
let key = <sr25519::Public as Ss58Codec>::from_ss58check(key_str).unwrap();
let key =
<sr25519::Public as Ss58Codec>::from_ss58check(key_str).map_err(|e| e.to_string())?;
let key_account = sp_runtime::AccountId32::from(key);

processed_balances.push((key_account, *amount));
Expand Down Expand Up @@ -266,14 +272,16 @@ pub fn finney_testnet_config() -> Result<ChainSpec, String> {
Vec<(sp_runtime::AccountId32, (u64, u16))>,
)> = Vec::new();
for (coldkey_str, hotkeys) in old_state.stakes.iter() {
let coldkey = <sr25519::Public as Ss58Codec>::from_ss58check(coldkey_str).unwrap();
let coldkey = <sr25519::Public as Ss58Codec>::from_ss58check(coldkey_str)
.map_err(|e| e.to_string())?;
let coldkey_account = sp_runtime::AccountId32::from(coldkey);

let mut processed_hotkeys: Vec<(sp_runtime::AccountId32, (u64, u16))> = Vec::new();

for (hotkey_str, amount_uid) in hotkeys.iter() {
let (amount, uid) = amount_uid;
let hotkey = <sr25519::Public as Ss58Codec>::from_ss58check(hotkey_str).unwrap();
let hotkey = <sr25519::Public as Ss58Codec>::from_ss58check(hotkey_str)
.map_err(|e| e.to_string())?;
let hotkey_account = sp_runtime::AccountId32::from(hotkey);

processed_hotkeys.push((hotkey_account, (*amount, *uid)));
Expand All @@ -285,7 +293,8 @@ pub fn finney_testnet_config() -> Result<ChainSpec, String> {
let mut balances_issuance: u64 = 0;
let mut processed_balances: Vec<(sp_runtime::AccountId32, u64)> = Vec::new();
for (key_str, amount) in old_state.balances.iter() {
let key = <sr25519::Public as Ss58Codec>::from_ss58check(key_str).unwrap();
let key =
<sr25519::Public as Ss58Codec>::from_ss58check(key_str).map_err(|e| e.to_string())?;
let key_account = sp_runtime::AccountId32::from(key);

processed_balances.push((key_account, *amount));
Expand Down
4 changes: 2 additions & 2 deletions pallets/commitments/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,8 @@ impl Encode for Data {
Data::None => vec![0u8; 1],
Data::Raw(ref x) => {
let l = x.len().min(128);
let mut r = vec![l as u8 + 1; l + 1];
r[1..].copy_from_slice(&x[..l]);
let mut r = vec![l as u8 + 1];
r.extend_from_slice(&x[..]);
r
}
Data::BlakeTwo256(ref h) => once(130).chain(h.iter().cloned()).collect(),
Expand Down
5 changes: 3 additions & 2 deletions pallets/registry/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use super::*;
use crate::Pallet as Registry;
use frame_benchmarking::v1::account;
use frame_benchmarking::v2::*;
use frame_support::traits::tokens::fungible::Mutate;
use frame_system::RawOrigin;

use sp_runtime::traits::Bounded;
Expand Down Expand Up @@ -40,7 +41,7 @@ mod benchmarks {
fn set_identity() {
// The target user
let caller: T::AccountId = whitelisted_caller();
let _ = T::Currency::make_free_balance_be(&caller, BalanceOf::<T>::max_value());
let _ = T::Currency::set_balance(&caller, BalanceOf::<T>::max_value());

#[extrinsic_call]
_(
Expand All @@ -56,7 +57,7 @@ mod benchmarks {
fn clear_identity() {
// The target user
let caller: T::AccountId = whitelisted_caller();
let _ = T::Currency::make_free_balance_be(&caller, BalanceOf::<T>::max_value());
let _ = T::Currency::set_balance(&caller, BalanceOf::<T>::max_value());

let vali_account = account::<T::AccountId>("account", 0, 0u32);

Expand Down
45 changes: 36 additions & 9 deletions pallets/registry/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,20 @@ pub use pallet::*;
pub use types::*;
pub use weights::WeightInfo;

use frame_support::traits::Currency;
use frame_support::traits::tokens::{
fungible::{self, MutateHold as _},
Precision,
};
use sp_runtime::traits::Zero;
use sp_std::boxed::Box;

type BalanceOf<T> =
<<T as Config>::Currency as Currency<<T as frame_system::Config>::AccountId>>::Balance;
<<T as Config>::Currency as fungible::Inspect<<T as frame_system::Config>::AccountId>>::Balance;

#[frame_support::pallet]
pub mod pallet {
use super::*;
use frame_support::{pallet_prelude::*, traits::ReservableCurrency};
use frame_support::{pallet_prelude::*, traits::tokens::fungible};
use frame_system::pallet_prelude::*;

#[pallet::pallet]
Expand All @@ -36,7 +39,8 @@ pub mod pallet {
type RuntimeEvent: From<Event<Self>> + IsType<<Self as frame_system::Config>::RuntimeEvent>;

// Currency type that will be used to place deposits on neurons
type Currency: ReservableCurrency<Self::AccountId> + Send + Sync;
type Currency: fungible::Mutate<Self::AccountId>
+ fungible::MutateHold<Self::AccountId, Reason = Self::RuntimeHoldReason>;

// Weight information for extrinsics in this pallet.
type WeightInfo: WeightInfo;
Expand All @@ -56,6 +60,9 @@ pub mod pallet {
/// The amount held on deposit per additional field for a registered identity.
#[pallet::constant]
type FieldDeposit: Get<BalanceOf<Self>>;

/// Reasons for putting funds on hold.
type RuntimeHoldReason: From<HoldReason>;
}

#[pallet::event]
Expand All @@ -75,6 +82,11 @@ pub mod pallet {
NotRegistered,
}

#[pallet::composite_enum]
pub enum HoldReason {
RegistryIdentity,
}

/// Identity data by account
#[pallet::storage]
#[pallet::getter(fn identity_of)]
Expand Down Expand Up @@ -125,11 +137,21 @@ pub mod pallet {
let old_deposit = id.deposit;
id.deposit = T::InitialDeposit::get() + fd;
if id.deposit > old_deposit {
T::Currency::reserve(&who, id.deposit - old_deposit)?;
T::Currency::hold(
&HoldReason::RegistryIdentity.into(),
&who,
id.deposit - old_deposit,
)?;
Comment on lines -128 to +144
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to do the currency -> fungible migration here? Seems unrelated to direct indexing and I think you need a migration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should have gone into the 1.0 branch, but I forgot to commit it and now i'm rolling into this.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gregzaitsev is this what you were referring to a while back ? I remember we had some conversation about it. #353

}
if old_deposit > id.deposit {
let err_amount = T::Currency::unreserve(&who, old_deposit - id.deposit);
debug_assert!(err_amount.is_zero());
let release_res = T::Currency::release(
&HoldReason::RegistryIdentity.into(),
&who,
old_deposit - id.deposit,
Precision::BestEffort,
);
debug_assert!(release_res
.is_ok_and(|released_amount| released_amount == (old_deposit - id.deposit)));
}

<IdentityOf<T>>::insert(&identified, id);
Expand All @@ -153,8 +175,13 @@ pub mod pallet {
let id = <IdentityOf<T>>::take(&identified).ok_or(Error::<T>::NotRegistered)?;
let deposit = id.total_deposit();

let err_amount = T::Currency::unreserve(&who, deposit);
debug_assert!(err_amount.is_zero());
let release_res = T::Currency::release(
&HoldReason::RegistryIdentity.into(),
&who,
deposit,
Precision::BestEffort,
);
debug_assert!(release_res.is_ok_and(|released_amount| released_amount == deposit));

Self::deposit_event(Event::IdentityDissolved { who: identified });

Expand Down
4 changes: 2 additions & 2 deletions pallets/registry/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,8 @@ impl Encode for Data {
Data::None => vec![0u8; 1],
Data::Raw(ref x) => {
let l = x.len().min(64);
let mut r = vec![l as u8 + 1; l + 1];
r[1..].copy_from_slice(&x[..l]);
let mut r = vec![l as u8 + 1];
r.extend_from_slice(&x[..]);
r
}
Data::BlakeTwo256(ref h) => once(66u8).chain(h.iter().cloned()).collect(),
Expand Down
18 changes: 6 additions & 12 deletions pallets/subtensor/src/block_step.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,11 +74,8 @@ impl<T: Config> Pallet<T> {
}
}

pub fn has_loaded_emission_tuples(netuid: u16) -> bool {
LoadedEmission::<T>::contains_key(netuid)
}
pub fn get_loaded_emission_tuples(netuid: u16) -> Vec<(T::AccountId, u64, u64)> {
LoadedEmission::<T>::get(netuid).unwrap()
pub fn get_loaded_emission_tuples(netuid: u16) -> Option<Vec<(T::AccountId, u64, u64)>> {
LoadedEmission::<T>::get(netuid)
}

// Reads from the loaded emission storage which contains lists of pending emission tuples ( hotkey, amount )
Expand All @@ -87,11 +84,10 @@ impl<T: Config> Pallet<T> {
pub fn drain_emission(_: u64) {
// --- 1. We iterate across each network.
for (netuid, _) in <Tempo<T> as IterableStorageMap<u16, u16>>::iter() {
if !Self::has_loaded_emission_tuples(netuid) {
let Some(tuples_to_drain) = Self::get_loaded_emission_tuples(netuid) else {
// There are no tuples to emit.
continue;
} // There are no tuples to emit.
let tuples_to_drain: Vec<(T::AccountId, u64, u64)> =
Self::get_loaded_emission_tuples(netuid);
};
let mut total_emitted: u64 = 0;
for (hotkey, server_amount, validator_amount) in tuples_to_drain.iter() {
Self::emit_inflation_through_hotkey_account(
Expand Down Expand Up @@ -189,10 +185,8 @@ impl<T: Config> Pallet<T> {
// --- 10. Sink the emission tuples onto the already loaded.
let mut concat_emission_tuples: Vec<(T::AccountId, u64, u64)> =
emission_tuples_this_block.clone();
if Self::has_loaded_emission_tuples(netuid) {
if let Some(mut current_emission_tuples) = Self::get_loaded_emission_tuples(netuid) {
// 10.a We already have loaded emission tuples, so we concat the new ones.
let mut current_emission_tuples: Vec<(T::AccountId, u64, u64)> =
Self::get_loaded_emission_tuples(netuid);
concat_emission_tuples.append(&mut current_emission_tuples);
}
LoadedEmission::<T>::insert(netuid, concat_emission_tuples);
Expand Down
9 changes: 3 additions & 6 deletions pallets/subtensor/src/delegate_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ impl<T: Config> Pallet<T> {
}

let delegate: AccountIdOf<T> =
T::AccountId::decode(&mut delegate_account_vec.as_bytes_ref()).unwrap();
T::AccountId::decode(&mut delegate_account_vec.as_bytes_ref()).ok()?;
// Check delegate exists
if !<Delegates<T>>::contains_key(delegate.clone()) {
return None;
Expand All @@ -108,12 +108,9 @@ impl<T: Config> Pallet<T> {
}

pub fn get_delegated(delegatee_account_vec: Vec<u8>) -> Vec<(DelegateInfo<T>, Compact<u64>)> {
if delegatee_account_vec.len() != 32 {
let Ok(delegatee) = T::AccountId::decode(&mut delegatee_account_vec.as_bytes_ref()) else {
return Vec::new(); // No delegates for invalid account
}

let delegatee: AccountIdOf<T> =
T::AccountId::decode(&mut delegatee_account_vec.as_bytes_ref()).unwrap();
};

let mut delegates: Vec<(DelegateInfo<T>, Compact<u64>)> = Vec::new();
for delegate in <Delegates<T> as IterableStorageMap<T::AccountId, u16>>::iter_keys() {
Expand Down
Loading
Loading