Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
85 commits
Select commit Hold shift + click to select a range
e89dff9
First steps to stash/controller separation
gavofyork Feb 7, 2019
8e9c2d5
Merge remote-tracking branch 'origin/master' into gav-new-staking
gavofyork Feb 13, 2019
29d1b20
More drafting
gavofyork Feb 14, 2019
ac5d897
More drafting
gavofyork Feb 14, 2019
bfc089f
Finish draft.
gavofyork Feb 14, 2019
fe19f73
Optimisation
gavofyork Feb 14, 2019
24cedfb
Remove accidental commit
gavofyork Feb 14, 2019
0876c86
Make it build.
gavofyork Feb 15, 2019
c9f643f
Fix linked map for traits.
tomusdrw Feb 15, 2019
9190f7d
Fix Option<_> variant.
tomusdrw Feb 15, 2019
4b5274e
Merge branch 'master' into td-fixlinked
tomusdrw Feb 15, 2019
5354a2e
Improve naming a tad
tomusdrw Feb 15, 2019
00ba07e
Rebuild runtime
tomusdrw Feb 15, 2019
78bcf1a
Merge remote-tracking branch 'origin/master' into gav-new-staking2
gavofyork Feb 15, 2019
5ec3741
Merge remote-tracking branch 'origin/td-fixlinked' into gav-new-staking2
gavofyork Feb 15, 2019
ca18c79
Builds!
gavofyork Feb 15, 2019
a24dd8a
First test.
gavofyork Feb 15, 2019
15a3100
Bump RT version
gavofyork Feb 15, 2019
bd0a3de
Minor fix
gavofyork Feb 15, 2019
68f5405
Update Mock
shawntabrizi Feb 19, 2019
001a665
adds the correct reward testcase (+staking eras which was already ok)
kianenigma Feb 19, 2019
7042842
fixes the basic staking testcase to work properly (along with a small…
kianenigma Feb 20, 2019
75bf9d7
New logic to avoid controller transferring stash.
gavofyork Feb 20, 2019
f4ba1d6
Fix some build issues.
gavofyork Feb 20, 2019
a008ee1
Merge branch 'gav-new-staking' of github.com:paritytech/substrate int…
gavofyork Feb 20, 2019
27ad50e
adding some comments to tests
shawntabrizi Feb 20, 2019
93d0eb0
Merge branch 'gav-new-staking' of https://github.com/paritytech/subst…
shawntabrizi Feb 20, 2019
9f7ba25
Merge remote-tracking branch 'origin/master' into gav-new-staking
gavofyork Feb 20, 2019
b755466
Merge branch 'gav-new-staking' of github.com:paritytech/substrate int…
gavofyork Feb 20, 2019
50a212c
Fix impls.
gavofyork Feb 20, 2019
1eb43f7
adds a few more lines to explain the test case
kianenigma Feb 20, 2019
6eaddd1
Merge branch 'gav-new-staking' of github.com:paritytech/substrate int…
kianenigma Feb 20, 2019
e8661b2
More fixes.
gavofyork Feb 20, 2019
7284530
Merge branch 'gav-new-staking' of github.com:paritytech/substrate int…
kianenigma Feb 20, 2019
7e9d469
gets the basic test up and running again
kianenigma Feb 20, 2019
31bed92
Fix rest of build
gavofyork Feb 20, 2019
cadce5e
Merge branch 'gav-new-staking' of github.com:paritytech/substrate int…
gavofyork Feb 20, 2019
37adc08
Rebuild wasm
gavofyork Feb 20, 2019
039f4ea
Fix docs.
gavofyork Feb 20, 2019
4e67748
fix staking test with new chnages
kianenigma Feb 21, 2019
7fe7e01
updating some tests, pending questions
shawntabrizi Feb 21, 2019
a911ee9
Merge branch 'gav-new-staking' of https://github.com/paritytech/subst…
shawntabrizi Feb 21, 2019
514d5a6
More working tests
shawntabrizi Feb 21, 2019
1808869
adds double staking test
kianenigma Feb 21, 2019
2c4f738
Docs
gavofyork Feb 21, 2019
a163f87
Merge branch 'gav-new-staking' of github.com:paritytech/substrate int…
gavofyork Feb 21, 2019
1296562
Merge remote-tracking branch 'origin/master' into gav-new-staking
gavofyork Feb 22, 2019
200eea1
remove invalid slashing test
kianenigma Feb 23, 2019
15b9da0
Payee stuff.
gavofyork Feb 25, 2019
634ab7b
Merge branch 'gav-new-staking' of github.com:paritytech/substrate int…
gavofyork Feb 25, 2019
94a46d4
Fix build
gavofyork Feb 25, 2019
1a2ec9e
Docs
gavofyork Feb 25, 2019
dd7fba6
Fix test
gavofyork Feb 25, 2019
4b0e1e6
Fix a couple of tests
gavofyork Feb 25, 2019
61224f6
Layout plan for finishing tests before Pragmen
shawntabrizi Feb 25, 2019
e4ea05f
Merge branch 'gav-new-staking' of https://github.com/paritytech/subst…
shawntabrizi Feb 25, 2019
3b9917a
Add some working tests
shawntabrizi Feb 25, 2019
6f8f93d
re-build staking and reward tests
kianenigma Feb 25, 2019
4029139
Merge branch 'gav-new-staking' of github.com:paritytech/substrate int…
kianenigma Feb 25, 2019
cc8f195
Add more tests
shawntabrizi Feb 25, 2019
b0129a0
Merge branch 'gav-new-staking' of https://github.com/paritytech/subst…
shawntabrizi Feb 25, 2019
92c0621
fix offline grace test
shawntabrizi Feb 26, 2019
16a9239
Nominator should have payee checked for cleanup
shawntabrizi Feb 26, 2019
1064cfc
adds more nomination tets
kianenigma Feb 26, 2019
6865917
merged
kianenigma Feb 26, 2019
937c89c
adds validator prefs tests
kianenigma Feb 26, 2019
f606db2
Fix and clean up some TODOs
shawntabrizi Feb 27, 2019
6bf11f9
Fix a couple of issues
gavofyork Feb 27, 2019
9a02201
Fix tests
shawntabrizi Feb 27, 2019
be9f4f6
Merge branch 'gav-new-staking' of github.com:paritytech/substrate int…
kianenigma Feb 27, 2019
4ddaa42
noting warnings from tests
shawntabrizi Feb 28, 2019
54ee55b
final fix of local tests
kianenigma Feb 28, 2019
a53c3fd
Merge branch 'gav-new-staking' of https://github.com/paritytech/subst…
shawntabrizi Feb 28, 2019
93da8d8
Fix slot_stake bug
shawntabrizi Feb 28, 2019
4603cfe
Half baked test
shawntabrizi Feb 28, 2019
8bb2e43
fix a few gobal tests
kianenigma Feb 28, 2019
2f43287
Merge branch 'gav-new-staking' of github.com:paritytech/substrate int…
kianenigma Feb 28, 2019
a4ca045
Add logic to limit `unstake_threshold` set in storage
shawntabrizi Feb 28, 2019
ee54da0
Make sure to check before writing!
shawntabrizi Feb 28, 2019
51d25ed
Move a couple of comments
gavofyork Mar 1, 2019
231a02d
devops-parity updated wasm runtime blobs b58d9517 and merged in maste…
devops-parity Mar 1, 2019
68b5938
fix last broken slot_stake test
kianenigma Mar 1, 2019
3a74b73
Merge branch 'gav-new-staking' of github.com:paritytech/substrate int…
kianenigma Mar 1, 2019
ae1e1e5
Merge remote-tracking branch 'origin/master' into gav-new-staking
gavofyork Mar 1, 2019
da9cb52
Ignore broken test
gavofyork Mar 2, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add more tests
  • Loading branch information
shawntabrizi committed Feb 25, 2019
commit cc8f1951a39f21912f61c8b0ab5cdd720ed7e169
4 changes: 2 additions & 2 deletions srml/staking/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -741,8 +741,8 @@ impl<T: Trait> Module<T> {
let event = if new_slash_count > max_slashes {
// They're bailing.
let slash = Self::current_offline_slash()
// Multiply current_offline_slash by 2^unstake_threshold
.checked_shl(prefs.unstake_threshold)
// Multiply current_offline_slash by 2^(unstake_threshold with upper bound)
.checked_shl(unstake_threshold)
.unwrap_or_else(Self::slot_stake);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unclear to me what slot_stake is. Comment above the storage value says:

  /// The highest and lowest staked validator slashable balances.

But it is only a single value...

Elsewhere in the code comments it says:

		// Figure out the minimum stake behind a slot.
		let slot_stake = candidates.last().map(|i| i.1.total).unwrap_or_default();
		<SlotStake<T>>::put(&slot_stake);

Which implies it is a minimum. But then it does not make sense to be used here. I am thinking the logic should be the following:

  • Either punish the user by current_offline_slash * exponential weighting factor

and if that overflows

  • Punish the user by the max stake of any validator (which is then limited by how much they actually exposed in slash_validator

So a max value makes sense to me...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the docs. It's meant to be the amount of the least staked active validator.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gavofyork Does this imply that all validators are only held accountable to the amount of stake staked by the minimum validator?

i.e. if a validator gets chosen with some small amount of stake, that all other validators can only be punished for being offline by that small amount? (hence the fallback)

But if that is the case, why do we only fallback to this when the exponential weighting factor overflows? Doesn't this cause a situation where users will choose high unstake_threshold such that they get a lesser penalty for doing "more bad"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the argument @shawntabrizi mentioned, in a nutshell, one validator (given that it has the funds) can choose a stash_balance + unstake_threshold combination in a way the .checked_shl(unstake_threshold).unwrap_or_else(Self::slot_stake); overflows and then benefit from being slashed only as if it had Self::slot_stake in stash => less punishment than what it deserves.

The counter-argument could be that with the typically expected balances that validators will put in the stash and the fact that max unstake_threshold is 10 this is not really feasible to happen?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as explained in chat, slot_stake is the amount by which all slots are staked. the security parameters of the system assume that each validator slot (and the stakers behind them) may be punished by at least this amount and thus it should be used as the maximum. with MAX_UNSTAKE_THRESHOLD and OfflineSlash that are too high then theoretically a validator could have a theoretical punishment that is greater than slot_stake but not so big as to overflow the Balance. in this case, it would be a little unfair as they would be better to aim to overflow. however, it's not expected that the parameters would ever let this situation happen, and even if it did, it would be such a large write off anyway, it's not like it's compromising the system's security in any way. it's potentially going to slash a little more than it really should.

as discussed elsewhere, i'm happy to cap it at slot_stake anyway.

let _ = Self::slash_validator(&v, slash);
<Validators<T>>::remove(&v);
Expand Down
149 changes: 140 additions & 9 deletions srml/staking/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -546,7 +546,7 @@ fn max_unstake_threshold_works() {
assert_eq!(Staking::current_offline_slash(), 20);
// Account 10 will have unstake_threshold 10
<Validators<Test>>::insert(10, ValidatorPrefs {
unstake_threshold: 10,
unstake_threshold: MAX_UNSTAKE_THRESHOLD,
validator_payment: 0,
});
// Account 20 will have unstake_threshold 100, which should be limited to 10
Expand All @@ -556,12 +556,12 @@ fn max_unstake_threshold_works() {
});

// Report each user 1 more than the max_unstake_threshold
Staking::on_offline_validator(10, 11);
Staking::on_offline_validator(20, 11);
Staking::on_offline_validator(10, MAX_UNSTAKE_THRESHOLD as usize + 1);
Staking::on_offline_validator(20, MAX_UNSTAKE_THRESHOLD as usize + 1);

// Show that each balance only gets reduced by 2^max_unstake_threshold
assert_eq!(Balances::free_balance(&10), u64::max_value() - 2_u64.pow(10) * 20);
assert_eq!(Balances::free_balance(&20), u64::max_value() - 2_u64.pow(10) * 20);
assert_eq!(Balances::free_balance(&10), u64::max_value() - 2_u64.pow(MAX_UNSTAKE_THRESHOLD) * 20);
assert_eq!(Balances::free_balance(&20), u64::max_value() - 2_u64.pow(MAX_UNSTAKE_THRESHOLD) * 20);
});
}

Expand Down Expand Up @@ -677,7 +677,32 @@ fn consolidate_unlocked_works() {

#[test]
fn bond_extra_works() {
// TODO: Learn what it is and test it
// Tests that extra `free_balance` in the stash can be added to stake
with_externalities(&mut ExtBuilder::default().build(),
|| {
// Check that account 10 is a validator
assert!(<Validators<Test>>::exists(10));
// Check that account 10 is bonded to account 11
assert_eq!(Staking::bonded(&11), Some(10));
// Check how much is at stake
assert_eq!(Staking::ledger(&10), Some(StakingLedger { stash: 11, total: 1000, active: 1000, unlocking: vec![] }));

// Give account 11 some large free balance greater than total
Balances::set_free_balance(&11, 1000000);
// Check the balance of the stash account
assert_eq!(Balances::free_balance(&11), 1000000);

// Call the bond_extra function from controller, add only 100
assert_ok!(Staking::bond_extra(Origin::signed(10), 100));
// There should be 100 more `total` and `active` in the ledger
assert_eq!(Staking::ledger(&10), Some(StakingLedger { stash: 11, total: 1000 + 100, active: 1000 + 100, unlocking: vec![] }));

// Call the bond_extra function with a large number, should handle it
assert_ok!(Staking::bond_extra(Origin::signed(10), u64::max_value()));
// The full amount of the funds should now be in the total and active
assert_eq!(Staking::ledger(&10), Some(StakingLedger { stash: 11, total: 1000000, active: 1000000, unlocking: vec![] }));

});
}

#[test]
Expand All @@ -702,7 +727,113 @@ fn slot_stake_does_something() {
}

#[test]
fn on_free_balance_zero_removes_user_from_storage() {
// TODO: When free balance < existential deposit, user is removed
// All storage items about that user are cleaned up
fn on_free_balance_zero_stash_removes_validator() {
// Tests that validator storage items are cleaned up when stash is empty
// Tests that storage items are untouched when controller is empty
with_externalities(&mut ExtBuilder::default()
.existential_deposit(10)
.build(),
|| {
// Check that account 10 is a validator
assert!(<Validators<Test>>::exists(10));
// Check the balance of the validator account
assert_eq!(Balances::free_balance(&10), 256);
// Check the balance of the stash account
assert_eq!(Balances::free_balance(&11), 2560);
// Check these two accounts are bonded
assert_eq!(Staking::bonded(&11), Some(10));

// Set some storage items which we expect to be cleaned up
// Initiate slash count storage item
Staking::on_offline_validator(10, 1);
// Set payee information
assert_ok!(Staking::set_payee(Origin::signed(10), RewardDestination::Stash));

// Check storage items that should be cleaned up
assert!(<Ledger<Test>>::exists(&10));
assert!(<Validators<Test>>::exists(&10));
assert!(<SlashCount<Test>>::exists(&10));
assert!(<Payee<Test>>::exists(&10));

// Reduce free_balance of controller to 0
Balances::set_free_balance(&10, 0);
// Check total balance of account 10
assert_eq!(Balances::total_balance(&10), 0);

// Check the balance of the stash account has not been touched
assert_eq!(Balances::free_balance(&11), 2560);
// Check these two accounts are still bonded
assert_eq!(Staking::bonded(&11), Some(10));

// Check storage items have not changed
assert!(<Ledger<Test>>::exists(&10));
assert!(<Validators<Test>>::exists(&10));
assert!(<SlashCount<Test>>::exists(&10));
assert!(<Payee<Test>>::exists(&10));

// Reduce free_balance of stash to 0
Balances::set_free_balance(&11, 0);
// Check total balance of stash
assert_eq!(Balances::total_balance(&11), 0);

// Check storage items do not exist
assert!(!<Ledger<Test>>::exists(&10));
assert!(!<Validators<Test>>::exists(&10));
assert!(!<Nominators<Test>>::exists(&10));
assert!(!<SlashCount<Test>>::exists(&10));
assert!(!<Payee<Test>>::exists(&10));
assert!(!<Bonded<Test>>::exists(&11));
});
}

#[test]
fn on_free_balance_zero_stash_removes_nominator() {
// Tests that nominator storage items are cleaned up when stash is empty
// Tests that storage items are untouched when controller is empty
with_externalities(&mut ExtBuilder::default()
.existential_deposit(10)
.build(),
|| {
// Make 10 a nominator
assert_ok!(Staking::nominate(Origin::signed(10), vec![20]));
// Check that account 10 is a nominator
assert!(<Nominators<Test>>::exists(10));
// Check the balance of the nominator account
assert_eq!(Balances::free_balance(&10), 256);
// Check the balance of the stash account
assert_eq!(Balances::free_balance(&11), 2560);
// Check these two accounts are bonded
assert_eq!(Staking::bonded(&11), Some(10));

// Check storage items that should be cleaned up
assert!(<Ledger<Test>>::exists(&10));
assert!(<Nominators<Test>>::exists(&10));

// Reduce free_balance of controller to 0
Balances::set_free_balance(&10, 0);
// Check total balance of account 10
assert_eq!(Balances::total_balance(&10), 0);

// Check the balance of the stash account has not been touched
assert_eq!(Balances::free_balance(&11), 2560);
// Check these two accounts are still bonded
assert_eq!(Staking::bonded(&11), Some(10));

// Check storage items have not changed
assert!(<Ledger<Test>>::exists(&10));
assert!(<Nominators<Test>>::exists(&10));

// Reduce free_balance of stash to 0
Balances::set_free_balance(&11, 0);
// Check total balance of stash
assert_eq!(Balances::total_balance(&11), 0);

// Check storage items do not exist
assert!(!<Ledger<Test>>::exists(&10));
assert!(!<Validators<Test>>::exists(&10));
assert!(!<Nominators<Test>>::exists(&10));
assert!(!<SlashCount<Test>>::exists(&10));
assert!(!<Payee<Test>>::exists(&10));
assert!(!<Bonded<Test>>::exists(&11));
});
}