-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
x/bank: Add sanity check on endBlock for total amount of atoms #1381
Comments
Note: these sanity checks don't need to be run by all nodes in order to be effective, they just need to be run by enough stake to halt the network and/or trigger the circuit breaker if an invariant is broken (at which point governance can step in). |
@ebuchman Do you think this should be prioritized (saw you moved to Current Iteration)? I think this is a good idea but I'm not sure it's worth doing before the Hub is feature-complete and we know what all the invariants will be - more of a bonus safety feature than a core concern. |
Removed. |
I know there was some talk (and implementation??) of the invariant checking. Did this get implemented, or can we close this issue? |
It has not been implemented. This is a subset of circuit breaker discussions essentially. |
This is done! |
EndBlock should have a sanity check to ensure that the amount of coins is as expected, this way atoms can't get printed. One way to do this is to store all keys that were accessed in a block, and then at the end of the block iterate over the keys to check that the total amount of atoms changed is 0. (Or whatever x/staking says the delta should be) This won't be perfect, since a bug in staking could cause the value to be altered the exact same way, but it still significantly helps as now the attack / undetected bug vector goes from "there exists a money printing bug" to "there exists a money printing bug, AND a bug in staking's inflation that correlate perfectly".
There are two primary concerns that I see, one is that this can take a long time to compute, and so not everyone should run it. I agree, and we can have it be a config option, along with the other invariant checks.
Second, this only works under apps with default behavior w.r.t. how money is printed. This is true, and I think this means we should have a way for sdk applications to load the default modules with or without such assertions.
this is from a discussion w/ @cwgoes and @adrianbrink
The text was updated successfully, but these errors were encountered: