cycles-wallet
cycles-wallet copied to clipboard
Do not rely on the pre_upgrade hook
The wallet canisters stores valuable data, so we need to make sure it is upgradable without loosing data.
This means that pre_upgrade cannot be used, because:
- It could OOM (unlikely, but the possibility exists)
- It could trap, in which case there would be no way to upgrade. There is at least one explicit unwrap in the hook at https://github.com/dfinity/wallet-rs/blob/121162b9f1a21d3c450aaec30796b7fbed15e967/wallet/src/lib.rs#L47
The solution, for critical canisters, before we have a nice stable memory API, is simply to move this code to be ran at the end of each update method. This way, if any change in (persistent but not stable) state causes some failure in the persistence in stable memory, then that call is immediately rejected.
In addition, there's evidence that pre-upgrade has not been tested very thoroughly: https://dfinity.slack.com/archives/C014CQ9N2G5/p1602056869013100
@luc-mercier this is still the case right? Has there been any new conclusion not captured here: https://docs.google.com/document/d/13GCGj5SaUe7JhgiMmiW7GSIzejtQ4c1uohJFY89tobc/edit# (is pre_upgrade still banned in NNS and should it be banned here too)
Yes, nothing has changed, and this is still one of the issue that scares me the most wrt to the health of the IC.