[Feature] Configurable Tempo & Owner-Triggered Epochs#2638
Conversation
evgeny-s
added
the
skip-cargo-audit
evgeny-s
changed the title
| Self::blocks_until_next_epoch(netuid, Self::get_tempo(netuid), current_block) == 0 | ||
| let tempo = Self::get_tempo(netuid); | ||
| if tempo == 0 { | ||
| return false; |
There was a problem hiding this comment.
This means that if tempo is set to 0, manual trigger or max cap of MAX_TEMPO will not work. Is this by design?
There was a problem hiding this comment.
Yes, the previous implementation meant that if tempo is 0, we don't run tempo on this subnet.
We don't have this case in Mainnet, but we support it. There is still a possibility to set the tempo to 0 by the root.
So we support the same semantics.
| /// stateful scheduler (period `tempo + 1`, anchored on `LastEpochBlock`). Used by the | ||
| /// admin-freeze-window predicate and external tooling. Returns `u64::MAX` when | ||
| /// `tempo == 0` (legacy defensive short-circuit). | ||
| pub fn blocks_until_next_epoch(netuid: NetUid, tempo: u16, block_number: u64) -> u64 { |
There was a problem hiding this comment.
"Period is tempo + 1: next firing at last + tempo + 1." comment and the code below is not anymore correct.
There was a problem hiding this comment.
Updated the function name and the comment to avoid the confusion: ad7ba80
|
|
||
| let now = Self::get_current_block_as_u64(); | ||
|
|
||
| Tempo::<T>::insert(netuid, tempo); |
There was a problem hiding this comment.
Should this be a function since Tempo and LastEpochBlock are always updated together?
|
Hey, @basfroman , we have a bunch of tests that failed on the SDK side. |
|
|
||
| /// Effective activity cutoff in blocks, derived from `ActivityCutoffFactorMilli` and `Tempo`. | ||
| /// `cutoff_blocks = (factor × tempo) / 1000`, clamped to ≥ 1. | ||
| pub fn get_activity_cutoff_blocks(netuid: NetUid) -> u64 { |
There was a problem hiding this comment.
should we use it to replace get_activity_cutoff everywhere? for instance in metagraph.
There was a problem hiding this comment.
Yes, because ActivityCutoff is deprecated and will be removed in the follow-up release.
Who can help with updating it?
There was a problem hiding this comment.
It is fine to remove the storage later. Actually, my question is for https://github.com/opentensor/subtensor/blob/devnet-ready/pallets/subtensor/src/rpc_info/metagraph.rs#L713, if we should use get_activity_cutoff_blocks to replace get_activity_cutoff? It will change the return type, from u16 to u64.
There was a problem hiding this comment.
Good catch, for me to update:
- metagraph call
- breaking changes section in the description, regarding the metagraph
There was a problem hiding this comment.
Could you please mark ActivityCutoff as #[deprecated(note = "Some explanation")]? It will propagate to metadata.
# Conflicts: # pallets/subtensor/src/utils/rate_limiting.rs
# Conflicts: # pallets/subtensor/src/coinbase/run_coinbase.rs # pallets/subtensor/src/macros/dispatches.rs # pallets/subtensor/src/macros/events.rs # pallets/subtensor/src/macros/hooks.rs # pallets/subtensor/src/tests/migration.rs # pallets/subtensor/src/tests/mod.rs # pallets/subtensor/src/utils/rate_limiting.rs
- Updated reveal/commit logic based on stateful epoch index - Updated migration for CR-v2 storage item
🛡️ AI Review — Skeptic (security review)VERDICT: SAFE Baseline scrutiny: established contributor with write permission, same author/committer, no known Gittensor allowlist hit, targeting devnet-ready. Reviewed the pre-fetched PR metadata, prior Skeptic sticky, changed-file list, commits, comments, and the runtime diff statically. No FindingsNo findings. ConclusionNo security vulnerability or malicious behavior found in the reviewed diff. Verdict is SAFE. 🔍 AI Review — Auditor (domain review)VERDICT: 👎 Established contributor with write permission; no direct Gittensor allowlist hit, LIKELY by repo-contribution heuristic; overlapping PRs share common runtime files but do not duplicate this dynamic-tempo scope. The Auditor proposed a replacement PR description, but the current body is non-trivial; not overwriting. Maintainers: ask the Auditor to regenerate if you want it. The prior owner-extrinsic guard findings still apply. The implementation still authenticates only against I did not run the test suite; the issue is visible statically. I also attempted the devnet-ready Findings
Prior-comment reconciliation
ConclusionBlocking on the missing subnet-existence guard in the new owner extrinsics. Add 📜 Previous run (superseded)
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| impl<T: Config> Pallet<T> { | ||
| /// Owner-side `set_tempo` implementation. | ||
| pub fn do_set_tempo(origin: OriginFor<T>, netuid: NetUid, tempo: u16) -> DispatchResult { | ||
| let who = Self::ensure_subnet_owner(origin, netuid)?; |
There was a problem hiding this comment.
[MEDIUM] Guard set_tempo against non-existent subnets
ensure_subnet_owner only compares against SubnetOwner[netuid], whose storage type has a default account value; it does not prove that NetworksAdded[netuid] exists. This direct owner extrinsic should match the admin-utils pattern and reject unknown netuids before writing Tempo / LastEpochBlock / rate-limit state. Otherwise a call against an unregistered netuid can create scheduler/hyperparameter state outside the subnet lifecycle.
| let who = Self::ensure_subnet_owner(origin, netuid)?; | |
| ensure!(Self::if_subnet_exist(netuid), Error::<T>::SubnetNotExists); | |
| let who = Self::ensure_subnet_owner(origin, netuid)?; |
| netuid: NetUid, | ||
| factor_milli: u32, | ||
| ) -> DispatchResult { | ||
| let who = Self::ensure_subnet_owner(origin, netuid)?; |
There was a problem hiding this comment.
[MEDIUM] Guard activity-cutoff updates against non-existent subnets
This has the same lifecycle hole as set_tempo: the owner check does not establish that the subnet exists, but the call writes ActivityCutoffFactorMilli and rate-limit state. Add the explicit subnet existence check before consuming the origin.
| let who = Self::ensure_subnet_owner(origin, netuid)?; | |
| ensure!(Self::if_subnet_exist(netuid), Error::<T>::SubnetNotExists); | |
| let who = Self::ensure_subnet_owner(origin, netuid)?; |
| /// Schedules the triggered epoch to fire after `AdminFreezeWindow` blocks; that | ||
| /// countdown engages the freeze window for the subnet via `is_in_admin_freeze_window`. | ||
| pub fn do_trigger_epoch(origin: OriginFor<T>, netuid: NetUid) -> Result<(), DispatchError> { | ||
| let who = Self::ensure_subnet_owner(origin, netuid)?; |
There was a problem hiding this comment.
[MEDIUM] Guard manual epoch triggers against non-existent subnets
trigger_epoch can currently write PendingEpochAt for a netuid that is not in NetworksAdded. Since PendingEpochAt participates in scheduling and the admin freeze predicate, keep it inside the normal subnet lifecycle by rejecting missing subnets first.
| let who = Self::ensure_subnet_owner(origin, netuid)?; | |
| ensure!(Self::if_subnet_exist(netuid), Error::<T>::SubnetNotExists); | |
| let who = Self::ensure_subnet_owner(origin, netuid)?; |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
| impl<T: Config> Pallet<T> { | ||
| /// Owner-side `set_tempo` implementation. | ||
| pub fn do_set_tempo(origin: OriginFor<T>, netuid: NetUid, tempo: u16) -> DispatchResult { | ||
| let who = Self::ensure_subnet_owner(origin, netuid)?; |
There was a problem hiding this comment.
[MEDIUM] Guard set_tempo against non-existent subnets
ensure_subnet_owner only compares SubnetOwner[netuid] to the signer; it does not prove NetworksAdded[netuid] is alive. Prior delete-subnet migrations removed NetworksAdded without clearing SubnetOwner, so an old owner of a deleted subnet can still pass this check and recreate Tempo / LastEpochBlock entries for a non-existent netuid. Check Self::if_subnet_exist(netuid) before the owner check, and update the benchmarked weight if the added read is not already covered.
| let who = Self::ensure_subnet_owner(origin, netuid)?; | |
| ensure!(Self::if_subnet_exist(netuid), Error::<T>::SubnetNotExists); | |
| let who = Self::ensure_subnet_owner(origin, netuid)?; |
| netuid: NetUid, | ||
| factor_milli: u32, | ||
| ) -> DispatchResult { | ||
| let who = Self::ensure_subnet_owner(origin, netuid)?; |
There was a problem hiding this comment.
[MEDIUM] Guard activity-cutoff updates against non-existent subnets
This path has the same stale-owner problem: it can write ActivityCutoffFactorMilli[netuid] for a netuid that is no longer present in NetworksAdded. Add the subnet-existence check before accepting the owner origin, and account for the extra storage read in weights if needed.
| let who = Self::ensure_subnet_owner(origin, netuid)?; | |
| ensure!(Self::if_subnet_exist(netuid), Error::<T>::SubnetNotExists); | |
| let who = Self::ensure_subnet_owner(origin, netuid)?; |
| /// Schedules the triggered epoch to fire after `AdminFreezeWindow` blocks; that | ||
| /// countdown engages the freeze window for the subnet via `is_in_admin_freeze_window`. | ||
| pub fn do_trigger_epoch(origin: OriginFor<T>, netuid: NetUid) -> Result<(), DispatchError> { | ||
| let who = Self::ensure_subnet_owner(origin, netuid)?; |
There was a problem hiding this comment.
[MEDIUM] Guard manual epoch triggers against non-existent subnets
A stale SubnetOwner entry can still schedule PendingEpochAt[netuid] for a deleted subnet because this extrinsic never checks NetworksAdded. The scheduler only iterates live subnets, but leaving trigger state behind for dead netuids is inconsistent with the rest of subnet lifecycle cleanup and can become active if the netuid is later reused. Require Self::if_subnet_exist(netuid) before the owner check, and update the weight if needed.
| let who = Self::ensure_subnet_owner(origin, netuid)?; | |
| ensure!(Self::if_subnet_exist(netuid), Error::<T>::SubnetNotExists); | |
| let who = Self::ensure_subnet_owner(origin, netuid)?; |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
evgeny-s
added
the
breaking-change
|
@opentensor/cerebrum / @opentensor/gyrus / @opentensor/cortex breaking change detected! Please prepare accordingly! |
| /// locked-mass decay. Passing `false` removes the flag, returning the | ||
| /// caller's lock to normal decay. | ||
| #[pallet::call_index(138)] | ||
| #[pallet::call_index(139)] |
There was a problem hiding this comment.
Why are we changing the call index here?
| }, | ||
|
|
||
| /// Activity-cutoff factor (per-mille) set on a subnet by its owner. | ||
| ActivityCutoffFactorMilliSet(NetUid, u32), |
There was a problem hiding this comment.
We should prefer named fields over tuple for events, easier to access from client side. event.netuid is more meaningful than event.0.
| /// `should_run_epoch` returned true but `is_epoch_input_state_consistent` returned false; | ||
| /// schedule advanced, epoch execution skipped. | ||
| EpochSkippedDueToInconsistentState { | ||
| /// The subnet identifier. | ||
| netuid: NetUid, | ||
| /// The block at which the slot was consumed. | ||
| block: u64, | ||
| }, |
There was a problem hiding this comment.
It would be more flexible to have EpochSkipped, more future-proof if we decide to add new skip reasons.
| /// Tempo value out of `[MinTempo, MaxTempo]` bounds. | ||
| TempoOutOfBounds, | ||
| /// Activity-cutoff factor out of `[MinActivityCutoffFactorMilli, MaxActivityCutoffFactorMilli]` bounds. | ||
| ActivityCutoffFactorMilliOutOfBounds, | ||
| /// `trigger_epoch` called while a previously triggered epoch is still pending. | ||
| EpochTriggerAlreadyPending, |
There was a problem hiding this comment.
Prefer plain English doc comments here rather than references to specific function names, since these comments are exposed through runtime metadata and may be shown client-side.
| /// `should_run_epoch` returned true but `is_epoch_input_state_consistent` returned false; | ||
| /// schedule advanced, epoch execution skipped. |
|
|
||
| /// Effective activity cutoff in blocks, derived from `ActivityCutoffFactorMilli` and `Tempo`. | ||
| /// `cutoff_blocks = (factor × tempo) / 1000`, clamped to ≥ 1. | ||
| pub fn get_activity_cutoff_blocks(netuid: NetUid) -> u64 { |
There was a problem hiding this comment.
Could you please mark ActivityCutoff as #[deprecated(note = "Some explanation")]? It will propagate to metadata.
| /// Lower bound for owner-set tempo. Also the fixed cooldown for `set_tempo`. | ||
| pub const MIN_TEMPO: u16 = 360; | ||
| /// Upper bound for owner-set tempo (≈ 7 days at 12 s/block). | ||
| pub const MAX_TEMPO: u16 = 50_400; | ||
| /// Lower bound for activity-cutoff factor (per-mille). 1_000 = one full tempo. | ||
| pub const MIN_ACTIVITY_CUTOFF_FACTOR_MILLI: u32 = 1_000; | ||
| /// Upper bound for activity-cutoff factor (per-mille). 50_000 = 50 tempos. | ||
| pub const MAX_ACTIVITY_CUTOFF_FACTOR_MILLI: u32 = 50_000; | ||
| /// Default activity-cutoff factor (per-mille). 13_889 ≈ legacy 5000-block cutoff | ||
| /// at default tempo 360 (`13_889 * 360 / 1000 = 5_000`, exact via ceiling rounding). | ||
| pub const INITIAL_ACTIVITY_CUTOFF_FACTOR_MILLI: u32 = 13_889; | ||
| /// Per-block cap on number of epochs that may execute in a single `block_step`. | ||
| pub const MAX_EPOCHS_PER_BLOCK: u32 = prod_or_fast!(2, 32); |
There was a problem hiding this comment.
Do we want this to be hardcoded here or maybe as config params?
| netuid: NetUid, | ||
| factor_milli: u32, | ||
| ) -> DispatchResult { | ||
| let who = Self::ensure_subnet_owner(origin, netuid)?; |
There was a problem hiding this comment.
Do we also want root to be able to change this?
| // If the subnet is deferred past this block the | ||
| // commits are taken once here and the later block(s) become no-ops. | ||
| let cur_epoch = Self::current_epoch_with_lookahead(netuid); |
There was a problem hiding this comment.
Here we reveal commits before knowing whether the subnet epoch will actually execute in this block. If MaxEpochsPerBlock has already been reached, the epoch can be deferred, leaving the revealed weights public before being used. That seems to weaken the commit-reveal property and may allow actors to react to revealed weights through other epoch-relevant state. Can we either reveal only on the successful execution path, or document/test why this gap is not exploitable?
| // Bonds masking inside `distribute_emission` reads `LastMechansimStepBlock` and | ||
| // must see the previous successful run, so we delay the write until after. | ||
| Self::distribute_emissions_to_subnets(&emissions_to_distribute); | ||
|
|
||
| // --- 7. Mark each successful epoch run as the last mechanism step. | ||
| for netuid in emissions_to_distribute.keys() { | ||
| LastMechansimStepBlock::<T>::insert(*netuid, current_block); | ||
| } | ||
| } |
There was a problem hiding this comment.
Could we move the LastMechansimStepBlock update into distribute_emissions_to_subnets after each successful distribute_emission call? Keeping the write outside the distribution function makes this sequencing easy to miss for future call sites.
4 participants
Description
Implementation of the specification.
Demo (non-technical): https://dynamic-tempo-non-technical.netlify.app/
Demo (technical): https://dynamic-tempo-technical.netlify.app/#1
Related Issue(s)
Type of Change
Breaking Change
Check the 11. Breaking changes & compatibility section.
Checklist
./scripts/fix_rust.shto ensure my code is formatted and linted correctlyScreenshots (if applicable)
Please include any relevant screenshots or GIFs that demonstrate the changes made.
Additional Notes
Please provide any additional information or context that may be helpful for reviewers.
Simulations (miner rewards Events):
Trigger epoch:
Set tempo: