search

bitcoindevkit / bdk-ffi

Please consider this project *experimental*. But we hope to have an official release out soon.
Other
88 stars 39 forks source link

Database is getting corrupted on Android #114

Closed thunderbiscuit closed 2 years ago

thunderbiscuit commented 2 years ago

I assume this is related to the flush() method Ricardo implemented a little while back.

I cannot seem to reproduce the problem consistently which doesn't help, but the gist of it is that at some point the sats sent out are not recognized as having been spent, and no amount of sync can fix the issue. The only way to fix is to delete and reload/resync the wallet from scratch.

I think we should try to flush every time we sync, but I haven't been able to find examples of that, and my attempts currently don't compile.

thunderbiscuit commented 2 years ago

I am now able to reproduce the error consistently. I can get the bug to "work" on my device and in the emulator. Here are two ways I can do it. The second one (Test 2) is really the purest form I am able to make it so far.

Test 1

  1. Receive 7000 sats and sync until the wallet sees the funds
  2. Send 1000 sats
  3. Close app without syncing
  4. Open app without sync (balance says 0)
  5. Try to send 5000 -> Error (wallet hasn't synced yet, not enough funds)
  6. Sync (correct amount is displayed: 5856)
  7. Close the app
  8. Open the app and sync (wrong balance!)

12856 (the actual 5856 plus our initial 7000)

Test 2

  1. Receive 7000 (sync)
  2. Send 1000
  3. Sync and close the app (correct balance)
  4. Open app without sync
  5. Try to send 5000 -> Error ((Electrum(Protocol(String("sendrawtransaction RPC error: {\"code\":-25,\"message\":\"bad-txns-inputs-missingorspent\"}")))))
  6. Sync (wrong balance!) (again the balance is 12856, which is the true balance plus the initial utxo of 7000)
thunderbiscuit commented 2 years ago

Tried to reproduce on the cli but I don't get the same behaviour, and the balance shows correctly.

thunderbiscuit commented 2 years ago

Potential bug is on step 3 of test 2 above.

If the sync function of step 3 is executed (correct balance displayed) but not flushed to disk at that time, it would make sense that when we kill the app abruptly the db doesn't know about the tx yet. In step 5, an attempt to build a new psbt but without any sync makes the library use the only utxo it knows about, the initial received 7000 sats. Somehow when that fails to broadcast then that's where it's unclear to me what happens but in any case the new balance (the change from the transaction in step 2) and the original utxo are added together and you get a total balance of 12856 sats.

notmandatory commented 2 years ago

This is great to have steps to reproduce. I'm pretty sure the DB is only updated during sync, so this supports your theory that the sync and immediately closing the app is the problem. If you always sync before trying to create a spending TX does that fix the issue? It's only a workaround, but would help confirm. Probably next step is try try to flush the DB after sync and see if that does what we need it to, though I suspect there's always a window where if you close fast enough the sync won't be committed to the DB.

thunderbiscuit commented 2 years ago

If you always sync before trying to create a spending TX does that fix the issue?

Thanks for the suggestion, I just assumed it would but in fact we've narrowed down our bug! The balance is wrong right on second startup without even trying to generate a new transaction. So the newest version of the steps to reproduce the bug are:

Test 3

  1. Receive 7000 (sync)
  2. Send 1000
  3. Sync (displays correct balance)
  4. Kill the app
  5. Sync (wrong balance!)

I think this supports the idea that the required flush after sync is our issue.

afilini commented 2 years ago

It looks like most additions to the DB are flushed correctly, but deletions are not: all the issues you've described involve spending a UTXO (which a later sync removes from the DB) and then reloading the app.

A quick interesting test you could do is:

  1. Receive 7000 (sync)
  2. Send 1000 (do not sync)
  3. Receive 5000
  4. Sync (here your wallet should see a deleted UTXO plus a new UTXO)
  5. Kill the app
  6. Sync

If this ends up working, I think this issue might have already been (implicitly) fixed by https://github.com/bitcoindevkit/bdk/pull/515 because with that PR instead of removing spent UTXOs we issue updates to mark them as spent.

thunderbiscuit commented 2 years ago

Thanks for the idea! I didn't know about PR #515.

Unfortunately using the workflow/test you proposed didn't solve the issue (does that help narrowing down where the problem might be?). Balance is still over by the extra amount of the first utxo (7000) after steps 4-5-6, even though the correct balance is displayed on sync in step 4. But you're right it appears to be a problem of deleting spent utxos, or recognizing them as spent at least.

Would the issue of confirmation/pending be a part of this? I could try to test to sync/kill app only after the transaction is confirmed.

notmandatory commented 2 years ago

This is fixed by #116 and using sqlite DB instead of Sled. See also https://github.com/bitcoindevkit/bdk-kotlin/pull/19