Skip to content

Conversation

@open-junius
Copy link
Contributor

@open-junius open-junius commented Apr 22, 2025

Description

Related Issue(s)

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Other (please describe):

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

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have run cargo fmt and cargo clippy to ensure my code is formatted and linted correctly
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Screenshots (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.

@open-junius
Copy link
Contributor Author

Did the nonce fix get reverted/regressed in Bittensor? Nonce issue for setting weights on hotkey was addressed there a while back, regressed in Feb, then fixed again. opentensor/bittensor#2534 Not opposed to this, but it had something addressing it at Bittensor layer.

  • This would also trigger identity fees on all subnets for valis using one hotkey atm.
  • How does this appear to stakers when looking for staking options?

Can you elaborate your comment? can't understand it. Did you suggest any code change in Bittensor?

This PR's original issue mentioned resolving the fact that a single hotkey used as a validator across all subnets suffered from an inability to set weights consistently (related to nonces and transaction pools?). The link I provided was a resolution for that issue at the Bittensor level, not Subtensor, so I was unsure why this was being raised unless that fix regressed. Original issue mentioned several things, but this was the item I mentioned above: "Due to a ratelimits, validators can only set weights from one hotkey on one subnet in any given block."

My understanding for the solution is the new netuid parameter give the coldkey ability to swap hotkey in any subnet. It will remove the limit of tx rate. also save the cost to register hotkey in every subnet.
Please @ppolewicz give more context about it, and provide precise answer

@open-junius open-junius requested a review from a team May 14, 2025 06:36
@sam0x17 sam0x17 requested review from JohnReedV and l0r1s May 19, 2025 03:31
pub const InitialEmaPriceHalvingPeriod: u64 = 201_600_u64; // 4 weeks
pub const DurationOfStartCall: u64 = 7 * 24 * 60 * 60 / 12; // 7 days
pub const InitialKeySwapOnSubnetCost: u64 = 10_000_000;
pub const HotkeySwapOnSubnetInterval: u64 = 7 * 24 * 60 * 60 / 12; // 7 days
Copy link
Collaborator

@l0r1s l0r1s May 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is 7 days divided by 12 here? The comment should be 7 days / 12 then

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one block each 12 seconds. like RootEnterDuration: BlockNumber = 5 * 60 * 24; // 24 hours

Comment on lines 215 to 216
/// Estimating the maximum stake for limited staking operations returned zero.
/// Zero max stake amount
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an issue with the comment here, no? Should be line 215 only

Suggested change
/// Estimating the maximum stake for limited staking operations returned zero.
/// Zero max stake amount
/// Estimating the maximum stake for limited staking operations returned zero.

Copy link
Contributor Author

@open-junius open-junius May 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be a mistake, will remove it.

sam0x17
sam0x17 previously approved these changes May 30, 2025
l0r1s
l0r1s previously approved these changes Jun 4, 2025

// 17. Return the weight of the operation
// 22. Return the weight of the operation
Ok(Some(weight).into())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the goal to emit a refund of the weight here?

Comment on lines 940 to 941
#[pallet::weight((Weight::from_parts(240_600_000, 0)
.saturating_add(T::DbWeight::get().reads(31))
Copy link
Collaborator

@l0r1s l0r1s Jun 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if there is a limit on the number of hotkey a user can have but I think this is related to the way you handle weight in the impl, you are trying to make it dynamic and return the weight at the end but what is returned is refunded, not billed to the user. What it does is take the weight defined here, take the returned weight and refund the difference. Is it what we want? What you could do is benchmark with a parameter on a big number of hotkey, like 100 (I don't know if this is realist), which will be the max amount billed to the user and then refund the difference based on the actual number of hotkey changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to make the benchmark work for the extrinsic. we can discuss it internally.
The extrinsic just change one hotkey, I don't understand why we need bench test for 100 keys.
Do you mean we change the hotkey over 100 subnets

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to modify the benchmark.

Copy link
Collaborator

@l0r1s l0r1s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The benchmark job has been skipped?

@l0r1s l0r1s removed the skip-validate-benchmarks This PR fails validate benchmarks but needs to be merged anyway label Jun 4, 2025
@JohnReedV JohnReedV dismissed stale reviews from l0r1s and sam0x17 via ff01f9e June 4, 2025 15:57
@sam0x17 sam0x17 merged commit 548f984 into devnet-ready Jun 4, 2025
29 checks passed
sam0x17 added a commit that referenced this pull request Jun 14, 2025
sam0x17 added a commit that referenced this pull request Jun 14, 2025
This was referenced Jun 18, 2025
@sam0x17 sam0x17 mentioned this pull request Jun 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-cargo-audit This PR fails cargo audit but needs to be merged anyway

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants