From c5eb9e5d2a86af12a69338445fb38c3170612b54 Mon Sep 17 00:00:00 2001 From: Philip Robinson Date: Fri, 25 Mar 2022 13:52:03 +0000 Subject: [PATCH] fix: fix handling of creating faux transaction for recovered outputs (#3959) Description --- There existed a bug in the UTXO scanner that if a block had multiple outputs in it it would assign a single TxId for all the output but try import a separate faux transaction for each output. After the first output the rest would be rejected as duplicated outputs. This PR updates the OMS scanning process to produce a unique TxId per output to avoid this issue. How Has This Been Tested? --- cargo test and manually --- .../src/output_manager_service/handle.rs | 38 ++++++++----------- .../recovery/standard_outputs_recoverer.rs | 16 +++++--- .../src/output_manager_service/service.rs | 21 ++++++---- .../utxo_scanner_service/utxo_scanner_task.rs | 24 ++++++------ .../output_manager_service_tests/service.rs | 7 ++-- .../support/output_manager_service_mock.rs | 23 +++++------ .../transaction_service_tests/service.rs | 9 ++--- 7 files changed, 70 insertions(+), 68 deletions(-) diff --git a/base_layer/wallet/src/output_manager_service/handle.rs b/base_layer/wallet/src/output_manager_service/handle.rs index 5846ccd312..a3fd1a3512 100644 --- a/base_layer/wallet/src/output_manager_service/handle.rs +++ b/base_layer/wallet/src/output_manager_service/handle.rs @@ -122,14 +122,8 @@ pub enum OutputManagerRequest { num_kernels: usize, num_outputs: usize, }, - ScanForRecoverableOutputs { - outputs: Vec, - tx_id: TxId, - }, - ScanOutputs { - outputs: Vec, - tx_id: TxId, - }, + ScanForRecoverableOutputs(Vec), + ScanOutputs(Vec), AddKnownOneSidedPaymentScript(KnownOneSidedPaymentScript), CreateOutputWithFeatures { value: MicroTari, @@ -194,8 +188,8 @@ impl fmt::Display for OutputManagerRequest { "FeeEstimate(amount: {}, fee_per_gram: {}, num_kernels: {}, num_outputs: {})", amount, fee_per_gram, num_kernels, num_outputs ), - ScanForRecoverableOutputs { .. } => write!(f, "ScanForRecoverableOutputs"), - ScanOutputs { .. } => write!(f, "ScanOutputs"), + ScanForRecoverableOutputs(_) => write!(f, "ScanForRecoverableOutputs"), + ScanOutputs(_) => write!(f, "ScanOutputs"), AddKnownOneSidedPaymentScript(_) => write!(f, "AddKnownOneSidedPaymentScript"), CreateOutputWithFeatures { value, features } => { write!(f, "CreateOutputWithFeatures({}, {})", value, features,) @@ -247,8 +241,8 @@ pub enum OutputManagerResponse { PublicRewindKeys(Box), RecoveryByte(u8), FeeEstimate(MicroTari), - RewoundOutputs(Vec), - ScanOutputs(Vec), + RewoundOutputs(Vec), + ScanOutputs(Vec), AddKnownOneSidedPaymentScript, CreateOutputWithFeatures { output: Box, @@ -300,6 +294,12 @@ pub struct PublicRewindKeys { pub rewind_blinding_public_key: PublicKey, } +#[derive(Debug, Clone)] +pub struct RecoveredOutput { + pub tx_id: TxId, + pub output: UnblindedOutput, +} + #[derive(Clone)] pub struct OutputManagerHandle { handle: SenderService>, @@ -728,11 +728,10 @@ impl OutputManagerHandle { pub async fn scan_for_recoverable_outputs( &mut self, outputs: Vec, - tx_id: TxId, - ) -> Result, OutputManagerError> { + ) -> Result, OutputManagerError> { match self .handle - .call(OutputManagerRequest::ScanForRecoverableOutputs { outputs, tx_id }) + .call(OutputManagerRequest::ScanForRecoverableOutputs(outputs)) .await?? { OutputManagerResponse::RewoundOutputs(outputs) => Ok(outputs), @@ -743,13 +742,8 @@ impl OutputManagerHandle { pub async fn scan_outputs_for_one_sided_payments( &mut self, outputs: Vec, - tx_id: TxId, - ) -> Result, OutputManagerError> { - match self - .handle - .call(OutputManagerRequest::ScanOutputs { outputs, tx_id }) - .await?? - { + ) -> Result, OutputManagerError> { + match self.handle.call(OutputManagerRequest::ScanOutputs(outputs)).await?? { OutputManagerResponse::ScanOutputs(outputs) => Ok(outputs), _ => Err(OutputManagerError::UnexpectedApiResponse), } diff --git a/base_layer/wallet/src/output_manager_service/recovery/standard_outputs_recoverer.rs b/base_layer/wallet/src/output_manager_service/recovery/standard_outputs_recoverer.rs index 43c2ded686..2582496b88 100644 --- a/base_layer/wallet/src/output_manager_service/recovery/standard_outputs_recoverer.rs +++ b/base_layer/wallet/src/output_manager_service/recovery/standard_outputs_recoverer.rs @@ -43,6 +43,7 @@ use crate::{ key_manager_service::KeyManagerInterface, output_manager_service::{ error::{OutputManagerError, OutputManagerStorageError}, + handle::RecoveredOutput, resources::OutputManagerKeyManagerBranch, storage::{ database::{OutputManagerBackend, OutputManagerDatabase}, @@ -84,8 +85,7 @@ where pub async fn scan_and_recover_outputs( &mut self, outputs: Vec, - tx_id: TxId, - ) -> Result, OutputManagerError> { + ) -> Result, OutputManagerError> { let start = Instant::now(); let outputs_length = outputs.len(); let mut rewound_outputs: Vec<(UnblindedOutput, BulletRangeProof)> = outputs @@ -131,7 +131,7 @@ where rewind_time.as_millis(), ); - let mut rewound_outputs_to_return = Vec::new(); + let mut rewound_outputs_with_tx_id: Vec = Vec::new(); for (output, proof) in rewound_outputs.iter_mut() { let db_output = DbUnblindedOutput::rewindable_from_unblinded_output( output.clone(), @@ -140,6 +140,7 @@ where None, Some(proof), )?; + let tx_id = TxId::new_random(); let output_hex = db_output.commitment.to_hex(); if let Err(e) = self.db.add_unspent_output_with_tx_id(tx_id, db_output).await { match e { @@ -153,6 +154,11 @@ where _ => return Err(OutputManagerError::from(e)), } } + + rewound_outputs_with_tx_id.push(RecoveredOutput { + output: output.clone(), + tx_id, + }); self.update_outputs_script_private_key_and_update_key_manager_index(output) .await?; trace!( @@ -162,9 +168,9 @@ where output.value, output.features, ); - rewound_outputs_to_return.push(output.clone()); } - Ok(rewound_outputs_to_return) + + Ok(rewound_outputs_with_tx_id) } /// Find the key manager index that corresponds to the spending key in the rewound output, if found then modify diff --git a/base_layer/wallet/src/output_manager_service/service.rs b/base_layer/wallet/src/output_manager_service/service.rs index 3e32e60f8d..f5be35d74a 100644 --- a/base_layer/wallet/src/output_manager_service/service.rs +++ b/base_layer/wallet/src/output_manager_service/service.rs @@ -77,6 +77,7 @@ use crate::{ OutputManagerRequest, OutputManagerResponse, PublicRewindKeys, + RecoveredOutput, }, recovery::StandardUtxoRecoverer, resources::{OutputManagerKeyManagerBranch, OutputManagerResources}, @@ -420,17 +421,17 @@ where OutputManagerRequest::CalculateRecoveryByte { spending_key, value } => Ok( OutputManagerResponse::RecoveryByte(self.calculate_recovery_byte(spending_key, value)?), ), - OutputManagerRequest::ScanForRecoverableOutputs { outputs, tx_id } => StandardUtxoRecoverer::new( + OutputManagerRequest::ScanForRecoverableOutputs(outputs) => StandardUtxoRecoverer::new( self.resources.master_key_manager.clone(), self.resources.rewind_data.clone(), self.resources.factories.clone(), self.resources.db.clone(), ) - .scan_and_recover_outputs(outputs, tx_id) + .scan_and_recover_outputs(outputs) .await .map(OutputManagerResponse::RewoundOutputs), - OutputManagerRequest::ScanOutputs { outputs, tx_id } => self - .scan_outputs_for_one_sided_payments(outputs, tx_id) + OutputManagerRequest::ScanOutputs(outputs) => self + .scan_outputs_for_one_sided_payments(outputs) .await .map(OutputManagerResponse::ScanOutputs), OutputManagerRequest::AddKnownOneSidedPaymentScript(known_script) => self @@ -2006,12 +2007,11 @@ where async fn scan_outputs_for_one_sided_payments( &mut self, outputs: Vec, - tx_id: TxId, - ) -> Result, OutputManagerError> { + ) -> Result, OutputManagerError> { let known_one_sided_payment_scripts: Vec = self.resources.db.get_all_known_one_sided_payment_scripts().await?; - let mut rewound_outputs: Vec = Vec::new(); + let mut rewound_outputs: Vec = Vec::new(); for output in outputs { let position = known_one_sided_payment_scripts .iter() @@ -2062,9 +2062,14 @@ where )?; let output_hex = output.commitment.to_hex(); + let tx_id = TxId::new_random(); + match self.resources.db.add_unspent_output_with_tx_id(tx_id, db_output).await { Ok(_) => { - rewound_outputs.push(rewound_output); + rewound_outputs.push(RecoveredOutput { + output: rewound_output, + tx_id, + }); }, Err(OutputManagerStorageError::DuplicateOutput) => { warn!( diff --git a/base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs b/base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs index 4134088f2d..29726fda2a 100644 --- a/base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs +++ b/base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs @@ -436,11 +436,11 @@ where TBackend: WalletBackend + 'static total_scanned += outputs.len(); let start = Instant::now(); - let (tx_id, found_outputs) = self.scan_for_outputs(outputs).await?; + let found_outputs = self.scan_for_outputs(outputs).await?; scan_for_outputs_profiling.push(start.elapsed()); let (count, amount) = self - .import_utxos_to_transaction_service(found_outputs, tx_id, current_height) + .import_utxos_to_transaction_service(found_outputs, current_height) .await?; self.resources @@ -492,18 +492,17 @@ where TBackend: WalletBackend + 'static async fn scan_for_outputs( &mut self, outputs: Vec, - ) -> Result<(TxId, Vec<(UnblindedOutput, String)>), UtxoScannerError> { - let mut found_outputs: Vec<(UnblindedOutput, String)> = Vec::new(); - let tx_id = TxId::new_random(); + ) -> Result, UtxoScannerError> { + let mut found_outputs: Vec<(UnblindedOutput, String, TxId)> = Vec::new(); if self.mode == UtxoScannerMode::Recovery { found_outputs.append( &mut self .resources .output_manager_service - .scan_for_recoverable_outputs(outputs.clone(), tx_id) + .scan_for_recoverable_outputs(outputs.clone()) .await? .into_iter() - .map(|uo| (uo, self.resources.recovery_message.clone())) + .map(|ro| (ro.output, self.resources.recovery_message.clone(), ro.tx_id)) .collect(), ); }; @@ -511,19 +510,18 @@ where TBackend: WalletBackend + 'static &mut self .resources .output_manager_service - .scan_outputs_for_one_sided_payments(outputs.clone(), tx_id) + .scan_outputs_for_one_sided_payments(outputs.clone()) .await? .into_iter() - .map(|uo| (uo, self.resources.one_sided_payment_message.clone())) + .map(|ro| (ro.output, self.resources.one_sided_payment_message.clone(), ro.tx_id)) .collect(), ); - Ok((tx_id, found_outputs)) + Ok(found_outputs) } async fn import_utxos_to_transaction_service( &mut self, - utxos: Vec<(UnblindedOutput, String)>, - tx_id: TxId, + utxos: Vec<(UnblindedOutput, String, TxId)>, current_height: u64, ) -> Result<(u64, MicroTari), UtxoScannerError> { let mut num_recovered = 0u64; @@ -532,7 +530,7 @@ where TBackend: WalletBackend + 'static // value is a placeholder. let source_public_key = CommsPublicKey::default(); - for (uo, message) in utxos { + for (uo, message, tx_id) in utxos { match self .import_unblinded_utxo_to_transaction_service( uo.clone(), diff --git a/base_layer/wallet/tests/output_manager_service_tests/service.rs b/base_layer/wallet/tests/output_manager_service_tests/service.rs index 4fc4f0e419..52e1d5b095 100644 --- a/base_layer/wallet/tests/output_manager_service_tests/service.rs +++ b/base_layer/wallet/tests/output_manager_service_tests/service.rs @@ -2136,7 +2136,6 @@ async fn scan_for_recovery_test() { .into_iter() .chain(non_rewindable_outputs.clone().into_iter()) .collect::>(), - TxId::from(1u64), ) .await .unwrap(); @@ -2146,7 +2145,9 @@ async fn scan_for_recovery_test() { assert_eq!(recovered_outputs.len(), NUM_REWINDABLE - 1); for o in rewindable_unblinded_outputs.iter().skip(1) { - assert!(recovered_outputs.iter().any(|ro| ro.spending_key == o.spending_key)); + assert!(recovered_outputs + .iter() + .any(|ro| ro.output.spending_key == o.spending_key)); } } @@ -2172,7 +2173,7 @@ async fn recovered_output_key_not_in_keychain() { let result = oms .output_manager_handle - .scan_for_recoverable_outputs(vec![rewindable_output], TxId::from(1u64)) + .scan_for_recoverable_outputs(vec![rewindable_output]) .await; assert!(matches!( diff --git a/base_layer/wallet/tests/support/output_manager_service_mock.rs b/base_layer/wallet/tests/support/output_manager_service_mock.rs index 2e3fe7d9ab..794f2df0d6 100644 --- a/base_layer/wallet/tests/support/output_manager_service_mock.rs +++ b/base_layer/wallet/tests/support/output_manager_service_mock.rs @@ -24,11 +24,12 @@ use std::sync::{Arc, Mutex}; use futures::StreamExt; use log::*; +use tari_common_types::transaction::TxId; use tari_service_framework::{reply_channel, reply_channel::Receiver}; use tari_shutdown::ShutdownSignal; use tari_wallet::output_manager_service::{ error::OutputManagerError, - handle::{OutputManagerEvent, OutputManagerHandle, OutputManagerRequest, OutputManagerResponse}, + handle::{OutputManagerEvent, OutputManagerHandle, OutputManagerRequest, OutputManagerResponse, RecoveredOutput}, storage::models::DbUnblindedOutput, }; use tokio::sync::{broadcast, broadcast::Sender, oneshot}; @@ -96,17 +97,17 @@ impl OutputManagerServiceMock { ) { info!(target: LOG_TARGET, "Handling Request: {}", request); match request { - OutputManagerRequest::ScanForRecoverableOutputs { - outputs: requested_outputs, - tx_id: _tx_id, - } => { + OutputManagerRequest::ScanForRecoverableOutputs(requested_outputs) => { let lock = acquire_lock!(self.state.recoverable_outputs); let outputs = (*lock) .clone() .into_iter() .filter_map(|dbuo| { if requested_outputs.iter().any(|ro| dbuo.commitment == ro.commitment) { - Some(dbuo.unblinded_output) + Some(RecoveredOutput { + output: dbuo.unblinded_output, + tx_id: TxId::new_random(), + }) } else { None } @@ -120,17 +121,17 @@ impl OutputManagerServiceMock { e }); }, - OutputManagerRequest::ScanOutputs { - outputs: requested_outputs, - tx_id: _tx_id, - } => { + OutputManagerRequest::ScanOutputs(requested_outputs) => { let lock = acquire_lock!(self.state.one_sided_payments); let outputs = (*lock) .clone() .into_iter() .filter_map(|dbuo| { if requested_outputs.iter().any(|ro| dbuo.commitment == ro.commitment) { - Some(dbuo.unblinded_output) + Some(RecoveredOutput { + output: dbuo.unblinded_output, + tx_id: TxId::new_random(), + }) } else { None } diff --git a/base_layer/wallet/tests/transaction_service_tests/service.rs b/base_layer/wallet/tests/transaction_service_tests/service.rs index 92b0b633e5..db25d0f9b1 100644 --- a/base_layer/wallet/tests/transaction_service_tests/service.rs +++ b/base_layer/wallet/tests/transaction_service_tests/service.rs @@ -925,18 +925,15 @@ fn recover_one_sided_transaction() { let outputs = completed_tx.transaction.body.outputs().clone(); let unblinded = bob_oms - .scan_outputs_for_one_sided_payments(outputs.clone(), TxId::new_random()) + .scan_outputs_for_one_sided_payments(outputs.clone()) .await .unwrap(); // Bob should be able to claim 1 output. assert_eq!(1, unblinded.len()); - assert_eq!(value, unblinded[0].value); + assert_eq!(value, unblinded[0].output.value); // Should ignore already existing outputs - let unblinded = bob_oms - .scan_outputs_for_one_sided_payments(outputs, TxId::new_random()) - .await - .unwrap(); + let unblinded = bob_oms.scan_outputs_for_one_sided_payments(outputs).await.unwrap(); assert!(unblinded.is_empty()); }); }