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
| } | ||
|
|
||
| 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!
pallets/parachain-staking/src/lib.rs
Outdated
| 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.
| 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.
|
The preview deployment is ready. 🟢 Open Preview | Open Build Logs Last updated at: 2025-08-20 14:41:18 CET |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces the initial phase of the slashing mechanism for collators in the parachain staking module. Key changes include enabling/disabling slashing via genesis and callable extrinsics, adding new weight functions and types to support unjailing and collateral management, and updating migration and test logic to reflect these changes.
- Introduces the slashing_enabled flag in genesis and runtime.
- Adds new extrinsics: set_slashing_enabled, set_min_unjailed_duration, and unjail_candidate.
- Updates tests, weights, migrations, and chain specs to integrate the new slashing behavior.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| precompiles/parachain-staking/src/mock.rs | Adds slashing_enabled field for genesis configuration. |
| pallets/parachain-staking/src/weights.rs | Provides weight functions for the new slashing-related extrinsics. |
| pallets/parachain-staking/src/weightinfo.rs | Declares new weight functions used for slashing and unjailing. |
| pallets/parachain-staking/src/types.rs | Introduces the JailingStatus enum for capturing candidate status. |
| pallets/parachain-staking/src/tests.rs | Updates tests to account for slashing_enabled events and behavior. |
| pallets/parachain-staking/src/mock.rs | Updates genesis config to include slashing_enabled. |
| pallets/parachain-staking/src/migrations.rs | Adjusts migration logic and storage version to enable slashing. |
| pallets/parachain-staking/src/lib.rs | Implements new storage items, events, and extrinsics for slashing. |
| node/src/parachain/*_chain_spec.rs | Updates chain specs to set slashing_enabled true in genesis config. |
Comments suppressed due to low confidence (3)
pallets/parachain-staking/src/lib.rs:564
- [nitpick] Consider adding a brief comment here to explain why slashing is triggered only on the first block of each round; this will aid future maintainers in understanding the design decision.
if SlashingEnabled::<T>::get() && current_round.first == n {
pallets/parachain-staking/src/lib.rs:2066
- Verify that using T::MaxTopCandidates::get() as the parameter for the unjail_candidate weight function accurately reflects the actual computational cost; consider revisiting the weight parameters if future changes affect performance.
))]
pallets/parachain-staking/src/migrations.rs:118
- Consider explicitly using the V12 constant instead of Versions::default() when updating the storage version to improve clarity in the migration process.
);
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.