ParityOnHowToDealWithBrokenRuntime

bedeho / Joystream

I keep asking about how modules should deal with possible panic resulting from broken invariants, sorry to go on about it, but here is another one:

I am reading the democracy module, and from what I am seeing, when the staked amount assocaited with a preimage is released, an operation is initiated which may fail, from a static point of view, namely repatraite_reserved. Now obviously, if this reservation logic has been correctly managed, this is not possible during runtime. However, in the codepath, the error case is explicitly ignored by doing let _ = repatraite .... This is sort of a silent way of handling broken invaraints.

a) am I understanding this correctly
b) is it correct that this approach to dealing with broken invaraints is prevalent in the Substrate/Polkadot code base, even more so than the approach of trying to report errors for broken invariants
shawntabrizi

When dealing with the runtime logic, you need to weigh "best effort" against "attack vectors".

In the context of repatriate_reserved, we are looking to move some reserved funds from one account to another. In this case, if there was some storage mis-management logic, we can choose to:

    Fail the extrinsic, possibly keeping the accounts in question in a "locked state" until governence steps in.
    Do best effort transfer.

if best effort is not a path of attack to the blockchain, and is reasonable in the mind of the runtime developer, it is obviously preferred, since it will ensure your chain continues to move forward even if there are some errors.
If it is a possible point of attack, it is important to NOT do the best effort, as you then expose your chain to... well an attack vector.
So a good example of this is returning funds versus depositing funds. It would NOT be okay to do "best effort" when taking a deposit. The deposit has been put there as an economic hurdle that a user must overcome in order to use resources of the blockchain. If you allow best effort, it may be that you allow a user to do an actio without paying the appropriate costs
however if you are trying to return funds... well this is not an attack vector for the blockchain. It just sucks for the user if something wrong happened
we should not be minting new funds, so instead we just return as much as we have available
they can take it up with governance/runtime developers later if the issue is found
bedeho / Joystream
Ok, that is very interesting. So take this example I was provided yesterday, where broken invariant detection is built into the error handling

https://github.com/paritytech/substrate/blob/d940c02440f80c5e0ded092c4dbc9f048312d8a6/frame/balances/src/lib.rs#L1028

Its not so clear to me how the two cases (demcracy unreservation vs fund transfer) fall along the rule boundry you are suggesting.
shawntabrizi
Well, the first thing to note about that line is that assuming there are no bugs in managing total issuance, it would be impossible for that addition to fail. Hopefully the note makes sense.
In terms of approaching that line anyway, we could just the same make it saturated_add versus checked_add. I think both are valid ways to handle the operation
Since the function already returns a result, using checked_add makes sense, because might as well allow it to fail when it should
there are some other functions where it must not fail, and cannot return a result. You will find many of these functions as a result of our "verify first, write last scheme"
if this function was one of those, and we could not allow it to fail, the only choice would be to use saturated_add here, which would also be fine
Look at reserve vs unreserve:
https://github.com/paritytech/substrate/blob/d940c02440f80c5e0ded092c4dbc9f048312d8a6/frame/balances/src/lib.rs#L1229
reserve returns a result, unreserve cannot fail
This embodies the principal I mentioned above
Unreserve cannot fail, but it does return any amount which may have not been fully unreserved. You as the runtime developer can choose how to handle it.
bedeho / Joystream
Just to confirm, you are saying this L1236 only returns overflow if the balance module is broken?
shawntabrizi
Yes. the total balance in the entire blockchain (all free + all reserved) is placed in a type T::Balance
Your balance is also placed in T::Balance type. Your balance must be less than (or equal to) the total in the system, and therefore if total balance does not overflow, by definition your balance cannot overflow
sschiessl

Ricardo

    sschiessl: You can follow the instructions from: https://substrate.dev/docs/en/overview/getting-started#manual-installation

Thanks!
bedeho / Joystream
Ok, excellent, thanks shawntabrizi , will think about this rule moving forward, I think adding it to your Substrate Recipes would be really great, we have spent a lot of time trying to come up with answers to this internally ;)
shawntabrizi
Yeah, one last note here. Consider the block number in Substrate. Right now it is u32 in Substrate. Assuming a 6 second block time that is something like 800 years (someone check my math). So... what happens 800 years from now?
the answer is substrate today does not directly handle this issue. But we have runtime upgrades, and an extensible modular platform to address the issue when it comes time to

Same thoughts should be applied to your own runtime modules.

    make sure shit doesnt break
    if shit does break, make sure it does not provide a reasonable attack vector
    fix broken shit

bedeho / Joystream
Makes a lot of sense. As long as you don't panic, I guess one satisfied 2, we were just wondering whether we should always encode broken invaraints in our error types, or alternatively always just be silent. Now we have some guidance on how to use both.
Or I should say, def. do not panic to satisfy 2, but that may not be enough, as you pointed out.