From 7d743af0c3c52c60674445d6ef5be615e8b50a09 Mon Sep 17 00:00:00 2001 From: Heorhii Azarov Date: Fri, 27 Dec 2024 14:22:15 +0200 Subject: [PATCH 1/9] Remove descendants of conflicting txs --- mempool/src/pool/mod.rs | 4 +- .../main_widget/tabs/wallet/delegation.rs | 2 +- wallet/src/account/mod.rs | 15 +- wallet/src/account/output_cache/mod.rs | 184 ++++++++++-------- 4 files changed, 115 insertions(+), 90 deletions(-) diff --git a/mempool/src/pool/mod.rs b/mempool/src/pool/mod.rs index 36e6bbf41..b58f047c7 100644 --- a/mempool/src/pool/mod.rs +++ b/mempool/src/pool/mod.rs @@ -378,7 +378,9 @@ impl<'a> TxFinalizer<'a> { TxAdditionOutcome::Rejected { transaction, error } => { let tx_id = *transaction.tx_id(); let origin = transaction.origin(); - log::trace!("Rejected transaction {tx_id}, checking orphan status"); + log::trace!( + "Rejected transaction {tx_id} with error {error}. Checking orphan status" + ); self.try_add_orphan(tx_pool, transaction, error) .inspect_err(|err| match &mut self.events_mode { diff --git a/node-gui/src/main_window/main_widget/tabs/wallet/delegation.rs b/node-gui/src/main_window/main_widget/tabs/wallet/delegation.rs index d251046dc..9fe4c8d4b 100644 --- a/node-gui/src/main_window/main_widget/tabs/wallet/delegation.rs +++ b/node-gui/src/main_window/main_widget/tabs/wallet/delegation.rs @@ -87,7 +87,7 @@ pub fn view_delegation( .map(|(del_id, (pool_id, b))| (*del_id, *pool_id, *b)) { let delegation_address = Address::new(chain_config, delegation_id) - .expect("Encoding pool id to address can't fail (GUI)"); + .expect("Encoding delegation id to address can't fail (GUI)"); let pool_address = Address::new(chain_config, pool_id) .expect("Encoding pool id to address can't fail (GUI)"); let delegate_staking_amount = diff --git a/wallet/src/account/mod.rs b/wallet/src/account/mod.rs index 78743873d..7c3db32db 100644 --- a/wallet/src/account/mod.rs +++ b/wallet/src/account/mod.rs @@ -2056,7 +2056,8 @@ impl Account { let tx_state = TxState::Confirmed(block_height, block.timestamp(), idx as u64); let wallet_tx = WalletTx::Tx(TxData::new(signed_tx.clone(), tx_state)); - self.update_conflicting_txs(&wallet_tx, block, db_tx)?; + + self.update_conflicting_txs(signed_tx.transaction(), block, db_tx)?; new_tx_was_added |= self .add_wallet_tx_if_relevant_and_remove_from_user_txs( @@ -2084,15 +2085,17 @@ impl Account { /// Check for any conflicting txs and update the new state in the DB fn update_conflicting_txs( &mut self, - wallet_tx: &WalletTx, + confirmed_tx: &Transaction, block: &Block, db_tx: &mut StoreTxRw, ) -> WalletResult<()> { let acc_id = self.get_account_id(); - let conflicting_tx = self.output_cache.check_conflicting(wallet_tx, block.get_id().into()); - for tx in conflicting_tx { - let id = AccountWalletTxId::new(acc_id.clone(), tx.id()); - db_tx.set_transaction(&id, tx)?; + 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)?; } Ok(()) diff --git a/wallet/src/account/output_cache/mod.rs b/wallet/src/account/output_cache/mod.rs index a0dec7261..3c943dc31 100644 --- a/wallet/src/account/output_cache/mod.rs +++ b/wallet/src/account/output_cache/mod.rs @@ -680,20 +680,15 @@ impl OutputCache { )) } - pub fn check_conflicting(&mut self, tx: &WalletTx, block_id: Id) -> Vec<&WalletTx> { - let is_unconfirmed = match tx.state() { - TxState::Inactive(_) - | TxState::InMempool(_) - | TxState::Conflicted(_) - | TxState::Abandoned => true, - TxState::Confirmed(_, _, _) => false, - }; - - if is_unconfirmed { - return vec![]; - } + pub fn update_conflicting_txs( + &mut self, + confirmed_tx: &Transaction, + block_id: Id, + ) -> WalletResult>> { + // Collect all conflicting txs + let mut conflicting_txs = vec![]; - let frozen_token_id = tx.inputs().iter().find_map(|inp| match inp { + let frozen_token_id = confirmed_tx.inputs().iter().find_map(|inp| match inp { TxInput::Utxo(_) | TxInput::Account(_) => None, TxInput::AccountCommand(_, cmd) => match cmd { AccountCommand::MintTokens(_, _) @@ -708,7 +703,6 @@ impl OutputCache { }, }); - let mut conflicting_txs = vec![]; if let Some(frozen_token_id) = frozen_token_id { for unconfirmed in self.unconfirmed_descendants.keys() { let unconfirmed_tx = self.txs.get(unconfirmed).expect("must be present"); @@ -716,20 +710,26 @@ impl OutputCache { let unconfirmed_tx = self.txs.get_mut(unconfirmed).expect("must be present"); match unconfirmed_tx { WalletTx::Tx(ref mut tx) => { - tx.set_state(TxState::Conflicted(block_id)); + conflicting_txs.push(tx.get_transaction().get_id()); } WalletTx::Block(_) => {} }; - - conflicting_txs.push(unconfirmed); } } } - conflicting_txs - .into_iter() - .map(|tx_id| self.txs.get(tx_id).expect("must be present")) - .collect_vec() + // FIXME: check utxos and account nonces here + + // Remove all descendants of conflicting txs + let mut conflicting_txs_with_descendants = vec![]; + for conflicting_tx in conflicting_txs { + let descendants = + self.remove_descendants_and_mark_as(conflicting_tx, TxState::Conflicted(block_id))?; + + conflicting_txs_with_descendants.extend(descendants.into_iter()); + } + + Ok(conflicting_txs_with_descendants) } fn uses_token(&self, unconfirmed_tx: &WalletTx, frozen_token_id: &TokenId) -> bool { @@ -1364,90 +1364,110 @@ impl OutputCache { }) } - /// Mark a transaction and its descendants as abandoned - /// Returns a Vec of the transaction Ids that have been abandoned - pub fn abandon_transaction( + fn remove_descendants_and_mark_as( &mut self, tx_id: Id, + new_state: TxState, ) -> WalletResult>> { - let mut all_abandoned = Vec::new(); - let mut to_abandon = BTreeSet::from_iter([OutPointSourceId::from(tx_id)]); + let mut all_txs = Vec::new(); + let mut to_update = BTreeSet::from_iter([OutPointSourceId::from(tx_id)]); - while let Some(outpoint_source_id) = to_abandon.pop_first() { - all_abandoned.push(*outpoint_source_id.get_tx_id().expect("must be a transaction")); + while let Some(outpoint_source_id) = to_update.pop_first() { + all_txs.push(*outpoint_source_id.get_tx_id().expect("must be a transaction")); if let Some(descendants) = self.unconfirmed_descendants.remove(&outpoint_source_id) { - to_abandon.extend(descendants.into_iter()) + to_update.extend(descendants.into_iter()) } } - for tx_id in all_abandoned.iter().rev().copied() { + for tx_id in all_txs.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(_) | TxState::Conflicted(_) => { - tx.set_state(TxState::Abandoned); - for input in tx.get_transaction().inputs() { - match input { - TxInput::Utxo(outpoint) => { - self.consumed.insert(outpoint.clone(), *tx.state()); - } - TxInput::Account(outpoint) => match outpoint.account() { - AccountSpending::DelegationBalance(delegation_id, _) => { - if let Some(data) = - self.delegations.get_mut(delegation_id) - { - data.last_nonce = outpoint.nonce().decrement(); - data.last_parent = find_parent( - &self.unconfirmed_descendants, - tx_id.into(), - ); - } + WalletTx::Tx(tx) => { + ensure!( + new_state != *tx.state(), + WalletError::CannotAbandonTransaction(*tx.state()) + ); + + match tx.state() { + TxState::Inactive(_) | TxState::Conflicted(_) | TxState::Abandoned => { + tx.set_state(new_state); + for input in tx.get_transaction().inputs() { + match input { + TxInput::Utxo(outpoint) => { + self.consumed.insert(outpoint.clone(), *tx.state()); } - }, - TxInput::AccountCommand(nonce, op) => match op { - AccountCommand::MintTokens(token_id, _) - | AccountCommand::UnmintTokens(token_id) - | AccountCommand::LockTokenSupply(token_id) - | AccountCommand::FreezeToken(token_id, _) - | AccountCommand::UnfreezeToken(token_id) - | AccountCommand::ChangeTokenMetadataUri(token_id, _) - | AccountCommand::ChangeTokenAuthority(token_id, _) => { - if let Some(data) = - self.token_issuance.get_mut(token_id) - { - data.last_nonce = nonce.decrement(); - data.last_parent = find_parent( - &self.unconfirmed_descendants, - tx_id.into(), - ); - data.unconfirmed_txs.remove(&tx_id.into()); + TxInput::Account(outpoint) => match outpoint.account() { + AccountSpending::DelegationBalance( + delegation_id, + _, + ) => { + if let Some(data) = + self.delegations.get_mut(delegation_id) + { + data.last_nonce = outpoint.nonce().decrement(); + data.last_parent = find_parent( + &self.unconfirmed_descendants, + tx_id.into(), + ); + } } - } - AccountCommand::ConcludeOrder(order_id) - | AccountCommand::FillOrder(order_id, _, _) => { - if let Some(data) = self.orders.get_mut(order_id) { - data.last_nonce = nonce.decrement(); - data.last_parent = find_parent( - &self.unconfirmed_descendants, - tx_id.into(), - ); + }, + TxInput::AccountCommand(nonce, op) => match op { + AccountCommand::MintTokens(token_id, _) + | AccountCommand::UnmintTokens(token_id) + | AccountCommand::LockTokenSupply(token_id) + | AccountCommand::FreezeToken(token_id, _) + | AccountCommand::UnfreezeToken(token_id) + | AccountCommand::ChangeTokenMetadataUri(token_id, _) + | AccountCommand::ChangeTokenAuthority(token_id, _) => { + if let Some(data) = + self.token_issuance.get_mut(token_id) + { + data.last_nonce = nonce.decrement(); + data.last_parent = find_parent( + &self.unconfirmed_descendants, + tx_id.into(), + ); + data.unconfirmed_txs.remove(&tx_id.into()); + } } - } - }, + AccountCommand::ConcludeOrder(order_id) + | AccountCommand::FillOrder(order_id, _, _) => { + if let Some(data) = self.orders.get_mut(order_id) { + data.last_nonce = nonce.decrement(); + data.last_parent = find_parent( + &self.unconfirmed_descendants, + tx_id.into(), + ); + } + } + }, + } } + Ok(()) + } + TxState::Confirmed(..) | TxState::InMempool(..) => { + Err(WalletError::CannotAbandonTransaction(*tx.state())) } - Ok(()) } - state => Err(WalletError::CannotAbandonTransaction(*state)), - }, + } }, Entry::Vacant(_) => Err(WalletError::CannotFindTransactionWithId(tx_id)), }?; } - Ok(all_abandoned) + Ok(all_txs) + } + + /// Mark a transaction and its descendants as abandoned + /// Returns a Vec of the transaction Ids that have been abandoned + pub fn abandon_transaction( + &mut self, + tx_id: Id, + ) -> WalletResult>> { + self.remove_descendants_and_mark_as(tx_id, TxState::Abandoned) } pub fn get_transaction(&self, transaction_id: Id) -> WalletResult<&TxData> { From 769da7f21dec9ef24ff4388fe44f377b107a02ee Mon Sep 17 00:00:00 2001 From: Heorhii Azarov Date: Fri, 10 Jan 2025 16:04:35 +0200 Subject: [PATCH 2/9] Use ref for AccountType::From --- .../test-framework/src/block_builder.rs | 4 ++-- .../test-framework/src/pos_block_builder.rs | 4 ++-- .../src/transaction_verifier/mod.rs | 24 +++++++++---------- .../src/chain/transaction/account_outpoint.rs | 14 +++++------ mempool/src/pool/entry.rs | 6 ++--- 5 files changed, 26 insertions(+), 26 deletions(-) diff --git a/chainstate/test-framework/src/block_builder.rs b/chainstate/test-framework/src/block_builder.rs index a4750d06e..d4e814f02 100644 --- a/chainstate/test-framework/src/block_builder.rs +++ b/chainstate/test-framework/src/block_builder.rs @@ -210,10 +210,10 @@ impl<'f> BlockBuilder<'f> { } TxInput::Account(outpoint) => { self.account_nonce_tracker - .insert(outpoint.account().clone().into(), outpoint.nonce()); + .insert(outpoint.account().into(), outpoint.nonce()); } TxInput::AccountCommand(nonce, op) => { - self.account_nonce_tracker.insert(op.clone().into(), *nonce); + self.account_nonce_tracker.insert(op.into(), *nonce); } }; }); diff --git a/chainstate/test-framework/src/pos_block_builder.rs b/chainstate/test-framework/src/pos_block_builder.rs index 8153214b8..58fcffbbf 100644 --- a/chainstate/test-framework/src/pos_block_builder.rs +++ b/chainstate/test-framework/src/pos_block_builder.rs @@ -450,10 +450,10 @@ impl<'f> PoSBlockBuilder<'f> { } TxInput::Account(outpoint) => { self.account_nonce_tracker - .insert(outpoint.account().clone().into(), outpoint.nonce()); + .insert(outpoint.account().into(), outpoint.nonce()); } TxInput::AccountCommand(nonce, op) => { - self.account_nonce_tracker.insert(op.clone().into(), *nonce); + self.account_nonce_tracker.insert(op.into(), *nonce); } }; }); diff --git a/chainstate/tx-verifier/src/transaction_verifier/mod.rs b/chainstate/tx-verifier/src/transaction_verifier/mod.rs index 2a08adbb2..ccefcc8c7 100644 --- a/chainstate/tx-verifier/src/transaction_verifier/mod.rs +++ b/chainstate/tx-verifier/src/transaction_verifier/mod.rs @@ -367,7 +367,7 @@ where let res = self .spend_input_from_account( outpoint.nonce(), - outpoint.account().clone().into(), + outpoint.account().into(), ) .and_then(|_| { // If the input spends from delegation account, this means the user is @@ -496,10 +496,10 @@ where match input { TxInput::Utxo(_) => { /* do nothing */ } TxInput::Account(outpoint) => { - self.unspend_input_from_account(outpoint.account().clone().into())?; + self.unspend_input_from_account(outpoint.account().into())?; } TxInput::AccountCommand(_, account_op) => { - self.unspend_input_from_account(account_op.clone().into())?; + self.unspend_input_from_account(account_op.into())?; } }; } @@ -553,7 +553,7 @@ where TxInput::AccountCommand(nonce, account_op) => match account_op { AccountCommand::MintTokens(token_id, amount) => { let res = self - .spend_input_from_account(*nonce, account_op.clone().into()) + .spend_input_from_account(*nonce, account_op.into()) .and_then(|_| { self.tokens_accounting_cache .mint_tokens(*token_id, *amount) @@ -563,7 +563,7 @@ where } AccountCommand::UnmintTokens(ref token_id) => { let res = self - .spend_input_from_account(*nonce, account_op.clone().into()) + .spend_input_from_account(*nonce, account_op.into()) .and_then(|_| { // actual amount to unmint is determined by the number of burned tokens in the outputs let total_burned = @@ -582,7 +582,7 @@ where } AccountCommand::LockTokenSupply(token_id) => { let res = self - .spend_input_from_account(*nonce, account_op.clone().into()) + .spend_input_from_account(*nonce, account_op.into()) .and_then(|_| { self.tokens_accounting_cache .lock_circulating_supply(*token_id) @@ -592,7 +592,7 @@ where } AccountCommand::FreezeToken(token_id, is_unfreezable) => { let res = self - .spend_input_from_account(*nonce, account_op.clone().into()) + .spend_input_from_account(*nonce, account_op.into()) .and_then(|_| { self.tokens_accounting_cache .freeze_token(*token_id, *is_unfreezable) @@ -602,7 +602,7 @@ where } AccountCommand::UnfreezeToken(token_id) => { let res = self - .spend_input_from_account(*nonce, account_op.clone().into()) + .spend_input_from_account(*nonce, account_op.into()) .and_then(|_| { self.tokens_accounting_cache .unfreeze_token(*token_id) @@ -612,7 +612,7 @@ where } AccountCommand::ChangeTokenAuthority(token_id, new_authority) => { let res = self - .spend_input_from_account(*nonce, account_op.clone().into()) + .spend_input_from_account(*nonce, account_op.into()) .and_then(|_| { self.tokens_accounting_cache .change_authority(*token_id, new_authority.clone()) @@ -622,7 +622,7 @@ where } AccountCommand::ChangeTokenMetadataUri(token_id, new_metadata_uri) => { let res = self - .spend_input_from_account(*nonce, account_op.clone().into()) + .spend_input_from_account(*nonce, account_op.into()) .and_then(|_| { self.tokens_accounting_cache .change_metadata_uri(*token_id, new_metadata_uri.clone()) @@ -831,7 +831,7 @@ where | AccountCommand::ChangeTokenMetadataUri(..) => None, AccountCommand::ConcludeOrder(order_id) => { let res = self - .spend_input_from_account(*nonce, account_op.clone().into()) + .spend_input_from_account(*nonce, account_op.into()) .and_then(|_| { self.orders_accounting_cache .conclude_order(*order_id) @@ -841,7 +841,7 @@ where } AccountCommand::FillOrder(order_id, fill, _) => { let res = self - .spend_input_from_account(*nonce, account_op.clone().into()) + .spend_input_from_account(*nonce, account_op.into()) .and_then(|_| { self.orders_accounting_cache .fill_order(*order_id, *fill) diff --git a/common/src/chain/transaction/account_outpoint.rs b/common/src/chain/transaction/account_outpoint.rs index 5428dce51..3b5f8a4eb 100644 --- a/common/src/chain/transaction/account_outpoint.rs +++ b/common/src/chain/transaction/account_outpoint.rs @@ -36,16 +36,16 @@ pub enum AccountType { Order(OrderId), } -impl From for AccountType { - fn from(spending: AccountSpending) -> Self { +impl From<&AccountSpending> for AccountType { + fn from(spending: &AccountSpending) -> Self { match spending { - AccountSpending::DelegationBalance(id, _) => AccountType::Delegation(id), + AccountSpending::DelegationBalance(id, _) => AccountType::Delegation(*id), } } } -impl From for AccountType { - fn from(op: AccountCommand) -> Self { +impl From<&AccountCommand> for AccountType { + fn from(op: &AccountCommand) -> Self { match op { AccountCommand::MintTokens(id, _) | AccountCommand::UnmintTokens(id) @@ -53,9 +53,9 @@ impl From for AccountType { | AccountCommand::FreezeToken(id, _) | AccountCommand::UnfreezeToken(id) | AccountCommand::ChangeTokenAuthority(id, _) - | AccountCommand::ChangeTokenMetadataUri(id, _) => AccountType::Token(id), + | AccountCommand::ChangeTokenMetadataUri(id, _) => AccountType::Token(*id), AccountCommand::ConcludeOrder(id) | AccountCommand::FillOrder(id, _, _) => { - AccountType::Order(id) + AccountType::Order(*id) } } } diff --git a/mempool/src/pool/entry.rs b/mempool/src/pool/entry.rs index bfbfbdae3..2ae5913f5 100644 --- a/mempool/src/pool/entry.rs +++ b/mempool/src/pool/entry.rs @@ -60,7 +60,7 @@ impl TxDependency { fn from_account(acct: &AccountSpending, nonce: AccountNonce) -> Self { match acct { AccountSpending::DelegationBalance(_, _) => { - Self::DelegationAccount(TxAccountDependency::new(acct.clone().into(), nonce)) + Self::DelegationAccount(TxAccountDependency::new(acct.into(), nonce)) } } } @@ -74,10 +74,10 @@ impl TxDependency { | AccountCommand::UnfreezeToken(_) | AccountCommand::ChangeTokenMetadataUri(_, _) | AccountCommand::ChangeTokenAuthority(_, _) => { - Self::TokenSupplyAccount(TxAccountDependency::new(acct.clone().into(), nonce)) + Self::TokenSupplyAccount(TxAccountDependency::new(acct.into(), nonce)) } AccountCommand::ConcludeOrder(_) | AccountCommand::FillOrder(_, _, _) => { - Self::OrderAccount(TxAccountDependency::new(acct.clone().into(), nonce)) + Self::OrderAccount(TxAccountDependency::new(acct.into(), nonce)) } } } From 7281875114a8e0ae04aece6651cb06f2d9f0eff1 Mon Sep 17 00:00:00 2001 From: Heorhii Azarov Date: Fri, 27 Dec 2024 15:20:16 +0200 Subject: [PATCH 3/9] Check nonces while updating conflicting txs --- wallet/src/account/mod.rs | 10 +- wallet/src/account/output_cache/mod.rs | 120 +++++--- wallet/src/wallet/mod.rs | 14 +- wallet/src/wallet/tests.rs | 361 ++++++++++++++++++++++++- wallet/types/src/wallet_tx.rs | 10 - 5 files changed, 465 insertions(+), 50 deletions(-) diff --git a/wallet/src/account/mod.rs b/wallet/src/account/mod.rs index 7c3db32db..17002663b 100644 --- a/wallet/src/account/mod.rs +++ b/wallet/src/account/mod.rs @@ -33,7 +33,6 @@ use common::size_estimation::{ use common::Uint256; use crypto::key::hdkd::child_number::ChildNumber; use mempool::FeeRate; -use output_cache::OrderData; use serialization::hex_encoded::HexEncoded; use utils::ensure; pub use utxo_selector::UtxoSelectorError; @@ -85,7 +84,8 @@ use wallet_types::{ }; pub use self::output_cache::{ - DelegationData, FungibleTokenInfo, PoolData, TxInfo, UnconfirmedTokenInfo, UtxoWithTxOutput, + DelegationData, FungibleTokenInfo, OrderData, PoolData, TxInfo, UnconfirmedTokenInfo, + UtxoWithTxOutput, }; use self::output_cache::{OutputCache, TokenIssuanceData}; use self::transaction_list::{get_transaction_list, TransactionList}; @@ -900,6 +900,12 @@ impl Account { .ok_or(WalletError::UnknownTokenId(*token_id)) } + pub fn get_orders(&self) -> impl Iterator { + self.output_cache + .orders() + .filter(|(_, data)| self.is_destination_mine(&data.conclude_key)) + } + pub fn find_order(&self, order_id: &OrderId) -> WalletResult<&OrderData> { self.output_cache .order_data(order_id) diff --git a/wallet/src/account/output_cache/mod.rs b/wallet/src/account/output_cache/mod.rs index 3c943dc31..efb45b089 100644 --- a/wallet/src/account/output_cache/mod.rs +++ b/wallet/src/account/output_cache/mod.rs @@ -30,8 +30,8 @@ use common::{ RPCFungibleTokenInfo, RPCIsTokenFrozen, RPCTokenTotalSupply, TokenId, TokenIssuance, TokenTotalSupply, }, - AccountCommand, AccountNonce, AccountSpending, DelegationId, Destination, GenBlock, - OrderId, OutPointSourceId, PoolId, Transaction, TxInput, TxOutput, UtxoOutPoint, + AccountCommand, AccountNonce, AccountSpending, AccountType, DelegationId, Destination, + GenBlock, OrderId, OutPointSourceId, PoolId, Transaction, TxInput, TxOutput, UtxoOutPoint, }, primitives::{id::WithId, per_thousand::PerThousand, Amount, BlockHeight, Id, Idable}, }; @@ -621,6 +621,10 @@ impl OutputCache { self.token_issuance.get(token_id) } + pub fn orders(&self) -> impl Iterator { + self.orders.iter() + } + pub fn order_data(&self, order_id: &OrderId) -> Option<&OrderData> { self.orders.get(order_id) } @@ -688,38 +692,67 @@ impl OutputCache { // Collect all conflicting txs let mut conflicting_txs = vec![]; - let frozen_token_id = confirmed_tx.inputs().iter().find_map(|inp| match inp { - TxInput::Utxo(_) | TxInput::Account(_) => None, - TxInput::AccountCommand(_, cmd) => match cmd { - AccountCommand::MintTokens(_, _) - | AccountCommand::UnmintTokens(_) - | AccountCommand::LockTokenSupply(_) - | AccountCommand::ChangeTokenMetadataUri(_, _) - | AccountCommand::ChangeTokenAuthority(_, _) - | AccountCommand::UnfreezeToken(_) - | AccountCommand::ConcludeOrder(_) - | AccountCommand::FillOrder(_, _, _) => None, - AccountCommand::FreezeToken(frozen_token_id, _) => Some(frozen_token_id), - }, - }); + let mut frozen_token_id: Option = None; + let mut confirmed_account_nonce: Option<(AccountType, AccountNonce)> = None; - if let Some(frozen_token_id) = frozen_token_id { + for input in confirmed_tx.inputs() { + match input { + TxInput::Utxo(_) => { + //TODO: check conflicting utxo spends + } + TxInput::Account(outpoint) => { + confirmed_account_nonce = Some((outpoint.account().into(), outpoint.nonce())); + } + TxInput::AccountCommand(nonce, cmd) => { + confirmed_account_nonce = Some((cmd.into(), *nonce)); + match cmd { + AccountCommand::MintTokens(_, _) + | AccountCommand::UnmintTokens(_) + | AccountCommand::LockTokenSupply(_) + | AccountCommand::ChangeTokenMetadataUri(_, _) + | AccountCommand::ChangeTokenAuthority(_, _) + | AccountCommand::UnfreezeToken(_) + | AccountCommand::ConcludeOrder(_) + | AccountCommand::FillOrder(_, _, _) => { /* do nothing */ } + | AccountCommand::FreezeToken(token_id, _) => { + frozen_token_id = Some(*token_id); + } + } + } + } + } + + if frozen_token_id.is_some() | confirmed_account_nonce.is_some() { for unconfirmed in self.unconfirmed_descendants.keys() { - let unconfirmed_tx = self.txs.get(unconfirmed).expect("must be present"); - if self.uses_token(unconfirmed_tx, frozen_token_id) { - let unconfirmed_tx = self.txs.get_mut(unconfirmed).expect("must be present"); - match unconfirmed_tx { - WalletTx::Tx(ref mut tx) => { + if let Some(frozen_token_id) = frozen_token_id { + let unconfirmed_tx = self.txs.get(unconfirmed).expect("must be present"); + if self.uses_token(unconfirmed_tx, &frozen_token_id) { + let unconfirmed_tx = + self.txs.get_mut(unconfirmed).expect("must be present"); + if let WalletTx::Tx(ref mut tx) = unconfirmed_tx { conflicting_txs.push(tx.get_transaction().get_id()); } - WalletTx::Block(_) => {} - }; + } + } + + if let Some((confirmed_account, confirmed_account_nonce)) = confirmed_account_nonce + { + let unconfirmed_tx = self.txs.get(unconfirmed).expect("must be present"); + if uses_conflicting_nonce( + unconfirmed_tx, + confirmed_account, + confirmed_account_nonce, + ) { + let unconfirmed_tx = + self.txs.get_mut(unconfirmed).expect("must be present"); + if let WalletTx::Tx(ref mut tx) = unconfirmed_tx { + conflicting_txs.push(tx.get_transaction().get_id()); + } + } } } } - // FIXME: check utxos and account nonces here - // Remove all descendants of conflicting txs let mut conflicting_txs_with_descendants = vec![]; for conflicting_tx in conflicting_txs { @@ -784,7 +817,10 @@ impl OutputCache { } pub fn add_tx(&mut self, tx_id: OutPointSourceId, tx: WalletTx) -> WalletResult<()> { - let already_present = self.txs.get(&tx_id).is_some_and(|tx| !tx.state().is_abandoned()); + let already_present = self.txs.get(&tx_id).is_some_and(|tx| match tx.state() { + TxState::Abandoned | TxState::Conflicted(_) => false, + TxState::Confirmed(_, _, _) | TxState::InMempool(_) | TxState::Inactive(_) => true, + }); let is_unconfirmed = match tx.state() { TxState::Inactive(_) | TxState::InMempool(_) @@ -1385,10 +1421,9 @@ impl OutputCache { Entry::Occupied(mut entry) => match entry.get_mut() { WalletTx::Block(_) => Err(WalletError::CannotFindTransactionWithId(tx_id)), WalletTx::Tx(tx) => { - ensure!( - new_state != *tx.state(), - WalletError::CannotAbandonTransaction(*tx.state()) - ); + if new_state == *tx.state() { + continue; + } match tx.state() { TxState::Inactive(_) | TxState::Conflicted(_) | TxState::Abandoned => { @@ -1448,9 +1483,9 @@ impl OutputCache { } Ok(()) } - TxState::Confirmed(..) | TxState::InMempool(..) => { - Err(WalletError::CannotAbandonTransaction(*tx.state())) - } + TxState::Confirmed(..) | TxState::InMempool(..) => Err( + WalletError::CannotChangeTransactionState(*tx.state(), new_state), + ), } } }, @@ -1712,3 +1747,20 @@ fn apply_total_supply_mutations_from_tx( Ok(total_supply) } + +fn uses_conflicting_nonce( + unconfirmed_tx: &WalletTx, + confirmed_account_type: AccountType, + confirmed_nonce: AccountNonce, +) -> bool { + unconfirmed_tx.inputs().iter().all(|inp| match inp { + TxInput::Utxo(_) => true, + TxInput::AccountCommand(nonce, cmd) => { + confirmed_account_type == cmd.into() && *nonce <= confirmed_nonce + } + TxInput::Account(outpoint) => { + confirmed_account_type == outpoint.account().into() + && outpoint.nonce() <= confirmed_nonce + } + }) +} diff --git a/wallet/src/wallet/mod.rs b/wallet/src/wallet/mod.rs index 11755f16b..7fe0b912c 100644 --- a/wallet/src/wallet/mod.rs +++ b/wallet/src/wallet/mod.rs @@ -20,7 +20,7 @@ use std::sync::Arc; use crate::account::transaction_list::TransactionList; use crate::account::{CoinSelectionAlgo, TxInfo}; use crate::account::{ - CurrentFeeRate, DelegationData, PoolData, TransactionToSign, UnconfirmedTokenInfo, + CurrentFeeRate, DelegationData, OrderData, PoolData, TransactionToSign, UnconfirmedTokenInfo, UtxoSelectorError, }; use crate::key_chain::{ @@ -175,8 +175,8 @@ pub enum WalletError { NotUtxoInput, #[error("Coin selection error: {0}")] CoinSelectionError(#[from] UtxoSelectorError), - #[error("Cannot abandon a transaction in {0} state")] - CannotAbandonTransaction(TxState), + #[error("Cannot change a transaction's state from {0} to {1}")] + CannotChangeTransactionState(TxState, TxState), #[error("Transaction with Id {0} not found")] CannotFindTransactionWithId(Id), #[error("Address error: {0}")] @@ -1885,6 +1885,14 @@ impl Wallet { }) } + pub fn get_orders( + &self, + account_index: U31, + ) -> WalletResult> { + let orders = self.get_account(account_index)?.get_orders(); + Ok(orders) + } + pub fn create_order_tx( &mut self, account_index: U31, diff --git a/wallet/src/wallet/tests.rs b/wallet/src/wallet/tests.rs index 3aaebc9d8..2790d0ca6 100644 --- a/wallet/src/wallet/tests.rs +++ b/wallet/src/wallet/tests.rs @@ -5924,4 +5924,363 @@ fn create_order_fill_partially_conclude(#[case] seed: Seed) { ); } } -// create order, fill partially, conclude + +#[rstest] +#[trace] +#[case(Seed::from_entropy())] +fn conflicting_delegation_account_nonce(#[case] seed: Seed) { + let mut rng = make_seedable_rng(seed); + let chain_config = Arc::new(create_unit_test_config()); + + let mut wallet = create_wallet(chain_config.clone()); + + let coin_balance = get_coin_balance(&wallet); + assert_eq!(coin_balance, Amount::ZERO); + + // Generate a new block which sends reward to the wallet + let delegation_amount = Amount::from_atoms(rng.gen_range(2..100)); + let block1_amount = (chain_config.min_stake_pool_pledge() + delegation_amount).unwrap(); + let _ = create_block(&chain_config, &mut wallet, vec![], block1_amount, 0); + + let pool_ids = wallet.get_pool_ids(DEFAULT_ACCOUNT_INDEX, WalletPoolsFilter::All).unwrap(); + assert!(pool_ids.is_empty()); + + let coin_balance = get_coin_balance(&wallet); + assert_eq!(coin_balance, block1_amount); + + let pool_amount = chain_config.min_stake_pool_pledge(); + + let stake_pool_transaction = wallet + .create_stake_pool_tx( + DEFAULT_ACCOUNT_INDEX, + FeeRate::from_amount_per_kb(Amount::ZERO), + FeeRate::from_amount_per_kb(Amount::ZERO), + StakePoolDataArguments { + amount: pool_amount, + margin_ratio_per_thousand: PerThousand::new_from_rng(&mut rng), + cost_per_block: Amount::ZERO, + decommission_key: Destination::AnyoneCanSpend, + }, + ) + .unwrap(); + + let (address, _) = create_block( + &chain_config, + &mut wallet, + vec![stake_pool_transaction], + Amount::ZERO, + 1, + ); + + let coin_balance = get_coin_balance(&wallet); + assert_eq!(coin_balance, (block1_amount - pool_amount).unwrap(),); + + let pool_ids = wallet.get_pool_ids(DEFAULT_ACCOUNT_INDEX, WalletPoolsFilter::All).unwrap(); + assert_eq!(pool_ids.len(), 1); + + let pool_id = pool_ids.first().unwrap().0; + let (delegation_id, delegation_tx) = wallet + .create_delegation( + DEFAULT_ACCOUNT_INDEX, + vec![make_create_delegation_output(address.clone(), pool_id)], + FeeRate::from_amount_per_kb(Amount::ZERO), + FeeRate::from_amount_per_kb(Amount::ZERO), + ) + .unwrap(); + + let _ = create_block( + &chain_config, + &mut wallet, + vec![delegation_tx], + Amount::ZERO, + 2, + ); + + let mut delegations = wallet.get_delegations(DEFAULT_ACCOUNT_INDEX).unwrap().collect_vec(); + assert_eq!(delegations.len(), 1); + let (deleg_id, deleg_data) = delegations.pop().unwrap(); + assert_eq!(*deleg_id, delegation_id); + assert!(deleg_data.not_staked_yet); + assert_eq!(deleg_data.last_nonce, None); + assert_eq!(deleg_data.pool_id, pool_id); + assert_eq!(&deleg_data.destination, address.as_object()); + + let delegation_stake_tx = wallet + .create_transaction_to_addresses( + DEFAULT_ACCOUNT_INDEX, + [TxOutput::DelegateStaking(delegation_amount, delegation_id)], + SelectedInputs::Utxos(vec![]), + BTreeMap::new(), + FeeRate::from_amount_per_kb(Amount::ZERO), + FeeRate::from_amount_per_kb(Amount::ZERO), + ) + .unwrap(); + + let _ = create_block( + &chain_config, + &mut wallet, + vec![delegation_stake_tx], + Amount::ZERO, + 3, + ); + + let spend_from_delegation_tx_1 = wallet + .create_transaction_to_addresses_from_delegation( + DEFAULT_ACCOUNT_INDEX, + address.clone(), + Amount::from_atoms(1), + delegation_id, + Amount::from_atoms(2), + FeeRate::from_amount_per_kb(Amount::ZERO), + ) + .unwrap(); + + wallet + .add_account_unconfirmed_tx( + DEFAULT_ACCOUNT_INDEX, + spend_from_delegation_tx_1.clone(), + &WalletEventsNoOp, + ) + .unwrap(); + + let spend_from_delegation_tx_2 = wallet + .create_transaction_to_addresses_from_delegation( + DEFAULT_ACCOUNT_INDEX, + address.clone(), + Amount::from_atoms(1), + delegation_id, + Amount::from_atoms(2), + FeeRate::from_amount_per_kb(Amount::ZERO), + ) + .unwrap(); + + wallet + .add_account_unconfirmed_tx( + DEFAULT_ACCOUNT_INDEX, + spend_from_delegation_tx_2.clone(), + &WalletEventsNoOp, + ) + .unwrap(); + + // Check delegation after unconfirmed tx status + let mut delegations = wallet.get_delegations(DEFAULT_ACCOUNT_INDEX).unwrap().collect_vec(); + assert_eq!(delegations.len(), 1); + let (deleg_id, deleg_data) = delegations.pop().unwrap(); + assert_eq!(*deleg_id, delegation_id); + assert_eq!(deleg_data.last_nonce, Some(AccountNonce::new(1))); + + let _ = create_block( + &chain_config, + &mut wallet, + vec![spend_from_delegation_tx_1], + Amount::ZERO, + 4, + ); + + // if confirmed tx is added conflicting txs must be removed from the output cache + let mut delegations = wallet.get_delegations(DEFAULT_ACCOUNT_INDEX).unwrap().collect_vec(); + assert_eq!(delegations.len(), 1); + let (deleg_id, deleg_data) = delegations.pop().unwrap(); + assert_eq!(*deleg_id, delegation_id); + assert_eq!(deleg_data.last_nonce, Some(AccountNonce::new(0))); +} + +#[rstest] +#[trace] +#[case(Seed::from_entropy())] +fn conflicting_order_account_nonce(#[case] seed: Seed) { + let mut rng = make_seedable_rng(seed); + let chain_config = Arc::new(create_unit_test_config()); + + let mut wallet = create_wallet(chain_config.clone()); + + let coin_balance = get_coin_balance(&wallet); + assert_eq!(coin_balance, Amount::ZERO); + + // Generate a new block which sends reward to the wallet + let delegation_amount = Amount::from_atoms(rng.gen_range(2..100)); + let block1_amount = (chain_config.min_stake_pool_pledge() + delegation_amount).unwrap(); + let _ = create_block(&chain_config, &mut wallet, vec![], block1_amount, 0); + + // Issue a token + let address2 = wallet.get_new_address(DEFAULT_ACCOUNT_INDEX).unwrap().1; + + let token_issuance = + random_token_issuance_v1(&chain_config, address2.as_object().clone(), &mut rng); + let (issued_token_id, token_issuance_transaction) = wallet + .issue_new_token( + DEFAULT_ACCOUNT_INDEX, + TokenIssuance::V1(token_issuance.clone()), + FeeRate::from_amount_per_kb(Amount::ZERO), + FeeRate::from_amount_per_kb(Amount::ZERO), + ) + .unwrap(); + + let block2_amount = chain_config.token_supply_change_fee(BlockHeight::zero()); + let _ = create_block( + &chain_config, + &mut wallet, + vec![token_issuance_transaction], + block2_amount, + 1, + ); + + // Mint some tokens + let freezable = token_issuance.is_freezable.as_bool(); + let token_info = RPCFungibleTokenInfo::new( + issued_token_id, + token_issuance.token_ticker, + token_issuance.number_of_decimals, + token_issuance.metadata_uri, + Amount::ZERO, + token_issuance.total_supply.into(), + false, + RPCIsTokenFrozen::NotFrozen { freezable }, + token_issuance.authority, + ); + + let unconfirmed_token_info = + wallet.get_token_unconfirmed_info(DEFAULT_ACCOUNT_INDEX, &token_info).unwrap(); + + let token_amount_to_mint = Amount::from_atoms(rng.gen_range(2..100)); + let mint_transaction = wallet + .mint_tokens( + DEFAULT_ACCOUNT_INDEX, + &unconfirmed_token_info, + token_amount_to_mint, + address2.clone(), + FeeRate::from_amount_per_kb(Amount::ZERO), + FeeRate::from_amount_per_kb(Amount::ZERO), + ) + .unwrap(); + + let _ = create_block( + &chain_config, + &mut wallet, + vec![mint_transaction], + Amount::ZERO, + 2, + ); + + // Create an order selling tokens for coins + let buy_amount = Amount::from_atoms(111); + let sell_amount = token_amount_to_mint; + let ask_value = OutputValue::Coin(buy_amount); + let give_value = OutputValue::TokenV1(issued_token_id, sell_amount); + let (order_id, create_order_tx) = wallet + .create_order_tx( + DEFAULT_ACCOUNT_INDEX, + ask_value.clone(), + give_value.clone(), + address2.clone(), + FeeRate::from_amount_per_kb(Amount::ZERO), + FeeRate::from_amount_per_kb(Amount::ZERO), + ) + .unwrap(); + + let _ = create_block( + &chain_config, + &mut wallet, + vec![create_order_tx], + Amount::ZERO, + 3, + ); + + let mut orders = wallet.get_orders(DEFAULT_ACCOUNT_INDEX).unwrap().collect_vec(); + assert_eq!(orders.len(), 1); + let (actual_order_id, actual_order_data) = orders.pop().unwrap(); + assert_eq!(order_id, *actual_order_id); + assert_eq!(actual_order_data.last_nonce, None); + assert_eq!(&actual_order_data.conclude_key, address2.as_object()); + + // Create 2 fill orders txs and put them in unconfirmed + let order_info = RpcOrderInfo { + conclude_key: address2.clone().into_object(), + initially_given: RpcOutputValue::Token { + id: issued_token_id, + amount: buy_amount, + }, + initially_asked: RpcOutputValue::Coin { + amount: sell_amount, + }, + give_balance: sell_amount, + ask_balance: buy_amount, + nonce: None, + }; + + let fill_order_tx_1 = wallet + .create_fill_order_tx( + DEFAULT_ACCOUNT_INDEX, + order_id, + order_info.clone(), + Amount::from_atoms(10), + None, + FeeRate::from_amount_per_kb(Amount::ZERO), + FeeRate::from_amount_per_kb(Amount::ZERO), + ) + .unwrap(); + + wallet + .add_account_unconfirmed_tx( + DEFAULT_ACCOUNT_INDEX, + fill_order_tx_1.clone(), + &WalletEventsNoOp, + ) + .unwrap(); + + let order_info = RpcOrderInfo { + conclude_key: address2.clone().into_object(), + initially_given: RpcOutputValue::Token { + id: issued_token_id, + amount: buy_amount, + }, + initially_asked: RpcOutputValue::Coin { + amount: sell_amount, + }, + give_balance: sell_amount, + ask_balance: buy_amount, + nonce: Some(AccountNonce::new(0)), + }; + + let fill_order_tx_2 = wallet + .create_fill_order_tx( + DEFAULT_ACCOUNT_INDEX, + order_id, + order_info.clone(), + Amount::from_atoms(3), + None, + FeeRate::from_amount_per_kb(Amount::ZERO), + FeeRate::from_amount_per_kb(Amount::ZERO), + ) + .unwrap(); + + wallet + .add_account_unconfirmed_tx( + DEFAULT_ACCOUNT_INDEX, + fill_order_tx_2.clone(), + &WalletEventsNoOp, + ) + .unwrap(); + + // Check order data after unconfirmed tx status + let mut orders = wallet.get_orders(DEFAULT_ACCOUNT_INDEX).unwrap().collect_vec(); + assert_eq!(orders.len(), 1); + let (actual_order_id, order_data) = orders.pop().unwrap(); + assert_eq!(*actual_order_id, order_id); + assert_eq!(order_data.last_nonce, Some(AccountNonce::new(1))); + + let _ = create_block( + &chain_config, + &mut wallet, + vec![fill_order_tx_1], + Amount::ZERO, + 4, + ); + + // if confirmed tx is added conflicting txs must be removed from the output cache + let mut orders = wallet.get_orders(DEFAULT_ACCOUNT_INDEX).unwrap().collect_vec(); + assert_eq!(orders.len(), 1); + let (actual_order_id, order_data) = orders.pop().unwrap(); + assert_eq!(*actual_order_id, order_id); + assert_eq!(order_data.last_nonce, Some(AccountNonce::new(0))); +} diff --git a/wallet/types/src/wallet_tx.rs b/wallet/types/src/wallet_tx.rs index 8979e77a4..b8c15f987 100644 --- a/wallet/types/src/wallet_tx.rs +++ b/wallet/types/src/wallet_tx.rs @@ -84,16 +84,6 @@ impl TxState { TxState::Abandoned => "Abandoned", } } - - pub fn is_abandoned(&self) -> bool { - match self { - TxState::Abandoned => true, - TxState::Confirmed(_, _, _) - | TxState::Conflicted(_) - | TxState::InMempool(_) - | TxState::Inactive(_) => false, - } - } } impl Display for TxState { From 6c83ac17d26a874a6765c7844eb41ac0aae160ce Mon Sep 17 00:00:00 2001 From: Heorhii Azarov Date: Tue, 21 Jan 2025 10:40:52 +0200 Subject: [PATCH 4/9] Confirmed tx should not be marked as conflicted --- test/functional/wallet_conflict.py | 2 +- wallet/src/account/output_cache/mod.rs | 164 ++++++----- wallet/src/wallet/tests.rs | 380 ++++++++++++++++++++++++- 3 files changed, 446 insertions(+), 100 deletions(-) diff --git a/test/functional/wallet_conflict.py b/test/functional/wallet_conflict.py index 33f066972..c91126e1d 100644 --- a/test/functional/wallet_conflict.py +++ b/test/functional/wallet_conflict.py @@ -198,7 +198,7 @@ async def async_test(self): # check we cannot abandon an already confirmed transaction assert_in("Success", await wallet.select_account(1)) - assert_in("Cannot abandon a transaction in Confirmed at height 6", await wallet.abandon_transaction(new_transfer_tx_id)) + assert_in("Cannot change a transaction's state from Confirmed", await wallet.abandon_transaction(new_transfer_tx_id)) diff --git a/wallet/src/account/output_cache/mod.rs b/wallet/src/account/output_cache/mod.rs index efb45b089..133680824 100644 --- a/wallet/src/account/output_cache/mod.rs +++ b/wallet/src/account/output_cache/mod.rs @@ -689,9 +689,6 @@ impl OutputCache { confirmed_tx: &Transaction, block_id: Id, ) -> WalletResult>> { - // Collect all conflicting txs - let mut conflicting_txs = vec![]; - let mut frozen_token_id: Option = None; let mut confirmed_account_nonce: Option<(AccountType, AccountNonce)> = None; @@ -722,14 +719,15 @@ impl OutputCache { } } - if frozen_token_id.is_some() | confirmed_account_nonce.is_some() { + // Collect all conflicting txs + let mut conflicting_txs = vec![]; + + if frozen_token_id.is_some() || confirmed_account_nonce.is_some() { for unconfirmed in self.unconfirmed_descendants.keys() { if let Some(frozen_token_id) = frozen_token_id { let unconfirmed_tx = self.txs.get(unconfirmed).expect("must be present"); if self.uses_token(unconfirmed_tx, &frozen_token_id) { - let unconfirmed_tx = - self.txs.get_mut(unconfirmed).expect("must be present"); - if let WalletTx::Tx(ref mut tx) = unconfirmed_tx { + if let WalletTx::Tx(tx) = unconfirmed_tx { conflicting_txs.push(tx.get_transaction().get_id()); } } @@ -743,9 +741,7 @@ impl OutputCache { confirmed_account, confirmed_account_nonce, ) { - let unconfirmed_tx = - self.txs.get_mut(unconfirmed).expect("must be present"); - if let WalletTx::Tx(ref mut tx) = unconfirmed_tx { + if let WalletTx::Tx(tx) = unconfirmed_tx { conflicting_txs.push(tx.get_transaction().get_id()); } } @@ -755,11 +751,16 @@ impl OutputCache { // Remove all descendants of conflicting txs let mut conflicting_txs_with_descendants = vec![]; + for conflicting_tx in conflicting_txs { - let descendants = - self.remove_descendants_and_mark_as(conflicting_tx, TxState::Conflicted(block_id))?; + if conflicting_tx != confirmed_tx.get_id() { + let descendants = self.remove_descendants_and_mark_as( + conflicting_tx, + TxState::Conflicted(block_id), + )?; - conflicting_txs_with_descendants.extend(descendants.into_iter()); + conflicting_txs_with_descendants.extend(descendants.into_iter()); + } } Ok(conflicting_txs_with_descendants) @@ -828,8 +829,11 @@ impl OutputCache { | TxState::Abandoned => true, TxState::Confirmed(_, _, _) => false, }; + if is_unconfirmed && !already_present { self.unconfirmed_descendants.insert(tx_id.clone(), BTreeSet::new()); + } else { + self.unconfirmed_descendants.remove(&tx_id); } self.update_inputs(&tx, is_unconfirmed, &tx_id, already_present)?; @@ -944,13 +948,14 @@ impl OutputCache { match input { TxInput::Utxo(outpoint) => { self.consumed.insert(outpoint.clone(), tx.state()); - if is_unconfirmed { - self.unconfirmed_descendants - .get_mut(&outpoint.source_id()) - .as_mut() - .map(|descendants| descendants.insert(tx_id.clone())); - } else { - self.unconfirmed_descendants.remove(tx_id); + if let Some(descendants) = + self.unconfirmed_descendants.get_mut(&outpoint.source_id()) + { + if is_unconfirmed { + descendants.insert(tx_id.clone()); + } else { + descendants.remove(tx_id); + } } } TxInput::Account(outpoint) => match outpoint.account() { @@ -1420,74 +1425,65 @@ impl OutputCache { 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) => { - if new_state == *tx.state() { - continue; - } - - match tx.state() { - TxState::Inactive(_) | TxState::Conflicted(_) | TxState::Abandoned => { - tx.set_state(new_state); - for input in tx.get_transaction().inputs() { - match input { - TxInput::Utxo(outpoint) => { - self.consumed.insert(outpoint.clone(), *tx.state()); - } - TxInput::Account(outpoint) => match outpoint.account() { - AccountSpending::DelegationBalance( - delegation_id, - _, - ) => { - if let Some(data) = - self.delegations.get_mut(delegation_id) - { - data.last_nonce = outpoint.nonce().decrement(); - data.last_parent = find_parent( - &self.unconfirmed_descendants, - tx_id.into(), - ); - } + WalletTx::Tx(tx) => match tx.state() { + TxState::Inactive(_) | TxState::Conflicted(_) | TxState::Abandoned => { + tx.set_state(new_state); + for input in tx.get_transaction().inputs() { + match input { + TxInput::Utxo(outpoint) => { + self.consumed.insert(outpoint.clone(), *tx.state()); + } + TxInput::Account(outpoint) => match outpoint.account() { + AccountSpending::DelegationBalance(delegation_id, _) => { + if let Some(data) = + self.delegations.get_mut(delegation_id) + { + data.last_nonce = outpoint.nonce().decrement(); + data.last_parent = find_parent( + &self.unconfirmed_descendants, + tx_id.into(), + ); } - }, - TxInput::AccountCommand(nonce, op) => match op { - AccountCommand::MintTokens(token_id, _) - | AccountCommand::UnmintTokens(token_id) - | AccountCommand::LockTokenSupply(token_id) - | AccountCommand::FreezeToken(token_id, _) - | AccountCommand::UnfreezeToken(token_id) - | AccountCommand::ChangeTokenMetadataUri(token_id, _) - | AccountCommand::ChangeTokenAuthority(token_id, _) => { - if let Some(data) = - self.token_issuance.get_mut(token_id) - { - data.last_nonce = nonce.decrement(); - data.last_parent = find_parent( - &self.unconfirmed_descendants, - tx_id.into(), - ); - data.unconfirmed_txs.remove(&tx_id.into()); - } + } + }, + TxInput::AccountCommand(nonce, op) => match op { + AccountCommand::MintTokens(token_id, _) + | AccountCommand::UnmintTokens(token_id) + | AccountCommand::LockTokenSupply(token_id) + | AccountCommand::FreezeToken(token_id, _) + | AccountCommand::UnfreezeToken(token_id) + | AccountCommand::ChangeTokenMetadataUri(token_id, _) + | AccountCommand::ChangeTokenAuthority(token_id, _) => { + if let Some(data) = + self.token_issuance.get_mut(token_id) + { + data.last_nonce = nonce.decrement(); + data.last_parent = find_parent( + &self.unconfirmed_descendants, + tx_id.into(), + ); + data.unconfirmed_txs.remove(&tx_id.into()); } - AccountCommand::ConcludeOrder(order_id) - | AccountCommand::FillOrder(order_id, _, _) => { - if let Some(data) = self.orders.get_mut(order_id) { - data.last_nonce = nonce.decrement(); - data.last_parent = find_parent( - &self.unconfirmed_descendants, - tx_id.into(), - ); - } + } + AccountCommand::ConcludeOrder(order_id) + | AccountCommand::FillOrder(order_id, _, _) => { + if let Some(data) = self.orders.get_mut(order_id) { + data.last_nonce = nonce.decrement(); + data.last_parent = find_parent( + &self.unconfirmed_descendants, + tx_id.into(), + ); } - }, - } + } + }, } - Ok(()) } - TxState::Confirmed(..) | TxState::InMempool(..) => Err( - WalletError::CannotChangeTransactionState(*tx.state(), new_state), - ), + Ok(()) } - } + TxState::Confirmed(..) | TxState::InMempool(..) => Err( + WalletError::CannotChangeTransactionState(*tx.state(), new_state), + ), + }, }, Entry::Vacant(_) => Err(WalletError::CannotFindTransactionWithId(tx_id)), }?; @@ -1753,8 +1749,8 @@ fn uses_conflicting_nonce( confirmed_account_type: AccountType, confirmed_nonce: AccountNonce, ) -> bool { - unconfirmed_tx.inputs().iter().all(|inp| match inp { - TxInput::Utxo(_) => true, + unconfirmed_tx.inputs().iter().any(|inp| match inp { + TxInput::Utxo(_) => false, TxInput::AccountCommand(nonce, cmd) => { confirmed_account_type == cmd.into() && *nonce <= confirmed_nonce } diff --git a/wallet/src/wallet/tests.rs b/wallet/src/wallet/tests.rs index 2790d0ca6..040ccbb54 100644 --- a/wallet/src/wallet/tests.rs +++ b/wallet/src/wallet/tests.rs @@ -197,6 +197,21 @@ fn get_coin_balance_with_inactive(wallet: &DefaultWallet) -> Amount { coin_balance } +fn get_balance_with( + wallet: &DefaultWallet, + currency: Currency, + utxo_states: UtxoStates, + with_locked: WithLocked, +) -> Amount { + let coin_balance = wallet + .get_balance(DEFAULT_ACCOUNT_INDEX, utxo_states, with_locked) + .unwrap() + .get(¤cy) + .copied() + .unwrap_or(Amount::ZERO); + coin_balance +} + fn get_coin_balance(wallet: &DefaultWallet) -> Amount { get_coin_balance_for_acc(wallet, DEFAULT_ACCOUNT_INDEX) } @@ -5925,6 +5940,12 @@ fn create_order_fill_partially_conclude(#[case] seed: Seed) { } } +// Create 2 wallet from the same mnemonic. +// Create a pool and a delegation with some stake for both wallets. +// Add 2 consequtive txs that spend share from delelgation to the first wallet as unconfirmed. +// Add 1 txs that spends share from delegation with different amount (so that tx id is different) +// via the second wallet and submit it to the block. +// Check that 2 unconfirmed txs from the first wallet conflicts with confirmed tx and got removed. #[rstest] #[trace] #[case(Seed::from_entropy())] @@ -5932,6 +5953,224 @@ fn conflicting_delegation_account_nonce(#[case] seed: Seed) { let mut rng = make_seedable_rng(seed); let chain_config = Arc::new(create_unit_test_config()); + let mut wallet = create_wallet(chain_config.clone()); + let mut wallet2 = create_wallet(chain_config.clone()); + + let coin_balance = get_coin_balance(&wallet); + assert_eq!(coin_balance, Amount::ZERO); + + // Generate a new block which sends reward to the wallet + let delegation_amount = Amount::from_atoms(rng.gen_range(10..100)); + let block1_amount = (chain_config.min_stake_pool_pledge() + delegation_amount).unwrap(); + let (_, block1) = create_block(&chain_config, &mut wallet, vec![], block1_amount, 0); + scan_wallet(&mut wallet2, BlockHeight::new(0), vec![block1]); + + let pool_ids = wallet.get_pool_ids(DEFAULT_ACCOUNT_INDEX, WalletPoolsFilter::All).unwrap(); + assert!(pool_ids.is_empty()); + + let coin_balance = get_coin_balance(&wallet); + assert_eq!(coin_balance, block1_amount); + + // Create a pool + let pool_amount = chain_config.min_stake_pool_pledge(); + + let stake_pool_transaction = wallet + .create_stake_pool_tx( + DEFAULT_ACCOUNT_INDEX, + FeeRate::from_amount_per_kb(Amount::ZERO), + FeeRate::from_amount_per_kb(Amount::ZERO), + StakePoolDataArguments { + amount: pool_amount, + margin_ratio_per_thousand: PerThousand::new_from_rng(&mut rng), + cost_per_block: Amount::ZERO, + decommission_key: Destination::AnyoneCanSpend, + }, + ) + .unwrap(); + + let (address, block2) = create_block( + &chain_config, + &mut wallet, + vec![stake_pool_transaction.clone()], + Amount::ZERO, + 1, + ); + scan_wallet(&mut wallet2, BlockHeight::new(1), vec![block2]); + + let coin_balance = get_coin_balance(&wallet); + assert_eq!(coin_balance, (block1_amount - pool_amount).unwrap(),); + + let pool_ids = wallet.get_pool_ids(DEFAULT_ACCOUNT_INDEX, WalletPoolsFilter::All).unwrap(); + assert_eq!(pool_ids.len(), 1); + + // Create a delegation + let pool_id = pool_ids.first().unwrap().0; + let (delegation_id, delegation_tx) = wallet + .create_delegation( + DEFAULT_ACCOUNT_INDEX, + vec![make_create_delegation_output(address.clone(), pool_id)], + FeeRate::from_amount_per_kb(Amount::ZERO), + FeeRate::from_amount_per_kb(Amount::ZERO), + ) + .unwrap(); + + let (_, block3) = create_block( + &chain_config, + &mut wallet, + vec![delegation_tx], + Amount::ZERO, + 2, + ); + scan_wallet(&mut wallet2, BlockHeight::new(2), vec![block3]); + + let mut delegations = wallet.get_delegations(DEFAULT_ACCOUNT_INDEX).unwrap().collect_vec(); + assert_eq!(delegations.len(), 1); + let (deleg_id, deleg_data) = delegations.pop().unwrap(); + assert_eq!(*deleg_id, delegation_id); + assert!(deleg_data.not_staked_yet); + assert_eq!(deleg_data.last_nonce, None); + assert_eq!(deleg_data.pool_id, pool_id); + assert_eq!(&deleg_data.destination, address.as_object()); + + // Delegate some coins + let delegation_stake_tx = wallet + .create_transaction_to_addresses( + DEFAULT_ACCOUNT_INDEX, + [TxOutput::DelegateStaking(delegation_amount, delegation_id)], + SelectedInputs::Utxos(vec![]), + BTreeMap::new(), + FeeRate::from_amount_per_kb(Amount::ZERO), + FeeRate::from_amount_per_kb(Amount::ZERO), + ) + .unwrap(); + + let (_, block4) = create_block( + &chain_config, + &mut wallet, + vec![delegation_stake_tx], + Amount::ZERO, + 3, + ); + scan_wallet(&mut wallet2, BlockHeight::new(3), vec![block4]); + + let coin_balance_after_delegating = get_coin_balance(&wallet); + assert_eq!( + coin_balance_after_delegating, + (block1_amount - pool_amount).and_then(|v| v - delegation_amount).unwrap(), + ); + + // Add unconfirmed txs + let withdraw_amount_1 = Amount::from_atoms(1); + let spend_from_delegation_tx_1 = wallet + .create_transaction_to_addresses_from_delegation( + DEFAULT_ACCOUNT_INDEX, + address.clone(), + withdraw_amount_1, + delegation_id, + delegation_amount, + FeeRate::from_amount_per_kb(Amount::ZERO), + ) + .unwrap(); + + wallet + .add_account_unconfirmed_tx( + DEFAULT_ACCOUNT_INDEX, + spend_from_delegation_tx_1.clone(), + &WalletEventsNoOp, + ) + .unwrap(); + + let withdraw_amount_2 = withdraw_amount_1; + let spend_from_delegation_tx_2 = wallet + .create_transaction_to_addresses_from_delegation( + DEFAULT_ACCOUNT_INDEX, + address.clone(), + withdraw_amount_2, + delegation_id, + delegation_amount, + FeeRate::from_amount_per_kb(Amount::ZERO), + ) + .unwrap(); + + wallet + .add_account_unconfirmed_tx( + DEFAULT_ACCOUNT_INDEX, + spend_from_delegation_tx_2.clone(), + &WalletEventsNoOp, + ) + .unwrap(); + + // Check delegation after unconfirmed tx status + let mut delegations = wallet.get_delegations(DEFAULT_ACCOUNT_INDEX).unwrap().collect_vec(); + assert_eq!(delegations.len(), 1); + let (deleg_id, deleg_data) = delegations.pop().unwrap(); + assert_eq!(*deleg_id, delegation_id); + assert_eq!(deleg_data.last_nonce, Some(AccountNonce::new(1))); + + // Create and submit tx with different tx id + let withdraw_amount_3 = Amount::from_atoms(5); + let spend_from_delegation_tx_3 = wallet2 + .create_transaction_to_addresses_from_delegation( + DEFAULT_ACCOUNT_INDEX, + address.clone(), + withdraw_amount_3, + delegation_id, + delegation_amount, + FeeRate::from_amount_per_kb(Amount::ZERO), + ) + .unwrap(); + + let (_, block5) = create_block( + &chain_config, + &mut wallet2, + vec![spend_from_delegation_tx_3], + Amount::ZERO, + 4, + ); + scan_wallet(&mut wallet, BlockHeight::new(4), vec![block5]); + + // if confirmed tx is added conflicting txs must be removed from the output cache + let mut delegations = wallet.get_delegations(DEFAULT_ACCOUNT_INDEX).unwrap().collect_vec(); + assert_eq!(delegations.len(), 1); + let (deleg_id, deleg_data) = delegations.pop().unwrap(); + assert_eq!(*deleg_id, delegation_id); + assert_eq!(deleg_data.last_nonce, Some(AccountNonce::new(0))); + + let coin_balance = get_balance_with( + &wallet, + Currency::Coin, + UtxoState::Confirmed.into(), + WithLocked::Any, + ); + assert_eq!( + coin_balance, + (coin_balance_after_delegating + withdraw_amount_3).unwrap() + ); + + let coin_balance = get_balance_with( + &wallet, + Currency::Coin, + UtxoState::Confirmed | UtxoState::Inactive, + WithLocked::Any, + ); + assert_eq!( + coin_balance, + (coin_balance_after_delegating + withdraw_amount_3).unwrap() + ); +} + +// Create a pool and a delegation with some share. +// Create 2 consequtive txs that spend from delegation account and add them as unconfirmed. +// Check confirmed/unconfirmed balance and ensure that account nonce is incremented in OutputCache. +// Submit the first tx in a block. +// Check that Confirmed balance changed but +#[rstest] +#[trace] +#[case(Seed::from_entropy())] +fn conflicting_delegation_account_nonce_same_tx(#[case] seed: Seed) { + let mut rng = make_seedable_rng(seed); + let chain_config = Arc::new(create_unit_test_config()); + let mut wallet = create_wallet(chain_config.clone()); let coin_balance = get_coin_balance(&wallet); @@ -5942,6 +6181,7 @@ fn conflicting_delegation_account_nonce(#[case] seed: Seed) { let block1_amount = (chain_config.min_stake_pool_pledge() + delegation_amount).unwrap(); let _ = create_block(&chain_config, &mut wallet, vec![], block1_amount, 0); + // Create a pool let pool_ids = wallet.get_pool_ids(DEFAULT_ACCOUNT_INDEX, WalletPoolsFilter::All).unwrap(); assert!(pool_ids.is_empty()); @@ -5978,6 +6218,7 @@ fn conflicting_delegation_account_nonce(#[case] seed: Seed) { let pool_ids = wallet.get_pool_ids(DEFAULT_ACCOUNT_INDEX, WalletPoolsFilter::All).unwrap(); assert_eq!(pool_ids.len(), 1); + // Create a delegation let pool_id = pool_ids.first().unwrap().0; let (delegation_id, delegation_tx) = wallet .create_delegation( @@ -6005,6 +6246,7 @@ fn conflicting_delegation_account_nonce(#[case] seed: Seed) { assert_eq!(deleg_data.pool_id, pool_id); assert_eq!(&deleg_data.destination, address.as_object()); + // Delegate some coins let delegation_stake_tx = wallet .create_transaction_to_addresses( DEFAULT_ACCOUNT_INDEX, @@ -6024,13 +6266,21 @@ fn conflicting_delegation_account_nonce(#[case] seed: Seed) { 3, ); + let coin_balance_after_delegating = get_coin_balance(&wallet); + assert_eq!( + coin_balance_after_delegating, + (block1_amount - pool_amount).and_then(|v| v - delegation_amount).unwrap(), + ); + + // Create first tx that spends from delegation + let withdraw_amount_1 = Amount::from_atoms(1); let spend_from_delegation_tx_1 = wallet .create_transaction_to_addresses_from_delegation( DEFAULT_ACCOUNT_INDEX, address.clone(), - Amount::from_atoms(1), + withdraw_amount_1, delegation_id, - Amount::from_atoms(2), + delegation_amount, FeeRate::from_amount_per_kb(Amount::ZERO), ) .unwrap(); @@ -6043,13 +6293,15 @@ fn conflicting_delegation_account_nonce(#[case] seed: Seed) { ) .unwrap(); + // Create second tx that spends from delegation + let withdraw_amount_2 = withdraw_amount_1; let spend_from_delegation_tx_2 = wallet .create_transaction_to_addresses_from_delegation( DEFAULT_ACCOUNT_INDEX, address.clone(), - Amount::from_atoms(1), + withdraw_amount_2, delegation_id, - Amount::from_atoms(2), + delegation_amount, FeeRate::from_amount_per_kb(Amount::ZERO), ) .unwrap(); @@ -6069,6 +6321,28 @@ fn conflicting_delegation_account_nonce(#[case] seed: Seed) { assert_eq!(*deleg_id, delegation_id); assert_eq!(deleg_data.last_nonce, Some(AccountNonce::new(1))); + let coin_balance = get_balance_with( + &wallet, + Currency::Coin, + UtxoState::Confirmed.into(), + WithLocked::Any, + ); + assert_eq!(coin_balance, coin_balance_after_delegating); + + let coin_balance = get_balance_with( + &wallet, + Currency::Coin, + UtxoState::Confirmed | UtxoState::Inactive, + WithLocked::Any, + ); + assert_eq!( + coin_balance, + (coin_balance_after_delegating + withdraw_amount_1) + .and_then(|v| v + withdraw_amount_2) + .unwrap() + ); + + // Submit first tx in a block let _ = create_block( &chain_config, &mut wallet, @@ -6077,12 +6351,36 @@ fn conflicting_delegation_account_nonce(#[case] seed: Seed) { 4, ); - // if confirmed tx is added conflicting txs must be removed from the output cache + // Confirmed tx should replace the first one leaving the second one as descendant let mut delegations = wallet.get_delegations(DEFAULT_ACCOUNT_INDEX).unwrap().collect_vec(); assert_eq!(delegations.len(), 1); let (deleg_id, deleg_data) = delegations.pop().unwrap(); assert_eq!(*deleg_id, delegation_id); - assert_eq!(deleg_data.last_nonce, Some(AccountNonce::new(0))); + assert_eq!(deleg_data.last_nonce, Some(AccountNonce::new(1))); + + let coin_balance = get_balance_with( + &wallet, + Currency::Coin, + UtxoState::Confirmed.into(), + WithLocked::Any, + ); + assert_eq!( + coin_balance, + (coin_balance_after_delegating + withdraw_amount_1).unwrap() + ); + + let coin_balance = get_balance_with( + &wallet, + Currency::Coin, + UtxoState::Confirmed | UtxoState::Inactive, + WithLocked::Any, + ); + assert_eq!( + coin_balance, + (coin_balance_after_delegating + withdraw_amount_1) + .and_then(|v| v + withdraw_amount_2) + .unwrap() + ); } #[rstest] @@ -6098,8 +6396,7 @@ fn conflicting_order_account_nonce(#[case] seed: Seed) { assert_eq!(coin_balance, Amount::ZERO); // Generate a new block which sends reward to the wallet - let delegation_amount = Amount::from_atoms(rng.gen_range(2..100)); - let block1_amount = (chain_config.min_stake_pool_pledge() + delegation_amount).unwrap(); + let block1_amount = chain_config.fungible_token_issuance_fee(); let _ = create_block(&chain_config, &mut wallet, vec![], block1_amount, 0); // Issue a token @@ -6142,7 +6439,8 @@ fn conflicting_order_account_nonce(#[case] seed: Seed) { let unconfirmed_token_info = wallet.get_token_unconfirmed_info(DEFAULT_ACCOUNT_INDEX, &token_info).unwrap(); - let token_amount_to_mint = Amount::from_atoms(rng.gen_range(2..100)); + let token_amount_to_mint = Amount::from_atoms(100); + let reward_to_spend_on_orders = Amount::from_atoms(100); let mint_transaction = wallet .mint_tokens( DEFAULT_ACCOUNT_INDEX, @@ -6158,12 +6456,12 @@ fn conflicting_order_account_nonce(#[case] seed: Seed) { &chain_config, &mut wallet, vec![mint_transaction], - Amount::ZERO, + reward_to_spend_on_orders, 2, ); // Create an order selling tokens for coins - let buy_amount = Amount::from_atoms(111); + let buy_amount = reward_to_spend_on_orders; let sell_amount = token_amount_to_mint; let ask_value = OutputValue::Coin(buy_amount); let give_value = OutputValue::TokenV1(issued_token_id, sell_amount); @@ -6186,6 +6484,11 @@ fn conflicting_order_account_nonce(#[case] seed: Seed) { 3, ); + let (coin_balance_after_create_order, token_balance_after_create_order) = + get_currency_balances(&wallet); + assert_eq!(coin_balance_after_create_order, reward_to_spend_on_orders); + assert!(token_balance_after_create_order.is_empty()); + let mut orders = wallet.get_orders(DEFAULT_ACCOUNT_INDEX).unwrap().collect_vec(); assert_eq!(orders.len(), 1); let (actual_order_id, actual_order_data) = orders.pop().unwrap(); @@ -6208,12 +6511,14 @@ fn conflicting_order_account_nonce(#[case] seed: Seed) { nonce: None, }; + let spend_coins_1 = Amount::from_atoms(10); + let recieved_tokens_1 = spend_coins_1; let fill_order_tx_1 = wallet .create_fill_order_tx( DEFAULT_ACCOUNT_INDEX, order_id, order_info.clone(), - Amount::from_atoms(10), + spend_coins_1, None, FeeRate::from_amount_per_kb(Amount::ZERO), FeeRate::from_amount_per_kb(Amount::ZERO), @@ -6242,12 +6547,14 @@ fn conflicting_order_account_nonce(#[case] seed: Seed) { nonce: Some(AccountNonce::new(0)), }; + let spend_coins_2 = Amount::from_atoms(3); + let recieved_tokens_2 = spend_coins_2; let fill_order_tx_2 = wallet .create_fill_order_tx( DEFAULT_ACCOUNT_INDEX, order_id, order_info.clone(), - Amount::from_atoms(3), + spend_coins_2, None, FeeRate::from_amount_per_kb(Amount::ZERO), FeeRate::from_amount_per_kb(Amount::ZERO), @@ -6277,10 +6584,53 @@ fn conflicting_order_account_nonce(#[case] seed: Seed) { 4, ); - // if confirmed tx is added conflicting txs must be removed from the output cache + // if confirmed tx is added conflicting txs must be replaced in the output cache, leaving descendants intact let mut orders = wallet.get_orders(DEFAULT_ACCOUNT_INDEX).unwrap().collect_vec(); assert_eq!(orders.len(), 1); let (actual_order_id, order_data) = orders.pop().unwrap(); assert_eq!(*actual_order_id, order_id); - assert_eq!(order_data.last_nonce, Some(AccountNonce::new(0))); + assert_eq!(order_data.last_nonce, Some(AccountNonce::new(1))); + + let coin_balance_confirmed = get_balance_with( + &wallet, + Currency::Coin, + UtxoState::Confirmed.into(), + WithLocked::Any, + ); + assert_eq!( + coin_balance_confirmed, + (coin_balance_after_create_order - spend_coins_1).unwrap() + ); + + let coin_balance_unconfirmed = get_balance_with( + &wallet, + Currency::Coin, + UtxoState::Confirmed | UtxoState::Inactive, + WithLocked::Any, + ); + assert_eq!( + coin_balance_unconfirmed, + (coin_balance_after_create_order - spend_coins_1) + .and_then(|v| v - spend_coins_2) + .unwrap() + ); + + let token_balance_confirmed = get_balance_with( + &wallet, + Currency::Token(issued_token_id), + UtxoState::Confirmed.into(), + WithLocked::Any, + ); + assert_eq!(token_balance_confirmed, recieved_tokens_1); + + let token_balance_unconfirmed = get_balance_with( + &wallet, + Currency::Token(issued_token_id), + UtxoState::Confirmed | UtxoState::Inactive, + WithLocked::Any, + ); + assert_eq!( + token_balance_unconfirmed, + (recieved_tokens_1 + recieved_tokens_2).unwrap() + ); } From 8c5fdacfcfeb865b0d1d47d3c3c0fd370fc3e9b7 Mon Sep 17 00:00:00 2001 From: Heorhii Azarov Date: Wed, 22 Jan 2025 16:43:19 +0200 Subject: [PATCH 5/9] Fix comments --- wallet/src/account/output_cache/mod.rs | 80 ++++++++++++++------------ wallet/src/wallet/mod.rs | 2 + 2 files changed, 45 insertions(+), 37 deletions(-) diff --git a/wallet/src/account/output_cache/mod.rs b/wallet/src/account/output_cache/mod.rs index 133680824..94b7c9b11 100644 --- a/wallet/src/account/output_cache/mod.rs +++ b/wallet/src/account/output_cache/mod.rs @@ -689,60 +689,66 @@ impl OutputCache { confirmed_tx: &Transaction, block_id: Id, ) -> WalletResult>> { - let mut frozen_token_id: Option = None; - let mut confirmed_account_nonce: Option<(AccountType, AccountNonce)> = None; + struct Conflict { + frozen_token_id: Option, + confirmed_account_nonce: Option<(AccountType, AccountNonce)>, + } - for input in confirmed_tx.inputs() { + let conflict = confirmed_tx.inputs().iter().find_map(|input| { match input { TxInput::Utxo(_) => { //TODO: check conflicting utxo spends + None } - TxInput::Account(outpoint) => { - confirmed_account_nonce = Some((outpoint.account().into(), outpoint.nonce())); - } - TxInput::AccountCommand(nonce, cmd) => { - confirmed_account_nonce = Some((cmd.into(), *nonce)); - match cmd { - AccountCommand::MintTokens(_, _) - | AccountCommand::UnmintTokens(_) - | AccountCommand::LockTokenSupply(_) - | AccountCommand::ChangeTokenMetadataUri(_, _) - | AccountCommand::ChangeTokenAuthority(_, _) - | AccountCommand::UnfreezeToken(_) - | AccountCommand::ConcludeOrder(_) - | AccountCommand::FillOrder(_, _, _) => { /* do nothing */ } - | AccountCommand::FreezeToken(token_id, _) => { - frozen_token_id = Some(*token_id); - } - } - } + TxInput::Account(outpoint) => Some(Conflict { + frozen_token_id: None, + confirmed_account_nonce: Some((outpoint.account().into(), outpoint.nonce())), + }), + TxInput::AccountCommand(nonce, cmd) => match cmd { + AccountCommand::MintTokens(_, _) + | AccountCommand::UnmintTokens(_) + | AccountCommand::LockTokenSupply(_) + | AccountCommand::ChangeTokenMetadataUri(_, _) + | AccountCommand::ChangeTokenAuthority(_, _) + | AccountCommand::UnfreezeToken(_) + | AccountCommand::ConcludeOrder(_) + | AccountCommand::FillOrder(_, _, _) => Some(Conflict { + frozen_token_id: None, + confirmed_account_nonce: Some((cmd.into(), *nonce)), + }), + | AccountCommand::FreezeToken(token_id, _) => Some(Conflict { + frozen_token_id: Some(*token_id), + confirmed_account_nonce: Some((cmd.into(), *nonce)), + }), + }, } - } + }); // Collect all conflicting txs - let mut conflicting_txs = vec![]; + let mut conflicting_txs = BTreeSet::new(); - if frozen_token_id.is_some() || confirmed_account_nonce.is_some() { + if let Some(conflict) = conflict { for unconfirmed in self.unconfirmed_descendants.keys() { - if let Some(frozen_token_id) = frozen_token_id { - let unconfirmed_tx = self.txs.get(unconfirmed).expect("must be present"); + let unconfirmed_tx = self.txs.get(unconfirmed).expect("must be present"); + + if let Some(frozen_token_id) = conflict.frozen_token_id { if self.uses_token(unconfirmed_tx, &frozen_token_id) { if let WalletTx::Tx(tx) = unconfirmed_tx { - conflicting_txs.push(tx.get_transaction().get_id()); + conflicting_txs.insert(tx.get_transaction().get_id()); } } } - if let Some((confirmed_account, confirmed_account_nonce)) = confirmed_account_nonce + if let Some((confirmed_account, confirmed_account_nonce)) = + conflict.confirmed_account_nonce { - let unconfirmed_tx = self.txs.get(unconfirmed).expect("must be present"); if uses_conflicting_nonce( unconfirmed_tx, confirmed_account, confirmed_account_nonce, ) { if let WalletTx::Tx(tx) = unconfirmed_tx { - conflicting_txs.push(tx.get_transaction().get_id()); + conflicting_txs.insert(tx.get_transaction().get_id()); } } } @@ -832,7 +838,7 @@ impl OutputCache { if is_unconfirmed && !already_present { self.unconfirmed_descendants.insert(tx_id.clone(), BTreeSet::new()); - } else { + } else if !is_unconfirmed { self.unconfirmed_descendants.remove(&tx_id); } @@ -951,11 +957,11 @@ impl OutputCache { if let Some(descendants) = self.unconfirmed_descendants.get_mut(&outpoint.source_id()) { - if is_unconfirmed { - descendants.insert(tx_id.clone()); - } else { - descendants.remove(tx_id); - } + ensure!( + is_unconfirmed, + WalletError::ConfirmedTxAmongUnconfirmedDescendants(tx_id.clone()) + ); + descendants.insert(tx_id.clone()); } } TxInput::Account(outpoint) => match outpoint.account() { diff --git a/wallet/src/wallet/mod.rs b/wallet/src/wallet/mod.rs index 7fe0b912c..de28dfa93 100644 --- a/wallet/src/wallet/mod.rs +++ b/wallet/src/wallet/mod.rs @@ -251,6 +251,8 @@ pub enum WalletError { OrderInfoMissing(OrderId), #[error("Calculating filled amount for order {0} failed")] CalculateOrderFilledAmountFailed(OrderId), + #[error("Transaction from {0:?} is confirmed and among unconfirmed descendants")] + ConfirmedTxAmongUnconfirmedDescendants(OutPointSourceId), } /// Result type used for the wallet From 3327b2287f730dfb88dd0fda8f1bdf90cf2cd510 Mon Sep 17 00:00:00 2001 From: Heorhii Azarov Date: Wed, 29 Jan 2025 11:20:27 +0200 Subject: [PATCH 6/9] Error on changing to same tx state --- wallet/src/account/output_cache/mod.rs | 130 +++++++++++++------------ wallet/src/wallet/tests.rs | 77 +++++++++++++-- 2 files changed, 140 insertions(+), 67 deletions(-) diff --git a/wallet/src/account/output_cache/mod.rs b/wallet/src/account/output_cache/mod.rs index 94b7c9b11..01cc87244 100644 --- a/wallet/src/account/output_cache/mod.rs +++ b/wallet/src/account/output_cache/mod.rs @@ -748,7 +748,9 @@ impl OutputCache { confirmed_account_nonce, ) { if let WalletTx::Tx(tx) = unconfirmed_tx { - conflicting_txs.insert(tx.get_transaction().get_id()); + if confirmed_tx.get_id() != tx.get_transaction().get_id() { + conflicting_txs.insert(tx.get_transaction().get_id()); + } } } } @@ -759,14 +761,10 @@ impl OutputCache { let mut conflicting_txs_with_descendants = vec![]; for conflicting_tx in conflicting_txs { - if conflicting_tx != confirmed_tx.get_id() { - let descendants = self.remove_descendants_and_mark_as( - conflicting_tx, - TxState::Conflicted(block_id), - )?; + let descendants = + self.remove_descendants_and_mark_as(conflicting_tx, TxState::Conflicted(block_id))?; - conflicting_txs_with_descendants.extend(descendants.into_iter()); - } + conflicting_txs_with_descendants.extend(descendants.into_iter()); } Ok(conflicting_txs_with_descendants) @@ -1431,65 +1429,75 @@ impl OutputCache { 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(_) | TxState::Conflicted(_) | TxState::Abandoned => { - tx.set_state(new_state); - for input in tx.get_transaction().inputs() { - match input { - TxInput::Utxo(outpoint) => { - self.consumed.insert(outpoint.clone(), *tx.state()); - } - TxInput::Account(outpoint) => match outpoint.account() { - AccountSpending::DelegationBalance(delegation_id, _) => { - if let Some(data) = - self.delegations.get_mut(delegation_id) - { - data.last_nonce = outpoint.nonce().decrement(); - data.last_parent = find_parent( - &self.unconfirmed_descendants, - tx_id.into(), - ); - } + WalletTx::Tx(tx) => { + ensure!( + tx.state() != &new_state, + WalletError::CannotChangeTransactionState(*tx.state(), new_state) + ); + + match tx.state() { + TxState::Inactive(_) | TxState::Conflicted(_) | TxState::Abandoned => { + tx.set_state(new_state); + for input in tx.get_transaction().inputs() { + match input { + TxInput::Utxo(outpoint) => { + self.consumed.insert(outpoint.clone(), *tx.state()); } - }, - TxInput::AccountCommand(nonce, op) => match op { - AccountCommand::MintTokens(token_id, _) - | AccountCommand::UnmintTokens(token_id) - | AccountCommand::LockTokenSupply(token_id) - | AccountCommand::FreezeToken(token_id, _) - | AccountCommand::UnfreezeToken(token_id) - | AccountCommand::ChangeTokenMetadataUri(token_id, _) - | AccountCommand::ChangeTokenAuthority(token_id, _) => { - if let Some(data) = - self.token_issuance.get_mut(token_id) - { - data.last_nonce = nonce.decrement(); - data.last_parent = find_parent( - &self.unconfirmed_descendants, - tx_id.into(), - ); - data.unconfirmed_txs.remove(&tx_id.into()); + TxInput::Account(outpoint) => match outpoint.account() { + AccountSpending::DelegationBalance( + delegation_id, + _, + ) => { + if let Some(data) = + self.delegations.get_mut(delegation_id) + { + data.last_nonce = outpoint.nonce().decrement(); + data.last_parent = find_parent( + &self.unconfirmed_descendants, + tx_id.into(), + ); + } } - } - AccountCommand::ConcludeOrder(order_id) - | AccountCommand::FillOrder(order_id, _, _) => { - if let Some(data) = self.orders.get_mut(order_id) { - data.last_nonce = nonce.decrement(); - data.last_parent = find_parent( - &self.unconfirmed_descendants, - tx_id.into(), - ); + }, + TxInput::AccountCommand(nonce, op) => match op { + AccountCommand::MintTokens(token_id, _) + | AccountCommand::UnmintTokens(token_id) + | AccountCommand::LockTokenSupply(token_id) + | AccountCommand::FreezeToken(token_id, _) + | AccountCommand::UnfreezeToken(token_id) + | AccountCommand::ChangeTokenMetadataUri(token_id, _) + | AccountCommand::ChangeTokenAuthority(token_id, _) => { + if let Some(data) = + self.token_issuance.get_mut(token_id) + { + data.last_nonce = nonce.decrement(); + data.last_parent = find_parent( + &self.unconfirmed_descendants, + tx_id.into(), + ); + data.unconfirmed_txs.remove(&tx_id.into()); + } } - } - }, + AccountCommand::ConcludeOrder(order_id) + | AccountCommand::FillOrder(order_id, _, _) => { + if let Some(data) = self.orders.get_mut(order_id) { + data.last_nonce = nonce.decrement(); + data.last_parent = find_parent( + &self.unconfirmed_descendants, + tx_id.into(), + ); + } + } + }, + } } + Ok(()) } - Ok(()) + TxState::Confirmed(..) | TxState::InMempool(..) => Err( + WalletError::CannotChangeTransactionState(*tx.state(), new_state), + ), } - TxState::Confirmed(..) | TxState::InMempool(..) => Err( - WalletError::CannotChangeTransactionState(*tx.state(), new_state), - ), - }, + } }, Entry::Vacant(_) => Err(WalletError::CannotFindTransactionWithId(tx_id)), }?; diff --git a/wallet/src/wallet/tests.rs b/wallet/src/wallet/tests.rs index 040ccbb54..96a6f292b 100644 --- a/wallet/src/wallet/tests.rs +++ b/wallet/src/wallet/tests.rs @@ -3899,6 +3899,13 @@ fn wallet_abandone_transactions(#[case] seed: Seed) { wallet.scan_mempool(txs_to_keep.as_slice(), &WalletEventsNoOp).unwrap(); let coin_balance = get_coin_balance_with_inactive(&wallet); assert_eq!(coin_balance, coins_after_abandon); + + // Abandone the same tx again + let result = wallet.abandon_transaction(DEFAULT_ACCOUNT_INDEX, transaction_id); + assert_eq!( + result.unwrap_err(), + WalletError::CannotChangeTransactionState(TxState::Abandoned, TxState::Abandoned) + ); } #[rstest] @@ -5940,12 +5947,12 @@ fn create_order_fill_partially_conclude(#[case] seed: Seed) { } } -// Create 2 wallet from the same mnemonic. +// Create 2 wallets from the same mnemonic. // Create a pool and a delegation with some stake for both wallets. -// Add 2 consequtive txs that spend share from delelgation to the first wallet as unconfirmed. +// Add 2 consecutive txs that spend share from delegation to the first wallet as unconfirmed. // Add 1 txs that spends share from delegation with different amount (so that tx id is different) // via the second wallet and submit it to the block. -// Check that 2 unconfirmed txs from the first wallet conflicts with confirmed tx and got removed. +// Check that 2 unconfirmed txs from the first wallet conflict with confirmed tx and got removed. #[rstest] #[trace] #[case(Seed::from_entropy())] @@ -6071,6 +6078,7 @@ fn conflicting_delegation_account_nonce(#[case] seed: Seed) { FeeRate::from_amount_per_kb(Amount::ZERO), ) .unwrap(); + let spend_from_delegation_tx_1_id = spend_from_delegation_tx_1.transaction().get_id(); wallet .add_account_unconfirmed_tx( @@ -6091,6 +6099,7 @@ fn conflicting_delegation_account_nonce(#[case] seed: Seed) { FeeRate::from_amount_per_kb(Amount::ZERO), ) .unwrap(); + let spend_from_delegation_tx_2_id = spend_from_delegation_tx_2.transaction().get_id(); wallet .add_account_unconfirmed_tx( @@ -6119,6 +6128,7 @@ fn conflicting_delegation_account_nonce(#[case] seed: Seed) { FeeRate::from_amount_per_kb(Amount::ZERO), ) .unwrap(); + let spend_from_delegation_tx_3_id = spend_from_delegation_tx_3.transaction().get_id(); let (_, block5) = create_block( &chain_config, @@ -6127,6 +6137,7 @@ fn conflicting_delegation_account_nonce(#[case] seed: Seed) { Amount::ZERO, 4, ); + let block5_id = block5.get_id(); scan_wallet(&mut wallet, BlockHeight::new(4), vec![block5]); // if confirmed tx is added conflicting txs must be removed from the output cache @@ -6157,13 +6168,62 @@ fn conflicting_delegation_account_nonce(#[case] seed: Seed) { coin_balance, (coin_balance_after_delegating + withdraw_amount_3).unwrap() ); + + assert_eq!( + *wallet + .get_transaction(DEFAULT_ACCOUNT_INDEX, spend_from_delegation_tx_1_id) + .unwrap() + .state(), + TxState::Conflicted(block5_id.into()) + ); + assert_eq!( + *wallet + .get_transaction(DEFAULT_ACCOUNT_INDEX, spend_from_delegation_tx_2_id) + .unwrap() + .state(), + TxState::Conflicted(block5_id.into()) + ); + assert_eq!( + *wallet + .get_transaction(DEFAULT_ACCOUNT_INDEX, spend_from_delegation_tx_3_id) + .unwrap() + .state(), + TxState::Confirmed( + BlockHeight::new(5), + chain_config.genesis_block().timestamp(), + 0 + ) + ); + + // Abandone conflicting txs + wallet + .abandon_transaction(DEFAULT_ACCOUNT_INDEX, spend_from_delegation_tx_1_id) + .unwrap(); + assert_eq!( + *wallet + .get_transaction(DEFAULT_ACCOUNT_INDEX, spend_from_delegation_tx_1_id) + .unwrap() + .state(), + TxState::Abandoned + ); + + wallet + .abandon_transaction(DEFAULT_ACCOUNT_INDEX, spend_from_delegation_tx_2_id) + .unwrap(); + assert_eq!( + *wallet + .get_transaction(DEFAULT_ACCOUNT_INDEX, spend_from_delegation_tx_2_id) + .unwrap() + .state(), + TxState::Abandoned + ); } // Create a pool and a delegation with some share. -// Create 2 consequtive txs that spend from delegation account and add them as unconfirmed. +// Create 2 consecutive txs that spend from delegation account and add them as unconfirmed. // Check confirmed/unconfirmed balance and ensure that account nonce is incremented in OutputCache. // Submit the first tx in a block. -// Check that Confirmed balance changed but +// Check that confirmed balance changed and unconfirmed stayed the same. #[rstest] #[trace] #[case(Seed::from_entropy())] @@ -6383,6 +6443,11 @@ fn conflicting_delegation_account_nonce_same_tx(#[case] seed: Seed) { ); } +// Issue and mint some tokens +// Create an order selling tokens for coins +// Create 2 fill order txs and add them to a wallet as unconfirmed +// Confirm the first tx in a block and check that it is accounted in confirmed balance +// and also that unconfirmed balance has second tx. #[rstest] #[trace] #[case(Seed::from_entropy())] @@ -6496,7 +6561,7 @@ fn conflicting_order_account_nonce(#[case] seed: Seed) { assert_eq!(actual_order_data.last_nonce, None); assert_eq!(&actual_order_data.conclude_key, address2.as_object()); - // Create 2 fill orders txs and put them in unconfirmed + // Create 2 fill order txs and put them in unconfirmed let order_info = RpcOrderInfo { conclude_key: address2.clone().into_object(), initially_given: RpcOutputValue::Token { From 529d6bfe899b0ca2cfb34c0f992d4f791bac8585 Mon Sep 17 00:00:00 2001 From: Heorhii Azarov Date: Thu, 30 Jan 2025 13:08:05 +0200 Subject: [PATCH 7/9] Fix abandoning conflicting txs --- wallet/src/account/output_cache/mod.rs | 196 ++++++++++++++----------- wallet/src/wallet/tests.rs | 12 ++ 2 files changed, 126 insertions(+), 82 deletions(-) diff --git a/wallet/src/account/output_cache/mod.rs b/wallet/src/account/output_cache/mod.rs index 01cc87244..7a438eab5 100644 --- a/wallet/src/account/output_cache/mod.rs +++ b/wallet/src/account/output_cache/mod.rs @@ -761,8 +761,40 @@ impl OutputCache { let mut conflicting_txs_with_descendants = vec![]; for conflicting_tx in conflicting_txs { - let descendants = - self.remove_descendants_and_mark_as(conflicting_tx, TxState::Conflicted(block_id))?; + let descendants = self.remove_descendants(conflicting_tx); + + // Mark conflicting tx and its descendants as Conflicting and update OutputCache data accordingly + for tx_id in descendants.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::Conflicted(block_id)); + OutputCache::rollback_tx_data( + tx, + &self.unconfirmed_descendants, + &mut self.consumed, + &mut self.delegations, + &mut self.token_issuance, + &mut self.orders, + ); + Ok(()) + } + TxState::Abandoned + | TxState::Confirmed(..) + | TxState::InMempool(..) + | TxState::Conflicted(..) => { + Err(WalletError::CannotChangeTransactionState( + *tx.state(), + TxState::Conflicted(block_id), + )) + } + }, + }, + Entry::Vacant(_) => Err(WalletError::CannotFindTransactionWithId(tx_id)), + }?; + } conflicting_txs_with_descendants.extend(descendants.into_iter()); } @@ -1409,11 +1441,7 @@ impl OutputCache { }) } - fn remove_descendants_and_mark_as( - &mut self, - tx_id: Id, - new_state: TxState, - ) -> WalletResult>> { + fn remove_descendants(&mut self, tx_id: Id) -> Vec> { let mut all_txs = Vec::new(); let mut to_update = BTreeSet::from_iter([OutPointSourceId::from(tx_id)]); @@ -1425,85 +1453,56 @@ impl OutputCache { } } - for tx_id in all_txs.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) => { - ensure!( - tx.state() != &new_state, - WalletError::CannotChangeTransactionState(*tx.state(), new_state) - ); + all_txs + } - match tx.state() { - TxState::Inactive(_) | TxState::Conflicted(_) | TxState::Abandoned => { - tx.set_state(new_state); - for input in tx.get_transaction().inputs() { - match input { - TxInput::Utxo(outpoint) => { - self.consumed.insert(outpoint.clone(), *tx.state()); - } - TxInput::Account(outpoint) => match outpoint.account() { - AccountSpending::DelegationBalance( - delegation_id, - _, - ) => { - if let Some(data) = - self.delegations.get_mut(delegation_id) - { - data.last_nonce = outpoint.nonce().decrement(); - data.last_parent = find_parent( - &self.unconfirmed_descendants, - tx_id.into(), - ); - } - } - }, - TxInput::AccountCommand(nonce, op) => match op { - AccountCommand::MintTokens(token_id, _) - | AccountCommand::UnmintTokens(token_id) - | AccountCommand::LockTokenSupply(token_id) - | AccountCommand::FreezeToken(token_id, _) - | AccountCommand::UnfreezeToken(token_id) - | AccountCommand::ChangeTokenMetadataUri(token_id, _) - | AccountCommand::ChangeTokenAuthority(token_id, _) => { - if let Some(data) = - self.token_issuance.get_mut(token_id) - { - data.last_nonce = nonce.decrement(); - data.last_parent = find_parent( - &self.unconfirmed_descendants, - tx_id.into(), - ); - data.unconfirmed_txs.remove(&tx_id.into()); - } - } - AccountCommand::ConcludeOrder(order_id) - | AccountCommand::FillOrder(order_id, _, _) => { - if let Some(data) = self.orders.get_mut(order_id) { - data.last_nonce = nonce.decrement(); - data.last_parent = find_parent( - &self.unconfirmed_descendants, - tx_id.into(), - ); - } - } - }, - } - } - Ok(()) - } - TxState::Confirmed(..) | TxState::InMempool(..) => Err( - WalletError::CannotChangeTransactionState(*tx.state(), new_state), - ), + // After tx is abandoned or marked as conflicted its effect on OutputCache should be rolled back + fn rollback_tx_data( + tx: &TxData, + unconfirmed_descendants: &BTreeMap>, + consumed: &mut BTreeMap, + delegations: &mut BTreeMap, + token_issuance: &mut BTreeMap, + orders: &mut BTreeMap, + ) { + let tx_id = tx.get_transaction().get_id(); + for input in tx.get_transaction().inputs() { + match input { + TxInput::Utxo(outpoint) => { + consumed.insert(outpoint.clone(), *tx.state()); + } + TxInput::Account(outpoint) => match outpoint.account() { + AccountSpending::DelegationBalance(delegation_id, _) => { + if let Some(data) = delegations.get_mut(delegation_id) { + data.last_nonce = outpoint.nonce().decrement(); + data.last_parent = find_parent(unconfirmed_descendants, tx_id.into()); } } }, - Entry::Vacant(_) => Err(WalletError::CannotFindTransactionWithId(tx_id)), - }?; + TxInput::AccountCommand(nonce, op) => match op { + AccountCommand::MintTokens(token_id, _) + | AccountCommand::UnmintTokens(token_id) + | AccountCommand::LockTokenSupply(token_id) + | AccountCommand::FreezeToken(token_id, _) + | AccountCommand::UnfreezeToken(token_id) + | AccountCommand::ChangeTokenMetadataUri(token_id, _) + | AccountCommand::ChangeTokenAuthority(token_id, _) => { + if let Some(data) = token_issuance.get_mut(token_id) { + data.last_nonce = nonce.decrement(); + data.last_parent = find_parent(unconfirmed_descendants, tx_id.into()); + data.unconfirmed_txs.remove(&tx_id.into()); + } + } + AccountCommand::ConcludeOrder(order_id) + | AccountCommand::FillOrder(order_id, _, _) => { + if let Some(data) = orders.get_mut(order_id) { + data.last_nonce = nonce.decrement(); + data.last_parent = find_parent(unconfirmed_descendants, tx_id.into()); + } + } + }, + } } - - Ok(all_txs) } /// Mark a transaction and its descendants as abandoned @@ -1512,7 +1511,40 @@ impl OutputCache { &mut self, tx_id: Id, ) -> WalletResult>> { - self.remove_descendants_and_mark_as(tx_id, TxState::Abandoned) + let all_abandoned = self.remove_descendants(tx_id); + + 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); + OutputCache::rollback_tx_data( + tx, + &self.unconfirmed_descendants, + &mut self.consumed, + &mut self.delegations, + &mut self.token_issuance, + &mut self.orders, + ); + Ok(()) + } + TxState::Conflicted(_) => { + tx.set_state(TxState::Abandoned); + Ok(()) + } + state => Err(WalletError::CannotChangeTransactionState( + *state, + TxState::Abandoned, + )), + }, + }, + Entry::Vacant(_) => Err(WalletError::CannotFindTransactionWithId(tx_id)), + }?; + } + + Ok(all_abandoned) } pub fn get_transaction(&self, transaction_id: Id) -> WalletResult<&TxData> { diff --git a/wallet/src/wallet/tests.rs b/wallet/src/wallet/tests.rs index 96a6f292b..1bda5a913 100644 --- a/wallet/src/wallet/tests.rs +++ b/wallet/src/wallet/tests.rs @@ -6207,6 +6207,12 @@ fn conflicting_delegation_account_nonce(#[case] seed: Seed) { TxState::Abandoned ); + let mut delegations = wallet.get_delegations(DEFAULT_ACCOUNT_INDEX).unwrap().collect_vec(); + assert_eq!(delegations.len(), 1); + let (deleg_id, deleg_data) = delegations.pop().unwrap(); + assert_eq!(*deleg_id, delegation_id); + assert_eq!(deleg_data.last_nonce, Some(AccountNonce::new(0))); + wallet .abandon_transaction(DEFAULT_ACCOUNT_INDEX, spend_from_delegation_tx_2_id) .unwrap(); @@ -6217,6 +6223,12 @@ fn conflicting_delegation_account_nonce(#[case] seed: Seed) { .state(), TxState::Abandoned ); + + let mut delegations = wallet.get_delegations(DEFAULT_ACCOUNT_INDEX).unwrap().collect_vec(); + assert_eq!(delegations.len(), 1); + let (deleg_id, deleg_data) = delegations.pop().unwrap(); + assert_eq!(*deleg_id, delegation_id); + assert_eq!(deleg_data.last_nonce, Some(AccountNonce::new(0))); } // Create a pool and a delegation with some share. From 83b515af65b3242e1c6d5ead1f1348492eadcfa5 Mon Sep 17 00:00:00 2001 From: Heorhii Azarov Date: Mon, 3 Feb 2025 11:26:47 +0200 Subject: [PATCH 8/9] Fix comments --- wallet/src/account/output_cache/mod.rs | 62 ++++++++++++++------------ wallet/src/wallet/mod.rs | 6 +++ wallet/src/wallet/tests.rs | 36 +++++++-------- 3 files changed, 57 insertions(+), 47 deletions(-) diff --git a/wallet/src/account/output_cache/mod.rs b/wallet/src/account/output_cache/mod.rs index 7a438eab5..99b56ff87 100644 --- a/wallet/src/account/output_cache/mod.rs +++ b/wallet/src/account/output_cache/mod.rs @@ -698,6 +698,7 @@ impl OutputCache { match input { TxInput::Utxo(_) => { //TODO: check conflicting utxo spends + // See https://github.com/mintlayer/mintlayer-core/issues/1875 None } TxInput::Account(outpoint) => Some(Conflict { @@ -731,26 +732,24 @@ impl OutputCache { for unconfirmed in self.unconfirmed_descendants.keys() { let unconfirmed_tx = self.txs.get(unconfirmed).expect("must be present"); - if let Some(frozen_token_id) = conflict.frozen_token_id { - if self.uses_token(unconfirmed_tx, &frozen_token_id) { - if let WalletTx::Tx(tx) = unconfirmed_tx { + if let WalletTx::Tx(tx) = unconfirmed_tx { + if let Some(frozen_token_id) = conflict.frozen_token_id { + if self.uses_token(unconfirmed_tx, &frozen_token_id) { conflicting_txs.insert(tx.get_transaction().get_id()); } } - } - if let Some((confirmed_account, confirmed_account_nonce)) = - conflict.confirmed_account_nonce - { - if uses_conflicting_nonce( - unconfirmed_tx, - confirmed_account, - confirmed_account_nonce, - ) { - if let WalletTx::Tx(tx) = unconfirmed_tx { - if confirmed_tx.get_id() != tx.get_transaction().get_id() { - conflicting_txs.insert(tx.get_transaction().get_id()); - } + if let Some((confirmed_account, confirmed_account_nonce)) = + conflict.confirmed_account_nonce + { + 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()); } } } @@ -761,15 +760,17 @@ impl OutputCache { let mut conflicting_txs_with_descendants = vec![]; for conflicting_tx in conflicting_txs { - let descendants = self.remove_descendants(conflicting_tx); + let txs_to_rollback = self.remove_from_unconfirmed_descendants(conflicting_tx); // Mark conflicting tx and its descendants as Conflicting and update OutputCache data accordingly - for tx_id in descendants.iter().rev().copied() { + for tx_id in txs_to_rollback.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::Block(_) => { + Err(WalletError::TransactionIdCannotMapToBlock(tx_id)) + } WalletTx::Tx(tx) => match tx.state() { - TxState::Inactive(_) => { + TxState::Inactive(_) | TxState::InMempool(_) => { tx.set_state(TxState::Conflicted(block_id)); OutputCache::rollback_tx_data( tx, @@ -783,20 +784,18 @@ impl OutputCache { } TxState::Abandoned | TxState::Confirmed(..) - | TxState::InMempool(..) | TxState::Conflicted(..) => { - Err(WalletError::CannotChangeTransactionState( - *tx.state(), - TxState::Conflicted(block_id), - )) + Err(WalletError::CannotMarkTxAsConflictedIfInState(*tx.state())) } }, }, - Entry::Vacant(_) => Err(WalletError::CannotFindTransactionWithId(tx_id)), + Entry::Vacant(_) => { + Err(WalletError::CannotFindDescendantTransactionWithId(tx_id)) + } }?; } - conflicting_txs_with_descendants.extend(descendants.into_iter()); + conflicting_txs_with_descendants.extend(txs_to_rollback.into_iter()); } Ok(conflicting_txs_with_descendants) @@ -1441,7 +1440,12 @@ impl OutputCache { }) } - fn remove_descendants(&mut self, tx_id: Id) -> Vec> { + // Removes a tx from unconfirmed descendant. + // Returns provided tx and all the descendants. + fn remove_from_unconfirmed_descendants( + &mut self, + tx_id: Id, + ) -> Vec> { let mut all_txs = Vec::new(); let mut to_update = BTreeSet::from_iter([OutPointSourceId::from(tx_id)]); @@ -1511,7 +1515,7 @@ impl OutputCache { &mut self, tx_id: Id, ) -> WalletResult>> { - let all_abandoned = self.remove_descendants(tx_id); + let all_abandoned = self.remove_from_unconfirmed_descendants(tx_id); for tx_id in all_abandoned.iter().rev().copied() { match self.txs.entry(tx_id.into()) { diff --git a/wallet/src/wallet/mod.rs b/wallet/src/wallet/mod.rs index de28dfa93..4696c1c01 100644 --- a/wallet/src/wallet/mod.rs +++ b/wallet/src/wallet/mod.rs @@ -177,8 +177,14 @@ pub enum WalletError { CoinSelectionError(#[from] UtxoSelectorError), #[error("Cannot change a transaction's state from {0} to {1}")] CannotChangeTransactionState(TxState, TxState), + #[error("Cannot mark as conflicted transaction in {0} state")] + CannotMarkTxAsConflictedIfInState(TxState), #[error("Transaction with Id {0} not found")] CannotFindTransactionWithId(Id), + #[error("Transaction with Id {0} cannot map to a block")] + TransactionIdCannotMapToBlock(Id), + #[error("Descendant transaction with Id {0} not found")] + CannotFindDescendantTransactionWithId(Id), #[error("Address error: {0}")] AddressError(#[from] AddressError), #[error("Unknown pool id {0}")] diff --git a/wallet/src/wallet/tests.rs b/wallet/src/wallet/tests.rs index 1bda5a913..e3dbfe32f 100644 --- a/wallet/src/wallet/tests.rs +++ b/wallet/src/wallet/tests.rs @@ -3481,17 +3481,17 @@ fn lock_then_transfer(#[case] seed: Seed) { scan_wallet(&mut wallet, BlockHeight::new(1), vec![block2]); // check balance - let balance_without_locked_transer = + let balance_without_locked_transfer = ((block1_amount * 2).unwrap() - amount_to_lock_then_transfer).unwrap(); let coin_balance = get_coin_balance(&wallet); - assert_eq!(coin_balance, balance_without_locked_transer); + assert_eq!(coin_balance, balance_without_locked_transfer); // check that for block_count_lock, the amount is not included for idx in 0..block_count_lock { let coin_balance = get_coin_balance(&wallet); // check that the amount is still not unlocked - assert_eq!(coin_balance, balance_without_locked_transer); + assert_eq!(coin_balance, balance_without_locked_transfer); let currency_balances = wallet .get_balance( @@ -3524,7 +3524,7 @@ fn lock_then_transfer(#[case] seed: Seed) { let coin_balance = get_coin_balance(&wallet); assert_eq!( coin_balance, - (balance_without_locked_transer + amount_to_lock_then_transfer).unwrap() + (balance_without_locked_transfer + amount_to_lock_then_transfer).unwrap() ); } @@ -3882,15 +3882,15 @@ fn wallet_abandone_transactions(#[case] seed: Seed) { assert_eq!(abandonable_transactions.len(), transactions.len()); - let txs_to_abandone = rng.gen_range(0..total_num_transactions) as usize; - let (txs_to_keep, txs_to_abandone) = transactions.split_at(txs_to_abandone); + let txs_to_abandon = rng.gen_range(0..total_num_transactions) as usize; + let (txs_to_keep, txs_to_abandon) = transactions.split_at(txs_to_abandon); - assert!(!txs_to_abandone.is_empty()); + assert!(!txs_to_abandon.is_empty()); - let transaction_id = txs_to_abandone.first().unwrap().0.transaction().get_id(); + let transaction_id = txs_to_abandon.first().unwrap().0.transaction().get_id(); wallet.abandon_transaction(DEFAULT_ACCOUNT_INDEX, transaction_id).unwrap(); - let coins_after_abandon = txs_to_abandone.first().unwrap().1; + let coins_after_abandon = txs_to_abandon.first().unwrap().1; let coin_balance = get_coin_balance_with_inactive(&wallet); assert_eq!(coin_balance, coins_after_abandon); @@ -3900,7 +3900,7 @@ fn wallet_abandone_transactions(#[case] seed: Seed) { let coin_balance = get_coin_balance_with_inactive(&wallet); assert_eq!(coin_balance, coins_after_abandon); - // Abandone the same tx again + // Abandon the same tx again let result = wallet.abandon_transaction(DEFAULT_ACCOUNT_INDEX, transaction_id); assert_eq!( result.unwrap_err(), @@ -6195,7 +6195,7 @@ fn conflicting_delegation_account_nonce(#[case] seed: Seed) { ) ); - // Abandone conflicting txs + // Abandon conflicting txs wallet .abandon_transaction(DEFAULT_ACCOUNT_INDEX, spend_from_delegation_tx_1_id) .unwrap(); @@ -6455,9 +6455,9 @@ fn conflicting_delegation_account_nonce_same_tx(#[case] seed: Seed) { ); } -// Issue and mint some tokens -// Create an order selling tokens for coins -// Create 2 fill order txs and add them to a wallet as unconfirmed +// Issue and mint some tokens. +// Create an order selling tokens for coins. +// Create 2 fill order txs and add them to a wallet as unconfirmed. // Confirm the first tx in a block and check that it is accounted in confirmed balance // and also that unconfirmed balance has second tx. #[rstest] @@ -6589,7 +6589,7 @@ fn conflicting_order_account_nonce(#[case] seed: Seed) { }; let spend_coins_1 = Amount::from_atoms(10); - let recieved_tokens_1 = spend_coins_1; + let received_tokens_1 = spend_coins_1; let fill_order_tx_1 = wallet .create_fill_order_tx( DEFAULT_ACCOUNT_INDEX, @@ -6625,7 +6625,7 @@ fn conflicting_order_account_nonce(#[case] seed: Seed) { }; let spend_coins_2 = Amount::from_atoms(3); - let recieved_tokens_2 = spend_coins_2; + let received_tokens_2 = spend_coins_2; let fill_order_tx_2 = wallet .create_fill_order_tx( DEFAULT_ACCOUNT_INDEX, @@ -6698,7 +6698,7 @@ fn conflicting_order_account_nonce(#[case] seed: Seed) { UtxoState::Confirmed.into(), WithLocked::Any, ); - assert_eq!(token_balance_confirmed, recieved_tokens_1); + assert_eq!(token_balance_confirmed, received_tokens_1); let token_balance_unconfirmed = get_balance_with( &wallet, @@ -6708,6 +6708,6 @@ fn conflicting_order_account_nonce(#[case] seed: Seed) { ); assert_eq!( token_balance_unconfirmed, - (recieved_tokens_1 + recieved_tokens_2).unwrap() + (received_tokens_1 + received_tokens_2).unwrap() ); } From caa2c45ca9ab0943ff4eab1fbb9954e5914a9630 Mon Sep 17 00:00:00 2001 From: Heorhii Azarov Date: Tue, 4 Feb 2025 10:44:02 +0200 Subject: [PATCH 9/9] Fix conflicting duplicates --- wallet/src/account/output_cache/mod.rs | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/wallet/src/account/output_cache/mod.rs b/wallet/src/account/output_cache/mod.rs index 99b56ff87..81eceef52 100644 --- a/wallet/src/account/output_cache/mod.rs +++ b/wallet/src/account/output_cache/mod.rs @@ -782,9 +782,17 @@ impl OutputCache { ); Ok(()) } - TxState::Abandoned - | TxState::Confirmed(..) - | TxState::Conflicted(..) => { + TxState::Conflicted(..) => { + // It's possible to try to mark descendant as conflicting twice + // because unconfirmed_descendants contains a tx as child and as parent. + // So it's not an error only if done during this function call. + ensure!( + conflicting_txs_with_descendants.contains(&tx_id), + WalletError::CannotMarkTxAsConflictedIfInState(*tx.state()) + ); + Ok(()) + } + TxState::Abandoned | TxState::Confirmed(..) => { Err(WalletError::CannotMarkTxAsConflictedIfInState(*tx.state())) } },