Everyone's convinction to subnet owners is immediate#2687
Conversation
…ediate conviction
gztensor
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| .map(|lock| Self::roll_forward_owner_lock(netuid, lock, now).conviction) | ||
| .unwrap_or_else(|| U64F64::saturating_from_num(0)), | ||
| ) | ||
| let owner_conviction = OwnerLock::<T>::get(netuid) |
There was a problem hiding this comment.
[HIGH] OwnerLock storage is reinterpreted without migrating existing aggregates
OwnerLock previously represented aggregate locks made by the subnet owner coldkey, regardless of which hotkey was locked to and whether that coldkey was in decaying mode. This PR now credits any existing OwnerLock directly to SubnetOwnerHotkey and rolls it as (owner_lock=true, perpetual_lock=true), while the new DecayingOwnerLock starts empty. Without an on-chain migration that rebuilds OwnerLock, DecayingOwnerLock, HotkeyLock, and DecayingHotkeyLock from individual Lock entries under the new (hotkey == SubnetOwnerHotkey, is_perpetual) rules, legacy owner-coldkey locks to non-owner hotkeys or decaying owner locks are misattributed to the current owner hotkey. That corrupts hotkey_conviction, get_total_conviction, and subnet_king, and can incorrectly preserve or change subnet ownership. Add a storage-versioned migration or keep backward-compatible interpretation until migration completes.
🛡️ AI Review — Skeptic (security review)VERDICT: SAFE BASELINE scrutiny: established write-permission opentensor contributor; no Gittensor allowlist hit; branch feat/conviction-v2 -> devnet-ready is valid. No FindingsNo findings. Prior-comment reconciliation
ConclusionNo malicious code or security vulnerability found in the current diff. The prior OwnerLock transition concern is addressed by clearing the old conviction lock maps before the new aggregate layout is used. 📜 Previous run (superseded)
🔍 AI Review — Auditor (domain review)VERDICT: 👎 Gittensor: LIKELY; established subtensor contributor with write permission and substantial recent merged PR history. PR body was empty/trivial; the Auditor has auto-filled it. Please review. Auto-fix applied: bumped Duplicate-work check: overlapping open PRs appear to share common hot files or deployment/runtime surface area; I did not find a better duplicate candidate for this specific conviction owner-hotkey change. Findings
ConclusionBlocking on bounded-execution issues in runtime upgrade / hotkey-swap paths. The conviction logic is covered with focused tests, but the unbounded storage work must be made safe before merge. |
|
🔄 AI review updated — Skeptic: VULNERABLE |
| .map(|lock| Self::roll_forward_owner_lock(netuid, lock, now).conviction) | ||
| .unwrap_or_else(|| U64F64::saturating_from_num(0)), | ||
| ) | ||
| let owner_conviction = OwnerLock::<T>::get(netuid) |
There was a problem hiding this comment.
[HIGH] OwnerLock storage is reinterpreted without migrating existing aggregates
OwnerLock already exists on-chain with the old meaning: aggregate locks made by the subnet owner coldkey. This PR changes the runtime to treat the same storage item as aggregate locks to the subnet owner hotkey, and this line credits that legacy value to SubnetOwnerHotkey. Existing OwnerLock entries can therefore be assigned to the wrong hotkey, and prior decaying owner-coldkey aggregates can also be rolled as perpetual owner-hotkey conviction. That can corrupt hotkey_conviction, subnet_king, and subsequent owner reassignment after the runtime upgrade.
This needs a storage migration that rebuilds owner-hotkey aggregate buckets from the individual Lock records, splitting perpetual vs decaying entries into OwnerLock and DecayingOwnerLock, or otherwise clears/recomputes the legacy OwnerLock before these new reads are live.
|
🔄 AI review updated — Skeptic: VULNERABLE |
|
|
||
| // This only affects testnet: mainnet has not had this conviction lock state | ||
| // deployed with live values yet. | ||
| let lock_removal = Lock::<T>::clear(u32::MAX, None); |
There was a problem hiding this comment.
[HIGH] Migration clears user-sized lock storage in one upgrade block
Lock, HotkeyLock, DecayingHotkeyLock, OwnerLock, DecayingOwnerLock, and DecayingLock are cleared with u32::MAX in a single on_runtime_upgrade migration. These maps can scale with user lock activity, and computing the weight after clear() does not bound the work already performed. If testnet/devnet has more entries than expected, the upgrade can exceed block limits or stall. Make this bounded with finite clear limits plus a cursor/progress flag, or otherwise gate/prove the exact live state size before running it.
|
|
||
| // Gather locks for old hotkey (only if hotkey locks exist, otherwise skip to save reads) | ||
| if !hotkey_locks_to_transfer.is_empty() || !decaying_hotkey_locks_to_transfer.is_empty() { | ||
| if !netuids_to_transfer.is_empty() { |
There was a problem hiding this comment.
[HIGH] Owner hotkey swaps can scan the entire lock table
With old_is_owner_hotkey || new_is_owner_hotkey feeding netuids_to_transfer, any swap involving a subnet owner hotkey enters Lock::<T>::iter() and scans every lock across all coldkeys/subnets. This is reachable from signed swap_hotkey / swap_hotkey_v2, while the call has a fixed pre-dispatch weight. Returning dynamic post-dispatch weight after the scan does not bound block execution. Use a bounded cursor, maintain a reverse index by hotkey, or disallow this path from user extrinsics and handle owner-hotkey aggregate repair through a bounded migration.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
1 participant
Motivation
The conviction lock model should award immediate owner conviction based on the subnet owner hotkey, not only based on the subnet owner coldkey. This allows any coldkey locking stake to the subnet owner hotkey to contribute immediately to owner conviction.
What Changed
DecayingOwnerLockstorage for decaying locks that target the subnet owner hotkey.SubnetOwnerHotkeywhile perpetual/decaying classification remains based on the coldkey'sDecayingLockflag.Behavioral Impact
Locks targeting the subnet owner hotkey now receive immediate owner conviction regardless of which coldkey created the lock. Locks by the subnet owner coldkey to non-owner hotkeys mature normally instead of being treated as owner locks.
Migration / Spec Version
This PR changes runtime behavior and storage layout. The Auditor auto-fix bumped
spec_versionfrom408to409.The included migration clears existing conviction lock storage intended to be testnet-only legacy state before the updated design takes effect.
Testing
The PR adds focused unit tests in
pallets/subtensor/src/tests/locks.rsandpallets/subtensor/src/tests/migration.rs. The author checklist states./scripts/fix_rust.shand relevant tests were run locally.