Subnet Protocol Alpha Accounting#2645
Conversation
7a2985d to
de0e9f2
Compare
🛡️ AI Review — Skeptic (security review)VERDICT: SAFE Baseline scrutiny: established Opentensor contributor with repo write permission; branch chain-buy-cache -> devnet-ready; no trusted Gittensor allowlist hit. Static review only. The PR does not modify The known dissolve weight-accounting mismatch remains an Auditor-tracked domain finding, not a Skeptic security finding here because the affected calls are root-only. FindingsNo findings. ConclusionNo malicious behavior or security vulnerability was found in the reviewed diff. 🔍 AI Review — Auditor (domain review)VERDICT: 👍 Established opentensor contributor with repo write permission and long subtensor history; Gittensor association UNKNOWN from trusted allowlists. PR body is substantive and matches the implementation. I did not find a duplicate-work concern among the overlapping open PRs; the overlaps appear to be adjacent subtensor/runtime churn rather than competing implementations.
Findings
Prior-comment reconciliation
ConclusionThe protocol-alpha accounting change is consistent with the PR description and has focused tests for coinbase caching and deregistration settlement. The carried-forward weight-accounting issue remains medium severity because the affected dissolve calls are root-only, so I am not blocking merge on it. 📜 Previous run (superseded)
|
| let protocol_alpha_value_u128: u128 = SubnetAlphaIn::<T>::get(netuid) | ||
| .saturating_add(SubnetProtocolAlpha::<T>::get(netuid)) | ||
| .to_u64() as u128; |
There was a problem hiding this comment.
[MEDIUM] Update dissolve weights for protocol-alpha storage
This adds a new SubnetProtocolAlpha read to the dissolve path, and the same function now also removes that map later in the cleanup. The dispatch weights for both dissolve_network and root_dissolve_network in pallets/subtensor/src/macros/dispatches.rs still charge the old reads(6) / writes(31), so the runtime undercharges this path by at least the new protocol-alpha read/write. Please update those annotations or regenerate the relevant weight so the storage proof and DB ops match the new dissolve accounting.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
| let protocol_alpha_value_u128: u128 = SubnetAlphaIn::<T>::get(netuid) | ||
| .saturating_add(SubnetProtocolAlpha::<T>::get(netuid)) |
There was a problem hiding this comment.
[MEDIUM] Update dissolve weights for protocol-alpha storage
This adds a SubnetProtocolAlpha read to the dissolve cleanup path, and the same function also removes SubnetProtocolAlpha later. The static weights for dissolve_network and root_dissolve_network in pallets/subtensor/src/macros/dispatches.rs still use reads(6) / writes(31) and do not account for this new storage item. Update those weights at minimum by +1 read/+1 write, or regenerate/replace the hard-coded dissolve weights with a benchmark that exercises SubnetProtocolAlpha.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
| let protocol_alpha_value_u128: u128 = SubnetAlphaIn::<T>::get(netuid) | ||
| .saturating_add(SubnetProtocolAlpha::<T>::get(netuid)) |
There was a problem hiding this comment.
[MEDIUM] Update dissolve weights for protocol-alpha storage
destroy_alpha_in_out_stakes now reads SubnetProtocolAlpha here and later removes it, but the dissolve_network / root_dissolve_network dispatch weights are still hard-coded at reads(6) / writes(31). Please add the new read/write to those weights, or re-benchmark and wire the dissolve weight through generated weights, so the root dissolve path charges for the extra storage access.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
| let protocol_alpha_value_u128: u128 = SubnetAlphaIn::<T>::get(netuid) | ||
| .saturating_add(SubnetProtocolAlpha::<T>::get(netuid)) |
There was a problem hiding this comment.
[MEDIUM] Update dissolve weights for protocol-alpha storage
do_dissolve_network now reads SubnetProtocolAlpha and later removes it, but the hard-coded weights on both dissolve_network and root_dissolve_network still charge only reads(6) / writes(31). Please account for the extra storage read and write on those dispatch weights. This is root-only, so I am keeping it medium severity rather than blocking as a user-callable weight undercharge.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
| let mut stakers: Vec<(T::AccountId, T::AccountId, u128)> = Vec::new(); | ||
| let mut total_alpha_value_u128: u128 = 0; | ||
| let protocol_alpha_value_u128: u128 = SubnetAlphaIn::<T>::get(netuid) | ||
| .saturating_add(SubnetProtocolAlpha::<T>::get(netuid)) |
There was a problem hiding this comment.
[MEDIUM] Update dissolve weights for protocol-alpha storage
This adds protocol-alpha storage access during subnet dissolve, but the call weights for dissolve_network / root_dissolve_network in pallets/subtensor/src/macros/dispatches.rs are still hard-coded at reads(6) / writes(31). The new flow reads SubnetAlphaIn and SubnetProtocolAlpha for the pro-rata denominator and removes SubnetProtocolAlpha during cleanup, so the hard-coded weights under-account the storage work. Update both dissolve call weights to include the added reads/writes, or wire them through a benchmarked WeightInfo entry instead of leaving the stale constants.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
Description
This PR adds protocol-owned alpha accounting for subnet chain buys and includes that alpha in subnet deregistration settlement.