New slashing logic #570
New slashing logic #570
Conversation
* Slashing improvements - unstake when balance too low - unstake after N slashes according to val prefs - don't early-terminate session/era unless unstaked - offline grace period before punishment * Fix warning * Cleanups and ensure slash_count decays * Bump authoring version and introduce needed authoring stub * Rename * Fix offline tracker * Fix offline tracker * Renames * Add test * Tests * Tests.
* Bump version, don't propose invalid blocks * Fix build. * Fixes. * More fixes. * Fix tests. * Fix more tests * More tests fixed
|
Hm, there are some parts of polkadot checked-in. Is it intended? |
- Don't slash/unstake/change session when too few staking participants - Introduce set_balance PrivCall
| @@ -57,7 +57,7 @@ test:rust:stable: &test | |||
| - export PATH="${CI_PROJECT_DIR}/cargo/bin/:$PATH" | |||
| - ./scripts/build.sh | |||
| - ./scripts/build-demos.sh | |||
| - time cargo test --all | |||
| - time cargo test --all --release | |||
There was a problem hiding this comment.
Why do we need to run tests in --release mode?
There was a problem hiding this comment.
they go incredibly slow otherwise. the sync tests take ages.
There was a problem hiding this comment.
yeah, thought as much. But I wonder maybe it is worth to add RUSTFLAGS="-C debug-assertions"?
| @@ -1,4 +1,30 @@ | |||
| # Substrate | |||
|
|
|||
| Framework for blockchain innovators. | |||
| More to come here. | |||
| Next-generation framework for blockchain innovation. | |||
substrate/runtime/session/src/lib.rs
Outdated
| } | ||
|
|
||
| pub trait Trait: timestamp::Trait { | ||
| // the position of the required timestamp-set extrinsic. | ||
| const NOTE_OFFLINE_POSITION: u32; | ||
| const NOTE_MISSED_PROPOSAL_POSITION: u32; |
There was a problem hiding this comment.
Could you update a comment above?
substrate/runtime/session/src/lib.rs
Outdated
| "note_offline extrinsic must be at position {} in the block", | ||
| T::NOTE_OFFLINE_POSITION | ||
| T::NOTE_MISSED_PROPOSAL_POSITION |
There was a problem hiding this comment.
Could you update error message?
| pub fn slashable_balance(who: &T::AccountId) -> T::Balance { | ||
| Self::nominators_for(who).iter() | ||
| .map(Self::voting_balance) | ||
| .fold(Self::voting_balance(who), |acc, x| acc + x) |
There was a problem hiding this comment.
I wonder if it would be more clear if we express this function as
Self::nomination_balance(who) + Self::voting_balance(who)
?
There was a problem hiding this comment.
this should be faster/smaller since it avoids creation of a temporary zero. (unless the optimiser is super clever...)
There was a problem hiding this comment.
hm, that is really interesting if that really has some impact on the code size. Let me check that...
There was a problem hiding this comment.
yes, it has!
simple: 3100320
optimized: 3099090
so total difference between them is: 1 230!
substrate/runtime/staking/src/lib.rs
Outdated
| @@ -420,9 +551,23 @@ impl<T: Trait> Module<T> { | |||
|
|
|||
| /// Force there to be a new era. This also forces a new session immediately after by | |||
| /// setting `normal_rotation` to be false. Validators will get slashed. | |||
| fn force_new_era(should_slash: bool) -> Result { | |||
| fn force_new_era(apply_rewards: bool) -> Result { | |||
There was a problem hiding this comment.
Could you update the comment?
substrate/runtime/staking/src/lib.rs
Outdated
| // Avoid making new era if it would leave us with fewer than the minimum needed validators | ||
| if intentions.len() < Self::minimum_validator_count() { | ||
| return | ||
| } |
There was a problem hiding this comment.
But isn't it too late for this? We've already changed current era index and applied some pending changes.
There was a problem hiding this comment.
comment out of date - just mean "reevaluate validator set"
| if <TotalStake<T>>::exists() { | ||
| <TotalStake<T>>::put(<Module<T>>::total_stake() - value); | ||
| if let Some(v) = <Module<T>>::total_stake().checked_sub(&value) { | ||
| <TotalStake<T>>::put(v); |
There was a problem hiding this comment.
Why is this conditionally applied? This seems to be a way for introducing inconsistency to this counter
There was a problem hiding this comment.
if TotalStake is implemented correctly, they're equivalent. however if it's not and it undercounts, leading to eventual underflow, then a panic is worse than ignoring, since an invalid TotalStake can be fixed in a later block, but if it gets so low that all transactions cause a panic, rescuing the chain becomes an issue of rolling back which is far harder and possibly even impractical to orchestrate.
substrate/runtime/staking/src/lib.rs
Outdated
| pub unstake_threshold: u32, | ||
| } | ||
|
|
||
| impl Decode for SlashPreference { |
There was a problem hiding this comment.
Can we use derive(Encode, Decode) here?
* Update to latest Substrate master (paritytech#570) * Bump substrate/version (paritytech#557) * Bump version and Substrate (paritytech#560) * Bump version and Substrate * Bump version and Substrate * Bump versions * bump substrate to release specific v0.6.15 * Update lock * Prepare Polkadot update for Substrate runtime interface 2.0 (paritytech#563) * Prepare Polkadot update for Substrate runtime interface 2.0 * bump substrate to release specific v0.6.15 * Switch to `polkadot-master` * Version bump * Master backports * Bump runtime * Fix tests * Fix tests * Another fix.
No description provided.