From 51671dedfab54624c7f649b604972b3867c4f5e8 Mon Sep 17 00:00:00 2001 From: Greg Zaitsev Date: Wed, 8 Oct 2025 16:47:32 -0400 Subject: [PATCH 1/6] Refactor childkeys/parentkeys, add migration to clean state --- pallets/subtensor/src/macros/errors.rs | 2 + pallets/subtensor/src/macros/hooks.rs | 4 +- .../src/migrations/migrate_fix_childkeys.rs | 40 + pallets/subtensor/src/migrations/mod.rs | 1 + pallets/subtensor/src/staking/set_children.rs | 789 ++++++++++++++++-- pallets/subtensor/src/swap/swap_hotkey.rs | 63 +- pallets/subtensor/src/tests/children.rs | 29 + pallets/subtensor/src/tests/swap_hotkey.rs | 60 +- .../src/tests/swap_hotkey_with_subnet.rs | 56 +- 9 files changed, 903 insertions(+), 141 deletions(-) create mode 100644 pallets/subtensor/src/migrations/migrate_fix_childkeys.rs diff --git a/pallets/subtensor/src/macros/errors.rs b/pallets/subtensor/src/macros/errors.rs index e241ff068f..759e74f6e3 100644 --- a/pallets/subtensor/src/macros/errors.rs +++ b/pallets/subtensor/src/macros/errors.rs @@ -262,5 +262,7 @@ mod errors { UidMapCouldNotBeCleared, /// Trimming would exceed the max immune neurons percentage TrimmingWouldExceedMaxImmunePercentage, + /// Violating the rules of Childkey-Parentkey consistency + ChildParentInconsistency, } } diff --git a/pallets/subtensor/src/macros/hooks.rs b/pallets/subtensor/src/macros/hooks.rs index a055668f1a..f8e73d2cdb 100644 --- a/pallets/subtensor/src/macros/hooks.rs +++ b/pallets/subtensor/src/macros/hooks.rs @@ -149,7 +149,9 @@ mod hooks { // Migrate subnet locked balances .saturating_add(migrations::migrate_subnet_locked::migrate_restore_subnet_locked::()) // Migrate subnet burn cost to 2500 - .saturating_add(migrations::migrate_network_lock_cost_2500::migrate_network_lock_cost_2500::()); + .saturating_add(migrations::migrate_network_lock_cost_2500::migrate_network_lock_cost_2500::()) + // Cleanup child/parent keys + .saturating_add(migrations::migrate_fix_childkeys::migrate_fix_childkeys::()); weight } diff --git a/pallets/subtensor/src/migrations/migrate_fix_childkeys.rs b/pallets/subtensor/src/migrations/migrate_fix_childkeys.rs new file mode 100644 index 0000000000..cc0b6988e0 --- /dev/null +++ b/pallets/subtensor/src/migrations/migrate_fix_childkeys.rs @@ -0,0 +1,40 @@ +use super::*; +use alloc::string::String; + +pub fn migrate_fix_childkeys() -> Weight { + let migration_name = b"migrate_fix_childkeys".to_vec(); + let mut weight = T::DbWeight::get().reads(1); + + if HasMigrationRun::::get(&migration_name) { + log::info!( + "Migration '{:?}' has already run. Skipping.", + String::from_utf8_lossy(&migration_name) + ); + return weight; + } + + log::info!( + "Running migration '{}'", + String::from_utf8_lossy(&migration_name) + ); + + //////////////////////////////////////////////////////// + // Actual migration + + Pallet::::clean_zero_childkey_vectors(&mut weight); + Pallet::::clean_zero_parentkey_vectors(&mut weight); + Pallet::::clean_self_loops(&mut weight); + Pallet::::repair_child_parent_consistency(&mut weight); + + //////////////////////////////////////////////////////// + + HasMigrationRun::::insert(&migration_name, true); + weight = weight.saturating_add(T::DbWeight::get().writes(1)); + + log::info!( + target: "runtime", + "Migration '{}' completed successfully.", + String::from_utf8_lossy(&migration_name) + ); + weight +} diff --git a/pallets/subtensor/src/migrations/mod.rs b/pallets/subtensor/src/migrations/mod.rs index b036783d9d..e92b86aa29 100644 --- a/pallets/subtensor/src/migrations/mod.rs +++ b/pallets/subtensor/src/migrations/mod.rs @@ -15,6 +15,7 @@ pub mod migrate_crv3_v2_to_timelocked; pub mod migrate_delete_subnet_21; pub mod migrate_delete_subnet_3; pub mod migrate_disable_commit_reveal; +pub mod migrate_fix_childkeys; pub mod migrate_fix_is_network_member; pub mod migrate_fix_root_subnet_tao; pub mod migrate_fix_root_tao_and_alpha_in; diff --git a/pallets/subtensor/src/staking/set_children.rs b/pallets/subtensor/src/staking/set_children.rs index cf7103b7ab..9df7dbdf09 100644 --- a/pallets/subtensor/src/staking/set_children.rs +++ b/pallets/subtensor/src/staking/set_children.rs @@ -1,8 +1,415 @@ use super::*; +use sp_std::collections::{btree_map::BTreeMap, btree_set::BTreeSet}; use subtensor_runtime_common::NetUid; +pub struct PCRelations { + /// The distinguished `hotkey` this structure is built around. + pivot: T::AccountId, + children: BTreeMap, + parents: BTreeMap, +} + +impl PCRelations { + /// Create empty relations for a given pivot. + pub fn new(hotkey: T::AccountId) -> Self { + Self { + pivot: hotkey, + children: BTreeMap::new(), + parents: BTreeMap::new(), + } + } + + //////////////////////////////////////////////////////////// + // Constraint checkers + + /// Ensures sum(proportions) <= u64::MAX + pub fn ensure_total_proportions(children: &BTreeMap) -> DispatchResult { + let total: u128 = children + .values() + .fold(0u128, |acc, &w| acc.saturating_add(w as u128)); + ensure!(total <= u64::MAX as u128, Error::::ProportionOverflow); + Ok(()) + } + + /// Ensure that the number of children does not exceed 5 + pub fn ensure_childkey_count(children: &BTreeMap) -> DispatchResult { + ensure!(children.len() <= 5, Error::::TooManyChildren); + + Ok(()) + } + + /// Ensures the given children or parent set doesn't contain pivot + pub fn ensure_no_self_loop( + pivot: &T::AccountId, + hotkey_set: &BTreeMap, + ) -> DispatchResult { + ensure!(!hotkey_set.contains_key(pivot), Error::::InvalidChild); + Ok(()) + } + + /// Ensures that children and parents sets do not have any overlap + pub fn ensure_bipartite_separation( + children: &BTreeMap, + parents: &BTreeMap, + ) -> DispatchResult { + let has_overlap = children.keys().any(|c| parents.contains_key(c)); + ensure!(!has_overlap, Error::::ChildParentInconsistency); + Ok(()) + } + + /// Validate that applying `pending_children_vec` to `relations` (as the new + /// pivot->children mapping) preserves all invariants. + /// + /// Checks: + /// 1) No self-loop: pivot must not appear among children. + /// 2) Sum of child proportions fits in `u64`. + /// 3) Bipartite role separation: no child may also be a parent. + pub fn ensure_pending_consistency( + &self, + pending_children_vec: &Vec<(u64, T::AccountId)>, + ) -> DispatchResult { + // Build a deduped children map (last proportion wins if duplicates present). + let mut new_children: BTreeMap = BTreeMap::new(); + for (prop, child) in pending_children_vec { + new_children.insert(child.clone(), *prop); + } + + // Check constraints + Self::ensure_no_self_loop(&self.pivot, &new_children)?; + Self::ensure_childkey_count(&new_children)?; + Self::ensure_total_proportions(&new_children)?; + Self::ensure_bipartite_separation(&new_children, &self.parents)?; + + Ok(()) + } + + //////////////////////////////////////////////////////////// + // Getters + + #[inline] + pub fn pivot(&self) -> &T::AccountId { + &self.pivot + } + #[inline] + pub fn children(&self) -> &BTreeMap { + &self.children + } + #[inline] + pub fn parents(&self) -> &BTreeMap { + &self.parents + } + + //////////////////////////////////////////////////////////// + // Safe updaters + + /// Replace the pivot->children mapping after validating invariants. + /// + /// Invariants: + /// - No self-loop: child != pivot + /// - sum(proportions) fits in u64 (checked as u128 to avoid overflow mid-sum) + pub fn link_children(&mut self, new_children: BTreeMap) -> DispatchResult { + // Check constraints + Self::ensure_no_self_loop(&self.pivot, &new_children)?; + Self::ensure_total_proportions(&new_children)?; + Self::ensure_bipartite_separation(&new_children, &self.parents)?; + + self.children = new_children; + Ok(()) + } + + pub fn link_parents(&mut self, new_parents: BTreeMap) -> DispatchResult { + // Check constraints + Self::ensure_no_self_loop(&self.pivot, &new_parents)?; + Self::ensure_bipartite_separation(&self.children, &new_parents)?; + + self.parents = new_parents; + Ok(()) + } + + #[inline] + fn upsert_edge(list: &mut Vec<(u64, T::AccountId)>, proportion: u64, id: &T::AccountId) { + for (p, who) in list.iter_mut() { + if who == id { + *p = proportion; + return; + } + } + list.push((proportion, id.clone())); + } + + #[inline] + fn remove_edge(list: &mut Vec<(u64, T::AccountId)>, id: &T::AccountId) { + list.retain(|(_, who)| who != id); + } + + /// Change the pivot hotkey for these relations. + /// Ensures there are no self-loops with the new pivot. + pub fn rebind_pivot(&mut self, new_pivot: T::AccountId) -> DispatchResult { + // No self-loop via children or parents for the new pivot. + Self::ensure_no_self_loop(&new_pivot, &self.children)?; + Self::ensure_no_self_loop(&new_pivot, &self.parents)?; + + self.pivot = new_pivot; + Ok(()) + } +} + impl Pallet { + /// Loads all records from ChildKeys and ParentKeys where (hotkey, netuid) is the key. + /// Produces a parent->(child->prop) adjacency map that **cannot violate** + /// the required consistency because all inserts go through `link`. + fn load_child_parent_relations(hotkey: &T::AccountId, netuid: NetUid) -> PCRelations { + let mut rel = PCRelations::::new(hotkey.clone()); + + // Load children: (prop, child) from ChildKeys(hotkey, netuid) + let child_links = ChildKeys::::get(hotkey, netuid); + let mut children = BTreeMap::::new(); + for (prop, child) in child_links { + // Ignore any accidental self-loop in storage + if child != *hotkey { + children.insert(child, prop); + } + } + // Validate & set (enforce no self-loop and sum limit) + let _ = rel.link_children(children); + + // Load parents: (prop, parent) from ParentKeys(hotkey, netuid) + let parent_links = ParentKeys::::get(hotkey, netuid); + let mut parents = BTreeMap::::new(); + for (prop, parent) in parent_links { + if parent != *hotkey { + parents.insert(parent, prop); + } + } + // Keep the same validation rules for parents (no self-loop, bounded sum). + let _ = rel.link_parents(parents); + + rel + } + + /// Build a `PCRelations` for `pivot` (parent) from the `PendingChildKeys` queue, + /// preserving the current `ParentKeys(pivot, netuid)` so `persist_child_parent_relations` + /// won’t accidentally clear existing parents. + /// + /// PendingChildKeys layout: + /// (netuid, pivot) -> (Vec<(proportion, child)>) + pub fn load_relations_from_pending( + pivot: T::AccountId, + pending_children_vec: &Vec<(u64, T::AccountId)>, + netuid: NetUid, + ) -> Result, DispatchError> { + let mut rel = PCRelations::::new(pivot.clone()); + + // Deduplicate into a BTreeMap (last wins if duplicates). + let mut children: BTreeMap = BTreeMap::new(); + for (prop, child) in pending_children_vec { + if *child != pivot { + children.insert(child.clone(), *prop); + } + } + + // Enforce invariants (no self-loop, total weight <= u64::MAX) + rel.link_children(children)?; + + // Preserve the current parents of the pivot so `persist_child_parent_relations` + // won’t clear them when we only intend to update children. + let existing_parents_vec = ParentKeys::::get(pivot.clone(), netuid); + let mut parents: BTreeMap = BTreeMap::new(); + for (w, parent) in existing_parents_vec { + if parent != pivot { + parents.insert(parent, w); + } + } + // This uses the same basic checks (no self-loop, bounded sum). + // If you didn't expose link_parents, inline the simple validations here. + rel.link_parents(parents)?; + + Ok(rel) + } + + /// Persist the `relations` around `hotkey` to storage, updating both directions: + /// - Writes ChildKeys(hotkey, netuid) = children + /// and synchronizes ParentKeys(child, netuid) entries accordingly. + /// - Writes ParentKeys(hotkey, netuid) = parents + /// and synchronizes ChildKeys(parent, netuid) entries accordingly. + /// + /// This is a **diff-based** update that only touches affected neighbors. + pub fn persist_child_parent_relations( + relations: PCRelations, + netuid: NetUid, + weight: &mut Weight, + ) -> DispatchResult { + let pivot = relations.pivot().clone(); + + // --------------------------- + // 1) Pivot -> Children side + // --------------------------- + let new_children_map = relations.children(); + let new_children_vec: Vec<(u64, T::AccountId)> = new_children_map + .iter() + .map(|(c, w)| (*w, c.clone())) + .collect(); + + let prev_children_vec = ChildKeys::::get(&pivot, netuid); + weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 0)); + + // Overwrite pivot's children vector + Self::set_childkeys(pivot.clone(), netuid, new_children_vec.clone()); + weight.saturating_accrue(T::DbWeight::get().reads_writes(0, 1)); + + // Build quick-lookup sets for diffing + let prev_children_set: BTreeSet = + prev_children_vec.iter().map(|(_, c)| c.clone()).collect(); + let new_children_set: BTreeSet = new_children_map.keys().cloned().collect(); + + // Added children = new / prev + for added in new_children_set + .iter() + .filter(|c| !prev_children_set.contains(*c)) + { + let mut pk = ParentKeys::::get(added.clone(), netuid); + let w = *new_children_map.get(added).expect("in set; qed"); + PCRelations::::upsert_edge(&mut pk, w, &pivot); + Self::set_parentkeys(added.clone(), netuid, pk.clone()); + weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 1)); + } + + // Updated children = intersection where proportion changed + for common in new_children_set.intersection(&prev_children_set) { + let new_p = *new_children_map.get(common).expect("in set; qed"); + let mut pk = ParentKeys::::get(common.clone(), netuid); + PCRelations::::upsert_edge(&mut pk, new_p, &pivot); + Self::set_parentkeys(common.clone(), netuid, pk.clone()); + weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 1)); + } + + // Removed children = prev / new + for removed in prev_children_set + .iter() + .filter(|c| !new_children_set.contains(*c)) + { + let mut pk = ParentKeys::::get(&removed, netuid); + PCRelations::::remove_edge(&mut pk, &pivot); + Self::set_parentkeys(removed.clone(), netuid, pk.clone()); + weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 1)); + } + + // --------------------------- + // 2) Parents -> Pivot side + // --------------------------- + let new_parents_map = relations.parents(); + let new_parents_vec: Vec<(u64, T::AccountId)> = new_parents_map + .iter() + .map(|(p, w)| (*w, p.clone())) + .collect(); + + let prev_parents_vec = ParentKeys::::get(&pivot, netuid); + + // Overwrite pivot's parents vector + Self::set_parentkeys(pivot.clone(), netuid, new_parents_vec.clone()); + + let prev_parents_set: BTreeSet = + prev_parents_vec.iter().map(|(_, p)| p.clone()).collect(); + let new_parents_set: BTreeSet = new_parents_map.keys().cloned().collect(); + + // Added parents = new / prev => ensure ChildKeys(parent) has (w, pivot) + for added in new_parents_set + .iter() + .filter(|p| !prev_parents_set.contains(*p)) + { + let mut ck = ChildKeys::::get(added.clone(), netuid); + let w = *new_parents_map.get(added).expect("in set; qed"); + PCRelations::::upsert_edge(&mut ck, w, &pivot); + Self::set_childkeys(added.clone(), netuid, ck); + weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 1)); + } + + // Updated parents = intersection where weight changed + for common in new_parents_set.intersection(&prev_parents_set) { + let new_w = *new_parents_map.get(common).expect("in set; qed"); + let mut ck = ChildKeys::::get(common.clone(), netuid); + PCRelations::::upsert_edge(&mut ck, new_w, &pivot); + Self::set_childkeys(common.clone(), netuid, ck); + weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 1)); + } + + // Removed parents = prev / new => remove (pivot) from ChildKeys(parent) + for removed in prev_parents_set + .iter() + .filter(|p| !new_parents_set.contains(*p)) + { + let mut ck = ChildKeys::::get(removed.clone(), netuid); + PCRelations::::remove_edge(&mut ck, &pivot); + Self::set_childkeys(removed.clone(), netuid, ck); + weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 1)); + } + + Ok(()) + } + + /// Swap all parent/child relations from `old_hotkey` to `new_hotkey` on `netuid`. + /// Steps: + /// 1) Load relations around `old_hotkey` + /// 2) Clean up storage references to `old_hotkey` (both directions) + /// 3) Rebind pivot to `new_hotkey` + /// 4) Persist relations around `new_hotkey` + pub fn parent_child_swap_hotkey( + old_hotkey: &T::AccountId, + new_hotkey: &T::AccountId, + netuid: NetUid, + weight: &mut Weight, + ) -> DispatchResult { + // 1) Load the current relations around old_hotkey + let mut relations = Self::load_child_parent_relations(old_hotkey, netuid); + weight.saturating_accrue(T::DbWeight::get().reads_writes(2, 0)); + + // 2) Clean up all storage entries that reference old_hotkey + // 2a) For each child of old_hotkey: remove old_hotkey from ParentKeys(child, netuid) + for (child, _) in relations.children().iter() { + let mut pk = ParentKeys::::get(child.clone(), netuid); + PCRelations::::remove_edge(&mut pk, old_hotkey); + Self::set_parentkeys(child.clone(), netuid, pk.clone()); + weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 1)); + } + // 2b) For each parent of old_hotkey: remove old_hotkey from ChildKeys(parent, netuid) + for (parent, _) in relations.parents().iter() { + let mut ck = ChildKeys::::get(parent.clone(), netuid); + PCRelations::::remove_edge(&mut ck, old_hotkey); + ChildKeys::::insert(parent.clone(), netuid, ck); + weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 1)); + } + // 2c) Clear direct maps of old_hotkey + ChildKeys::::insert( + old_hotkey.clone(), + netuid, + Vec::<(u64, T::AccountId)>::new(), + ); + Self::set_parentkeys( + old_hotkey.clone(), + netuid, + Vec::<(u64, T::AccountId)>::new(), + ); + weight.saturating_accrue(T::DbWeight::get().reads_writes(0, 2)); + + // 3) Rebind pivot to new_hotkey (validate no self-loop with existing maps) + relations.rebind_pivot(new_hotkey.clone())?; + + // 4) Swap PendingChildKeys( netuid, parent ) --> Vec<(proportion,child), cool_down_block> + // Fail if consistency breaks + if PendingChildKeys::::contains_key(netuid, old_hotkey) { + let (children, cool_down_block) = PendingChildKeys::::get(netuid, old_hotkey); + relations.ensure_pending_consistency(&children)?; + + PendingChildKeys::::remove(netuid, old_hotkey); + PendingChildKeys::::insert(netuid, new_hotkey, (children, cool_down_block)); + weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 2)); + } + + // 5) Persist relations under the new pivot (diffs vs existing state at new_hotkey) + Self::persist_child_parent_relations(relations, netuid, weight) + } + /// ---- The implementation for the extrinsic do_set_child_singular: Sets a single child. /// This function allows a coldkey to set children keys. /// @@ -70,19 +477,6 @@ impl Pallet { Error::::NonAssociatedColdKey ); - // Ensure that the number of children does not exceed 5. - ensure!(children.len() <= 5, Error::::TooManyChildren); - - // Ensure that each child is not the hotkey. - for (_, child_i) in &children { - ensure!(child_i != &hotkey, Error::::InvalidChild); - } - // Ensure that the sum of the proportions does not exceed u64::MAX. - let _total_proportion: u64 = children - .iter() - .try_fold(0u64, |acc, &(proportion, _)| acc.checked_add(proportion)) - .ok_or(Error::::ProportionOverflow)?; - // Ensure there are no duplicates in the list of children. let mut unique_children = Vec::new(); for (_, child_i) in &children { @@ -93,6 +487,14 @@ impl Pallet { unique_children.push(child_i.clone()); } + // Ensure we don't break consistency when these new childkeys are set: + // - Ensure that the number of children does not exceed 5 + // - Each child is not the hotkey. + // - The sum of the proportions does not exceed u64::MAX. + // - Bipartite separation (no A <-> B relations) + let relations = Self::load_child_parent_relations(&hotkey, netuid); + relations.ensure_pending_consistency(&children)?; + // Check that the parent key has at least the minimum own stake // if children vector is not empty // (checking with check_weights_min_stake wouldn't work because it considers @@ -165,60 +567,29 @@ impl Pallet { PendingChildKeys::::iter_prefix(netuid).for_each( |(hotkey, (children, cool_down_block))| { if cool_down_block < current_block { - // Erase myself from old children's parents. - let old_children: Vec<(u64, T::AccountId)> = - ChildKeys::::get(hotkey.clone(), netuid); - - // Iterate over all my old children and remove myself from their parent's map. - for (_, old_child_i) in old_children.clone().iter() { - // Get the old child's parents on this network. - let my_old_child_parents: Vec<(u64, T::AccountId)> = - ParentKeys::::get(old_child_i.clone(), netuid); - - // Filter my hotkey from my old children's parents list. - let filtered_parents: Vec<(u64, T::AccountId)> = my_old_child_parents - .into_iter() - .filter(|(_, parent)| *parent != hotkey) - .collect(); - - // Update the parent list in storage - ParentKeys::::insert(old_child_i, netuid, filtered_parents); - } - - // Insert my new children + proportion list into the map. - ChildKeys::::insert(hotkey.clone(), netuid, children.clone()); - - // Update the parents list for my new children. - for (proportion, new_child_i) in children.clone().iter() { - // Get the child's parents on this network. - let mut new_child_previous_parents: Vec<(u64, T::AccountId)> = - ParentKeys::::get(new_child_i.clone(), netuid); - - // Append my hotkey and proportion to my new child's parents list. - // NOTE: There are no duplicates possible because I previously removed my self from my old children. - new_child_previous_parents.push((*proportion, hotkey.clone())); - - // Update the parents list in storage. - ParentKeys::::insert( - new_child_i.clone(), - netuid, - new_child_previous_parents, - ); + // If child-parent consistency is broken, we will fail setting new children silently + let maybe_relations = + Self::load_relations_from_pending(hotkey.clone(), &children, netuid); + if let Ok(relations) = maybe_relations { + let mut _weight: Weight = T::DbWeight::get().reads(0); + if let Ok(()) = + Self::persist_child_parent_relations(relations, netuid, &mut _weight) + { + // Log and emit event. + log::trace!( + "SetChildren( netuid:{:?}, hotkey:{:?}, children:{:?} )", + hotkey, + netuid, + children.clone() + ); + Self::deposit_event(Event::SetChildren( + hotkey.clone(), + netuid, + children.clone(), + )); + } } - // Log and emit event. - log::trace!( - "SetChildren( netuid:{:?}, hotkey:{:?}, children:{:?} )", - hotkey, - netuid, - children.clone() - ); - Self::deposit_event(Event::SetChildren( - hotkey.clone(), - netuid, - children.clone(), - )); - // Remove pending children PendingChildKeys::::remove(netuid, hotkey); } @@ -357,4 +728,292 @@ impl Pallet { pub fn get_childkey_take(hotkey: &T::AccountId, netuid: NetUid) -> u16 { ChildkeyTake::::get(hotkey, netuid) } + + //////////////////////////////////////////////////////////// + // State cleaners (for use in migration) + // TODO: Deprecate when the state is clean for a while + + pub fn clean_zero_childkey_vectors(weight: &mut Weight) { + // Collect keys to delete first to avoid mutating while iterating. + let mut to_remove: Vec<(T::AccountId, NetUid)> = Vec::new(); + + for (parent, netuid, children) in ChildKeys::::iter() { + // Account for the read + *weight = weight.saturating_add(T::DbWeight::get().reads(1)); + + if children.is_empty() { + to_remove.push((parent, netuid)); + } + } + + // Remove all empty entries + for (parent, netuid) in &to_remove { + ChildKeys::::remove(parent, netuid); + // Account for the write + *weight = weight.saturating_add(T::DbWeight::get().writes(1)); + } + log::info!( + target: "runtime", + "Removed {} empty childkey vectors.", + to_remove.len() + ); + } + + pub fn set_childkeys( + parent: T::AccountId, + netuid: NetUid, + childkey_vec: Vec<(u64, T::AccountId)>, + ) { + if childkey_vec.is_empty() { + ChildKeys::::remove(parent, netuid); + } else { + ChildKeys::::insert(parent, netuid, childkey_vec); + } + } + + pub fn set_parentkeys( + child: T::AccountId, + netuid: NetUid, + parentkey_vec: Vec<(u64, T::AccountId)>, + ) { + if parentkey_vec.is_empty() { + ParentKeys::::remove(child, netuid); + } else { + ParentKeys::::insert(child, netuid, parentkey_vec); + } + } + + /// Remove self-loops in `ChildKeys` and `ParentKeys`. + /// If, after removal, a value-vector becomes empty, the storage key is removed. + pub fn clean_self_loops(weight: &mut Weight) { + // ------------------------------- + // 1) ChildKeys: (parent, netuid) -> Vec<(w, child)> + // Remove any entries where child == parent. + // ------------------------------- + let mut to_update_ck: Vec<((T::AccountId, NetUid), Vec<(u64, T::AccountId)>)> = Vec::new(); + let mut to_remove_ck: Vec<(T::AccountId, NetUid)> = Vec::new(); + + for (parent, netuid, children) in ChildKeys::::iter() { + *weight = weight.saturating_add(T::DbWeight::get().reads(1)); + + // Filter out self-loops + let filtered: Vec<(u64, T::AccountId)> = children + .clone() + .into_iter() + .filter(|(_, c)| *c != parent) + .collect(); + + // If nothing changed, skip + // (we can detect by comparing lengths; safer is to re-check if any removed existed) + // For simplicity, just compare lengths: + // If len unchanged and the previous vector had no self-loop, skip. + // If there *was* a self-loop and filtered is empty, we'll remove the key. + if filtered.len() == children.len() { + // No change -> continue + continue; + } + + if filtered.is_empty() { + to_remove_ck.push((parent, netuid)); + } else { + to_update_ck.push(((parent, netuid), filtered)); + } + } + + // Apply ChildKeys updates/removals + for ((parent, netuid), new_vec) in &to_update_ck { + Self::set_childkeys(parent.clone(), *netuid, new_vec.clone()); + *weight = weight.saturating_add(T::DbWeight::get().writes(1)); + } + for (parent, netuid) in &to_remove_ck { + ChildKeys::::remove(parent, netuid); + *weight = weight.saturating_add(T::DbWeight::get().writes(1)); + } + log::info!( + target: "runtime", + "Removed {} self-looping childkeys.", + to_update_ck.len().saturating_add(to_remove_ck.len()) + ); + + // ------------------------------- + // 2) ParentKeys: (child, netuid) -> Vec<(w, parent)> + // Remove any entries where parent == child. + // ------------------------------- + let mut to_update_pk: Vec<((T::AccountId, NetUid), Vec<(u64, T::AccountId)>)> = Vec::new(); + let mut to_remove_pk: Vec<(T::AccountId, NetUid)> = Vec::new(); + + for (child, netuid, parents) in ParentKeys::::iter() { + *weight = weight.saturating_add(T::DbWeight::get().reads(1)); + + // Filter out self-loops + let filtered: Vec<(u64, T::AccountId)> = parents + .clone() + .into_iter() + .filter(|(_, p)| *p != child) + .collect(); + + // If unchanged, skip + if filtered.len() == parents.len() { + continue; + } + + if filtered.is_empty() { + to_remove_pk.push((child, netuid)); + } else { + to_update_pk.push(((child, netuid), filtered)); + } + } + + // Apply ParentKeys updates/removals + for ((child, netuid), new_vec) in &to_update_pk { + Self::set_parentkeys(child.clone(), *netuid, new_vec.clone()); + *weight = weight.saturating_add(T::DbWeight::get().writes(1)); + } + for (child, netuid) in &to_remove_pk { + ParentKeys::::remove(child, netuid); + *weight = weight.saturating_add(T::DbWeight::get().writes(1)); + } + log::info!( + target: "runtime", + "Removed {} self-looping parentkeys.", + to_update_pk.len().saturating_add(to_remove_pk.len()) + ); + } + + pub fn clean_zero_parentkey_vectors(weight: &mut Weight) { + // Collect keys to delete first to avoid mutating while iterating. + let mut to_remove: Vec<(T::AccountId, NetUid)> = Vec::new(); + + for (parent, netuid, children) in ParentKeys::::iter() { + // Account for the read + *weight = weight.saturating_add(T::DbWeight::get().reads(1)); + + if children.is_empty() { + to_remove.push((parent, netuid)); + } + } + + // Remove all empty entries + for (parent, netuid) in &to_remove { + ParentKeys::::remove(parent, netuid); + // Account for the write + *weight = weight.saturating_add(T::DbWeight::get().writes(1)); + } + log::info!( + target: "runtime", + "Removed {} empty parentkey vectors.", + to_remove.len() + ); + } + + /// Make ChildKeys and ParentKeys bidirectionally consistent by + /// **removing** entries that don't have a matching counterpart. + /// A match means the exact tuple `(p, other_id)` is present on the opposite map. + /// + /// Rules: + /// - For each (parent, netuid) -> [(p, child)...] in ChildKeys: + /// keep only those (p, child) that appear in ParentKeys(child, netuid) as (p, parent). + /// If resulting list is empty, remove the key. + /// - For each (child, netuid) -> [(p, parent)...] in ParentKeys: + /// keep only those (p, parent) that appear in ChildKeys(parent, netuid) as (p, child). + /// If resulting list is empty, remove the key. + pub fn repair_child_parent_consistency(weight: &mut Weight) { + // ------------------------------- + // 1) Prune ChildKeys by checking ParentKeys + // ------------------------------- + let mut ck_updates: Vec<((T::AccountId, NetUid), Vec<(u64, T::AccountId)>)> = Vec::new(); + let mut ck_removes: Vec<(T::AccountId, NetUid)> = Vec::new(); + + for (parent, netuid, children) in ChildKeys::::iter() { + *weight = weight.saturating_add(T::DbWeight::get().reads(1)); + + // Keep (p, child) only if ParentKeys(child, netuid) contains (p, parent) + let mut filtered: Vec<(u64, T::AccountId)> = Vec::with_capacity(children.len()); + for (p, child) in children.clone().into_iter() { + let rev = ParentKeys::::get(&child, netuid); + *weight = weight.saturating_add(T::DbWeight::get().reads(1)); + let has_match = rev.iter().any(|(pr, pa)| *pr == p && *pa == parent); + if has_match { + filtered.push((p, child)); + } + } + + if filtered.is_empty() { + ck_removes.push((parent, netuid)); + } else { + // Only write if changed + if children != filtered { + ck_updates.push(((parent, netuid), filtered)); + } + } + } + + for ((parent, netuid), new_vec) in &ck_updates { + Self::set_childkeys(parent.clone(), *netuid, new_vec.clone()); + *weight = weight.saturating_add(T::DbWeight::get().writes(1)); + } + for (parent, netuid) in &ck_removes { + ChildKeys::::remove(parent, netuid); + *weight = weight.saturating_add(T::DbWeight::get().writes(1)); + } + log::info!( + target: "runtime", + "Updated {} childkey inconsistent records.", + ck_updates.len() + ); + log::info!( + target: "runtime", + "Removed {} childkey inconsistent records.", + ck_removes.len() + ); + + // ------------------------------- + // 2) Prune ParentKeys by checking ChildKeys + // ------------------------------- + let mut pk_updates: Vec<((T::AccountId, NetUid), Vec<(u64, T::AccountId)>)> = Vec::new(); + let mut pk_removes: Vec<(T::AccountId, NetUid)> = Vec::new(); + + for (child, netuid, parents) in ParentKeys::::iter() { + *weight = weight.saturating_add(T::DbWeight::get().reads(1)); + + // Keep (p, parent) only if ChildKeys(parent, netuid) contains (p, child) + let mut filtered: Vec<(u64, T::AccountId)> = Vec::with_capacity(parents.len()); + for (p, parent) in parents.clone().into_iter() { + let fwd = ChildKeys::::get(&parent, netuid); + *weight = weight.saturating_add(T::DbWeight::get().reads(1)); + let has_match = fwd.iter().any(|(pr, ch)| *pr == p && *ch == child); + if has_match { + filtered.push((p, parent)); + } + } + + if filtered.is_empty() { + pk_removes.push((child, netuid)); + } else { + // Only write if changed + if parents != filtered { + pk_updates.push(((child, netuid), filtered)); + } + } + } + + for ((child, netuid), new_vec) in &pk_updates { + Self::set_parentkeys(child.clone(), *netuid, new_vec.clone()); + *weight = weight.saturating_add(T::DbWeight::get().writes(1)); + } + for (child, netuid) in &pk_removes { + ParentKeys::::remove(child, netuid); + *weight = weight.saturating_add(T::DbWeight::get().writes(1)); + } + log::info!( + target: "runtime", + "Updated {} parentkey inconsistent records.", + pk_updates.len() + ); + log::info!( + target: "runtime", + "Removed {} parentkey inconsistent records.", + pk_removes.len() + ); + } } diff --git a/pallets/subtensor/src/swap/swap_hotkey.rs b/pallets/subtensor/src/swap/swap_hotkey.rs index 82a16bc800..5f617a11e0 100644 --- a/pallets/subtensor/src/swap/swap_hotkey.rs +++ b/pallets/subtensor/src/swap/swap_hotkey.rs @@ -189,7 +189,7 @@ impl Pallet { // 5. execute the hotkey swap on all subnets for netuid in Self::get_all_subnet_netuids() { - Self::perform_hotkey_swap_on_one_subnet(old_hotkey, new_hotkey, weight, netuid); + Self::perform_hotkey_swap_on_one_subnet(old_hotkey, new_hotkey, weight, netuid)?; } // 6. Swap LastTxBlock @@ -320,7 +320,7 @@ impl Pallet { } // 9. Perform the hotkey swap - Self::perform_hotkey_swap_on_one_subnet(old_hotkey, new_hotkey, &mut weight, netuid); + Self::perform_hotkey_swap_on_one_subnet(old_hotkey, new_hotkey, &mut weight, netuid)?; // 10. Update the last transaction block for the coldkey Self::set_last_tx_block(coldkey, block); @@ -344,7 +344,7 @@ impl Pallet { new_hotkey: &T::AccountId, weight: &mut Weight, netuid: NetUid, - ) { + ) -> DispatchResult { // 1. Swap total hotkey alpha for all subnets it exists on. // TotalHotkeyAlpha( hotkey, netuid ) -> alpha -- the total alpha that the hotkey has on a specific subnet. let alpha = TotalHotkeyAlpha::::take(old_hotkey, netuid); @@ -450,62 +450,9 @@ impl Pallet { } } // 4. Swap ChildKeys. - // ChildKeys( parent, netuid ) --> Vec<(proportion,child)> -- the child keys of the parent. - let my_children: Vec<(u64, T::AccountId)> = ChildKeys::::get(old_hotkey, netuid); - // Remove the old hotkey's child entries - ChildKeys::::remove(old_hotkey, netuid); - // Insert the same child entries for the new hotkey - ChildKeys::::insert(new_hotkey, netuid, my_children.clone()); - weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 2)); - - for (_, child_key_i) in my_children { - // For each child, update their parent list - let mut child_parents: Vec<(u64, T::AccountId)> = - ParentKeys::::get(child_key_i.clone(), netuid); - for parent in child_parents.iter_mut() { - // If the parent is the old hotkey, replace it with the new hotkey - if parent.1 == *old_hotkey { - parent.1 = new_hotkey.clone(); - } - } - // Update the child's parent list - ParentKeys::::insert(child_key_i, netuid, child_parents); - weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 1)); - } - // } - // 5. Swap ParentKeys. - // ParentKeys( child, netuid ) --> Vec<(proportion,parent)> -- the parent keys of the child. - let parents: Vec<(u64, T::AccountId)> = ParentKeys::::get(old_hotkey, netuid); - // Remove the old hotkey's parent entries - ParentKeys::::remove(old_hotkey, netuid); - // Insert the same parent entries for the new hotkey - ParentKeys::::insert(new_hotkey, netuid, parents.clone()); - weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 2)); - - for (_, parent_key_i) in parents { - // For each parent, update their children list - let mut parent_children: Vec<(u64, T::AccountId)> = - ChildKeys::::get(parent_key_i.clone(), netuid); - for child in parent_children.iter_mut() { - // If the child is the old hotkey, replace it with the new hotkey - if child.1 == *old_hotkey { - child.1 = new_hotkey.clone(); - } - } - // Update the parent's children list - ChildKeys::::insert(parent_key_i, netuid, parent_children); - weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 1)); - } - // 6. Swap PendingChildKeys. - // PendingChildKeys( netuid, parent ) --> Vec<(proportion,child), cool_down_block> - if PendingChildKeys::::contains_key(netuid, old_hotkey) { - let (children, cool_down_block) = PendingChildKeys::::get(netuid, old_hotkey); - PendingChildKeys::::remove(netuid, old_hotkey); - PendingChildKeys::::insert(netuid, new_hotkey, (children, cool_down_block)); - weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 2)); - } + Self::parent_child_swap_hotkey(old_hotkey, new_hotkey, netuid, weight)?; // Also check for others with our hotkey as a child for (hotkey, (children, cool_down_block)) in PendingChildKeys::::iter_prefix(netuid) { @@ -596,5 +543,7 @@ impl Pallet { } } } + + Ok(()) } } diff --git a/pallets/subtensor/src/tests/children.rs b/pallets/subtensor/src/tests/children.rs index 0fee0af2ca..45fba25c2b 100644 --- a/pallets/subtensor/src/tests/children.rs +++ b/pallets/subtensor/src/tests/children.rs @@ -4170,3 +4170,32 @@ fn test_do_set_childkey_take_rate_limit_exceeded() { )); }); } + +// SKIP_WASM_BUILD=1 RUST_LOG=debug cargo test --package pallet-subtensor --lib -- tests::children::test_set_child_keys_empty_vector_clears_storage --exact --show-output +#[test] +fn test_set_child_keys_empty_vector_clears_storage() { + new_test_ext(1).execute_with(|| { + let sn_owner_hotkey = U256::from(1001); + let sn_owner_coldkey = U256::from(1002); + let parent = U256::from(1); + let child = U256::from(2); + let netuid = add_dynamic_network(&sn_owner_hotkey, &sn_owner_coldkey); + + // Initialize ChildKeys for `parent` with a non-empty vector + ChildKeys::::insert(parent, netuid, vec![(u64::MAX, child)]); + + // Sanity: entry exists right now because we explicitly inserted it + assert!(ChildKeys::::contains_key(parent, netuid)); + + // Set children to empty + let empty_children: Vec<(u64, U256)> = Vec::new(); + mock_set_children_no_epochs(netuid, &parent, &empty_children); + + // When the child vector is empty, we should NOT keep an empty vec in storage. + // The key must be fully removed (no entry), not just zero-length value. + assert!(!ChildKeys::::contains_key(parent, netuid)); + + // `get` returns empty due to ValueQuery default, but presence is false. + assert!(ChildKeys::::get(parent, netuid).is_empty()); + }); +} diff --git a/pallets/subtensor/src/tests/swap_hotkey.rs b/pallets/subtensor/src/tests/swap_hotkey.rs index 5991baf07a..877c8b8720 100644 --- a/pallets/subtensor/src/tests/swap_hotkey.rs +++ b/pallets/subtensor/src/tests/swap_hotkey.rs @@ -1273,14 +1273,10 @@ fn test_swap_complex_parent_child_structure() { assert!(ChildKeys::::get(old_hotkey, netuid).is_empty()); // Verify parent's ChildKeys update - assert_eq!( - ChildKeys::::get(parent1, netuid), - vec![(100u64, new_hotkey), (500u64, U256::from(8))] - ); - assert_eq!( - ChildKeys::::get(parent2, netuid), - vec![(200u64, new_hotkey), (600u64, U256::from(9))] - ); + assert!(ChildKeys::::get(parent1, netuid).contains(&(500u64, U256::from(8))),); + assert!(ChildKeys::::get(parent1, netuid).contains(&(100u64, new_hotkey)),); + assert!(ChildKeys::::get(parent2, netuid).contains(&(600u64, U256::from(9))),); + assert!(ChildKeys::::get(parent2, netuid).contains(&(200u64, new_hotkey)),); }); } @@ -1292,7 +1288,7 @@ fn test_swap_parent_hotkey_childkey_maps() { let coldkey = U256::from(2); let child = U256::from(3); let child_other = U256::from(4); - let parent_new = U256::from(4); + let parent_new = U256::from(5); add_network(netuid, 1, 0); SubtensorModule::create_account_if_non_existent(&coldkey, &parent_old); @@ -1465,3 +1461,49 @@ fn test_swap_hotkey_swap_rate_limits() { ); }); } + +#[test] +fn test_swap_parent_hotkey_self_loops_in_pending() { + new_test_ext(1).execute_with(|| { + let netuid = NetUid::from(1); + let parent_old = U256::from(1); + let coldkey = U256::from(2); + let child = U256::from(3); + let child_other = U256::from(4); + + // Same as child_other, so it will self-loop when pending is set. Should fail. + let parent_new = U256::from(4); + add_network(netuid, 1, 0); + SubtensorModule::create_account_if_non_existent(&coldkey, &parent_old); + + // Set child and verify state maps + mock_set_children(&coldkey, &parent_old, netuid, &[(u64::MAX, child)]); + // Wait rate limit + step_rate_limit(&TransactionType::SetChildren, netuid); + // Schedule some pending child keys. + mock_schedule_children(&coldkey, &parent_old, netuid, &[(u64::MAX, child_other)]); + + assert_eq!( + ParentKeys::::get(child, netuid), + vec![(u64::MAX, parent_old)] + ); + assert_eq!( + ChildKeys::::get(parent_old, netuid), + vec![(u64::MAX, child)] + ); + let existing_pending_child_keys = PendingChildKeys::::get(netuid, parent_old); + assert_eq!(existing_pending_child_keys.0, vec![(u64::MAX, child_other)]); + + // Swap + let mut weight = Weight::zero(); + assert_err!( + SubtensorModule::perform_hotkey_swap_on_all_subnets( + &parent_old, + &parent_new, + &coldkey, + &mut weight + ), + Error::::InvalidChild + ); + }) +} diff --git a/pallets/subtensor/src/tests/swap_hotkey_with_subnet.rs b/pallets/subtensor/src/tests/swap_hotkey_with_subnet.rs index c7baa55387..740f3e97fa 100644 --- a/pallets/subtensor/src/tests/swap_hotkey_with_subnet.rs +++ b/pallets/subtensor/src/tests/swap_hotkey_with_subnet.rs @@ -1129,6 +1129,48 @@ fn test_swap_child_keys() { }); } +// SKIP_WASM_BUILD=1 RUST_LOG=debug cargo test --package pallet-subtensor --lib -- tests::swap_hotkey_with_subnet::test_swap_child_keys_self_loop --exact --show-output +#[test] +fn test_swap_child_keys_self_loop() { + new_test_ext(1).execute_with(|| { + let old_hotkey = U256::from(1); + let new_hotkey = U256::from(2); + let coldkey = U256::from(3); + let netuid = add_dynamic_network(&old_hotkey, &coldkey); + let amount = AlphaCurrency::from(12345); + SubtensorModule::add_balance_to_coldkey_account(&coldkey, u64::MAX); + + // Only for checking + TotalHotkeyAlpha::::insert(old_hotkey, netuid, AlphaCurrency::from(amount)); + + let children = vec![(200u64, new_hotkey)]; + + // Initialize ChildKeys for old_hotkey + ChildKeys::::insert(old_hotkey, netuid, children.clone()); + + // Perform the swap extrinsic + System::set_block_number(System::block_number() + HotkeySwapOnSubnetInterval::get()); + assert_err!( + SubtensorModule::swap_hotkey( + RuntimeOrigin::signed(coldkey), + old_hotkey, + new_hotkey, + Some(netuid) + ), + Error::::InvalidChild + ); + + // Verify the swap didn't happen + assert_eq!(ChildKeys::::get(old_hotkey, netuid), children); + assert!(ChildKeys::::get(new_hotkey, netuid).is_empty()); + assert_eq!(TotalHotkeyAlpha::::get(old_hotkey, netuid), amount); + assert_eq!( + TotalHotkeyAlpha::::get(new_hotkey, netuid), + AlphaCurrency::from(0) + ); + }); +} + // SKIP_WASM_BUILD=1 RUST_LOG=debug cargo test --test swap_hotkey_with_subnet -- test_swap_parent_keys --exact --nocapture #[test] fn test_swap_parent_keys() { @@ -1273,14 +1315,10 @@ fn test_swap_complex_parent_child_structure() { assert!(ChildKeys::::get(old_hotkey, netuid).is_empty()); // Verify parent's ChildKeys update - assert_eq!( - ChildKeys::::get(parent1, netuid), - vec![(100u64, new_hotkey), (500u64, U256::from(8))] - ); - assert_eq!( - ChildKeys::::get(parent2, netuid), - vec![(200u64, new_hotkey), (600u64, U256::from(9))] - ); + assert!(ChildKeys::::get(parent1, netuid).contains(&(500u64, U256::from(8))),); + assert!(ChildKeys::::get(parent1, netuid).contains(&(100u64, new_hotkey)),); + assert!(ChildKeys::::get(parent2, netuid).contains(&(600u64, U256::from(9))),); + assert!(ChildKeys::::get(parent2, netuid).contains(&(200u64, new_hotkey)),); }); } @@ -1291,7 +1329,7 @@ fn test_swap_parent_hotkey_childkey_maps() { let coldkey = U256::from(2); let child = U256::from(3); let child_other = U256::from(4); - let parent_new = U256::from(4); + let parent_new = U256::from(5); let netuid = add_dynamic_network(&parent_old, &coldkey); SubtensorModule::add_balance_to_coldkey_account(&coldkey, u64::MAX); From 21b9671e21520da393289b1eb350c3a0107535db Mon Sep 17 00:00:00 2001 From: Greg Zaitsev Date: Wed, 8 Oct 2025 16:55:16 -0400 Subject: [PATCH 2/6] Move and document set_childkeys and set_parentkeys --- pallets/subtensor/src/staking/set_children.rs | 46 +++++++++---------- 1 file changed, 22 insertions(+), 24 deletions(-) diff --git a/pallets/subtensor/src/staking/set_children.rs b/pallets/subtensor/src/staking/set_children.rs index 9df7dbdf09..e28b1d8b92 100644 --- a/pallets/subtensor/src/staking/set_children.rs +++ b/pallets/subtensor/src/staking/set_children.rs @@ -156,6 +156,28 @@ impl PCRelations { } impl Pallet { + /// Set childkeys vector making sure there are no empty vectors in the state + fn set_childkeys(parent: T::AccountId, netuid: NetUid, childkey_vec: Vec<(u64, T::AccountId)>) { + if childkey_vec.is_empty() { + ChildKeys::::remove(parent, netuid); + } else { + ChildKeys::::insert(parent, netuid, childkey_vec); + } + } + + /// Set parentkeys vector making sure there are no empty vectors in the state + fn set_parentkeys( + child: T::AccountId, + netuid: NetUid, + parentkey_vec: Vec<(u64, T::AccountId)>, + ) { + if parentkey_vec.is_empty() { + ParentKeys::::remove(child, netuid); + } else { + ParentKeys::::insert(child, netuid, parentkey_vec); + } + } + /// Loads all records from ChildKeys and ParentKeys where (hotkey, netuid) is the key. /// Produces a parent->(child->prop) adjacency map that **cannot violate** /// the required consistency because all inserts go through `link`. @@ -759,30 +781,6 @@ impl Pallet { ); } - pub fn set_childkeys( - parent: T::AccountId, - netuid: NetUid, - childkey_vec: Vec<(u64, T::AccountId)>, - ) { - if childkey_vec.is_empty() { - ChildKeys::::remove(parent, netuid); - } else { - ChildKeys::::insert(parent, netuid, childkey_vec); - } - } - - pub fn set_parentkeys( - child: T::AccountId, - netuid: NetUid, - parentkey_vec: Vec<(u64, T::AccountId)>, - ) { - if parentkey_vec.is_empty() { - ParentKeys::::remove(child, netuid); - } else { - ParentKeys::::insert(child, netuid, parentkey_vec); - } - } - /// Remove self-loops in `ChildKeys` and `ParentKeys`. /// If, after removal, a value-vector becomes empty, the storage key is removed. pub fn clean_self_loops(weight: &mut Weight) { From 41d22077c70d219e0897236cc8091f220cfa5f2a Mon Sep 17 00:00:00 2001 From: Greg Zaitsev Date: Wed, 8 Oct 2025 17:02:42 -0400 Subject: [PATCH 3/6] Unignore errors in load_child_parent_relations --- pallets/subtensor/src/staking/set_children.rs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/pallets/subtensor/src/staking/set_children.rs b/pallets/subtensor/src/staking/set_children.rs index e28b1d8b92..ce6e57cb57 100644 --- a/pallets/subtensor/src/staking/set_children.rs +++ b/pallets/subtensor/src/staking/set_children.rs @@ -181,7 +181,10 @@ impl Pallet { /// Loads all records from ChildKeys and ParentKeys where (hotkey, netuid) is the key. /// Produces a parent->(child->prop) adjacency map that **cannot violate** /// the required consistency because all inserts go through `link`. - fn load_child_parent_relations(hotkey: &T::AccountId, netuid: NetUid) -> PCRelations { + fn load_child_parent_relations( + hotkey: &T::AccountId, + netuid: NetUid, + ) -> Result, DispatchError> { let mut rel = PCRelations::::new(hotkey.clone()); // Load children: (prop, child) from ChildKeys(hotkey, netuid) @@ -194,7 +197,7 @@ impl Pallet { } } // Validate & set (enforce no self-loop and sum limit) - let _ = rel.link_children(children); + rel.link_children(children)?; // Load parents: (prop, parent) from ParentKeys(hotkey, netuid) let parent_links = ParentKeys::::get(hotkey, netuid); @@ -205,9 +208,9 @@ impl Pallet { } } // Keep the same validation rules for parents (no self-loop, bounded sum). - let _ = rel.link_parents(parents); + rel.link_parents(parents)?; - rel + Ok(rel) } /// Build a `PCRelations` for `pivot` (parent) from the `PendingChildKeys` queue, @@ -383,7 +386,7 @@ impl Pallet { weight: &mut Weight, ) -> DispatchResult { // 1) Load the current relations around old_hotkey - let mut relations = Self::load_child_parent_relations(old_hotkey, netuid); + let mut relations = Self::load_child_parent_relations(old_hotkey, netuid)?; weight.saturating_accrue(T::DbWeight::get().reads_writes(2, 0)); // 2) Clean up all storage entries that reference old_hotkey @@ -514,7 +517,7 @@ impl Pallet { // - Each child is not the hotkey. // - The sum of the proportions does not exceed u64::MAX. // - Bipartite separation (no A <-> B relations) - let relations = Self::load_child_parent_relations(&hotkey, netuid); + let relations = Self::load_child_parent_relations(&hotkey, netuid)?; relations.ensure_pending_consistency(&children)?; // Check that the parent key has at least the minimum own stake From 765808dea0c0acd4df51dbc668663ecfffae739c Mon Sep 17 00:00:00 2001 From: Greg Zaitsev Date: Wed, 8 Oct 2025 17:11:47 -0400 Subject: [PATCH 4/6] Extend test_set_child_keys_empty_vector_clears_storage to parent keys --- pallets/subtensor/src/tests/children.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pallets/subtensor/src/tests/children.rs b/pallets/subtensor/src/tests/children.rs index 45fba25c2b..ba6ad66808 100644 --- a/pallets/subtensor/src/tests/children.rs +++ b/pallets/subtensor/src/tests/children.rs @@ -4183,9 +4183,11 @@ fn test_set_child_keys_empty_vector_clears_storage() { // Initialize ChildKeys for `parent` with a non-empty vector ChildKeys::::insert(parent, netuid, vec![(u64::MAX, child)]); + ParentKeys::::insert(child, netuid, vec![(u64::MAX, parent)]); // Sanity: entry exists right now because we explicitly inserted it assert!(ChildKeys::::contains_key(parent, netuid)); + assert!(ParentKeys::::contains_key(child, netuid)); // Set children to empty let empty_children: Vec<(u64, U256)> = Vec::new(); @@ -4194,8 +4196,10 @@ fn test_set_child_keys_empty_vector_clears_storage() { // When the child vector is empty, we should NOT keep an empty vec in storage. // The key must be fully removed (no entry), not just zero-length value. assert!(!ChildKeys::::contains_key(parent, netuid)); + assert!(!ParentKeys::::contains_key(child, netuid)); // `get` returns empty due to ValueQuery default, but presence is false. assert!(ChildKeys::::get(parent, netuid).is_empty()); + assert!(ParentKeys::::get(child, netuid).is_empty()); }); } From d0214dbb1e8aab6c8ec2d45dc36b7cdbfd6f92cc Mon Sep 17 00:00:00 2001 From: Greg Zaitsev Date: Wed, 8 Oct 2025 18:34:48 -0400 Subject: [PATCH 5/6] Remove .expect from set_children.rs --- pallets/subtensor/src/macros/hooks.rs | 2 +- pallets/subtensor/src/staking/set_children.rs | 46 ++++++++++++------- 2 files changed, 30 insertions(+), 18 deletions(-) diff --git a/pallets/subtensor/src/macros/hooks.rs b/pallets/subtensor/src/macros/hooks.rs index 6a00099c32..20651f68fc 100644 --- a/pallets/subtensor/src/macros/hooks.rs +++ b/pallets/subtensor/src/macros/hooks.rs @@ -151,7 +151,7 @@ mod hooks { // Migrate subnet burn cost to 2500 .saturating_add(migrations::migrate_network_lock_cost_2500::migrate_network_lock_cost_2500::()) // Cleanup child/parent keys - .saturating_add(migrations::migrate_fix_childkeys::migrate_fix_childkeys::()), + .saturating_add(migrations::migrate_fix_childkeys::migrate_fix_childkeys::()) // Migrate AutoStakeDestinationColdkeys .saturating_add(migrations::migrate_auto_stake_destination::migrate_auto_stake_destination::()); weight diff --git a/pallets/subtensor/src/staking/set_children.rs b/pallets/subtensor/src/staking/set_children.rs index ce6e57cb57..5a1ba2a721 100644 --- a/pallets/subtensor/src/staking/set_children.rs +++ b/pallets/subtensor/src/staking/set_children.rs @@ -273,7 +273,7 @@ impl Pallet { let new_children_map = relations.children(); let new_children_vec: Vec<(u64, T::AccountId)> = new_children_map .iter() - .map(|(c, w)| (*w, c.clone())) + .map(|(c, p)| (*p, c.clone())) .collect(); let prev_children_vec = ChildKeys::::get(&pivot, netuid); @@ -293,30 +293,36 @@ impl Pallet { .iter() .filter(|c| !prev_children_set.contains(*c)) { + let p = match new_children_map.get(added) { + Some(p) => *p, + None => return Err(Error::::ChildParentInconsistency.into()), + }; let mut pk = ParentKeys::::get(added.clone(), netuid); - let w = *new_children_map.get(added).expect("in set; qed"); - PCRelations::::upsert_edge(&mut pk, w, &pivot); - Self::set_parentkeys(added.clone(), netuid, pk.clone()); + PCRelations::::upsert_edge(&mut pk, p, &pivot); + Self::set_parentkeys(added.clone(), netuid, pk); weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 1)); } // Updated children = intersection where proportion changed for common in new_children_set.intersection(&prev_children_set) { - let new_p = *new_children_map.get(common).expect("in set; qed"); + let new_p = match new_children_map.get(common) { + Some(p) => *p, + None => return Err(Error::::ChildParentInconsistency.into()), + }; let mut pk = ParentKeys::::get(common.clone(), netuid); PCRelations::::upsert_edge(&mut pk, new_p, &pivot); - Self::set_parentkeys(common.clone(), netuid, pk.clone()); + Self::set_parentkeys(common.clone(), netuid, pk); weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 1)); } - // Removed children = prev / new + // Removed children = prev \ new => remove (pivot) from ParentKeys(child) for removed in prev_children_set .iter() .filter(|c| !new_children_set.contains(*c)) { - let mut pk = ParentKeys::::get(&removed, netuid); + let mut pk = ParentKeys::::get(removed.clone(), netuid); PCRelations::::remove_edge(&mut pk, &pivot); - Self::set_parentkeys(removed.clone(), netuid, pk.clone()); + Self::set_parentkeys(removed.clone(), netuid, pk); weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 1)); } @@ -326,7 +332,7 @@ impl Pallet { let new_parents_map = relations.parents(); let new_parents_vec: Vec<(u64, T::AccountId)> = new_parents_map .iter() - .map(|(p, w)| (*w, p.clone())) + .map(|(p, pr)| (*pr, p.clone())) .collect(); let prev_parents_vec = ParentKeys::::get(&pivot, netuid); @@ -338,28 +344,34 @@ impl Pallet { prev_parents_vec.iter().map(|(_, p)| p.clone()).collect(); let new_parents_set: BTreeSet = new_parents_map.keys().cloned().collect(); - // Added parents = new / prev => ensure ChildKeys(parent) has (w, pivot) + // Added parents = new / prev => ensure ChildKeys(parent) has (p, pivot) for added in new_parents_set .iter() .filter(|p| !prev_parents_set.contains(*p)) { + let p_val = match new_parents_map.get(added) { + Some(p) => *p, + None => return Err(Error::::ChildParentInconsistency.into()), + }; let mut ck = ChildKeys::::get(added.clone(), netuid); - let w = *new_parents_map.get(added).expect("in set; qed"); - PCRelations::::upsert_edge(&mut ck, w, &pivot); + PCRelations::::upsert_edge(&mut ck, p_val, &pivot); Self::set_childkeys(added.clone(), netuid, ck); weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 1)); } - // Updated parents = intersection where weight changed + // Updated parents = intersection where proportion changed for common in new_parents_set.intersection(&prev_parents_set) { - let new_w = *new_parents_map.get(common).expect("in set; qed"); + let new_p = match new_parents_map.get(common) { + Some(p) => *p, + None => return Err(Error::::ChildParentInconsistency.into()), + }; let mut ck = ChildKeys::::get(common.clone(), netuid); - PCRelations::::upsert_edge(&mut ck, new_w, &pivot); + PCRelations::::upsert_edge(&mut ck, new_p, &pivot); Self::set_childkeys(common.clone(), netuid, ck); weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 1)); } - // Removed parents = prev / new => remove (pivot) from ChildKeys(parent) + // Removed parents = prev \ new => remove (pivot) from ChildKeys(parent) for removed in prev_parents_set .iter() .filter(|p| !new_parents_set.contains(*p)) From 1b9eb7df69fc5e75dfba7c81c3b905829bf38c1a Mon Sep 17 00:00:00 2001 From: Greg Zaitsev Date: Thu, 9 Oct 2025 10:55:03 -0400 Subject: [PATCH 6/6] Address code review comments --- pallets/subtensor/src/staking/set_children.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/pallets/subtensor/src/staking/set_children.rs b/pallets/subtensor/src/staking/set_children.rs index 5a1ba2a721..c7ebd62c04 100644 --- a/pallets/subtensor/src/staking/set_children.rs +++ b/pallets/subtensor/src/staking/set_children.rs @@ -341,7 +341,7 @@ impl Pallet { Self::set_parentkeys(pivot.clone(), netuid, new_parents_vec.clone()); let prev_parents_set: BTreeSet = - prev_parents_vec.iter().map(|(_, p)| p.clone()).collect(); + prev_parents_vec.into_iter().map(|(_, p)| p).collect(); let new_parents_set: BTreeSet = new_parents_map.keys().cloned().collect(); // Added parents = new / prev => ensure ChildKeys(parent) has (p, pivot) @@ -361,12 +361,11 @@ impl Pallet { // Updated parents = intersection where proportion changed for common in new_parents_set.intersection(&prev_parents_set) { - let new_p = match new_parents_map.get(common) { - Some(p) => *p, - None => return Err(Error::::ChildParentInconsistency.into()), - }; + let new_p = new_parents_map + .get(common) + .ok_or(Error::::ChildParentInconsistency)?; let mut ck = ChildKeys::::get(common.clone(), netuid); - PCRelations::::upsert_edge(&mut ck, new_p, &pivot); + PCRelations::::upsert_edge(&mut ck, *new_p, &pivot); Self::set_childkeys(common.clone(), netuid, ck); weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 1)); }