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

Document, clean and stabilise phragmen's behavior with dupe candiadtes and voters. #4593

Closed
kianenigma opened this issue Jan 10, 2020 · 8 comments
Labels
A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. I4-annoyance The client behaves within expectations, however this “expected behaviour” itself is at issue. Z6-mentor An easy task where a mentor is available. Please indicate in the issue who the mentor could be.

Comments

@kianenigma
Copy link
Contributor

Both staking and elections should not allow duplicate candidates, but still, it happened #4592. A simple fix should make sure that phragmen is internally also dealing with this, in the proper way. Currently, out of luck, the dupes are being ignored, but it should be done in a more mature manner.

@kianenigma kianenigma added I4-annoyance The client behaves within expectations, however this “expected behaviour” itself is at issue. Z6-mentor An easy task where a mentor is available. Please indicate in the issue who the mentor could be. labels Jan 10, 2020
@shawntabrizi shawntabrizi self-assigned this Jan 12, 2020
@shawntabrizi
Copy link
Member

@kianenigma lets do this together this week

@kianenigma
Copy link
Contributor Author

More precisely:

  • dupe candidate
  • dupe voter
  • dupe target

@kianenigma
Copy link
Contributor Author

I have found a bug related to this: using offline-phragmen, I realized that this assertion in the reduce code is failing, only in polkadot at the moment:

debug_assert!(other_votes_count <= 2);

Which means:

Someone submitted a duplicate vote, and it somehow made its way into the reduce code. After tweaking the error message, I find this dude:

Screenshot 2020-07-20 at 17 35 27

Surprising is that it seems like seq-phragmen elected both of the duplicate edges. This should have not happened, and needs investigating.

Screenshot 2020-07-20 at 17 38 32

@kianenigma
Copy link
Contributor Author

More evidence:

#17 --> NO_IDENT [8889bb12ffc22c93e6190aeac259184d7181bed3f0cc9938d27315f8e61c8c4a (1462Rm6Z...)] [total backing = 16934,734DOT (16,934,734,626,230,101) (4 voters)] [own backing = 0,010DOT (10,000,000,000)]
  Voters:
    #1 [amount = 6720,128DOT (6,720,128,027,299,203)] 75d7a967cb385d6f867d150f0727d9cba858c65d192530b44298a06c9bfdea9c (13fWfiEQ...)
    #2 [amount = 0,000DOT (42,560,803)] 75d7a967cb385d6f867d150f0727d9cba858c65d192530b44298a06c9bfdea9c (13fWfiEQ...)
    #3 [amount = 10214,596DOT (10,214,596,556,370,095)] aa6908fa06b85012d44fc8966d9760fb0b0fd9282be9f239927a0089e29e94c8 (14rSLrbz...)
    *#4 [amount = 0,010DOT (10,000,000,000)] 8889bb12ffc22c93e6190aeac259184d7181bed3f0cc9938d27315f8e61c8c4a (1462Rm6Z...)

@kianenigma
Copy link
Contributor Author

kianenigma commented Jul 20, 2020

Albeit, on chain this seems to be fine, meaning that the final exposure is sane, yet it might be dubious and unfair.

Screenshot 2020-07-20 at 17 41 06

@kianenigma
Copy link
Contributor Author

As an additional part of this, Both staking and elections-phragmen should ensure that the votes submitted by the voter are all unique, rather than relying on the underlying election crate to filter it out for them.

@coriolinus
Copy link
Contributor

One random note unrelated to this PR per se: this pallet had quite a bad rap for not having a well defined behaviour with duplicate votes and candidates and targets etc. Now, finally, everything that goes through setup_input (called prior to seq_phragmen and phragmms) gets sanitized. Would be good to ensure these functions have the same standard.

Originally posted by @kianenigma in #8236 (comment)

@stale
Copy link

stale bot commented Jul 7, 2021

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Jul 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. I4-annoyance The client behaves within expectations, however this “expected behaviour” itself is at issue. Z6-mentor An easy task where a mentor is available. Please indicate in the issue who the mentor could be.
Projects
Status: Done
Development

No branches or pull requests

3 participants