fix: tms validation correctly updating (#6079)

Description
---
Fixes TMS validation to not keep updating the mined height of coinbase
transactions

Motivation and Context
---
Currently, there is a bug in the coinbase validation where if its not
mined, it keeps increasing the mined height.
This changes it to use the OMS to track coinbases as they need to be
tracked from the output via the OMS.

applications/minotari_app_grpc/proto/block.proto

@@ -121,7 +121,6 @@ message NewBlockHeaderTemplate {
     bytes total_kernel_offset = 4;
     // Proof of work metadata
     ProofOfWork pow = 5;
-//    uint64 target_difficulty = 6;
     // Sum of script offsets for all kernels in this block.
     bytes total_script_offset = 7;
 }

applications/minotari_app_grpc/proto/wallet.proto

@@ -227,6 +227,8 @@ enum TransactionStatus {
     TRANSACTION_STATUS_COINBASE_UNCONFIRMED = 12;
     // This is Coinbase transaction that is detected from chain
     TRANSACTION_STATUS_COINBASE_CONFIRMED = 13;
+    // This is Coinbase transaction that is not currently detected as mined
+    TRANSACTION_STATUS_COINBASE_NOT_IN_BLOCK_CHAIN = 14;
 }
 
 message GetCompletedTransactionsRequest { }

applications/minotari_app_grpc/src/conversions/transaction.rs

@@ -104,6 +104,7 @@ impl From<TransactionStatus> for grpc::TransactionStatus {
             OneSidedConfirmed => grpc::TransactionStatus::OneSidedConfirmed,
             CoinbaseUnconfirmed => grpc::TransactionStatus::CoinbaseUnconfirmed,
             CoinbaseConfirmed => grpc::TransactionStatus::CoinbaseConfirmed,
+            CoinbaseNotInBlockChain => grpc::TransactionStatus::CoinbaseNotInBlockChain,
             Queued => grpc::TransactionStatus::Queued,
         }
     }

base_layer/common_types/src/transaction.rs

@@ -38,10 +38,13 @@ pub enum TransactionStatus {
     OneSidedConfirmed = 9,
     /// This transaction is still being queued for initial sending
     Queued = 10,
-    /// This transaction import status is used when a one-sided transaction has been scanned but is unconfirmed
+    /// This transaction import status is used when a coinbase transaction has been scanned but is unconfirmed
     CoinbaseUnconfirmed = 11,
-    /// This transaction import status is used when a one-sided transaction has been scanned and confirmed
+    /// This transaction import status is used when a coinbase transaction has been scanned and confirmed
     CoinbaseConfirmed = 12,
+    /// This transaction import status is used when a coinbase transaction has been scanned but the outputs are not
+    /// currently confirmed on the blockchain via the output manager
+    CoinbaseNotInBlockChain = 13,
 }
 
 impl TransactionStatus {
@@ -55,7 +58,9 @@ impl TransactionStatus {
     pub fn is_coinbase(&self) -> bool {
         matches!(
             self,
-            TransactionStatus::CoinbaseUnconfirmed | TransactionStatus::CoinbaseConfirmed
+            TransactionStatus::CoinbaseUnconfirmed |
+                TransactionStatus::CoinbaseConfirmed |
+                TransactionStatus::CoinbaseNotInBlockChain
         )
     }
 
@@ -81,9 +86,9 @@ impl TransactionStatus {
             TransactionStatus::Imported |
             TransactionStatus::OneSidedUnconfirmed |
             TransactionStatus::OneSidedConfirmed => TransactionStatus::OneSidedConfirmed,
-            TransactionStatus::CoinbaseConfirmed | TransactionStatus::CoinbaseUnconfirmed => {
-                TransactionStatus::CoinbaseConfirmed
-            },
+            TransactionStatus::CoinbaseNotInBlockChain |
+            TransactionStatus::CoinbaseConfirmed |
+            TransactionStatus::CoinbaseUnconfirmed => TransactionStatus::CoinbaseConfirmed,
         }
     }
 
@@ -100,9 +105,9 @@ impl TransactionStatus {
             TransactionStatus::Imported |
             TransactionStatus::OneSidedUnconfirmed |
             TransactionStatus::OneSidedConfirmed => TransactionStatus::OneSidedUnconfirmed,
-            TransactionStatus::CoinbaseConfirmed | TransactionStatus::CoinbaseUnconfirmed => {
-                TransactionStatus::CoinbaseUnconfirmed
-            },
+            TransactionStatus::CoinbaseConfirmed |
+            TransactionStatus::CoinbaseUnconfirmed |
+            TransactionStatus::CoinbaseNotInBlockChain => TransactionStatus::CoinbaseUnconfirmed,
         }
     }
 }
@@ -131,6 +136,7 @@ impl TryFrom<i32> for TransactionStatus {
             10 => Ok(TransactionStatus::Queued),
             11 => Ok(TransactionStatus::CoinbaseUnconfirmed),
             12 => Ok(TransactionStatus::CoinbaseConfirmed),
+            13 => Ok(TransactionStatus::CoinbaseNotInBlockChain),
             code => Err(TransactionConversionError { code }),
         }
     }
@@ -152,6 +158,7 @@ impl Display for TransactionStatus {
             TransactionStatus::OneSidedConfirmed => write!(f, "One-Sided Confirmed"),
             TransactionStatus::CoinbaseUnconfirmed => write!(f, "Coinbase Unconfirmed"),
             TransactionStatus::CoinbaseConfirmed => write!(f, "Coinbase Confirmed"),
+            TransactionStatus::CoinbaseNotInBlockChain => write!(f, "Coinbase not mined"),
             TransactionStatus::Queued => write!(f, "Queued"),
         }
     }
@@ -165,9 +172,9 @@ pub enum ImportStatus {
     OneSidedUnconfirmed,
     /// This transaction import status is used when a one-sided transaction has been scanned and confirmed
     OneSidedConfirmed,
-    /// This transaction import status is used when a one-sided transaction has been scanned but is unconfirmed
+    /// This transaction import status is used when a coinbasetransaction has been scanned but is unconfirmed
     CoinbaseUnconfirmed,
-    /// This transaction import status is used when a one-sided transaction has been scanned and confirmed
+    /// This transaction import status is used when a coinbase transaction has been scanned and confirmed
     CoinbaseConfirmed,
 }
 

base_layer/core/src/base_node/rpc/service.rs

@@ -313,7 +313,7 @@ impl<B: BlockchainBackend + 'static> BaseNodeWalletService for BaseNodeWalletRpc
                 location: response.location,
                 block_hash: response.block_hash,
                 confirmations: response.confirmations,
-                block_height: response.height_of_longest_chain - response.confirmations,
+                block_height: response.height_of_longest_chain.saturating_sub(response.confirmations),
                 mined_timestamp: response.mined_timestamp,
             });
         }

base_layer/wallet/src/transaction_service/protocols/transaction_validation_protocol.rs

@@ -285,7 +285,10 @@ where
                 .map_err(TransactionServiceError::ProtobufConversionError)?;
             let sig = response.signature;
             if let Some(unconfirmed_tx) = batch_signatures.get(&sig) {
-                if response.location == TxLocation::Mined && response.block_hash.is_some() {
+                if response.location == TxLocation::Mined &&
+                    response.block_hash.is_some() &&
+                    response.mined_timestamp.is_some()
+                {
                     mined.push((
                         (*unconfirmed_tx).clone(),
                         response.block_height,
@@ -296,10 +299,8 @@ where
                 } else {
                     warn!(
                         target: LOG_TARGET,
-                        "Marking transaction {} as unmined and confirmed '{}' with block '{}' (Operation ID: {})",
+                        "Transaction {} is unmined (Operation ID: {})",
                         &unconfirmed_tx.tx_id,
-                        response.confirmations >= self.config.num_confirmations_required,
-                        response.block_hash.is_some(),
                         self.operation_id,
                     );
                     unmined.push((*unconfirmed_tx).clone());

base_layer/wallet/src/transaction_service/service.rs

@@ -2530,7 +2530,7 @@ where
             JoinHandle<Result<OperationId, TransactionServiceProtocolError<OperationId>>>,
         >,
     ) -> Result<OperationId, TransactionServiceError> {
-        self.resources.db.mark_all_transactions_as_unvalidated()?;
+        self.resources.db.mark_all_non_coinbases_transactions_as_unvalidated()?;
         self.start_transaction_validation_protocol(join_handles).await
     }
 

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

@@ -136,7 +136,7 @@ pub trait TransactionBackend: Send + Sync + Clone {
     /// Clears the mined block and height of a transaction
     fn set_transaction_as_unmined(&self, tx_id: TxId) -> Result<(), TransactionStorageError>;
     /// Reset optional 'mined height' and 'mined in block' fields to nothing
-    fn mark_all_transactions_as_unvalidated(&self) -> Result<(), TransactionStorageError>;
+    fn mark_all_non_coinbases_transactions_as_unvalidated(&self) -> Result<(), TransactionStorageError>;
     /// Light weight method to retrieve pertinent transaction sender info for all pending inbound transactions
     fn get_pending_inbound_transaction_sender_info(
         &self,
@@ -147,6 +147,10 @@ pub trait TransactionBackend: Send + Sync + Clone {
         &self,
         height: u64,
     ) -> Result<Vec<CompletedTransaction>, TransactionStorageError>;
+    fn fetch_unmined_coinbase_transactions_from_height(
+        &self,
+        height: u64,
+    ) -> Result<Vec<CompletedTransaction>, TransactionStorageError>;
 }
 
 #[derive(Clone, PartialEq)]
@@ -429,6 +433,14 @@ where T: TransactionBackend + 'static
         Ok(t)
     }
 
+    pub fn get_unmined_coinbase_transactions(
+        &self,
+        height: u64,
+    ) -> Result<Vec<CompletedTransaction>, TransactionStorageError> {
+        let t = self.db.fetch_unmined_coinbase_transactions_from_height(height)?;
+        Ok(t)
+    }
+
     pub fn fetch_last_mined_transaction(&self) -> Result<Option<CompletedTransaction>, TransactionStorageError> {
         self.db.fetch_last_mined_transaction()
     }
@@ -683,8 +695,8 @@ where T: TransactionBackend + 'static
         self.db.set_transaction_as_unmined(tx_id)
     }
 
-    pub fn mark_all_transactions_as_unvalidated(&self) -> Result<(), TransactionStorageError> {
-        self.db.mark_all_transactions_as_unvalidated()
+    pub fn mark_all_non_coinbases_transactions_as_unvalidated(&self) -> Result<(), TransactionStorageError> {
+        self.db.mark_all_non_coinbases_transactions_as_unvalidated()
     }
 
     pub fn set_transaction_mined_height(

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

@@ -918,18 +918,23 @@ impl TransactionBackend for TransactionServiceSqliteDatabase {
         Ok(result)
     }
 
-    fn mark_all_transactions_as_unvalidated(&self) -> Result<(), TransactionStorageError> {
+    // Exclude coinbases as they are validated from the OMS service, and we use these fields to know which tx to
+    // extract, thus we should not wipe it out. Coinbases can also not be mined in a different height so the data will
+    // never be wrong.
+    fn mark_all_non_coinbases_transactions_as_unvalidated(&self) -> Result<(), TransactionStorageError> {
         let start = Instant::now();
         let mut conn = self.database_connection.get_pooled_connection()?;
         let acquire_lock = start.elapsed();
         let result = diesel::update(completed_transactions::table)
+            .filter(completed_transactions::status.ne(TransactionStatus::CoinbaseNotInBlockChain as i32))
+            .filter(completed_transactions::status.ne(TransactionStatus::CoinbaseUnconfirmed as i32))
+            .filter(completed_transactions::status.ne(TransactionStatus::CoinbaseConfirmed as i32))
             .set((
                 completed_transactions::cancelled.eq::<Option<i32>>(None),
                 completed_transactions::mined_height.eq::<Option<i64>>(None),
                 completed_transactions::mined_in_block.eq::<Option<Vec<u8>>>(None),
             ))
             .execute(&mut conn)?;
-
         trace!(target: LOG_TARGET, "rows updated: {:?}", result);
         if start.elapsed().as_millis() > 0 {
             trace!(
@@ -1035,6 +1040,27 @@ impl TransactionBackend for TransactionServiceSqliteDatabase {
         Ok(coinbases)
     }
 
+    fn fetch_unmined_coinbase_transactions_from_height(
+        &self,
+        height: u64,
+    ) -> Result<Vec<CompletedTransaction>, TransactionStorageError> {
+        let mut conn = self.database_connection.get_pooled_connection()?;
+        let cipher = acquire_read_lock!(self.cipher);
+
+        let coinbases = CompletedTransactionSql::index_by_status_and_cancelled_from_block_height(
+            TransactionStatus::CoinbaseNotInBlockChain,
+            false,
+            height as i64,
+            &mut conn,
+        )?
+        .into_iter()
+        .map(|ct: CompletedTransactionSql| {
+            CompletedTransaction::try_from(ct, &cipher).map_err(TransactionStorageError::from)
+        })
+        .collect::<Result<Vec<CompletedTransaction>, TransactionStorageError>>()?;
+        Ok(coinbases)
+    }
+
     fn fetch_confirmed_detected_transactions_from_height(
         &self,
         height: u64,
@@ -1849,37 +1875,37 @@ impl CompletedTransactionSql {
     }
 
     pub fn set_as_unmined(tx_id: TxId, conn: &mut SqliteConnection) -> Result<(), TransactionStorageError> {
-        // This query uses two sub-queries to retrieve existing values in the table
+        let (current_status, current_mined_height) = *completed_transactions::table
+            .filter(completed_transactions::tx_id.eq(tx_id.as_u64() as i64))
+            .select((completed_transactions::status, completed_transactions::mined_height))
+            .load::<(i32, Option<i64>)>(conn)?
+            .first()
+            .ok_or(TransactionStorageError::DieselError(DieselError::NotFound))?;
+        let current_status = TransactionStatus::try_from(current_status)
+            .map_err(|_| TransactionStorageError::UnexpectedResult("Unknown status".to_string()))?;
         diesel::update(completed_transactions::table.filter(completed_transactions::tx_id.eq(tx_id.as_u64() as i64)))
             .set(UpdateCompletedTransactionSql {
-                status: {
-                    if let Some(status) = completed_transactions::table
-                        .filter(completed_transactions::tx_id.eq(tx_id.as_u64() as i64))
-                        .select(completed_transactions::status)
-                        .load::<i32>(conn)?
-                        .first()
-                    {
-                        if *status == TransactionStatus::OneSidedConfirmed as i32 ||
-                            *status == TransactionStatus::OneSidedUnconfirmed as i32
-                        {
-                            Some(TransactionStatus::OneSidedUnconfirmed as i32)
-                        } else if *status == TransactionStatus::CoinbaseUnconfirmed as i32 ||
-                            *status == TransactionStatus::CoinbaseConfirmed as i32
-                        {
-                            Some(TransactionStatus::CoinbaseUnconfirmed as i32)
-                        } else if *status == TransactionStatus::Imported as i32 {
-                            Some(TransactionStatus::Imported as i32)
-                        } else if *status == TransactionStatus::Broadcast as i32 {
-                            Some(TransactionStatus::Broadcast as i32)
-                        } else {
-                            Some(TransactionStatus::Completed as i32)
-                        }
+                status: match current_status {
+                    TransactionStatus::OneSidedConfirmed | TransactionStatus::OneSidedUnconfirmed => {
+                        Some(TransactionStatus::OneSidedUnconfirmed as i32)
+                    },
+                    TransactionStatus::CoinbaseUnconfirmed |
+                    TransactionStatus::CoinbaseConfirmed |
+                    TransactionStatus::CoinbaseNotInBlockChain => {
+                        Some(TransactionStatus::CoinbaseNotInBlockChain as i32)
+                    },
+                    TransactionStatus::Imported => Some(TransactionStatus::Imported as i32),
+                    TransactionStatus::Broadcast => Some(TransactionStatus::Broadcast as i32),
+                    _ => Some(TransactionStatus::Completed as i32),
+                },
+                mined_in_block: Some(None),
+                mined_height: {
+                    if current_status.is_coinbase() {
+                        Some(current_mined_height)
                     } else {
-                        return Err(TransactionStorageError::DieselError(DieselError::NotFound));
+                        Some(None)
                     }
                 },
-                mined_in_block: Some(None),
-                mined_height: Some(None),
                 confirmations: Some(None),
                 // Turns out it should not be cancelled
                 cancelled: Some(None),
@@ -2109,6 +2135,7 @@ impl UnconfirmedTransactionInfoSql {
                     .and(completed_transactions::status.ne(TransactionStatus::OneSidedConfirmed as i32))
                     .and(completed_transactions::status.ne(TransactionStatus::CoinbaseUnconfirmed as i32))
                     .and(completed_transactions::status.ne(TransactionStatus::CoinbaseConfirmed as i32))
+                    .and(completed_transactions::status.ne(TransactionStatus::CoinbaseNotInBlockChain as i32))
                     // Filter out any transaction without a kernel signature
                     .and(completed_transactions::transaction_signature_nonce.ne(vec![0u8; 32]))
                     .and(completed_transactions::transaction_signature_key.ne(vec![0u8; 32]))