fix: fix ffi import external utxo from faucet (#3956)

Description --- - Fixed importing external UTXOs via the FFI _by changing the philosophy about having inconsistent recovery byte data in the wallet database_ - Renamed import UTXO methods to be more descriptive - Added a unit test to test importing external UTXOs via the FFI _**Note:** The FFI import UTXO method was renamed; this is a breaking change on the FFI interface._ Motivation and Context --- See above How Has This Been Tested? --- Added a new unit test that test the import UTXO interface
tari-project · Mar 28, 2022 · 3480323 · 3480323
1 parent cc6c80a
commit 3480323
Show file tree

Hide file tree

Showing 14 changed files with 327 additions and 71 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/applications/tari_console_wallet/src/grpc/wallet_grpc_server.rs b/applications/tari_console_wallet/src/grpc/wallet_grpc_server.rs
@@ -636,7 +636,11 @@ impl wallet_server::Wallet for WalletGrpcServer {
         for o in unblinded_outputs.iter() {
             tx_ids.push(
                 wallet
-                    .import_unblinded_utxo(o.clone(), &CommsPublicKey::default(), "Imported via gRPC".to_string())
+                    .import_unblinded_output_as_non_rewindable(
+                        o.clone(),
+                        &CommsPublicKey::default(),
+                        "Imported via gRPC".to_string(),
+                    )
                     .await
                     .map_err(|e| Status::internal(format!("{:?}", e)))?
                     .into(),

diff --git a/base_layer/core/src/transactions/transaction_components/unblinded_output.rs b/base_layer/core/src/transactions/transaction_components/unblinded_output.rs
@@ -29,6 +29,7 @@ use std::{
     ops::Shl,
 };
 
+use log::*;
 use rand::rngs::OsRng;
 use serde::{Deserialize, Serialize};
 use tari_common_types::types::{
@@ -44,7 +45,7 @@ use tari_crypto::{
     commitment::HomomorphicCommitmentFactory,
     keys::{PublicKey as PublicKeyTrait, SecretKey},
     range_proof::{RangeProofError, RangeProofService},
-    tari_utilities::ByteArray,
+    tari_utilities::{hex::to_hex, ByteArray},
 };
 use tari_script::{ExecutionStack, TariScript};
 
@@ -66,6 +67,8 @@ use crate::{
     },
 };
 
+pub const LOG_TARGET: &str = "c::tx::tx_components::unblinded_output";
+
 /// An unblinded output is one where the value and spending key (blinding factor) are known. This can be used to
 /// build both inputs and outputs (every input comes from an output)
 // TODO: Try to get rid of 'Serialize' and 'Deserialize' traits here; see related comment at 'struct RawTransactionInfo'
@@ -207,10 +210,14 @@ impl UnblindedOutput {
 
         let recovery_byte = OutputFeatures::create_unique_recovery_byte(&commitment, None);
         if self.features.recovery_byte != recovery_byte {
-            return Err(TransactionError::ValidationError(format!(
-                "Recovery byte set incorrectly - expected {} got {}",
-                recovery_byte, self.features.recovery_byte
-            )));
+            // This is not a hard error by choice; we allow inconsistent recovery byte data into the wallet database
+            error!(
+                target: LOG_TARGET,
+                "Recovery byte set incorrectly - expected {}, got {} for commitment {}",
+                recovery_byte,
+                self.features.recovery_byte,
+                to_hex(commitment.as_bytes()),
+            );
         }
 
         let output = TransactionOutput::new(
@@ -262,10 +269,14 @@ impl UnblindedOutput {
 
         let recovery_byte = OutputFeatures::create_unique_recovery_byte(&commitment, Some(rewind_data));
         if self.features.recovery_byte != recovery_byte {
-            return Err(TransactionError::ValidationError(format!(
-                "Recovery byte set incorrectly (with rewind) - expected {} got {}",
-                recovery_byte, self.features.recovery_byte
-            )));
+            // This is not a hard error by choice; we allow inconsistent recovery byte data into the wallet database
+            error!(
+                target: LOG_TARGET,
+                "Recovery byte set incorrectly (with rewind) - expected {}, got {} for commitment {}",
+                recovery_byte,
+                self.features.recovery_byte,
+                to_hex(commitment.as_bytes()),
+            );
         }
 
         let output = TransactionOutput::new(

diff --git a/base_layer/core/src/transactions/transaction_protocol/transaction_initializer.rs b/base_layer/core/src/transactions/transaction_protocol/transaction_initializer.rs
@@ -36,9 +36,10 @@ use tari_crypto::{
     commitment::HomomorphicCommitmentFactory,
     keys::{PublicKey as PublicKeyTrait, SecretKey},
     ristretto::pedersen::PedersenCommitmentFactory,
-    tari_utilities::fixed_set::FixedSet,
+    tari_utilities::{fixed_set::FixedSet, hex::to_hex},
 };
 use tari_script::{ExecutionStack, TariScript};
+use tari_utilities::ByteArray;
 
 use crate::{
     consensus::{ConsensusConstants, ConsensusEncodingSized},
@@ -216,10 +217,14 @@ impl SenderTransactionInitializer {
         let commitment = commitment_factory.commit(&output.spending_key, &PrivateKey::from(output.value));
         let recovery_byte = OutputFeatures::create_unique_recovery_byte(&commitment, self.rewind_data.as_ref());
         if recovery_byte != output.features.recovery_byte {
-            return self.clone().build_err(&*format!(
-                "Recovery byte not valid (expected {}, got {}), cannot add output: {:?}",
-                recovery_byte, output.features.recovery_byte, output
-            ))?;
+            // This is not a hard error by choice; we allow inconsistent recovery byte data into the wallet database
+            error!(
+                target: LOG_TARGET,
+                "Recovery byte set incorrectly (with output) - expected {}, got {} for commitment {}",
+                recovery_byte,
+                output.features.recovery_byte,
+                to_hex(commitment.as_bytes()),
+            );
         }
         let e = TransactionOutput::build_metadata_signature_challenge(
             &output.script,

diff --git a/base_layer/wallet/src/output_manager_service/handle.rs b/base_layer/wallet/src/output_manager_service/handle.rs
@@ -115,6 +115,7 @@ pub enum OutputManagerRequest {
     CalculateRecoveryByte {
         spending_key: PrivateKey,
         value: u64,
+        with_rewind_data: bool,
     },
     FeeEstimate {
         amount: MicroTari,
@@ -172,7 +173,9 @@ impl fmt::Display for OutputManagerRequest {
             RemoveEncryption => write!(f, "RemoveEncryption"),
             GetCoinbaseTransaction(_) => write!(f, "GetCoinbaseTransaction"),
             GetPublicRewindKeys => write!(f, "GetPublicRewindKeys"),
-            CalculateRecoveryByte { spending_key, value } => write!(
+            CalculateRecoveryByte {
+                spending_key, value, ..
+            } => write!(
                 f,
                 "CalculateRecoveryByte ({},{})",
                 spending_key.as_bytes().to_vec().to_hex(),
@@ -630,10 +633,15 @@ impl OutputManagerHandle {
         &mut self,
         spending_key: PrivateKey,
         value: u64,
+        with_rewind_data: bool,
     ) -> Result<u8, OutputManagerError> {
         match self
             .handle
-            .call(OutputManagerRequest::CalculateRecoveryByte { spending_key, value })
+            .call(OutputManagerRequest::CalculateRecoveryByte {
+                spending_key,
+                value,
+                with_rewind_data,
+            })
             .await??
         {
             OutputManagerResponse::RecoveryByte(rk) => Ok(rk),

diff --git a/base_layer/wallet/src/output_manager_service/service.rs b/base_layer/wallet/src/output_manager_service/service.rs
@@ -418,9 +418,15 @@ where
             OutputManagerRequest::GetPublicRewindKeys => Ok(OutputManagerResponse::PublicRewindKeys(Box::new(
                 self.get_rewind_public_keys(),
             ))),
-            OutputManagerRequest::CalculateRecoveryByte { spending_key, value } => Ok(
-                OutputManagerResponse::RecoveryByte(self.calculate_recovery_byte(spending_key, value)?),
-            ),
+            OutputManagerRequest::CalculateRecoveryByte {
+                spending_key,
+                value,
+                with_rewind_data,
+            } => Ok(OutputManagerResponse::RecoveryByte(self.calculate_recovery_byte(
+                spending_key,
+                value,
+                with_rewind_data,
+            )?)),
             OutputManagerRequest::ScanForRecoverableOutputs(outputs) => StandardUtxoRecoverer::new(
                 self.resources.master_key_manager.clone(),
                 self.resources.rewind_data.clone(),
@@ -593,9 +599,19 @@ where
         self.validate_outputs()
     }
 
-    pub fn calculate_recovery_byte(&mut self, spending_key: PrivateKey, value: u64) -> Result<u8, OutputManagerError> {
+    pub fn calculate_recovery_byte(
+        &mut self,
+        spending_key: PrivateKey,
+        value: u64,
+        with_rewind_data: bool,
+    ) -> Result<u8, OutputManagerError> {
         let commitment = self.resources.factories.commitment.commit_value(&spending_key, value);
-        let recovery_byte = OutputFeatures::create_unique_recovery_byte(&commitment, Some(&self.resources.rewind_data));
+        let rewind_data = if with_rewind_data {
+            Some(&self.resources.rewind_data)
+        } else {
+            None
+        };
+        let recovery_byte = OutputFeatures::create_unique_recovery_byte(&commitment, rewind_data);
         Ok(recovery_byte)
     }
 
@@ -1326,7 +1342,7 @@ where
         }
 
         let (spending_key, script_private_key) = self.get_spend_and_script_keys().await?;
-        let recovery_byte = self.calculate_recovery_byte(spending_key.clone(), amount.as_u64())?;
+        let recovery_byte = self.calculate_recovery_byte(spending_key.clone(), amount.as_u64(), true)?;
         let output_features = OutputFeatures {
             recovery_byte,
             unique_id: unique_id.clone(),
@@ -1672,7 +1688,7 @@ where
             let output_amount = amount_per_split;
 
             let (spending_key, script_private_key) = self.get_spend_and_script_keys().await?;
-            let recovery_byte = self.calculate_recovery_byte(spending_key.clone(), output_amount.as_u64())?;
+            let recovery_byte = self.calculate_recovery_byte(spending_key.clone(), output_amount.as_u64(), true)?;
             let output_features = OutputFeatures {
                 recovery_byte,
                 ..Default::default()

diff --git a/base_layer/wallet/src/output_manager_service/storage/sqlite_db/mod.rs b/base_layer/wallet/src/output_manager_service/storage/sqlite_db/mod.rs
@@ -1156,7 +1156,7 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase {
         if start.elapsed().as_millis() > 0 {
             trace!(
                 target: LOG_TARGET,
-                "sqlite profile - reinstate_cancelled_inbound_output: lock {} + db_op {} = {} ms",
+                "sqlite profile - add_unvalidated_output: lock {} + db_op {} = {} ms",
                 acquire_lock.as_millis(),
                 (start.elapsed() - acquire_lock).as_millis(),
                 start.elapsed().as_millis()

diff --git a/base_layer/wallet/src/wallet.rs b/base_layer/wallet/src/wallet.rs
@@ -398,11 +398,10 @@ where
         self.updater_service.clone().unwrap()
     }
 
-    /// Import an external spendable UTXO into the wallet. The output will be added to the Output Manager and made
-    /// EncumberedToBeReceived. A faux incoming transaction will be created to provide a record of the event. The TxId
-    /// of the generated transaction is returned.
-
-    pub async fn import_utxo(
+    /// Import an external spendable UTXO into the wallet as a non-rewindable/non-recoverable UTXO. The output will be
+    /// added to the Output Manager and made EncumberedToBeReceived. A faux incoming transaction will be created to
+    /// provide a record of the event. The TxId of the generated transaction is returned.
+    pub async fn import_external_utxo_as_non_rewindable(
         &mut self,
         amount: MicroTari,
         spending_key: &PrivateKey,
@@ -449,22 +448,24 @@ where
             .map_err(WalletError::TransactionError)?
             .to_hex();
 
+        // As non-rewindable
         self.output_manager_service
             .add_unvalidated_output(tx_id, unblinded_output, None)
             .await?;
 
         info!(
             target: LOG_TARGET,
-            "UTXO (Commitment: {}) imported into wallet as 'ImportStatus::Imported'", commitment_hex
+            "UTXO (Commitment: {}) imported into wallet as 'ImportStatus::Imported' and is non-rewindable",
+            commitment_hex
         );
 
         Ok(tx_id)
     }
 
-    /// Import an external spendable UTXO into the wallet. The output will be added to the Output Manager and made
-    /// spendable. A faux incoming transaction will be created to provide a record of the event. The TxId of the
-    /// generated transaction is returned.
-    pub async fn import_unblinded_utxo(
+    /// Import an external spendable UTXO into the wallet as a non-rewindable/non-recoverable UTXO. The output will be
+    /// added to the Output Manager and made spendable. A faux incoming transaction will be created to provide a record
+    /// of the event. The TxId of the generated transaction is returned.
+    pub async fn import_unblinded_output_as_non_rewindable(
         &mut self,
         unblinded_output: UnblindedOutput,
         source_public_key: &CommsPublicKey,
@@ -483,13 +484,14 @@ where
             )
             .await?;
 
+        // As non-rewindable
         self.output_manager_service
             .add_output_with_tx_id(tx_id, unblinded_output.clone(), None)
             .await?;
 
         info!(
             target: LOG_TARGET,
-            "UTXO (Commitment: {}) imported into wallet as 'ImportStatus::Imported'",
+            "UTXO (Commitment: {}) imported into wallet as 'ImportStatus::Imported' and is non-rewindable",
             unblinded_output
                 .as_transaction_input(&self.factories.commitment)?
                 .commitment()

diff --git a/base_layer/wallet/tests/support/utils.rs b/base_layer/wallet/tests/support/utils.rs
@@ -68,7 +68,7 @@ pub async fn make_input<R: Rng + CryptoRng>(
     // further down the line
     if let Some(mut oms) = oms {
         if let Ok(val) = oms
-            .calculate_recovery_byte(utxo.spending_key.clone(), utxo.value.clone().as_u64())
+            .calculate_recovery_byte(utxo.spending_key.clone(), utxo.value.clone().as_u64(), true)
             .await
         {
             utxo.features.set_recovery_byte(val);
@@ -99,7 +99,7 @@ pub async fn make_input_with_features<R: Rng + CryptoRng>(
     // 'TestParamsHelpers::new()'; this will influence validation of output features and the metadata signature
     // further down the line
     if let Ok(val) = oms
-        .calculate_recovery_byte(utxo.spending_key.clone(), utxo.value.clone().as_u64())
+        .calculate_recovery_byte(utxo.spending_key.clone(), utxo.value.clone().as_u64(), true)
         .await
     {
         utxo.features.set_recovery_byte(val);

diff --git a/base_layer/wallet/tests/wallet.rs b/base_layer/wallet/tests/wallet.rs
@@ -767,7 +767,7 @@ async fn test_import_utxo() {
     let expected_output_hash = output.hash();
 
     let tx_id = alice_wallet
-        .import_utxo(
+        .import_external_utxo_as_non_rewindable(
             utxo.value,
             &utxo.spending_key,
             script.clone(),

diff --git a/base_layer/wallet_ffi/Cargo.toml b/base_layer/wallet_ffi/Cargo.toml
@@ -27,6 +27,7 @@ openssl = { version = "0.10", features = ["vendored"] }
 rand = "0.8"
 thiserror = "1.0.26"
 tokio = "1.11"
+env_logger = "0.7.0"
 
 # <workaround>
 # Temporary workaround until crates utilizing openssl have been updated from security-framework 2.4.0