From 25e0ade1bb02a65c6885533bd20dedb01fa30067 Mon Sep 17 00:00:00 2001 From: Hansie Odendaal Date: Wed, 23 Mar 2022 11:00:57 +0200 Subject: [PATCH 1/2] Fix FFI import UTXO - Fixed importing external UTXOs via the FFI; changed 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 --- Cargo.lock | 1 + .../src/grpc/wallet_grpc_server.rs | 6 +- .../src/output_manager_service/handle.rs | 12 +- .../src/output_manager_service/service.rs | 30 +- .../storage/sqlite_db/mod.rs | 2 +- base_layer/wallet/src/wallet.rs | 24 +- base_layer/wallet/tests/support/utils.rs | 4 +- base_layer/wallet/tests/wallet.rs | 2 +- base_layer/wallet_ffi/Cargo.toml | 1 + base_layer/wallet_ffi/src/lib.rs | 283 ++++++++++++++++-- base_layer/wallet_ffi/wallet.h | 6 +- integration_tests/helpers/ffi/ffiInterface.js | 4 +- 12 files changed, 319 insertions(+), 56 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f3a075d5ff..4189db1051 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7450,6 +7450,7 @@ name = "tari_wallet_ffi" version = "0.30.0" dependencies = [ "chrono", + "env_logger 0.7.1", "futures 0.3.21", "lazy_static 1.4.0", "libc", diff --git a/applications/tari_console_wallet/src/grpc/wallet_grpc_server.rs b/applications/tari_console_wallet/src/grpc/wallet_grpc_server.rs index cf65f2fd1a..df2d545cb9 100644 --- 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/wallet/src/output_manager_service/handle.rs b/base_layer/wallet/src/output_manager_service/handle.rs index a3fd1a3512..c5e77b01cf 100644 --- 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 { 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 index f5be35d74a..f15d55c662 100644 --- 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 { + pub fn calculate_recovery_byte( + &mut self, + spending_key: PrivateKey, + value: u64, + with_rewind_data: bool, + ) -> Result { 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 index 008334673f..af82b7238d 100644 --- 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 index 9cb5f0282c..3bac5ac99d 100644 --- 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 index cac681bec6..662f1468d5 100644 --- 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( // 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( // '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 index 1f2d6a21b3..89100f8c25 100644 --- 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 index aadf3e6b94..d055e953d4 100644 --- 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" # # Temporary workaround until crates utilizing openssl have been updated from security-framework 2.4.0 diff --git a/base_layer/wallet_ffi/src/lib.rs b/base_layer/wallet_ffi/src/lib.rs index 5d9a47c191..797a976f8f 100644 --- a/base_layer/wallet_ffi/src/lib.rs +++ b/base_layer/wallet_ffi/src/lib.rs @@ -123,7 +123,7 @@ use tari_comms::{ use tari_comms_dht::{store_forward::SafConfig, DbConnectionUrl, DhtConfig}; use tari_core::{ covenants::Covenant, - transactions::{tari_amount::MicroTari, CryptoFactories}, + transactions::{tari_amount::MicroTari, transaction_components::OutputFeatures, CryptoFactories}, }; use tari_crypto::{ keys::{PublicKey as PublicKeyTrait, SecretKey}, @@ -141,7 +141,7 @@ use tari_utilities::{hex, hex::Hex}; use tari_wallet::{ connectivity_service::WalletConnectivityInterface, contacts_service::storage::database::Contact, - error::{WalletError, WalletStorageError}, + error::{WalletError, WalletError::OutputManagerError, WalletStorageError}, storage::{ database::WalletDatabase, sqlite_db::wallet::WalletSqliteDatabase, @@ -5137,26 +5137,31 @@ pub unsafe extern "C" fn wallet_get_public_key(wallet: *mut TariWallet, error_ou Box::into_raw(Box::new(pk)) } -/// Import a UTXO into the wallet. This will add a spendable UTXO and create a faux completed transaction to record the -/// event. +/// Import an external UTXO into the wallet as a non-rewindable (i.e. non-recoverable) output. This will add a spendable +/// UTXO (as EncumberedToBeReceived) and create a faux completed transaction to record the event. /// /// ## Arguments /// `wallet` - The TariWallet pointer /// `amount` - The value of the UTXO in MicroTari /// `spending_key` - The private spending key /// `source_public_key` - The public key of the source of the transaction +/// `features` - Options for an output's structure or use +/// `metadata_signature` - UTXO signature with the script offset private key, k_O +/// `sender_offset_public_key` - Tari script offset pubkey, K_O +/// `script_private_key` - Tari script private key, k_S, is used to create the script signature +/// `covenant` - The covenant that will be executed when spending this output /// `message` - The message that the transaction will have /// `error_out` - Pointer to an int which will be modified to an error code should one occur, may not be null. Functions /// as an out parameter. /// /// ## Returns -/// `c_ulonglong` - Returns the TransactionID of the generated transaction, note that it will be zero if transaction is -/// null +/// `c_ulonglong` - Returns the TransactionID of the generated transaction, note that it will be zero if the +/// transaction is null /// /// # Safety /// None #[no_mangle] -pub unsafe extern "C" fn wallet_import_utxo( +pub unsafe extern "C" fn wallet_import_external_utxo_as_non_rewindable( wallet: *mut TariWallet, amount: c_ulonglong, spending_key: *mut TariPrivateKey, @@ -5207,11 +5212,11 @@ pub unsafe extern "C" fn wallet_import_utxo( return 0; } - if features.is_null() { - error = LibWalletError::from(InterfaceError::NullError("features".to_string())).code; - ptr::swap(error_out, &mut error as *mut c_int); - return 0; - } + let mut updated_features = if features.is_null() { + OutputFeatures::default() + } else { + (*features).clone() + }; let covenant = if covenant.is_null() { Covenant::default() @@ -5247,21 +5252,43 @@ pub unsafe extern "C" fn wallet_import_utxo( let public_script_key = PublicKey::from_secret_key(&(*spending_key)); + // On the blockchain, 'V0' of the output features encode and decode without the recovery byte, whereas 'V1' onwards + // adds it in, however, for the wallet, the recovery byte must be consistent with the value commitment to be + // accepted in the wallet irrespective if it is 'V0' or 'V1' + let recovery_byte = + match (*wallet) + .runtime + .block_on((*wallet).wallet.output_manager_service.calculate_recovery_byte( + (*spending_key).clone(), + amount, + false, + )) { + Ok(val) => val, + Err(e) => { + error = LibWalletError::from(OutputManagerError(e)).code; + ptr::swap(error_out, &mut error as *mut c_int); + return 0; + }, + }; + updated_features.set_recovery_byte(recovery_byte); + // TODO: the script_lock_height can be something other than 0, for example an HTLC transaction - match (*wallet).runtime.block_on((*wallet).wallet.import_utxo( - MicroTari::from(amount), - &(*spending_key).clone(), - script!(Nop), - inputs!(public_script_key), - &(*source_public_key).clone(), - (*features).clone(), - message_string, - (*metadata_signature).clone(), - &(*script_private_key).clone(), - &(*sender_offset_public_key).clone(), - 0, - covenant, - )) { + match (*wallet) + .runtime + .block_on((*wallet).wallet.import_external_utxo_as_non_rewindable( + MicroTari::from(amount), + &(*spending_key).clone(), + script!(Nop), + inputs!(public_script_key), + &(*source_public_key).clone(), + updated_features, + message_string, + (*metadata_signature).clone(), + &(*script_private_key).clone(), + &(*sender_offset_public_key).clone(), + 0, + covenant, + )) { Ok(tx_id) => { if let Err(e) = (*wallet) .runtime @@ -6344,6 +6371,7 @@ mod test { use libc::{c_char, c_uchar, c_uint}; use tari_common_types::{emoji, transaction::TransactionStatus}; + use tari_core::transactions::test_helpers::{create_unblinded_output, TestParams}; use tari_key_manager::{mnemonic::MnemonicLanguage, mnemonic_wordlists}; use tari_test_utils::random; use tari_wallet::storage::sqlite_utilities::run_migration_and_create_sqlite_connection; @@ -7462,6 +7490,209 @@ mod test { } } + fn get_next_memory_address() -> Multiaddr { + let port = MemoryTransport::acquire_next_memsocket_port(); + format!("/memory/{}", port).parse().unwrap() + } + + #[test] + pub fn test_import_external_utxo() { + unsafe { + let mut error = 0; + let error_ptr = &mut error as *mut c_int; + let mut recovery_in_progress = true; + let recovery_in_progress_ptr = &mut recovery_in_progress as *mut bool; + + // create a new wallet + let db_name = CString::new(random::string(8).as_str()).unwrap(); + let db_name_str: *const c_char = CString::into_raw(db_name) as *const c_char; + let temp_dir = tempdir().unwrap(); + let db_path = CString::new(temp_dir.path().to_str().unwrap()).unwrap(); + let db_path_str: *const c_char = CString::into_raw(db_path) as *const c_char; + let transport_type = transport_memory_create(); + let address = transport_memory_get_address(transport_type, error_ptr); + let address_str = CStr::from_ptr(address).to_str().unwrap().to_owned(); + let address_str = CString::new(address_str).unwrap().into_raw() as *const c_char; + + let network = CString::new(NETWORK_STRING).unwrap(); + let network_str: *const c_char = CString::into_raw(network) as *const c_char; + + let config = comms_config_create( + address_str, + transport_type, + db_name_str, + db_path_str, + 20, + 10800, + network_str, + error_ptr, + ); + + let wallet_ptr = wallet_create( + config, + ptr::null(), + 0, + 0, + ptr::null(), + ptr::null(), + received_tx_callback, + received_tx_reply_callback, + received_tx_finalized_callback, + broadcast_callback, + mined_callback, + mined_unconfirmed_callback, + scanned_callback, + scanned_unconfirmed_callback, + direct_send_callback, + store_and_forward_send_callback, + tx_cancellation_callback, + txo_validation_complete_callback, + contacts_liveness_data_updated_callback, + balance_updated_callback, + transaction_validation_complete_callback, + saf_messages_received_callback, + connectivity_status_callback, + recovery_in_progress_ptr, + error_ptr, + ); + + let node_identity = + NodeIdentity::random(&mut OsRng, get_next_memory_address(), PeerFeatures::COMMUNICATION_NODE); + let base_node_peer_public_key_ptr = Box::into_raw(Box::new(node_identity.public_key().clone())); + let base_node_peer_address_ptr = + CString::into_raw(CString::new(node_identity.public_address().to_string()).unwrap()) as *const c_char; + wallet_add_base_node_peer( + wallet_ptr, + base_node_peer_public_key_ptr, + base_node_peer_address_ptr, + error_ptr, + ); + + // Create an unblinded output with a non-default recovery byte + let default_features = OutputFeatures::default(); + let utxo_1; + loop { + let test_params = TestParams::new(); + let utxo_temp = create_unblinded_output( + script!(Nop), + default_features.clone(), + test_params.clone(), + MicroTari::from(100_000), + ); + if utxo_temp.features.recovery_byte != default_features.recovery_byte { + utxo_1 = utxo_temp; + break; + } + } + assert_ne!(utxo_1.features.recovery_byte, default_features.recovery_byte); + + // Test the consistent features case, i.e. with valid 'recovery_byte' + let amount = utxo_1.value.as_u64(); + let spending_key_ptr = Box::into_raw(Box::new(utxo_1.spending_key)); + let features_ptr = Box::into_raw(Box::new(utxo_1.features.clone())); + let source_public_key_ptr = Box::into_raw(Box::new(TariPublicKey::default())); + let metadata_signature_ptr = Box::into_raw(Box::new(utxo_1.metadata_signature)); + let sender_offset_public_key_ptr = Box::into_raw(Box::new(utxo_1.sender_offset_public_key)); + let script_private_key_ptr = Box::into_raw(Box::new(utxo_1.script_private_key)); + let covenant_ptr = Box::into_raw(Box::new(utxo_1.covenant)); + let message_ptr = CString::into_raw(CString::new("For my friend").unwrap()) as *const c_char; + + let tx_id = wallet_import_external_utxo_as_non_rewindable( + wallet_ptr, + amount, + spending_key_ptr, + source_public_key_ptr, + features_ptr, + metadata_signature_ptr, + sender_offset_public_key_ptr, + script_private_key_ptr, + covenant_ptr, + message_ptr, + error_ptr, + ); + assert_eq!(error, 0); + assert!(tx_id > 0); + + // Cleanup + string_destroy(message_ptr as *mut c_char); + let _covenant = Box::from_raw(covenant_ptr); + let _script_private_key = Box::from_raw(script_private_key_ptr); + let _sender_offset_public_key = Box::from_raw(sender_offset_public_key_ptr); + let _metadata_signature = Box::from_raw(metadata_signature_ptr); + let _features = Box::from_raw(features_ptr); + let _source_public_key = Box::from_raw(source_public_key_ptr); + let _spending_key = Box::from_raw(spending_key_ptr); + + // Create an unblinded output with a non-default recovery byte + let mut utxo_2; + loop { + let test_params = TestParams::new(); + let utxo_temp = create_unblinded_output( + script!(Nop), + default_features.clone(), + test_params.clone(), + MicroTari::from(200_000), + ); + if utxo_temp.features.recovery_byte != default_features.recovery_byte { + utxo_2 = utxo_temp; + break; + } + } + assert_ne!(utxo_2.features.recovery_byte, default_features.recovery_byte); + // Reset the 'recovery_byte' to default; i.e. the 'metadata_signature' will now be inconsistent + utxo_2.features.set_recovery_byte(default_features.recovery_byte); + + // Test the inconsistent features case, i.e. with invalid 'recovery_byte' + let amount = utxo_2.value.as_u64(); + let spending_key_ptr = Box::into_raw(Box::new(utxo_2.spending_key)); + let source_public_key_ptr = Box::into_raw(Box::new(TariPublicKey::default())); + let features_ptr = Box::into_raw(Box::new(utxo_2.features)); + let metadata_signature_ptr = Box::into_raw(Box::new(utxo_2.metadata_signature)); + let sender_offset_public_key_ptr = Box::into_raw(Box::new(utxo_2.sender_offset_public_key)); + let script_private_key_ptr = Box::into_raw(Box::new(utxo_2.script_private_key)); + let covenant_ptr = Box::into_raw(Box::new(utxo_2.covenant)); + let message_ptr = CString::into_raw(CString::new("For my friend").unwrap()) as *const c_char; + + let tx_id = wallet_import_external_utxo_as_non_rewindable( + wallet_ptr, + amount, + spending_key_ptr, + source_public_key_ptr, + features_ptr, + metadata_signature_ptr, + sender_offset_public_key_ptr, + script_private_key_ptr, + covenant_ptr, + message_ptr, + error_ptr, + ); + assert_eq!(error, 0); + assert!(tx_id > 0); + + // Cleanup + string_destroy(message_ptr as *mut c_char); + let _covenant = Box::from_raw(covenant_ptr); + let _script_private_key = Box::from_raw(script_private_key_ptr); + let _sender_offset_public_key = Box::from_raw(sender_offset_public_key_ptr); + let _metadata_signature = Box::from_raw(metadata_signature_ptr); + let _features = Box::from_raw(features_ptr); + let _source_public_key = Box::from_raw(source_public_key_ptr); + let _spending_key = Box::from_raw(spending_key_ptr); + + let _base_node_peer_public_key = Box::from_raw(base_node_peer_public_key_ptr); + string_destroy(base_node_peer_address_ptr as *mut c_char); + + string_destroy(network_str as *mut c_char); + string_destroy(db_name_str as *mut c_char); + string_destroy(db_path_str as *mut c_char); + string_destroy(address_str as *mut c_char); + transport_type_destroy(transport_type); + + comms_config_destroy(config); + wallet_destroy(wallet_ptr); + } + } + #[test] pub fn test_seed_words() { unsafe { diff --git a/base_layer/wallet_ffi/wallet.h b/base_layer/wallet_ffi/wallet.h index 4c1d36a31d..be2ce6e65f 100644 --- a/base_layer/wallet_ffi/wallet.h +++ b/base_layer/wallet_ffi/wallet.h @@ -672,9 +672,9 @@ struct TariPendingInboundTransaction *wallet_get_pending_inbound_transaction_by_ // Get a Cancelled transaction from a TariWallet by its TransactionId. Pending Inbound or Outbound transaction will be converted to a CompletedTransaction struct TariCompletedTransaction *wallet_get_cancelled_transaction_by_id(struct TariWallet *wallet, unsigned long long transaction_id, int *error_out); -// Import a UTXO into the wallet. This will add a spendable UTXO and create a faux completed transaction to record the -// event. -unsigned long long wallet_import_utxo( +// Import an external UTXO into the wallet as a non-rewindable (i.e. non-recoverable) output. This will add a spendable +// UTXO and create a faux completed transaction to record the event. +unsigned long long wallet_import_external_utxo_as_non_rewindable( struct TariWallet *wallet, unsigned long long amount, struct TariPrivateKey *spending_key, diff --git a/integration_tests/helpers/ffi/ffiInterface.js b/integration_tests/helpers/ffi/ffiInterface.js index 8591d07c6a..7b98803ce0 100644 --- a/integration_tests/helpers/ffi/ffiInterface.js +++ b/integration_tests/helpers/ffi/ffiInterface.js @@ -385,7 +385,7 @@ class InterfaceFFI { this.ptr, [this.ptr, this.ulonglong, this.intPtr], ], - wallet_import_utxo: [ + wallet_import_external_utxo_as_non_rewindable: [ this.ulonglong, [ this.ptr, @@ -1524,7 +1524,7 @@ class InterfaceFFI { message ) { let error = this.initError(); - let result = this.fn.wallet_import_utxo( + let result = this.fn.wallet_import_external_utxo_as_non_rewindable( ptr, amount, spending_key_ptr, From c1ffe6306d5d0c93efed225383d9f7977160fa91 Mon Sep 17 00:00:00 2001 From: Hansie Odendaal Date: Mon, 28 Mar 2022 13:03:55 +0200 Subject: [PATCH 2/2] review comments --- .../unblinded_output.rs | 29 +++++++---- .../transaction_initializer.rs | 15 ++++-- base_layer/wallet_ffi/src/lib.rs | 51 +++++-------------- 3 files changed, 44 insertions(+), 51 deletions(-) diff --git a/base_layer/core/src/transactions/transaction_components/unblinded_output.rs b/base_layer/core/src/transactions/transaction_components/unblinded_output.rs index 59f08d4e7b..8dcea791ba 100644 --- 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 index 64081c8e85..b05c989055 100644 --- 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_ffi/src/lib.rs b/base_layer/wallet_ffi/src/lib.rs index 797a976f8f..7e2a712f5c 100644 --- a/base_layer/wallet_ffi/src/lib.rs +++ b/base_layer/wallet_ffi/src/lib.rs @@ -121,10 +121,7 @@ use tari_comms::{ types::{CommsPublicKey, CommsSecretKey}, }; use tari_comms_dht::{store_forward::SafConfig, DbConnectionUrl, DhtConfig}; -use tari_core::{ - covenants::Covenant, - transactions::{tari_amount::MicroTari, transaction_components::OutputFeatures, CryptoFactories}, -}; +use tari_core::transactions::{tari_amount::MicroTari, CryptoFactories}; use tari_crypto::{ keys::{PublicKey as PublicKeyTrait, SecretKey}, tari_utilities::ByteArray, @@ -141,7 +138,7 @@ use tari_utilities::{hex, hex::Hex}; use tari_wallet::{ connectivity_service::WalletConnectivityInterface, contacts_service::storage::database::Contact, - error::{WalletError, WalletError::OutputManagerError, WalletStorageError}, + error::{WalletError, WalletStorageError}, storage::{ database::WalletDatabase, sqlite_db::wallet::WalletSqliteDatabase, @@ -5212,16 +5209,16 @@ pub unsafe extern "C" fn wallet_import_external_utxo_as_non_rewindable( return 0; } - let mut updated_features = if features.is_null() { - OutputFeatures::default() - } else { - (*features).clone() - }; + if features.is_null() { + error = LibWalletError::from(InterfaceError::NullError("features".to_string())).code; + ptr::swap(error_out, &mut error as *mut c_int); + return 0; + } - let covenant = if covenant.is_null() { - Covenant::default() - } else { - (*covenant).clone() + if covenant.is_null() { + error = LibWalletError::from(InterfaceError::NullError("covenant".to_string())).code; + ptr::swap(error_out, &mut error as *mut c_int); + return 0; }; let message_string; @@ -5252,26 +5249,6 @@ pub unsafe extern "C" fn wallet_import_external_utxo_as_non_rewindable( let public_script_key = PublicKey::from_secret_key(&(*spending_key)); - // On the blockchain, 'V0' of the output features encode and decode without the recovery byte, whereas 'V1' onwards - // adds it in, however, for the wallet, the recovery byte must be consistent with the value commitment to be - // accepted in the wallet irrespective if it is 'V0' or 'V1' - let recovery_byte = - match (*wallet) - .runtime - .block_on((*wallet).wallet.output_manager_service.calculate_recovery_byte( - (*spending_key).clone(), - amount, - false, - )) { - Ok(val) => val, - Err(e) => { - error = LibWalletError::from(OutputManagerError(e)).code; - ptr::swap(error_out, &mut error as *mut c_int); - return 0; - }, - }; - updated_features.set_recovery_byte(recovery_byte); - // TODO: the script_lock_height can be something other than 0, for example an HTLC transaction match (*wallet) .runtime @@ -5281,13 +5258,13 @@ pub unsafe extern "C" fn wallet_import_external_utxo_as_non_rewindable( script!(Nop), inputs!(public_script_key), &(*source_public_key).clone(), - updated_features, + (*features).clone(), message_string, (*metadata_signature).clone(), &(*script_private_key).clone(), &(*sender_offset_public_key).clone(), 0, - covenant, + (*covenant).clone(), )) { Ok(tx_id) => { if let Err(e) = (*wallet) @@ -7569,7 +7546,7 @@ mod test { ); // Create an unblinded output with a non-default recovery byte - let default_features = OutputFeatures::default(); + let default_features = TariOutputFeatures::default(); let utxo_1; loop { let test_params = TestParams::new();