Fix conviction aggregate roll-forward- #3060 - #3073
Conversation
… challengers to independently meet the 10% conviction threshold.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| weight = weight.saturating_add(T::DbWeight::get().reads(3)); | ||
|
|
||
| // Collect before rewriting Lock so mutation cannot disturb the iterator. | ||
| let locks: Vec<_> = Lock::<T>::iter().collect(); |
There was a problem hiding this comment.
[HIGH] Runtime upgrade materializes every lock without a hard bound
on_runtime_upgrade collects the entire permissionlessly growable Lock map into WASM memory, then clears several maps and rewrites every retained row in the same upgrade block. The archive snapshot is not a protocol bound and state can grow before deployment; returning the consumed weight only after execution cannot prevent an overweight or memory-exhausting upgrade. Stage this migration with a cursor and per-block limit, or enforce and validate a hard storage bound before performing the rebuild.
| /// complete member-scaled work instead of adding permanent storage bookkeeping. | ||
| pub fn owner_transition_member_count(netuid: NetUid, new_owner_hotkey: &T::AccountId) -> u32 { | ||
| let old_owner_hotkey = SubnetOwnerHotkey::<T>::get(netuid); | ||
| let old_owner_members = LockingColdkeys::<T>::iter_prefix((netuid, &old_owner_hotkey)) |
There was a problem hiding this comment.
[HIGH] Ownership transitions scan an unbounded member index
This unbounded prefix scan is evaluated while determining dispatch weight, and the transition subsequently collects and rewrites every indexed member. LockingColdkeys has no protocol-level per-hotkey bound, so historical observations do not constrain adversarial state growth. Automatic ownership changes also reach the same work from the block hook. Dynamic weight accounting does not stop execution once the block limit is exceeded; introduce a maintained bound or a staged transition with bounded work per block.
🛡️ AI Review — Skeptic (security review)VERDICT: VULNERABLE VERY HIGH scrutiny: 61-day-old account with one public repository; author has write access, no Gittensor association found, and several commits have other authors. The migration is now scheduled, but both previously identified unbounded runtime paths remain reachable. Findings
Prior-comment reconciliation
ConclusionThe PR appears legitimate, but the runtime upgrade and ownership transitions can perform unbounded work. Both paths must be bounded or staged before merge. 📜 Previous run (superseded)
# 🔍 AI Review — Auditor (domain review) has not yet run on this PR. |
|
🔄 AI review updated — Skeptic: VULNERABLE |
# Conflicts: # docs/migration.mdx # pallets/admin-utils/src/weights.rs # pallets/subtensor/src/macros/hooks.rs # pallets/subtensor/src/migrations/mod.rs # pallets/subtensor/src/staking/lock.rs # pallets/subtensor/src/tests/locks.rs # runtime/src/lib.rs # sdk/python/bittensor/_generated/calls.py # sdk/python/bittensor/_generated/constants.py # sdk/python/bittensor/_generated/errors.py # sdk/python/bittensor/_generated/runtime_apis.py # sdk/python/bittensor/_generated/storage.py # website/apps/bittensor-website/src/app/(pages-without-footer)/releases/page.tsx
eco-tests changed — indexer review requiredThis PR modifies files under Changed files
|
| weight = weight.saturating_add(T::DbWeight::get().reads(3)); | ||
|
|
||
| // Collect before rewriting Lock so mutation cannot disturb the iterator. | ||
| let locks: Vec<_> = Lock::<T>::iter().collect(); |
There was a problem hiding this comment.
[HIGH] Runtime upgrade materializes every lock without a hard bound
This runtime upgrade collects the entire Lock map and then clears and rebuilds several maps in one block. The observed mainnet row count is not an enforced bound; unexpected state growth can make the upgrade exceed block resources and jeopardize chain liveness. Stage the migration across blocks or enforce a proven hard bound before upgrading.
| /// complete member-scaled work instead of adding permanent storage bookkeeping. | ||
| pub fn owner_transition_member_count(netuid: NetUid, new_owner_hotkey: &T::AccountId) -> u32 { | ||
| let old_owner_hotkey = SubnetOwnerHotkey::<T>::get(netuid); | ||
| let old_owner_members = LockingColdkeys::<T>::iter_prefix((netuid, &old_owner_hotkey)) |
There was a problem hiding this comment.
[HIGH] Ownership transitions scan an unbounded member index
This prefix scan has no enforced cardinality limit and runs while determining dispatch weight; the transition later scans and materializes the same unbounded index. Returned member-scaled weight accounts for work after it occurs but does not prevent oversized validation or execution. Bound membership per subnet or implement a staged transition.
|
🔄 AI review updated — Skeptic: VULNERABLE |
| let coldkeys: Vec<T::AccountId> = LockingColdkeys::<T>::iter_prefix((netuid, hotkey)) | ||
| .map(|(coldkey, ())| coldkey) | ||
| .collect(); |
There was a problem hiding this comment.
[HIGH] Ownership transitions scan an unbounded member index
Every ownership change collects and processes the entire (netuid, hotkey) reverse-index prefix, with no enforced member bound. These transitions are reachable from recurring runtime paths, including automatic takeover, lease termination, and administrative owner changes; an oversized index can therefore exhaust block resources. Reported weight after completion does not bound execution. Use a bounded collection with an invariant-enforced maximum, or stage the transition across blocks.
|
🔄 AI review updated — Skeptic: VULNERABLE |
| weight = weight.saturating_add(T::DbWeight::get().reads(3)); | ||
|
|
||
| // Collect before rewriting Lock so mutation cannot disturb the iterator. | ||
| let locks: Vec<_> = Lock::<T>::iter().collect(); |
There was a problem hiding this comment.
[HIGH] Runtime upgrade materializes every lock without a hard bound
This migration is now invoked synchronously from on_runtime_upgrade, yet it collects every Lock row and subsequently clears/rebuilds several complete maps with u32::MAX limits. The archived count is only an observation, not an enforced bound; lock state can grow before deployment. An oversized upgrade can exceed block resources and halt runtime-upgrade execution. Stage this repair across bounded pages or enforce a protocol-level maximum proven within the upgrade budget.
| let coldkeys: Vec<T::AccountId> = LockingColdkeys::<T>::iter_prefix((netuid, hotkey)) | ||
| .map(|(coldkey, ())| coldkey) | ||
| .collect(); |
There was a problem hiding this comment.
[HIGH] Ownership transitions scan an unbounded member index
Every ownership change materializes and processes the full (netuid, hotkey) reverse-index prefix. The number of coldkeys is not bounded here, while each member causes multiple storage reads and writes. A sufficiently populated hotkey can therefore make automatic, lease, or administrative ownership transition exceed block resources. Use a bounded collection with an enforced maximum or make ownership reclassification a paged state machine with transitions disabled until completion.
|
🔄 AI review updated — Skeptic: VULNERABLE |
3 participants
Summary
Fix conviction-lock aggregate accounting and rebuild aggregates corrupted by partial roll-forward semantics.
This change keeps the current
maintakeover policy unchanged: the winning hotkey must independently hold strictly more than 18% of eligible alpha (SubnetAlphaOut - SubnetProtocolAlpha - AlphaBurned), using exact U256 arithmetic.Motivation
An individual lock could be rolled forward and have only its delta applied to its aggregate before advancing the aggregate timestamp. This broke the aggregate invariant:
Ownership changes also updated aggregate buckets without consistently reclassifying canonical individual locks, allowing stale owner conviction to reappear after later member updates.
Changes
Aggregate accounting
ConvictionModelto one individual lock and its corresponding aggregate class.Ownership transitions
Takeover policy
Preserve the current
mainbehavior:Runtime migration
Add
migrate_rebuild_conviction_aggregates, guarded byHasMigrationRun.The migration:
Lockrows.LockingColdkeys.Test coverage
Added regression coverage for:
Validation
cargo fmt --check --allgit diff --checkFull builds and tests are left to CI per repository guidance.
Runtime version
The rebase preserves
main’s currentspec_versionof 451. A subsequent version bump orno-spec-version-bumplabel may be required before merge.