Restrict sudo_set_subnet_emission_enabled to root origin#2691
Conversation
Switch the dispatch from ensure_sn_owner_or_root_with_limits to ensure_root so only the Triumvirate (root) can toggle pool-side emission for a subnet. Drop the owner rate-limit recording since no subnet owner can take this path anymore. Co-authored-by: Cursor <cursoragent@cursor.com>
🛡️ AI Review — Skeptic (security review)VERDICT: SAFE BASELINE scrutiny: established OpenTensor contributor with write permission; no Gittensor allowlist hit found; Cursor co-author disclosed; branch unconst/root-only-subnet-emission-toggle -> devnet-ready. Static review only. The current diff tightens FindingsNo findings. ConclusionNo malicious intent or security vulnerability found. The changed dispatch path narrows privilege and keeps the existing admin-window, subnet-existence, root-subnet, storage-write, event, and logging behavior on the root path. 🔍 AI Review — Auditor (domain review)VERDICT: 👍 Gittensor: LIKELY; established OpenTensor contributor with write permission, reviewed primarily for domain correctness. PR body is substantive and matches the implementation. Duplicate-work check: PR #2694 appears to target the same root-only subnet-emission toggle by title and overlap. This PR is the better candidate because it includes the authorization change plus a dispatch-level regression test for owner denial and root success. Recommend closing #2694; the other overlapping PRs appear to share broad files only, not scope. I attempted the targeted test with Findings
Other findings
Prior-comment reconciliation
ConclusionThe prior blocking coverage gap is addressed by the new root-only regression test, and the authorization change is narrow. One low-severity proxy cleanup remains, but it does not block merge. 📜 Previous run (superseded)
|
| &[Hyperparameter::SubnetEmissionEnabled.into()], | ||
| )?; | ||
| ensure_root(origin)?; | ||
| pallet_subtensor::Pallet::<T>::ensure_admin_window_open(netuid)?; |
There was a problem hiding this comment.
[MEDIUM] Add a regression test for the new root-only origin
This line is the entire behavior change, but the PR only cites a storage-level coinbase test that would still pass if this dispatch remained subnet-owner-or-root. Add an admin-utils dispatch test that sets SubnetOwner, verifies a signed subnet owner now receives DispatchError::BadOrigin and leaves SubnetEmissionEnabled unchanged, then verifies RuntimeOrigin::root() can still toggle the flag.
A focused test in pallets/admin-utils/src/tests/mod.rs should cover the changed authorization boundary directly.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
Cover the changed authorization boundary directly: a signed subnet owner must receive DispatchError::BadOrigin and leave SubnetEmissionEnabled unchanged, and RuntimeOrigin::root() must still be able to toggle the flag. The admin freeze window is disabled in the test so neither path is blocked by it. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the [MEDIUM] reviewer feedback: Added
This test would fail under the previous Run locally: Result: |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
Summary
sudo_set_subnet_emission_enableddispatch fromensure_sn_owner_or_root_with_limitstoensure_root, so only the Triumvirate (root) can toggle pool-side emission for a subnet.record_owner_rl(...)call — no subnet owner can take this path anymore, so there is no owner rate-limit window to record.Behavior preserved on the root path:
ensure_admin_window_open,if_subnet_exist, theNotPermittedOnRootSubnetcheck, the storage write, theSubnetEmissionEnabledSetevent, and the debug log are all unchanged.Test plan
cargo check -p pallet-admin-utils --tests— cleancargo check -p pallet-subtensor --tests— cleancargo check -p node-subtensor-runtime— cleancargo test -p pallet-subtensor --lib tests::coinbase::test_sudo_set_subnet_emission_enabled_multiple_subnets_multiple_toggles— passes (storage-level test, unaffected by the origin change)Made with Cursor