Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Don't use fixed nominator count for report_equivocation weight calculation #14471

Merged
merged 4 commits into from
Jul 18, 2023

Conversation

andresilva
Copy link
Contributor

@andresilva andresilva commented Jun 27, 2023

This PR changes the report_equivocation weight calculation to take into account the runtime configured maximum number of nominators per validator, rather than using a constant value.

polkadot companion: paritytech/polkadot#7432

@andresilva andresilva added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Jun 27, 2023
@andresilva andresilva requested a review from a team June 27, 2023 14:14
@davxy
Copy link
Member

davxy commented Jun 28, 2023

I struggle a bit with the idea that a consensus algorithm is so strictly bound to a staking concept such as nominators (that in practice is not mandatory to be used).

Isn't better to set this once and read from some external source?

(Just an idea) What about providing a WeightInfo implementation which has the ability to plug some external "extra cost" source (via a trait bound?). Staking pallet then implements this trait and can be used in any place to augment the weight with the cost related to the max nominators.

@andresilva
Copy link
Contributor Author

@davxy I agree that currently the weight implementation is highly coupled with the existing staking implementation. We can probably abstract it in a way to decrease this coupling, although TBH I'm not sure how valuable that would be. Moving this to the staking pallet would make it depend on the internals of GRANDPA (basically reversing the coupling). And the WeightInfo is already pluggable in the pallet, so if you're using GRANDPA with another staking system you can already bring in your own weight implementation. Either way, feel free to create an issue to track this, I don't think such changes should be done in this PR.

@louismerlin louismerlin added D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Jul 11, 2023
@bkchr bkchr requested a review from a team July 11, 2023 13:23
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

👍

@andresilva
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit e806cef into master Jul 18, 2023
@paritytech paritytech deleted a comment from paritytech-processbot bot Jul 18, 2023
@paritytech-processbot paritytech-processbot bot deleted the andre/report-equivocation-weight branch July 18, 2023 14:11
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
…ation (paritytech#14471)

* babe: fix report_equivocation weight calculation

* grandpa: fix report_equivocation weight calculation

* beefy: fix report_equivocation weight calculation

* runtime: add missing MaxNominators constant
Ank4n pushed a commit that referenced this pull request Jul 22, 2023
…ation (#14471)

* babe: fix report_equivocation weight calculation

* grandpa: fix report_equivocation weight calculation

* beefy: fix report_equivocation weight calculation

* runtime: add missing MaxNominators constant
Lederstrumpf added a commit to acatangiu/substrate that referenced this pull request Aug 3, 2023
Lederstrumpf added a commit to Lederstrumpf/substrate that referenced this pull request Aug 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants