Fix conviction aggregate roll-forward - #3060
Conversation
… challengers to independently meet the 10% conviction threshold.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🛡️ AI Review — Skeptic (security review)VERDICT: SAFE VERY HIGH scrutiny (young account, one public repository), tempered by repository write access; two scoped UnArbosFive follow-up commits; no Gittensor association found; fix/zero-lock-aggregates → main. The AI-review trust boundary is unchanged, and the runtime-affecting changes include the required FindingsNo findings. ConclusionThe migration reconstructs aggregates from canonical lock rows with saturating arithmetic, while the detach–roll–recombine flow keeps individual and aggregate state synchronized. No malicious behavior or security vulnerability was found. 🔍 AI Review — Auditor (domain review)VERDICT: 👎 LIKELY Gittensor-associated contributor; young public account with repository write access and prior merged subtensor contributions, while runtime changes still receive full scrutiny. The lock-accounting changes and regression tests appear coherent, and Description discrepancyThe PR body still omits the one-shot runtime migration that scans and rewrites every individual lock row, rebuilds five indexes/aggregate maps, and removes dust. This deployment impact should be documented. Static analysis was sufficient; no runtime checks were needed. Findings
Prior-comment reconciliation
ConclusionThe functional fix appears sound, but the migration remains structurally unbounded and lacks try-runtime invariant validation. Both migration-safety blockers must be resolved before merge. 📜 Previous run (superseded)
|
|
🔄 AI review updated — Skeptic: VULNERABLE |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
🔄 AI review updated — Skeptic: VULNERABLE |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
…ubtensor into fix/zero-lock-aggregates
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
| /// if it has become dust), then merged into its appropriate new aggregate. | ||
| /// This preserves earned conviction while establishing one common timestamp | ||
| /// for every individual and aggregate contribution. | ||
| pub fn migrate_rebuild_conviction_aggregates<T: Config>() -> Weight { |
There was a problem hiding this comment.
[HIGH] Add try-runtime migration invariants
This state-rewriting migration still has no pre_upgrade/post_upgrade checks. Add try-runtime validation that records canonical lock totals/counts before execution and verifies afterward that retained individual locks and all four rebuilt aggregate maps agree by lock class, dust rows are absent, and the reverse index exactly matches retained locks. Unit tests of selected fixtures do not validate the migration against arbitrary live state.
| 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] Bound the runtime-upgrade migration
Lock::iter().collect() remains unbounded, followed by five clear(u32::MAX, None) operations and rewrites proportional to all retained locks. The observed mainnet counts document current size but do not enforce an execution bound or protect against state growth before deployment. Use a staged/cursor-based migration with bounded work per block, or enforce a conservative hard bound before allocating and prove the complete worst-case weight fits the runtime-upgrade budget.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
2 participants
Summary
Fixes conviction-lock aggregate accounting and tightens subnet takeover eligibility.
Changes
Testing