minibits-cash / minibits_wallet

MIT License
74 stars 7 forks source link

Closing (or crash) during lightning send causes lose of ecash! #28

Closed mariaa144 closed 9 months ago

mariaa144 commented 11 months ago

If Minibits is closed or crashes during the lightning send process then ecash is likely to be lost (or at least unavailable to the user).

To reproduce:

  1. Initiate a lightning send
  2. As soon as Minibits is waiting in the mint for the send close Minibits
  3. Reload Minibits and notice the transaction is in DRAFT status
  4. Go to remove old proofs and notice a proof will be removed
  5. Notice the balance is lower and the lightning transaction was not completed!
  6. Further lightning transactions give an error. See screenshots.

I would have expected an error in the wallet transaction but at no point should there have been a loss of funds even if the app crashes during a lightning send.

Screenshots of errors:

Screenshot_20231217-074757 Screenshot_20231217-074707 Screenshot_20231217-074620

I noticed a related issue in the eNuts wallet. https://github.com/cashubtc/eNuts/issues/286

minibits-cash commented 11 months ago

Thanks for the report, very appreciated that we start to move on to negative test cases.

There seems to be a combination of 2 issues reported here:

  1. Handling of sudden failures on wallet side during the transaction (phone switch off, crash, app closure by user). I need to review / go step by step your usecase if I can make it more resilient. Existing audit trail and separate append-only storage of all proofs were designed from the beginning in that direcrion.

  2. Fact that mint logged the proofs as spent in spite of the fact that the payment failed is likely a bug in the mint side - something similar happened during recent development and there is a fix in the making for Nutshell.

  3. You should be able to unbrick the wallet by going to Settings > Backup and recovery > Increase recovery index + Remove spent ecash

Will write here further updates on mint fix and wallet improvements.

mariaa144 commented 11 months ago

I was able to unbrick my wallet doing what you suggested and moving some ecash to myself.

I am using an older mint from July this year. It is the latest Voltage provides. However, at least on eNuts I was able to reproduce the issue on a more recent mint as well. I didn't try reproducing the issue using Minibits on an updated mint.

minibits-cash commented 11 months ago

Good to hear wallet is back, I released that recovery path only yesterday.

I will post here the status of the related mint fix after discussing the status with @callebtc . It's been for sure not yet released.

KKA11010 commented 11 months ago

Thanks for the report, very appreciated that we start to move on to negative test cases.

There seems to be a combination of 2 issues reported here:

1. Handling of sudden failures on wallet side during the transaction (phone switch off, crash, app closure by user). I need to review / go step by step your usecase if I can make it more resilient. Existing audit trail and separate append-only storage of all proofs were designed from the beginning in that direcrion.

2. Fact that mint logged the proofs as spent in spite of the fact that the payment failed is likely a bug in the mint side - something similar happened during recent development and there is a fix in the making for Nutshell.

3. You should be able to unbrick the wallet by going to Settings > Backup and recovery > Increase recovery index + Remove spent ecash

Will write here further updates on mint fix and wallet improvements.

@misovan regarding your second point, I am also looking forward to hear what the issue is on the mint side!

minibits-cash commented 11 months ago

Shortly, if the wallet provides blinded message with blinding factor "B_" for split, that the mint already got in the past, the mint spends the related ecash before it fails to provide a blinded signature back to the wallet.

This situation can happen on wallet side for various reasons after implementing the deterministic secrets which uses incremental recovery indexes to derive secrets. In case wallet does not increment the index, the blinded message is generated using secret generated using th same derivation path as before.

Wallet may fail to increment the index because of a bug (I've discovered this during development of backup and recovery) or, as you see above, because of external failures, that can not be fully prevented.

That's why the mint needs to check there is not existing blinded signature generated from blinded message with the same blinding factor before it spends and not let it fail too late on low-level DB integrity constraint as above.

minibits-cash commented 11 months ago

I believe I have a working concept how to recover lost funds coming from unexpected network failures or crashes that can not be fully avoided on mobile devices. Please give me some more time to test it and discuss it with protocol devs, as there is more than 1 possible design.

minibits-cash commented 11 months ago

Fix released for testing in v0.1.5-beta.20. Wallet proof indexes are now incremented before making the request to the mint. Index interval of expected proofs to be recieved is persisted during the request (as "in-flight"), deleted on successful response. If response had not been received, new "checkInFlight" method takes the missing indexes and runs recovery of related proofs with the mint and removes original (already spent) proofs from the wallet.

There is still pending fix on mint side, tracked as #381 on nutshell.

mariaa144 commented 11 months ago

The Nutshell issue you referenced is here: https://github.com/cashubtc/nutshell/issues/381

minibits-cash commented 9 months ago

Known room for occurrence of this bug has been eliminated in recent v0.1.6-beta release. At the same time, related mint fix released in 0.4.2 version of Nutshell extension for LNBits should prevent the spend/loss of ecash in case the issue would appear in some not covered situation again (potential known case is recovery from seed without completing the ecash recovery up to the end).