review comments 2

base_layer/wallet/src/output_manager_service/storage/sqlite_db/mod.rs

@@ -417,21 +417,6 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase {
     }
 
     // Perform a batch update of the received outputs; this is more efficient than updating each output individually.
-    // This SQL query is a dummy `INSERT INTO` statement combined with an `ON CONFLICT` clause and `UPDATE` action.
-    // It specifies what action should be taken if a unique constraint violation occurs during the execution of the
-    // `INSERT INTO` statement. The `INSERT INTO` statement must list all columns that cannot be NULL should it
-    // succeed. We provide `commitment` values that will cause a unique constraint violation, triggering the
-    // `ON CONFLICT` clause. The `ON CONFLICT` clause ensures that if a row with a matching commitment already
-    // exists, the specified columns (`mined_height`, `mined_in_block`, `status`, `mined_timestamp`,
-    // `marked_deleted_at_height`, `marked_deleted_in_block`, `last_validation_timestamp`) will be updated with the
-    // provided values. The `UPDATE` action updates the existing row with the new values provided by the
-    // `INSERT INTO` statement. The `excluded` keyword refers to the new data being inserted or updated and allows
-    // accessing the values provided in the `VALUES` clause of the `INSERT INTO` statement.
-    // Note:
-    //   `diesel` does not support batch updates, so we have to do it manually. For example, this
-    //   `diesel::insert_into(...).values(&...).on_conflict(outputs::hash).do_update().set((...)).execute(&mut conn)?;`
-    //   errors with
-    //   `the trait bound `BatchInsert<Vec<....>` is not satisfied`
     fn set_received_outputs_mined_height_and_statuses(
         &self,
         updates: Vec<ReceivedOutputInfoForBatch>,
@@ -446,6 +431,26 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase {
             updates.len()
         );
 
+        let commitments: Vec<Commitment> = updates.iter().map(|update| update.commitment.clone()).collect();
+        if !OutputSql::verify_outputs_exist(&commitments, &mut conn)? {
+            return Err(OutputManagerStorageError::ValuesNotFound);
+        }
+
+        // This SQL query is a dummy `INSERT INTO` statement combined with an `ON CONFLICT` clause and `UPDATE` action.
+        // It specifies what action should be taken if a unique constraint violation occurs during the execution of the
+        // `INSERT INTO` statement. The `INSERT INTO` statement must list all columns that cannot be NULL should it
+        // succeed. We provide `commitment` values that will cause a unique constraint violation, triggering the
+        // `ON CONFLICT` clause. The `ON CONFLICT` clause ensures that if a row with a matching commitment already
+        // exists, the specified columns (`mined_height`, `mined_in_block`, `status`, `mined_timestamp`,
+        // `marked_deleted_at_height`, `marked_deleted_in_block`, `last_validation_timestamp`) will be updated with the
+        // provided values. The `UPDATE` action updates the existing row with the new values provided by the
+        // `INSERT INTO` statement. The `excluded` keyword refers to the new data being inserted or updated and allows
+        // accessing the values provided in the `VALUES` clause of the `INSERT INTO` statement.
+        // Note:
+        //   `diesel` does not support batch updates, so we have to do it manually. For example, this
+        //   `diesel::insert_into(...).values(&...).on_conflict(outputs::hash).do_update().set((...)).execute(&mut
+        // conn)?;`   errors with
+        //   `the trait bound `BatchInsert<Vec<....>` is not satisfied`
         let mut query = String::from(
             "INSERT INTO outputs ( commitment, mined_height, mined_in_block, status, mined_timestamp, spending_key, \
              value, output_type, maturity, hash, script, input_data, script_private_key, sender_offset_public_key, \
@@ -685,21 +690,6 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase {
     }
 
     // Perform a batch update of the spent outputs; this is more efficient than updating each output individually.
-    // This SQL query is a dummy `INSERT INTO` statement combined with an `ON CONFLICT` clause and `UPDATE` action.
-    // It specifies what action should be taken if a unique constraint violation occurs during the execution of the
-    // `INSERT INTO` statement. The `INSERT INTO` statement must list all columns that cannot be NULL should it
-    // succeed. We provide `commitment` values that will cause a unique constraint violation, triggering the
-    // `ON CONFLICT` clause. The `ON CONFLICT` clause ensures that if a row with a matching commitment already
-    // exists, the specified columns (`mined_height`, `mined_in_block`, `status`, `mined_timestamp`,
-    // `marked_deleted_at_height`, `marked_deleted_in_block`, `last_validation_timestamp`) will be updated with the
-    // provided values. The `UPDATE` action updates the existing row with the new values provided by the
-    // `INSERT INTO` statement. The `excluded` keyword refers to the new data being inserted or updated and allows
-    // accessing the values provided in the `VALUES` clause of the `INSERT INTO` statement.
-    // Note:
-    //   `diesel` does not support batch updates, so we have to do it manually. For example, this
-    //   `diesel::insert_into(...).values(&...).on_conflict(outputs::hash).do_update().set((...)).execute(&mut conn)?;`
-    //   errors with
-    //   `the trait bound `BatchInsert<Vec<....>` is not satisfied`
     fn mark_outputs_as_spent(&self, updates: Vec<SpentOutputInfoForBatch>) -> Result<(), OutputManagerStorageError> {
         let start = Instant::now();
         let mut conn = self.database_connection.get_pooled_connection()?;
@@ -711,6 +701,26 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase {
             updates.len()
         );
 
+        let commitments: Vec<Commitment> = updates.iter().map(|update| update.commitment.clone()).collect();
+        if !OutputSql::verify_outputs_exist(&commitments, &mut conn)? {
+            return Err(OutputManagerStorageError::ValuesNotFound);
+        }
+
+        // This SQL query is a dummy `INSERT INTO` statement combined with an `ON CONFLICT` clause and `UPDATE` action.
+        // It specifies what action should be taken if a unique constraint violation occurs during the execution of the
+        // `INSERT INTO` statement. The `INSERT INTO` statement must list all columns that cannot be NULL should it
+        // succeed. We provide `commitment` values that will cause a unique constraint violation, triggering the
+        // `ON CONFLICT` clause. The `ON CONFLICT` clause ensures that if a row with a matching commitment already
+        // exists, the specified columns (`mined_height`, `mined_in_block`, `status`, `mined_timestamp`,
+        // `marked_deleted_at_height`, `marked_deleted_in_block`, `last_validation_timestamp`) will be updated with the
+        // provided values. The `UPDATE` action updates the existing row with the new values provided by the
+        // `INSERT INTO` statement. The `excluded` keyword refers to the new data being inserted or updated and allows
+        // accessing the values provided in the `VALUES` clause of the `INSERT INTO` statement.
+        // Note:
+        //   `diesel` does not support batch updates, so we have to do it manually. For example, this
+        //   `diesel::insert_into(...).values(&...).on_conflict(outputs::hash).do_update().set((...)).execute(&mut
+        // conn)?;`   errors with
+        //   `the trait bound `BatchInsert<Vec<....>` is not satisfied`
         let mut query = String::from(
             "INSERT INTO outputs ( commitment, marked_deleted_at_height, marked_deleted_in_block, status, \
              mined_height, mined_in_block, mined_timestamp, spending_key, value, output_type, maturity, hash, script, \

base_layer/wallet/src/output_manager_service/storage/sqlite_db/output_sql.rs

@@ -42,6 +42,7 @@ use tari_core::transactions::{
 use tari_crypto::tari_utilities::ByteArray;
 use tari_key_manager::key_manager_service::KeyId;
 use tari_script::{ExecutionStack, TariScript};
+use tari_utilities::hex::Hex;
 
 use crate::{
     output_manager_service::{
@@ -378,6 +379,31 @@ impl OutputSql {
             .load(conn)?)
     }
 
+    /// Verify that outputs with specified commitments exist in the database
+    pub fn verify_outputs_exist(
+        commitments: &[Commitment],
+        conn: &mut SqliteConnection,
+    ) -> Result<bool, OutputManagerStorageError> {
+        #[derive(QueryableByName, Clone)]
+        struct CountQueryResult {
+            #[diesel(sql_type = diesel::sql_types::BigInt)]
+            count: i64,
+        }
+        let placeholders = commitments
+            .iter()
+            .map(|v| format!("x'{}'", v.to_hex()))
+            .collect::<Vec<_>>()
+            .join(", ");
+        let query = sql_query(format!(
+            "SELECT COUNT(*) as count FROM outputs WHERE commitment IN ({})",
+            placeholders
+        ));
+        let query_result = query.load::<CountQueryResult>(conn)?;
+        let commitments_len = i64::try_from(commitments.len())
+            .map_err(|e| OutputManagerStorageError::ConversionError { reason: e.to_string() })?;
+        Ok(query_result[0].count == commitments_len)
+    }
+
     /// Return the available, time locked, pending incoming and pending outgoing balance
     #[allow(clippy::cast_possible_wrap)]
     pub fn get_balance(

base_layer/wallet/tests/output_manager_service_tests/storage.rs

@@ -20,6 +20,8 @@
 // WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE
 // USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
+use std::convert::TryFrom;
+
 use minotari_wallet::output_manager_service::{
     error::OutputManagerStorageError,
     service::Balance,
@@ -34,15 +36,15 @@ use minotari_wallet::output_manager_service::{
 use rand::{rngs::OsRng, RngCore};
 use tari_common_types::{
     transaction::TxId,
-    types::{FixedHash, PrivateKey},
+    types::{FixedHash, HashOutput, PrivateKey},
 };
 use tari_core::transactions::{
     key_manager::create_memory_db_key_manager,
     tari_amount::MicroMinotari,
     transaction_components::OutputFeatures,
 };
 use tari_crypto::keys::SecretKey;
-use tari_utilities::hex::Hex;
+use tari_utilities::{hex::Hex, ByteArray};
 
 use crate::support::{data::get_temp_sqlite_database_connection, utils::make_input};
 
@@ -403,6 +405,12 @@ pub async fn test_raw_custom_queries_regression() {
         unspent.push((kmo.hash, true));
         unspent_outputs.push(kmo);
     }
+
+    let unknown = HashOutput::try_from(PrivateKey::random(&mut rand::thread_rng()).as_bytes()).unwrap();
+    let mut unspent_with_unknown = unspent.clone();
+    unspent_with_unknown.push((unknown, true));
+    assert!(db.mark_outputs_as_unspent(unspent_with_unknown).is_err());
+
     db.mark_outputs_as_unspent(unspent).unwrap();
 
     // Add some sent transactions with outputs to be spent and received
@@ -476,6 +484,29 @@ pub async fn test_raw_custom_queries_regression() {
             mined_timestamp: 0,
         });
     }
+
+    let uo = make_input(
+        &mut OsRng,
+        MicroMinotari::from(100 + OsRng.next_u64() % 1000),
+        &OutputFeatures::default(),
+        &key_manager,
+    )
+    .await;
+    let unknown = DbWalletOutput::from_wallet_output(uo, &key_manager, None, OutputSource::Standard, None, None)
+        .await
+        .unwrap();
+    let mut updates_info_with_unknown = updates_info.clone();
+    updates_info_with_unknown.push(ReceivedOutputInfoForBatch {
+        commitment: unknown.commitment.clone(),
+        mined_height: 2,
+        mined_in_block: block_hashes[0],
+        confirmed: true,
+        mined_timestamp: 0,
+    });
+    assert!(db
+        .set_received_outputs_mined_height_and_statuses(updates_info_with_unknown)
+        .is_err());
+
     db.set_received_outputs_mined_height_and_statuses(updates_info).unwrap();
 
     for (i, to_be_received) in pending_txs[0].outputs_to_be_received.iter().enumerate() {
@@ -507,6 +538,16 @@ pub async fn test_raw_custom_queries_regression() {
             mark_deleted_in_block,
         });
     }
+
+    let mut updates_info_with_unknown = updates_info.clone();
+    updates_info_with_unknown.push(SpentOutputInfoForBatch {
+        commitment: unknown.commitment,
+        confirmed: true,
+        mark_deleted_at_height: 4,
+        mark_deleted_in_block: block_hashes[0],
+    });
+    assert!(db.mark_outputs_as_spent(updates_info_with_unknown).is_err());
+
     db.mark_outputs_as_spent(updates_info).unwrap();
 
     for (i, to_be_spent) in pending_txs[0].outputs_to_be_spent.iter().enumerate() {