Merge #392: Store in DB the time a Spend transaction draft was last updated

f262ca2d1ca864e37704ab2a35d9426f85a31c6c tests: reduce the number of workers for the executor (Antoine Poinsot)
104c6e1a093238045cad0c149f61f424ac0a9dc7 commands: add an 'updated_at' field to listspendtxs entries (Antoine Poinsot)
6b666e75c0fc306dc58574a48346af0bc958a153 db: unit test the migration from v0 to v1 (Antoine Poinsot)
29ae0a4a5e5e17ae0ca4951ed3b4e7043803c683 db: add a new 'updated_at' column to Spend transactions (Antoine Poinsot)

Pull request description:

  This is an alternative to #390.

  We add a new column in the `spend_transactions` SQLite table, and as such introduce a straightforward migration system. This field is in turn always set in `updatespendtx` when we store the new PSBT, and listed in `listspendtxs` entries. It is useful for instance to sort spend transactions on the GUI (#281).

ACKs for top commit:
  darosior:
    ACK f262ca2d1ca864e37704ab2a35d9426f85a31c6c -- i've tested it both manually and with #393, and Edouard too with the GUI

Tree-SHA512: 44d3309fcc83069125ab74a20a6eb2c7c662df30fe0da4b90e5f74ca57f26e76c604453bf040f1bf2718e123350d3c42b946755c1650a84ee1beb69913bff856

doc/API.md

@@ -174,6 +174,7 @@ This command does not take any parameter for now.
 | Field          | Type              | Description                                                             |
 | -------------- | ----------------- | ----------------------------------------------------------------------- |
 | `psbt`         | string            | Base64-encoded PSBT of the Spend transaction.                           |
+| `updated_at`   | int or null       | UNIX timestamp of the last time this PSBT was updated.                  |
 
 
 ### `delspendtx`

src/commands/mod.rs

@@ -563,7 +563,7 @@ impl DaemonControl {
         let spend_txs = db_conn
             .list_spend()
             .into_iter()
-            .map(|psbt| ListSpendEntry { psbt })
+            .map(|(psbt, updated_at)| ListSpendEntry { psbt, updated_at })
             .collect();
         ListSpendResult { spend_txs }
     }
@@ -840,6 +840,7 @@ pub struct CreateSpendResult {
 pub struct ListSpendEntry {
     #[serde(serialize_with = "ser_base64", deserialize_with = "deser_base64")]
     pub psbt: Psbt,
+    pub updated_at: Option<u32>,
 }
 
 #[derive(Debug, Clone, Serialize, Deserialize)]

src/database/mod.rs

@@ -112,8 +112,8 @@ pub trait DatabaseConnection {
     /// Insert a new Spend transaction or replace an existing one.
     fn store_spend(&mut self, psbt: &Psbt);
 
-    /// List all existing Spend transactions.
-    fn list_spend(&mut self) -> Vec<Psbt>;
+    /// List all existing Spend transactions, along with an optional last update timestamp.
+    fn list_spend(&mut self) -> Vec<(Psbt, Option<u32>)>;
 
     /// Delete a Spend transaction from database.
     fn delete_spend(&mut self, txid: &bitcoin::Txid);
@@ -241,10 +241,10 @@ impl DatabaseConnection for SqliteConn {
         self.store_spend(psbt)
     }
 
-    fn list_spend(&mut self) -> Vec<Psbt> {
+    fn list_spend(&mut self) -> Vec<(Psbt, Option<u32>)> {
         self.list_spend()
             .into_iter()
-            .map(|db_spend| db_spend.psbt)
+            .map(|db_spend| (db_spend.psbt, db_spend.updated_at))
             .collect()
     }
 

src/database/sqlite/schema.rs

@@ -74,7 +74,8 @@ CREATE TABLE addresses (
 CREATE TABLE spend_transactions (
     id INTEGER PRIMARY KEY NOT NULL,
     psbt BLOB UNIQUE NOT NULL,
-    txid BLOB UNIQUE NOT NULL
+    txid BLOB UNIQUE NOT NULL,
+    updated_at INTEGER
 );
 ";
 
@@ -253,6 +254,7 @@ pub struct DbSpendTransaction {
     pub id: i64,
     pub psbt: Psbt,
     pub txid: bitcoin::Txid,
+    pub updated_at: Option<u32>,
 }
 
 impl TryFrom<&rusqlite::Row<'_>> for DbSpendTransaction {
@@ -268,6 +270,13 @@ impl TryFrom<&rusqlite::Row<'_>> for DbSpendTransaction {
         let txid: bitcoin::Txid = encode::deserialize(&txid).expect("We only store valid txids");
         assert_eq!(txid, psbt.unsigned_tx.txid());
 
-        Ok(DbSpendTransaction { id, psbt, txid })
+        let updated_at = row.get(3)?;
+
+        Ok(DbSpendTransaction {
+            id,
+            psbt,
+            txid,
+            updated_at,
+        })
     }
 }

src/database/sqlite/utils.rs

@@ -1,4 +1,4 @@
-use crate::database::sqlite::{schema::SCHEMA, FreshDbOptions, SqliteDbError, DB_VERSION};
+use crate::database::sqlite::{FreshDbOptions, SqliteDbError, DB_VERSION};
 
 use std::{convert::TryInto, fs, path, time};
 
@@ -50,11 +50,14 @@ where
         .collect::<rusqlite::Result<Vec<T>>>()
 }
 
-// Sqlite supports up to i64, thus rusqlite prevents us from inserting u64's.
-// We use this to panic rather than inserting a truncated integer into the database (as we'd have
-// done by using `n as u32`).
-fn timestamp_to_u32(n: u64) -> u32 {
-    n.try_into()
+/// The current time as the number of seconds since the UNIX epoch, truncated to u32 since SQLite
+/// only supports i64 integers.
+pub fn curr_timestamp() -> u32 {
+    time::SystemTime::now()
+        .duration_since(time::UNIX_EPOCH)
+        .expect("System clock went backward the epoch?")
+        .as_secs()
+        .try_into()
         .expect("Is this the year 2106 yet? Misconfigured system clock.")
 }
 
@@ -79,6 +82,7 @@ pub fn create_db_file(db_path: &path::Path) -> Result<(), std::io::Error> {
     };
 }
 
+/// Create a fresh Liana database with the given schema.
 pub fn create_fresh_db(
     db_path: &path::Path,
     options: FreshDbOptions,
@@ -86,10 +90,7 @@ pub fn create_fresh_db(
 ) -> Result<(), SqliteDbError> {
     create_db_file(db_path)?;
 
-    let timestamp = time::SystemTime::now()
-        .duration_since(time::UNIX_EPOCH)
-        .map(|dur| timestamp_to_u32(dur.as_secs()))
-        .expect("System clock went backward the epoch?");
+    let timestamp = curr_timestamp();
 
     // Fill the initial addresses. On a fresh database, the deposit_derivation_index is
     // necessarily 0.
@@ -113,10 +114,10 @@ pub fn create_fresh_db(
 
     let mut conn = rusqlite::Connection::open(db_path)?;
     db_exec(&mut conn, |tx| {
-        tx.execute_batch(SCHEMA)?;
+        tx.execute_batch(options.schema)?;
         tx.execute(
             "INSERT INTO version (version) VALUES (?1)",
-            rusqlite::params![DB_VERSION],
+            rusqlite::params![options.version],
         )?;
         tx.execute(
             "INSERT INTO tip (network, blockheight, blockhash) VALUES (?1, NULL, NULL)",
@@ -134,3 +135,55 @@ pub fn create_fresh_db(
 
     Ok(())
 }
+
+pub fn db_version(conn: &mut rusqlite::Connection) -> Result<i64, SqliteDbError> {
+    Ok(db_query(
+        conn,
+        "SELECT version FROM version",
+        rusqlite::params![],
+        |row| {
+            let version: i64 = row.get(0)?;
+            Ok(version)
+        },
+    )?
+    .pop()
+    .expect("There is always a row in the version table"))
+}
+
+// In Liana 0.4 we upgraded the schema to hold a timestamp for transaction drafts. Existing
+// transaction drafts are not set any timestamp on purpose.
+fn migrate_v0_to_v1(conn: &mut rusqlite::Connection) -> Result<(), SqliteDbError> {
+    db_exec(conn, |tx| {
+        tx.execute(
+            "ALTER TABLE spend_transactions ADD COLUMN updated_at",
+            rusqlite::params![],
+        )?;
+        tx.execute("UPDATE version SET version = 1", rusqlite::params![])?;
+        Ok(())
+    })?;
+
+    Ok(())
+}
+
+/// Check the database version and if necessary apply the migrations to upgrade it to the current
+/// one.
+pub fn maybe_apply_migration(db_path: &path::Path) -> Result<(), SqliteDbError> {
+    let mut conn = rusqlite::Connection::open(db_path)?;
+
+    // Iteratively apply the database migrations necessary.
+    loop {
+        let version = db_version(&mut conn)?;
+        match version {
+            DB_VERSION => {
+                log::info!("Database is up to date.");
+                return Ok(());
+            }
+            0 => {
+                log::warn!("Upgrading database from version 0 to version 1.");
+                migrate_v0_to_v1(&mut conn)?;
+                log::warn!("Migration from database version 0 to version 1 successful.");
+            }
+            _ => return Err(SqliteDbError::UnsupportedVersion(version)),
+        }
+    }
+}

src/lib.rs

@@ -184,10 +184,10 @@ fn setup_sqlite(
         .iter()
         .collect();
     let options = if fresh_data_dir {
-        Some(FreshDbOptions {
-            bitcoind_network: config.bitcoin_config.network,
-            main_descriptor: config.main_descriptor.clone(),
-        })
+        Some(FreshDbOptions::new(
+            config.bitcoin_config.network,
+            config.main_descriptor.clone(),
+        ))
     } else {
         None
     };

src/testutils.rs

@@ -120,7 +120,7 @@ struct DummyDbState {
     change_index: bip32::ChildNumber,
     curr_tip: Option<BlockChainTip>,
     coins: HashMap<bitcoin::OutPoint, Coin>,
-    spend_txs: HashMap<bitcoin::Txid, Psbt>,
+    spend_txs: HashMap<bitcoin::Txid, (Psbt, Option<u32>)>,
 }
 
 pub struct DummyDatabase {
@@ -293,14 +293,20 @@ impl DatabaseConnection for DummyDatabase {
             .write()
             .unwrap()
             .spend_txs
-            .insert(txid, psbt.clone());
+            .insert(txid, (psbt.clone(), None));
     }
 
     fn spend_tx(&mut self, txid: &bitcoin::Txid) -> Option<Psbt> {
-        self.db.read().unwrap().spend_txs.get(txid).cloned()
+        self.db
+            .read()
+            .unwrap()
+            .spend_txs
+            .get(txid)
+            .cloned()
+            .map(|x| x.0)
     }
 
-    fn list_spend(&mut self) -> Vec<Psbt> {
+    fn list_spend(&mut self) -> Vec<(Psbt, Option<u32>)> {
         self.db
             .read()
             .unwrap()

tests/test_framework/utils.py

@@ -12,7 +12,7 @@ from io import BytesIO
 from .serializations import CTransaction, PSBT
 
 TIMEOUT = int(os.getenv("TIMEOUT", 20))
-EXECUTOR_WORKERS = int(os.getenv("EXECUTOR_WORKERS", 20))
+EXECUTOR_WORKERS = int(os.getenv("EXECUTOR_WORKERS", 5))
 VERBOSE = os.getenv("VERBOSE", "0") == "1"
 LOG_LEVEL = os.getenv("LOG_LEVEL", "debug")
 assert LOG_LEVEL in ["trace", "debug", "info", "warn", "error"]

tests/test_rpc.py

@@ -159,6 +159,7 @@ def test_list_spend(lianad, bitcoind):
     assert "psbt" in res_b
 
     # Store them both in DB.
+    time_before_update = int(time.time())
     assert len(lianad.rpc.listspendtxs()["spend_txs"]) == 0
     lianad.rpc.updatespend(res["psbt"])
     lianad.rpc.updatespend(res_b["psbt"])
@@ -168,7 +169,9 @@ def test_list_spend(lianad, bitcoind):
     list_res = lianad.rpc.listspendtxs()["spend_txs"]
     assert len(list_res) == 2
     first_psbt = next(entry for entry in list_res if entry["psbt"] == res["psbt"])
+    assert time_before_update <= first_psbt["updated_at"] <= int(time.time())
     second_psbt = next(entry for entry in list_res if entry["psbt"] == res_b["psbt"])
+    assert time_before_update <= second_psbt["updated_at"] <= int(time.time())
 
     # If we delete the first one, we'll get only the second one.
     first_psbt = PSBT.from_base64(res["psbt"])