Add min_childkey_take_per_subnet hyperparameter#2734
Conversation
gztensor
added
the
skip-cargo-audit
🛡️ AI Review — Skeptic (security review)VERDICT: SAFE BASELINE scrutiny: author has write permission, established contribution history, no Gittensor allowlist hit, and branch feat/min-ck-take-hyperparam targets devnet-ready. Static review found a narrow diff limited to hotkey-swap storage read-weight accounting, a focused test expectation update, and a runtime FindingsNo findings. ConclusionNo malicious pattern or security vulnerability was found. The changed runtime path adds weight accounting before existing storage helper calls and does not introduce a new panic path, origin bypass, dependency surface, or build-time execution surface. 🔍 AI Review — Auditor (domain review)VERDICT: 👍 Gittensor: LIKELY; no allowlist match, but author has write permission and substantial recent subtensor history. Description discrepancy: the PR body still describes adding Auto-fix: none. FindingsNo findings. Prior-comment reconciliation
ConclusionThe remaining diff is narrow, accounts for the added new-hotkey ownership reads before the storage helpers run, and keeps a runtime spec bump in place. I did not find a remaining blocking domain issue. |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // 3. If the new hotkey already exists globally, ensure the coldkey owns it | ||
| if Self::hotkey_account_exists(new_hotkey) { | ||
| ensure!( | ||
| Self::coldkey_owns_hotkey(&coldkey, new_hotkey), | ||
| Error::<T>::NonAssociatedColdKey | ||
| ); | ||
| } | ||
|
|
||
| // 4. Initialize the weight for this operation | ||
| let mut weight = T::DbWeight::get().reads(2); |
There was a problem hiding this comment.
[HIGH] Account for the new hotkey-owner reads in swap weights
This new check performs extra Owner storage reads before weight is initialized: hotkey_account_exists(new_hotkey) reads once, and when it exists coldkey_owns_hotkey reads again via contains_key plus get. Those reads are not included in the returned post-dispatch weight, and the pre-dispatch weights for swap_hotkey / swap_hotkey_v2 were not updated or rebenchmarked. Since this is a runtime extrinsic, changed storage access must be reflected in weight. Initialize/accrue the dynamic weight before this check or collapse it into a single charged storage access, then update the generated swap_hotkey weight and the hardcoded swap_hotkey_v2 weight.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
| // 3. If the new hotkey already exists globally, ensure the coldkey owns it | ||
| if Self::hotkey_account_exists(new_hotkey) { | ||
| ensure!( | ||
| Self::coldkey_owns_hotkey(&coldkey, new_hotkey), | ||
| Error::<T>::NonAssociatedColdKey | ||
| ); | ||
| } | ||
|
|
||
| // 4. Initialize the weight for this operation | ||
| let mut weight = T::DbWeight::get().reads(2); |
There was a problem hiding this comment.
[HIGH] Account for the new hotkey-owner reads in swap weights
This new ownership gate performs additional Owner storage reads before weight is initialized. On the successful path where new_hotkey already exists and is owned by the caller, hotkey_account_exists(new_hotkey) reads once and coldkey_owns_hotkey(&coldkey, new_hotkey) reads again via contains_key plus get, but the returned post-dispatch weight still starts at only the two reads for the old-hotkey ownership check. That under-reports the actual runtime work and can over-refund swap_hotkey / swap_hotkey_v2.
| // 3. If the new hotkey already exists globally, ensure the coldkey owns it | |
| if Self::hotkey_account_exists(new_hotkey) { | |
| ensure!( | |
| Self::coldkey_owns_hotkey(&coldkey, new_hotkey), | |
| Error::<T>::NonAssociatedColdKey | |
| ); | |
| } | |
| // 4. Initialize the weight for this operation | |
| let mut weight = T::DbWeight::get().reads(2); | |
| // 3. Initialize the weight for ownership checks | |
| let mut weight = T::DbWeight::get().reads(2); | |
| // 4. If the new hotkey already exists globally, ensure the coldkey owns it | |
| if Self::hotkey_account_exists(new_hotkey) { | |
| weight.saturating_accrue(T::DbWeight::get().reads(1)); | |
| ensure!( | |
| Self::coldkey_owns_hotkey(&coldkey, new_hotkey), | |
| Error::<T>::NonAssociatedColdKey | |
| ); | |
| weight.saturating_accrue(T::DbWeight::get().reads(2)); | |
| } else { | |
| weight.saturating_accrue(T::DbWeight::get().reads(1)); | |
| } |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
…ubtensor into feat/min-ck-take-hyperparam
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
1 participant
Description
Add a new hyperparameter
min_childkey_take_per_subnetfor the existing admin util extrinsic with matching name.Type of Change
Breaking Change
Introduces a new
min_childkey_take_per_subnetfield inget_subnet_hyperparams_v3.Checklist
./scripts/fix_rust.shto ensure my code is formatted and linted correctly