Disallow using existing TSS account IDs in Staking pallet #657
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Couple small things with the tests, but otherwise this looks good.
I'm not sure if benchmarking / weights need to be updated - if so i don't know how to do that.
We don't need to change the benchmarks. Next time we run them they should account for the extra storage read.
| @@ -356,6 +362,11 @@ pub mod pallet { | |||
| let validator_id = <T as pallet_session::Config>::ValidatorId::try_from(stash) | |||
| .or(Err(Error::<T>::InvalidValidatorId))?; | |||
|
|
|||
| ensure!( | |||
There was a problem hiding this comment.
nit: I would move this under the other ensure! check. Keeps those assertions close together and lets us bail earlier
| assert_ok!(FrameStaking::bond( | ||
| RuntimeOrigin::signed(2), | ||
| 100u64, | ||
| pallet_staking::RewardDestination::Account(2), | ||
| )); | ||
| assert_noop!( | ||
| Staking::validate( | ||
| RuntimeOrigin::signed(2), | ||
| pallet_staking::ValidatorPrefs::default(), | ||
| vec![20], | ||
| 3, | ||
| NULL_ARR | ||
| ), | ||
| Error::<Test>::TssAccountAlreadyExists | ||
| ); |
There was a problem hiding this comment.
I'd move this into its own test just to make sure we can isolate the original issue
| // Check that we cannot change to a TSS account which already exists | ||
| assert_ok!(FrameStaking::bond( | ||
| RuntimeOrigin::signed(2), | ||
| 100u64, | ||
| pallet_staking::RewardDestination::Account(2), | ||
| )); | ||
| assert_ok!(Staking::validate( | ||
| RuntimeOrigin::signed(2), | ||
| pallet_staking::ValidatorPrefs::default(), | ||
| vec![20], | ||
| 5, | ||
| NULL_ARR | ||
| )); | ||
|
|
||
| assert_noop!( | ||
| Staking::change_threshold_accounts(RuntimeOrigin::signed(1), 5, NULL_ARR), | ||
| Error::<Test>::TssAccountAlreadyExists | ||
| ); |
|
Oh one thing I want to bring up. Do we even need to have the ability to change Threshold Accounts on the fly? Really the only scenario I can think of where this would be needed is if a validator wipes their KVDB mid-session. In this case I'd be tempted to have them get kicked from the set and have to re-join with a new identity. Wdyt @JesseAbram? (P.S I know we used this to fix our genesis issue once, but that seems like a one off use case to me) |
HCastano
changed the title
Ya I mean sometimes a good key rotation is in order, or maybe someone got paranoid, or an employee left and they wanna be safe |
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
HCastano
added
the
Bump `impl_version`
This addresses #654 by adding a check to make sure that validators may not use TSS account IDs which already exist.
I'm not sure if benchmarking / weights need to be updated - if so i don't know how to do that.