From 97c1e0ff86f05e1c0d91a125ba69db8e48c52608 Mon Sep 17 00:00:00 2001 From: markojovanovic_elite Date: Wed, 22 Apr 2026 15:27:17 -0500 Subject: [PATCH 1/2] fix: match remove-side NetUid::ROOT guard in add_stake_adjust_root_claimed remove_stake_adjust_root_claimed_for_hotkey_and_coldkey explicitly skips NetUid::ROOT when iterating over RootClaimable, but the add counterpart does not. Under normal operation run_coinbase excludes ROOT from the coinbase loop (run_coinbase.rs:31), so RootClaimable should never carry a ROOT entry and the asymmetry is invisible. If anything ever inserts a ROOT entry though, RootClaimed would tick up on every stake add but never drain on remove, since only the remove side has the guard. Over enough cycles RootClaimed for the ROOT tuple would exceed claimable and the existing saturating_sub in get_root_owed_* would silently clamp owed to 0. Mirror the guard in the add path and add a regression test that seeds RootClaimable with both a ROOT entry and a normal netuid entry, calls add_stake_adjust_root_claimed, and checks ROOT stays at 0 while the normal entry is bumped by rate * amount. --- pallets/subtensor/src/staking/claim_root.rs | 11 +++++ pallets/subtensor/src/tests/claim_root.rs | 53 +++++++++++++++++++++ 2 files changed, 64 insertions(+) diff --git a/pallets/subtensor/src/staking/claim_root.rs b/pallets/subtensor/src/staking/claim_root.rs index 58babb79a6..c27faa2d1d 100644 --- a/pallets/subtensor/src/staking/claim_root.rs +++ b/pallets/subtensor/src/staking/claim_root.rs @@ -253,6 +253,17 @@ impl Pallet { // Iterate over all the subnets this hotkey is staked on for root. let root_claimable = RootClaimable::::get(hotkey); for (netuid, claimable_rate) in root_claimable.iter() { + // RootClaimable is keyed by the *emitting* subnet, so NetUid::ROOT + // should never appear here under normal coinbase operation + // (run_coinbase filters ROOT out before populating this map). + // Skip it defensively to stay symmetric with + // `remove_stake_adjust_root_claimed_for_hotkey_and_coldkey`; + // otherwise a stray ROOT entry would grow RootClaimed on adds + // but never shrink on removes. + if *netuid == NetUid::ROOT.into() { + continue; + } + // Get current staker root claimed value. let root_claimed: u128 = RootClaimed::::get((netuid, hotkey, coldkey)); diff --git a/pallets/subtensor/src/tests/claim_root.rs b/pallets/subtensor/src/tests/claim_root.rs index f86ca0b1e6..ee1e331bde 100644 --- a/pallets/subtensor/src/tests/claim_root.rs +++ b/pallets/subtensor/src/tests/claim_root.rs @@ -2059,3 +2059,56 @@ fn test_claim_root_with_moved_stake() { assert_abs_diff_eq!(bob_stake_diff2, estimated_stake as u64, epsilon = 100u64,); }); } + +// Symmetric to remove_stake_adjust_root_claimed: an unexpected ROOT entry in +// RootClaimable should be skipped by the add path as well. Previously only +// the remove side guarded against it, which meant a stray ROOT entry would +// grow RootClaimed on adds without ever being drained on removes. +#[test] +fn test_add_stake_adjust_root_claimed_skips_root_netuid() { + new_test_ext(1).execute_with(|| { + let hotkey = U256::from(1); + let coldkey = U256::from(2); + let other_netuid = NetUid::from(7); + + let rate = I96F32::saturating_from_num(3); + + // Seed RootClaimable with both a ROOT entry and a non-root entry. + // ROOT should not normally be present here, but we want the add path + // to tolerate it the same way the remove path already does. + let mut claimable = std::collections::BTreeMap::new(); + claimable.insert(NetUid::ROOT, rate); + claimable.insert(other_netuid, rate); + RootClaimable::::insert(hotkey, claimable); + + // Nothing claimed yet. + assert_eq!( + RootClaimed::::get((NetUid::ROOT, hotkey, coldkey)), + 0u128 + ); + assert_eq!( + RootClaimed::::get((other_netuid, hotkey, coldkey)), + 0u128 + ); + + let amount: u64 = 100; + SubtensorModule::add_stake_adjust_root_claimed_for_hotkey_and_coldkey( + &hotkey, &coldkey, amount, + ); + + // ROOT entry must stay untouched. + assert_eq!( + RootClaimed::::get((NetUid::ROOT, hotkey, coldkey)), + 0u128 + ); + + // The non-root entry should have been bumped by rate * amount. + let expected: u128 = rate + .saturating_mul(I96F32::from(amount)) + .saturating_to_num::(); + assert_eq!( + RootClaimed::::get((other_netuid, hotkey, coldkey)), + expected + ); + }); +} From 83b8e7a000bed608223d432eeb38b0ce27ebf602 Mon Sep 17 00:00:00 2001 From: markojovanovic_elite Date: Wed, 22 Apr 2026 21:24:10 -0500 Subject: [PATCH 2/2] flip approach: drop remove-side ROOT guard, document the invariant Per review: RootClaimable is never populated with a NetUid::ROOT key under any current code path, so the original remove-side guard was itself defensive against a case that can't happen. Mirroring that guard onto the add side was chasing a ghost. Do the other direction: drop the ROOT special-case from remove and leave a block comment at the top of the iteration pair explaining why RootClaimable never carries ROOT. `increase_root_claimable_for_hotkey_and_subnet` is the only writer, called from `run_coinbase.rs:627` inside the `netuid != NetUid::ROOT` loop (run_coinbase.rs:31). `transfer_root_claimable_for_new_hotkey` and `finalize_all_subnet_root_dividends` only propagate or remove existing keys. Both functions now iterate the map uniformly. Rewrite the test to pin the symmetric-iteration property: seed both a ROOT entry and a regular entry into RootClaimable, call the add path, then call the remove path with the same amount, and assert both entries drain back to zero. --- pallets/subtensor/src/staking/claim_root.rs | 28 +++++------- pallets/subtensor/src/tests/claim_root.rs | 47 ++++++++++++--------- 2 files changed, 38 insertions(+), 37 deletions(-) diff --git a/pallets/subtensor/src/staking/claim_root.rs b/pallets/subtensor/src/staking/claim_root.rs index c27faa2d1d..e80177e04b 100644 --- a/pallets/subtensor/src/staking/claim_root.rs +++ b/pallets/subtensor/src/staking/claim_root.rs @@ -245,25 +245,24 @@ impl Pallet { weight } + // Invariant: `RootClaimable[hotkey]` never carries a `NetUid::ROOT` key. + // `increase_root_claimable_for_hotkey_and_subnet` is the only writer, and + // its sole caller `run_coinbase` filters ROOT out of the distribution loop + // before populating the map (see `run_coinbase.rs:31`). + // `transfer_root_claimable_for_new_hotkey` and + // `finalize_all_subnet_root_dividends` only propagate or remove existing + // keys, so they can't introduce a ROOT key either. The two functions + // below therefore iterate the full map without a ROOT special-case; if a + // ROOT entry ever does appear through a future code path, add and remove + // will stay symmetric rather than drifting. + pub fn add_stake_adjust_root_claimed_for_hotkey_and_coldkey( hotkey: &T::AccountId, coldkey: &T::AccountId, amount: u64, ) { - // Iterate over all the subnets this hotkey is staked on for root. let root_claimable = RootClaimable::::get(hotkey); for (netuid, claimable_rate) in root_claimable.iter() { - // RootClaimable is keyed by the *emitting* subnet, so NetUid::ROOT - // should never appear here under normal coinbase operation - // (run_coinbase filters ROOT out before populating this map). - // Skip it defensively to stay symmetric with - // `remove_stake_adjust_root_claimed_for_hotkey_and_coldkey`; - // otherwise a stray ROOT entry would grow RootClaimed on adds - // but never shrink on removes. - if *netuid == NetUid::ROOT.into() { - continue; - } - // Get current staker root claimed value. let root_claimed: u128 = RootClaimed::::get((netuid, hotkey, coldkey)); @@ -284,13 +283,8 @@ impl Pallet { coldkey: &T::AccountId, amount: AlphaBalance, ) { - // Iterate over all the subnets this hotkey is staked on for root. let root_claimable = RootClaimable::::get(hotkey); for (netuid, claimable_rate) in root_claimable.iter() { - if *netuid == NetUid::ROOT.into() { - continue; // Skip the root netuid. - } - // Get current staker root claimed value. let root_claimed: u128 = RootClaimed::::get((netuid, hotkey, coldkey)); diff --git a/pallets/subtensor/src/tests/claim_root.rs b/pallets/subtensor/src/tests/claim_root.rs index ee1e331bde..71d099b84c 100644 --- a/pallets/subtensor/src/tests/claim_root.rs +++ b/pallets/subtensor/src/tests/claim_root.rs @@ -2060,12 +2060,14 @@ fn test_claim_root_with_moved_stake() { }); } -// Symmetric to remove_stake_adjust_root_claimed: an unexpected ROOT entry in -// RootClaimable should be skipped by the add path as well. Previously only -// the remove side guarded against it, which meant a stray ROOT entry would -// grow RootClaimed on adds without ever being drained on removes. +// RootClaimable should never carry a NetUid::ROOT key under current code +// paths, so this scenario isn't reachable today. The test pins the +// symmetric-iteration property: if a ROOT entry ever does sneak in through +// a future code path, add and remove both process it the same way so the +// pair stays balanced rather than drifting as it would if only one side +// handled it specially. #[test] -fn test_add_stake_adjust_root_claimed_skips_root_netuid() { +fn test_add_and_remove_stake_adjust_root_claimed_are_symmetric() { new_test_ext(1).execute_with(|| { let hotkey = U256::from(1); let coldkey = U256::from(2); @@ -2074,41 +2076,46 @@ fn test_add_stake_adjust_root_claimed_skips_root_netuid() { let rate = I96F32::saturating_from_num(3); // Seed RootClaimable with both a ROOT entry and a non-root entry. - // ROOT should not normally be present here, but we want the add path - // to tolerate it the same way the remove path already does. + // ROOT should not normally be here; we seed it to verify the two + // functions stay symmetric regardless. let mut claimable = std::collections::BTreeMap::new(); claimable.insert(NetUid::ROOT, rate); claimable.insert(other_netuid, rate); RootClaimable::::insert(hotkey, claimable); - // Nothing claimed yet. + let amount: u64 = 100; + SubtensorModule::add_stake_adjust_root_claimed_for_hotkey_and_coldkey( + &hotkey, &coldkey, amount, + ); + + let expected: u128 = rate + .saturating_mul(I96F32::from(amount)) + .saturating_to_num::(); + + // Both entries got bumped the same way by the add path. assert_eq!( RootClaimed::::get((NetUid::ROOT, hotkey, coldkey)), - 0u128 + expected ); assert_eq!( RootClaimed::::get((other_netuid, hotkey, coldkey)), - 0u128 + expected ); - let amount: u64 = 100; - SubtensorModule::add_stake_adjust_root_claimed_for_hotkey_and_coldkey( - &hotkey, &coldkey, amount, + // Remove the same amount and check both entries drain back to 0. + SubtensorModule::remove_stake_adjust_root_claimed_for_hotkey_and_coldkey( + &hotkey, + &coldkey, + AlphaBalance::from(amount), ); - // ROOT entry must stay untouched. assert_eq!( RootClaimed::::get((NetUid::ROOT, hotkey, coldkey)), 0u128 ); - - // The non-root entry should have been bumped by rate * amount. - let expected: u128 = rate - .saturating_mul(I96F32::from(amount)) - .saturating_to_num::(); assert_eq!( RootClaimed::::get((other_netuid, hotkey, coldkey)), - expected + 0u128 ); }); }