Fix alpha accounting - #3072
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🛡️ AI Review — Skeptic (security review)VERDICT: SAFE VERY HIGH scrutiny: 40-day-old account with one public repo; repository write access, no known Gittensor association, and commits from two other repository contributors. Static review found no exploitable panic, authorization bypass, unsafe arithmetic, dependency change, or AI-review trust-boundary modification. The migrations are guarded and use saturating accounting; ownership-transition work is charged by affected membership. FindingsNo findings. ConclusionNo malicious behavior or security vulnerability was identified in the PR diff. 🔍 AI Review — Auditor (domain review)VERDICT: 👎 Gittensor association UNKNOWN; newer contributor with repository write access, so the economic constants and bundled runtime changes receive maximum scrutiny. The spec version is bumped to 445, and generation-scoped cleanup now prevents recycled netuids from inheriting old The description still omits the miner-burn emission scaling and conviction lock/aggregate rewrites bundled into this branch. PRs #3071 and #3073 are the focused candidates for those changes. PR #3074 uses this same head branch and supersedes this closed PR; PR #3074 is the better candidate. Recommend keeping this PR closed. Findings
Other findings
Prior-comment reconciliation
ConclusionThe generation-safety blocker is fixed, but the reconstruction evidence remains absent and the PR description does not disclose major bundled economic and lock-model changes. Continue review on the superseding PR after those issues are resolved. 📜 Previous run (superseded)
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| fn test_rao_alpha_out_corrections_match_csv() { | ||
| use crate::migrations::migrate_fix_rao_alpha_out_accounting::ALPHA_OUT_CORRECTIONS; | ||
|
|
||
| let csv = include_str!("../../../../rao-double-dividend-subnet-alpha-out-correction.csv"); |
There was a problem hiding this comment.
[MEDIUM] Referenced reconstruction CSV is absent
This include_str! targets rao-double-dividend-subnet-alpha-out-correction.csv, but that file is neither in the PR nor the repository. Rust expands this at compile time, so the Subtensor test target cannot compile and the claimed focused test run cannot match this head. Add the reconstruction artifact at the referenced path or make the verification self-contained.
| return; | ||
| let eligible_alpha = subnet_alpha_out | ||
| .saturating_sub(SubnetProtocolAlpha::<T>::get(netuid)) | ||
| .saturating_sub(T::AlphaAssets::alpha_burned(netuid)); |
There was a problem hiding this comment.
[HIGH] Burned alpha is not scoped to the current subnet generation
AlphaBurned is keyed only by netuid and is never cleared when a subnet is dissolved or that netuid is registered again, while SubnetAlphaOut is reset for the new generation. Subtracting the cumulative value therefore lets burns from an old asset reduce—or saturate to zero—the takeover quorum of an unrelated replacement subnet. This can permit takeover below 10% of current eligible alpha or disable takeover entirely. Clear/generation-scope the alpha-assets counters during subnet teardown/registration, migrate existing recycled netuids safely, and add a dissolve/re-register regression test.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
… challengers to independently meet the 10% conviction threshold.
6b7d01e to
750af38
Compare
| let eligible_alpha = subnet_alpha_out | ||
| .saturating_sub(SubnetProtocolAlpha::<T>::get(netuid)) | ||
| .saturating_sub(T::AlphaAssets::alpha_burned(netuid)); |
There was a problem hiding this comment.
[HIGH] Burned alpha is not scoped to the current subnet generation
AlphaBurned is keyed only by netuid, and dissolution does not clear it. When a netuid is reused, this subtracts burns belonging to the dissolved subnet and lowers the new generation’s takeover quorum. Scope the counter by subnet generation or reset it atomically during dissolution before using it here.
| fn test_rao_alpha_out_corrections_match_csv() { | ||
| use crate::migrations::migrate_fix_rao_alpha_out_accounting::ALPHA_OUT_CORRECTIONS; | ||
|
|
||
| let csv = include_str!("../../../../rao-double-dividend-subnet-alpha-out-correction.csv"); |
There was a problem hiding this comment.
[MEDIUM] Referenced reconstruction CSV is absent
This include_str! target is not present in the PR, so compiling the Subtensor tests fails before this verification can run. Check in rao-double-dividend-subnet-alpha-out-correction.csv at the referenced repository-root path, including the source/provenance needed to audit the embedded migration constants.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
| /// generation was dissolved, so applying its correction to the current generation | ||
| /// would corrupt an unrelated asset. Tests verify these embedded constants against | ||
| /// the checked-in reconstruction data. |
There was a problem hiding this comment.
[MEDIUM] Referenced reconstruction data is not checked in
No reconstruction dataset is present in this PR, so reviewers cannot reproduce or independently verify the 56 hard-coded economic corrections. Check in the source data (and preferably the deterministic generation/validation script), then retain a test that compares every embedded row against that artifact.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
3 participants
Summary
Fixes two historical
SubnetAlphaOutaccounting deviations and updates the subnet ownership-transfer quorum to exclude alpha that cannot support a challenger.The migration only corrects accounting counters. It does not mint alpha, modify stake, or replay historical chain state.
Historical accounting reconstruction
Every block from the RAO activation at block
4,920,351through block8,780,303was scanned across the applicable runtime versions.Duplicated RAO dividends
Between blocks
4,920,351and4,962,968, local dividends were duplicated: participant stake received both the full local dividend and the root portion, whileSubnetAlphaOutrecorded only the intended issuance.At hotfix block
4,962,968, the per-subnet difference betweenTotalHotkeyAlphaandSubnetAlphaOutwas used to calculate the missing accounting amount. The migration increasesSubnetAlphaOutby that stabilized difference.Subnets 65 and 66 were not missed:
4,950,813and accumulated a correction of4,932.684752021alpha.4,958,013and accumulated a correction of2,003.518827049alpha.Their corrections are smaller because they were exposed to the duplication only from their registration blocks until the hotfix.
Root accounting
Legacy root dividends were credited directly to root stake without increasing
SubnetAlphaOut(0).The root discrepancy was measured immediately before Root Reborn at block
8,765,683and verified again at block8,822,961. The difference remained unchanged at728,652.620877147alpha, confirming that the deviation had stopped before applying the correction.Excluded subnet generations
Eleven netuids were excluded:
15, 16, 26, 31, 36, 38, 40, 47, 49, 57, 58Their affected historical subnet generations were dissolved and the netuids were subsequently reused. Applying the old correction would modify an unrelated current subnet asset.
Migration behavior
SubnetAlphaOutby a total of1,618,308.219994798alpha.Ownership-transfer threshold
Changes the challenger requirement from:
to:
Subtraction is saturating, and ownership cannot transfer when the resulting eligible alpha is zero.
AlphaBurnedis exposed throughAlphaAssetsInterfaceto avoid coupling Subtensor directly to alpha-assets storage.Testing