Never delete spent utxos from the database

A `is_spent` field is added to LocalUtxo; when a txo is spent we set
this field to true instead of deleting the entire utxo from the
database.
This allows us to create txs double-spending txs already in blockchain.
Listunspent won't return spent utxos, effectively excluding them from the
coin selection and balance calculation

CHANGELOG.md

@@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - `verify` flag removed from `TransactionDetails`.
 - Add `get_internal_address` to allow you to get internal addresses just as you get external addresses.
 - added `ensure_addresses_cached` to `Wallet` to let offline wallets load and cache addresses in their database
+- Add `is_spent` field to `LocalUtxo`; when we notice that a utxo has been spent we set `is_spent` field to true instead of deleting it from the db.
 
 ### Sync API change
 

src/blockchain/compact_filters/mod.rs

@@ -163,11 +163,19 @@ impl CompactFiltersBlockchain {
             if let Some(previous_output) = database.get_previous_output(&input.previous_output)? {
                 inputs_sum += previous_output.value;
 
-                if database.is_mine(&previous_output.script_pubkey)? {
+                // this output is ours, we have a path to derive it
+                if let Some((keychain, _)) =
+                    database.get_path_from_script_pubkey(&previous_output.script_pubkey)?
+                {
                     outgoing += previous_output.value;
 
-                    debug!("{} input #{} is mine, removing from utxo", tx.txid(), i);
+                    debug!("{} input #{} is mine, setting utxo as spent", tx.txid(), i);
-                    updates.del_utxo(&input.previous_output)?;
+                    updates.set_utxo(&LocalUtxo {
+                        outpoint: input.previous_output,
+                        txout: previous_output.clone(),
+                        keychain,
+                        is_spent: true,
+                    })?;
                 }
             }
         }
@@ -185,6 +193,7 @@ impl CompactFiltersBlockchain {
                     outpoint: OutPoint::new(tx.txid(), i as u32),
                     txout: output.clone(),
                     keychain,
+                    is_spent: false,
                 })?;
                 incoming += output.value;
 

src/blockchain/rpc.rs

@@ -249,7 +249,7 @@ impl WalletSync for RpcBlockchain {
         let mut list_txs_ids = HashSet::new();
 
         for tx_result in list_txs.iter().filter(|t| {
-            // list_txs returns all conflicting tx we want to
+            // list_txs returns all conflicting txs, we want to
             // filter out replaced tx => unconfirmed and not in the mempool
             t.info.confirmations > 0 || self.client.get_mempool_entry(&t.info.txid).is_ok()
         }) {
@@ -332,20 +332,23 @@ impl WalletSync for RpcBlockchain {
                             value: u.amount.as_sat(),
                             script_pubkey: u.script_pub_key,
                         },
+                        is_spent: false,
                     })),
                 },
             )
             .collect::<Result<HashSet<_>, Error>>()?;
 
         let spent: HashSet<_> = known_utxos.difference(&current_utxos).collect();
-        for s in spent {
+        for utxo in spent {
-            debug!("removing utxo: {:?}", s);
+            debug!("setting as spent utxo: {:?}", utxo);
-            db.del_utxo(&s.outpoint)?;
+            let mut spent_utxo = utxo.clone();
+            spent_utxo.is_spent = true;
+            db.set_utxo(&spent_utxo)?;
         }
         let received: HashSet<_> = current_utxos.difference(&known_utxos).collect();
-        for s in received {
+        for utxo in received {
-            debug!("adding utxo: {:?}", s);
+            debug!("adding utxo: {:?}", utxo);
-            db.set_utxo(s)?;
+            db.set_utxo(utxo)?;
         }
 
         for (keykind, index) in indexes {

src/blockchain/script_sync.rs

@@ -332,7 +332,23 @@ impl<'a, D: BatchDatabase> State<'a, D> {
             batch.del_tx(txid, true)?;
         }
 
-        // Set every tx we observed
+        let mut spent_utxos = HashSet::new();
+
+        // track all the spent utxos
+        for finished_tx in &finished_txs {
+            let tx = finished_tx
+                .transaction
+                .as_ref()
+                .expect("transaction will always be present here");
+            for input in &tx.input {
+                spent_utxos.insert(&input.previous_output);
+            }
+        }
+
+        // set every utxo we observed, unless it's already spent
+        // we don't do this in the loop above as we want to know all the spent outputs before
+        // adding the non-spent to the batch in case there are new tranasactions
+        // that spend form each other.
         for finished_tx in &finished_txs {
             let tx = finished_tx
                 .transaction
@@ -343,30 +359,22 @@ impl<'a, D: BatchDatabase> State<'a, D> {
                     self.db.get_path_from_script_pubkey(&output.script_pubkey)?
                 {
                     // add utxos we own from the new transactions we've seen.
-                    batch.set_utxo(&LocalUtxo {
+                    let outpoint = OutPoint {
-                        outpoint: OutPoint {
                         txid: finished_tx.txid,
                         vout: i as u32,
-                        },
+                    };
+
+                    batch.set_utxo(&LocalUtxo {
+                        outpoint,
                         txout: output.clone(),
                         keychain,
+                        // Is this UTXO in the spent_utxos set?
+                        is_spent: spent_utxos.get(&outpoint).is_some(),
                     })?;
                 }
             }
-            batch.set_tx(finished_tx)?;
-        }
 
-        // we don't do this in the loop above since we may want to delete some of the utxos we
+            batch.set_tx(finished_tx)?;
-        // just added in case there are new tranasactions that spend form each other.
-        for finished_tx in &finished_txs {
-            let tx = finished_tx
-                .transaction
-                .as_ref()
-                .expect("transaction will always be present here");
-            for input in &tx.input {
-                // Delete any spent utxos
-                batch.del_utxo(&input.previous_output)?;
-            }
         }
 
         for (keychain, last_active_index) in self.last_active_index {

src/database/keyvalue.rs

@@ -43,6 +43,7 @@ macro_rules! impl_batch_operations {
             let value = json!({
                 "t": utxo.txout,
                 "i": utxo.keychain,
+                "s": utxo.is_spent,
             });
             self.insert(key, serde_json::to_vec(&value)?)$($after_insert)*;
 
@@ -125,8 +126,9 @@ macro_rules! impl_batch_operations {
                     let mut val: serde_json::Value = serde_json::from_slice(&b)?;
                     let txout = serde_json::from_value(val["t"].take())?;
                     let keychain = serde_json::from_value(val["i"].take())?;
+                    let is_spent = val.get_mut("s").and_then(|s| s.take().as_bool()).unwrap_or(false);
 
-                    Ok(Some(LocalUtxo { outpoint: outpoint.clone(), txout, keychain }))
+                    Ok(Some(LocalUtxo { outpoint: outpoint.clone(), txout, keychain, is_spent, }))
                 }
             }
         }
@@ -246,11 +248,16 @@ impl Database for Tree {
                 let mut val: serde_json::Value = serde_json::from_slice(&v)?;
                 let txout = serde_json::from_value(val["t"].take())?;
                 let keychain = serde_json::from_value(val["i"].take())?;
+                let is_spent = val
+                    .get_mut("s")
+                    .and_then(|s| s.take().as_bool())
+                    .unwrap_or(false);
 
                 Ok(LocalUtxo {
                     outpoint,
                     txout,
                     keychain,
+                    is_spent,
                 })
             })
             .collect()
@@ -314,11 +321,16 @@ impl Database for Tree {
                 let mut val: serde_json::Value = serde_json::from_slice(&b)?;
                 let txout = serde_json::from_value(val["t"].take())?;
                 let keychain = serde_json::from_value(val["i"].take())?;
+                let is_spent = val
+                    .get_mut("s")
+                    .and_then(|s| s.take().as_bool())
+                    .unwrap_or(false);
 
                 Ok(LocalUtxo {
                     outpoint: *outpoint,
                     txout,
                     keychain,
+                    is_spent,
                 })
             })
             .transpose()

src/database/memory.rs

@@ -150,8 +150,10 @@ impl BatchOperations for MemoryDatabase {
 
     fn set_utxo(&mut self, utxo: &LocalUtxo) -> Result<(), Error> {
         let key = MapKey::Utxo(Some(&utxo.outpoint)).as_map_key();
-        self.map
+        self.map.insert(
-            .insert(key, Box::new((utxo.txout.clone(), utxo.keychain)));
+            key,
+            Box::new((utxo.txout.clone(), utxo.keychain, utxo.is_spent)),
+        );
 
         Ok(())
     }
@@ -228,11 +230,12 @@ impl BatchOperations for MemoryDatabase {
         match res {
             None => Ok(None),
             Some(b) => {
-                let (txout, keychain) = b.downcast_ref().cloned().unwrap();
+                let (txout, keychain, is_spent) = b.downcast_ref().cloned().unwrap();
                 Ok(Some(LocalUtxo {
                     outpoint: *outpoint,
                     txout,
                     keychain,
+                    is_spent,
                 }))
             }
         }
@@ -326,11 +329,12 @@ impl Database for MemoryDatabase {
             .range::<Vec<u8>, _>((Included(&key), Excluded(&after(&key))))
             .map(|(k, v)| {
                 let outpoint = deserialize(&k[1..]).unwrap();
-                let (txout, keychain) = v.downcast_ref().cloned().unwrap();
+                let (txout, keychain, is_spent) = v.downcast_ref().cloned().unwrap();
                 Ok(LocalUtxo {
                     outpoint,
                     txout,
                     keychain,
+                    is_spent,
                 })
             })
             .collect()
@@ -389,11 +393,12 @@ impl Database for MemoryDatabase {
     fn get_utxo(&self, outpoint: &OutPoint) -> Result<Option<LocalUtxo>, Error> {
         let key = MapKey::Utxo(Some(outpoint)).as_map_key();
         Ok(self.map.get(&key).map(|b| {
-            let (txout, keychain) = b.downcast_ref().cloned().unwrap();
+            let (txout, keychain, is_spent) = b.downcast_ref().cloned().unwrap();
             LocalUtxo {
                 outpoint: *outpoint,
                 txout,
                 keychain,
+                is_spent,
             }
         }))
     }
@@ -526,6 +531,7 @@ macro_rules! populate_test_db {
                     vout: vout as u32,
                 },
                 keychain: $crate::KeychainKind::External,
+                is_spent: false,
             })
             .unwrap();
         }

src/database/mod.rs

@@ -316,6 +316,7 @@ pub mod test {
             txout,
             outpoint,
             keychain: KeychainKind::External,
+            is_spent: true,
         };
 
         tree.set_utxo(&utxo).unwrap();

src/database/sqlite.rs

@@ -40,6 +40,7 @@ static MIGRATIONS: &[&str] = &[
     "CREATE TABLE transaction_details (txid BLOB, timestamp INTEGER, received INTEGER, sent INTEGER, fee INTEGER, height INTEGER);",
     "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;",
 ];
 
 /// Sqlite database stored on filesystem
@@ -83,14 +84,16 @@ impl SqliteDatabase {
         vout: u32,
         txid: &[u8],
         script: &[u8],
+        is_spent: bool,
     ) -> Result<i64, Error> {
-        let mut statement = self.connection.prepare_cached("INSERT INTO utxos (value, keychain, vout, txid, script) VALUES (:value, :keychain, :vout, :txid, :script)")?;
+        let mut statement = self.connection.prepare_cached("INSERT INTO utxos (value, keychain, vout, txid, script, is_spent) VALUES (:value, :keychain, :vout, :txid, :script, :is_spent)")?;
         statement.execute(named_params! {
             ":value": value,
             ":keychain": keychain,
             ":vout": vout,
             ":txid": txid,
-            ":script": script
+            ":script": script,
+            ":is_spent": is_spent,
         })?;
 
         Ok(self.connection.last_insert_rowid())
@@ -291,7 +294,7 @@ impl SqliteDatabase {
     fn select_utxos(&self) -> Result<Vec<LocalUtxo>, Error> {
         let mut statement = self
             .connection
-            .prepare_cached("SELECT value, keychain, vout, txid, script FROM utxos")?;
+            .prepare_cached("SELECT value, keychain, vout, txid, script, is_spent FROM utxos")?;
         let mut utxos: Vec<LocalUtxo> = vec![];
         let mut rows = statement.query([])?;
         while let Some(row) = rows.next()? {
@@ -300,6 +303,7 @@ impl SqliteDatabase {
             let vout = row.get(2)?;
             let txid: Vec<u8> = row.get(3)?;
             let script: Vec<u8> = row.get(4)?;
+            let is_spent: bool = row.get(5)?;
 
             let keychain: KeychainKind = serde_json::from_str(&keychain)?;
 
@@ -310,19 +314,16 @@ impl SqliteDatabase {
                     script_pubkey: script.into(),
                 },
                 keychain,
+                is_spent,
             })
         }
 
         Ok(utxos)
     }
 
-    fn select_utxo_by_outpoint(
+    fn select_utxo_by_outpoint(&self, txid: &[u8], vout: u32) -> Result<Option<LocalUtxo>, Error> {
-        &self,
-        txid: &[u8],
-        vout: u32,
-    ) -> Result<Option<(u64, KeychainKind, Script)>, Error> {
         let mut statement = self.connection.prepare_cached(
-            "SELECT value, keychain, script FROM utxos WHERE txid=:txid AND vout=:vout",
+            "SELECT value, keychain, script, is_spent FROM utxos WHERE txid=:txid AND vout=:vout",
         )?;
         let mut rows = statement.query(named_params! {":txid": txid,":vout": vout})?;
         match rows.next()? {
@@ -331,9 +332,18 @@ impl SqliteDatabase {
                 let keychain: String = row.get(1)?;
                 let keychain: KeychainKind = serde_json::from_str(&keychain)?;
                 let script: Vec<u8> = row.get(2)?;
-                let script: Script = script.into();
+                let script_pubkey: Script = script.into();
+                let is_spent: bool = row.get(3)?;
 
-                Ok(Some((value, keychain, script)))
+                Ok(Some(LocalUtxo {
+                    outpoint: OutPoint::new(deserialize(txid)?, vout),
+                    txout: TxOut {
+                        value,
+                        script_pubkey,
+                    },
+                    keychain,
+                    is_spent,
+                }))
             }
             None => Ok(None),
         }
@@ -620,6 +630,7 @@ impl BatchOperations for SqliteDatabase {
             utxo.outpoint.vout,
             &utxo.outpoint.txid,
             utxo.txout.script_pubkey.as_bytes(),
+            utxo.is_spent,
         )?;
         Ok(())
     }
@@ -694,16 +705,9 @@ impl BatchOperations for SqliteDatabase {
 
     fn del_utxo(&mut self, outpoint: &OutPoint) -> Result<Option<LocalUtxo>, Error> {
         match self.select_utxo_by_outpoint(&outpoint.txid, outpoint.vout)? {
-            Some((value, keychain, script_pubkey)) => {
+            Some(local_utxo) => {
                 self.delete_utxo_by_outpoint(&outpoint.txid, outpoint.vout)?;
-                Ok(Some(LocalUtxo {
+                Ok(Some(local_utxo))
-                    outpoint: *outpoint,
-                    txout: TxOut {
-                        value,
-                        script_pubkey,
-                    },
-                    keychain,
-                }))
             }
             None => Ok(None),
         }
@@ -832,17 +836,7 @@ impl Database for SqliteDatabase {
     }
 
     fn get_utxo(&self, outpoint: &OutPoint) -> Result<Option<LocalUtxo>, Error> {
-        match self.select_utxo_by_outpoint(&outpoint.txid, outpoint.vout)? {
+        self.select_utxo_by_outpoint(&outpoint.txid, outpoint.vout)
-            Some((value, keychain, script_pubkey)) => Ok(Some(LocalUtxo {
-                outpoint: *outpoint,
-                txout: TxOut {
-                    value,
-                    script_pubkey,
-                },
-                keychain,
-            })),
-            None => Ok(None),
-        }
     }
 
     fn get_raw_tx(&self, txid: &Txid) -> Result<Option<Transaction>, Error> {

src/testutils/blockchain_tests.rs

@@ -1124,6 +1124,47 @@ macro_rules! bdk_blockchain_tests {
                 assert_eq!(tx_2.received, 10_000);
                 assert_eq!(tx_2.sent, 0);
             }
+
+            #[test]
+            fn test_double_spend() {
+                // We create a tx and then we try to double spend it; BDK will always allow
+                // us to do so, as it never forgets about spent UTXOs
+                let (wallet, blockchain, descriptors, mut test_client) = init_single_sig();
+                let node_addr = test_client.get_node_address(None);
+                let _ = test_client.receive(testutils! {
+                    @tx ( (@external descriptors, 0) => 50_000 )
+                });
+
+                wallet.sync(&blockchain, SyncOptions::default()).unwrap();
+                let mut builder = wallet.build_tx();
+                builder.add_recipient(node_addr.script_pubkey(), 25_000);
+                let (mut psbt, _details) = builder.finish().unwrap();
+                let finalized = wallet.sign(&mut psbt, Default::default()).unwrap();
+                assert!(finalized, "Cannot finalize transaction");
+                let initial_tx = psbt.extract_tx();
+                let _sent_txid = blockchain.broadcast(&initial_tx).unwrap();
+                wallet.sync(&blockchain, SyncOptions::default()).unwrap();
+                for utxo in wallet.list_unspent().unwrap() {
+                    // Making sure the TXO we just spent is not returned by list_unspent
+                    assert!(utxo.outpoint != initial_tx.input[0].previous_output, "wallet displays spent txo in unspents");
+                }
+                // We can still create a transaction double spending `initial_tx`
+                let mut builder = wallet.build_tx();
+                builder
+                    .add_utxo(initial_tx.input[0].previous_output)
+                    .expect("Can't manually add an UTXO spent");
+                test_client.generate(1, Some(node_addr));
+                wallet.sync(&blockchain, SyncOptions::default()).unwrap();
+                // Even after confirmation, we can still create a tx double spend it
+                let mut builder = wallet.build_tx();
+                builder
+                    .add_utxo(initial_tx.input[0].previous_output)
+                    .expect("Can't manually add an UTXO spent");
+                for utxo in wallet.list_unspent().unwrap() {
+                    // Making sure the TXO we just spent is not returned by list_unspent
+                    assert!(utxo.outpoint != initial_tx.input[0].previous_output, "wallet displays spent txo in unspents");
+                }
+            }
         }
     };
 

src/types.rs

@@ -131,6 +131,8 @@ pub struct LocalUtxo {
     pub txout: TxOut,
     /// Type of keychain
     pub keychain: KeychainKind,
+    /// Whether this UTXO is spent or not
+    pub is_spent: bool,
 }
 
 /// A [`Utxo`] with its `satisfaction_weight`.

src/wallet/coin_selection.rs

@@ -569,6 +569,7 @@ mod test {
                     script_pubkey: Script::new(),
                 },
                 keychain: KeychainKind::External,
+                is_spent: false,
             }),
         }
     }
@@ -596,6 +597,7 @@ mod test {
                         script_pubkey: Script::new(),
                     },
                     keychain: KeychainKind::External,
+                    is_spent: false,
                 }),
             });
         }
@@ -615,6 +617,7 @@ mod test {
                     script_pubkey: Script::new(),
                 },
                 keychain: KeychainKind::External,
+                is_spent: false,
             }),
         };
         vec![utxo; utxos_number]

src/wallet/mod.rs

@@ -387,7 +387,13 @@ where
     /// Note that this method only operates on the internal database, which first needs to be
     /// [`Wallet::sync`] manually.
     pub fn list_unspent(&self) -> Result<Vec<LocalUtxo>, Error> {
-        self.database.borrow().iter_utxos()
+        Ok(self
+            .database
+            .borrow()
+            .iter_utxos()?
+            .into_iter()
+            .filter(|l| !l.is_spent)
+            .collect())
     }
 
     /// Returns the `UTXO` owned by this wallet corresponding to `outpoint` if it exists in the
@@ -879,6 +885,7 @@ where
                     outpoint: txin.previous_output,
                     txout,
                     keychain,
+                    is_spent: true,
                 };
 
                 Ok(WeightedUtxo {

src/wallet/tx_builder.rs

@@ -838,6 +838,7 @@ mod test {
                 },
                 txout: Default::default(),
                 keychain: KeychainKind::External,
+                is_spent: false,
             },
             LocalUtxo {
                 outpoint: OutPoint {
@@ -846,6 +847,7 @@ mod test {
                 },
                 txout: Default::default(),
                 keychain: KeychainKind::Internal,
+                is_spent: false,
             },
         ]
     }