Skip to content

Commit c85c14d

Browse files
azarovhazarovh
committed
Fix comments
1 parent babfca4 commit c85c14d

File tree

6 files changed

+364
-83
lines changed

6 files changed

+364
-83
lines changed

‎Cargo.lock

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎wallet/Cargo.toml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,9 @@ thiserror.workspace = true
3737
zeroize.workspace = true
3838

3939
[dev-dependencies]
40+
chainstate-test-framework = { path = "../chainstate/test-framework" }
4041
test-utils = { path = "../test-utils" }
42+
4143
serial_test = "3.2"
4244

4345
rstest.workspace = true

‎wallet/src/account/mod.rs

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2120,9 +2120,9 @@ impl<K: AccountKeyChains> Account<K> {
21202120
let conflicting_txs =
21212121
self.output_cache.update_conflicting_txs(confirmed_tx, block.get_id().into())?;
21222122

2123-
for tx_id in conflicting_txs {
2124-
let id = AccountWalletCreatedTxId::new(acc_id.clone(), tx_id);
2125-
db_tx.del_user_transaction(&id)?;
2123+
for (tx_id, tx) in conflicting_txs {
2124+
db_tx.set_transaction(&AccountWalletTxId::new(acc_id.clone(), tx.id()), &tx)?;
2125+
db_tx.del_user_transaction(&AccountWalletCreatedTxId::new(acc_id.clone(), tx_id))?;
21262126
}
21272127

21282128
Ok(())
@@ -2282,9 +2282,9 @@ impl<K: AccountKeyChains> Account<K> {
22822282
let abandoned_txs = self.output_cache.abandon_transaction(tx_id)?;
22832283
let acc_id = self.get_account_id();
22842284

2285-
for tx_id in abandoned_txs {
2286-
let id = AccountWalletCreatedTxId::new(acc_id.clone(), tx_id);
2287-
db_tx.del_user_transaction(&id)?;
2285+
for (tx_id, tx) in abandoned_txs {
2286+
db_tx.set_transaction(&AccountWalletTxId::new(acc_id.clone(), tx.id()), &tx)?;
2287+
db_tx.del_user_transaction(&AccountWalletCreatedTxId::new(acc_id.clone(), tx_id))?;
22882288
}
22892289

22902290
Ok(())

‎wallet/src/account/output_cache/mod.rs

Lines changed: 106 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515

1616
use std::{
1717
cmp::Reverse,
18-
collections::{btree_map::Entry, BTreeMap, BTreeSet},
18+
collections::{btree_map::Entry, BTreeMap, BTreeSet, VecDeque},
1919
ops::Add,
2020
};
2121

@@ -750,7 +750,7 @@ impl OutputCache {
750750
&mut self,
751751
confirmed_tx: &Transaction,
752752
block_id: Id<GenBlock>,
753-
) -> WalletResult<Vec<Id<Transaction>>> {
753+
) -> WalletResult<Vec<(Id<Transaction>, WalletTx)>> {
754754
struct ConflictCheck {
755755
frozen_token_id: Option<TokenId>,
756756
confirmed_account_nonce: Option<(AccountType, AccountNonce)>,
@@ -802,76 +802,97 @@ impl OutputCache {
802802
for unconfirmed in self.unconfirmed_descendants.keys() {
803803
let unconfirmed_tx = self.txs.get(unconfirmed).expect("must be present");
804804

805-
if let WalletTx::Tx(tx) = unconfirmed_tx {
806-
if let Some(frozen_token_id) = conflict_check.frozen_token_id {
807-
if self.violates_frozen_token(unconfirmed_tx, &frozen_token_id) {
808-
conflicting_txs.insert(tx.get_transaction().get_id());
809-
continue;
805+
match unconfirmed_tx {
806+
WalletTx::Tx(tx) => {
807+
if let Some(frozen_token_id) = conflict_check.frozen_token_id {
808+
if self.violates_frozen_token(unconfirmed_tx, &frozen_token_id) {
809+
conflicting_txs.insert(tx.get_transaction().get_id());
810+
continue;
811+
}
810812
}
811-
}
812813

813-
if let Some((confirmed_account, confirmed_account_nonce)) =
814-
conflict_check.confirmed_account_nonce
815-
{
816-
if confirmed_tx.get_id() != tx.get_transaction().get_id()
817-
&& uses_conflicting_nonce(
818-
unconfirmed_tx,
819-
confirmed_account,
820-
confirmed_account_nonce,
821-
)
814+
if let Some((confirmed_account, confirmed_account_nonce)) =
815+
conflict_check.confirmed_account_nonce
822816
{
823-
conflicting_txs.insert(tx.get_transaction().get_id());
824-
continue;
817+
if confirmed_tx.get_id() != tx.get_transaction().get_id()
818+
&& uses_conflicting_nonce(
819+
unconfirmed_tx,
820+
confirmed_account,
821+
confirmed_account_nonce,
822+
)
823+
{
824+
conflicting_txs.insert(tx.get_transaction().get_id());
825+
continue;
826+
}
825827
}
826828
}
827-
}
829+
WalletTx::Block(_) => {
830+
utils::debug_panic_or_log!("Cannot be block reward");
831+
}
832+
};
828833
}
829834
}
830835

831836
// Remove all descendants of conflicting txs
832837
let mut conflicting_txs_with_descendants = vec![];
833838

839+
// Note: `conflicting_txs` can contain not only parent txs but also children if they have their
840+
// own descendants.
841+
// Because `conflicting_txs` is a set we can't tell the order of processing, so have to track all txs
842+
// to avoid marking as conflicted txs that has already been marked because they are descendants.
843+
let mut processed_transactions = BTreeSet::new();
844+
834845
for conflicting_tx in conflicting_txs {
835-
let txs_to_rollback = self.remove_from_unconfirmed_descendants(conflicting_tx);
846+
let tx_ids_to_rollback = self.remove_from_unconfirmed_descendants(conflicting_tx);
836847

837848
// Mark conflicting tx and its descendants as Conflicting and update OutputCache data accordingly
838849
let mut tx_to_rollback_data = vec![];
839-
for tx_id in txs_to_rollback.iter().rev().copied() {
840-
match self.txs.entry(tx_id.into()) {
841-
Entry::Occupied(mut entry) => match entry.get_mut() {
842-
WalletTx::Block(_) => {
843-
Err(WalletError::TransactionIdCannotMapToBlock(tx_id))
844-
}
845-
WalletTx::Tx(tx) => match tx.state() {
846-
TxState::Inactive(_) | TxState::InMempool(_) => {
847-
tx.set_state(TxState::Conflicted(block_id));
848-
tx_to_rollback_data.push(entry.get().clone());
849-
Ok(())
850+
for tx_id in tx_ids_to_rollback.into_iter().rev() {
851+
if !processed_transactions.contains(&tx_id) {
852+
match self.txs.entry(tx_id.into()) {
853+
Entry::Occupied(mut entry) => match entry.get_mut() {
854+
WalletTx::Block(_) => {
855+
Err(WalletError::TransactionIdCannotMapToBlock(tx_id))
850856
}
851-
TxState::Conflicted(..)
852-
| TxState::Abandoned
853-
| TxState::Confirmed(..) => {
854-
Err(WalletError::CannotMarkTxAsConflictedIfInState(*tx.state()))
857+
WalletTx::Tx(tx) => {
858+
match tx.state() {
859+
TxState::Inactive(_) | TxState::InMempool(_) => {
860+
tx.set_state(TxState::Conflicted(block_id));
861+
tx_to_rollback_data.push(WalletTx::Tx(tx.clone()));
862+
}
863+
TxState::Conflicted(..)
864+
| TxState::Abandoned
865+
| TxState::Confirmed(..) => {
866+
return Err(WalletError::CannotMarkTxAsConflictedIfInState(
867+
*tx.state(),
868+
))
869+
}
870+
}
871+
conflicting_txs_with_descendants
872+
.push((tx_id, WalletTx::Tx(tx.clone())));
873+
processed_transactions.insert(tx_id);
874+
Ok(())
855875
}
856876
},
857-
},
858-
Entry::Vacant(_) => {
859-
Err(WalletError::CannotFindDescendantTransactionWithId(tx_id))
860-
}
861-
}?;
877+
Entry::Vacant(_) => {
878+
Err(WalletError::CannotFindDescendantTransactionWithId(tx_id))
879+
}
880+
}?;
881+
}
862882
}
863883

864884
for tx in tx_to_rollback_data {
865885
self.rollback_tx_data(&tx)?;
866886
}
867-
868-
conflicting_txs_with_descendants.extend(txs_to_rollback.into_iter());
869887
}
870888

871889
Ok(conflicting_txs_with_descendants)
872890
}
873891

874892
fn violates_frozen_token(&self, unconfirmed_tx: &WalletTx, frozen_token_id: &TokenId) -> bool {
893+
// We check only inputs because currently it's not possible to have a token in an output
894+
// without having it in an input.
895+
// But potentially we should check outputs too.
875896
unconfirmed_tx.inputs().iter().any(|inp| match inp {
876897
TxInput::Utxo(outpoint) => self.txs.get(&outpoint.source_id()).is_some_and(|tx| {
877898
let output =
@@ -947,9 +968,9 @@ impl OutputCache {
947968
TxState::Confirmed(_, _, _) => false,
948969
};
949970

950-
if is_unconfirmed && !already_present {
951-
self.unconfirmed_descendants.insert(tx_id.clone(), BTreeSet::new());
952-
} else if !is_unconfirmed {
971+
if is_unconfirmed {
972+
self.unconfirmed_descendants.entry(tx_id.clone()).or_default();
973+
} else {
953974
self.unconfirmed_descendants.remove(&tx_id);
954975
}
955976

@@ -1197,7 +1218,6 @@ impl OutputCache {
11971218
if let Some(descendants) = data
11981219
.last_parent
11991220
.as_ref()
1200-
.filter(|parent_tx_id| *parent_tx_id != tx_id)
12011221
.and_then(|parent_tx_id| unconfirmed_descendants.get_mut(parent_tx_id))
12021222
{
12031223
descendants.insert(tx_id.clone());
@@ -1229,7 +1249,6 @@ impl OutputCache {
12291249
if let Some(descendants) = data
12301250
.last_parent
12311251
.as_ref()
1232-
.filter(|parent_tx_id| *parent_tx_id != tx_id)
12331252
.and_then(|parent_tx_id| unconfirmed_descendants.get_mut(parent_tx_id))
12341253
{
12351254
descendants.insert(tx_id.clone());
@@ -1264,7 +1283,6 @@ impl OutputCache {
12641283
if let Some(descendants) = data
12651284
.last_parent
12661285
.as_ref()
1267-
.filter(|parent_tx_id| *parent_tx_id != tx_id)
12681286
.and_then(|parent_tx_id| unconfirmed_descendants.get_mut(parent_tx_id))
12691287
{
12701288
descendants.insert(tx_id.clone());
@@ -1275,7 +1293,7 @@ impl OutputCache {
12751293

12761294
pub fn remove_confirmed_tx(&mut self, tx_id: &OutPointSourceId) -> WalletResult<()> {
12771295
if let Some(tx) = self.txs.remove(tx_id) {
1278-
matches!(tx.state(), TxState::Confirmed(..));
1296+
assert!(matches!(tx.state(), TxState::Confirmed(..)));
12791297
self.rollback_tx_data(&tx)?;
12801298
}
12811299

@@ -1474,16 +1492,21 @@ impl OutputCache {
14741492
}
14751493

14761494
// Removes a tx and all its descendants from unconfirmed descendants collection.
1477-
// Returns provided tx and all the descendants in that specific order.
1495+
// Returns provided tx and all the descendants in such way that parent always comes before its
1496+
// descendants. Returned collection contains no duplicates.
14781497
fn remove_from_unconfirmed_descendants(
14791498
&mut self,
14801499
tx_id: Id<Transaction>,
14811500
) -> Vec<Id<Transaction>> {
14821501
let mut all_txs = Vec::new();
1483-
let mut to_update = BTreeSet::from_iter([OutPointSourceId::from(tx_id)]);
1502+
let mut all_txs_as_set = BTreeSet::new();
1503+
let mut to_update: VecDeque<OutPointSourceId> = [tx_id.into()].into();
14841504

1485-
while let Some(outpoint_source_id) = to_update.pop_first() {
1486-
all_txs.push(*outpoint_source_id.get_tx_id().expect("must be a transaction"));
1505+
while let Some(outpoint_source_id) = to_update.pop_front() {
1506+
let tx_id = *outpoint_source_id.get_tx_id().expect("must be a transaction");
1507+
if all_txs_as_set.insert(tx_id) {
1508+
all_txs.push(tx_id);
1509+
}
14871510

14881511
if let Some(descendants) = self.unconfirmed_descendants.remove(&outpoint_source_id) {
14891512
to_update.extend(descendants.into_iter())
@@ -1498,7 +1521,8 @@ impl OutputCache {
14981521
fn rollback_tx_data(&mut self, tx: &WalletTx) -> WalletResult<()> {
14991522
let tx_id = tx.id();
15001523

1501-
// Iterate in reverse to handle situations where an account is modified twice in the same tx
1524+
// Iterate in reverse to handle situations where an account is modified twice in the same tx.
1525+
// It's currently impossible to have more than 1 account command but it's safer to use rev() anyway.
15021526
for input in tx.inputs().iter().rev() {
15031527
match input {
15041528
TxInput::Utxo(outpoint) => {
@@ -1572,7 +1596,7 @@ impl OutputCache {
15721596
}
15731597
}
15741598

1575-
for output in tx.outputs() {
1599+
for output in tx.outputs().iter().rev() {
15761600
match output {
15771601
TxOutput::CreateStakePool(pool_id, _) => {
15781602
self.pools.remove(pool_id);
@@ -1613,43 +1637,47 @@ impl OutputCache {
16131637
}
16141638

16151639
/// Mark a transaction and its descendants as abandoned
1616-
/// Returns a Vec of the transaction Ids that have been abandoned
1640+
/// Returns a Vec of the transactions and the Ids that have been abandoned
16171641
pub fn abandon_transaction(
16181642
&mut self,
16191643
tx_id: Id<Transaction>,
1620-
) -> WalletResult<Vec<Id<Transaction>>> {
1644+
) -> WalletResult<Vec<(Id<Transaction>, WalletTx)>> {
16211645
let all_abandoned = self.remove_from_unconfirmed_descendants(tx_id);
1646+
let mut all_abandoned_txs = Vec::with_capacity(all_abandoned.len());
16221647

16231648
let mut txs_to_rollback = vec![];
16241649
for tx_id in all_abandoned.iter().rev().copied() {
16251650
match self.txs.entry(tx_id.into()) {
16261651
Entry::Occupied(mut entry) => match entry.get_mut() {
16271652
WalletTx::Block(_) => Err(WalletError::CannotFindTransactionWithId(tx_id)),
1628-
WalletTx::Tx(tx) => match tx.state() {
1629-
TxState::Inactive(_) => {
1630-
tx.set_state(TxState::Abandoned);
1631-
txs_to_rollback.push(entry.get().clone());
1632-
Ok(())
1633-
}
1634-
TxState::Conflicted(_) => {
1635-
tx.set_state(TxState::Abandoned);
1636-
Ok(())
1653+
WalletTx::Tx(tx) => {
1654+
all_abandoned_txs.push(WalletTx::Tx(tx.clone()));
1655+
match tx.state() {
1656+
TxState::Inactive(_) => {
1657+
tx.set_state(TxState::Abandoned);
1658+
txs_to_rollback.push(entry.get().clone());
1659+
Ok(())
1660+
}
1661+
TxState::Conflicted(_) => {
1662+
tx.set_state(TxState::Abandoned);
1663+
Ok(())
1664+
}
1665+
state => Err(WalletError::CannotChangeTransactionState(
1666+
*state,
1667+
TxState::Abandoned,
1668+
)),
16371669
}
1638-
state => Err(WalletError::CannotChangeTransactionState(
1639-
*state,
1640-
TxState::Abandoned,
1641-
)),
1642-
},
1670+
}
16431671
},
16441672
Entry::Vacant(_) => Err(WalletError::CannotFindTransactionWithId(tx_id)),
16451673
}?;
16461674
}
16471675

1648-
for tx in txs_to_rollback {
1649-
self.rollback_tx_data(&tx)?;
1676+
for tx in &txs_to_rollback {
1677+
self.rollback_tx_data(tx)?;
16501678
}
16511679

1652-
Ok(all_abandoned)
1680+
Ok(all_abandoned.into_iter().zip_eq(all_abandoned_txs).collect())
16531681
}
16541682

16551683
pub fn get_transaction(&self, transaction_id: Id<Transaction>) -> WalletResult<&TxData> {
@@ -1915,3 +1943,6 @@ fn uses_conflicting_nonce(
19151943
}
19161944
})
19171945
}
1946+
1947+
#[cfg(test)]
1948+
mod tests;

0 commit comments

Comments
 (0)