-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Don't slash all outgoing members. #7394
Conversation
|
but that means there is currently a bug ? because if a member get downgraded to runners-up and then back to member and then downgraded again it will be slashed 2 times, while having deposited only once. If I'm correct then this issue should prioritised as high or medium |
|
Yes this is the behaviour, and indeed let's try and also merge this for the upcoming upgrade. |
|
also it might deserve a runtime note as well, I mean if someone get slashed_reserved too much and then fail to unreserve to funds unrelated to phragmen they could legitimetly ask for refund as well. EDIT: I see you state that |
|
The reasoning for my wording is just that when I coded this initially, it was my intention to be a very strict module where losing any role would cause you to lose your bond. But now I see this is not really sensible. |
shawntabrizi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
|
I want to make sure I fully understand this. Users moving between |
they will be slashed reserved CandidacyDeposit, as to_burn_bond is extended with outgoing runners up EDIT: by not in the list I understood not in runners-up nor in members. |
|
ah sorry I misclicked I want to rerequest a review for myself :-/ |
|
@kianenigma to_burn_bond is extended with outgoing runners-up, but a candidate moving from runners-up to member is an outgoing runners-up, will he get incorrectly slashed ? |
Then this seems just as bad then? Someone on the border is going to get slashed. I think the only people who should be slashed are current members who actually completely leave the set. In fact, I probably would not build in any slashing mechanism as part of this whole process, but rather build an explicit external mechanism to trigger a slash on a user. It is not clear that the behavior of dropping out of any set is actually the result of some malicious actions, and thus qualifies for a slash. |
|
substrate/frame/elections-phragmen/src/lib.rs Line 991 in c5a80cb
nvm i think this is fine, see below |
|
And in-fact you do not track the amount being slashed is only from the reserved balance allocated for this pallet. You can easily slash into other pallet's reserved balances. I would strongly recommend that you remove the slashing mechanism for now until it is properly tracked and handled. |
Yes its planned in https://github.com/paritytech/substrate/pull/7040/files EDIT: I mixed myself, only vote tracking is done in above PR |
|
I agree, the slashing here is too aggressive. I agree that outgoing members and runners-up should not be slashed. Should we keep the slash for loser candidates? that one is more sensible. It discourages any small account to give a shot at being a council member. The point is to only submit a candidacy when you think you are likely to win. But I guess this can be removed as well. |
|
@kianenigma I think that one is fine. especially since the weight to submit candidacy grows with the number of candidates. You can have a reasonable attack to fill the beginning with a bunch of candidates, and force real candidates to pay larger fees to submit their candidacy. |
|
@kianenigma sorry for so many follow ups here. I think i did not understand the pallet fully until I read more of the code. I think actually the slash from runner up to nothing is fine because the user has the opportunity to renounce their candidacy at any time. They should leave the system voluntarily or get slashed for wasting our time. Basically, as long as you are a candidate, we should have a reserved balance for you, and if you dont leave voluntarily, we will slash. This rule was broken when we moved a member to runner-up. They are still a candidate, but did not have a reserved balance. so it is important that |
This was the exact reasoning at first. As a member and runner-up, you have to renounce in time to prevent slash. I honestly still think this is too aggressive and could be relaxed a bit. Nonetheless, because we want this PR in fast, I am okay with: Merge this as-is and re-think the slashing once everything is fixed. Currently
So I think your requirements are met. Failed candidates are also slashed separately (see |
|
what about #7394 (comment) from runners-up to member ppl get slashed_reserved too no ? should we fix here or in another follow up PR ? |
What i realized from reading the code more is that once they leave the Then being slashed is fine because a user should hopefully realize they will not be close to winning and thus should have left the election voluntarily. I mean it is really up to @kianenigma how strict he wants to be with these reserved deposits, but whereas I had an explicit problem before, i dont really see any choice now as better over the other. |
runners-up are candidate: let mut candidates = Self::candidates();
// candidates who explicitly called `submit_candidacy`. Only these folks are at risk of
// losing their bond.
let exposed_candidates = candidates.clone();
// current members are always a candidate for the next round as well.
// this is guaranteed to not create any duplicates.
candidates.append(&mut Self::members_ids());
// previous runners_up are also always candidates for the next round.
candidates.append(&mut Self::runners_up_ids()); |
|
@thiolliere is right this is also a case where we slash by mistake. You can be a runner and then be upgraded to a winner. This will cause you to be in the outgoing set of the runners-up and mistakingly slashed, while we shouldn't. |
|
See the new test case that I added, it covers both cases and now I think we are finally good. |
|
Much thanks to both of you for the rapid reviews ❤️ |
|
/benchmark runtime pallet pallet_elections_phragmen |
|
Finished benchmark for branch: kiz-fix-faulty-election-more Benchmark: Runtime Benchmarks Pallet cargo run --release --features runtime-benchmarks --manifest-path bin/node/cli/Cargo.toml -- benchmark --chain dev --steps 50 --repeat 20 --extrinsic "*" --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output ./bin/node/runtime/src/weights --header ./HEADER --pallet pallet_elections_phragmen ResultsPallet: "pallet_elections_phragmen", Extrinsic: "vote", Lowest values: [], Highest values: [], Steps: [50], Repeat: 20
|
…/node/cli/Cargo.toml -- benchmark --chain dev --steps 50 --repeat 20 --extrinsic * --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output ./bin/node/runtime/src/weights --header ./HEADER --pallet pallet_elections_phragmen
|
Insubstantial weight changes. |
|
bot merge |
|
Waiting for commit status. |
|
Merge failed: |
|
bot merge |
|
Trying merge. |
|
Merge failed: |
|
bot merge |
|
Trying merge. |
A small follow up to #7384, this is not a bug per se but the general slashing attitude of the module was still a bit too aggressive. A members who is now downgraded to a runner-up should not yet be slashed. Instead, only those that are entirely ejected (no longer member + no longer runner-up) should be slashed.
This could also cause members who bounced back and forth between being a member and runner to lose their
CandidacyBond.