-
Notifications
You must be signed in to change notification settings - Fork 683
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[balances] Safeguard against consumer ref underflow #3865
Conversation
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
); | ||
|
||
// normal transfers still work: | ||
hypothetically!({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we easily inject this at the end of all tests in this crate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be curious to also know if it's feasible as a try-state hook
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking forward to adding it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3-5s is acceptable imo
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_consume
is true, but the assumption ofconsumer>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.