Feat/1205528919110616 slashing liveness fault #323
Conversation
…info is used where needed
…igate prepare_delayed_reward from running twice at genesis
…t/1205528919110616_slashing-liveness-fault
| @@ -534,7 +543,13 @@ pub mod pallet { | |||
| crate::migrations::on_runtime_upgrade::<T>() | |||
| } | |||
|
|
|||
| fn on_finalize(_n: BlockNumberFor<T>) { | |||
| fn on_finalize(n: BlockNumberFor<T>) { | |||
There was a problem hiding this comment.
One question about the spec but not about the implementation. If the sudo user triggers the force_new_round, should we punish the collators who are not generating the block? Or should we waive it?
There was a problem hiding this comment.
For now, I kept it like this because it was better for testing purposes. That’s the only way I found to test it in an environment like Chopsticks. Another reason I kept it is that forcing a new round shouldn’t happen frequently. The only scenario I can think of is when we change the token economy. If we really want to force a new round in production we can always deactivate slashing. We can discuss that with the product team.
There was a problem hiding this comment.
Yep, let us discuss with them
If we really want to force a new round in production we can always deactivate slashing
That's a good workaround!
| return; | ||
| } | ||
|
|
||
| if Self::remove_candidate(&collator, &state).is_err() { |
There was a problem hiding this comment.
Does that mean this collator will lose all delegator's staking? If that is the case, this punishment is also severe. Let us check with our business team about the spec
There was a problem hiding this comment.
Okay, let’s check with them. Another workaround would be to remove the candidate from the pool and add a new field to the Candidate object that indicates whether the candidate can be selected. After calling an extrinsic, this candidate could change this value to allow it to be selected again.
| @@ -102,6 +109,21 @@ mod upgrade { | |||
|
|
|||
| log::info!("V11 Migrating Done."); | |||
| } | |||
|
|
|||
| if onchain_storage_version < StorageVersion::new(Versions::V12 as u16) { | |||
There was a problem hiding this comment.
Do you think whether we should remove the above migration logic? Or is keeping them better?
There was a problem hiding this comment.
Are you implying that since the migration has already occurred and the round is now mandatory, it might be reasonable to eliminate it? I think that would make sense but let’s discuss this with @talhadaar.
There was a problem hiding this comment.
Yep, you are right. I'm thinking because only Krest has the old version; however, we shouldn't upgrade it to the lastest version directly. Therefore, it's possible to remove it. Let us discuss it
There was a problem hiding this comment.
I'm not sure what this entails just yet, i'll have a closer look at this PR.
This PR introduces the initial phase of our slashing mechanism. Currently, if a collator fails to produce a block during an entire session, it is removed from the candidate pool. Collators can rejoin the pool after being kicked out by following the usual process of becoming a collator. This is merely a warning for now; no token slashing is being implemented yet.