Conviction updates#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.
There was a problem hiding this comment.
This would be valid if we were preserving existing conviction state. We are intentionally not preserving it on testnet. The final migration clears all current conviction lock maps, including OwnerLock, DecayingOwnerLock, HotkeyLock, DecayingHotkeyLock, individual Lock, and DecayingLock, before the new interpretation can operate on persisted values. Since mainnet has not had this state live, no rebuild migration is needed.
🛡️ AI Review — Skeptic (security review)VERDICT: SAFE BASELINE scrutiny: established write-permission opentensor contributor; no Gittensor allowlist hit found; 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 remains addressed by the migration that clears old conviction lock maps before the revised owner-hotkey aggregate semantics are used. 🔍 AI Review — Auditor (domain review)VERDICT: 👎 Gittensor: LIKELY; PR body is substantive and matches the implementation. Duplicate-work check: overlapping open PRs share common runtime/pallet files or deployment surface, but I did not find a better duplicate candidate for this conviction owner-hotkey update. I did not run tests because both blockers are static bounded-execution issues carried forward from the prior Auditor pass. Findings
Prior-comment reconciliation
ConclusionBlocking on the same bounded-execution issues from the prior Auditor pass. The conviction behavior has focused tests, but runtime upgrade and hotkey-swap paths must be bounded before merge. 📜 Previous run (superseded)
|
|
🔄 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: 👎 |
…into feat/conviction-v2
… auto-lock, 1/2 year unlock rate, fix for owner immediate conviction
gztensor
changed the title
|
|
||
| // 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
This migration calls clear(u32::MAX, None) over Lock and then repeats the same pattern for every aggregate lock map. These maps are user-sized, so a populated testnet/devnet can force the runtime upgrade to delete an unbounded number of keys in one on_runtime_upgrade. Returning the measured weight after the work does not make the upgrade bounded; the block still has to execute all deletions before it can finish. Please convert this to a bounded migration with a cursor and per-block limit, or gate it behind a provable small upper bound for the target network state.
| // 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() { | ||
| for ((coldkey, netuid, hotkey), lock) in Lock::<T>::iter() { |
There was a problem hiding this comment.
[HIGH] Owner hotkey swaps can scan the entire lock table
When any relevant netuid is found, swap_hotkey_locks iterates Lock::<T>::iter() and filters by hotkey == old_hotkey. Lock is keyed by (coldkey, netuid, hotkey), so this is a full-table scan over all coldkey lock entries, not just the old hotkey's locks. This runs inside user-callable hotkey swap flow and is not reflected by the fixed extrinsic weight, so a large lock table can make hotkey swaps exceed block limits. The lock index needs to support bounded prefix iteration for the old hotkey, or this path needs a bounded/chunked design.
There was a problem hiding this comment.
Lock cardinality is bounded by the existing stake-position cardinality, and hotkey swap already contains unbounded/prefix iteration over alpha stake positions, so this does not introduce a fundamentally new unbounded storage class.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
gztensor
added
the
skip-cargo-audit
|
|
||
| // 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
This migration calls clear(u32::MAX, None) on Lock and then repeats the same unbounded clear for five more conviction maps. These maps are user-sized, so a runtime upgrade can spend an unbounded amount of work in one on_runtime_upgrade pass before the chain can produce the next block. The returned weight accounts for work after it has already happened; it does not bound execution. Make this migration bounded, e.g. with stored cursors and limited per-block cleanup, or prove from live state that these prefixes are bounded to a safe maximum before shipping the upgrade.
| // 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() { | ||
| for ((coldkey, netuid, hotkey), lock) in Lock::<T>::iter() { |
There was a problem hiding this comment.
[HIGH] Owner hotkey swaps can scan the entire lock table
When netuids_to_transfer is non-empty, swap_hotkey_locks falls back to Lock::<T>::iter() and scans every lock across all coldkeys and subnets to find entries for old_hotkey. This is reachable from the user-facing all-subnet hotkey swap path, so a single extrinsic can do work proportional to global lock table size. The post-dispatch weight returned by the helper does not prevent the unbounded scan from executing. Keep an index keyed by hotkey, restrict the operation to bounded prefixes, or split the movement into a bounded multi-step process.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
|
|
||
| // 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
This runtime-upgrade migration clears every entry in Lock, HotkeyLock, DecayingHotkeyLock, OwnerLock, DecayingOwnerLock, and DecayingLock with clear(u32::MAX, None) in a single on_runtime_upgrade pass. Returning the measured weight after deletion does not bound execution; if testnet has user-sized lock state, the upgrade block still has to perform all deletions before the weight is known and can exceed block limits. Make this bounded, e.g. with cursor-based cleanup across blocks or a hard pre-upgrade guarantee that these maps are empty before this migration runs.
| // 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() { | ||
| for ((coldkey, netuid, hotkey), lock) in Lock::<T>::iter() { |
There was a problem hiding this comment.
[HIGH] Owner hotkey swaps can scan the entire lock table
swap_hotkey_locks falls back to Lock::<T>::iter() whenever any affected netuid exists, then filters by hotkey in memory. This is reachable from the hotkey-swap path and is bounded by total lock-table size, not by the caller's hotkey or subnet count. The dynamic post-dispatch weight accounting does not prevent the block from doing the full scan first. Use a storage index keyed by hotkey, migrate the data shape, or otherwise bound this path before allowing owner/hotkey lock movement.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
2 participants
Description
Functional changes
get_coldkey_lockRefactoring
The conviction maturing and unlock logic as well as aggregate tracking is incapsulated in
struct ConvictionModel.Type of Change
Checklist
./scripts/fix_rust.shto ensure my code is formatted and linted correctly