Skip to content

Cleanup staking hotkeys on stake removal - #3095

Open
UnArbosFour wants to merge 3 commits into
release-448from
fix/cleanup-staking-hotkeys
Open

Cleanup staking hotkeys on stake removal#3095
UnArbosFour wants to merge 3 commits into
release-448from
fix/cleanup-staking-hotkeys

Conversation

@UnArbosFour

Copy link
Copy Markdown
Contributor

Summary

Clean up stale StakingHotkeys associations when a hotkey/coldkey pair no longer has stake.

Changes

  • Added maybe_remove_staking_hotkey.
  • Invoked cleanup after:
    • Fully unstaking from a subnet.
    • Transferring stake within a subnet.
  • Retain associations when:
    • The pair still has stake on another subnet.
    • A nonzero basket watermark must remain discoverable for claims or coldkey swaps.
  • Remove the storage map entry when its hotkey vector becomes empty.
  • Added unit and integration coverage for unstaking, transfers, multi-subnet stake, and basket claimants.

Testing

cargo test --package pallet-subtensor --lib

1438 passed
9 ignored
0 failed

cargo fmt --all -- --check and git diff --check also pass.

@vercel

vercel Bot commented Aug 19, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
subtensor Ready Ready Preview Aug 19, 2026 8:49pm

Request Review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.

Comment thread .github/workflows/cargo-audit.yml
Comment thread pallets/subtensor/src/staking/stake_utils.rs
@github-actions

github-actions Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

🛡️ AI Review — Skeptic (security review)

VERDICT: VULNERABLE

VERY HIGH scrutiny: 48-day-old account with one public repository and write access; fix/cleanup-staking-hotkeys -> release-448; no Gittensor association established.

The cleanup remains underweighted in both its idle migration and normal staking paths. In particular, the migration converts an entire unbounded row into persistent cursor state despite accounting for only fixed database operations.

Findings

Sev File Finding
HIGH pallets/subtensor/src/migrations/migrate_cleanup_staking_hotkeys.rs:142 Bound migration state before copying an entire staking-hotkey row inline
MEDIUM pallets/subtensor/src/staking/stake_utils.rs:889 Regenerate weights for staking-hotkey cleanup inline

Prior-comment reconciliation

  • 6a25f210: no longer applies — The current diff against release-448 does not modify cargo-audit.yml or introduce the advisory suppression, so it is not a PR-specific finding in this run.
  • ea8efc7a: not addressed — The migration still copies the complete unbounded hotkey vector into remaining_hotkeys while charging fixed read/write weights.
  • a3e23c96: not addressed — Benchmarks now exercise cleanup, but the generated weight file remains unchanged.

Conclusion

The change appears legitimate, but unbounded migration-state construction and stale generated weights create runtime resource-exhaustion risk. These issues should be fixed before merge.


📜 Previous run (superseded)
Sev File Finding Status
HIGH .github/workflows/cargo-audit.yml:105 Do not suppress a remotely triggerable h2 DoS advisory ⏭️ No longer applies
The current diff against release-448 does not modify cargo-audit.yml or introduce the advisory suppression, so it is not a PR-specific finding in this run.
HIGH pallets/subtensor/src/migrations/migrate_cleanup_staking_hotkeys.rs:199 Bound migration state before copying an entire staking-hotkey row ➡️ Carried forward to current findings
The migration still copies the complete unbounded hotkey vector into remaining_hotkeys while charging fixed read/write weights.
MEDIUM pallets/subtensor/src/staking/stake_utils.rs:648 Rebenchmark stake calls after adding storage cleanup ➡️ Carried forward to current findings
Benchmarks now exercise cleanup, but the generated weight file remains unchanged.

# 🔍 AI Review — Auditor (domain review) has not yet run on this PR.

@github-actions

Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: VULNERABLE

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.

Comment thread .github/workflows/cargo-audit.yml
Comment thread pallets/subtensor/src/staking/stake_utils.rs
Comment thread pallets/subtensor/src/migrations/migrate_cleanup_staking_hotkeys.rs
@github-actions

Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: VULNERABLE

@UnArbosFour
UnArbosFour changed the base branch from main to release-448 August 19, 2026 19:23

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.

Comment on lines +141 to +142
progress.vector_writes = progress.vector_writes.saturating_add(1);
continue;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] Bound migration state before copying an entire staking-hotkey row

hotkeys is an unbounded storage vector, yet the migration decodes and copies the entire row into remaining_hotkeys in one idle pass. row_load_weight() charges only one fixed database read, and cursor persistence is also charged as a fixed write, so a sufficiently large row can consume unbounded CPU, memory, proof size, and encoded-state work beyond the supplied idle-weight limit. Process the row through a bounded cursor/page or impose and account for a strict maximum row size.

@@ -868,13 +889,7 @@ impl<T: Config> Pallet<T> {
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] Regenerate weights for staking-hotkey cleanup

This newly reachable cleanup adds prefix iteration, a basket read, and potentially a linear vector read/retain/write to stake-removal calls. The benchmarks were adjusted, but this PR does not update the generated weights.rs, leaving deployed calls charged according to the old storage behavior. Regenerate and commit the affected weights, ensuring the benchmark covers the maximum supported StakingHotkeys row size.

@github-actions

Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: VULNERABLE

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.

1 participant