Skip to content

Fix sqlite database set_utxo to insert or update utxos#605

Merged
notmandatory merged 4 commits intobitcoindevkit:masterfrom
notmandatory:fix/sqlite_upsert_utxo
May 24, 2022
Merged

Fix sqlite database set_utxo to insert or update utxos#605
notmandatory merged 4 commits intobitcoindevkit:masterfrom
notmandatory:fix/sqlite_upsert_utxo

Conversation

@notmandatory
Copy link
Copy Markdown
Member

@notmandatory notmandatory commented May 12, 2022

Description

This PR fixes #591 by:

  1. Add sqlite MIGRATIONS statements to remove duplicate utxos and add unique utxos index on txid and vout.
  2. Do an upsert (if insert fails update) instead of an insert in set_utxo().
  3. Update database::test::test_utxo to also verify set_utxo() doesn't insert duplicate utxos.

Notes to the reviewers

I verified the updated test_utxo fails as expected before my fix and passes after the fix. I tested the new migrations using the below bdk-cli command and a manually updated sqlite db with duplicate utxos.

cargo run --no-default-features --features cli,sqlite-db,esplora-ureq -- wallet -w test1 --descriptor "wpkh(tpubEBr4i6yk5nf5DAaJpsi9N2pPYBeJ7fZ5Z9rmN4977iYLCGco1VyjB9tvvuvYtfZzjD5A8igzgw3HeWeeKFmanHYqksqZXYXGsw5zjnj7KM9/*)" sync

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature
  • I've updated CHANGELOG.md

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@notmandatory notmandatory self-assigned this May 12, 2022
@notmandatory notmandatory force-pushed the fix/sqlite_upsert_utxo branch from b41f24e to b8954a2 Compare May 12, 2022 03:35
@notmandatory notmandatory marked this pull request as ready for review May 12, 2022 03:38
@notmandatory notmandatory force-pushed the fix/sqlite_upsert_utxo branch from b8954a2 to 1281ffd Compare May 12, 2022 04:40
Comment thread src/database/sqlite.rs Outdated
"INSERT INTO transaction_details SELECT txid, timestamp, received, sent, fee, height FROM transaction_details_old;",
"DROP TABLE transaction_details_old;",
"ALTER TABLE utxos ADD COLUMN is_spent;",
"DELETE FROM utxos WHERE rowid > (SELECT MIN(rowid) FROM utxos u2 WHERE utxos.txid = u2.txid AND utxos.vout = u2.vout);",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I discussed a bit with @afilini about this, while theoretically the duplicated lines should contain the same info, we can't be so sure about that...
If the duplicated lines contain different info (for example: same outpoint, different is_spent), we might end up in an inconsistent state.
Alekos suggested wiping the entire db.
I suggested wiping it only if we find some duplicated outpoints with the different info - if this can be done in a single SQL query (should be doable, but I'm not entirely sure). What do you think?

Copy link
Copy Markdown
Member Author

@notmandatory notmandatory May 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I'm so proud of my little SQL de-duplicating! haha, anyway, rather than risk an inconsistent state I'll update this to always wipe the database, it's only a cache after all. And I have to agree with @afilini to wipe the data even if there are no dups since either way we will need to warn users that it MIGHT be wiped on this release and they'll need to be prepared to handle it with a sync() (and any reasonable app will do a sync at startup).

Comment thread src/database/sqlite.rs Outdated
@notmandatory notmandatory force-pushed the fix/sqlite_upsert_utxo branch from 1281ffd to 154b10b Compare May 18, 2022 20:36
@notmandatory
Copy link
Copy Markdown
Member Author

notmandatory commented May 18, 2022

I force-pushed 154b10b to DELETE all data from tables (except for script version table). Tested locally with bdk-cli and verified MIGRATIONS scripts all run and able to re-sync after. I also added a warning in the CHANGELOG that anyone using the sqlite-db feature will need to do a wallet.sync().

@notmandatory notmandatory force-pushed the fix/sqlite_upsert_utxo branch from 47936d4 to 4023a7d Compare May 18, 2022 20:46
@notmandatory notmandatory force-pushed the fix/sqlite_upsert_utxo branch from 4023a7d to 2471908 Compare May 19, 2022 20:21
@notmandatory
Copy link
Copy Markdown
Member Author

This is now ready to go, I also added a fix for our blockchain integration tests in CI that haven't been running.

Once this PR is merge I'm going to cherry pick it into a 0.18.1 fix release since @thunderbiscuit's SoB mentee found that our latest bdk-kotlin (and also I'll guess bdk-swift) release is broken, duplicating balances, because they use the SQLite DB.

@notmandatory notmandatory modified the milestones: Release 0.19.0 Feature Freeze, 0.18.1 May 20, 2022
Copy link
Copy Markdown
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 35feb10 - Code looks good, but I didn't do any local test to see if the db gets wiped

@notmandatory notmandatory modified the milestones: Release 0.18.1, Release 0.19.0 Feature Freeze May 24, 2022
@notmandatory
Copy link
Copy Markdown
Member Author

Thanks for the utACK @danielabrozzoni, I'm going to merge now, should be good based on my manually testing, as discussed on the call this is what I did with the unreleased master branch version of bdk-cli:

  1. Sync'd with dependency bdk 0.18.0 and new sqlite-db feature enabled:
cargo run --no-default-features --features cli,electrum,sqlite-db -- wallet --descriptor "wpkh(tpubEBr4i6yk5nf5DAaJpsi9N2pPYBeJ7fZ5Z9rmN4977iYLCGco1VyjB9tvvuvYtfZzjD5A8igzgw3HeWeeKFmanHYqksqZXYXGsw5zjnj7KM9/*)" sync
  1. Manually checked sqlite DB file tables, verified old schema is there with utxo data, I used database tool in AndroidStudio but can also use sqlite cli tool
  2. With same build of bdk-cli got wallet balance, sync'd wallet again and checked balance again, show's double utxos
  3. Changed bdk-cli Cargo.toml dependency for bdk to this PR branch version
  4. Re-sync'd with bdk-cli and verified correct balance
  5. Manually checked sqlite DB tables again, verified new schema is there with new utxo unique constraints and no duplicates

@notmandatory notmandatory merged commit fbd98b4 into bitcoindevkit:master May 24, 2022
@danielabrozzoni
Copy link
Copy Markdown
Member

Post-merge ACK 35feb10

@notmandatory notmandatory deleted the fix/sqlite_upsert_utxo branch October 24, 2022 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SqliteDatabase set_utxo always inserts new utxo

2 participants