fix: Prevent and clean up zero-valued entries in Alpha storage maps during hotkey and coldkey swaps by bittoby · Pull Request #2513 · opentensor/subtensor

bittoby · 2026-03-18T20:03:09Z

Description

Prevent and clean up zero-valued entries in Alpha storage maps during hotkey and coldkey swaps.

Coldkey and hotkey swap logic was inserting zero-valued entries into Alpha, TotalHotkeyShares, TotalHotkeyAlphaLastEpoch, and AlphaDividendsPerSubnet without checking if the value was zero first. These entries persist indefinitely and bloat storage - a single coldkey swap could produce ~18k zero entries, and roughly 52% of Alpha entries on-chain are zero-valued.

This PR adds zero-guards to all insertion points in swap_coldkey.rs and swap_hotkey.rs, and includes a one-time migration to purge existing zero entries from the four affected storage maps.

Related Issue(s)

Closes Alpha and hotkey storage maps accumulate persistent zero-valued entries #2398

Type of Change

Bug fix (non-breaking change which fixes an issue)

Breaking Change

No breaking 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
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

Additional Notes

The migration collects the full Alpha map into memory before removing zero entries. This is intentional - mutating a Substrate storage map while iterating over it is unsafe. The other three maps are smaller and iterated directly. All four maps use HasMigrationRun to ensure idempotency.

bittoby · 2026-03-18T21:30:46Z

@open-junius Could you please review this PR? Welcome to any feedbacks.
thank you

open-junius · 2026-03-19T02:38:40Z

change the target branch as devnet-ready, not main.

bittoby · 2026-03-19T04:43:43Z

I changed target branch. Thanks! Ready to review

pallets/subtensor/src/swap/swap_hotkey.rs

open-junius · 2026-03-19T12:37:34Z

please check https://github.com/opentensor/subtensor/pull/2509/changes, which also prevent some zero value insert. Could you include these storage into the PR.

bittoby · 2026-03-19T13:18:22Z

@open-junius Thanks for your feedback! I updated all following your feedback. Could you please review again?

open-junius · 2026-03-20T08:42:54Z

please merge the latest devnet-ready, will fix the e2e test.

bittoby · 2026-03-20T11:59:32Z

@open-junius I merged the latest devnet-ready. Could you take a look once more?
thank you

bittoby · 2026-03-20T17:47:18Z

@open-junius All e2e tests passed. thank you. The benchmark testing issue is not relate to my change.

bittoby · 2026-03-23T11:49:51Z

@open-junius I hope you had a great weekend. Any updates for me, please.

bittoby · 2026-03-24T15:39:16Z

@evgeny-s @open-junius

Converting to SteppedMigration would require adding pallet-migrations to the runtime since MultiBlockMigrator is currently () That's a bigger scope change.

I think the cleanest approach is to drop the migration from this PR and just ship the zero-value insert guards (which prevent new zeros from being created). The purge of existing zeros can be handled properly in a separate PR with the right multi-block infrastructure. I’m planning to take care of that follow-up PR too after this one.

Let me know what you'd prefer.

bittoby · 2026-03-24T20:14:40Z

@evgeny-s @open-junius did you check my description? Should I have to update my PR?

evgeny-s · 2026-03-24T21:04:33Z

pallets/subtensor/src/staking/claim_root.rs


-            // Set the new root claimed value.
-            RootClaimed::<T>::insert((netuid, hotkey, coldkey), new_root_claimed);
+            // Set the new root claimed value, removing if zero to avoid storage bloat.


The comment is saying "removing", but I don't see it here.

But below we remove it. Fix it for consistency

evgeny-s · 2026-03-24T21:14:33Z

@evgeny-s @open-junius did you check my description? Should I have to update my PR?

I would rather keep it with the migration. Please test it first on the mainnet clone, and use multi-block migration if needed. Let's see other opinions on this regard.
Also, it would be great to add several TS tests for this PR.

bittoby · 2026-03-24T21:19:46Z

@evgeny-s @open-junius did you check my description? Should I have to update my PR?

I would rather keep it with the migration. Please test it first on the mainnet clone, and use multi-block migration if needed. Let's see other opinions on this regard. Also, it would be great to add several TS tests for this PR.

I absolute agree with you. If possible, I want to keep migration in this PR, too. Now I am testing locally. After finishing, I will share result here. Thanks @evgeny-s

bittoby · 2026-03-25T11:16:09Z

@evgeny-s

Updated:

Converted migration to multi-block on_idle approach - on_runtime_upgrade is O(1), cleanup runs one map per block across 4 phases
Fixed claim_root.rs - added missing else { remove } branch for consistency
Tested against Finney mainnet clone via build-patched-spec - node runs and produces blocks normally with the migration code
Added TS e2e test for zero-alpha prevention
Reverted unrelated check-node-compat.yml change

open-junius · 2026-03-25T12:27:31Z

ts-tests/suites/zombienet_staking/12-zero-alpha-prevention.test.ts

+                const stakeAfterRemove = await getStake(api, hotkey.address, coldkey.address, netuid);
+
+                // Stake should be zero or very close to zero (owner minimum may be retained)
+                log(`Stake after full remove: ${stakeAfterRemove}`);


If we can't check in the code. maybe we don't need this test case. since 0 is default value, we can't tell the difference. It seems we need lower level api to check if the key exists or not.

In my opinion, it should be enough you already verify them in rust unit test. no need to add it in e2e.

…block on_idle migration to purge existing ones

bittoby · 2026-03-25T12:37:07Z

@open-junius Thanks for your feedback. I removed e2e test. Please review again.

bittoby · 2026-03-25T12:42:51Z

@open-junius Could you tell me why assert-clean-merges testing failed?

open-junius · 2026-03-25T13:07:11Z

@open-junius Could you tell me why assert-clean-merges testing failed?

You can ignore it. we need merge the main back to devnet-ready sometimes if there is any hotfix in main branch.

…into fix/remove-zero-alpha-entries

bittoby · 2026-03-25T18:28:18Z

@evgeny-s @open-junius could you please review again once more? I think this almost done

bittoby · 2026-03-26T11:26:30Z

These testing failure doesn't seem to relate to my change. Please check the log.
@open-junius

open-junius · 2026-03-26T13:41:12Z

These testing failure doesn't seem to relate to my change. Please check the log. @open-junius
I see, it is common issue in devnet-ready branch. I noticed you split the data cleanup into phases.
Could you follow the method recommended by @evgeny-s to get the amount and time needed to run migration?

…timeouts

bittoby · 2026-03-26T17:18:07Z

I have updated the migration script more correctly:
It now uses remaining_weight to dynamically limit how many entries are processed per block instead of a fixed batch size. This helps prevent block timeouts on large maps.

bittoby · 2026-03-26T17:18:38Z

Tested on Finney mainnet clone via build-patched-spec:

Alpha zero entries: ~580,000
Time: ~14 min (70 blocks) to remove 99% of them

No OOM or block timeout issues.

bittoby · 2026-03-26T17:19:40Z

@open-junius @evgeny-s I'd like you review once more ^^. It works well for me

open-junius · 2026-03-27T09:37:47Z

I have updated the migration script more correctly: It now uses remaining_weight to dynamically limit how many entries are processed per block instead of a fixed batch size. This helps prevent block timeouts on large maps.

It is great, let me review it again.

open-junius · 2026-03-27T09:38:41Z

Tested on Finney mainnet clone via build-patched-spec:

Alpha zero entries: ~580,000

Time: ~14 min (70 blocks) to remove 99% of them

No OOM or block timeout issues.

Thanks for collect the data, it is vaulable.

open-junius · 2026-03-27T12:52:31Z

pallets/subtensor/src/migrations/migrate_remove_zero_alpha.rs

+
+/// Called from `on_runtime_upgrade`. Schedules the cleanup by setting phase = 1
+/// if the migration hasn't run yet. This is O(1) — no iteration.
+pub fn migrate_remove_zero_alpha<T: Config>() -> Weight {


After the actual cleanup executed in the on_idle, I am wondering if we really need the migration part.
For instance, we can set the ZeroAlphaCleanupPhase init value as 1 in storage.
then on_idle start to handle it until the value go to 0.
Or use different values for different steps, make it clearer.
What's your opinion?

Good point. We could simplify by setting the default to 1, but the issue is: after cleanup completes and sets phase=0, a node restart or fresh sync would read the default (1) again and re-trigger cleanup. The current approach uses HasMigrationRun as a permanent guard to prevent this.

Good point. We could simplify by setting the default to 1, but the issue is: after cleanup completes and sets phase=0, a node restart or fresh sync would read the default (1) again and re-trigger cleanup. The current approach uses HasMigrationRun as a permanent guard to prevent this.

@open-junius what do you think about it? Still should i need to update?

bittoby changed the base branch from main to devnet-ready March 19, 2026 02:47

bittoby changed the base branch from devnet-ready to main March 19, 2026 03:17

bittoby changed the base branch from main to devnet-ready March 19, 2026 04:36

bittoby closed this Mar 19, 2026

bittoby force-pushed the fix/remove-zero-alpha-entries branch from 8cd929a to 369a319 Compare March 19, 2026 04:37

bittoby reopened this Mar 19, 2026