fix: fix Unique Constraint bug when requesting a coinbase output at…

… same height (#3004)

base_layer/wallet/src/output_manager_service/error.rs

@@ -20,7 +20,7 @@
 // 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 crate::{base_node_service::error::BaseNodeServiceError, output_manager_service::storage::database::DbKey};
+use crate::base_node_service::error::BaseNodeServiceError;
 use diesel::result::Error as DieselError;
 use tari_comms::{peer_manager::node_id::NodeIdError, protocol::rpc::RpcError};
 use tari_comms_dht::outbound::DhtOutboundError;
@@ -115,8 +115,12 @@ pub enum OutputManagerError {
 pub enum OutputManagerStorageError {
     #[error("Tried to insert an output that already exists in the database")]
     DuplicateOutput,
-    #[error("Value not found: `{0}`")]
-    ValueNotFound(DbKey),
+    #[error(
+        "Tried to insert an pending transaction encumberance for a transaction ID that already exists in the database"
+    )]
+    DuplicateTransaction,
+    #[error("Value not found")]
+    ValueNotFound,
     #[error("Unexpected result: `{0}`")]
     UnexpectedResult(String),
     #[error("If an pending transaction does not exist to be confirmed")]

base_layer/wallet/src/output_manager_service/service.rs

@@ -654,11 +654,6 @@ where TBackend: OutputManagerBackend + 'static
         fees: MicroTari,
         block_height: u64,
     ) -> Result<Transaction, OutputManagerError> {
-        self.resources
-            .db
-            .cancel_pending_transaction_at_block_height(block_height)
-            .await?;
-
         let (spending_key, script_key) = self
             .resources
             .master_key_manager
@@ -691,6 +686,25 @@ where TBackend: OutputManagerBackend + 'static
             &self.resources.factories,
         )?;
 
+        // Clear any existing pending coinbase transactions for this blockheight
+        self.resources
+            .db
+            .cancel_pending_transaction_at_block_height(block_height)
+            .await?;
+
+        // Clear any matching coinbase outputs for this block_height AND commitment. Even if the older output is valid
+        // we are losing no information as this output has the same commitment.
+        match self
+            .resources
+            .db
+            .remove_coinbase_output_at_block_height(output.commitment.clone(), output.unblinded_output.height)
+            .await
+        {
+            Ok(_) => {},
+            Err(OutputManagerStorageError::ValueNotFound) => {},
+            Err(e) => return Err(e.into()),
+        }
+
         self.resources
             .db
             .accept_incoming_pending_transaction(tx_id, output, Some(block_height))

base_layer/wallet/src/output_manager_service/storage/database.rs

@@ -133,6 +133,7 @@ pub enum DbKey {
     KeyManagerState,
     InvalidOutputs,
     KnownOneSidedPaymentScripts,
+    CoinbaseOutput { commitment: Commitment, block_height: u64 },
 }
 
 #[derive(Debug)]
@@ -146,6 +147,7 @@ pub enum DbValue {
     AllPendingTransactionOutputs(HashMap<TxId, PendingTransactionOutputs>),
     KeyManagerState(KeyManagerState),
     KnownOneSidedPaymentScripts(Vec<KnownOneSidedPaymentScript>),
+    CoinbaseOutput(Box<DbUnblindedOutput>),
 }
 
 pub enum DbKeyValuePair {
@@ -167,7 +169,7 @@ macro_rules! fetch {
     ($db:ident, $key_val:expr, $key_var:ident) => {{
         let key = DbKey::$key_var($key_val);
         match $db.fetch(&key) {
-            Ok(None) => Err(OutputManagerStorageError::ValueNotFound(key)),
+            Ok(None) => Err(OutputManagerStorageError::ValueNotFound),
             Ok(Some(DbValue::$key_var(k))) => Ok(*k),
             Ok(Some(other)) => unexpected_result(key, other),
             Err(e) => log_error(key, e),
@@ -631,6 +633,46 @@ where T: OutputManagerBackend + 'static
             .and_then(|inner_result| inner_result)
     }
 
+    pub async fn remove_coinbase_output_at_block_height(
+        &self,
+        commitment: Commitment,
+        block_height: u64,
+    ) -> Result<(), OutputManagerStorageError> {
+        let db_clone = self.db.clone();
+        tokio::task::spawn_blocking(move || {
+            match db_clone.write(WriteOperation::Remove(DbKey::CoinbaseOutput {
+                commitment: commitment.clone(),
+                block_height,
+            })) {
+                Ok(None) => log_error(
+                    DbKey::CoinbaseOutput {
+                        commitment,
+                        block_height,
+                    },
+                    OutputManagerStorageError::ValueNotFound,
+                ),
+                Ok(Some(DbValue::CoinbaseOutput(_))) => Ok(()),
+                Ok(Some(other)) => unexpected_result(
+                    DbKey::CoinbaseOutput {
+                        commitment,
+                        block_height,
+                    },
+                    other,
+                ),
+                Err(e) => log_error(
+                    DbKey::CoinbaseOutput {
+                        commitment,
+                        block_height,
+                    },
+                    e,
+                ),
+            }
+        })
+        .await
+        .map_err(|err| OutputManagerStorageError::BlockingTaskSpawnError(err.to_string()))??;
+        Ok(())
+    }
+
     pub async fn update_output_mined_height(&self, tx_id: u64, height: u64) -> Result<(), OutputManagerStorageError> {
         let db_clone = self.db.clone();
         tokio::task::spawn_blocking(move || db_clone.update_mined_height(tx_id, height))
@@ -712,6 +754,7 @@ impl Display for DbKey {
             DbKey::InvalidOutputs => f.write_str(&"Invalid Outputs Key"),
             DbKey::TimeLockedUnspentOutputs(_t) => f.write_str(&"Timelocked Outputs"),
             DbKey::KnownOneSidedPaymentScripts => f.write_str(&"Known claiming scripts"),
+            DbKey::CoinbaseOutput { .. } => f.write_str("Coinbase Output"),
         }
     }
 }
@@ -728,6 +771,7 @@ impl Display for DbValue {
             DbValue::KeyManagerState(_) => f.write_str("Key Manager State"),
             DbValue::InvalidOutputs(_) => f.write_str("Invalid Outputs"),
             DbValue::KnownOneSidedPaymentScripts(_) => f.write_str(&"Known claiming scripts"),
+            DbValue::CoinbaseOutput(_) => f.write_str("Coinbase Output"),
         }
     }
 }

base_layer/wallet/src/output_manager_service/storage/memory_db.rs

@@ -150,6 +150,14 @@ impl OutputManagerBackend for OutputManagerMemoryDatabase {
             DbKey::KnownOneSidedPaymentScripts => Some(DbValue::KnownOneSidedPaymentScripts(
                 db.known_one_sided_payment_scripts.clone(),
             )),
+            DbKey::CoinbaseOutput {
+                commitment,
+                block_height,
+            } => db
+                .unspent_outputs
+                .iter()
+                .find(|v| &v.output.commitment == commitment && &v.output.unblinded_output.height == block_height)
+                .map(|v| DbValue::CoinbaseOutput(Box::new(DbUnblindedOutput::from((*v).clone())))),
         };
 
         Ok(result)
@@ -197,7 +205,7 @@ impl OutputManagerBackend for OutputManagerMemoryDatabase {
                     .iter()
                     .position(|v| v.output.unblinded_output.spending_key == k)
                 {
-                    None => return Err(OutputManagerStorageError::ValueNotFound(DbKey::SpentOutput(k))),
+                    None => return Err(OutputManagerStorageError::ValueNotFound),
                     Some(pos) => {
                         return Ok(Some(DbValue::SpentOutput(Box::new(DbUnblindedOutput::from(
                             db.spent_outputs.remove(pos),
@@ -209,7 +217,7 @@ impl OutputManagerBackend for OutputManagerMemoryDatabase {
                     .iter()
                     .position(|v| v.output.unblinded_output.spending_key == k)
                 {
-                    None => return Err(OutputManagerStorageError::ValueNotFound(DbKey::UnspentOutput(k))),
+                    None => return Err(OutputManagerStorageError::ValueNotFound),
                     Some(pos) => {
                         return Ok(Some(DbValue::UnspentOutput(Box::new(DbUnblindedOutput::from(
                             db.unspent_outputs.remove(pos),
@@ -220,11 +228,21 @@ impl OutputManagerBackend for OutputManagerMemoryDatabase {
                     if let Some(p) = db.pending_transactions.remove(&tx_id) {
                         return Ok(Some(DbValue::PendingTransactionOutputs(Box::new(p))));
                     } else {
-                        return Err(OutputManagerStorageError::ValueNotFound(
-                            DbKey::PendingTransactionOutputs(tx_id),
-                        ));
+                        return Err(OutputManagerStorageError::ValueNotFound);
                     }
                 },
+                DbKey::CoinbaseOutput { commitment, .. } => match db
+                    .unspent_outputs
+                    .iter()
+                    .position(|v| v.output.commitment == commitment)
+                {
+                    None => return Err(OutputManagerStorageError::ValueNotFound),
+                    Some(pos) => {
+                        return Ok(Some(DbValue::UnspentOutput(Box::new(DbUnblindedOutput::from(
+                            db.unspent_outputs.remove(pos),
+                        )))));
+                    },
+                },
                 DbKey::UnspentOutputs => return Err(OutputManagerStorageError::OperationNotSupported),
                 DbKey::SpentOutputs => return Err(OutputManagerStorageError::OperationNotSupported),
                 DbKey::AllPendingTransactionOutputs => return Err(OutputManagerStorageError::OperationNotSupported),
@@ -245,9 +263,7 @@ impl OutputManagerBackend for OutputManagerMemoryDatabase {
             pending_tx = db.short_term_pending_transactions.remove(&tx_id);
         }
 
-        let mut pending_tx = pending_tx.ok_or(OutputManagerStorageError::ValueNotFound(
-            DbKey::PendingTransactionOutputs(tx_id),
-        ))?;
+        let mut pending_tx = pending_tx.ok_or(OutputManagerStorageError::ValueNotFound)?;
 
         // Add Spent outputs
         for o in pending_tx.outputs_to_be_spent.drain(..) {
@@ -308,12 +324,10 @@ impl OutputManagerBackend for OutputManagerMemoryDatabase {
     fn confirm_encumbered_outputs(&self, tx_id: u64) -> Result<(), OutputManagerStorageError> {
         let mut db = acquire_write_lock!(self.db);
 
-        let pending_tx =
-            db.short_term_pending_transactions
-                .remove(&tx_id)
-                .ok_or(OutputManagerStorageError::ValueNotFound(
-                    DbKey::PendingTransactionOutputs(tx_id),
-                ))?;
+        let pending_tx = db
+            .short_term_pending_transactions
+            .remove(&tx_id)
+            .ok_or(OutputManagerStorageError::ValueNotFound)?;
 
         let _ = db.pending_transactions.insert(pending_tx.tx_id, pending_tx);
 
@@ -341,9 +355,7 @@ impl OutputManagerBackend for OutputManagerMemoryDatabase {
             pending_tx = db.short_term_pending_transactions.remove(&tx_id);
         }
 
-        let mut pending_tx = pending_tx.ok_or(OutputManagerStorageError::ValueNotFound(
-            DbKey::PendingTransactionOutputs(tx_id),
-        ))?;
+        let mut pending_tx = pending_tx.ok_or(OutputManagerStorageError::ValueNotFound)?;
 
         for o in pending_tx.outputs_to_be_spent.drain(..) {
             db.unspent_outputs.push(DbOutput::new(tx_id, o));

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

@@ -253,6 +253,22 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase {
                         .collect::<Result<Vec<_>, _>>()?,
                 ))
             },
+            DbKey::CoinbaseOutput {
+                commitment,
+                block_height,
+            } => match OutputSql::find_by_commitment_and_block_height(&commitment.to_vec(), *block_height, &(*conn)) {
+                Ok(mut o) => {
+                    self.decrypt_if_necessary(&mut o)?;
+                    Some(DbValue::CoinbaseOutput(Box::new(DbUnblindedOutput::try_from(o)?)))
+                },
+                Err(e) => {
+                    match e {
+                        OutputManagerStorageError::DieselError(DieselError::NotFound) => (),
+                        e => return Err(e),
+                    };
+                    None
+                },
+            },
         };
 
         Ok(result)
@@ -291,7 +307,7 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase {
                 },
                 DbKeyValuePair::PendingTransactionOutputs(tx_id, p) => {
                     if PendingTransactionOutputSql::find(tx_id, &(*conn)).is_ok() {
-                        return Err(OutputManagerStorageError::DuplicateOutput);
+                        return Err(OutputManagerStorageError::DuplicateTransaction);
                     }
 
                     PendingTransactionOutputSql::new(
@@ -348,6 +364,23 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase {
                         };
                     },
                 },
+                DbKey::CoinbaseOutput {
+                    commitment,
+                    block_height,
+                } => {
+                    match OutputSql::find_by_commitment_and_block_height(&commitment.to_vec(), block_height, &(*conn)) {
+                        Ok(o) => {
+                            o.delete(&(*conn))?;
+                            return Ok(Some(DbValue::CoinbaseOutput(Box::new(DbUnblindedOutput::try_from(o)?))));
+                        },
+                        Err(e) => {
+                            match e {
+                                OutputManagerStorageError::DieselError(DieselError::NotFound) => (),
+                                e => return Err(e),
+                            };
+                        },
+                    }
+                },
                 DbKey::PendingTransactionOutputs(tx_id) => match PendingTransactionOutputSql::find(tx_id, &(*conn)) {
                     Ok(p) => {
                         let mut outputs = OutputSql::find_by_tx_id_and_encumbered(p.tx_id as u64, &(*conn))?;
@@ -483,9 +516,7 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase {
             Err(e) => {
                 match e {
                     OutputManagerStorageError::DieselError(DieselError::NotFound) => {
-                        return Err(OutputManagerStorageError::ValueNotFound(
-                            DbKey::PendingTransactionOutputs(tx_id),
-                        ))
+                        return Err(OutputManagerStorageError::ValueNotFound)
                     },
                     e => return Err(e),
                 };
@@ -547,9 +578,7 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase {
             Err(e) => {
                 match e {
                     OutputManagerStorageError::DieselError(DieselError::NotFound) => {
-                        return Err(OutputManagerStorageError::ValueNotFound(
-                            DbKey::PendingTransactionOutputs(tx_id),
-                        ))
+                        return Err(OutputManagerStorageError::ValueNotFound)
                     },
                     e => return Err(e),
                 };
@@ -944,7 +973,6 @@ impl OutputSql {
             .first::<OutputSql>(conn)?)
     }
 
-    /// Find a particular Output by its public_spending_key
     pub fn find_by_commitment(
         commitment: &[u8],
         conn: &SqliteConnection,
@@ -956,6 +984,17 @@ impl OutputSql {
             .first::<OutputSql>(conn)?)
     }
 
+    pub fn find_by_commitment_and_block_height(
+        commitment: &[u8],
+        height: u64,
+        conn: &SqliteConnection,
+    ) -> Result<OutputSql, OutputManagerStorageError> {
+        Ok(outputs::table
+            .filter(outputs::commitment.eq(commitment))
+            .filter(outputs::height.eq(height as i64))
+            .first::<OutputSql>(conn)?)
+    }
+
     /// Find outputs via tx_id
     pub fn find_by_tx_id(tx_id: TxId, conn: &SqliteConnection) -> Result<Vec<OutputSql>, OutputManagerStorageError> {
         Ok(outputs::table

base_layer/wallet/tests/output_manager_service/service.rs

@@ -935,7 +935,7 @@ fn cancel_transaction<T: OutputManagerBackend + 'static>(backend: T) {
         .unwrap();
 
     match runtime.block_on(oms.cancel_transaction(1)) {
-        Err(OutputManagerError::OutputManagerStorageError(OutputManagerStorageError::ValueNotFound(_))) => {},
+        Err(OutputManagerError::OutputManagerStorageError(OutputManagerStorageError::ValueNotFound)) => {},
         _ => panic!("Value should not exist"),
     }
 
@@ -2193,3 +2193,31 @@ fn test_oms_key_manager_discrepancy() {
         Err(OutputManagerError::MasterSecretKeyMismatch)
     ));
 }
+
+#[test]
+fn get_coinbase_tx_for_same_height() {
+    let db_name = format!("{}.sqlite3", random_string(8).as_str());
+    let db_tempdir = tempdir().unwrap();
+    let db_folder = db_tempdir.path().to_str().unwrap().to_string();
+    let db_path = format!("{}/{}", db_folder, db_name);
+    let connection = run_migration_and_create_sqlite_connection(&db_path).unwrap();
+
+    let mut runtime = Runtime::new().unwrap();
+    let (mut oms, _shutdown, _, _, _, _, _) =
+        setup_output_manager_service(&mut runtime, OutputManagerSqliteDatabase::new(connection, None), true);
+
+    runtime
+        .block_on(oms.get_coinbase_transaction(1, 100_000.into(), 100.into(), 1))
+        .unwrap();
+
+    let pending_transactions = runtime.block_on(oms.get_pending_transactions()).unwrap();
+    assert!(pending_transactions.values().any(|p| p.tx_id == 1));
+
+    runtime
+        .block_on(oms.get_coinbase_transaction(2, 100_000.into(), 100.into(), 1))
+        .unwrap();
+
+    let pending_transactions = runtime.block_on(oms.get_pending_transactions()).unwrap();
+    assert!(!pending_transactions.values().any(|p| p.tx_id == 1));
+    assert!(pending_transactions.values().any(|p| p.tx_id == 2));
+}