fix!: fix tx and txo validation callback bug in wallet FFI (#3695)

Description
---

A recent change to the transaction service events meant that the Transaction Validation callback was only being called when state changed and not on success like it used to.
It was also noted that the callbacks no longer reported when a validation failed.

This PR updates the Tx and TXO validation callbacks to be called on completion of the respective validation protocols and also report if it was a success or not. Due to the simplification of the validation protocols the callbacks were simplified so that the feedback is a boolean denoting success or failure.

How Has This Been Tested?
---
unit tests updated

base_layer/wallet/src/output_manager_service/handle.rs

@@ -232,32 +232,20 @@ pub type OutputManagerEventReceiver = broadcast::Receiver<Arc<OutputManagerEvent
 /// Events that can be published on the Output Manager Service Event Stream
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub enum OutputManagerEvent {
-    TxoValidationTimedOut(u64),
     TxoValidationSuccess(u64),
     TxoValidationFailure(u64),
-    TxoValidationAborted(u64),
-    TxoValidationDelayed(u64),
     Error(String),
 }
 
 impl fmt::Display for OutputManagerEvent {
     fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
         match self {
-            OutputManagerEvent::TxoValidationTimedOut(tx) => {
-                write!(f, "TxoValidationTimedOut for {}", tx)
-            },
             OutputManagerEvent::TxoValidationSuccess(tx) => {
                 write!(f, "TxoValidationSuccess for {}", tx)
             },
             OutputManagerEvent::TxoValidationFailure(tx) => {
                 write!(f, "TxoValidationFailure for {}", tx)
             },
-            OutputManagerEvent::TxoValidationAborted(tx) => {
-                write!(f, "TxoValidationAborted for {}", tx)
-            },
-            OutputManagerEvent::TxoValidationDelayed(tx) => {
-                write!(f, "TxoValidationDelayed for {}", tx)
-            },
             OutputManagerEvent::Error(error) => {
                 write!(f, "Error {}", error)
             },

base_layer/wallet/src/output_manager_service/service.rs

@@ -71,7 +71,7 @@ use crate::{
     output_manager_service::{
         config::OutputManagerServiceConfig,
         error::{OutputManagerError, OutputManagerProtocolError, OutputManagerStorageError},
-        handle::{OutputManagerEventSender, OutputManagerRequest, OutputManagerResponse},
+        handle::{OutputManagerEvent, OutputManagerEventSender, OutputManagerRequest, OutputManagerResponse},
         recovery::StandardUtxoRecoverer,
         resources::OutputManagerResources,
         storage::{
@@ -474,7 +474,7 @@ where
         );
 
         let shutdown = self.resources.shutdown_signal.clone();
-
+        let event_publisher = self.resources.event_publisher.clone();
         tokio::spawn(async move {
             match utxo_validation.execute(shutdown).await {
                 Ok(id) => {
@@ -488,6 +488,12 @@ where
                         target: LOG_TARGET,
                         "Error completing UTXO Validation Protocol (Id: {}): {:?}", id, error
                     );
+                    if let Err(e) = event_publisher.send(Arc::new(OutputManagerEvent::TxoValidationFailure(id))) {
+                        debug!(
+                            target: LOG_TARGET,
+                            "Error sending event because there are no subscribers: {:?}", e
+                        );
+                    }
                 },
             }
         });

base_layer/wallet/src/transaction_service/handle.rs

@@ -201,6 +201,8 @@ pub enum TransactionEvent {
         is_valid: bool,
     },
     TransactionValidationStateChanged(OperationId),
+    TransactionValidationCompleted(OperationId),
+    TransactionValidationFailed(OperationId),
     Error(String),
 }
 
@@ -263,6 +265,12 @@ impl fmt::Display for TransactionEvent {
             TransactionEvent::TransactionValidationStateChanged(operation_id) => {
                 write!(f, "Transaction validation state changed: {}", operation_id)
             },
+            TransactionEvent::TransactionValidationCompleted(operation_id) => {
+                write!(f, "Transaction validation completed: {}", operation_id)
+            },
+            TransactionEvent::TransactionValidationFailed(operation_id) => {
+                write!(f, "Transaction validation failed: {}", operation_id)
+            },
         }
     }
 }

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

@@ -191,6 +191,7 @@ where
         if state_changed {
             self.publish_event(TransactionEvent::TransactionValidationStateChanged(self.operation_id));
         }
+        self.publish_event(TransactionEvent::TransactionValidationCompleted(self.operation_id));
         Ok(self.operation_id)
     }
 

base_layer/wallet/src/transaction_service/service.rs

@@ -1860,7 +1860,9 @@ where
                 );
                 let _ = self
                     .event_publisher
-                    .send(Arc::new(TransactionEvent::Error(format!("{:?}", error))));
+                    .send(Arc::new(TransactionEvent::TransactionValidationFailed(
+                        OperationId::from(id),
+                    )));
             },
         }
     }

base_layer/wallet_ffi/src/callback_handler.rs

@@ -69,14 +69,6 @@ use tari_wallet::{
 
 const LOG_TARGET: &str = "wallet::transaction_service::callback_handler";
 
-#[derive(Clone, Copy)]
-enum CallbackValidationResults {
-    Success,           // 0
-    Aborted,           // 1
-    Failure,           // 2
-    BaseNodeNotInSync, // 3
-}
-
 pub struct CallbackHandler<TBackend>
 where TBackend: TransactionBackend + 'static
 {
@@ -89,9 +81,9 @@ where TBackend: TransactionBackend + 'static
     callback_direct_send_result: unsafe extern "C" fn(u64, bool),
     callback_store_and_forward_send_result: unsafe extern "C" fn(u64, bool),
     callback_transaction_cancellation: unsafe extern "C" fn(*mut CompletedTransaction, u64),
-    callback_txo_validation_complete: unsafe extern "C" fn(u64, u8),
+    callback_txo_validation_complete: unsafe extern "C" fn(u64, bool),
     callback_balance_updated: unsafe extern "C" fn(*mut Balance),
-    callback_transaction_validation_complete: unsafe extern "C" fn(u64, u8),
+    callback_transaction_validation_complete: unsafe extern "C" fn(u64, bool),
     callback_saf_messages_received: unsafe extern "C" fn(),
     db: TransactionDatabase<TBackend>,
     transaction_service_event_stream: TransactionEventReceiver,
@@ -124,9 +116,9 @@ where TBackend: TransactionBackend + 'static
         callback_direct_send_result: unsafe extern "C" fn(u64, bool),
         callback_store_and_forward_send_result: unsafe extern "C" fn(u64, bool),
         callback_transaction_cancellation: unsafe extern "C" fn(*mut CompletedTransaction, u64),
-        callback_txo_validation_complete: unsafe extern "C" fn(u64, u8),
+        callback_txo_validation_complete: unsafe extern "C" fn(u64, bool),
         callback_balance_updated: unsafe extern "C" fn(*mut Balance),
-        callback_transaction_validation_complete: unsafe extern "C" fn(u64, u8),
+        callback_transaction_validation_complete: unsafe extern "C" fn(u64, bool),
         callback_saf_messages_received: unsafe extern "C" fn(),
     ) -> Self {
         info!(
@@ -258,10 +250,15 @@ where TBackend: TransactionBackend + 'static
                                     self.receive_transaction_mined_unconfirmed_event(tx_id, num_confirmations).await;
                                     self.trigger_balance_refresh().await;
                                 },
-                                TransactionEvent::TransactionValidationStateChanged(request_key)  => {
-                                    self.transaction_validation_complete_event(request_key.as_u64());
+                                TransactionEvent::TransactionValidationStateChanged(_request_key)  => {
                                     self.trigger_balance_refresh().await;
                                 },
+                                TransactionEvent::TransactionValidationCompleted(request_key)  => {
+                                    self.transaction_validation_complete_event(request_key.as_u64(), true);
+                                },
+                                TransactionEvent::TransactionValidationFailed(request_key)  => {
+                                    self.transaction_validation_complete_event(request_key.as_u64(), false);
+                                },
                                 TransactionEvent::TransactionMinedRequestTimedOut(_tx_id) |
                                 TransactionEvent::TransactionImported(_tx_id) |
                                 TransactionEvent::TransactionCompletedImmediately(_tx_id)
@@ -281,17 +278,11 @@ where TBackend: TransactionBackend + 'static
                             trace!(target: LOG_TARGET, "Output Manager Service Callback Handler event {:?}", msg);
                             match (*msg).clone() {
                                 OutputManagerEvent::TxoValidationSuccess(request_key) => {
-                                    self.output_validation_complete_event(request_key,  CallbackValidationResults::Success);
+                                    self.output_validation_complete_event(request_key,  true);
                                     self.trigger_balance_refresh().await;
                                 },
                                 OutputManagerEvent::TxoValidationFailure(request_key) => {
-                                    self.output_validation_complete_event(request_key,  CallbackValidationResults::Failure);
-                                },
-                                OutputManagerEvent::TxoValidationAborted(request_key) => {
-                                    self.output_validation_complete_event(request_key,  CallbackValidationResults::Aborted);
-                                },
-                                OutputManagerEvent::TxoValidationDelayed(request_key) => {
-                                    self.output_validation_complete_event(request_key,  CallbackValidationResults::BaseNodeNotInSync);
+                                    self.output_validation_complete_event(request_key,  false);
                                 },
                                 // Only the above variants are mapped to callbacks
                                 _ => (),
@@ -496,40 +487,26 @@ where TBackend: TransactionBackend + 'static
         }
     }
 
-    fn transaction_validation_complete_event(&mut self, request_key: u64) {
+    fn transaction_validation_complete_event(&mut self, request_key: u64, success: bool) {
         debug!(
             target: LOG_TARGET,
             "Calling Transaction Validation Complete callback function for Request Key: {}", request_key,
         );
         unsafe {
-            (self.callback_transaction_validation_complete)(request_key, CallbackValidationResults::Success as u8);
+            (self.callback_transaction_validation_complete)(request_key, success);
         }
     }
 
-    fn output_validation_complete_event(&mut self, request_key: u64, result: CallbackValidationResults) {
+    fn output_validation_complete_event(&mut self, request_key: u64, success: bool) {
         debug!(
             target: LOG_TARGET,
-            "Calling Output Validation Complete callback function for Request Key: {} with result {:?}",
+            "Calling Output Validation Complete callback function for Request Key: {} with success = {:?}",
             request_key,
-            result as u8,
+            success,
         );
 
-        match result {
-            CallbackValidationResults::Success => unsafe {
-                (self.callback_txo_validation_complete)(request_key, CallbackValidationResults::Success as u8);
-            },
-            CallbackValidationResults::Aborted => unsafe {
-                (self.callback_txo_validation_complete)(request_key, CallbackValidationResults::Aborted as u8);
-            },
-            CallbackValidationResults::Failure => unsafe {
-                (self.callback_txo_validation_complete)(request_key, CallbackValidationResults::Failure as u8);
-            },
-            CallbackValidationResults::BaseNodeNotInSync => unsafe {
-                (self.callback_txo_validation_complete)(
-                    request_key,
-                    CallbackValidationResults::BaseNodeNotInSync as u8,
-                );
-            },
+        unsafe {
+            (self.callback_txo_validation_complete)(request_key, success);
         }
     }
 

base_layer/wallet_ffi/src/callback_handler_tests.rs

@@ -181,7 +181,7 @@ mod test {
         Box::from_raw(tx);
     }
 
-    unsafe extern "C" fn txo_validation_complete_callback(_tx_id: u64, result: u8) {
+    unsafe extern "C" fn txo_validation_complete_callback(_tx_id: u64, result: bool) {
         let mut lock = CALLBACK_STATE.lock().unwrap();
         lock.callback_txo_validation_complete += result as u32;
         drop(lock);
@@ -194,7 +194,7 @@ mod test {
         Box::from_raw(balance);
     }
 
-    unsafe extern "C" fn transaction_validation_complete_callback(request_key: u64, _result: u8) {
+    unsafe extern "C" fn transaction_validation_complete_callback(request_key: u64, _result: bool) {
         let mut lock = CALLBACK_STATE.lock().unwrap();
         lock.callback_transaction_validation_complete += request_key as u32;
         drop(lock);
@@ -485,58 +485,22 @@ mod test {
             )))
             .unwrap();
 
-        oms_event_sender
-            .send(Arc::new(OutputManagerEvent::TxoValidationFailure(1u64)))
-            .unwrap();
-
-        oms_event_sender
-            .send(Arc::new(OutputManagerEvent::TxoValidationFailure(1u64)))
-            .unwrap();
-
-        oms_event_sender
-            .send(Arc::new(OutputManagerEvent::TxoValidationFailure(1u64)))
-            .unwrap();
-
         transaction_event_sender
             .send(Arc::new(TransactionEvent::TransactionValidationStateChanged(
                 2u64.into(),
             )))
             .unwrap();
 
         oms_event_sender
-            .send(Arc::new(OutputManagerEvent::TxoValidationAborted(1u64)))
-            .unwrap();
-
-        oms_event_sender
-            .send(Arc::new(OutputManagerEvent::TxoValidationAborted(1u64)))
-            .unwrap();
-
-        oms_event_sender
-            .send(Arc::new(OutputManagerEvent::TxoValidationAborted(1u64)))
+            .send(Arc::new(OutputManagerEvent::TxoValidationFailure(1u64)))
             .unwrap();
 
         transaction_event_sender
-            .send(Arc::new(TransactionEvent::TransactionValidationStateChanged(
-                3u64.into(),
-            )))
-            .unwrap();
-
-        oms_event_sender
-            .send(Arc::new(OutputManagerEvent::TxoValidationDelayed(1u64)))
-            .unwrap();
-
-        oms_event_sender
-            .send(Arc::new(OutputManagerEvent::TxoValidationDelayed(1u64)))
-            .unwrap();
-
-        oms_event_sender
-            .send(Arc::new(OutputManagerEvent::TxoValidationDelayed(1u64)))
+            .send(Arc::new(TransactionEvent::TransactionValidationCompleted(3u64.into())))
             .unwrap();
 
         transaction_event_sender
-            .send(Arc::new(TransactionEvent::TransactionValidationStateChanged(
-                4u64.into(),
-            )))
+            .send(Arc::new(TransactionEvent::TransactionValidationCompleted(4u64.into())))
             .unwrap();
 
         dht_event_sender
@@ -558,9 +522,9 @@ mod test {
         assert!(lock.tx_cancellation_callback_called_completed);
         assert!(lock.tx_cancellation_callback_called_outbound);
         assert!(lock.saf_messages_received);
-        assert_eq!(lock.callback_txo_validation_complete, 18);
+        assert_eq!(lock.callback_txo_validation_complete, 3);
         assert_eq!(lock.callback_balance_updated, 5);
-        assert_eq!(lock.callback_transaction_validation_complete, 10);
+        assert_eq!(lock.callback_transaction_validation_complete, 7);
 
         drop(lock);
     }

base_layer/wallet_ffi/src/lib.rs

@@ -3144,12 +3144,12 @@ unsafe fn init_logging(
 /// }
 /// `callback_txo_validation_complete` - The callback function pointer matching the function signature. This is called
 /// when a TXO validation process is completed. The request_key is used to identify which request this
-/// callback references and the second parameter is a u8 that represent the ClassbackValidationResults enum.
+/// callback references and the second parameter is a is a bool that returns if the validation was successful or not.
 /// `callback_balance_updated` - The callback function pointer matching the function signature. This is called whenever
 /// the balance changes.
 /// `callback_transaction_validation_complete` - The callback function pointer matching the function signature. This is
 /// called when a Transaction validation process is completed. The request_key is used to identify which request this
-/// callback references and the second parameter is a u8 that represent the ClassbackValidationResults enum.
+/// callback references and the second parameter is a bool that returns if the validation was successful or not.
 /// `callback_saf_message_received` - The callback function pointer that will be called when the Dht has determined that
 /// is has connected to enough of its neighbours to be confident that it has received any SAF messages that were waiting
 /// for it.
@@ -3181,9 +3181,9 @@ pub unsafe extern "C" fn wallet_create(
     callback_direct_send_result: unsafe extern "C" fn(c_ulonglong, bool),
     callback_store_and_forward_send_result: unsafe extern "C" fn(c_ulonglong, bool),
     callback_transaction_cancellation: unsafe extern "C" fn(*mut TariCompletedTransaction, u64),
-    callback_txo_validation_complete: unsafe extern "C" fn(u64, u8),
+    callback_txo_validation_complete: unsafe extern "C" fn(u64, bool),
     callback_balance_updated: unsafe extern "C" fn(*mut TariBalance),
-    callback_transaction_validation_complete: unsafe extern "C" fn(u64, u8),
+    callback_transaction_validation_complete: unsafe extern "C" fn(u64, bool),
     callback_saf_messages_received: unsafe extern "C" fn(),
     recovery_in_progress: *mut bool,
     error_out: *mut c_int,
@@ -6044,15 +6044,15 @@ mod test {
         completed_transaction_destroy(tx);
     }
 
-    unsafe extern "C" fn txo_validation_complete_callback(_tx_id: c_ulonglong, _result: u8) {
+    unsafe extern "C" fn txo_validation_complete_callback(_tx_id: c_ulonglong, _result: bool) {
         // assert!(true); //optimized out by compiler
     }
 
     unsafe extern "C" fn balance_updated_callback(_balance: *mut TariBalance) {
         // assert!(true); //optimized out by compiler
     }
 
-    unsafe extern "C" fn transaction_validation_complete_callback(_tx_id: c_ulonglong, _result: u8) {
+    unsafe extern "C" fn transaction_validation_complete_callback(_tx_id: c_ulonglong, _result: bool) {
         // assert!(true); //optimized out by compiler
     }