[balances] Safeguard against consumer ref underflow #3865
Conversation
ggwpez
changed the title
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
| ); | ||
|
|
||
| // normal transfers still work: | ||
| hypothetically!({ |
There was a problem hiding this comment.
oh this is a great macro, I always manually used a transactional layer!
| @@ -278,6 +279,23 @@ pub fn info_from_weight(w: Weight) -> DispatchInfo { | |||
| DispatchInfo { weight: w, ..Default::default() } | |||
| } | |||
|
|
|||
| /// Check that the total-issuance matches the sum of all accounts' total balances. | |||
| pub fn ensure_ti_valid() { | |||
There was a problem hiding this comment.
Can we easily inject this at the end of all tests in this crate?
There was a problem hiding this comment.
Ah yes. I though about adding a fn register_cleanup_hook to the TestExternalities.
Then we could set this in the externalities as hook and have it run at the end without any refactoring. Otherwise i think we need to refactor to not directly use the execute_with but wrap it again.
There was a problem hiding this comment.
I would be curious to also know if it's feasible as a try-state hook
There was a problem hiding this comment.
Otherwise i think we need to refactor to not directly use the execute_with but wrap it again.
Yeah this is what most pallets do, and I generally find it easier.
I would be curious to also know if it's feasible as a try-state hook
Ideally it should be, but yeah iterating all accounts will ruin everything else 🙈 We need to think of a system to separate try-state hooks that we always run in a place like CI vs. those that we want to run every month etc.
There was a problem hiding this comment.
I would be really interested to bench it. We have a lot of really heavy staking hooks today without much issue, maybe as long as it's O(n) it's OK.
There was a problem hiding this comment.
looking forward to adding it :)
There was a problem hiding this comment.
I ran these checks in remote-externalities to find some issues with this: https://github.com/ggwpez/wtfwt
takes like 3-5 secs for one run on Polkadot.
| @@ -278,6 +279,23 @@ pub fn info_from_weight(w: Weight) -> DispatchInfo { | |||
| DispatchInfo { weight: w, ..Default::default() } | |||
| } | |||
|
|
|||
| /// Check that the total-issuance matches the sum of all accounts' total balances. | |||
| pub fn ensure_ti_valid() { | |||
There was a problem hiding this comment.
I would be curious to also know if it's feasible as a try-state hook
There are some accounts that do not have a consumer ref while having a reserve. This adds a fail-safe mechanism to trigger in the case that `does_consume` is true, but the assumption of `consumer>0` is not. This should prevent those accounts from loosing balance and the TI from getting messed up even more, but is not an "ideal" fix. TBH an ideal fix is not possible, since on-chain data is in an invalid state. --------- Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
There are some accounts that do not have a consumer ref while having a reserve. This adds a fail-safe mechanism to trigger in the case that `does_consume` is true, but the assumption of `consumer>0` is not. This should prevent those accounts from loosing balance and the TI from getting messed up even more, but is not an "ideal" fix. TBH an ideal fix is not possible, since on-chain data is in an invalid state. --------- Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
There are some accounts that do not have a consumer ref while having a reserve. This adds a fail-safe mechanism to trigger in the case that `does_consume` is true, but the assumption of `consumer>0` is not. This should prevent those accounts from loosing balance and the TI from getting messed up even more, but is not an "ideal" fix. TBH an ideal fix is not possible, since on-chain data is in an invalid state. --------- Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Bump pallet-balances to include paritytech/polkadot-sdk#3865 and clean up CHANGELOG. - [ ] Does not require a CHANGELOG entry --------- Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
There are some accounts that do not have a consumer ref while having a reserve. This adds a fail-safe mechanism to trigger in the case that `does_consume` is true, but the assumption of `consumer>0` is not. This should prevent those accounts from loosing balance and the TI from getting messed up even more, but is not an "ideal" fix. TBH an ideal fix is not possible, since on-chain data is in an invalid state. --------- Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
There are some accounts that do not have a consumer ref while having a reserve.
This adds a fail-safe mechanism to trigger in the case that
does_consumeis true, but the assumption ofconsumer>0is not.This should prevent those accounts from loosing balance and the TI from getting messed up even more, but is not an "ideal" fix. TBH an ideal fix is not possible, since on-chain data is in an invalid state.