cycles-wallet icon indicating copy to clipboard operation
cycles-wallet copied to clipboard

Do not rely on the pre_upgrade hook

Open luc-mercier opened this issue 5 years ago • 3 comments

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.

luc-mercier avatar Oct 06 '20 16:10 luc-mercier

In addition, there's evidence that pre-upgrade has not been tested very thoroughly: https://dfinity.slack.com/archives/C014CQ9N2G5/p1602056869013100

luc-mercier avatar Oct 07 '20 07:10 luc-mercier

@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)

p-shahi avatar Feb 10 '21 02:02 p-shahi

Yes, nothing has changed, and this is still one of the issue that scares me the most wrt to the health of the IC.

luc-mercier avatar Feb 10 '21 10:02 luc-mercier