Skip to content

fix(coinbase): clear AutoStakeDestination on subnet dissolve#2652

Open
RUNECTZ33 wants to merge 2 commits into
opentensor:devnet-readyfrom
RUNECTZ33:fix/autostake-destination-cleanup-on-dissolve
Open

fix(coinbase): clear AutoStakeDestination on subnet dissolve#2652
RUNECTZ33 wants to merge 2 commits into
opentensor:devnet-readyfrom
RUNECTZ33:fix/autostake-destination-cleanup-on-dissolve

Conversation

@RUNECTZ33
Copy link
Copy Markdown

Description

AutoStakeDestination and its reverse index AutoStakeDestinationColdkeys are StorageDoubleMaps with netuid as the second key. Neither is cleaned up by remove_network during subnet dissolution. Because netuids are recycled by subsequent do_register_network calls, a stale (coldkey, dissolved_netuid) → hotkey mapping silently survives and applies to the next subnet registered on that netuid.

The misdirection surfaces in coinbase::run_coinbase (auto-stake path) at run_coinbase.rs:557:

let owner: T::AccountId = Owner::<T>::get(&hotkey);
let maybe_dest = AutoStakeDestination::<T>::get(&owner, netuid);
// ...
let destination = maybe_dest.clone().unwrap_or(hotkey.clone());

The lookup returns the stale destination from the previous occupant of that netuid. Mining incentive on the newly-registered subnet is then auto-staked to a hotkey that has no relationship to the new subnet.

Fix

Add two iter().filter_map().remove() blocks in the --- 21. section of remove_network, matching the existing pattern used for the other DMaps that embed netuid as a non-leading key (ChildkeyTake, ChildKeys, ParentKeys, LastHotkeyEmissionOnNetuid, TotalHotkeyAlphaLastEpoch, StakingOperationRateLimiter).

Tests

  • Extended dissolve_clears_all_per_subnet_storages to populate both maps before dissolve and assert both are cleared after.
  • Added a focused regression dissolve_clears_auto_stake_destination_preventing_stale_routing that exercises the user-facing routing hazard directly.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Other (please describe):

Breaking Change

N/A — adds cleanup that was previously missing. No behavioral change for live subnets, no migration, no new dependencies, no API changes.

Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have run ./scripts/fix_rust.sh to ensure my code is formatted and linted correctly (no local Rust toolchain — relying on CI; happy to follow up on any fmt/clippy nits)
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings (visual style matches surrounding existing patterns)
  • I have added tests that prove my fix

`AutoStakeDestination` and its reverse index `AutoStakeDestinationColdkeys`
both embed `netuid` as the second key of a `StorageDoubleMap`, but neither
is cleaned up by `remove_network` during subnet dissolution. Because
netuids are recycled by subsequent `do_register_network` calls, a stale
`(coldkey, dissolved_netuid) → hotkey` mapping silently survives and
applies to the *next* subnet registered on that netuid.

The misdirection surfaces in `coinbase::run_coinbase` (auto-stake path),
where the lookup `AutoStakeDestination::<T>::get(&owner, netuid)` returns
the stale destination from the previous occupant of that netuid. Mining
incentive on the newly-registered subnet is then auto-staked to a hotkey
that has no relationship to the new subnet.

Add two `iter().filter_map().remove()` blocks in the `--- 21.` section of
`remove_network`, matching the existing pattern used for the other DMaps
that embed netuid as a non-leading key (ChildkeyTake, ChildKeys,
ParentKeys, LastHotkeyEmissionOnNetuid, TotalHotkeyAlphaLastEpoch,
StakingOperationRateLimiter).

Extends `dissolve_clears_all_per_subnet_storages` to cover both maps and
adds a focused regression `dissolve_clears_auto_stake_destination_preventing_stale_routing`
that exercises the actual user-facing routing hazard.

No new dependencies, no API changes, no migration required.
@open-junius
Copy link
Copy Markdown
Contributor

format the code.

- `cargo fmt` collapses a wrapped `assert!` macro into a single line.
- Bump runtime `spec_version` 406 → 407 per the `Check spec_version bump`
  CI requirement (this PR touches `remove_network`, which is reachable
  from a runtime extrinsic).
@RUNECTZ33
Copy link
Copy Markdown
Author

Thanks @open-junius — pushed a fix-up:

  • cargo fmt: rustfmt re-collapsed a wrapped assert! in the new test (was: assert!(\n ...is_empty()\n); → now: assert!(...is_empty());).
  • spec_version: bumped 406 → 407 in runtime/src/lib.rs to satisfy Check spec_version bump.

Notes on the remaining failed checks (happy to act on any of these if you'd prefer):

  • validate-benchmarks: the job auto-generated a patch and reports "Patch ready at .bench_patch/benchmark_patch.diff — add 'apply-benchmark-patch' label to apply." — defer to maintainers on the label.
  • assert-clean-merges: the merge from testnet into this fork is clean ✅; the failure is on the main merge with conflicts in three files this PR doesn't touch (pallets/subtensor/src/subnets/subnet.rs, pallets/subtensor/src/tests/subnet.rs, runtime/src/lib.rs) — devnet-ready has diverged from main ahead of the next deploy, so this is expected to be resolved at the testnet→main merge step rather than in this PR.
  • cargo audit: all reported items are unmaintained warnings for transitive deps; no RUSTSEC vulnerabilities introduced by this change. Pre-existing on devnet-ready HEAD.
  • check testnet: TryRuntime_on_runtime_upgrade panics with "TotalIssuance diff greater than allowable delta". This PR doesn't touch issuance — remove_network only clears two StorageDoubleMap entries that map (coldkey/hotkey, netuid) → AccountId / Vec<AccountId>. Looks like a pre-existing testnet-state issue.

If any of the above are mistaken, let me know and I'll iterate.

@RUNECTZ33
Copy link
Copy Markdown
Author

Quick note in case it's helpful: both this PR's fix-up commit and #2654 are sitting in action_required on the workflow runs (2652 fix-up workflow, 2654 first run) — looks like the first-contributor CI approval gate is the hold. Happy to wait for the queue; just flagging in case a maintainer can re-approve in one click.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants