diff --git a/pallets/subtensor/src/macros/hooks.rs b/pallets/subtensor/src/macros/hooks.rs index 31be3e5e4f..0a782641b9 100644 --- a/pallets/subtensor/src/macros/hooks.rs +++ b/pallets/subtensor/src/macros/hooks.rs @@ -143,7 +143,7 @@ mod hooks { .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::()) - // Cleanup child/parent keys + // Cleanup child/parent keys (includes removing self-loops) .saturating_add(migrations::migrate_fix_childkeys::migrate_fix_childkeys::()) // Migrate AutoStakeDestinationColdkeys .saturating_add(migrations::migrate_auto_stake_destination::migrate_auto_stake_destination::()) diff --git a/pallets/subtensor/src/staking/set_children.rs b/pallets/subtensor/src/staking/set_children.rs index c7ebd62c04..3144d791b0 100644 --- a/pallets/subtensor/src/staking/set_children.rs +++ b/pallets/subtensor/src/staking/set_children.rs @@ -143,6 +143,11 @@ impl PCRelations { 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 Pallet { 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 Pallet { 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::::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)); + let (mut children, cool_down_block) = PendingChildKeys::::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::::remove(netuid, old_hotkey); + PendingChildKeys::::insert(netuid, new_hotkey, (children, cool_down_block)); + } else { + // If all children were filtered out, just remove the pending entry + PendingChildKeys::::remove(netuid, old_hotkey); + } weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 2)); } diff --git a/pallets/subtensor/src/swap/swap_hotkey.rs b/pallets/subtensor/src/swap/swap_hotkey.rs index 4fdf87fb7b..b3ba3e1083 100644 --- a/pallets/subtensor/src/swap/swap_hotkey.rs +++ b/pallets/subtensor/src/swap/swap_hotkey.rs @@ -430,6 +430,7 @@ impl Pallet { 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::::iter_prefix(netuid) { weight.saturating_accrue(T::DbWeight::get().reads(1)); @@ -438,10 +439,17 @@ impl Pallet { { 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::::remove(netuid, hotkey.clone()); - PendingChildKeys::::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::::remove(netuid, hotkey.clone()); + } else { + PendingChildKeys::::remove(netuid, hotkey.clone()); + PendingChildKeys::::insert(netuid, hotkey, (new_children, cool_down_block)); + } weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 2)); } } diff --git a/pallets/subtensor/src/tests/swap_hotkey.rs b/pallets/subtensor/src/tests/swap_hotkey.rs index 71191d1951..ecb66db3a6 100644 --- a/pallets/subtensor/src/tests/swap_hotkey.rs +++ b/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::::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::::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::::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::::insert(old_hotkey, netuid, vec![(100u64, new_hotkey)]); + ParentKeys::::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::::get(new_hotkey, netuid); + assert!(!children.iter().any(|(_, child)| *child == new_hotkey)); + + // Verify old_hotkey has no children + assert!(ChildKeys::::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::::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::::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::::insert( + old_hotkey, + netuid, + vec![(100u64, new_hotkey), (200u64, other_child)], + ); + ParentKeys::::insert(new_hotkey, netuid, vec![(100u64, old_hotkey)]); + ParentKeys::::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::::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); + }); +}