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

Allow 24 nominations per validator and publish count #7929

Closed
wants to merge 17 commits into from

Conversation

gavofyork
Copy link
Member

@gavofyork gavofyork commented Jan 19, 2021

polkadot companion: paritytech/polkadot#2417

@gavofyork gavofyork requested a review from kianenigma January 19, 2021 05:01
@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Jan 19, 2021
@gavofyork gavofyork added B7-runtimenoteworthy C1-low PR touches the given topic and has a low impact on builders. labels Jan 19, 2021
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

I would take such direction to prevent all runtimes being bound to this configuration:

Staking would accept type CompactSolution: sp_npos_elections::CompactSolution<T::AccountId> as input in which the max-naminations is implicitly defined.

Later on, once #7909 is merged this configuration will be moved to the new election pallet and staking would only need a type MaxNominations: Get<_>.

@kianenigma
Copy link
Contributor

Actually, doing the above requires a bit of making things generic in staking, which is pretty much in vain if we also merge #7909. I'd say lets wait until we are almost at the next release:

if 7909 is merged, there's no need for this.
else, we merge this one.

I'll get it in good shape anyhow though.

@gavofyork gavofyork requested a review from andresilva as a code owner January 21, 2021 15:15
@kianenigma
Copy link
Contributor

Should build well now and the 24 is configurable from the runtime. Lots of boilerplate. as you see, so I'd consider not doing it this way as this will all be removed by the linked PR anyhow.

};
pub use weights::WeightInfo;

/// The default solution type used in substrate-node with 24 maximum winners. A runtime can customize this to their needs.
pub mod default_solution {
Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly I put this mostly here so that it can be blindly used in tests/benchmarks etc. Polkadot or any other chain should probably generate their own version.

/// The compact solution type used to accept offchain solution.
///
/// This is implicitly encoding the maximum number of nominations as well.
type CompactSolution: CompactSolution + frame_support::Parameter + Default;
Copy link
Contributor

Choose a reason for hiding this comment

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

TLDR ^^^^^^^^^^^^^^^^^

@@ -208,7 +209,7 @@ benchmarks! {
let r in 1 .. MAX_REPORTERS;
// we skip 1 offender, because in such case there is no slashing
let o in 2 .. MAX_OFFENDERS;
let n in 0 .. MAX_NOMINATORS.min(MAX_NOMINATIONS as u32);
let n in 0 .. MAX_NOMINATORS.min(<T as pallet_staking::Config>::CompactSolution::LIMIT as u32);
Copy link
Contributor

Choose a reason for hiding this comment

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

I kinda blindly changed these and didn't check the full scope, they were 16 before, and now they are 24, just different syntax.

Copy link
Contributor

Choose a reason for hiding this comment

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

we can reexecute benchmarks to see

@kianenigma kianenigma added D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited A0-please_review Pull request needs code review. A7-needspolkadotpr and removed A0-please_review Pull request needs code review. A7-needspolkadotpr D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Feb 8, 2021
@gavofyork
Copy link
Member Author

image

companion failing @kianenigma

Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

the title of the PR should be "Allow 24 nominations per nominator and publish count", no ?

frame/staking/src/lib.rs Outdated Show resolved Hide resolved
frame/staking/src/lib.rs Outdated Show resolved Hide resolved
frame/staking/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

staking weight should be updated also I think,

otherwise looks good to me

kianenigma and others added 5 commits February 9, 2021 17:49
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
@kianenigma
Copy link
Contributor

kianenigma commented Feb 10, 2021

Has a companion now, locally my build was borked but had something to do with tracing in network crate, so def. not my issue. All runtimes build go well.

Will wait to see the result here.

Either way, it is ready for audit.

@kianenigma kianenigma added the D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited label Feb 10, 2021
@gnunicorn gnunicorn added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Mar 4, 2021
@gnunicorn
Copy link
Contributor

anyone working on this?

@kianenigma
Copy link
Contributor

awaiting audit as it is no longer trivial.

@kianenigma
Copy link
Contributor

kianenigma commented Mar 16, 2021

reintroduced in #8113

@kianenigma kianenigma closed this Mar 16, 2021
@shawntabrizi shawntabrizi removed the D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited label Feb 15, 2022
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. A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. C1-low PR touches the given topic and has a low impact on builders.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants