From 8c9070a512117bbf7c1eae83f554a6ccf239e05a Mon Sep 17 00:00:00 2001 From: Philip Date: Tue, 29 Mar 2022 13:05:09 +0100 Subject: [PATCH 1/3] fix: revert changes to key manager branch seed strings During the Key Manager refactor the strings used as branch seeds by the key manager were changed. This broke recovery as the keys generated for the rewind process were wrong and the rewound key index could no longer be found. --- .../storage/sqlite_db/mod.rs | 4 +-- .../recovery/standard_outputs_recoverer.rs | 2 +- .../src/output_manager_service/resources.rs | 29 +++++++++---------- .../src/output_manager_service/service.rs | 7 +++-- 4 files changed, 22 insertions(+), 20 deletions(-) diff --git a/base_layer/wallet/src/key_manager_service/storage/sqlite_db/mod.rs b/base_layer/wallet/src/key_manager_service/storage/sqlite_db/mod.rs index 1d9b128b48..1162185df7 100644 --- a/base_layer/wallet/src/key_manager_service/storage/sqlite_db/mod.rs +++ b/base_layer/wallet/src/key_manager_service/storage/sqlite_db/mod.rs @@ -94,11 +94,11 @@ impl KeyManagerSqliteDatabase { if !old_state.is_empty() { // there should only be 1 if there is an old state. let spending_km = KeyManagerState { - branch_seed: OutputManagerKeyManagerBranch::Spend.to_string(), + branch_seed: OutputManagerKeyManagerBranch::Spend.into(), primary_key_index: old_state[0].primary_key_index as u64, }; let spending_script_km = KeyManagerState { - branch_seed: OutputManagerKeyManagerBranch::SpendScript.to_string(), + branch_seed: OutputManagerKeyManagerBranch::SpendScript.into(), primary_key_index: old_state[0].primary_key_index as u64, }; let mut km_sql_spending = NewKeyManagerStateSql::from(spending_km); 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 2582496b88..76314dbd93 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 @@ -184,7 +184,7 @@ where let found_index = self .master_key_manager .find_key_index( - OutputManagerKeyManagerBranch::Coinbase.to_string(), + OutputManagerKeyManagerBranch::Coinbase.get_branch_key(), &output.spending_key, ) .await?; diff --git a/base_layer/wallet/src/output_manager_service/resources.rs b/base_layer/wallet/src/output_manager_service/resources.rs index e1069a12f9..b8b4b70cff 100644 --- a/base_layer/wallet/src/output_manager_service/resources.rs +++ b/base_layer/wallet/src/output_manager_service/resources.rs @@ -20,8 +20,6 @@ // 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 std::fmt::{Display, Error, Formatter}; - use tari_core::{ consensus::ConsensusConstants, transactions::{transaction_protocol::RewindData, CryptoFactories}, @@ -59,24 +57,25 @@ pub enum OutputManagerKeyManagerBranch { RecoveryByte, } -impl Display for OutputManagerKeyManagerBranch { - fn fmt(&self, fmt: &mut Formatter<'_>) -> Result<(), Error> { - let response = match self { - OutputManagerKeyManagerBranch::Spend => "Spend", - OutputManagerKeyManagerBranch::SpendScript => "Script", - OutputManagerKeyManagerBranch::Coinbase => "Coinbase", - OutputManagerKeyManagerBranch::CoinbaseScript => "Coinbase_script", - OutputManagerKeyManagerBranch::RecoveryViewOnly => "Recovery_viewonly", - OutputManagerKeyManagerBranch::RecoveryBlinding => "Recovery_blinding", - OutputManagerKeyManagerBranch::RecoveryByte => "Recovery_byte", - }; - fmt.write_str(response) +/// Warning: Changing these strings will affect the backwards compatibility of the wallet with older databases or +/// recovery. +impl OutputManagerKeyManagerBranch { + pub fn get_branch_key(&self) -> String { + match self { + OutputManagerKeyManagerBranch::Spend => "".to_string(), + OutputManagerKeyManagerBranch::SpendScript => "script".to_string(), + OutputManagerKeyManagerBranch::Coinbase => "coinbase".to_string(), + OutputManagerKeyManagerBranch::CoinbaseScript => "coinbase_script".to_string(), + OutputManagerKeyManagerBranch::RecoveryViewOnly => "recovery_viewonly".to_string(), + OutputManagerKeyManagerBranch::RecoveryBlinding => "recovery_blinding".to_string(), + OutputManagerKeyManagerBranch::RecoveryByte => "Recovery_byte".to_string(), + } } } #[allow(clippy::from_over_into)] impl Into for OutputManagerKeyManagerBranch { fn into(self) -> String { - self.to_string() + self.get_branch_key() } } diff --git a/base_layer/wallet/src/output_manager_service/service.rs b/base_layer/wallet/src/output_manager_service/service.rs index f15d55c662..7d694f85ab 100644 --- a/base_layer/wallet/src/output_manager_service/service.rs +++ b/base_layer/wallet/src/output_manager_service/service.rs @@ -1050,12 +1050,15 @@ where let spending_key = self .resources .master_key_manager - .get_key_at_index(OutputManagerKeyManagerBranch::Coinbase.to_string(), block_height) + .get_key_at_index(OutputManagerKeyManagerBranch::Coinbase.get_branch_key(), block_height) .await?; let script_private_key = self .resources .master_key_manager - .get_key_at_index(OutputManagerKeyManagerBranch::CoinbaseScript.to_string(), block_height) + .get_key_at_index( + OutputManagerKeyManagerBranch::CoinbaseScript.get_branch_key(), + block_height, + ) .await?; let nonce = PrivateKey::random(&mut OsRng); From c9941ba56eee228e45e1eeb18e442b0c542b4459 Mon Sep 17 00:00:00 2001 From: Philip Date: Tue, 29 Mar 2022 13:27:24 +0100 Subject: [PATCH 2/3] review comments --- .../storage/sqlite_db/mod.rs | 4 +-- .../recovery/standard_outputs_recoverer.rs | 19 ++++++++---- .../src/output_manager_service/resources.rs | 11 ++----- .../src/output_manager_service/service.rs | 29 +++++++++++-------- 4 files changed, 35 insertions(+), 28 deletions(-) diff --git a/base_layer/wallet/src/key_manager_service/storage/sqlite_db/mod.rs b/base_layer/wallet/src/key_manager_service/storage/sqlite_db/mod.rs index 1162185df7..e3ec4b4412 100644 --- a/base_layer/wallet/src/key_manager_service/storage/sqlite_db/mod.rs +++ b/base_layer/wallet/src/key_manager_service/storage/sqlite_db/mod.rs @@ -94,11 +94,11 @@ impl KeyManagerSqliteDatabase { if !old_state.is_empty() { // there should only be 1 if there is an old state. let spending_km = KeyManagerState { - branch_seed: OutputManagerKeyManagerBranch::Spend.into(), + branch_seed: OutputManagerKeyManagerBranch::Spend.get_branch_key(), primary_key_index: old_state[0].primary_key_index as u64, }; let spending_script_km = KeyManagerState { - branch_seed: OutputManagerKeyManagerBranch::SpendScript.into(), + branch_seed: OutputManagerKeyManagerBranch::SpendScript.get_branch_key(), primary_key_index: old_state[0].primary_key_index as u64, }; let mut km_sql_spending = NewKeyManagerStateSql::from(spending_km); 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 76314dbd93..67c1bb9665 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 @@ -190,23 +190,32 @@ where .await?; self.master_key_manager - .get_key_at_index(OutputManagerKeyManagerBranch::CoinbaseScript, found_index) + .get_key_at_index( + OutputManagerKeyManagerBranch::CoinbaseScript.get_branch_key(), + found_index, + ) .await? } else { let found_index = self .master_key_manager - .find_key_index(OutputManagerKeyManagerBranch::Spend, &output.spending_key) + .find_key_index( + OutputManagerKeyManagerBranch::Spend.get_branch_key(), + &output.spending_key, + ) .await?; self.master_key_manager - .update_current_key_index_if_higher(OutputManagerKeyManagerBranch::Spend, found_index) + .update_current_key_index_if_higher(OutputManagerKeyManagerBranch::Spend.get_branch_key(), found_index) .await?; self.master_key_manager - .update_current_key_index_if_higher(OutputManagerKeyManagerBranch::SpendScript, found_index) + .update_current_key_index_if_higher( + OutputManagerKeyManagerBranch::SpendScript.get_branch_key(), + found_index, + ) .await?; self.master_key_manager - .get_key_at_index(OutputManagerKeyManagerBranch::SpendScript, found_index) + .get_key_at_index(OutputManagerKeyManagerBranch::SpendScript.get_branch_key(), found_index) .await? }; diff --git a/base_layer/wallet/src/output_manager_service/resources.rs b/base_layer/wallet/src/output_manager_service/resources.rs index b8b4b70cff..7820fda11c 100644 --- a/base_layer/wallet/src/output_manager_service/resources.rs +++ b/base_layer/wallet/src/output_manager_service/resources.rs @@ -57,9 +57,9 @@ pub enum OutputManagerKeyManagerBranch { RecoveryByte, } -/// Warning: Changing these strings will affect the backwards compatibility of the wallet with older databases or -/// recovery. impl OutputManagerKeyManagerBranch { + /// Warning: Changing these strings will affect the backwards compatibility of the wallet with older databases or + /// recovery. pub fn get_branch_key(&self) -> String { match self { OutputManagerKeyManagerBranch::Spend => "".to_string(), @@ -72,10 +72,3 @@ impl OutputManagerKeyManagerBranch { } } } - -#[allow(clippy::from_over_into)] -impl Into for OutputManagerKeyManagerBranch { - fn into(self) -> String { - self.get_branch_key() - } -} diff --git a/base_layer/wallet/src/output_manager_service/service.rs b/base_layer/wallet/src/output_manager_service/service.rs index 7d694f85ab..1266d74578 100644 --- a/base_layer/wallet/src/output_manager_service/service.rs +++ b/base_layer/wallet/src/output_manager_service/service.rs @@ -134,13 +134,13 @@ where db.clear_short_term_encumberances().await?; Self::initialise_key_manager(&key_manager).await?; let rewind_key = key_manager - .get_key_at_index(OutputManagerKeyManagerBranch::RecoveryViewOnly, 0) + .get_key_at_index(OutputManagerKeyManagerBranch::RecoveryViewOnly.get_branch_key(), 0) .await?; let rewind_blinding_key = key_manager - .get_key_at_index(OutputManagerKeyManagerBranch::RecoveryBlinding, 0) + .get_key_at_index(OutputManagerKeyManagerBranch::RecoveryBlinding.get_branch_key(), 0) .await?; let recovery_byte_key = key_manager - .get_key_at_index(OutputManagerKeyManagerBranch::RecoveryByte, 0) + .get_key_at_index(OutputManagerKeyManagerBranch::RecoveryByte.get_branch_key(), 0) .await?; let rewind_data = RewindData { rewind_key, @@ -171,24 +171,26 @@ where } async fn initialise_key_manager(key_manager: &TKeyManagerInterface) -> Result<(), OutputManagerError> { - key_manager.add_new_branch(OutputManagerKeyManagerBranch::Spend).await?; key_manager - .add_new_branch(OutputManagerKeyManagerBranch::SpendScript) + .add_new_branch(OutputManagerKeyManagerBranch::Spend.get_branch_key()) .await?; key_manager - .add_new_branch(OutputManagerKeyManagerBranch::Coinbase) + .add_new_branch(OutputManagerKeyManagerBranch::SpendScript.get_branch_key()) .await?; key_manager - .add_new_branch(OutputManagerKeyManagerBranch::CoinbaseScript) + .add_new_branch(OutputManagerKeyManagerBranch::Coinbase.get_branch_key()) .await?; key_manager - .add_new_branch(OutputManagerKeyManagerBranch::RecoveryViewOnly) + .add_new_branch(OutputManagerKeyManagerBranch::CoinbaseScript.get_branch_key()) .await?; key_manager - .add_new_branch(OutputManagerKeyManagerBranch::RecoveryByte) + .add_new_branch(OutputManagerKeyManagerBranch::RecoveryViewOnly.get_branch_key()) + .await?; + key_manager + .add_new_branch(OutputManagerKeyManagerBranch::RecoveryByte.get_branch_key()) .await?; match key_manager - .add_new_branch(OutputManagerKeyManagerBranch::RecoveryBlinding) + .add_new_branch(OutputManagerKeyManagerBranch::RecoveryBlinding.get_branch_key()) .await { Ok(_) => Ok(()), @@ -717,12 +719,15 @@ where let result = self .resources .master_key_manager - .get_next_key(OutputManagerKeyManagerBranch::Spend) + .get_next_key(OutputManagerKeyManagerBranch::Spend.get_branch_key()) .await?; let script_key = self .resources .master_key_manager - .get_key_at_index(OutputManagerKeyManagerBranch::SpendScript, result.index) + .get_key_at_index( + OutputManagerKeyManagerBranch::SpendScript.get_branch_key(), + result.index, + ) .await?; Ok((result.key, script_key)) } From 5ae98bb71f1fd22223eca080e4e9051c466ef6a4 Mon Sep 17 00:00:00 2001 From: Philip Date: Tue, 29 Mar 2022 14:29:24 +0100 Subject: [PATCH 3/3] fix tests --- .../tests/output_manager_service_tests/service.rs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) 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 52e1d5b095..190b4b3b98 100644 --- a/base_layer/wallet/tests/output_manager_service_tests/service.rs +++ b/base_layer/wallet/tests/output_manager_service_tests/service.rs @@ -227,15 +227,15 @@ async fn setup_output_manager_service