diff --git a/pallets/commitments/src/lib.rs b/pallets/commitments/src/lib.rs index 5ed05744ed..d1f085747d 100644 --- a/pallets/commitments/src/lib.rs +++ b/pallets/commitments/src/lib.rs @@ -211,7 +211,8 @@ pub mod pallet { /// Set the commitment for a given netuid #[pallet::call_index(0)] #[pallet::weight(( - ::WeightInfo::set_commitment(), + ::WeightInfo::set_commitment() + .saturating_add(T::CanCommit::validation_weight()), DispatchClass::Normal, Pays::No ))] @@ -221,10 +222,8 @@ pub mod pallet { info: Box>, ) -> DispatchResult { let who = ensure_signed(origin.clone())?; - ensure!( - T::CanCommit::can_commit(netuid, &who), - Error::::AccountNotAllowedCommit - ); + T::CanCommit::validate(netuid, &who) + .map_err(|_| Error::::AccountNotAllowedCommit)?; let extra_fields = info.fields.len() as u32; ensure!( @@ -356,12 +355,21 @@ pub mod pallet { // Interfaces to interact with other pallets pub trait CanCommit { - fn can_commit(netuid: NetUid, who: &AccountId) -> bool; + type Error; + + fn validate(netuid: NetUid, who: &AccountId) -> Result<(), Self::Error>; + fn validation_weight() -> frame_support::weights::Weight; } impl CanCommit for () { - fn can_commit(_: NetUid, _: &A) -> bool { - false + type Error = (); + + fn validate(_: NetUid, _: &A) -> Result<(), Self::Error> { + Err(()) + } + + fn validation_weight() -> frame_support::weights::Weight { + frame_support::weights::Weight::zero() } } diff --git a/pallets/commitments/src/mock.rs b/pallets/commitments/src/mock.rs index 58ed8cd863..c626a6f784 100644 --- a/pallets/commitments/src/mock.rs +++ b/pallets/commitments/src/mock.rs @@ -90,8 +90,14 @@ impl TypeInfo for TestMaxFields { pub struct TestCanCommit; impl pallet_commitments::CanCommit for TestCanCommit { - fn can_commit(_netuid: NetUid, _who: &u64) -> bool { - true + type Error = (); + + fn validate(_netuid: NetUid, _who: &u64) -> Result<(), Self::Error> { + Ok(()) + } + + fn validation_weight() -> Weight { + Weight::zero() } } diff --git a/pallets/subtensor/src/extensions/subtensor.rs b/pallets/subtensor/src/extensions/subtensor.rs index 7899ed855e..983a1ce97d 100644 --- a/pallets/subtensor/src/extensions/subtensor.rs +++ b/pallets/subtensor/src/extensions/subtensor.rs @@ -8,13 +8,14 @@ use frame_support::{ traits::{IsSubType, OriginTrait}, weights::Weight, }; +use pallet_commitments::CanCommit; use scale_info::TypeInfo; use sp_runtime::traits::{ DispatchInfoOf, Dispatchable, Implication, TransactionExtension, ValidateResult, }; use sp_runtime::{ impl_tx_ext_default, - transaction_validity::{TransactionSource, TransactionValidityError}, + transaction_validity::{TransactionSource, TransactionValidityError, ValidTransaction}, }; use sp_std::marker::PhantomData; use subtensor_macros::freeze_struct; @@ -22,6 +23,7 @@ use subtensor_runtime_common::CustomTransactionError; type CallOf = ::RuntimeCall; type OriginOf = ::RuntimeOrigin; +type CommitmentPolicy = ::CanCommit; #[allow(deprecated)] impl From> for CustomTransactionError { @@ -77,11 +79,13 @@ impl SubtensorTransactionExtension { fn check(origin: &OriginOf, call: &CallOf) -> Result<(), Error> where - T: pallet_shield::Config, + T: pallet_commitments::Config + pallet_shield::Config, CallOf: Dispatchable> + IsSubType> + + IsSubType> + IsSubType>, OriginOf: OriginTrait, + CommitmentPolicy: CanCommit>, { let Some(who) = origin.as_signer() else { return Ok(()); @@ -89,6 +93,11 @@ impl SubtensorTransactionExtension { CheckColdkeySwap::::check(who, call)?; + let commitment_call: Option<&pallet_commitments::Call> = call.is_sub_type(); + if let Some(pallet_commitments::Call::set_commitment { netuid, .. }) = commitment_call { + CommitmentPolicy::::validate(*netuid, who)?; + } + if let Some(call) = applicable_call(call, CheckWeights::::applies_to) { CheckWeights::::check(who, call)?; } @@ -107,15 +116,33 @@ impl SubtensorTransactionExtension { Ok(()) } + + fn commitment_weight(call: &CallOf) -> Weight + where + T: pallet_commitments::Config, + CallOf: IsSubType>, + { + let commitment_call: Option<&pallet_commitments::Call> = call.is_sub_type(); + if matches!( + commitment_call, + Some(pallet_commitments::Call::set_commitment { .. }) + ) { + CommitmentPolicy::::validation_weight() + } else { + Weight::zero() + } + } } impl TransactionExtension> for SubtensorTransactionExtension where - T: Config + pallet_shield::Config + Send + Sync + TypeInfo, + T: Config + pallet_commitments::Config + pallet_shield::Config + Send + Sync + TypeInfo, CallOf: Dispatchable, Info = DispatchInfo, PostInfo = PostDispatchInfo> + IsSubType> + + IsSubType> + IsSubType>, OriginOf: Clone + OriginTrait, + CommitmentPolicy: CanCommit>, { const IDENTIFIER: &'static str = "SubtensorTransactionExtension"; @@ -131,6 +158,7 @@ where .saturating_add( as DE>>::weight(call)) .saturating_add( as DE>>::weight(call)) .saturating_add( as DE>>::weight(call)) + .saturating_add(Self::commitment_weight(call)) } fn validate( @@ -144,7 +172,17 @@ where _source: TransactionSource, ) -> ValidateResult> { Self::check(&origin, call) - .map(|()| (Default::default(), (), origin)) + .map(|()| { + let mut validity = ValidTransaction::default(); + if let Some(who) = origin.as_signer() + && let Some(call) = applicable_call(call, CheckRateLimits::::applies_to) + { + validity + .provides + .extend(CheckRateLimits::::provides_tags(who, call)); + } + (validity, (), origin) + }) .map_err(|error| TransactionValidityError::from(CustomTransactionError::from(error))) } @@ -206,6 +244,9 @@ mod tests { .saturating_add( as DE>::weight( call, )) + .saturating_add(SubtensorTransactionExtension::::commitment_weight( + call, + )) } #[test] @@ -278,6 +319,117 @@ mod tests { }); } + #[test] + fn validate_rejects_ineligible_metadata_commitment() { + new_test_ext(0).execute_with(|| { + let netuid = NetUid::from(1); + let hotkey = U256::from(1); + let coldkey = U256::from(2); + let commitment_call = || { + RuntimeCall::Commitments(pallet_commitments::Call::set_commitment { + netuid, + info: Box::new(pallet_commitments::CommitmentInfo { + fields: frame_support::BoundedVec::default(), + }), + }) + }; + + assert_eq!( + validate_signed(hotkey, &commitment_call()).unwrap_err(), + CustomTransactionError::SubnetNotExists.into() + ); + + add_network(netuid, 1, 0); + assert_eq!( + validate_signed(hotkey, &commitment_call()).unwrap_err(), + CustomTransactionError::UidNotFound.into() + ); + + setup_reserves( + netuid, + 1_000_000_000_000_u64.into(), + 1_000_000_000_000_u64.into(), + ); + register_ok_neuron(netuid, hotkey, coldkey, 0); + assert_ok!(validate_signed(hotkey, &commitment_call())); + }); + } + + #[test] + fn timelocked_commits_reject_at_validity_and_conflict_in_pool() { + new_test_ext(0).execute_with(|| { + let netuid = NetUid::from(1); + let hotkey = U256::from(1); + let coldkey = U256::from(2); + + add_network(netuid, 1, 0); + setup_reserves( + netuid, + 1_000_000_000_000_u64.into(), + 1_000_000_000_000_u64.into(), + ); + register_ok_neuron(netuid, hotkey, coldkey, 0); + SubtensorModule::set_stake_threshold(0); + SubtensorModule::set_weights_set_rate_limit(netuid, 100); + System::set_block_number(10_u64); + let uid = SubtensorModule::get_uid_for_net_and_hotkey(netuid, &hotkey).unwrap(); + let netuid_index = SubtensorModule::get_mechanism_storage_index(netuid, MechId::MAIN); + SubtensorModule::set_last_update_for_uid(netuid_index, uid, 10); + + let call = + RuntimeCall::SubtensorModule(SubtensorCall::commit_timelocked_mechanism_weights { + netuid, + mecid: MechId::MAIN, + commit: Default::default(), + reveal_round: 1, + commit_reveal_version: 4, + }); + assert_eq!( + validate_signed(hotkey, &call).unwrap_err(), + CustomTransactionError::RateLimitExceeded.into() + ); + + System::set_block_number(200_u64); + let first = validate_signed(hotkey, &call).unwrap(); + let second = validate_signed(hotkey, &call).unwrap(); + assert_eq!(first.provides.len(), 1); + assert_eq!(first.provides, second.provides); + }); + } + + #[test] + fn timelocked_commits_with_zero_rate_limit_do_not_conflict_in_pool() { + new_test_ext(0).execute_with(|| { + let netuid = NetUid::from(1); + let hotkey = U256::from(1); + let coldkey = U256::from(2); + + add_network(netuid, 1, 0); + setup_reserves( + netuid, + 1_000_000_000_000_u64.into(), + 1_000_000_000_000_u64.into(), + ); + register_ok_neuron(netuid, hotkey, coldkey, 0); + SubtensorModule::set_stake_threshold(0); + SubtensorModule::set_weights_set_rate_limit(netuid, 0); + + let call = + RuntimeCall::SubtensorModule(SubtensorCall::commit_timelocked_mechanism_weights { + netuid, + mecid: MechId::MAIN, + commit: Default::default(), + reveal_round: 1, + commit_reveal_version: 4, + }); + + let first = validate_signed(hotkey, &call).unwrap(); + let second = validate_signed(hotkey, &call).unwrap(); + assert!(first.provides.is_empty()); + assert!(second.provides.is_empty()); + }); + } + #[test] fn weight_matches_top_level_dispatch_extension_checks() { new_test_ext(1).execute_with(|| { @@ -293,6 +445,12 @@ mod tests { RuntimeCall::SubtensorModule(SubtensorCall::register_network { hotkey: U256::from(9), }), + RuntimeCall::Commitments(pallet_commitments::Call::set_commitment { + netuid: NetUid::from(1), + info: Box::new(pallet_commitments::CommitmentInfo { + fields: frame_support::BoundedVec::default(), + }), + }), ]; for call in calls { diff --git a/pallets/subtensor/src/guards/check_rate_limits.rs b/pallets/subtensor/src/guards/check_rate_limits.rs index e12c9d064b..b241903ce5 100644 --- a/pallets/subtensor/src/guards/check_rate_limits.rs +++ b/pallets/subtensor/src/guards/check_rate_limits.rs @@ -1,13 +1,14 @@ use super::{CallOf, DispatchableOriginOf, applicable_call}; use crate::weights::WeightInfo; use crate::{Call, Config, Error, Pallet, TransactionType}; +use codec::Encode; use frame_support::{ dispatch::{DispatchErrorWithPostInfo, DispatchExtension, DispatchInfo, PostDispatchInfo}, pallet_prelude::*, traits::{IsSubType, OriginTrait}, }; use sp_runtime::traits::Dispatchable; -use sp_std::marker::PhantomData; +use sp_std::{marker::PhantomData, vec, vec::Vec}; use subtensor_runtime_common::{NetUid, NetUidStorageIndex}; /// Dispatch extension for rate-limit checks that are safe to reject before dispatch. @@ -22,6 +23,9 @@ impl CheckRateLimits { call, Call::commit_weights { .. } | Call::commit_mechanism_weights { .. } + | Call::commit_timelocked_weights { .. } + | Call::commit_timelocked_mechanism_weights { .. } + | Call::commit_crv3_mechanism_weights { .. } | Call::set_weights { .. } | Call::set_mechanism_weights { .. } | Call::register_network { .. } @@ -60,6 +64,21 @@ impl CheckRateLimits { Pallet::::get_mechanism_storage_index(*netuid, *mecid), Error::::CommittingWeightsTooFast, ), + Call::commit_timelocked_weights { netuid, .. } => Self::check_weights_rate_limit( + who, + *netuid, + NetUidStorageIndex::from(*netuid), + Error::::CommittingWeightsTooFast, + ), + Call::commit_timelocked_mechanism_weights { netuid, mecid, .. } + | Call::commit_crv3_mechanism_weights { netuid, mecid, .. } => { + Self::check_weights_rate_limit( + who, + *netuid, + Pallet::::get_mechanism_storage_index(*netuid, *mecid), + Error::::CommittingWeightsTooFast, + ) + } Call::set_weights { netuid, .. } if !Pallet::::get_commit_reveal_weights_enabled(*netuid) => { @@ -88,6 +107,31 @@ impl CheckRateLimits { _ => Ok(()), } } + + /// One pending commit per hotkey and mechanism when commits are rate limited. Calls sharing + /// this tag also share the same on-chain rate limit, so the pool keeps only one candidate + /// instead of landing the rest as deterministic `CommittingWeightsTooFast` failures. + pub(crate) fn provides_tags(who: &T::AccountId, call: &Call) -> Vec> { + let (netuid, netuid_index) = match call { + Call::commit_weights { netuid, .. } + | Call::commit_timelocked_weights { netuid, .. } => { + (*netuid, NetUidStorageIndex::from(*netuid)) + } + Call::commit_mechanism_weights { netuid, mecid, .. } + | Call::commit_timelocked_mechanism_weights { netuid, mecid, .. } + | Call::commit_crv3_mechanism_weights { netuid, mecid, .. } => ( + *netuid, + Pallet::::get_mechanism_storage_index(*netuid, *mecid), + ), + _ => return Vec::new(), + }; + + if Pallet::::get_weights_set_rate_limit(netuid) == 0 { + return Vec::new(); + } + + vec![(b"weight-commit", who, netuid_index).encode()] + } } impl DispatchExtension> for CheckRateLimits @@ -191,6 +235,25 @@ mod tests { mecid: MechId::MAIN, commit_hash: sp_core::H256::zero(), }), + RuntimeCall::SubtensorModule(SubtensorCall::commit_timelocked_weights { + netuid, + commit: Default::default(), + reveal_round: 1, + commit_reveal_version: 4, + }), + RuntimeCall::SubtensorModule(SubtensorCall::commit_timelocked_mechanism_weights { + netuid, + mecid: MechId::MAIN, + commit: Default::default(), + reveal_round: 1, + commit_reveal_version: 4, + }), + RuntimeCall::SubtensorModule(SubtensorCall::commit_crv3_mechanism_weights { + netuid, + mecid: MechId::MAIN, + commit: Default::default(), + reveal_round: 1, + }), set_weights_call(netuid, 0), RuntimeCall::SubtensorModule(SubtensorCall::set_mechanism_weights { netuid, diff --git a/pallets/subtensor/src/tests/mock.rs b/pallets/subtensor/src/tests/mock.rs index 0452471018..9fcc28f866 100644 --- a/pallets/subtensor/src/tests/mock.rs +++ b/pallets/subtensor/src/tests/mock.rs @@ -52,8 +52,20 @@ impl Get for TestMaxFields { pub struct TestCanCommit; impl pallet_commitments::CanCommit for TestCanCommit { - fn can_commit(_netuid: NetUid, _who: &U256) -> bool { - true + type Error = crate::Error; + + fn validate(netuid: NetUid, who: &U256) -> Result<(), Self::Error> { + if !SubtensorModule::if_subnet_exist(netuid) { + return Err(crate::Error::::SubnetNotExists); + } + if !SubtensorModule::is_hotkey_registered_on_network(netuid, who) { + return Err(crate::Error::::HotKeyNotRegisteredInSubNet); + } + Ok(()) + } + + fn validation_weight() -> Weight { + ::DbWeight::get().reads(2) } } diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index f4ad13bf81..56fbc7a287 100644 --- a/runtime/src/lib.rs +++ b/runtime/src/lib.rs @@ -712,15 +712,34 @@ impl Get for MaxCommitFields { #[subtensor_macros::freeze_struct("c39297f5eb97ee82")] pub struct AllowCommitments; impl CanCommit for AllowCommitments { + type Error = pallet_subtensor::Error; + #[cfg(not(feature = "runtime-benchmarks"))] - fn can_commit(netuid: NetUid, address: &AccountId) -> bool { - SubtensorModule::if_subnet_exist(netuid) - && SubtensorModule::is_hotkey_registered_on_network(netuid, address) + fn validate(netuid: NetUid, address: &AccountId) -> Result<(), Self::Error> { + if !SubtensorModule::if_subnet_exist(netuid) { + return Err(pallet_subtensor::Error::::SubnetNotExists); + } + if !SubtensorModule::is_hotkey_registered_on_network(netuid, address) { + return Err(pallet_subtensor::Error::::HotKeyNotRegisteredInSubNet); + } + Ok(()) } #[cfg(feature = "runtime-benchmarks")] - fn can_commit(_: NetUid, _: &AccountId) -> bool { - true + fn validate(_: NetUid, _: &AccountId) -> Result<(), Self::Error> { + Ok(()) + } + + fn validation_weight() -> frame_support::weights::Weight { + #[cfg(not(feature = "runtime-benchmarks"))] + { + ::DbWeight::get().reads(2) + } + + #[cfg(feature = "runtime-benchmarks")] + { + frame_support::weights::Weight::zero() + } } }