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

Validator with apparently zero backing chosen #4331

Closed
gavofyork opened this issue Dec 9, 2019 · 8 comments
Closed

Validator with apparently zero backing chosen #4331

gavofyork opened this issue Dec 9, 2019 · 8 comments
Assignees
Labels
I2-security The client fails to follow expected, security-sensitive, behaviour.
Milestone

Comments

@gavofyork
Copy link
Member

At some point, a validator with zero exposure was found to be offline:

https://polkascan.io/pre/kusama-cc3/event/146198-0

If shouldn't be possible to be selected as a validator with zero exposure. Either there's something wrong with Phragmen or there's something wrong with the exposure fetching as part of im_online.

@gavofyork gavofyork added this to the 2.1 milestone Dec 9, 2019
@gavofyork gavofyork added the I2-security The client fails to follow expected, security-sensitive, behaviour. label Dec 9, 2019
@kianenigma
Copy link
Contributor

could very well be phragmen. I had it on my todo list to add a 'minimum entry barrier' parameter to it, since at the moment, it allows all candidates to be considered and potentially elected.

@kianenigma
Copy link
Contributor

kianenigma commented Dec 10, 2019

Meaning: Phragmen indeed allows one with zero stake to be elected. Yet, I am sure that staking should never allow someone to bond with a value less than ED, so it is even more unclear to me where the issue is. Simple patch is to add the aforementioned minimum_entry_barrier parameter to phragmen but I am not sure if this the concern of phragmen. Staking (or any other caller of phragmen) should never allow a candidate to have zero stake, if desired.

Also, unbonding in a way that leads to dust is also handled.

@burdges
Copy link

burdges commented Dec 12, 2019

We want some minimum self stake I thought, which presumably gives the minimum_entry_barrier, no? If we supported a separate minimum_exit/ellect_barrier then this requires tweaks to phragmen.

@gui1117
Copy link
Contributor

gui1117 commented Dec 12, 2019

Is it nearby the last session of an era ? if so I feel it could be related to #4121

let's say there is new era at session 5
at end of session 4 session pallet calls OnSessionEnding::on_session_ending(4, 6),
staking module actually executes this code

impl<T: Trait> OnSessionEnding<T::AccountId, Exposure<T::AccountId, BalanceOf<T>>> for Module<T> {
fn on_session_ending(_ending: SessionIndex, start_session: SessionIndex)
-> Option<(Vec<T::AccountId>, Vec<(T::AccountId, Exposure<T::AccountId, BalanceOf<T>>)>)>
{
Self::ensure_storage_upgraded();
Self::new_session(start_session - 1)
}
}

So new_session(5) is called and returns the new validator set.
session pallet receive the new validator set but this set in its queue because he was asking for the validator set of session 6.

Then at the end of the session 5 the value in storage staking::Stakers are the new validator set but the value in session::Validators is still the old validator set.
So if the offence ask for a validator which is not in next one its exposure is 0.

proposed fix is #4346

EDIT: a good way to check would be to query the state of the exposure in previous session and if the validator hasn't change

@gui1117
Copy link
Contributor

gui1117 commented Dec 13, 2019

I confirm:

  • at block 145500 exposure is some for the given account
  • at block 145621:
    • the exposure is set to none,
    • staking current_era storage change: new era
    • session validators storage doesn't change.
  • at block 146198:
    • session validators storage change,

@gui1117 gui1117 mentioned this issue Dec 13, 2019
4 tasks
@gui1117 gui1117 self-assigned this Dec 13, 2019
@kianenigma
Copy link
Contributor

Replaying the accounts transactions from CC2 from sudo_as():

  • block 9082: staking.bond: {value: 100,000,000,000, ctrl: oxff968774036fe8d9800cb8784cdaa4f09bf23096b2a68f589b933c4fe3c3e8d016}
  • block 9085: session.SetKeys: ...
  • block 9085: staking.validate: {prefs: {validatorPayment: 0}}
  • block 11061: session.SetKeys: ...
  • block 11080: staking.validate: {prefs: {validatorPayment: 0}}
  • block 12158: staking.validate: {prefs: {validatorPayment: 1,000,000,000}}

Nothing strange. Current status of the account seems unchanged. Still bonded with 100,000,000,000 and not elected. FYI ED is 10,000,000,000.

We want some minimum self stake I thought, which presumably gives the minimum_entry_barrier, no? If we supported a separate minimum_exit/ellect_barrier then this requires tweaks to phragmen.

This is implicitly forced via staking; See my previous comment:

Meaning: Phragmen indeed allows one with zero stake to be elected. Yet, I am sure that staking should never allow someone to bond with a value less than ED, so it is even more unclear to me where the issue is. Simple patch is to add the aforementioned minimum_entry_barrier parameter to phragmen but I am not sure if this the concern of phragmen. Staking (or any other caller of phragmen) should never allow a candidate to have zero stake, if desired.

Also, unbonding in a way that leads to dust is also handled.

Thanks @thiolliere for following up; Your explanations makes sense. I'm online is falling into default() getter of Exposure. Would be good if we could add some optional logs to warn in such cases where the default value of a map is being used? We do have some maps that, despite supporting returning a default, should always be fetched with existing values.

@kianenigma
Copy link
Contributor

As for phragmen's internal code: as mentioned, unless if further problems pop up, I rather not make it involved in such details. The API is defined as: if a code calls elect(A) with candidate set A, it means that any member of A could be elected, even if they have very little or no backing support. ATM, both staking and elections take care of this:

  • let new_set = new_set_with_approval
    .into_iter()
    .filter_map(|(m, a)| if a.is_zero() { None } else { Some(m) } )
    .collect::<Vec<T::AccountId>>();
  • in staking since all validators vote for themselves we know that we have the entry barrier.

@gui1117 gui1117 mentioned this issue Jan 14, 2020
@gnunicorn gnunicorn modified the milestones: 2.1, Polkadot Mar 4, 2020
@gui1117
Copy link
Contributor

gui1117 commented Mar 12, 2020

Close by #4474

@gui1117 gui1117 closed this as completed Mar 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
I2-security The client fails to follow expected, security-sensitive, behaviour.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants