extend subtensor extension#2569
Conversation
| setup_reserves( | ||
| netuid, | ||
| 1_000_000_000_000_u64.into(), | ||
| 1_000_000_000_000_u64.into(), |
There was a problem hiding this comment.
Let's start adding TypeScript tests for newly added extrinsics, maybe as a separate PR, for previously added.
You can check examples of those tests here.
There was a problem hiding this comment.
I don't get your point. The PR doesn't add any new extrinsic. setup_reserves is a helper function in test.
shamil-gadelshin
left a comment
There was a problem hiding this comment.
Looks good overall. I found a minor issue.
| Ok(()) | ||
| } | ||
|
|
||
| /// Same checks as [`Self::do_serve_prometheus`] before storage writes (for transaction extension). |
There was a problem hiding this comment.
It makes sense to use validate_serve_prometheus within do_serve_prometheus if it is the same code to remove a duplication.
There was a problem hiding this comment.
will remove the redundant part.
thewhaleking
left a comment
There was a problem hiding this comment.
One blocking bug in the new reveal_mechanism_weights SignedExtension arm — it discards mecid and looks up the commit hash under the wrong storage index, so any reveal under mecid != MechId::MAIN is rejected at the pool as CommitNotFound. Inline suggestion attached. Caught by bittensor e2e test_commit_and_reveal_weights_legacy.
| Some(Call::reveal_mechanism_weights { | ||
| netuid, | ||
| mecid: _, | ||
| uids, | ||
| values, | ||
| salt, | ||
| version_key, | ||
| }) => { | ||
| if Self::check_weights_min_stake(who, *netuid) { | ||
| let provided_hash = Pallet::<T>::get_commit_hash( | ||
| who, | ||
| NetUidStorageIndex::from(*netuid), | ||
| uids, | ||
| values, | ||
| salt, | ||
| *version_key, | ||
| ); | ||
| match Pallet::<T>::find_commit_block_via_hash(provided_hash) { | ||
| Some(commit_block) => { | ||
| if Pallet::<T>::is_reveal_block_range(*netuid, commit_block) { | ||
| Ok((Default::default(), (), origin)) | ||
| } else { | ||
| Err(CustomTransactionError::CommitBlockNotInRevealRange.into()) | ||
| } | ||
| } | ||
| None => Err(CustomTransactionError::CommitNotFound.into()), | ||
| } | ||
| } else { | ||
| Err(CustomTransactionError::StakeAmountTooLow.into()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Thank you! will fix it.
|
@thewhaleking there are two failed test cases now.
|
Great. bittensor#3352 will fix these aswell. Thanks! |
| .map(|validity| (validity, (), origin.clone())) | ||
| } | ||
|
|
||
| Some(Call::swap_hotkey_v2 { |
There was a problem hiding this comment.
will remove this call.
| } | ||
| Some(Call::increase_take { hotkey, take: _ }) | ||
| | Some(Call::decrease_take { hotkey, take: _ }) => { | ||
| Self::result_to_validity(Pallet::<T>::do_take_checks(who, hotkey), 0u64) |
There was a problem hiding this comment.
We also check if the take is in range, so probably it makes sense to add this check as well, otherwise we don't check the whole thing we do in the extrinsic.
There was a problem hiding this comment.
It makes sense to add the check for value scope. I will add it.
| if Self::check_weights_min_stake(who, *netuid) { | ||
| let provided_hash = Pallet::<T>::get_commit_hash( | ||
| who, | ||
| Pallet::<T>::get_mechanism_storage_index(*netuid, *mecid), | ||
| uids, | ||
| values, | ||
| salt, | ||
| *version_key, | ||
| ); | ||
| match Pallet::<T>::find_commit_block_via_hash(provided_hash) { | ||
| Some(commit_block) => { | ||
| if Pallet::<T>::is_reveal_block_range(*netuid, commit_block) { | ||
| Ok((Default::default(), (), origin)) | ||
| } else { | ||
| Err(CustomTransactionError::CommitBlockNotInRevealRange.into()) | ||
| } | ||
| } | ||
| None => Err(CustomTransactionError::CommitNotFound.into()), | ||
| } | ||
| } else { | ||
| Err(CustomTransactionError::StakeAmountTooLow.into()) | ||
| } |
There was a problem hiding this comment.
Nesting could be reduced, only a suggestion
| if Self::check_weights_min_stake(who, *netuid) { | |
| let provided_hash = Pallet::<T>::get_commit_hash( | |
| who, | |
| Pallet::<T>::get_mechanism_storage_index(*netuid, *mecid), | |
| uids, | |
| values, | |
| salt, | |
| *version_key, | |
| ); | |
| match Pallet::<T>::find_commit_block_via_hash(provided_hash) { | |
| Some(commit_block) => { | |
| if Pallet::<T>::is_reveal_block_range(*netuid, commit_block) { | |
| Ok((Default::default(), (), origin)) | |
| } else { | |
| Err(CustomTransactionError::CommitBlockNotInRevealRange.into()) | |
| } | |
| } | |
| None => Err(CustomTransactionError::CommitNotFound.into()), | |
| } | |
| } else { | |
| Err(CustomTransactionError::StakeAmountTooLow.into()) | |
| } | |
| if !Self::check_weights_min_stake(who, *netuid) { | |
| return Err(CustomTransactionError::StakeAmountTooLow.into()); | |
| } | |
| let provided_hash = Pallet::<T>::get_commit_hash( | |
| who, | |
| NetUidStorageIndex::from(*netuid), | |
| uids, | |
| values, | |
| salt, | |
| *version_key, | |
| ); | |
| let Some(commit_block) = Pallet::<T>::find_commit_block_via_hash(provided_hash) | |
| else { | |
| return Err(CustomTransactionError::CommitNotFound.into()); | |
| }; | |
| if !Pallet::<T>::is_reveal_block_range(*netuid, commit_block) { | |
| return Err(CustomTransactionError::CommitBlockNotInRevealRange.into()); | |
| } | |
| Ok((Default::default(), (), origin)) |
|
@thewhaleking add the value scope check for delegate take. need to update e2e tests. thanks! Some(Call::increase_take { hotkey, take })
| Some(Call::decrease_take { hotkey, take }) => {
if *take < Pallet::<T>::get_min_delegate_take() {
return Err(CustomTransactionError::DelegateTakeTooLow.into());
}
if *take > Pallet::<T>::get_max_delegate_take() {
return Err(CustomTransactionError::DelegateTakeTooHigh.into());
} |
Description
The PR try to add extension for all Pays::no extrinsic, to guarantee no one misuse it as malicious attack.
Related Issue(s)
Type of Change
Breaking Change
If this PR introduces a breaking change, please provide a detailed description of the impact and the migration path for existing applications.
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.