fix: match remove-side NetUid::ROOT guard in add_stake_adjust_root_cl…#2611
fix: match remove-side NetUid::ROOT guard in add_stake_adjust_root_cl…#2611boskodev790 wants to merge 2 commits intoopentensor:mainfrom
Conversation
…aimed 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.
|
RootClaimable can be inserted during run coinbase, which already exclulde the ROOT. could you specify any execution path "If anything ever inserts a ROOT entry though". |
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.
|
You're right, I went back through the write sites and there isn't one. Flipped the approach in the latest commit: dropped the guard from the remove side too, left a block comment above the pair pointing at the invariant and the line that enforces it. The test is rewritten as a symmetric-iteration check — seed ROOT + a regular entry into
|
2 participants
…aimed
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.
Description
Related Issue(s)
Type of Change
Breaking Change
If this PR introduces a breaking change, please provide a detailed description of the impact and the migration path for existing applications.
Checklist
./scripts/fix_rust.shto ensure my code is formatted and linted correctlyScreenshots (if applicable)
Please include any relevant screenshots or GIFs that demonstrate the changes made.
Additional Notes
Please provide any additional information or context that may be helpful for reviewers.