Add datatype for is_spent sqlite column

[vladimirfomene]

Description

During table creation, Sqlite does not throw an error when a column datatype is not defined. In addition, the datatype provided during table creation does not put a constraint on the type of data that can be put in that column. So you can easily put a string value in an integer column. Despite this, I think it is important for us to add the datatype for clarity.

Notes to the reviewers

You can read more about how Sqlite dynamic typing here. I have amended our migrate code with a new commit. The idea is to run migrations in a transaction so that they either succeed or fail. This prevents us from having the database in an inconsistent state at any point in time.

Checklists

All Submissions:

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

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

[danielabrozzoni]

tACK 0c9dabe - I created an example on master creating a wallet with sqlite, I manually checked theutxos table with the sqlite3 command, and is_spent didn't have a type. I then switched to this branch, ran the example again, inspected the table and found the same exact data, but the is_spent column's type was correct.

In case someone else wants to manually try this out, this is the code I used:

use bdk::blockchain::ElectrumBlockchain;
use bdk::database::SqliteDatabase;
use bdk::electrum_client::Client;
use bdk::{SyncOptions, Wallet};

fn main() -> Result<(), bdk::Error> {
    let client = Client::new("ssl://electrum.blockstream.info:60002")?;
    let blockchain = ElectrumBlockchain::from(client);
    let wallet = Wallet::new(
        "wpkh([c258d2e4/84h/1h/0h]tpubDDYkZojQFQjht8Tm4jsS3iuEmKjTiEGjG6KnuFNKKJb5A6ZUCUZKdvLdSDWofKi4ToRCwb9poe1XdqfUnP4jaJjCB2Zwv11ZLgSbnZSNecE/0/*)",
        Some("wpkh([c258d2e4/84h/1h/0h]tpubDDYkZojQFQjht8Tm4jsS3iuEmKjTiEGjG6KnuFNKKJb5A6ZUCUZKdvLdSDWofKi4ToRCwb9poe1XdqfUnP4jaJjCB2Zwv11ZLgSbnZSNecE/1/*)"),
        bitcoin::Network::Testnet,
        SqliteDatabase::new("/tmp/bdk.sqlite"),
    )?;

    wallet.sync(&blockchain, SyncOptions::default())?;

    println!("Descriptor balance: {} SAT", wallet.get_balance()?);

    Ok(())
}

[notmandatory]

Hey I forgot to discuss this one in our call today. Since it needs a little more work I'm moving it to the 0.23 milestone.

[danielabrozzoni]

Since it needs a little more work I'm moving it to the 0.23 milestone.

Well if Vlad can update this one in the next ~10 hours, we can squeeze it in, otherwise no problem with waiting for the next release :)

[vladimirfomene]

@afilini @danielabrozzoni I have added a new commit to our migration code which runs the migration statement in a transaction in such a way that they either succeed or fail. Let me know what you think about it.

[vladimirfomene]

@notmandatory, I have updated the change as recommended.

[rajarshimaitra]

tACK cd3e33e

Verified that is_spent datatype indeed changes and extra migration is added over an existing database successfully..

This is good to go.. Just one non blocking observation.

[rajarshimaitra]

If the intent of this log is to show migration steps with index number, currently its showing below for a db upgrade..

executing db migration 0: `ALTER TABLE utxos RENAME TO utxos_old;`
executing db migration 1: `CREATE TABLE utxos (value INTEGER, keychain TEXT, vout INTEGER, txid BLOB, script BLOB, is_spent BOOLEAN DEFAULT 0);`
executing db migration 2: `INSERT INTO utxos SELECT value, keychain, vout, txid, script, is_spent FROM utxos_old;`
executing db migration 3: `DROP TABLE utxos_old;`
executing db migration 4: `CREATE UNIQUE INDEX idx_utxos_txid_vout ON utxos(txid, vout);`

The indexing starts at 0 for a migration operation.
We might wanna index the migration with the actual line number of the full migration array? In that way it will be easier to debug later at which migration step problem occurred..

[vladimirfomene]

Great idea! I will implement it.

[vladimirfomene]

Done

[rajarshimaitra]

tACK 54d7684

Verified that the log is matching the migration statement index. Db schema version is being set correctly after migration..

[afilini]

ACK 54d7684