Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validator disabling in session enhancements #7663

Open
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

Overkillus
Copy link
Contributor

@Overkillus Overkillus commented Feb 21, 2025

On top of 7581.

Adds the missing testing cases and adresses some overdue review feedback post merge.

  • Clean up disabling interface
  • Review disabling severity defaults in the refactor
  • Verify and test disabling duration (should be era long)
  • Add always sorted try-runtime test for DisabledValidators.
  • expose severities
  • better debuggin/tracing
  • Make migration more robust
  • Ensure we dont disable for past era slashes (+test)
  • Move disabling strategy tests from staking to session (left some there as it also tests the offence pipeline)
  • more?

@Overkillus Overkillus self-assigned this Feb 21, 2025
@Overkillus Overkillus added T8-polkadot This PR/Issue is related to/affects the Polkadot network. T10-tests This PR/Issue is related to tests. labels Feb 21, 2025
@Overkillus Overkillus changed the title Mkz session disabling plus Validator disabling in session enhancements Feb 21, 2025
@tdimitrov
Copy link
Contributor

Looks good so far.

@Overkillus Overkillus marked this pull request as ready for review February 25, 2025 19:45
@Overkillus Overkillus requested a review from a team as a code owner February 25, 2025 19:45
@Overkillus Overkillus requested a review from ordian February 27, 2025 13:04
Copy link
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

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

small nits, looks good overall

@@ -81,7 +83,7 @@ pub mod v17 {

#[frame_support::storage_alias]
pub type DisabledValidators<T: Config> =
StorageValue<Pallet<T>, BoundedVec<(u32, OffenceSeverity), ConstU32<100>>, ValueQuery>;
StorageValue<Pallet<T>, BoundedVec<(u32, OffenceSeverity), ConstU32<300>>, ValueQuery>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't we wanted to merge v16 and v17?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure but how does that connect to this bounded vec limit? @Ank4n

Copy link
Contributor

Choose a reason for hiding this comment

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

GitHub wouldn’t let me comment on the mod v17 line.

@Gasila5

This comment was marked as spam.

Comment on lines +667 to +670
/// Public function to access the disabled validators with their severities.
pub fn disabled_validators_with_severities() -> Vec<(u32, OffenceSeverity)> {
DisabledValidators::<T>::get()
}
Copy link
Member

Choose a reason for hiding this comment

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

is the plan to expose this in a runtime API or how is it planned to be used?

Comment on lines -767 to -779
/// Disable the validator identified by `c`. (If using with the staking pallet,
/// this would be their *stash* account.)
///
/// Returns `false` either if the validator could not be found or it was already
/// disabled.
pub fn disable(c: &T::ValidatorId) -> bool {
Validators::<T>::get()
.iter()
.position(|i| i == c)
.map(|i| Self::disable_index(i as u32))
.unwrap_or(false)
}

Copy link
Member

Choose a reason for hiding this comment

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

any reason to keep disable_index, but not disable? i thought the former was done for backwards compatibility, but without the latter this is a breaking change

Copy link
Contributor Author

@Overkillus Overkillus Mar 4, 2025

Choose a reason for hiding this comment

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

We can disable, disable with severity and re-enable. All those could be called with either index or validator id. Instead of having 6+ different functions we provide index ones and supply pub fn validator_id_to_index(id: &T::ValidatorId) -> Option<u32> so users can do the translation themselves.

Do you think it would be sensible to duplicate all those calls with _validator_id where we just bake in the call to validator_id_to_index?

Comment on lines +16 to +19
- name: pallet-staking
bump: major
- name: pallet-session
bump: major
Copy link
Member

Choose a reason for hiding this comment

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

i believe it doesn't have to be major if you keep disable pub fn

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'd prefer removing old disable. It simplifies and unifies the interface while still allowing for old-scholl interface with the provided translation func (pub fn validator_id_to_index).

@kianenigma kianenigma mentioned this pull request Mar 14, 2025
33 tasks
@paritytech-workflow-stopper
Copy link

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/13897850312
Failed job name: test-linux-stable

1 similar comment
@paritytech-workflow-stopper
Copy link

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/13897850312
Failed job name: test-linux-stable

@paritytech-workflow-stopper
Copy link

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/13897850312
Failed job name: test-linux-stable-int

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T8-polkadot This PR/Issue is related to/affects the Polkadot network. T10-tests This PR/Issue is related to tests.
Projects
Status: No status
Status: In Progress
Development

Successfully merging this pull request may close these issues.

5 participants