Fix comments

Author: azarovh

Cargo.lock

@@ -37,7 +37,9 @@ thiserror.workspace = true
 zeroize.workspace = true
 
 [dev-dependencies]
+chainstate-test-framework = { path = "../chainstate/test-framework" }
 test-utils = { path = "../test-utils" }
+
 serial_test = "3.2"
 
 rstest.workspace = true

wallet/Cargo.toml

@@ -2020,9 +2020,9 @@ impl<K: AccountKeyChains> Account<K> {
         let conflicting_txs =
             self.output_cache.update_conflicting_txs(confirmed_tx, block.get_id().into())?;
 
-        for tx_id in conflicting_txs {
-            let id = AccountWalletCreatedTxId::new(acc_id.clone(), tx_id);
-            db_tx.del_user_transaction(&id)?;
+        for (tx_id, tx) in conflicting_txs {
+            db_tx.set_transaction(&AccountWalletTxId::new(acc_id.clone(), tx.id()), &tx)?;
+            db_tx.del_user_transaction(&AccountWalletCreatedTxId::new(acc_id.clone(), tx_id))?;
         }
 
         Ok(())
@@ -2182,9 +2182,9 @@ impl<K: AccountKeyChains> Account<K> {
         let abandoned_txs = self.output_cache.abandon_transaction(tx_id)?;
         let acc_id = self.get_account_id();
 
-        for tx_id in abandoned_txs {
-            let id = AccountWalletCreatedTxId::new(acc_id.clone(), tx_id);
-            db_tx.del_user_transaction(&id)?;
+        for (tx_id, tx) in abandoned_txs {
+            db_tx.set_transaction(&AccountWalletTxId::new(acc_id.clone(), tx.id()), &tx)?;
+            db_tx.del_user_transaction(&AccountWalletCreatedTxId::new(acc_id.clone(), tx_id))?;
         }
 
         Ok(())

wallet/src/account/mod.rs

@@ -15,7 +15,7 @@
 
 use std::{
     cmp::Reverse,
-    collections::{btree_map::Entry, BTreeMap, BTreeSet},
+    collections::{btree_map::Entry, BTreeMap, BTreeSet, VecDeque},
     ops::Add,
 };
 
@@ -749,7 +749,7 @@ impl OutputCache {
         &mut self,
         confirmed_tx: &Transaction,
         block_id: Id<GenBlock>,
-    ) -> WalletResult<Vec<Id<Transaction>>> {
+    ) -> WalletResult<Vec<(Id<Transaction>, WalletTx)>> {
         struct ConflictCheck {
             frozen_token_id: Option<TokenId>,
             confirmed_account_nonce: Option<(AccountType, AccountNonce)>,
@@ -800,58 +800,67 @@ impl OutputCache {
             for unconfirmed in self.unconfirmed_descendants.keys() {
                 let unconfirmed_tx = self.txs.get(unconfirmed).expect("must be present");
 
-                if let WalletTx::Tx(tx) = unconfirmed_tx {
-                    if let Some(frozen_token_id) = conflict_check.frozen_token_id {
-                        if self.violates_frozen_token(unconfirmed_tx, &frozen_token_id) {
-                            conflicting_txs.insert(tx.get_transaction().get_id());
-                            continue;
+                match unconfirmed_tx {
+                    WalletTx::Tx(tx) => {
+                        if let Some(frozen_token_id) = conflict_check.frozen_token_id {
+                            if self.violates_frozen_token(unconfirmed_tx, &frozen_token_id) {
+                                conflicting_txs.insert(tx.get_transaction().get_id());
+                                continue;
+                            }
                         }
-                    }
 
-                    if let Some((confirmed_account, confirmed_account_nonce)) =
-                        conflict_check.confirmed_account_nonce
-                    {
-                        if confirmed_tx.get_id() != tx.get_transaction().get_id()
-                            && uses_conflicting_nonce(
-                                unconfirmed_tx,
-                                confirmed_account,
-                                confirmed_account_nonce,
-                            )
+                        if let Some((confirmed_account, confirmed_account_nonce)) =
+                            conflict_check.confirmed_account_nonce
                         {
-                            conflicting_txs.insert(tx.get_transaction().get_id());
-                            continue;
+                            if confirmed_tx.get_id() != tx.get_transaction().get_id()
+                                && uses_conflicting_nonce(
+                                    unconfirmed_tx,
+                                    confirmed_account,
+                                    confirmed_account_nonce,
+                                )
+                            {
+                                conflicting_txs.insert(tx.get_transaction().get_id());
+                                continue;
+                            }
                         }
                     }
-                }
+                    WalletTx::Block(_) => {
+                        utils::debug_panic_or_log!("Cannot be block reward");
+                    }
+                };
             }
         }
 
         // Remove all descendants of conflicting txs
+        let mut conflicting_tx_ids_with_descendants = vec![];
         let mut conflicting_txs_with_descendants = vec![];
 
         for conflicting_tx in conflicting_txs {
-            let txs_to_rollback = self.remove_from_unconfirmed_descendants(conflicting_tx);
+            let tx_ids_to_rollback = self.remove_from_unconfirmed_descendants(conflicting_tx);
 
             // Mark conflicting tx and its descendants as Conflicting and update OutputCache data accordingly
             let mut tx_to_rollback_data = vec![];
-            for tx_id in txs_to_rollback.iter().rev().copied() {
+            for tx_id in tx_ids_to_rollback.iter().rev().copied() {
                 match self.txs.entry(tx_id.into()) {
                     Entry::Occupied(mut entry) => match entry.get_mut() {
                         WalletTx::Block(_) => {
                             Err(WalletError::TransactionIdCannotMapToBlock(tx_id))
                         }
-                        WalletTx::Tx(tx) => match tx.state() {
-                            TxState::Inactive(_) | TxState::InMempool(_) => {
-                                tx.set_state(TxState::Conflicted(block_id));
-                                tx_to_rollback_data.push(entry.get().clone());
-                                Ok(())
+                        WalletTx::Tx(tx) => {
+                            conflicting_txs_with_descendants.push(WalletTx::Tx(tx.clone()));
+                            match tx.state() {
+                                TxState::Inactive(_) | TxState::InMempool(_) => {
+                                    tx.set_state(TxState::Conflicted(block_id));
+                                    tx_to_rollback_data.push(entry.get().clone());
+                                    Ok(())
+                                }
+                                TxState::Conflicted(..)
+                                | TxState::Abandoned
+                                | TxState::Confirmed(..) => {
+                                    Err(WalletError::CannotMarkTxAsConflictedIfInState(*tx.state()))
+                                }
                             }
-                            TxState::Conflicted(..)
-                            | TxState::Abandoned
-                            | TxState::Confirmed(..) => {
-                                Err(WalletError::CannotMarkTxAsConflictedIfInState(*tx.state()))
-                            }
-                        },
+                        }
                     },
                     Entry::Vacant(_) => {
                         Err(WalletError::CannotFindDescendantTransactionWithId(tx_id))
@@ -863,10 +872,13 @@ impl OutputCache {
                 self.rollback_tx_data(&tx)?;
             }
 
-            conflicting_txs_with_descendants.extend(txs_to_rollback.into_iter());
+            conflicting_tx_ids_with_descendants.extend(tx_ids_to_rollback.into_iter());
         }
 
-        Ok(conflicting_txs_with_descendants)
+        Ok(conflicting_tx_ids_with_descendants
+            .into_iter()
+            .zip_eq(conflicting_txs_with_descendants.into_iter())
+            .collect())
     }
 
     fn violates_frozen_token(&self, unconfirmed_tx: &WalletTx, frozen_token_id: &TokenId) -> bool {
@@ -933,9 +945,9 @@ impl OutputCache {
             TxState::Confirmed(_, _, _) => false,
         };
 
-        if is_unconfirmed && !already_present {
-            self.unconfirmed_descendants.insert(tx_id.clone(), BTreeSet::new());
-        } else if !is_unconfirmed {
+        if is_unconfirmed {
+            self.unconfirmed_descendants.entry(tx_id.clone()).or_default();
+        } else {
             self.unconfirmed_descendants.remove(&tx_id);
         }
 
@@ -1438,16 +1450,21 @@ impl OutputCache {
     }
 
     // Removes a tx and all its descendants from unconfirmed descendants collection.
-    // Returns provided tx and all the descendants in that specific order.
+    // Returns provided tx and all the descendants in such way that parent always comes before its
+    // descendants. Returned collection contains no duplicates.
     fn remove_from_unconfirmed_descendants(
         &mut self,
         tx_id: Id<Transaction>,
     ) -> Vec<Id<Transaction>> {
         let mut all_txs = Vec::new();
-        let mut to_update = BTreeSet::from_iter([OutPointSourceId::from(tx_id)]);
+        let mut all_txs_as_set = BTreeSet::new();
+        let mut to_update: VecDeque<OutPointSourceId> = [tx_id.into()].into();
 
-        while let Some(outpoint_source_id) = to_update.pop_first() {
-            all_txs.push(*outpoint_source_id.get_tx_id().expect("must be a transaction"));
+        while let Some(outpoint_source_id) = to_update.pop_front() {
+            let tx_id = *outpoint_source_id.get_tx_id().expect("must be a transaction");
+            if all_txs_as_set.insert(tx_id) {
+                all_txs.push(tx_id);
+            }
 
             if let Some(descendants) = self.unconfirmed_descendants.remove(&outpoint_source_id) {
                 to_update.extend(descendants.into_iter())
@@ -1501,7 +1518,7 @@ impl OutputCache {
             }
         }
 
-        for output in tx.outputs() {
+        for output in tx.outputs().iter().rev() {
             match output {
                 TxOutput::CreateStakePool(pool_id, _) => {
                     self.pools.remove(pool_id);
@@ -1542,43 +1559,47 @@ impl OutputCache {
     }
 
     /// Mark a transaction and its descendants as abandoned
-    /// Returns a Vec of the transaction Ids that have been abandoned
+    /// Returns a Vec of the transactions and the Ids that have been abandoned
     pub fn abandon_transaction(
         &mut self,
         tx_id: Id<Transaction>,
-    ) -> WalletResult<Vec<Id<Transaction>>> {
+    ) -> WalletResult<Vec<(Id<Transaction>, WalletTx)>> {
         let all_abandoned = self.remove_from_unconfirmed_descendants(tx_id);
+        let mut all_abandoned_txs = Vec::with_capacity(all_abandoned.len());
 
         let mut txs_to_rollback = vec![];
         for tx_id in all_abandoned.iter().rev().copied() {
             match self.txs.entry(tx_id.into()) {
                 Entry::Occupied(mut entry) => match entry.get_mut() {
                     WalletTx::Block(_) => Err(WalletError::CannotFindTransactionWithId(tx_id)),
-                    WalletTx::Tx(tx) => match tx.state() {
-                        TxState::Inactive(_) => {
-                            tx.set_state(TxState::Abandoned);
-                            txs_to_rollback.push(entry.get().clone());
-                            Ok(())
-                        }
-                        TxState::Conflicted(_) => {
-                            tx.set_state(TxState::Abandoned);
-                            Ok(())
+                    WalletTx::Tx(tx) => {
+                        all_abandoned_txs.push(WalletTx::Tx(tx.clone()));
+                        match tx.state() {
+                            TxState::Inactive(_) => {
+                                tx.set_state(TxState::Abandoned);
+                                txs_to_rollback.push(entry.get().clone());
+                                Ok(())
+                            }
+                            TxState::Conflicted(_) => {
+                                tx.set_state(TxState::Abandoned);
+                                Ok(())
+                            }
+                            state => Err(WalletError::CannotChangeTransactionState(
+                                *state,
+                                TxState::Abandoned,
+                            )),
                         }
-                        state => Err(WalletError::CannotChangeTransactionState(
-                            *state,
-                            TxState::Abandoned,
-                        )),
-                    },
+                    }
                 },
                 Entry::Vacant(_) => Err(WalletError::CannotFindTransactionWithId(tx_id)),
             }?;
         }
 
-        for tx in txs_to_rollback {
-            self.rollback_tx_data(&tx)?;
+        for tx in &txs_to_rollback {
+            self.rollback_tx_data(tx)?;
         }
 
-        Ok(all_abandoned)
+        Ok(all_abandoned.into_iter().zip_eq(all_abandoned_txs.into_iter()).collect())
     }
 
     pub fn get_transaction(&self, transaction_id: Id<Transaction>) -> WalletResult<&TxData> {
@@ -1840,3 +1861,6 @@ fn uses_conflicting_nonce(
         }
     })
 }
+
+#[cfg(test)]
+mod tests;