joystream
joystream copied to clipboard
Joystream
Staking/Nomination lock does not behave as rivalrous
While doing final checks as part of https://github.com/Joystream/joystream/issues/4162 I noticed that it's possible to use the same account (and therefore the same funds) to stake for both some rivalrous purpose (for example: working group application/role) and validation/nomination at the same time.
I suppose this should not be possible, because:
- Validation/nomination lock is not part of
NON_RIVALROUS_LOCKS - It's mentioned as Rivalrous in the handbook: https://joystream.gitbook.io/testnet-workspace/system/accounts-and-staking#locks-1
- It makes sense for it to be rivalrous, as nomination/validation stake is subject to slashing (and so are other rivalrous stakes)
Despite that I have been able to setup an Alice account with those 3 stakes simultaneously:
- staking account candidate stake (
boundsta, Non-Rivalrous) - forum working group role (lead) stake (
wg-forum, Rivalrous!) - nomination stake (
staking, Rivalrous!)
Notice that this is only possible if I create wg-forum stake before staking stake, because otherwise the application would fail on ensure!(T::StakingHandler::is_account_free_of_conflicting_stakes, /* ... */). There is no such check when staking as validator/nominator however, as this logic lives in pallet_staking which is outside of our control.
┆Issue is synchronized with this Asana task by Unito
to stake for both some rivalrous purpose (for example: working group application/role) and validation/nomination at the same time.
I guess this first off begs the question: why did we not find out about this sooner? It certainly means we don't have QA for this? Can we add this @dmtrjsg ?
We also need to add integration tests to for this clearly, so adding this issue https://github.com/Joystream/joystream/issues/4190.
There is no such check when staking as validator/nominator however, as this logic lives in pallet_staking which is outside of our control.
Damn, this is so bad. I can't even compute how we did not think of that, nor how two audits missed this basic point 🤯 . Seems there are two options
- Fork
pallet_staking, and only tweak the place it checks to use our lock type aware conflict chcker. - Make validation and nomination stacke non-rivalrous: main risk I can immediately think of is that it may corrupt the internal assumptions of the staking system if the balance on such an account is just slashed for exogenous reasons, possibly breaks some invariant, and at the very least means someone will earn returns on tokens that don't exist.
Damn, this is so bad (...)
I agree, I cannot think of any "clean" solution at the moment. Initially I thought we could alternatively create our own type (for example StakingCurrency) that implements Currency and LockableCurrency and just override the set_lock method, but unfortunately this method is infallible, so we can't really add any checks there.
The other solution would be to use locks for non-rivalrous staking and reserve for rivalrous staking, but this would be a relatively big and risky rework and would require adjustments in (almost) all of our apps.
but this would be a relatively big and risky rework and would require adjustments in (almost) all of our apps.
Agree, and I believe we did evaluate it carefully and passed, although at this point my confidence in that work has dropped a bit!
Initially I thought we could alternatively create our own type (for example StakingCurrency) that implements Currency and LockableCurrency and just override the set_lock method, but unfortunately this method is infallible, so we can't really add any checks there.
Following this thought, I think we can actually override Currency::free_balance() however in this custom StakingCurrency type. Provided we would only use it for pallet_staking::Config it should be safe to just make it return 0 in case there are any rivalrous locks :thinking:
That is actually quite interesting, it is almost too clever, like its a trap for people in the future, but I do like it.
I tested alternative approach using additional SignedExtension to validate transactions that make a call to the bond() dispatchable in https://github.com/Joystream/joystream/pull/4213
Tested manually and works as expected. Since a node will apply the checks when a tx is submitted to the mempool, the transaction will never make it into a block, and on submission the error will be TransactionInvalid with a custom code, as you would get if submitting an extrinsic with a bad nonce for example. The check also comes before paying transaction fees so user will not loose funds. Our Integrations tests ran successfully without any noticeable slow down. (time taken to complete the full test scenario)
Tested manually and works as expected. Since a node will apply the checks when a tx is submitted to the mempool, the transaction will never make it into a block
Does this mean this check is not part of consensus? if so then I don't think this really works, a motivated attacker could still produce a valid block which violates the rule.
Does this mean this check is not part of consensus? if so then I don't think this really works, a motivated attacker could still produce a valid block which violates the rule.
Of course it is still part of consensus, used to verify blocks and transactions. My point was that it was a bonus that the check is also applied before the tx enters mempool.
So after trying several approaches we finally bit the bullet and decided to make modification in the staking pallet. We are now using a fork of substrate, where we added a BondingRestriction trait on the pallet. Staking Pallet changes: https://github.com/Joystream/substrate/pull/8 Runtime configuration in joystream: https://github.com/Joystream/joystream/pull/4244
