From 9b02cd05ea3362506d6ddbb656006beb1652592c Mon Sep 17 00:00:00 2001 From: fluidvanadium Date: Fri, 12 Apr 2024 15:21:33 +0000 Subject: [PATCH 01/13] moved transaction_records_by_id methods into it --- .../blaze/block_management_reorg_detection.rs | 2 +- .../src/wallet/transaction_records_by_id.rs | 98 ++++++++++++++++++- zingolib/src/wallet/transactions.rs | 6 +- zingolib/src/wallet/transactions/recording.rs | 81 ++------------- 4 files changed, 107 insertions(+), 80 deletions(-) diff --git a/zingolib/src/blaze/block_management_reorg_detection.rs b/zingolib/src/blaze/block_management_reorg_detection.rs index 58b999876..fb0647208 100644 --- a/zingolib/src/blaze/block_management_reorg_detection.rs +++ b/zingolib/src/blaze/block_management_reorg_detection.rs @@ -334,7 +334,7 @@ impl BlockManagementData { transaction_metadata_set .write() .await - .remove_txns_at_height(reorg_height); + .invalidate_all_transactions_after_or_at_height(reorg_height); } } diff --git a/zingolib/src/wallet/transaction_records_by_id.rs b/zingolib/src/wallet/transaction_records_by_id.rs index 6e1d9e2e5..88f2160b8 100644 --- a/zingolib/src/wallet/transaction_records_by_id.rs +++ b/zingolib/src/wallet/transaction_records_by_id.rs @@ -1,8 +1,18 @@ use std::collections::HashMap; +use orchard::note_encryption::OrchardDomain; +use sapling_crypto::note_encryption::SaplingDomain; + +use zcash_note_encryption::Domain; +use zcash_primitives::consensus::BlockHeight; + use zcash_primitives::transaction::TxId; -use crate::wallet::data::TransactionRecord; +use crate::wallet::{ + data::TransactionRecord, + notes::NoteInterface, + traits::{DomainWalletExt, Recipient}, +}; #[derive(Debug)] pub struct TransactionRecordsById(pub HashMap); @@ -21,8 +31,94 @@ impl std::ops::DerefMut for TransactionRecordsById { } } impl TransactionRecordsById { + // initializers + // Associated function to create a TransactionRecordMap from a HashMap + pub fn default() -> Self { + TransactionRecordsById(HashMap::new()) + } pub fn from_map(map: HashMap) -> Self { TransactionRecordsById(map) } + + // modify methods + + pub fn invalidate_all_transactions_after_or_at_height(&mut self, reorg_height: BlockHeight) { + // First, collect txids that need to be removed + let txids_to_remove = self + .values() + .filter_map(|transaction_metadata| { + if transaction_metadata + .status + .is_confirmed_after_or_at(&reorg_height) + || transaction_metadata + .status + .is_broadcast_after_or_at(&reorg_height) + // tODo: why dont we only remove confirmed transactions. unconfirmed transactions may still be valid in the mempool and may later confirm or expire. + { + Some(transaction_metadata.txid) + } else { + None + } + }) + .collect::>(); + self.invalidate_txids(txids_to_remove); + } + + /// this function invalidiates a vec of txids + pub(crate) fn invalidate_txids(&mut self, txids_to_remove: Vec) { + for txid in &txids_to_remove { + self.remove(txid); + } + + // invalidate (roll back) any transparent spends in each invalidated tx + self.values_mut().for_each(|transaction_metadata| { + // Update UTXOs to roll back any spent utxos + transaction_metadata + .transparent_notes + .iter_mut() + .for_each(|utxo| { + if utxo.is_spent() && txids_to_remove.contains(&utxo.spent().unwrap().0) { + *utxo.spent_mut() = None; + } + + if utxo.unconfirmed_spent.is_some() + && txids_to_remove.contains(&utxo.unconfirmed_spent.unwrap().0) + { + utxo.unconfirmed_spent = None; + } + }) + }); + // roll back any sapling spends in each invalidated tx + self.invalidate_domain_specific_txids::(&txids_to_remove); + // roll back any orchard spends in each invalidated tx + self.invalidate_domain_specific_txids::(&txids_to_remove); + } + + pub(crate) fn invalidate_domain_specific_txids( + &mut self, + txids_to_remove: &[TxId], + ) where + ::Recipient: Recipient, + ::Note: PartialEq + Clone, + { + self.values_mut().for_each(|transaction_metadata| { + // Update notes to rollback any spent notes + D::to_notes_vec_mut(transaction_metadata) + .iter_mut() + .for_each(|nd| { + // Mark note as unspent if the txid being removed spent it. + if nd.spent().is_some() && txids_to_remove.contains(&nd.spent().unwrap().0) { + *nd.spent_mut() = None; + } + + // Remove unconfirmed spends too + if nd.pending_spent().is_some() + && txids_to_remove.contains(&nd.pending_spent().unwrap().0) + { + *nd.pending_spent_mut() = None; + } + }); + }); + } } diff --git a/zingolib/src/wallet/transactions.rs b/zingolib/src/wallet/transactions.rs index 60fc05d4d..b666f1762 100644 --- a/zingolib/src/wallet/transactions.rs +++ b/zingolib/src/wallet/transactions.rs @@ -1,5 +1,3 @@ -use std::collections::HashMap; - use crate::wallet::{data::WitnessTrees, transaction_records_by_id::TransactionRecordsById}; /// HashMap of all transactions in a wallet, keyed by txid. @@ -17,13 +15,13 @@ pub mod recording; impl TxMapAndMaybeTrees { pub(crate) fn new_with_witness_trees() -> TxMapAndMaybeTrees { Self { - transaction_records_by_id: TransactionRecordsById(HashMap::new()), + transaction_records_by_id: TransactionRecordsById::default(), witness_trees: Some(WitnessTrees::default()), } } pub(crate) fn new_treeless() -> TxMapAndMaybeTrees { Self { - transaction_records_by_id: TransactionRecordsById(HashMap::new()), + transaction_records_by_id: TransactionRecordsById::default(), witness_trees: None, } } diff --git a/zingolib/src/wallet/transactions/recording.rs b/zingolib/src/wallet/transactions/recording.rs index b0eb7eff5..b4b7fc1a0 100644 --- a/zingolib/src/wallet/transactions/recording.rs +++ b/zingolib/src/wallet/transactions/recording.rs @@ -24,85 +24,18 @@ use crate::{ use super::TxMapAndMaybeTrees; impl TxMapAndMaybeTrees { - pub fn remove_txids(&mut self, txids_to_remove: Vec) { - for txid in &txids_to_remove { - self.transaction_records_by_id.remove(txid); - } + pub fn invalidate_txids(&mut self, txids_to_remove: Vec) { self.transaction_records_by_id - .values_mut() - .for_each(|transaction_metadata| { - // Update UTXOs to rollback any spent utxos - transaction_metadata - .transparent_notes - .iter_mut() - .for_each(|utxo| { - if utxo.is_spent() && txids_to_remove.contains(&utxo.spent().unwrap().0) { - *utxo.spent_mut() = None; - } - - if utxo.unconfirmed_spent.is_some() - && txids_to_remove.contains(&utxo.unconfirmed_spent.unwrap().0) - { - utxo.unconfirmed_spent = None; - } - }) - }); - self.remove_domain_specific_txids::(&txids_to_remove); - self.remove_domain_specific_txids::(&txids_to_remove); - } - - fn remove_domain_specific_txids(&mut self, txids_to_remove: &[TxId]) - where - ::Recipient: Recipient, - ::Note: PartialEq + Clone, - { - self.transaction_records_by_id - .values_mut() - .for_each(|transaction_metadata| { - // Update notes to rollback any spent notes - D::to_notes_vec_mut(transaction_metadata) - .iter_mut() - .for_each(|nd| { - // Mark note as unspent if the txid being removed spent it. - if nd.spent().is_some() && txids_to_remove.contains(&nd.spent().unwrap().0) - { - *nd.spent_mut() = None; - } - - // Remove unconfirmed spends too - if nd.pending_spent().is_some() - && txids_to_remove.contains(&nd.pending_spent().unwrap().0) - { - *nd.pending_spent_mut() = None; - } - }); - }); + .invalidate_txids(txids_to_remove) } // During reorgs, we need to remove all txns at a given height, and all spends that refer to any removed txns. - pub fn remove_txns_at_height(&mut self, reorg_height: u64) { + pub fn invalidate_all_transactions_after_or_at_height(&mut self, reorg_height: u64) { let reorg_height = BlockHeight::from_u32(reorg_height as u32); - // First, collect txids that need to be removed - let txids_to_remove = self - .transaction_records_by_id - .values() - .filter_map(|transaction_metadata| { - if transaction_metadata - .status - .is_confirmed_after_or_at(&reorg_height) - || transaction_metadata - .status - .is_broadcast_after_or_at(&reorg_height) - // tODo: why dont we only remove confirmed transactions. unconfirmed transactions may still be valid in the mempool and may later confirm or expire. - { - Some(transaction_metadata.txid) - } else { - None - } - }) - .collect::>(); - self.remove_txids(txids_to_remove); + self.transaction_records_by_id + .invalidate_all_transactions_after_or_at_height(reorg_height); + if let Some(ref mut t) = self.witness_trees { t.witness_tree_sapling .truncate_removing_checkpoint(&(reorg_height - 1)) @@ -130,7 +63,7 @@ impl TxMapAndMaybeTrees { .iter() .for_each(|t| println!("Removing expired mempool tx {}", t)); - self.remove_txids(txids_to_remove); + self.invalidate_txids(txids_to_remove); } // Check this transaction to see if it is an outgoing transaction, and if it is, mark all received notes with non-textual memos in this From 22110be60177a96098d5059e71536474f6eaeeb3 Mon Sep 17 00:00:00 2001 From: fluidvanadium Date: Mon, 15 Apr 2024 15:47:40 +0000 Subject: [PATCH 02/13] Addressed review comments: Named functions and parameters for clarity Helperized new constructor Added doc comments --- .../src/wallet/transaction_records_by_id.rs | 49 +++++++++++-------- zingolib/src/wallet/transactions.rs | 4 +- 2 files changed, 30 insertions(+), 23 deletions(-) diff --git a/zingolib/src/wallet/transaction_records_by_id.rs b/zingolib/src/wallet/transaction_records_by_id.rs index 88f2160b8..62c626c3c 100644 --- a/zingolib/src/wallet/transaction_records_by_id.rs +++ b/zingolib/src/wallet/transaction_records_by_id.rs @@ -30,19 +30,21 @@ impl std::ops::DerefMut for TransactionRecordsById { &mut self.0 } } -impl TransactionRecordsById { - // initializers - // Associated function to create a TransactionRecordMap from a HashMap - pub fn default() -> Self { +/// This block implements constructors. +impl TransactionRecordsById { + // New empty Map + pub fn new() -> Self { TransactionRecordsById(HashMap::new()) } + // Associated function to create a TransactionRecordMap from a HashMap pub fn from_map(map: HashMap) -> Self { TransactionRecordsById(map) } - - // modify methods - +} +/// This block implements methods to modify the map. +impl TransactionRecordsById { + /// All information after a certain height is invalidated during a reorg. pub fn invalidate_all_transactions_after_or_at_height(&mut self, reorg_height: BlockHeight) { // First, collect txids that need to be removed let txids_to_remove = self @@ -64,40 +66,45 @@ impl TransactionRecordsById { .collect::>(); self.invalidate_txids(txids_to_remove); } - - /// this function invalidiates a vec of txids + /// this function invalidiates a vec of txids by removing them and then all references to them. pub(crate) fn invalidate_txids(&mut self, txids_to_remove: Vec) { for txid in &txids_to_remove { self.remove(txid); } - // invalidate (roll back) any transparent spends in each invalidated tx + self.invalidate_txid_specific_transparent_spends(&txids_to_remove); + // roll back any sapling spends in each invalidated tx + self.invalidate_txid_specific_domain_spends::(&txids_to_remove); + // roll back any orchard spends in each invalidated tx + self.invalidate_txid_specific_domain_spends::(&txids_to_remove); + } + /// any transparent note that is listed as being spent in one of these transactions will be marked as unspent. + pub(crate) fn invalidate_txid_specific_transparent_spends( + &mut self, + invalidated_txids: &Vec, + ) { self.values_mut().for_each(|transaction_metadata| { // Update UTXOs to roll back any spent utxos transaction_metadata .transparent_notes .iter_mut() .for_each(|utxo| { - if utxo.is_spent() && txids_to_remove.contains(&utxo.spent().unwrap().0) { + if utxo.is_spent() && invalidated_txids.contains(&utxo.spent().unwrap().0) { *utxo.spent_mut() = None; } if utxo.unconfirmed_spent.is_some() - && txids_to_remove.contains(&utxo.unconfirmed_spent.unwrap().0) + && invalidated_txids.contains(&utxo.unconfirmed_spent.unwrap().0) { utxo.unconfirmed_spent = None; } }) }); - // roll back any sapling spends in each invalidated tx - self.invalidate_domain_specific_txids::(&txids_to_remove); - // roll back any orchard spends in each invalidated tx - self.invalidate_domain_specific_txids::(&txids_to_remove); } - - pub(crate) fn invalidate_domain_specific_txids( + /// any shielded note of the specified domain that is listed as being spent in one of these transactions will be marked as unspent. + pub(crate) fn invalidate_txid_specific_domain_spends( &mut self, - txids_to_remove: &[TxId], + invalidated_txids: &[TxId], ) where ::Recipient: Recipient, ::Note: PartialEq + Clone, @@ -108,13 +115,13 @@ impl TransactionRecordsById { .iter_mut() .for_each(|nd| { // Mark note as unspent if the txid being removed spent it. - if nd.spent().is_some() && txids_to_remove.contains(&nd.spent().unwrap().0) { + if nd.spent().is_some() && invalidated_txids.contains(&nd.spent().unwrap().0) { *nd.spent_mut() = None; } // Remove unconfirmed spends too if nd.pending_spent().is_some() - && txids_to_remove.contains(&nd.pending_spent().unwrap().0) + && invalidated_txids.contains(&nd.pending_spent().unwrap().0) { *nd.pending_spent_mut() = None; } diff --git a/zingolib/src/wallet/transactions.rs b/zingolib/src/wallet/transactions.rs index b666f1762..c63d926fc 100644 --- a/zingolib/src/wallet/transactions.rs +++ b/zingolib/src/wallet/transactions.rs @@ -15,13 +15,13 @@ pub mod recording; impl TxMapAndMaybeTrees { pub(crate) fn new_with_witness_trees() -> TxMapAndMaybeTrees { Self { - transaction_records_by_id: TransactionRecordsById::default(), + transaction_records_by_id: TransactionRecordsById::new(), witness_trees: Some(WitnessTrees::default()), } } pub(crate) fn new_treeless() -> TxMapAndMaybeTrees { Self { - transaction_records_by_id: TransactionRecordsById::default(), + transaction_records_by_id: TransactionRecordsById::new(), witness_trees: None, } } From 85317445561ed00df5cd13926cdaab69f4785527 Mon Sep 17 00:00:00 2001 From: fluidvanadium Date: Mon, 15 Apr 2024 16:16:03 +0000 Subject: [PATCH 03/13] clippy auto --- zingolib/src/wallet/transaction_records_by_id.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/zingolib/src/wallet/transaction_records_by_id.rs b/zingolib/src/wallet/transaction_records_by_id.rs index 62c626c3c..2c74608d3 100644 --- a/zingolib/src/wallet/transaction_records_by_id.rs +++ b/zingolib/src/wallet/transaction_records_by_id.rs @@ -32,6 +32,12 @@ impl std::ops::DerefMut for TransactionRecordsById { } /// This block implements constructors. +impl Default for TransactionRecordsById { + fn default() -> Self { + Self::new() + } +} + impl TransactionRecordsById { // New empty Map pub fn new() -> Self { From 2ac142a06dd1f13db6a8e3774c39efd986c5ce9d Mon Sep 17 00:00:00 2001 From: fluidvanadium Date: Mon, 15 Apr 2024 16:16:37 +0000 Subject: [PATCH 04/13] clippy p2 --- zingolib/src/wallet/transaction_records_by_id.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zingolib/src/wallet/transaction_records_by_id.rs b/zingolib/src/wallet/transaction_records_by_id.rs index 2c74608d3..8f65e946b 100644 --- a/zingolib/src/wallet/transaction_records_by_id.rs +++ b/zingolib/src/wallet/transaction_records_by_id.rs @@ -87,7 +87,7 @@ impl TransactionRecordsById { /// any transparent note that is listed as being spent in one of these transactions will be marked as unspent. pub(crate) fn invalidate_txid_specific_transparent_spends( &mut self, - invalidated_txids: &Vec, + invalidated_txids: &[TxId], ) { self.values_mut().for_each(|transaction_metadata| { // Update UTXOs to roll back any spent utxos From 0742046c7bcd9baf74d85b11ce327512b90fef99 Mon Sep 17 00:00:00 2001 From: fluidvanadium Date: Tue, 16 Apr 2024 15:04:15 +0000 Subject: [PATCH 05/13] Update zingolib/src/wallet/transactions/recording.rs Co-authored-by: oscar-pepper <109323234+Oscar-Pepper@users.noreply.github.com> Signed-off-by: fluidvanadium --- zingolib/src/wallet/transactions/recording.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zingolib/src/wallet/transactions/recording.rs b/zingolib/src/wallet/transactions/recording.rs index b4b7fc1a0..d32a52b60 100644 --- a/zingolib/src/wallet/transactions/recording.rs +++ b/zingolib/src/wallet/transactions/recording.rs @@ -29,7 +29,7 @@ impl TxMapAndMaybeTrees { .invalidate_txids(txids_to_remove) } - // During reorgs, we need to remove all txns at a given height, and all spends that refer to any removed txns. + /// During reorgs, we need to remove all txns at a given height, and all spends that refer to any removed txns. pub fn invalidate_all_transactions_after_or_at_height(&mut self, reorg_height: u64) { let reorg_height = BlockHeight::from_u32(reorg_height as u32); From 9e7ee48d859f9118bc08e37592fee33cd4f067e3 Mon Sep 17 00:00:00 2001 From: fluidvanadium Date: Tue, 16 Apr 2024 15:05:00 +0000 Subject: [PATCH 06/13] Update zingolib/src/wallet/transaction_records_by_id.rs Co-authored-by: oscar-pepper <109323234+Oscar-Pepper@users.noreply.github.com> Signed-off-by: fluidvanadium --- zingolib/src/wallet/transaction_records_by_id.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/zingolib/src/wallet/transaction_records_by_id.rs b/zingolib/src/wallet/transaction_records_by_id.rs index 8f65e946b..f74465c74 100644 --- a/zingolib/src/wallet/transaction_records_by_id.rs +++ b/zingolib/src/wallet/transaction_records_by_id.rs @@ -107,7 +107,10 @@ impl TransactionRecordsById { }) }); } - /// any shielded note of the specified domain that is listed as being spent in one of these transactions will be marked as unspent. + /// Reverts any spent shielded notes in the given transactions to unspent. + /// + /// This is required in the case that the note was spent in an invalidated transaction. + /// Takes a slice of txids corresponding to the invalidated transactions. pub(crate) fn invalidate_txid_specific_domain_spends( &mut self, invalidated_txids: &[TxId], From dbc21c0df259645edc19d9fe575ab662ab466ebb Mon Sep 17 00:00:00 2001 From: fluidvanadium Date: Tue, 16 Apr 2024 15:05:05 +0000 Subject: [PATCH 07/13] Update zingolib/src/wallet/transaction_records_by_id.rs Co-authored-by: oscar-pepper <109323234+Oscar-Pepper@users.noreply.github.com> Signed-off-by: fluidvanadium --- zingolib/src/wallet/transaction_records_by_id.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/zingolib/src/wallet/transaction_records_by_id.rs b/zingolib/src/wallet/transaction_records_by_id.rs index f74465c74..a472e7ef1 100644 --- a/zingolib/src/wallet/transaction_records_by_id.rs +++ b/zingolib/src/wallet/transaction_records_by_id.rs @@ -84,7 +84,10 @@ impl TransactionRecordsById { // roll back any orchard spends in each invalidated tx self.invalidate_txid_specific_domain_spends::(&txids_to_remove); } - /// any transparent note that is listed as being spent in one of these transactions will be marked as unspent. + /// Reverts any spent transparent notes in the given transactions to unspent. + /// + /// This is required in the case that the note was spent in an invalidated transaction. + /// Takes a slice of txids corresponding to the invalidated transactions. pub(crate) fn invalidate_txid_specific_transparent_spends( &mut self, invalidated_txids: &[TxId], From ae3404218546b373d5c38061622a7e63fd379b6e Mon Sep 17 00:00:00 2001 From: fluidvanadium Date: Tue, 16 Apr 2024 15:05:27 +0000 Subject: [PATCH 08/13] Update zingolib/src/wallet/transaction_records_by_id.rs Co-authored-by: oscar-pepper <109323234+Oscar-Pepper@users.noreply.github.com> Signed-off-by: fluidvanadium --- zingolib/src/wallet/transaction_records_by_id.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/zingolib/src/wallet/transaction_records_by_id.rs b/zingolib/src/wallet/transaction_records_by_id.rs index a472e7ef1..e0f8f55f5 100644 --- a/zingolib/src/wallet/transaction_records_by_id.rs +++ b/zingolib/src/wallet/transaction_records_by_id.rs @@ -50,7 +50,9 @@ impl TransactionRecordsById { } /// This block implements methods to modify the map. impl TransactionRecordsById { - /// All information after a certain height is invalidated during a reorg. + /// Invalidates all transactions from a given height including the block with block height `reorg_height` + /// + /// All information above a certain height is invalidated during a reorg. pub fn invalidate_all_transactions_after_or_at_height(&mut self, reorg_height: BlockHeight) { // First, collect txids that need to be removed let txids_to_remove = self From d1b18bb3060a743c7cf9190bc3079edef9bac6de Mon Sep 17 00:00:00 2001 From: fluidvanadium Date: Tue, 16 Apr 2024 15:05:35 +0000 Subject: [PATCH 09/13] Update zingolib/src/wallet/transaction_records_by_id.rs Co-authored-by: oscar-pepper <109323234+Oscar-Pepper@users.noreply.github.com> Signed-off-by: fluidvanadium --- zingolib/src/wallet/transaction_records_by_id.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zingolib/src/wallet/transaction_records_by_id.rs b/zingolib/src/wallet/transaction_records_by_id.rs index e0f8f55f5..ba3ccd70b 100644 --- a/zingolib/src/wallet/transaction_records_by_id.rs +++ b/zingolib/src/wallet/transaction_records_by_id.rs @@ -39,7 +39,7 @@ impl Default for TransactionRecordsById { } impl TransactionRecordsById { - // New empty Map + /// Constructs a new TransactionRecordsById with an empty map. pub fn new() -> Self { TransactionRecordsById(HashMap::new()) } From 2187965701e0b824f504396122551d639b7faae1 Mon Sep 17 00:00:00 2001 From: fluidvanadium Date: Tue, 16 Apr 2024 15:06:15 +0000 Subject: [PATCH 10/13] Update zingolib/src/wallet/transaction_records_by_id.rs Co-authored-by: oscar-pepper <109323234+Oscar-Pepper@users.noreply.github.com> Signed-off-by: fluidvanadium --- zingolib/src/wallet/transaction_records_by_id.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/zingolib/src/wallet/transaction_records_by_id.rs b/zingolib/src/wallet/transaction_records_by_id.rs index ba3ccd70b..e164ba13f 100644 --- a/zingolib/src/wallet/transaction_records_by_id.rs +++ b/zingolib/src/wallet/transaction_records_by_id.rs @@ -33,6 +33,7 @@ impl std::ops::DerefMut for TransactionRecordsById { /// This block implements constructors. impl Default for TransactionRecordsById { + /// Default constructor fn default() -> Self { Self::new() } From e05b6d669821d9c22728cc8c39a3f3b3e3373678 Mon Sep 17 00:00:00 2001 From: fluidvanadium Date: Tue, 16 Apr 2024 15:20:19 +0000 Subject: [PATCH 11/13] clarified some function names. added some doc-comments. removed an accessory function --- Cargo.lock | 1 + .../src/wallet/transaction_records_by_id.rs | 26 +++++++++---------- zingolib/src/wallet/transactions/recording.rs | 9 +++---- 3 files changed, 16 insertions(+), 20 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8bbd2d834..0e7c90bb6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4292,6 +4292,7 @@ dependencies = [ "zingo-status", "zingo-testvectors", "zingoconfig", + "zip32", ] [[package]] diff --git a/zingolib/src/wallet/transaction_records_by_id.rs b/zingolib/src/wallet/transaction_records_by_id.rs index e164ba13f..3bad13819 100644 --- a/zingolib/src/wallet/transaction_records_by_id.rs +++ b/zingolib/src/wallet/transaction_records_by_id.rs @@ -73,25 +73,26 @@ impl TransactionRecordsById { } }) .collect::>(); - self.invalidate_txids(txids_to_remove); + self.invalidate_transactions(txids_to_remove); } - /// this function invalidiates a vec of txids by removing them and then all references to them. - pub(crate) fn invalidate_txids(&mut self, txids_to_remove: Vec) { + /// This function invalidiates a vec of txids by removing them and then all references to them. + /// + /// A transaction can be invalidated either by a reorg or if it was never confirmed by a miner. + /// This is required in the case that a note was spent in a invalidated transaction. + /// Takes a slice of txids corresponding to the invalidated transactions. + pub(crate) fn invalidate_transactions(&mut self, txids_to_remove: Vec) { for txid in &txids_to_remove { self.remove(txid); } - self.invalidate_txid_specific_transparent_spends(&txids_to_remove); + self.invalidate_transaction_specific_transparent_spends(&txids_to_remove); // roll back any sapling spends in each invalidated tx - self.invalidate_txid_specific_domain_spends::(&txids_to_remove); + self.invalidate_transaction_specific_domain_spends::(&txids_to_remove); // roll back any orchard spends in each invalidated tx - self.invalidate_txid_specific_domain_spends::(&txids_to_remove); + self.invalidate_transaction_specific_domain_spends::(&txids_to_remove); } /// Reverts any spent transparent notes in the given transactions to unspent. - /// - /// This is required in the case that the note was spent in an invalidated transaction. - /// Takes a slice of txids corresponding to the invalidated transactions. - pub(crate) fn invalidate_txid_specific_transparent_spends( + pub(crate) fn invalidate_transaction_specific_transparent_spends( &mut self, invalidated_txids: &[TxId], ) { @@ -114,10 +115,7 @@ impl TransactionRecordsById { }); } /// Reverts any spent shielded notes in the given transactions to unspent. - /// - /// This is required in the case that the note was spent in an invalidated transaction. - /// Takes a slice of txids corresponding to the invalidated transactions. - pub(crate) fn invalidate_txid_specific_domain_spends( + pub(crate) fn invalidate_transaction_specific_domain_spends( &mut self, invalidated_txids: &[TxId], ) where diff --git a/zingolib/src/wallet/transactions/recording.rs b/zingolib/src/wallet/transactions/recording.rs index d32a52b60..91e8d35b5 100644 --- a/zingolib/src/wallet/transactions/recording.rs +++ b/zingolib/src/wallet/transactions/recording.rs @@ -24,11 +24,6 @@ use crate::{ use super::TxMapAndMaybeTrees; impl TxMapAndMaybeTrees { - pub fn invalidate_txids(&mut self, txids_to_remove: Vec) { - self.transaction_records_by_id - .invalidate_txids(txids_to_remove) - } - /// During reorgs, we need to remove all txns at a given height, and all spends that refer to any removed txns. pub fn invalidate_all_transactions_after_or_at_height(&mut self, reorg_height: u64) { let reorg_height = BlockHeight::from_u32(reorg_height as u32); @@ -47,6 +42,7 @@ impl TxMapAndMaybeTrees { } } + /// Invalidates all those transactions which were broadcast but never 'confirmed' accepted by a miner. pub(crate) fn clear_expired_mempool(&mut self, latest_height: u64) { let cutoff = BlockHeight::from_u32((latest_height.saturating_sub(MAX_REORG as u64)) as u32); @@ -63,7 +59,8 @@ impl TxMapAndMaybeTrees { .iter() .for_each(|t| println!("Removing expired mempool tx {}", t)); - self.invalidate_txids(txids_to_remove); + self.transaction_records_by_id + .invalidate_transactions(txids_to_remove); } // Check this transaction to see if it is an outgoing transaction, and if it is, mark all received notes with non-textual memos in this From a3af1cfc68717db648a79fce4f4acb0d5bca7b0f Mon Sep 17 00:00:00 2001 From: fluidvanadium Date: Tue, 16 Apr 2024 15:57:47 +0000 Subject: [PATCH 12/13] improve doc comments --- .../src/wallet/transaction_records_by_id.rs | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/zingolib/src/wallet/transaction_records_by_id.rs b/zingolib/src/wallet/transaction_records_by_id.rs index 3bad13819..21c1477d5 100644 --- a/zingolib/src/wallet/transaction_records_by_id.rs +++ b/zingolib/src/wallet/transaction_records_by_id.rs @@ -31,25 +31,18 @@ impl std::ops::DerefMut for TransactionRecordsById { } } -/// This block implements constructors. -impl Default for TransactionRecordsById { - /// Default constructor - fn default() -> Self { - Self::new() - } -} - +/// Constructors impl TransactionRecordsById { /// Constructs a new TransactionRecordsById with an empty map. pub fn new() -> Self { TransactionRecordsById(HashMap::new()) } - // Associated function to create a TransactionRecordMap from a HashMap + // Constructs a TransactionRecordMap from a HashMap pub fn from_map(map: HashMap) -> Self { TransactionRecordsById(map) } } -/// This block implements methods to modify the map. +/// Methods to modify the map. impl TransactionRecordsById { /// Invalidates all transactions from a given height including the block with block height `reorg_height` /// @@ -75,7 +68,7 @@ impl TransactionRecordsById { .collect::>(); self.invalidate_transactions(txids_to_remove); } - /// This function invalidiates a vec of txids by removing them and then all references to them. + /// Invalidiates a vec of txids by removing them and then all references to them. /// /// A transaction can be invalidated either by a reorg or if it was never confirmed by a miner. /// This is required in the case that a note was spent in a invalidated transaction. @@ -142,3 +135,10 @@ impl TransactionRecordsById { }); } } + +impl Default for TransactionRecordsById { + /// Default constructor + fn default() -> Self { + Self::new() + } +} From 9861c489ccef058936a12923a0830b70baf7ca5c Mon Sep 17 00:00:00 2001 From: fluidvanadium Date: Tue, 16 Apr 2024 20:24:19 +0000 Subject: [PATCH 13/13] even clearer doc-comment --- zingolib/src/wallet/transaction_records_by_id.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zingolib/src/wallet/transaction_records_by_id.rs b/zingolib/src/wallet/transaction_records_by_id.rs index 21c1477d5..15a401728 100644 --- a/zingolib/src/wallet/transaction_records_by_id.rs +++ b/zingolib/src/wallet/transaction_records_by_id.rs @@ -72,7 +72,7 @@ impl TransactionRecordsById { /// /// A transaction can be invalidated either by a reorg or if it was never confirmed by a miner. /// This is required in the case that a note was spent in a invalidated transaction. - /// Takes a slice of txids corresponding to the invalidated transactions. + /// Takes a slice of txids corresponding to the invalidated transactions, searches all notes for being spent in one of those txids, and resets them to unspent. pub(crate) fn invalidate_transactions(&mut self, txids_to_remove: Vec) { for txid in &txids_to_remove { self.remove(txid);