fix: SwapHotkey self-loop check for childkeys

pallets/subtensor/src/macros/hooks.rs

@@ -143,7 +143,7 @@ mod hooks {
                 .saturating_add(migrations::migrate_subnet_locked::migrate_restore_subnet_locked::<T>())
                 // Migrate subnet burn cost to 2500
                 .saturating_add(migrations::migrate_network_lock_cost_2500::migrate_network_lock_cost_2500::<T>())
-                // Cleanup child/parent keys
+                // Cleanup child/parent keys (includes removing self-loops)
                 .saturating_add(migrations::migrate_fix_childkeys::migrate_fix_childkeys::<T>())
                 // Migrate AutoStakeDestinationColdkeys
                 .saturating_add(migrations::migrate_auto_stake_destination::migrate_auto_stake_destination::<T>())

pallets/subtensor/src/staking/set_children.rs

@@ -143,6 +143,11 @@ impl<T: Config> PCRelations<T> {
         list.retain(|(_, who)| who != id);
     }
 
+    /// Remove a child from the relations.
+    pub fn unlink_child(&mut self, child: T::AccountId) {
+        self.children.remove(&child);
+    }
+
     /// 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 {
@@ -400,6 +405,12 @@ impl<T: Config> Pallet<T> {
         let mut relations = Self::load_child_parent_relations(old_hotkey, netuid)?;
         weight.saturating_accrue(T::DbWeight::get().reads_writes(2, 0));
 
+        // 1.5) Remove new_hotkey from children if it exists to prevent self-loop after swap
+        // This allows the swap to proceed even if new_hotkey is a child of old_hotkey
+        if relations.children().contains_key(new_hotkey) {
+            relations.unlink_child(new_hotkey.clone());
+        }
+
         // 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() {
@@ -432,13 +443,20 @@ impl<T: Config> Pallet<T> {
         relations.rebind_pivot(new_hotkey.clone())?;
 
         // 4) Swap PendingChildKeys( netuid, parent ) --> Vec<(proportion,child), cool_down_block>
-        // Fail if consistency breaks
+        // Filter out new_hotkey from pending children to prevent self-loop
         if PendingChildKeys::<T>::contains_key(netuid, old_hotkey) {
-            let (children, cool_down_block) = PendingChildKeys::<T>::get(netuid, old_hotkey);
-            relations.ensure_pending_consistency(&children)?;
-
-            PendingChildKeys::<T>::remove(netuid, old_hotkey);
-            PendingChildKeys::<T>::insert(netuid, new_hotkey, (children, cool_down_block));
+            let (mut children, cool_down_block) = PendingChildKeys::<T>::get(netuid, old_hotkey);
+            // Remove new_hotkey from pending children to prevent self-loop
+            children.retain(|(_, child)| *child != *new_hotkey);
+
+            if !children.is_empty() {
+                relations.ensure_pending_consistency(&children)?;
+                PendingChildKeys::<T>::remove(netuid, old_hotkey);
+                PendingChildKeys::<T>::insert(netuid, new_hotkey, (children, cool_down_block));
+            } else {
+                // If all children were filtered out, just remove the pending entry
+                PendingChildKeys::<T>::remove(netuid, old_hotkey);
+            }
             weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 2));
         }
 

pallets/subtensor/src/swap/swap_hotkey.rs

@@ -430,6 +430,7 @@ impl<T: Config> Pallet<T> {
         Self::parent_child_swap_hotkey(old_hotkey, new_hotkey, netuid, weight)?;
 
         // Also check for others with our hotkey as a child
+        // Update PendingChildKeys for other hotkeys that have old_hotkey as a pending child
         for (hotkey, (children, cool_down_block)) in PendingChildKeys::<T>::iter_prefix(netuid) {
             weight.saturating_accrue(T::DbWeight::get().reads(1));
 
@@ -438,10 +439,17 @@ impl<T: Config> Pallet<T> {
             {
                 let mut new_children = children.clone();
                 let entry_to_remove = new_children.remove(potential_idx);
-                new_children.push((entry_to_remove.0, new_hotkey.clone())); // Keep the proportion.
-
-                PendingChildKeys::<T>::remove(netuid, hotkey.clone());
-                PendingChildKeys::<T>::insert(netuid, hotkey, (new_children, cool_down_block));
+                // Only add new_hotkey if it's not the same as hotkey (to prevent self-loop)
+                if new_hotkey != &hotkey {
+                    new_children.push((entry_to_remove.0, new_hotkey.clone())); // Keep the proportion.
+                }
+                // If new_children is empty after filtering, remove the pending entry
+                if new_children.is_empty() {
+                    PendingChildKeys::<T>::remove(netuid, hotkey.clone());
+                } else {
+                    PendingChildKeys::<T>::remove(netuid, hotkey.clone());
+                    PendingChildKeys::<T>::insert(netuid, hotkey, (new_children, cool_down_block));
+                }
                 weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 2));
             }
         }

pallets/subtensor/src/tests/swap_hotkey.rs

@@ -1457,17 +1457,18 @@ fn test_swap_parent_hotkey_self_loops_in_pending() {
         let existing_pending_child_keys = PendingChildKeys::<Test>::get(netuid, parent_old);
         assert_eq!(existing_pending_child_keys.0, vec![(u64::MAX, child_other)]);
 
-        // Swap
+        // Swap - should succeed but remove self-loop
         let mut weight = Weight::zero();
-        assert_err!(
-            SubtensorModule::perform_hotkey_swap_on_all_subnets(
-                &parent_old,
-                &parent_new,
-                &coldkey,
-                &mut weight
-            ),
-            Error::<Test>::InvalidChild
-        );
+        assert_ok!(SubtensorModule::perform_hotkey_swap_on_all_subnets(
+            &parent_old,
+            &parent_new,
+            &coldkey,
+            &mut weight
+        ));
+
+        // Verify that parent_new does NOT have child_other as a child (self-loop prevented)
+        // The swap should have filtered out child_other since it equals parent_new
+        assert!(!ChildKeys::<Test>::get(parent_new, netuid).iter().any(|(_, c)| *c == child_other));
     })
 }
 
@@ -1506,3 +1507,120 @@ fn test_swap_auto_stake_destination_coldkeys() {
         );
     });
 }
+
+// Test that swapping to a child hotkey removes the self-loop instead of failing
+#[test]
+fn test_swap_hotkey_to_child_removes_self_loop() {
+    new_test_ext(1).execute_with(|| {
+        let old_hotkey = U256::from(1);
+        let new_hotkey = U256::from(2); // This will be a child of old_hotkey
+        let coldkey = U256::from(3);
+        let netuid = NetUid::from(1u16);
+        let mut weight = Weight::zero();
+
+        add_network(netuid, 1, 0);
+        SubtensorModule::create_account_if_non_existent(&coldkey, &old_hotkey);
+        SubtensorModule::create_account_if_non_existent(&coldkey, &new_hotkey);
+
+        // Set up: new_hotkey is a child of old_hotkey
+        ChildKeys::<Test>::insert(old_hotkey, netuid, vec![(100u64, new_hotkey)]);
+        ParentKeys::<Test>::insert(new_hotkey, netuid, vec![(100u64, old_hotkey)]);
+
+        // Perform the swap - should succeed
+        assert_ok!(SubtensorModule::perform_hotkey_swap_on_all_subnets(
+            &old_hotkey,
+            &new_hotkey,
+            &coldkey,
+            &mut weight
+        ));
+
+        // Verify that new_hotkey does NOT have itself as a child (self-loop was removed)
+        let children = ChildKeys::<Test>::get(new_hotkey, netuid);
+        assert!(!children.iter().any(|(_, child)| *child == new_hotkey));
+
+        // Verify old_hotkey has no children
+        assert!(ChildKeys::<Test>::get(old_hotkey, netuid).is_empty());
+    });
+}
+
+// Test that swapping filters out new_hotkey from PendingChildKeys
+#[test]
+fn test_swap_hotkey_filters_pending_child_self_loop() {
+    new_test_ext(1).execute_with(|| {
+        let old_hotkey = U256::from(1);
+        let new_hotkey = U256::from(2); // This will be in PendingChildKeys for old_hotkey
+        let coldkey = U256::from(3);
+        let netuid = NetUid::from(1u16);
+        let mut weight = Weight::zero();
+        let current_block = SubtensorModule::get_current_block_as_u64();
+        let cooldown_block = current_block + 100;
+
+        add_network(netuid, 1, 0);
+        SubtensorModule::create_account_if_non_existent(&coldkey, &old_hotkey);
+        SubtensorModule::create_account_if_non_existent(&coldkey, &new_hotkey);
+
+        // Set up: new_hotkey is in PendingChildKeys for old_hotkey
+        PendingChildKeys::<Test>::insert(
+            netuid,
+            old_hotkey,
+            (vec![(100u64, new_hotkey), (200u64, U256::from(4))], cooldown_block),
+        );
+
+        // Perform the swap - should succeed
+        assert_ok!(SubtensorModule::perform_hotkey_swap_on_all_subnets(
+            &old_hotkey,
+            &new_hotkey,
+            &coldkey,
+            &mut weight
+        ));
+
+        // Verify that new_hotkey's PendingChildKeys does NOT contain itself
+        let pending = PendingChildKeys::<Test>::get(netuid, new_hotkey);
+        assert!(!pending.0.iter().any(|(_, child)| *child == new_hotkey));
+        // Should only contain the other child (U256::from(4))
+        assert_eq!(pending.0.len(), 1);
+        assert_eq!(pending.0[0].1, U256::from(4));
+    });
+}
+
+// Test that swapping when new_hotkey is a child with other children preserves other children
+#[test]
+fn test_swap_hotkey_to_child_preserves_other_children() {
+    new_test_ext(1).execute_with(|| {
+        let old_hotkey = U256::from(1);
+        let new_hotkey = U256::from(2); // This will be a child of old_hotkey
+        let other_child = U256::from(5);
+        let coldkey = U256::from(3);
+        let netuid = NetUid::from(1u16);
+        let mut weight = Weight::zero();
+
+        add_network(netuid, 1, 0);
+        SubtensorModule::create_account_if_non_existent(&coldkey, &old_hotkey);
+        SubtensorModule::create_account_if_non_existent(&coldkey, &new_hotkey);
+        SubtensorModule::create_account_if_non_existent(&coldkey, &other_child);
+
+        // Set up: old_hotkey has both new_hotkey and other_child as children
+        ChildKeys::<Test>::insert(
+            old_hotkey,
+            netuid,
+            vec![(100u64, new_hotkey), (200u64, other_child)],
+        );
+        ParentKeys::<Test>::insert(new_hotkey, netuid, vec![(100u64, old_hotkey)]);
+        ParentKeys::<Test>::insert(other_child, netuid, vec![(200u64, old_hotkey)]);
+
+        // Perform the swap - should succeed
+        assert_ok!(SubtensorModule::perform_hotkey_swap_on_all_subnets(
+            &old_hotkey,
+            &new_hotkey,
+            &coldkey,
+            &mut weight
+        ));
+
+        // Verify that new_hotkey has other_child as a child (self-loop was removed, but other_child preserved)
+        let children = ChildKeys::<Test>::get(new_hotkey, netuid);
+        assert!(!children.iter().any(|(_, child)| *child == new_hotkey)); // No self-loop
+        assert!(children.iter().any(|(_, child)| *child == other_child)); // Other child preserved
+        assert_eq!(children.len(), 1); // Only other_child remains
+        assert_eq!(children[0].1, other_child);
+    });
+}