feat: update coinbase handling for new tx and output validation (#3383)

* feat: update coinbase handling for new tx and output validation 

The new tx validation protocol would continuously try and revalidate abandoned coinbase transactions and the TXO validation would try validate abandoned or reforged Coinbases indefinitely.

This PR updates the Tx validation handing of Coinbases to give them the Coinbase status when they are abandoned which means they won’t be revalidated but will still be checked during a reorg. The TXO validation has been updated so that ab abandoned coinbase can be marked as Abandoned when the transaction validation detects the abandonment so that the output will not be validated indefinitely.

A new test is provided and a number of cucumber tests for this branch have been fixed.

* Fix typo

applications/ffi_client/index.js

@@ -195,9 +195,9 @@ try {
       let id = lib.wallet_start_transaction_validation(wallet, err);
       console.log("tx validation request id", id);
 
-      console.log("start utxo validation");
-      id = lib.wallet_start_utxo_validation(wallet, err);
-      console.log("utxo validation request id", id);
+      console.log("start txo validation");
+      id = lib.wallet_start_txo_validation(wallet, err);
+      console.log("txo validation request id", id);
     } catch (e) {
       console.error("validation error: ", e);
     }

applications/ffi_client/lib/index.js

@@ -74,7 +74,7 @@ const libWallet = ffi.Library("./libtari_wallet_ffi.dylib", {
   wallet_get_num_confirmations_required: [u64, [walletRef, errPtr]],
   wallet_set_num_confirmations_required: ["void", [walletRef, u64, errPtr]],
   wallet_start_transaction_validation: [u64, [walletRef, errPtr]],
-  wallet_start_utxo_validation: [u64, [walletRef, errPtr]],
+  wallet_start_txo_validation: [u64, [walletRef, errPtr]],
   wallet_start_recovery: [bool, [walletRef, publicKeyRef, fn, errPtr]],
 });
 

base_layer/wallet/src/output_manager_service/handle.rs

@@ -69,6 +69,7 @@ pub enum OutputManagerRequest {
     ScanOutputs(Vec<TransactionOutput>),
     AddKnownOneSidedPaymentScript(KnownOneSidedPaymentScript),
     ReinstateCancelledInboundTx(TxId),
+    SetCoinbaseAbandoned(TxId, bool),
 }
 
 impl fmt::Display for OutputManagerRequest {
@@ -106,6 +107,7 @@ impl fmt::Display for OutputManagerRequest {
             ScanOutputs(_) => write!(f, "ScanOutputs"),
             AddKnownOneSidedPaymentScript(_) => write!(f, "AddKnownOneSidedPaymentScript"),
             ReinstateCancelledInboundTx(_) => write!(f, "ReinstateCancelledInboundTx"),
+            SetCoinbaseAbandoned(_, _) => write!(f, "SetCoinbaseAbandoned"),
         }
     }
 }
@@ -128,7 +130,7 @@ pub enum OutputManagerResponse {
     InvalidOutputs(Vec<UnblindedOutput>),
     SeedWords(Vec<String>),
     BaseNodePublicKeySet,
-    UtxoValidationStarted(u64),
+    TxoValidationStarted(u64),
     Transaction((u64, Transaction, MicroTari, MicroTari)),
     EncryptionApplied,
     EncryptionRemoved,
@@ -138,6 +140,7 @@ pub enum OutputManagerResponse {
     ScanOutputs(Vec<UnblindedOutput>),
     AddKnownOneSidedPaymentScript,
     ReinstatedCancelledInboundTx,
+    CoinbaseAbandonedSet,
 }
 
 pub type OutputManagerEventSender = broadcast::Sender<Arc<OutputManagerEvent>>;
@@ -385,7 +388,7 @@ impl OutputManagerHandle {
 
     pub async fn validate_txos(&mut self) -> Result<u64, OutputManagerError> {
         match self.handle.call(OutputManagerRequest::ValidateUtxos).await?? {
-            OutputManagerResponse::UtxoValidationStarted(request_key) => Ok(request_key),
+            OutputManagerResponse::TxoValidationStarted(request_key) => Ok(request_key),
             _ => Err(OutputManagerError::UnexpectedApiResponse),
         }
     }
@@ -501,4 +504,15 @@ impl OutputManagerHandle {
             _ => Err(OutputManagerError::UnexpectedApiResponse),
         }
     }
+
+    pub async fn set_coinbase_abandoned(&mut self, tx_id: TxId, abandoned: bool) -> Result<(), OutputManagerError> {
+        match self
+            .handle
+            .call(OutputManagerRequest::SetCoinbaseAbandoned(tx_id, abandoned))
+            .await??
+        {
+            OutputManagerResponse::CoinbaseAbandonedSet => Ok(()),
+            _ => Err(OutputManagerError::UnexpectedApiResponse),
+        }
+    }
 }

base_layer/wallet/src/output_manager_service/service.rs

@@ -276,9 +276,9 @@ where TBackend: OutputManagerBackend + 'static
                 .set_base_node_public_key(pk)
                 .await
                 .map(|_| OutputManagerResponse::BaseNodePublicKeySet),
-            OutputManagerRequest::ValidateUtxos => self
-                .validate_outputs()
-                .map(OutputManagerResponse::UtxoValidationStarted),
+            OutputManagerRequest::ValidateUtxos => {
+                self.validate_outputs().map(OutputManagerResponse::TxoValidationStarted)
+            },
             OutputManagerRequest::GetInvalidOutputs => {
                 let outputs = self
                     .fetch_invalid_outputs()
@@ -330,6 +330,10 @@ where TBackend: OutputManagerBackend + 'static
                 .reinstate_cancelled_inbound_transaction(tx_id)
                 .await
                 .map(|_| OutputManagerResponse::ReinstatedCancelledInboundTx),
+            OutputManagerRequest::SetCoinbaseAbandoned(tx_id, abandoned) => self
+                .set_coinbase_abandoned(tx_id, abandoned)
+                .await
+                .map(|_| OutputManagerResponse::CoinbaseAbandonedSet),
         }
     }
 
@@ -1003,6 +1007,11 @@ where TBackend: OutputManagerBackend + 'static
         Ok(self.resources.db.get_invalid_outputs().await?)
     }
 
+    pub async fn set_coinbase_abandoned(&self, tx_id: TxId, abandoned: bool) -> Result<(), OutputManagerError> {
+        self.resources.db.set_coinbase_abandoned(tx_id, abandoned).await?;
+        Ok(())
+    }
+
     async fn create_coin_split(
         &mut self,
         amount_per_split: MicroTari,

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

@@ -115,6 +115,8 @@ pub trait OutputManagerBackend: Send + Sync + Clone {
         &self,
         block_height: u64,
     ) -> Result<(), OutputManagerStorageError>;
+    /// Set if a coinbase output is abandoned or not
+    fn set_coinbase_abandoned(&self, tx_id: TxId, abandoned: bool) -> Result<(), OutputManagerStorageError>;
 }
 
 /// Holds the state of the KeyManager being used by the Output Manager Service
@@ -595,7 +597,7 @@ where T: OutputManagerBackend + 'static
         Ok(())
     }
 
-    pub async fn set_output_as_unmined(&self, hash: HashOutput) -> Result<(), OutputManagerStorageError> {
+    pub async fn set_output_to_unmined(&self, hash: HashOutput) -> Result<(), OutputManagerStorageError> {
         let db = self.db.clone();
         tokio::task::spawn_blocking(move || db.set_output_to_unmined(hash))
             .await
@@ -624,6 +626,14 @@ where T: OutputManagerBackend + 'static
             .map_err(|err| OutputManagerStorageError::BlockingTaskSpawnError(err.to_string()))??;
         Ok(())
     }
+
+    pub async fn set_coinbase_abandoned(&self, tx_id: TxId, abandoned: bool) -> Result<(), OutputManagerStorageError> {
+        let db = self.db.clone();
+        tokio::task::spawn_blocking(move || db.set_coinbase_abandoned(tx_id, abandoned))
+            .await
+            .map_err(|err| OutputManagerStorageError::BlockingTaskSpawnError(err.to_string()))??;
+        Ok(())
+    }
 }
 
 fn unexpected_result<T>(req: DbKey, res: DbValue) -> Result<T, OutputManagerStorageError> {

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

@@ -131,4 +131,5 @@ pub enum OutputStatus {
     ShortTermEncumberedToBeReceived,
     ShortTermEncumberedToBeSpent,
     SpentMinedUnconfirmed,
+    AbandonedCoinbase,
 }

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

@@ -486,6 +486,31 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase {
         Ok(())
     }
 
+    fn set_coinbase_abandoned(&self, tx_id: TxId, abandoned: bool) -> Result<(), OutputManagerStorageError> {
+        let conn = self.database_connection.acquire_lock();
+
+        debug!(target: LOG_TARGET, "set_coinbase_abandoned(TxID: {})", tx_id);
+
+        let status = if abandoned {
+            OutputStatus::AbandonedCoinbase as i32
+        } else {
+            OutputStatus::EncumberedToBeReceived as i32
+        };
+
+        diesel::update(
+            outputs::table.filter(
+                outputs::received_in_tx_id
+                    .eq(Some(tx_id as i64))
+                    .and(outputs::coinbase_block_height.is_not_null()),
+            ),
+        )
+        .set((outputs::status.eq(status),))
+        .execute(&(*conn))
+        .num_rows_affected_or_not_found(1)?;
+
+        Ok(())
+    }
+
     fn short_term_encumber_outputs(
         &self,
         tx_id: u64,

base_layer/wallet/src/output_manager_service/tasks/txo_validation_task.rs

@@ -44,7 +44,7 @@ use tari_core::{
 use tari_crypto::tari_utilities::{hex::Hex, Hashable};
 use tari_shutdown::ShutdownSignal;
 
-const LOG_TARGET: &str = "wallet::output_service::txo_validation_task_v2";
+const LOG_TARGET: &str = "wallet::output_service::txo_validation_task";
 
 pub struct TxoValidationTask<TBackend: OutputManagerBackend + 'static> {
     base_node_pk: CommsPublicKey,
@@ -233,16 +233,16 @@ where TBackend: OutputManagerBackend + 'static
                 mined.len(),
                 unmined.len()
             );
-            for (tx, mined_height, mined_in_block, mmr_position) in &mined {
+            for (output, mined_height, mined_in_block, mmr_position) in &mined {
                 info!(
                     target: LOG_TARGET,
                     "Updating output comm:{}: hash {} as mined at height {} with current tip at {}",
-                    tx.commitment.to_hex(),
-                    tx.hash.to_hex(),
+                    output.commitment.to_hex(),
+                    output.hash.to_hex(),
                     mined_height,
                     tip_height
                 );
-                self.update_output_as_mined(&tx, mined_in_block, *mined_height, *mmr_position, tip_height)
+                self.update_output_as_mined(&output, mined_in_block, *mined_height, *mmr_position, tip_height)
                     .await?;
             }
         }
@@ -341,7 +341,7 @@ where TBackend: OutputManagerBackend + 'static
                     last_mined_output.commitment.to_hex()
                 );
                 self.db
-                    .set_output_as_unmined(last_mined_output.hash.clone())
+                    .set_output_to_unmined(last_mined_output.hash.clone())
                     .await
                     .for_protocol(self.operation_id)?;
             } else {

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

@@ -20,13 +20,16 @@
 // 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::transaction_service::{
-    config::TransactionServiceConfig,
-    error::{TransactionServiceError, TransactionServiceProtocolError, TransactionServiceProtocolErrorExt},
-    handle::{TransactionEvent, TransactionEventSender},
-    storage::{
-        database::{TransactionBackend, TransactionDatabase},
-        models::CompletedTransaction,
+use crate::{
+    output_manager_service::handle::OutputManagerHandle,
+    transaction_service::{
+        config::TransactionServiceConfig,
+        error::{TransactionServiceError, TransactionServiceProtocolError, TransactionServiceProtocolErrorExt},
+        handle::{TransactionEvent, TransactionEventSender},
+        storage::{
+            database::{TransactionBackend, TransactionDatabase},
+            models::{CompletedTransaction, TransactionStatus},
+        },
     },
 };
 use log::*;
@@ -60,6 +63,7 @@ pub struct TransactionValidationProtocol<TTransactionBackend: TransactionBackend
     connectivity_requester: ConnectivityRequester,
     config: TransactionServiceConfig,
     event_publisher: TransactionEventSender,
+    output_manager_handle: OutputManagerHandle,
 }
 
 #[allow(unused_variables)]
@@ -71,6 +75,7 @@ impl<TTransactionBackend: TransactionBackend + 'static> TransactionValidationPro
         connectivity_requester: ConnectivityRequester,
         config: TransactionServiceConfig,
         event_publisher: TransactionEventSender,
+        output_manager_handle: OutputManagerHandle,
     ) -> Self {
         Self {
             operation_id,
@@ -79,6 +84,7 @@ impl<TTransactionBackend: TransactionBackend + 'static> TransactionValidationPro
             connectivity_requester,
             config,
             event_publisher,
+            output_manager_handle,
         }
     }
 
@@ -298,7 +304,6 @@ impl<TTransactionBackend: TransactionBackend + 'static> TransactionValidationPro
                 }
             }
         }
-
         Ok((
             mined,
             unmined,
@@ -343,7 +348,7 @@ impl<TTransactionBackend: TransactionBackend + 'static> TransactionValidationPro
 
     #[allow(clippy::ptr_arg)]
     async fn update_transaction_as_mined(
-        &self,
+        &mut self,
         tx: &CompletedTransaction,
         mined_in_block: &BlockHash,
         mined_height: u64,
@@ -374,12 +379,21 @@ impl<TTransactionBackend: TransactionBackend + 'static> TransactionValidationPro
             })
         }
 
+        if tx.status == TransactionStatus::Coinbase {
+            if let Err(e) = self.output_manager_handle.set_coinbase_abandoned(tx.tx_id, false).await {
+                warn!(
+                    target: LOG_TARGET,
+                    "Could not mark coinbase output for TxId: {} as not abandoned: {}", tx.tx_id, e
+                );
+            };
+        }
+
         Ok(())
     }
 
     #[allow(clippy::ptr_arg)]
     async fn update_coinbase_as_abandoned(
-        &self,
+        &mut self,
         tx: &CompletedTransaction,
         mined_in_block: &BlockHash,
         mined_height: u64,
@@ -397,20 +411,36 @@ impl<TTransactionBackend: TransactionBackend + 'static> TransactionValidationPro
             .await
             .for_protocol(self.operation_id)?;
 
+        if let Err(e) = self.output_manager_handle.set_coinbase_abandoned(tx.tx_id, true).await {
+            warn!(
+                target: LOG_TARGET,
+                "Could not mark coinbase output for TxId: {} as abandoned: {}", tx.tx_id, e
+            );
+        };
+
         self.publish_event(TransactionEvent::TransactionCancelled(tx.tx_id));
 
         Ok(())
     }
 
     async fn update_transaction_as_unmined(
-        &self,
+        &mut self,
         tx: &CompletedTransaction,
     ) -> Result<(), TransactionServiceProtocolError> {
         self.db
             .set_transaction_as_unmined(tx.tx_id)
             .await
             .for_protocol(self.operation_id)?;
 
+        if tx.status == TransactionStatus::Coinbase {
+            if let Err(e) = self.output_manager_handle.set_coinbase_abandoned(tx.tx_id, false).await {
+                warn!(
+                    target: LOG_TARGET,
+                    "Could not mark coinbase output for TxId: {} as not abandoned: {}", tx.tx_id, e
+                );
+            };
+        }
+
         self.publish_event(TransactionEvent::TransactionBroadcast(tx.tx_id));
         Ok(())
     }

base_layer/wallet/src/transaction_service/service.rs

@@ -1588,6 +1588,7 @@ where
                     self.resources.connectivity_manager.clone(),
                     self.resources.config.clone(),
                     self.event_publisher.clone(),
+                    self.resources.output_manager_service.clone(),
                 );
 
                 let join_handle = tokio::spawn(protocol.execute());

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

@@ -1371,14 +1371,18 @@ impl CompletedTransactionSql {
         is_confirmed: bool,
         conn: &SqliteConnection,
     ) -> Result<(), TransactionStorageError> {
+        let status = if self.coinbase_block_height.is_some() && !is_valid {
+            TransactionStatus::Coinbase as i32
+        } else if is_confirmed {
+            TransactionStatus::MinedConfirmed as i32
+        } else {
+            TransactionStatus::MinedUnconfirmed as i32
+        };
+
         self.update(
             UpdateCompletedTransactionSql {
                 confirmations: Some(Some(num_confirmations as i64)),
-                status: Some(if is_confirmed {
-                    TransactionStatus::MinedConfirmed as i32
-                } else {
-                    TransactionStatus::MinedUnconfirmed as i32
-                }),
+                status: Some(status),
                 mined_height: Some(Some(mined_height as i64)),
                 mined_in_block: Some(Some(mined_in_block)),
                 valid: Some(is_valid as i32),