From 0e44ac95ba8c89c9b1cf95e0e77e7279842a2a97 Mon Sep 17 00:00:00 2001 From: Boris Oncev Date: Tue, 29 Apr 2025 15:51:42 +0200 Subject: [PATCH 1/4] fix tx fee calculation for large number of inputs - account for compact input length encoding - fix rounding error in case of individual input fee calculation vs total fee calculation using entire tx size - add missing limit for tx size/weight --- common/src/size_estimation/mod.rs | 16 +- wallet/src/account/mod.rs | 245 +++++++++++------- wallet/src/account/utxo_selector/mod.rs | 30 ++- .../src/account/utxo_selector/output_group.rs | 4 +- wallet/src/wallet/tests.rs | 110 +++++++- wasm-wrappers/src/lib.rs | 2 +- 6 files changed, 291 insertions(+), 116 deletions(-) diff --git a/common/src/size_estimation/mod.rs b/common/src/size_estimation/mod.rs index 67db3f015..27d457ac9 100644 --- a/common/src/size_estimation/mod.rs +++ b/common/src/size_estimation/mod.rs @@ -194,13 +194,25 @@ pub fn input_signature_size_from_destination( /// Return the encoded size for a SignedTransaction with specified outputs and empty inputs and /// signatures -pub fn tx_size_with_outputs(outputs: &[TxOutput]) -> usize { +pub fn tx_size_with_outputs(outputs: &[TxOutput], num_inputs: usize) -> usize { + #[derive(Encode)] + struct CompactSize { + #[codec(compact)] + value: u64, + } + let tx = SignedTransaction::new( Transaction::new(1, vec![], outputs.into()).expect("should not fail"), vec![], ) .expect("should not fail"); - serialization::Encode::encoded_size(&tx) + let size = serialization::Encode::encoded_size(&tx); + + let compact_size_diff = serialization::Encode::encoded_size(&CompactSize { + value: num_inputs as u64, + }) - serialization::Encode::encoded_size(&CompactSize { value: 0 }); + + size + (compact_size_diff * 2) // 2 for number of inputs and number of input signatures } fn get_tx_output_destination(txo: &TxOutput) -> Option<&Destination> { diff --git a/wallet/src/account/mod.rs b/wallet/src/account/mod.rs index 3ae7997f8..1ffc38619 100644 --- a/wallet/src/account/mod.rs +++ b/wallet/src/account/mod.rs @@ -37,6 +37,7 @@ use crypto::key::hdkd::child_number::ChildNumber; use mempool::FeeRate; use serialization::hex_encoded::HexEncoded; use utils::ensure; +use utxo_selector::SelectionResult; pub use utxo_selector::UtxoSelectorError; use wallet_types::account_id::AccountPrefixedId; use wallet_types::account_info::{StandaloneAddressDetails, StandaloneAddresses}; @@ -245,12 +246,6 @@ impl Account { Ok(()) })?; - let network_fee: Amount = fee_rates - .current_fee_rate - .compute_fee(tx_size_with_outputs(request.outputs())) - .map_err(|_| UtxoSelectorError::AmountArithmeticError)? - .into(); - let (utxos, selection_algo) = if input_utxos.is_empty() { ( self.get_utxos( @@ -288,27 +283,26 @@ impl Account { let amount_to_be_paid_in_currency_with_fees = output_currency_amounts.remove(&pay_fee_with_currency).unwrap_or(Amount::ZERO); - let mut total_fees_not_paid = network_fee; + let mut total_fees_not_paid = Amount::ZERO; + let mut total_tx_size = 0; let mut selected_inputs: BTreeMap<_, _> = output_currency_amounts .iter() .map(|(currency, output_amount)| -> WalletResult<_> { let utxos = utxos_by_currency.remove(currency).unwrap_or(vec![]); - let (preselected_amount, preselected_fee) = preselected_inputs - .remove(currency) - .map_or((Amount::ZERO, Amount::ZERO), |inputs| { - (inputs.amount, inputs.fee) - }); - - let (coin_change_fee, token_change_fee) = coin_and_token_output_change_fees( + let (preselected_amount, preselected_fee, preselected_input_size) = + preselected_inputs + .remove(currency) + .map_or((Amount::ZERO, Amount::ZERO, 0), |inputs| { + (inputs.amount, inputs.fee, inputs.total_input_size) + }); + + let (size_of_change, cost_of_change) = coin_and_token_output_change_fees( current_fee_rate, + currency, change_addresses.get(currency), )?; - let cost_of_change = match currency { - Currency::Coin => coin_change_fee, - Currency::Token(_) => token_change_fee, - }; let selection_result = select_coins( utxos, output_amount.sub(preselected_amount).unwrap_or(Amount::ZERO), @@ -317,8 +311,11 @@ impl Account { // when we allow paying fees with different currency Amount::ZERO, selection_algo, + self.chain_config.max_tx_size_for_mempool(), )?; + total_tx_size += selection_result.get_weight() + preselected_input_size; + total_fees_not_paid = (total_fees_not_paid + selection_result.get_total_fees()) .ok_or(WalletError::OutputAmountOverflow)?; total_fees_not_paid = (total_fees_not_paid + preselected_fee) @@ -331,19 +328,36 @@ impl Account { if change_amount > Amount::ZERO { total_fees_not_paid = (total_fees_not_paid + cost_of_change) .ok_or(WalletError::OutputAmountOverflow)?; + total_tx_size += size_of_change; } Ok((*currency, selection_result)) }) .try_collect()?; + let num_inputs = selected_inputs + .values() + .map(SelectionResult::num_selected_inputs) + .sum::() + + request.inputs().len(); + + let tx_outputs_size = tx_size_with_outputs(request.outputs(), num_inputs); + let tx_outputs_fee: Amount = current_fee_rate + .compute_fee(tx_outputs_size) + .map_err(|_| UtxoSelectorError::AmountArithmeticError)? + .into(); + let utxos = utxos_by_currency.remove(&pay_fee_with_currency).unwrap_or(vec![]); - let (preselected_amount, preselected_fee) = preselected_inputs + let (preselected_amount, preselected_fee, preselected_input_size) = preselected_inputs .remove(&pay_fee_with_currency) - .map_or((Amount::ZERO, Amount::ZERO), |inputs| { - (inputs.amount, inputs.fee) + .map_or((Amount::ZERO, Amount::ZERO, 0), |inputs| { + (inputs.amount, inputs.fee, inputs.total_input_size) }); + total_tx_size += tx_outputs_size + preselected_input_size; + + total_fees_not_paid = + (total_fees_not_paid + tx_outputs_fee).ok_or(WalletError::OutputAmountOverflow)?; total_fees_not_paid = (total_fees_not_paid + preselected_fee).ok_or(WalletError::OutputAmountOverflow)?; total_fees_not_paid = preselected_inputs @@ -351,18 +365,19 @@ impl Account { .try_fold(total_fees_not_paid, |total, inputs| total + inputs.fee) .ok_or(WalletError::OutputAmountOverflow)?; - let mut amount_to_be_paid_in_currency_with_fees = (amount_to_be_paid_in_currency_with_fees + total_tx_size = preselected_inputs.values().fold(total_tx_size, |total, inputs| { + total + inputs.total_input_size + }); + + let amount_to_be_paid_in_currency_with_fees = (amount_to_be_paid_in_currency_with_fees + total_fees_not_paid) .ok_or(WalletError::OutputAmountOverflow)?; - let (coin_change_fee, token_change_fee) = coin_and_token_output_change_fees( + let (size_of_change, cost_of_change) = coin_and_token_output_change_fees( current_fee_rate, + &pay_fee_with_currency, change_addresses.get(&pay_fee_with_currency), )?; - let cost_of_change = match pay_fee_with_currency { - Currency::Coin => coin_change_fee, - Currency::Token(_) => token_change_fee, - }; let selection_result = select_coins( utxos, @@ -370,29 +385,40 @@ impl Account { PayFee::PayFeeWithThisCurrency, cost_of_change, selection_algo, + self.chain_config.max_tx_size_for_mempool(), )?; - let selection_result = selection_result.add_change( + let mut selection_result = selection_result.add_change( (preselected_amount - amount_to_be_paid_in_currency_with_fees).unwrap_or(Amount::ZERO), )?; + let change_amount = selection_result.get_change(); + total_tx_size += selection_result.get_weight(); + total_fees_not_paid = (total_fees_not_paid + selection_result.get_total_fees()) + .ok_or(WalletError::OutputAmountOverflow)?; + if change_amount > Amount::ZERO { - amount_to_be_paid_in_currency_with_fees = (amount_to_be_paid_in_currency_with_fees - + cost_of_change) - .ok_or(WalletError::OutputAmountOverflow)?; + total_fees_not_paid = + (total_fees_not_paid + cost_of_change).ok_or(WalletError::OutputAmountOverflow)?; + + // There can be an small rounding error of computing the fee for each individual input + // and then summing them vs computing the fee of the total size at once. + let new_total_fees_not_paid = current_fee_rate + .compute_fee(total_tx_size + size_of_change) + .map_err(|_| UtxoSelectorError::AmountArithmeticError)? + .into(); + + // Add the difference if any back as change + selection_result = selection_result.add_change( + (total_fees_not_paid - new_total_fees_not_paid).unwrap_or(Amount::ZERO), + )?; } - output_currency_amounts.insert( - pay_fee_with_currency, - (amount_to_be_paid_in_currency_with_fees + selection_result.get_total_fees()) - .ok_or(WalletError::OutputAmountOverflow)?, - ); selected_inputs.insert(pay_fee_with_currency, selection_result); // Check outputs against inputs and create change self.check_outputs_and_add_change( &pay_fee_with_currency, - output_currency_amounts, selected_inputs, change_addresses, db_tx, @@ -403,16 +429,14 @@ impl Account { fn check_outputs_and_add_change( &mut self, pay_fee_with_currency: &Currency, - output_currency_amounts: BTreeMap, selected_inputs: BTreeMap, mut change_addresses: BTreeMap>, db_tx: &mut impl WalletStorageWriteLocked, mut request: SendRequest, ) -> Result { - for currency in output_currency_amounts.keys() { - let currency_result = selected_inputs.get(currency); - let change_amount = currency_result.map_or(Amount::ZERO, |result| result.get_change()); - let fees = currency_result.map_or(Amount::ZERO, |result| result.get_total_fees()); + for (currency, currency_result) in selected_inputs.iter() { + let change_amount = currency_result.get_change(); + let fees = currency_result.get_total_fees(); if fees > Amount::ZERO { request.add_fee(*pay_fee_with_currency, fees)?; @@ -457,18 +481,17 @@ impl Account { let input_size = serialization::Encode::encoded_size(&tx_input); let inp_sig_size = input_signature_size(&txo, Some(self))?; + let weight = input_size + inp_sig_size; let fee = fee_rates .current_fee_rate - .compute_fee(input_size + inp_sig_size) + .compute_fee(weight) .map_err(|_| UtxoSelectorError::AmountArithmeticError)?; let consolidate_fee = fee_rates .consolidate_fee_rate - .compute_fee(input_size + inp_sig_size) + .compute_fee(weight) .map_err(|_| UtxoSelectorError::AmountArithmeticError)?; - // TODO-#1120: calculate weight from the size of the input - let weight = 0; let out_group = OutputGroup::new((tx_input, txo), fee.into(), consolidate_fee.into(), weight)?; @@ -519,11 +542,13 @@ impl Account { None, )?; - let input_fees = grouped_inputs + let total_input_sizes = grouped_inputs .values() - .map(|input_amounts| input_amounts.fee) - .sum::>() - .ok_or(WalletError::OutputAmountOverflow)?; + .map(|input_amounts| input_amounts.total_input_size) + .sum::(); + let input_fees = *current_fee_rate + .compute_fee(total_input_sizes) + .map_err(|_| UtxoSelectorError::AmountArithmeticError)?; let coin_input = grouped_inputs.remove(&Currency::Coin).ok_or(WalletError::NoUtxos)?; @@ -550,14 +575,13 @@ impl Account { ); outputs.push(coin_output); - let tx_fee: Amount = current_fee_rate - .compute_fee(tx_size_with_outputs(outputs.as_slice())) + let tx_size = tx_size_with_outputs(outputs.as_slice(), request.inputs().len()); + let total_fee: Amount = current_fee_rate + .compute_fee(tx_size + total_input_sizes) .map_err(|_| UtxoSelectorError::AmountArithmeticError)? .into(); outputs.pop(); - let total_fee = (tx_fee + input_fees).ok_or(WalletError::OutputAmountOverflow)?; - let coin_output = TxOutput::Transfer( OutputValue::Coin( (coin_input.amount - total_fee) @@ -599,7 +623,7 @@ impl Account { let input_size = serialization::Encode::encoded_size(&tx_input); let total_fee: Amount = current_fee_rate .compute_fee( - tx_size_with_outputs(outputs.as_slice()) + tx_size_with_outputs(outputs.as_slice(), 1) + input_size + input_signature_size_from_destination( &delegation_data.destination, @@ -709,7 +733,7 @@ impl Account { current_fee_rate .compute_fee( - tx_size_with_outputs(outputs.as_slice()) + tx_size_with_outputs(outputs.as_slice(), 1) + input_signature_size_from_destination( &pool_data.decommission_key, Some(self), @@ -801,7 +825,7 @@ impl Account { let outputs = vec![output]; let network_fee: Amount = current_fee_rate .compute_fee( - tx_size_with_outputs(outputs.as_slice()) + tx_size_with_outputs(outputs.as_slice(), 1) + input_signature_size_from_destination( &delegation_data.destination, Some(self), @@ -2371,6 +2395,8 @@ struct PreselectedInputAmounts { pub fee: Amount, // Burn requirement introduced by an input pub burn: Amount, + // the total encoded size for the input along with the signature for it + pub total_input_size: usize, } impl Account { @@ -2488,9 +2514,16 @@ fn group_preselected_inputs( let mut update_preselected_inputs = |currency: Currency, amount: Amount, fee: Amount, burn: Amount| -> WalletResult<()> { + let total_input_size = input_size + inp_sig_size; + match preselected_inputs.entry(currency) { Entry::Vacant(entry) => { - entry.insert(PreselectedInputAmounts { amount, fee, burn }); + entry.insert(PreselectedInputAmounts { + amount, + fee, + burn, + total_input_size, + }); } Entry::Occupied(mut entry) => { let existing = entry.get_mut(); @@ -2500,6 +2533,7 @@ fn group_preselected_inputs( (existing.fee + fee).ok_or(WalletError::OutputAmountOverflow)?; existing.burn = (existing.burn + burn).ok_or(WalletError::OutputAmountOverflow)?; + existing.total_input_size += total_input_size; } } Ok(()) @@ -2550,9 +2584,14 @@ fn group_preselected_inputs( update_preselected_inputs( Currency::Token(*token_id), *amount, - (*fee + chain_config.token_supply_change_fee(block_height)) - .ok_or(WalletError::OutputAmountOverflow)?, + *fee, + Amount::ZERO, + )?; + update_preselected_inputs( + Currency::Coin, Amount::ZERO, + Amount::ZERO, + chain_config.token_supply_change_fee(block_height), )?; } AccountCommand::LockTokenSupply(token_id) @@ -2560,9 +2599,14 @@ fn group_preselected_inputs( update_preselected_inputs( Currency::Token(*token_id), Amount::ZERO, - (*fee + chain_config.token_supply_change_fee(block_height)) - .ok_or(WalletError::OutputAmountOverflow)?, + *fee, + Amount::ZERO, + )?; + update_preselected_inputs( + Currency::Coin, + Amount::ZERO, Amount::ZERO, + chain_config.token_supply_change_fee(block_height), )?; } AccountCommand::FreezeToken(token_id, _) @@ -2570,28 +2614,43 @@ fn group_preselected_inputs( update_preselected_inputs( Currency::Token(*token_id), Amount::ZERO, - (*fee + chain_config.token_freeze_fee(block_height)) - .ok_or(WalletError::OutputAmountOverflow)?, + *fee, + Amount::ZERO, + )?; + update_preselected_inputs( + Currency::Coin, Amount::ZERO, + Amount::ZERO, + chain_config.token_freeze_fee(block_height), )?; } AccountCommand::ChangeTokenAuthority(token_id, _) => { update_preselected_inputs( Currency::Token(*token_id), Amount::ZERO, - (*fee + chain_config.token_change_authority_fee(block_height)) - .ok_or(WalletError::OutputAmountOverflow)?, + *fee, Amount::ZERO, )?; + update_preselected_inputs( + Currency::Coin, + Amount::ZERO, + Amount::ZERO, + chain_config.token_change_authority_fee(block_height), + )?; } AccountCommand::ChangeTokenMetadataUri(token_id, _) => { update_preselected_inputs( Currency::Token(*token_id), Amount::ZERO, - (*fee + chain_config.token_change_metadata_uri_fee()) - .ok_or(WalletError::OutputAmountOverflow)?, + *fee, Amount::ZERO, )?; + update_preselected_inputs( + Currency::Coin, + Amount::ZERO, + Amount::ZERO, + chain_config.token_change_metadata_uri_fee(), + )?; } AccountCommand::ConcludeOrder(order_id) => { handle_conclude_order( @@ -2713,8 +2772,9 @@ fn handle_conclude_order( /// Returns the Amounts for Coin output and Token output fn coin_and_token_output_change_fees( feerate: mempool::FeeRate, + currency: &Currency, destination: Option<&Address>, -) -> WalletResult<(Amount, Amount)> { +) -> WalletResult<(usize, Amount)> { let destination = if let Some(addr) = destination { addr.as_object().clone() } else { @@ -2722,32 +2782,31 @@ fn coin_and_token_output_change_fees( Destination::PublicKeyHash(pub_key_hash) }; - let coin_output = TxOutput::Transfer(OutputValue::Coin(Amount::MAX), destination.clone()); - let token_output = TxOutput::Transfer( - OutputValue::TokenV1( - TokenId::zero(), - // TODO: as the amount is compact there is an edge case where those extra few bytes of - // size can cause the output fee to be go over the available amount of coins thus not - // including a change output, and losing money for the user - // e.g. available money X and need to transfer Y and the difference Z = X - Y is just - // enough the make an output with change but the amount having single byte encoding - // but by using Amount::MAX the algorithm thinks that the change output will cost more - // than Z and it will not create a change output - Amount::MAX, + let output = match currency { + Currency::Coin => TxOutput::Transfer(OutputValue::Coin(Amount::MAX), destination.clone()), + Currency::Token(_) => TxOutput::Transfer( + OutputValue::TokenV1( + TokenId::zero(), + // TODO: as the amount is compact there is an edge case where those extra few bytes of + // size can cause the output fee to be go over the available amount of coins thus not + // including a change output, and losing money for the user + // e.g. available money X and need to transfer Y and the difference Z = X - Y is just + // enough the make an output with change but the amount having single byte encoding + // but by using Amount::MAX the algorithm thinks that the change output will cost more + // than Z and it will not create a change output + Amount::MAX, + ), + destination, ), - destination, - ); + }; - Ok(( - feerate - .compute_fee(serialization::Encode::encoded_size(&coin_output)) - .map_err(|_| UtxoSelectorError::AmountArithmeticError)? - .into(), - feerate - .compute_fee(serialization::Encode::encoded_size(&token_output)) - .map_err(|_| UtxoSelectorError::AmountArithmeticError)? - .into(), - )) + let size = serialization::Encode::encoded_size(&output); + let fee = feerate + .compute_fee(size) + .map_err(|_| UtxoSelectorError::AmountArithmeticError)? + .into(); + + Ok((size, fee)) } #[cfg(test)] diff --git a/wallet/src/account/utxo_selector/mod.rs b/wallet/src/account/utxo_selector/mod.rs index 694ae0db5..db394dd85 100644 --- a/wallet/src/account/utxo_selector/mod.rs +++ b/wallet/src/account/utxo_selector/mod.rs @@ -33,7 +33,7 @@ pub struct SelectionResult { effective_value: Amount, target: Amount, waste: SignedAmount, - weight: u32, + weight: usize, fees: Amount, change: Amount, } @@ -51,6 +51,10 @@ impl SelectionResult { } } + pub fn get_weight(&self) -> usize { + self.weight + } + pub fn get_total_fees(&self) -> Amount { self.fees } @@ -68,6 +72,10 @@ impl SelectionResult { self.outputs } + pub fn num_selected_inputs(&self) -> usize { + self.outputs.len() + } + fn add_input( &mut self, group: &OutputGroup, @@ -200,7 +208,7 @@ fn select_coins_srd( target_value: Amount, rng: &mut impl Rng, change_cost: Amount, - max_weight: u32, + max_weight: usize, pay_fees: PayFee, ) -> Result { let mut result = SelectionResult::new(target_value); @@ -266,7 +274,7 @@ fn knapsack_solver( target_value: Amount, cost_of_change: Amount, rng: &mut impl Rng, - max_weight: u32, + max_weight: usize, pay_fees: PayFee, ) -> Result { let mut result = SelectionResult::new(target_value); @@ -473,7 +481,7 @@ fn select_coins_bnb( mut utxo_pool: Vec, selection_target: Amount, cost_of_change: Amount, - max_weight: u32, + max_weight: usize, pay_fees: PayFee, ) -> Result { let mut curr_value = Amount::ZERO; @@ -666,6 +674,7 @@ pub fn select_coins( pay_fees: PayFee, cost_of_change: Amount, coin_selection_algo: CoinSelectionAlgo, + max_tx_weight: usize, ) -> Result { if selection_target == Amount::ZERO && pay_fees == PayFee::DoNotPayFeeWithThisCurrency { return Ok(SelectionResult::new(selection_target)); @@ -687,9 +696,13 @@ pub fn select_coins( CoinSelectionAlgo::UsePreselected => { select_all_coins(utxo_pool, selection_target, pay_fees, cost_of_change) } - CoinSelectionAlgo::Randomize => { - select_random_coins(utxo_pool, selection_target, cost_of_change, pay_fees) - } + CoinSelectionAlgo::Randomize => select_random_coins( + utxo_pool, + selection_target, + cost_of_change, + pay_fees, + max_tx_weight, + ), } } @@ -698,9 +711,8 @@ fn select_random_coins( selection_target: Amount, cost_of_change: Amount, pay_fees: PayFee, + max_weight: usize, ) -> Result { - // TODO: set some max weight - let max_weight = 999; let mut rng = make_pseudo_rng(); let mut results = vec![]; diff --git a/wallet/src/account/utxo_selector/output_group.rs b/wallet/src/account/utxo_selector/output_group.rs index bfc21ff19..a52329ba0 100644 --- a/wallet/src/account/utxo_selector/output_group.rs +++ b/wallet/src/account/utxo_selector/output_group.rs @@ -36,7 +36,7 @@ pub struct OutputGroup { pub long_term_fee: Amount, /// Total weight of the UTXOs in this group. /// the size in bytes of the UTXOs - pub weight: u32, + pub weight: usize, } /// Should we pay fee with this currency or not in the case we pay the total fees with another @@ -52,7 +52,7 @@ impl OutputGroup { output: (TxInput, TxOutput), fee: Amount, long_term_fee: Amount, - weight: u32, + weight: usize, ) -> Result { let output_value = match &output.1 { TxOutput::Transfer(v, _) diff --git a/wallet/src/wallet/tests.rs b/wallet/src/wallet/tests.rs index 4e9d10e74..839416bf8 100644 --- a/wallet/src/wallet/tests.rs +++ b/wallet/src/wallet/tests.rs @@ -1395,6 +1395,8 @@ fn wallet_list_mainchain_transactions(#[case] seed: Seed) { #[trace] #[case(Seed::from_entropy())] fn wallet_transaction_with_fees(#[case] seed: Seed) { + use crate::destination_getters::{get_tx_output_destination, HtlcSpendingCondition}; + let mut rng = make_seedable_rng(seed); let chain_config = Arc::new(create_mainnet()); @@ -1404,7 +1406,7 @@ fn wallet_transaction_with_fees(#[case] seed: Seed) { assert_eq!(coin_balance, Amount::ZERO); // Generate a new block which sends reward to the wallet - let block1_amount = Amount::from_atoms(rng.gen_range(100000..1000000)); + let block1_amount = Amount::from_atoms(rng.gen_range(10000000..20000000)); let _ = create_block(&chain_config, &mut wallet, vec![], block1_amount, 0); let coin_balance = get_coin_balance(&wallet); @@ -1436,16 +1438,29 @@ fn wallet_transaction_with_fees(#[case] seed: Seed) { } } - let num_outputs = rng.gen_range(1..10); - let amount_to_transfer = Amount::from_atoms( - rng.gen_range(1..=(block1_amount.into_atoms() / num_outputs - NETWORK_FEE)), + // make a transaction with multiple outputs to the same address so we can later sweep them all + // again + let num_outputs = rng.gen_range(1..1000); + let amount_to_transfer_per_output = Amount::from_atoms( + rng.gen_range(1..=((block1_amount.into_atoms() - NETWORK_FEE) / num_outputs / 2)), ); + let amount_to_transfer = (amount_to_transfer_per_output * num_outputs).unwrap(); + let address = wallet.get_new_address(DEFAULT_ACCOUNT_INDEX).unwrap().1.into_object(); let outputs: Vec = (0..num_outputs) - .map(|_| gen_random_transfer(&mut rng, amount_to_transfer)) + .map(|_| { + TxOutput::Transfer( + OutputValue::Coin(amount_to_transfer_per_output), + address.clone(), + ) + }) + // also add some random outputs to test out different TxOutput types + .chain( + (0..num_outputs).map(|_| gen_random_transfer(&mut rng, amount_to_transfer_per_output)), + ) .collect(); - let feerate = FeeRate::from_amount_per_kb(Amount::from_atoms(1000)); + let feerate = FeeRate::from_amount_per_kb(Amount::from_atoms(rng.gen_range(1..1000))); let transaction = wallet .create_transaction_to_addresses( DEFAULT_ACCOUNT_INDEX, @@ -1459,7 +1474,12 @@ fn wallet_transaction_with_fees(#[case] seed: Seed) { .unwrap(); let tx_size = serialization::Encode::encoded_size(&transaction); - let fee = feerate.compute_fee(tx_size).unwrap(); + + // 16 bytes (u128) is the max tx size diffference in the estimation, + // because of the compact encoding for the change amount which is unknown beforhand + let max_size_diff = std::mem::size_of::(); + let min_fee = feerate.compute_fee(tx_size).unwrap(); + let max_fee = feerate.compute_fee(tx_size + max_size_diff).unwrap(); // register the successful transaction and check the balance wallet @@ -1469,8 +1489,80 @@ fn wallet_transaction_with_fees(#[case] seed: Seed) { &WalletEventsNoOp, ) .unwrap(); - let coin_balance = get_coin_balance_with_inactive(&wallet); - assert!(coin_balance <= ((block1_amount - amount_to_transfer).unwrap() - fee.into()).unwrap()); + let coin_balance1 = get_coin_balance_with_inactive(&wallet); + let expected_balance_max = + ((block1_amount - amount_to_transfer).unwrap() - min_fee.into()).unwrap(); + let expected_balance_min = + ((block1_amount - amount_to_transfer).unwrap() - max_fee.into()).unwrap(); + + assert!(coin_balance1 >= expected_balance_min); + assert!(coin_balance1 <= expected_balance_max); + + let selected_utxos = wallet + .get_utxos( + DEFAULT_ACCOUNT_INDEX, + UtxoType::Transfer | UtxoType::LockThenTransfer | UtxoType::IssueNft, + UtxoState::Confirmed | UtxoState::Inactive, + WithLocked::Unlocked, + ) + .unwrap() + .into_iter() + .filter(|(_, output)| { + get_tx_output_destination(output, &|_| None, HtlcSpendingCondition::Skip) + .is_some_and(|dest| dest == address) + }) + .collect::>(); + + // make sure we have selected all of the previously created outputs + assert!(selected_utxos.len() >= num_outputs as usize); + + let account1 = wallet.create_next_account(None).unwrap().0; + let address2 = wallet.get_new_address(account1).unwrap().1.into_object(); + let feerate = FeeRate::from_amount_per_kb(Amount::from_atoms(rng.gen_range(1..1000))); + let transaction = wallet + .create_sweep_transaction( + DEFAULT_ACCOUNT_INDEX, + address2, + selected_utxos, + feerate, + TxAdditionalInfo::new(), + ) + .unwrap(); + + let tx_size = serialization::Encode::encoded_size(&transaction); + let exact_fee = feerate.compute_fee(tx_size).unwrap(); + + // register the successful transaction and check the balance + wallet + .add_account_unconfirmed_tx( + DEFAULT_ACCOUNT_INDEX, + transaction.clone(), + &WalletEventsNoOp, + ) + .unwrap(); + let coin_balance2 = get_coin_balance_with_inactive(&wallet); + // sweep pays fees from the transfer amount itself + assert_eq!(coin_balance2, (coin_balance1 - amount_to_transfer).unwrap()); + + // add the tx to the new account and check the balance + wallet + .add_account_unconfirmed_tx(account1, transaction.clone(), &WalletEventsNoOp) + .unwrap(); + + let coin_balance3 = wallet + .get_balance( + account1, + UtxoState::Confirmed | UtxoState::Inactive | UtxoState::InMempool, + WithLocked::Unlocked, + ) + .unwrap() + .get(&Currency::Coin) + .copied() + .unwrap_or(Amount::ZERO); + + // the sweep command should pay exactly the correct fee as it has no change outputs + let expected_balance3 = (amount_to_transfer - exact_fee.into()).unwrap(); + assert_eq!(coin_balance3, expected_balance3); } #[test] diff --git a/wasm-wrappers/src/lib.rs b/wasm-wrappers/src/lib.rs index 39ae366c4..bd545825e 100644 --- a/wasm-wrappers/src/lib.rs +++ b/wasm-wrappers/src/lib.rs @@ -1123,7 +1123,7 @@ pub fn estimate_transaction_size( tx_outputs.push(output); } - let size = tx_size_with_outputs(&tx_outputs); + let size = tx_size_with_outputs(&tx_outputs, input_utxos_destinations.len()); let inputs_size = inputs.len(); let mut total_size = size + inputs_size; From c3ca95842bfbec517e6dc53dc59eac7191e56ff6 Mon Sep 17 00:00:00 2001 From: Boris Oncev Date: Tue, 29 Apr 2025 17:09:45 +0200 Subject: [PATCH 2/4] add tests for max tx weight --- wallet/src/account/utxo_selector/mod.rs | 29 ++++++- wallet/src/account/utxo_selector/tests.rs | 94 ++++++++++++++++++++++- 2 files changed, 121 insertions(+), 2 deletions(-) diff --git a/wallet/src/account/utxo_selector/mod.rs b/wallet/src/account/utxo_selector/mod.rs index db394dd85..328572e05 100644 --- a/wallet/src/account/utxo_selector/mod.rs +++ b/wallet/src/account/utxo_selector/mod.rs @@ -269,7 +269,7 @@ fn select_coins_srd( } /// Original coin selection algorithm as a fallback -fn knapsack_solver( +fn knapsack_solver_impl( groups: &mut Vec, target_value: Amount, cost_of_change: Amount, @@ -388,6 +388,33 @@ fn knapsack_solver( Ok(result) } +fn knapsack_solver( + groups: &mut Vec, + target_value: Amount, + cost_of_change: Amount, + rng: &mut impl Rng, + max_weight: usize, + pay_fees: PayFee, +) -> Result { + let result = knapsack_solver_impl( + groups, + target_value, + cost_of_change, + rng, + max_weight, + pay_fees, + ); + + if let Ok(result) = &result { + ensure!( + result.weight <= max_weight, + UtxoSelectorError::MaxWeightExceeded + ) + } + + result +} + /// Find a subset of the OutputGroups that is at least as large as, but as close as possible to, the /// target amount; solve subset sum. fn approximate_best_subset( diff --git a/wallet/src/account/utxo_selector/tests.rs b/wallet/src/account/utxo_selector/tests.rs index c6cdc628f..f1fdcfe6a 100644 --- a/wallet/src/account/utxo_selector/tests.rs +++ b/wallet/src/account/utxo_selector/tests.rs @@ -37,7 +37,7 @@ fn add_output(value: Amount, groups: &mut Vec) { value, fee: Amount::ZERO, long_term_fee: Amount::ZERO, - weight: 0, + weight: 1, }); } @@ -97,6 +97,37 @@ fn test_knapsack_solver_not_enough(#[case] seed: Seed) { assert_eq!(error, UtxoSelectorError::NoSolutionFound); } +#[rstest] +#[trace] +#[case(Seed::from_entropy())] +fn test_knapsack_solver_max_weight(#[case] seed: Seed) { + let mut rng = make_seedable_rng(seed); + + let mut groups = vec![]; + let mut target_value = Amount::ZERO; + let num_inputs = rng.gen_range(1..100); + for _ in 0..num_inputs { + let value = Amount::from_atoms(rng.gen_range(1..100)); + add_output(value, &mut groups); + target_value = (target_value + value).expect("can't overflow"); + } + + let cost_of_change = Amount::ZERO; + // make sure there cannot be a solution because it requires all inputs but the max weight does + // not allow to select them all + let max_weight = num_inputs - 1; + let result = knapsack_solver( + &mut groups, + target_value, + cost_of_change, + &mut rng, + max_weight, + PayFee::PayFeeWithThisCurrency, + ); + + assert_eq!(result.unwrap_err(), UtxoSelectorError::MaxWeightExceeded); +} + #[rstest] #[trace] #[case(Seed::from_entropy())] @@ -265,6 +296,36 @@ fn test_bnb_solver_exact_solution(#[case] seed: Seed) { assert_eq!(result.effective_value, target_value); } +#[rstest] +#[trace] +#[case(Seed::from_entropy())] +fn test_bnb_solver_max_weight(#[case] seed: Seed) { + let mut rng = make_seedable_rng(seed); + + let mut groups = vec![]; + let mut target_value = Amount::ZERO; + let num_inputs = rng.gen_range(1..100); + for _ in 0..num_inputs { + let value = Amount::from_atoms(rng.gen_range(1..100)); + add_output(value, &mut groups); + target_value = (target_value + value).expect("can't overflow"); + } + + let cost_of_change = Amount::ZERO; + // make sure there cannot be a solution because it requires all inputs but the max weight does + // not allow to select them all + let max_weight = num_inputs - 1; + let result = select_coins_bnb( + groups, + target_value, + cost_of_change, + max_weight, + PayFee::PayFeeWithThisCurrency, + ); + + assert_eq!(result.unwrap_err(), UtxoSelectorError::MaxWeightExceeded); +} + #[rstest] #[trace] #[case(Seed::from_entropy())] @@ -358,3 +419,34 @@ fn test_srd_solver_find_solution(#[case] seed: Seed) { assert!(result.effective_value >= target_value); } + +#[rstest] +#[trace] +#[case(Seed::from_entropy())] +fn test_srd_solver_max_weight(#[case] seed: Seed) { + let mut rng = make_seedable_rng(seed); + + let mut groups = vec![]; + let mut target_value = Amount::ZERO; + let num_inputs = rng.gen_range(1..100); + for _ in 0..num_inputs { + let value = Amount::from_atoms(rng.gen_range(1..100)); + add_output(value, &mut groups); + target_value = (target_value + value).expect("can't overflow"); + } + + let cost_of_change = Amount::ZERO; + // make sure there cannot be a solution because it requires all inputs but the max weight does + // not allow to select them all + let max_weight = num_inputs - 1; + let result = select_coins_srd( + &groups, + target_value, + &mut rng, + cost_of_change, + max_weight, + PayFee::PayFeeWithThisCurrency, + ); + + assert_eq!(result.unwrap_err(), UtxoSelectorError::MaxWeightExceeded); +} From 5a4271eb8f7efa8c89b044c05231eaae947c27bf Mon Sep 17 00:00:00 2001 From: Boris Oncev Date: Wed, 30 Apr 2025 16:25:57 +0200 Subject: [PATCH 3/4] fix comments --- common/src/size_estimation/mod.rs | 26 ++-- wallet/src/account/mod.rs | 190 ++++++++++++++++-------------- wallet/src/wallet/tests.rs | 16 ++- wasm-wrappers/src/lib.rs | 9 +- 4 files changed, 138 insertions(+), 103 deletions(-) diff --git a/common/src/size_estimation/mod.rs b/common/src/size_estimation/mod.rs index 27d457ac9..466b2a233 100644 --- a/common/src/size_estimation/mod.rs +++ b/common/src/size_estimation/mod.rs @@ -192,9 +192,9 @@ pub fn input_signature_size_from_destination( } } -/// Return the encoded size for a SignedTransaction with specified outputs and empty inputs and -/// signatures -pub fn tx_size_with_outputs(outputs: &[TxOutput], num_inputs: usize) -> usize { +/// Return the encoded size for a SignedTransaction also accounting for the compact encoding of the +/// vectors for the specified number of inputs and outputs +pub fn tx_size_with_num_inputs_and_outputs(num_outputs: usize, num_inputs: usize) -> usize { #[derive(Encode)] struct CompactSize { #[codec(compact)] @@ -202,17 +202,27 @@ pub fn tx_size_with_outputs(outputs: &[TxOutput], num_inputs: usize) -> usize { } let tx = SignedTransaction::new( - Transaction::new(1, vec![], outputs.into()).expect("should not fail"), + Transaction::new(1, vec![], vec![]).expect("should not fail"), vec![], ) .expect("should not fail"); let size = serialization::Encode::encoded_size(&tx); - let compact_size_diff = serialization::Encode::encoded_size(&CompactSize { - value: num_inputs as u64, - }) - serialization::Encode::encoded_size(&CompactSize { value: 0 }); + let input_compact_size_diff = + serialization::Encode::encoded_size(&CompactSize { + value: num_inputs as u64, + }) - serialization::Encode::encoded_size(&CompactSize { value: 0 }); - size + (compact_size_diff * 2) // 2 for number of inputs and number of input signatures + let output_compact_size_diff = + serialization::Encode::encoded_size(&CompactSize { + value: num_outputs as u64, + }) - serialization::Encode::encoded_size(&CompactSize { value: 0 }); + + size + output_compact_size_diff + (input_compact_size_diff * 2) // 2 for number of inputs and number of input signatures +} + +pub fn outputs_encoded_size(outputs: &[TxOutput]) -> usize { + outputs.iter().map(serialization::Encode::encoded_size).sum() } fn get_tx_output_destination(txo: &TxOutput) -> Option<&Destination> { diff --git a/wallet/src/account/mod.rs b/wallet/src/account/mod.rs index 1ffc38619..655ef099f 100644 --- a/wallet/src/account/mod.rs +++ b/wallet/src/account/mod.rs @@ -29,8 +29,8 @@ use common::chain::{ use common::primitives::id::WithId; use common::primitives::{Idable, H256}; use common::size_estimation::{ - input_signature_size, input_signature_size_from_destination, tx_size_with_outputs, - DestinationInfoProvider, + input_signature_size, input_signature_size_from_destination, outputs_encoded_size, + tx_size_with_num_inputs_and_outputs, DestinationInfoProvider, }; use common::Uint256; use crypto::key::hdkd::child_number::ChildNumber; @@ -297,7 +297,7 @@ impl Account { (inputs.amount, inputs.fee, inputs.total_input_size) }); - let (size_of_change, cost_of_change) = coin_and_token_output_change_fees( + let (size_of_change, cost_of_change) = output_change_size_and_fees( current_fee_rate, currency, change_addresses.get(currency), @@ -341,7 +341,9 @@ impl Account { .sum::() + request.inputs().len(); - let tx_outputs_size = tx_size_with_outputs(request.outputs(), num_inputs); + let tx_outputs_size = + tx_size_with_num_inputs_and_outputs(request.outputs().len(), num_inputs) + + outputs_encoded_size(request.outputs()); let tx_outputs_fee: Amount = current_fee_rate .compute_fee(tx_outputs_size) .map_err(|_| UtxoSelectorError::AmountArithmeticError)? @@ -373,7 +375,7 @@ impl Account { + total_fees_not_paid) .ok_or(WalletError::OutputAmountOverflow)?; - let (size_of_change, cost_of_change) = coin_and_token_output_change_fees( + let (size_of_change, cost_of_change) = output_change_size_and_fees( current_fee_rate, &pay_fee_with_currency, change_addresses.get(&pay_fee_with_currency), @@ -575,7 +577,8 @@ impl Account { ); outputs.push(coin_output); - let tx_size = tx_size_with_outputs(outputs.as_slice(), request.inputs().len()); + let tx_size = tx_size_with_num_inputs_and_outputs(outputs.len(), request.inputs().len()) + + outputs_encoded_size(outputs.as_slice()); let total_fee: Amount = current_fee_rate .compute_fee(tx_size + total_input_sizes) .map_err(|_| UtxoSelectorError::AmountArithmeticError)? @@ -623,7 +626,8 @@ impl Account { let input_size = serialization::Encode::encoded_size(&tx_input); let total_fee: Amount = current_fee_rate .compute_fee( - tx_size_with_outputs(outputs.as_slice(), 1) + tx_size_with_num_inputs_and_outputs(outputs.len(), 1) + + outputs_encoded_size(outputs.as_slice()) + input_size + input_signature_size_from_destination( &delegation_data.destination, @@ -733,7 +737,8 @@ impl Account { current_fee_rate .compute_fee( - tx_size_with_outputs(outputs.as_slice(), 1) + tx_size_with_num_inputs_and_outputs(outputs.len(), 1) + + outputs_encoded_size(outputs.as_slice()) + input_signature_size_from_destination( &pool_data.decommission_key, Some(self), @@ -825,7 +830,8 @@ impl Account { let outputs = vec![output]; let network_fee: Amount = current_fee_rate .compute_fee( - tx_size_with_outputs(outputs.as_slice(), 1) + tx_size_with_num_inputs_and_outputs(outputs.len(), 1) + + outputs_encoded_size(outputs.as_slice()) + input_signature_size_from_destination( &delegation_data.destination, Some(self), @@ -2399,6 +2405,26 @@ struct PreselectedInputAmounts { pub total_input_size: usize, } +impl PreselectedInputAmounts { + fn new_with_amount(amount: Amount) -> Self { + Self { + amount, + fee: Amount::ZERO, + burn: Amount::ZERO, + total_input_size: 0, + } + } + + fn new_with_burn(burn: Amount) -> Self { + Self { + amount: Amount::ZERO, + fee: Amount::ZERO, + burn, + total_input_size: 0, + } + } +} + impl Account { fn get_vrf_public_key( &mut self, @@ -2507,37 +2533,58 @@ fn group_preselected_inputs( { let input_size = serialization::Encode::encoded_size(&input); let inp_sig_size = input_signature_size_from_destination(destination, dest_info_provider)?; + let total_input_size = input_size + inp_sig_size; let fee = current_fee_rate - .compute_fee(input_size + inp_sig_size) - .map_err(|_| UtxoSelectorError::AmountArithmeticError)?; + .compute_fee(total_input_size) + .map_err(|_| UtxoSelectorError::AmountArithmeticError)? + .into(); - let mut update_preselected_inputs = - |currency: Currency, amount: Amount, fee: Amount, burn: Amount| -> WalletResult<()> { - let total_input_size = input_size + inp_sig_size; - - match preselected_inputs.entry(currency) { - Entry::Vacant(entry) => { - entry.insert(PreselectedInputAmounts { - amount, - fee, - burn, - total_input_size, - }); - } - Entry::Occupied(mut entry) => { - let existing = entry.get_mut(); - existing.amount = - (existing.amount + amount).ok_or(WalletError::OutputAmountOverflow)?; - existing.fee = - (existing.fee + fee).ok_or(WalletError::OutputAmountOverflow)?; - existing.burn = - (existing.burn + burn).ok_or(WalletError::OutputAmountOverflow)?; - existing.total_input_size += total_input_size; - } + // increase fee and total_input_size, this is same for all inputs + match preselected_inputs.entry(Currency::Coin) { + Entry::Vacant(entry) => { + entry.insert(PreselectedInputAmounts { + amount: Amount::ZERO, + fee, + burn: Amount::ZERO, + total_input_size, + }); + } + Entry::Occupied(mut entry) => { + let existing = entry.get_mut(); + existing.fee = (existing.fee + fee).ok_or(WalletError::OutputAmountOverflow)?; + existing.total_input_size += total_input_size; + } + }; + + // only increase amount and burn according to the specific input type + let mut update_preselected_inputs = |currency: Currency, + amount: Amount, + burn_currency: Currency, + burn: Amount| + -> WalletResult<()> { + match preselected_inputs.entry(currency) { + Entry::Vacant(entry) => { + entry.insert(PreselectedInputAmounts::new_with_amount(amount)); } - Ok(()) - }; + Entry::Occupied(mut entry) => { + let existing = entry.get_mut(); + existing.amount = + (existing.amount + amount).ok_or(WalletError::OutputAmountOverflow)?; + } + } + match preselected_inputs.entry(burn_currency) { + Entry::Vacant(entry) => { + entry.insert(PreselectedInputAmounts::new_with_burn(burn)); + } + Entry::Occupied(mut entry) => { + let existing = entry.get_mut(); + existing.burn = + (existing.burn + burn).ok_or(WalletError::OutputAmountOverflow)?; + } + } + Ok(()) + }; match input { TxInput::Utxo(_) => { @@ -2572,11 +2619,16 @@ fn group_preselected_inputs( ))) } }; - update_preselected_inputs(currency, value, *fee, Amount::ZERO)?; + update_preselected_inputs(currency, value, Currency::Coin, Amount::ZERO)?; } TxInput::Account(outpoint) => match outpoint.account() { AccountSpending::DelegationBalance(_, amount) => { - update_preselected_inputs(Currency::Coin, *amount, *fee, Amount::ZERO)?; + update_preselected_inputs( + Currency::Coin, + *amount, + Currency::Coin, + Amount::ZERO, + )?; } }, TxInput::AccountCommand(_, op) => match op { @@ -2584,13 +2636,7 @@ fn group_preselected_inputs( update_preselected_inputs( Currency::Token(*token_id), *amount, - *fee, - Amount::ZERO, - )?; - update_preselected_inputs( Currency::Coin, - Amount::ZERO, - Amount::ZERO, chain_config.token_supply_change_fee(block_height), )?; } @@ -2599,13 +2645,7 @@ fn group_preselected_inputs( update_preselected_inputs( Currency::Token(*token_id), Amount::ZERO, - *fee, - Amount::ZERO, - )?; - update_preselected_inputs( Currency::Coin, - Amount::ZERO, - Amount::ZERO, chain_config.token_supply_change_fee(block_height), )?; } @@ -2614,13 +2654,7 @@ fn group_preselected_inputs( update_preselected_inputs( Currency::Token(*token_id), Amount::ZERO, - *fee, - Amount::ZERO, - )?; - update_preselected_inputs( Currency::Coin, - Amount::ZERO, - Amount::ZERO, chain_config.token_freeze_fee(block_height), )?; } @@ -2628,13 +2662,7 @@ fn group_preselected_inputs( update_preselected_inputs( Currency::Token(*token_id), Amount::ZERO, - *fee, - Amount::ZERO, - )?; - update_preselected_inputs( Currency::Coin, - Amount::ZERO, - Amount::ZERO, chain_config.token_change_authority_fee(block_height), )?; } @@ -2642,13 +2670,7 @@ fn group_preselected_inputs( update_preselected_inputs( Currency::Token(*token_id), Amount::ZERO, - *fee, - Amount::ZERO, - )?; - update_preselected_inputs( Currency::Coin, - Amount::ZERO, - Amount::ZERO, chain_config.token_change_metadata_uri_fee(), )?; } @@ -2656,7 +2678,6 @@ fn group_preselected_inputs( handle_conclude_order( *order_id, order_info.as_ref(), - *fee, &mut update_preselected_inputs, )?; } @@ -2665,7 +2686,6 @@ fn group_preselected_inputs( *order_id, *fill_amount_in_ask_currency, order_info.as_ref(), - *fee, &mut update_preselected_inputs, OrdersVersion::V0, )?; @@ -2677,7 +2697,6 @@ fn group_preselected_inputs( *id, *fill_amount_in_ask_currency, order_info.as_ref(), - *fee, &mut update_preselected_inputs, OrdersVersion::V1, )?; @@ -2686,13 +2705,10 @@ fn group_preselected_inputs( handle_conclude_order( *order_id, order_info.as_ref(), - *fee, &mut update_preselected_inputs, )?; } - OrderAccountCommand::FreezeOrder(_) => { - update_preselected_inputs(Currency::Coin, Amount::ZERO, *fee, Amount::ZERO)?; - } + OrderAccountCommand::FreezeOrder(_) => {} }, } } @@ -2703,8 +2719,7 @@ fn handle_fill_order_op( order_id: OrderId, fill_amount_in_ask_currency: Amount, order_info: Option<&BTreeMap>, - fee: Amount, - update_preselected_inputs: &mut impl FnMut(Currency, Amount, Amount, Amount) -> WalletResult<()>, + update_preselected_inputs: &mut impl FnMut(Currency, Amount, Currency, Amount) -> WalletResult<()>, version: OrdersVersion, ) -> WalletResult<()> { let order_info = order_info @@ -2727,23 +2742,21 @@ fn handle_fill_order_op( .ok_or(WalletError::CalculateOrderFilledAmountFailed(order_id))?; let given_currency = Currency::from_rpc_output_value(&order_info.initially_given); - update_preselected_inputs(given_currency, filled_amount, fee, Amount::ZERO)?; - let asked_currency = Currency::from_rpc_output_value(&order_info.initially_asked); update_preselected_inputs( + given_currency, + filled_amount, asked_currency, - Amount::ZERO, - Amount::ZERO, fill_amount_in_ask_currency, )?; + Ok(()) } fn handle_conclude_order( order_id: OrderId, order_info: Option<&BTreeMap>, - fee: Amount, - update_preselected_inputs: &mut impl FnMut(Currency, Amount, Amount, Amount) -> WalletResult<()>, + update_preselected_inputs: &mut impl FnMut(Currency, Amount, Currency, Amount) -> WalletResult<()>, ) -> WalletResult<()> { let order_info = order_info .as_ref() @@ -2754,23 +2767,20 @@ fn handle_conclude_order( update_preselected_inputs( given_currency, order_info.give_balance, - Amount::ZERO, + Currency::Coin, Amount::ZERO, )?; let asked_currency = Currency::from_rpc_output_value(&order_info.initially_asked); let filled_amount = (order_info.initially_asked.amount() - order_info.ask_balance) .ok_or(WalletError::OutputAmountOverflow)?; - update_preselected_inputs(asked_currency, filled_amount, Amount::ZERO, Amount::ZERO)?; + update_preselected_inputs(asked_currency, filled_amount, Currency::Coin, Amount::ZERO)?; - // add fee - update_preselected_inputs(Currency::Coin, Amount::ZERO, fee, Amount::ZERO)?; Ok(()) } -/// Calculate the amount of fee that needs to be paid to add a change output -/// Returns the Amounts for Coin output and Token output -fn coin_and_token_output_change_fees( +/// Calculate the encoded size and fee that needs to be paid to add a change output +fn output_change_size_and_fees( feerate: mempool::FeeRate, currency: &Currency, destination: Option<&Address>, diff --git a/wallet/src/wallet/tests.rs b/wallet/src/wallet/tests.rs index 839416bf8..bf9230553 100644 --- a/wallet/src/wallet/tests.rs +++ b/wallet/src/wallet/tests.rs @@ -1391,10 +1391,20 @@ fn wallet_list_mainchain_transactions(#[case] seed: Seed) { assert!(txs.iter().any(|info| info.id == spend_from_tx_id)); } +// Check 3 scenarios for fee calculation by the wallet +// 1. give X amount of coins to the wallet +// 2. Try to create a transaction where the feerate is so high that the UTXO will need to pay more +// fee than it has so it will return an NoUTXOs error +// 3. Successfuly create a TX with many outputs and make sure the fee paid is within the range of +// [0-16] bytes * feerate (16 bytes is the MAX u128 Amount used by the change output) +// +// 4. Create a sweep TX using the previously created many outputs. The sweep command does not +// create any change output so check that the fee paid is exactly the correct one based on the +// final size of the transaction. #[rstest] #[trace] #[case(Seed::from_entropy())] -fn wallet_transaction_with_fees(#[case] seed: Seed) { +fn wallet_transactions_with_fees(#[case] seed: Seed) { use crate::destination_getters::{get_tx_output_destination, HtlcSpendingCondition}; let mut rng = make_seedable_rng(seed); @@ -1406,7 +1416,7 @@ fn wallet_transaction_with_fees(#[case] seed: Seed) { assert_eq!(coin_balance, Amount::ZERO); // Generate a new block which sends reward to the wallet - let block1_amount = Amount::from_atoms(rng.gen_range(10000000..20000000)); + let block1_amount = Amount::from_atoms(rng.gen_range(30000000..50000000)); let _ = create_block(&chain_config, &mut wallet, vec![], block1_amount, 0); let coin_balance = get_coin_balance(&wallet); @@ -1442,7 +1452,7 @@ fn wallet_transaction_with_fees(#[case] seed: Seed) { // again let num_outputs = rng.gen_range(1..1000); let amount_to_transfer_per_output = Amount::from_atoms( - rng.gen_range(1..=((block1_amount.into_atoms() - NETWORK_FEE) / num_outputs / 2)), + rng.gen_range(NETWORK_FEE..=((block1_amount.into_atoms() - NETWORK_FEE) / num_outputs / 2)), ); let amount_to_transfer = (amount_to_transfer_per_output * num_outputs).unwrap(); diff --git a/wasm-wrappers/src/lib.rs b/wasm-wrappers/src/lib.rs index bd545825e..ca701452c 100644 --- a/wasm-wrappers/src/lib.rs +++ b/wasm-wrappers/src/lib.rs @@ -72,7 +72,10 @@ use common::{ primitives::{ self, amount::UnsignedIntType, per_thousand::PerThousand, BlockHeight, Id, Idable, H256, }, - size_estimation::{input_signature_size_from_destination, tx_size_with_outputs}, + size_estimation::{ + input_signature_size_from_destination, outputs_encoded_size, + tx_size_with_num_inputs_and_outputs, + }, }; use crypto::key::{ extended::{ExtendedKeyKind, ExtendedPrivateKey, ExtendedPublicKey}, @@ -1123,7 +1126,9 @@ pub fn estimate_transaction_size( tx_outputs.push(output); } - let size = tx_size_with_outputs(&tx_outputs, input_utxos_destinations.len()); + let size = + tx_size_with_num_inputs_and_outputs(tx_outputs.len(), input_utxos_destinations.len()) + + outputs_encoded_size(tx_outputs.as_slice()); let inputs_size = inputs.len(); let mut total_size = size + inputs_size; From be736756f847cb7017bb429fb5cc7e0b9efabece Mon Sep 17 00:00:00 2001 From: Boris Oncev Date: Thu, 8 May 2025 17:35:43 +0200 Subject: [PATCH 4/4] fix comments --- common/src/size_estimation/mod.rs | 58 +++-- common/src/size_estimation/tests.rs | 97 ++++++++ serialization/core/src/lib.rs | 2 +- wallet/src/account/mod.rs | 218 ++++++++---------- wallet/src/account/utxo_selector/mod.rs | 2 +- wallet/src/send_request/mod.rs | 15 +- wallet/src/wallet/tests.rs | 97 +++++--- .../src/synced_controller.rs | 10 +- wasm-wrappers/src/lib.rs | 1 + 9 files changed, 318 insertions(+), 182 deletions(-) create mode 100644 common/src/size_estimation/tests.rs diff --git a/common/src/size_estimation/mod.rs b/common/src/size_estimation/mod.rs index 466b2a233..368613875 100644 --- a/common/src/size_estimation/mod.rs +++ b/common/src/size_estimation/mod.rs @@ -19,7 +19,7 @@ use std::{ }; use crypto::key::{PrivateKey, PublicKey, Signature}; -use serialization::Encode; +use serialization::{CompactLen, Encode}; use crate::chain::{ classic_multisig::ClassicMultisigChallenge, @@ -40,6 +40,8 @@ use crate::chain::{ pub enum SizeEstimationError { #[error("Unsupported input destination")] UnsupportedInputDestination(Destination), + #[error("Attempted to estimate the size of a TX with too many inputs or outputs {0}")] + TooManyElements(usize), } /// Return the encoded size of an input signature. @@ -194,31 +196,40 @@ pub fn input_signature_size_from_destination( /// Return the encoded size for a SignedTransaction also accounting for the compact encoding of the /// vectors for the specified number of inputs and outputs -pub fn tx_size_with_num_inputs_and_outputs(num_outputs: usize, num_inputs: usize) -> usize { - #[derive(Encode)] - struct CompactSize { - #[codec(compact)] - value: u64, +pub fn tx_size_with_num_inputs_and_outputs( + num_outputs: usize, + num_inputs: usize, +) -> Result { + lazy_static::lazy_static! { + static ref EMPTY_SIGNED_TX_SIZE: usize = { + let tx = SignedTransaction::new( + Transaction::new(1, vec![], vec![]).expect("should not fail"), + vec![], + ) + .expect("should not fail"); + serialization::Encode::encoded_size(&tx) + }; + } + lazy_static::lazy_static! { + static ref ZERO_COMPACT_SIZE: usize = { + serialization::Compact::::compact_len(&0) + }; } - let tx = SignedTransaction::new( - Transaction::new(1, vec![], vec![]).expect("should not fail"), - vec![], - ) - .expect("should not fail"); - let size = serialization::Encode::encoded_size(&tx); - - let input_compact_size_diff = - serialization::Encode::encoded_size(&CompactSize { - value: num_inputs as u64, - }) - serialization::Encode::encoded_size(&CompactSize { value: 0 }); + let input_compact_size_diff = serialization::Compact::::compact_len( + &(num_inputs + .try_into() + .map_err(|_| SizeEstimationError::TooManyElements(num_inputs))?), + ) - *ZERO_COMPACT_SIZE; - let output_compact_size_diff = - serialization::Encode::encoded_size(&CompactSize { - value: num_outputs as u64, - }) - serialization::Encode::encoded_size(&CompactSize { value: 0 }); + let output_compact_size_diff = serialization::Compact::::compact_len( + &(num_outputs + .try_into() + .map_err(|_| SizeEstimationError::TooManyElements(num_inputs))?), + ) - *ZERO_COMPACT_SIZE; - size + output_compact_size_diff + (input_compact_size_diff * 2) // 2 for number of inputs and number of input signatures + // 2 for number of inputs and number of input signatures + Ok(*EMPTY_SIGNED_TX_SIZE + output_compact_size_diff + (input_compact_size_diff * 2)) } pub fn outputs_encoded_size(outputs: &[TxOutput]) -> usize { @@ -241,3 +252,6 @@ fn get_tx_output_destination(txo: &TxOutput) -> Option<&Destination> { | TxOutput::CreateOrder(_) => None, } } + +#[cfg(test)] +mod tests; diff --git a/common/src/size_estimation/tests.rs b/common/src/size_estimation/tests.rs new file mode 100644 index 000000000..d9e763ba0 --- /dev/null +++ b/common/src/size_estimation/tests.rs @@ -0,0 +1,97 @@ +// Copyright (c) 2025 RBB S.r.l +// opensource@mintlayer.org +// SPDX-License-Identifier: MIT +// Licensed under the MIT License; +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://github.com/mintlayer/mintlayer-core/blob/master/LICENSE +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use randomness::Rng; +use rstest::rstest; +use test_utils::random::{make_seedable_rng, Seed}; + +use crate::chain::{ + signature::{ + inputsig::{standard_signature::StandardInputSignature, InputWitness}, + sighash::sighashtype::SigHashType, + }, + OutPointSourceId, SignedTransaction, Transaction, TxInput, +}; +use crate::primitives::{Amount, Id}; + +#[rstest] +#[trace] +#[case(Seed::from_entropy())] +fn estimate_tx_size( + #[case] seed: Seed, + #[values(1..64, 64..0x4000, 0x4000..0x4001)] inputs_range: std::ops::Range, + #[values(1..64, 64..0x4000, 0x4000..0x4001)] outputs_range: std::ops::Range, +) { + use crypto::key::{KeyKind, PrivateKey}; + use serialization::Encode; + + use crate::{ + chain::{ + output_value::OutputValue, + signature::inputsig::authorize_pubkey_spend::AuthorizedPublicKeySpend, Destination, + TxOutput, + }, + size_estimation::tx_size_with_num_inputs_and_outputs, + }; + + let mut rng = make_seedable_rng(seed); + + let num_inputs = rng.gen_range(inputs_range); + let inputs = (0..num_inputs) + .map(|_| { + TxInput::from_utxo( + OutPointSourceId::Transaction(Id::random_using(&mut rng)), + rng.gen_range(0..100), + ) + }) + .collect(); + + let num_outputs = rng.gen_range(outputs_range); + let outputs = (0..num_outputs) + .map(|_| { + let destination = Destination::PublicKey( + crypto::key::PrivateKey::new_from_rng(&mut rng, KeyKind::Secp256k1Schnorr).1, + ); + + TxOutput::Transfer( + OutputValue::Coin(Amount::from_atoms(rng.gen_range(1..10000))), + destination, + ) + }) + .collect(); + + let tx = Transaction::new(0, inputs, outputs).unwrap(); + let signatures = (0..num_inputs) + .map(|_| { + let private_key = + PrivateKey::new_from_rng(&mut rng, crypto::key::KeyKind::Secp256k1Schnorr).0; + let signature = private_key.sign_message(&[0; 32], &mut rng).unwrap(); + let raw_signature = AuthorizedPublicKeySpend::new(signature).encode(); + let standard = StandardInputSignature::new(SigHashType::all(), raw_signature); + InputWitness::Standard(standard) + }) + .collect(); + let tx = SignedTransaction::new(tx, signatures).unwrap(); + + let estimated_tx_size = + tx_size_with_num_inputs_and_outputs(num_outputs as usize, num_inputs as usize).unwrap() + + tx.inputs().iter().map(Encode::encoded_size).sum::() + + tx.signatures().iter().map(Encode::encoded_size).sum::() + + tx.outputs().iter().map(Encode::encoded_size).sum::(); + + let expected_tx_size = Encode::encoded_size(&tx); + + assert_eq!(estimated_tx_size, expected_tx_size); +} diff --git a/serialization/core/src/lib.rs b/serialization/core/src/lib.rs index 2207dd722..34bd13b82 100644 --- a/serialization/core/src/lib.rs +++ b/serialization/core/src/lib.rs @@ -17,7 +17,7 @@ // Re-export SCALE traits pub use parity_scale_codec::{ - Codec, Decode, DecodeAll, Encode, EncodeLike, Input, Output, WrapperTypeDecode, + Codec, CompactLen, Decode, DecodeAll, Encode, EncodeLike, Input, Output, WrapperTypeDecode, WrapperTypeEncode, }; diff --git a/wallet/src/account/mod.rs b/wallet/src/account/mod.rs index 655ef099f..6c72e630d 100644 --- a/wallet/src/account/mod.rs +++ b/wallet/src/account/mod.rs @@ -201,10 +201,10 @@ impl Account { #[allow(clippy::too_many_arguments)] fn select_inputs_for_send_request( &mut self, - request: SendRequest, + mut request: SendRequest, input_utxos: SelectedInputs, selection_algo: Option, - change_addresses: BTreeMap>, + mut change_addresses: BTreeMap>, db_tx: &mut impl WalletStorageWriteLocked, median_time: BlockTimestamp, fee_rates: CurrentFeeRate, @@ -236,6 +236,7 @@ impl Account { // update output currency amount with burn requirements of preselected inputs preselected_inputs + .amounts .iter() .filter_map(|(currency, input)| { (input.burn > Amount::ZERO).then_some((currency, input.burn)) @@ -286,21 +287,30 @@ impl Account { let mut total_fees_not_paid = Amount::ZERO; let mut total_tx_size = 0; + let mut get_or_create_change_destination = |currency| -> WalletResult<_> { + match change_addresses.remove(currency) { + None => { + let address = self.key_chain.next_unused_address(db_tx, KeyPurpose::Change)?.1; + Ok(address.into_object()) + } + Some(address) => Ok(address.into_object()), + } + }; + let mut selected_inputs: BTreeMap<_, _> = output_currency_amounts .iter() .map(|(currency, output_amount)| -> WalletResult<_> { let utxos = utxos_by_currency.remove(currency).unwrap_or(vec![]); - let (preselected_amount, preselected_fee, preselected_input_size) = - preselected_inputs - .remove(currency) - .map_or((Amount::ZERO, Amount::ZERO, 0), |inputs| { - (inputs.amount, inputs.fee, inputs.total_input_size) - }); + let preselected_amount = preselected_inputs + .amounts + .remove(currency) + .map_or(Amount::ZERO, |inputs| inputs.amount); + let change_address = get_or_create_change_destination(currency)?; let (size_of_change, cost_of_change) = output_change_size_and_fees( current_fee_rate, currency, - change_addresses.get(currency), + change_address.clone(), )?; let selection_result = select_coins( @@ -314,12 +324,10 @@ impl Account { self.chain_config.max_tx_size_for_mempool(), )?; - total_tx_size += selection_result.get_weight() + preselected_input_size; + total_tx_size += selection_result.get_weight(); total_fees_not_paid = (total_fees_not_paid + selection_result.get_total_fees()) .ok_or(WalletError::OutputAmountOverflow)?; - total_fees_not_paid = (total_fees_not_paid + preselected_fee) - .ok_or(WalletError::OutputAmountOverflow)?; let preselected_change = (preselected_amount - *output_amount).unwrap_or(Amount::ZERO); @@ -329,56 +337,64 @@ impl Account { total_fees_not_paid = (total_fees_not_paid + cost_of_change) .ok_or(WalletError::OutputAmountOverflow)?; total_tx_size += size_of_change; + + let change_output = match currency { + Currency::Coin => make_address_output(change_address, change_amount), + Currency::Token(token_id) => { + make_address_output_token(change_address, change_amount, *token_id) + } + }; + request.add_outputs([change_output]); } Ok((*currency, selection_result)) }) .try_collect()?; + let utxos = utxos_by_currency.remove(&pay_fee_with_currency).unwrap_or(vec![]); + let num_inputs = selected_inputs .values() .map(SelectionResult::num_selected_inputs) .sum::() + request.inputs().len(); + let max_possible_num_inputs = + num_inputs + utxos.iter().map(|group| group.outputs.len()).sum::(); + let num_change_outputs = selected_inputs + .values() + .filter(|selection_result| selection_result.get_change() > Amount::ZERO) + .count(); let tx_outputs_size = - tx_size_with_num_inputs_and_outputs(request.outputs().len(), num_inputs) + // +1 output for the possible change output for pay_fee_with_currency + tx_size_with_num_inputs_and_outputs(request.outputs().len() + num_change_outputs + 1, max_possible_num_inputs)? + outputs_encoded_size(request.outputs()); let tx_outputs_fee: Amount = current_fee_rate .compute_fee(tx_outputs_size) .map_err(|_| UtxoSelectorError::AmountArithmeticError)? .into(); - let utxos = utxos_by_currency.remove(&pay_fee_with_currency).unwrap_or(vec![]); - let (preselected_amount, preselected_fee, preselected_input_size) = preselected_inputs + let preselected_amount = preselected_inputs + .amounts .remove(&pay_fee_with_currency) - .map_or((Amount::ZERO, Amount::ZERO, 0), |inputs| { - (inputs.amount, inputs.fee, inputs.total_input_size) - }); + .map_or(Amount::ZERO, |inputs| inputs.amount); - total_tx_size += tx_outputs_size + preselected_input_size; + total_tx_size += tx_outputs_size + preselected_inputs.total_input_sizes; total_fees_not_paid = (total_fees_not_paid + tx_outputs_fee).ok_or(WalletError::OutputAmountOverflow)?; - total_fees_not_paid = - (total_fees_not_paid + preselected_fee).ok_or(WalletError::OutputAmountOverflow)?; - total_fees_not_paid = preselected_inputs - .values() - .try_fold(total_fees_not_paid, |total, inputs| total + inputs.fee) + total_fees_not_paid = (total_fees_not_paid + preselected_inputs.total_input_fees) .ok_or(WalletError::OutputAmountOverflow)?; - total_tx_size = preselected_inputs.values().fold(total_tx_size, |total, inputs| { - total + inputs.total_input_size - }); - let amount_to_be_paid_in_currency_with_fees = (amount_to_be_paid_in_currency_with_fees + total_fees_not_paid) .ok_or(WalletError::OutputAmountOverflow)?; + let change_address = get_or_create_change_destination(&pay_fee_with_currency)?; let (size_of_change, cost_of_change) = output_change_size_and_fees( current_fee_rate, &pay_fee_with_currency, - change_addresses.get(&pay_fee_with_currency), + change_address.clone(), )?; let selection_result = select_coins( @@ -399,11 +415,25 @@ impl Account { total_fees_not_paid = (total_fees_not_paid + selection_result.get_total_fees()) .ok_or(WalletError::OutputAmountOverflow)?; + // Recalculate the tx size with the known number of inputs and outputs + let num_change_outputs = if selection_result.get_change() > Amount::ZERO { + num_change_outputs + 1 + } else { + num_change_outputs + }; + + let new_tx_outputs_size = tx_size_with_num_inputs_and_outputs( + request.outputs().len() + num_change_outputs, + num_inputs + selection_result.num_selected_inputs(), + )? + outputs_encoded_size(request.outputs()); + + total_tx_size = total_tx_size - tx_outputs_size + new_tx_outputs_size; + if change_amount > Amount::ZERO { total_fees_not_paid = (total_fees_not_paid + cost_of_change).ok_or(WalletError::OutputAmountOverflow)?; - // There can be an small rounding error of computing the fee for each individual input + // There can be a small rounding error of computing the fee for each individual input // and then summing them vs computing the fee of the total size at once. let new_total_fees_not_paid = current_fee_rate .compute_fee(total_tx_size + size_of_change) @@ -414,54 +444,24 @@ impl Account { selection_result = selection_result.add_change( (total_fees_not_paid - new_total_fees_not_paid).unwrap_or(Amount::ZERO), )?; + let change_output = match pay_fee_with_currency { + Currency::Coin => make_address_output(change_address.clone(), change_amount), + Currency::Token(token_id) => { + make_address_output_token(change_address.clone(), change_amount, token_id) + } + }; + request.add_outputs([change_output]); + request.add_fee(pay_fee_with_currency, new_total_fees_not_paid)?; + } else { + let new_total_fees_not_paid = current_fee_rate + .compute_fee(total_tx_size) + .map_err(|_| UtxoSelectorError::AmountArithmeticError)? + .into(); + request.add_fee(pay_fee_with_currency, new_total_fees_not_paid)?; } selected_inputs.insert(pay_fee_with_currency, selection_result); - // Check outputs against inputs and create change - self.check_outputs_and_add_change( - &pay_fee_with_currency, - selected_inputs, - change_addresses, - db_tx, - request, - ) - } - - fn check_outputs_and_add_change( - &mut self, - pay_fee_with_currency: &Currency, - selected_inputs: BTreeMap, - mut change_addresses: BTreeMap>, - db_tx: &mut impl WalletStorageWriteLocked, - mut request: SendRequest, - ) -> Result { - for (currency, currency_result) in selected_inputs.iter() { - let change_amount = currency_result.get_change(); - let fees = currency_result.get_total_fees(); - - if fees > Amount::ZERO { - request.add_fee(*pay_fee_with_currency, fees)?; - } - - if change_amount > Amount::ZERO { - let change_address = if let Some(change_address) = change_addresses.remove(currency) - { - change_address - } else { - self.key_chain.next_unused_address(db_tx, KeyPurpose::Change)?.1 - }; - - let change_output = match currency { - Currency::Coin => make_address_output(change_address, change_amount), - Currency::Token(token_id) => { - make_address_output_token(change_address, change_amount, *token_id) - } - }; - request = request.with_outputs([change_output]); - } - } - let selected_inputs = selected_inputs .into_iter() .flat_map(|x| x.1.into_output_pairs()) @@ -544,17 +544,16 @@ impl Account { None, )?; - let total_input_sizes = grouped_inputs - .values() - .map(|input_amounts| input_amounts.total_input_size) - .sum::(); + let total_input_sizes = grouped_inputs.total_input_sizes; let input_fees = *current_fee_rate .compute_fee(total_input_sizes) .map_err(|_| UtxoSelectorError::AmountArithmeticError)?; - let coin_input = grouped_inputs.remove(&Currency::Coin).ok_or(WalletError::NoUtxos)?; + let coin_input = + grouped_inputs.amounts.remove(&Currency::Coin).ok_or(WalletError::NoUtxos)?; let mut outputs = grouped_inputs + .amounts .into_iter() .filter_map(|(currency, input_amounts)| { let value = match currency { @@ -577,7 +576,7 @@ impl Account { ); outputs.push(coin_output); - let tx_size = tx_size_with_num_inputs_and_outputs(outputs.len(), request.inputs().len()) + let tx_size = tx_size_with_num_inputs_and_outputs(outputs.len(), request.inputs().len())? + outputs_encoded_size(outputs.as_slice()); let total_fee: Amount = current_fee_rate .compute_fee(tx_size + total_input_sizes) @@ -626,7 +625,7 @@ impl Account { let input_size = serialization::Encode::encoded_size(&tx_input); let total_fee: Amount = current_fee_rate .compute_fee( - tx_size_with_num_inputs_and_outputs(outputs.len(), 1) + tx_size_with_num_inputs_and_outputs(outputs.len(), 1)? + outputs_encoded_size(outputs.as_slice()) + input_size + input_signature_size_from_destination( @@ -737,7 +736,7 @@ impl Account { current_fee_rate .compute_fee( - tx_size_with_num_inputs_and_outputs(outputs.len(), 1) + tx_size_with_num_inputs_and_outputs(outputs.len(), 1)? + outputs_encoded_size(outputs.as_slice()) + input_signature_size_from_destination( &pool_data.decommission_key, @@ -830,7 +829,7 @@ impl Account { let outputs = vec![output]; let network_fee: Amount = current_fee_rate .compute_fee( - tx_size_with_num_inputs_and_outputs(outputs.len(), 1) + tx_size_with_num_inputs_and_outputs(outputs.len(), 1)? + outputs_encoded_size(outputs.as_slice()) + input_signature_size_from_destination( &delegation_data.destination, @@ -2397,34 +2396,32 @@ impl common::size_estimation::DestinationInfoProvider for A struct PreselectedInputAmounts { // Available amount from input pub amount: Amount, - // Fee requirement introduced by an input - pub fee: Amount, // Burn requirement introduced by an input pub burn: Amount, - // the total encoded size for the input along with the signature for it - pub total_input_size: usize, } impl PreselectedInputAmounts { fn new_with_amount(amount: Amount) -> Self { Self { amount, - fee: Amount::ZERO, burn: Amount::ZERO, - total_input_size: 0, } } fn new_with_burn(burn: Amount) -> Self { Self { amount: Amount::ZERO, - fee: Amount::ZERO, burn, - total_input_size: 0, } } } +struct PreselectedInputs { + amounts: BTreeMap, + total_input_sizes: usize, + total_input_fees: Amount, +} + impl Account { fn get_vrf_public_key( &mut self, @@ -2526,8 +2523,10 @@ fn group_preselected_inputs( block_height: BlockHeight, dest_info_provider: Option<&dyn DestinationInfoProvider>, order_info: Option>, -) -> Result, WalletError> { +) -> Result { let mut preselected_inputs = BTreeMap::new(); + let mut total_input_sizes = 0; + let mut total_input_fees = Amount::ZERO; for (input, destination, utxo) in izip!(request.inputs(), request.destinations(), request.utxos()) { @@ -2541,21 +2540,8 @@ fn group_preselected_inputs( .into(); // increase fee and total_input_size, this is same for all inputs - match preselected_inputs.entry(Currency::Coin) { - Entry::Vacant(entry) => { - entry.insert(PreselectedInputAmounts { - amount: Amount::ZERO, - fee, - burn: Amount::ZERO, - total_input_size, - }); - } - Entry::Occupied(mut entry) => { - let existing = entry.get_mut(); - existing.fee = (existing.fee + fee).ok_or(WalletError::OutputAmountOverflow)?; - existing.total_input_size += total_input_size; - } - }; + total_input_sizes += total_input_size; + total_input_fees = (total_input_fees + fee).ok_or(WalletError::OutputAmountOverflow)?; // only increase amount and burn according to the specific input type let mut update_preselected_inputs = |currency: Currency, @@ -2712,7 +2698,12 @@ fn group_preselected_inputs( }, } } - Ok(preselected_inputs) + + Ok(PreselectedInputs { + amounts: preselected_inputs, + total_input_sizes, + total_input_fees, + }) } fn handle_fill_order_op( @@ -2783,17 +2774,10 @@ fn handle_conclude_order( fn output_change_size_and_fees( feerate: mempool::FeeRate, currency: &Currency, - destination: Option<&Address>, + destination: Destination, ) -> WalletResult<(usize, Amount)> { - let destination = if let Some(addr) = destination { - addr.as_object().clone() - } else { - let pub_key_hash = PublicKeyHash::from_low_u64_ne(0); - Destination::PublicKeyHash(pub_key_hash) - }; - let output = match currency { - Currency::Coin => TxOutput::Transfer(OutputValue::Coin(Amount::MAX), destination.clone()), + Currency::Coin => TxOutput::Transfer(OutputValue::Coin(Amount::MAX), destination), Currency::Token(_) => TxOutput::Transfer( OutputValue::TokenV1( TokenId::zero(), diff --git a/wallet/src/account/utxo_selector/mod.rs b/wallet/src/account/utxo_selector/mod.rs index 328572e05..c5c0ff26a 100644 --- a/wallet/src/account/utxo_selector/mod.rs +++ b/wallet/src/account/utxo_selector/mod.rs @@ -703,7 +703,7 @@ pub fn select_coins( coin_selection_algo: CoinSelectionAlgo, max_tx_weight: usize, ) -> Result { - if selection_target == Amount::ZERO && pay_fees == PayFee::DoNotPayFeeWithThisCurrency { + if selection_target == Amount::ZERO { return Ok(SelectionResult::new(selection_target)); } ensure!(!utxo_pool.is_empty(), UtxoSelectorError::NoUtxos); diff --git a/wallet/src/send_request/mod.rs b/wallet/src/send_request/mod.rs index 98e6f2761..483210048 100644 --- a/wallet/src/send_request/mod.rs +++ b/wallet/src/send_request/mod.rs @@ -57,19 +57,16 @@ pub struct SendRequest { fees: BTreeMap, } -pub fn make_address_output(address: Address, amount: Amount) -> TxOutput { - TxOutput::Transfer(OutputValue::Coin(amount), address.into_object()) +pub fn make_address_output(address: Destination, amount: Amount) -> TxOutput { + TxOutput::Transfer(OutputValue::Coin(amount), address) } pub fn make_address_output_token( - address: Address, + address: Destination, amount: Amount, token_id: TokenId, ) -> TxOutput { - TxOutput::Transfer( - OutputValue::TokenV1(token_id, amount), - address.into_object(), - ) + TxOutput::Transfer(OutputValue::TokenV1(token_id, amount), address) } pub fn make_issue_token_outputs( @@ -297,6 +294,10 @@ impl SendRequest { Ok(self) } + pub fn add_outputs(&mut self, outputs: impl IntoIterator) { + self.outputs.extend(outputs); + } + pub fn with_outputs(mut self, outputs: impl IntoIterator) -> Self { self.outputs.extend(outputs); self diff --git a/wallet/src/wallet/tests.rs b/wallet/src/wallet/tests.rs index bf9230553..9def037b3 100644 --- a/wallet/src/wallet/tests.rs +++ b/wallet/src/wallet/tests.rs @@ -355,7 +355,10 @@ where chain_config.genesis_block_id(), chain_config.genesis_block().timestamp(), ConsensusData::None, - BlockReward::new(vec![make_address_output(address.clone(), reward)]), + BlockReward::new(vec![make_address_output( + address.clone().into_object(), + reward, + )]), ) .unwrap(); @@ -764,7 +767,7 @@ fn wallet_balance_genesis() { ); let genesis_amount = Amount::from_atoms(23456); - let genesis_output = make_address_output(address, genesis_amount); + let genesis_output = make_address_output(address.into_object(), genesis_amount); if address_index.into_u32() == LOOKAHEAD_SIZE { test_balance_from_genesis(chain_type, vec![genesis_output], Amount::ZERO); @@ -854,7 +857,10 @@ fn wallet_balance_block_reward() { block1.get_id().into(), chain_config.genesis_block().timestamp(), ConsensusData::None, - BlockReward::new(vec![make_address_output(address, block2_amount)]), + BlockReward::new(vec![make_address_output( + address.into_object(), + block2_amount, + )]), ) .unwrap(); let block2_id = block2.header().block_id(); @@ -884,7 +890,10 @@ fn wallet_balance_block_reward() { block1.get_id().into(), chain_config.genesis_block().timestamp(), ConsensusData::None, - BlockReward::new(vec![make_address_output(address, block2_amount_new)]), + BlockReward::new(vec![make_address_output( + address.into_object(), + block2_amount_new, + )]), ) .unwrap(); let block2_new_id = block2_new.header().block_id(); @@ -919,7 +928,7 @@ fn wallet_balance_block_transactions() { let transaction1 = Transaction::new( 0, Vec::new(), - vec![make_address_output(address, tx_amount1)], + vec![make_address_output(address.into_object(), tx_amount1)], ) .unwrap(); let signed_transaction1 = SignedTransaction::new(transaction1, Vec::new()).unwrap(); @@ -962,7 +971,7 @@ fn wallet_balance_parent_child_transactions() { let transaction1 = Transaction::new( 0, Vec::new(), - vec![make_address_output(address1, tx_amount1)], + vec![make_address_output(address1.into_object(), tx_amount1)], ) .unwrap(); let transaction_id1 = transaction1.get_id(); @@ -971,7 +980,7 @@ fn wallet_balance_parent_child_transactions() { let transaction2 = Transaction::new( 0, vec![TxInput::from_utxo(OutPointSourceId::Transaction(transaction_id1), 0)], - vec![make_address_output(address2, tx_amount2)], + vec![make_address_output(address2.into_object(), tx_amount2)], ) .unwrap(); let signed_transaction2 = @@ -1134,7 +1143,7 @@ fn wallet_recover_new_account(#[case] seed: Seed) { let transaction1 = Transaction::new( 0, Vec::new(), - vec![make_address_output(address, tx_amount1)], + vec![make_address_output(address.into_object(), tx_amount1)], ) .unwrap(); let signed_transaction1 = SignedTransaction::new(transaction1, Vec::new()).unwrap(); @@ -1395,7 +1404,7 @@ fn wallet_list_mainchain_transactions(#[case] seed: Seed) { // 1. give X amount of coins to the wallet // 2. Try to create a transaction where the feerate is so high that the UTXO will need to pay more // fee than it has so it will return an NoUTXOs error -// 3. Successfuly create a TX with many outputs and make sure the fee paid is within the range of +// 3. Successfully create a TX with many outputs and make sure the fee paid is within the range of // [0-16] bytes * feerate (16 bytes is the MAX u128 Amount used by the change output) // // 4. Create a sweep TX using the previously created many outputs. The sweep command does not @@ -1485,8 +1494,8 @@ fn wallet_transactions_with_fees(#[case] seed: Seed) { let tx_size = serialization::Encode::encoded_size(&transaction); - // 16 bytes (u128) is the max tx size diffference in the estimation, - // because of the compact encoding for the change amount which is unknown beforhand + // 16 bytes (u128) is the max tx size difference in the estimation, + // because of the compact encoding for the change amount which is unknown beforehand let max_size_diff = std::mem::size_of::(); let min_fee = feerate.compute_fee(tx_size).unwrap(); let max_fee = feerate.compute_fee(tx_size + max_size_diff).unwrap(); @@ -1609,7 +1618,7 @@ fn spend_from_user_specified_utxos(#[case] seed: Seed) { KeyPurpose::ReceiveFunds, idx.try_into().unwrap(), ); - make_address_output(address, utxo_amount) + make_address_output(address.into_object(), utxo_amount) }) .collect_vec(); let block1 = Block::new( @@ -2280,7 +2289,10 @@ fn send_to_unknown_delegation(#[case] seed: Seed) { let (wallet2_delegation_id, delegation_tx) = wallet2 .create_delegation( DEFAULT_ACCOUNT_INDEX, - vec![make_create_delegation_output(address2.clone(), unknown_pool_id)], + vec![ + make_create_delegation_output(address2.clone(), unknown_pool_id), + make_address_output(address2.clone().into_object(), Amount::from_atoms(1)), + ], FeeRate::from_amount_per_kb(Amount::ZERO), FeeRate::from_amount_per_kb(Amount::ZERO), ) @@ -2350,7 +2362,10 @@ fn send_to_unknown_delegation(#[case] seed: Seed) { let (delegation_id, delegation_tx) = wallet .create_delegation( DEFAULT_ACCOUNT_INDEX, - vec![make_create_delegation_output(address.clone(), unknown_pool_id)], + vec![ + make_create_delegation_output(address.clone(), unknown_pool_id), + make_address_output(address.clone().into_object(), Amount::from_atoms(1)), + ], FeeRate::from_amount_per_kb(Amount::ZERO), FeeRate::from_amount_per_kb(Amount::ZERO), ) @@ -2434,7 +2449,10 @@ fn create_spend_from_delegations(#[case] seed: Seed) { let (delegation_id, delegation_tx) = wallet .create_delegation( DEFAULT_ACCOUNT_INDEX, - vec![make_create_delegation_output(address.clone(), pool_id)], + vec![ + make_create_delegation_output(address.clone(), pool_id), + make_address_output(address.clone().into_object(), Amount::from_atoms(1)), + ], FeeRate::from_amount_per_kb(Amount::ZERO), FeeRate::from_amount_per_kb(Amount::ZERO), ) @@ -4009,7 +4027,10 @@ fn lock_then_transfer(#[case] seed: Seed) { chain_config.genesis_block_id(), timestamp, ConsensusData::None, - BlockReward::new(vec![make_address_output(address.clone(), block1_amount)]), + BlockReward::new(vec![make_address_output( + address.clone().into_object(), + block1_amount, + )]), ) .unwrap(); @@ -4061,7 +4082,10 @@ fn lock_then_transfer(#[case] seed: Seed) { block1_id.into(), timestamp, ConsensusData::None, - BlockReward::new(vec![make_address_output(address, block1_amount)]), + BlockReward::new(vec![make_address_output( + address.into_object(), + block1_amount, + )]), ) .unwrap(); @@ -4238,7 +4262,7 @@ fn wallet_scan_multiple_transactions_from_mempool(#[case] seed: Seed) { KeyPurpose::ReceiveFunds, (idx + 1).try_into().unwrap(), ); - let change_output = make_address_output(address, change); + let change_output = make_address_output(address.into_object(), change); let new_output = TxOutput::Transfer( OutputValue::Coin(amount_to_transfer), @@ -4419,7 +4443,7 @@ fn wallet_abandon_transactions(#[case] seed: Seed) { KeyPurpose::ReceiveFunds, (idx + 1).try_into().unwrap(), ); - let change_output = make_address_output(address, change); + let change_output = make_address_output(address.into_object(), change); let new_output = TxOutput::Transfer( OutputValue::Coin(amount_to_transfer), @@ -4828,7 +4852,7 @@ fn sign_decommission_pool_request_between_accounts(#[case] seed: Seed) { // Generate a new block which sends reward to the wallet let block1_amount = Amount::from_atoms(rng.gen_range(NETWORK_FEE + 100..NETWORK_FEE + 10000)); let (addr, _) = create_block(&chain_config, &mut wallet, vec![], block1_amount, 0); - let utxo = make_address_output(addr.clone(), block1_amount); + let utxo = make_address_output(addr.clone().into_object(), block1_amount); let pool_ids = wallet.get_pool_ids(acc_0_index, WalletPoolsFilter::All).unwrap(); assert!(pool_ids.is_empty()); @@ -5131,7 +5155,8 @@ fn sign_send_request_cold_wallet(#[case] seed: Seed) { // Generate a new block which sends reward to the cold wallet address let block1_amount = Amount::from_atoms(rng.gen_range(NETWORK_FEE + 100..NETWORK_FEE + 10000)); - let reward_output = make_address_output(cold_wallet_address.clone(), block1_amount); + let reward_output = + make_address_output(cold_wallet_address.clone().into_object(), block1_amount); let block1 = Block::new( vec![], chain_config.genesis_block_id(), @@ -5236,7 +5261,7 @@ fn test_not_exhaustion_of_keys(#[case] seed: Seed) { // Generate a new block which sends reward to the cold wallet address let block1_amount = Amount::from_atoms(rng.gen_range(NETWORK_FEE + 100..NETWORK_FEE + 10000)); - let reward_output = make_address_output(address.clone(), block1_amount); + let reward_output = make_address_output(address.clone().into_object(), block1_amount); let block1 = Block::new( vec![], chain_config.genesis_block_id(), @@ -5295,7 +5320,7 @@ fn test_add_standalone_private_key(#[case] seed: Seed) { // Generate a new block which sends reward to the new address let block1_amount = Amount::from_atoms(rng.gen_range(NETWORK_FEE + 100..NETWORK_FEE + 10000)); - let output = make_address_output(address.clone(), block1_amount); + let output = make_address_output(address.clone().into_object(), block1_amount); let tx = SignedTransaction::new(Transaction::new(0, vec![], vec![output]).unwrap(), vec![]).unwrap(); @@ -5362,7 +5387,7 @@ fn test_add_standalone_multisig(#[case] seed: Seed) { // Generate a new block which sends reward to the new multisig address let block1_amount = Amount::from_atoms(rng.gen_range(NETWORK_FEE + 100..NETWORK_FEE + 10000)); - let output = make_address_output(multisig_address.clone(), block1_amount); + let output = make_address_output(multisig_address.clone().into_object(), block1_amount); let tx = SignedTransaction::new(Transaction::new(0, vec![], vec![output]).unwrap(), vec![]).unwrap(); @@ -6724,7 +6749,10 @@ fn conflicting_delegation_account_nonce(#[case] seed: Seed) { let (delegation_id, delegation_tx) = wallet1 .create_delegation( DEFAULT_ACCOUNT_INDEX, - vec![make_create_delegation_output(address.clone(), pool_id)], + vec![ + make_create_delegation_output(address.clone(), pool_id), + make_address_output(address.clone().into_object(), Amount::from_atoms(1)), + ], FeeRate::from_amount_per_kb(Amount::ZERO), FeeRate::from_amount_per_kb(Amount::ZERO), ) @@ -7007,7 +7035,10 @@ fn conflicting_delegation_account_nonce_same_wallet(#[case] seed: Seed) { let (delegation_id, delegation_tx) = wallet .create_delegation( DEFAULT_ACCOUNT_INDEX, - vec![make_create_delegation_output(address.clone(), pool_id)], + vec![ + make_create_delegation_output(address.clone(), pool_id), + make_address_output(address.clone().into_object(), Amount::from_atoms(1)), + ], FeeRate::from_amount_per_kb(Amount::ZERO), FeeRate::from_amount_per_kb(Amount::ZERO), ) @@ -7509,7 +7540,10 @@ fn conflicting_delegation_account_nonce_multiple_inputs(#[case] seed: Seed) { let (delegation_id, delegation_tx) = wallet .create_delegation( DEFAULT_ACCOUNT_INDEX, - vec![make_create_delegation_output(address.clone(), pool_id)], + vec![ + make_create_delegation_output(address.clone(), pool_id), + make_address_output(address.clone().into_object(), Amount::from_atoms(1)), + ], FeeRate::from_amount_per_kb(Amount::ZERO), FeeRate::from_amount_per_kb(Amount::ZERO), ) @@ -7777,7 +7811,10 @@ fn conflicting_delegation_account_with_reorg(#[case] seed: Seed) { let (delegation_id, delegation_tx) = wallet .create_delegation( DEFAULT_ACCOUNT_INDEX, - vec![make_create_delegation_output(address.clone(), pool_id)], + vec![ + make_create_delegation_output(address.clone(), pool_id), + make_address_output(address.clone().into_object(), Amount::from_atoms(1)), + ], FeeRate::from_amount_per_kb(Amount::ZERO), FeeRate::from_amount_per_kb(Amount::ZERO), ) @@ -7957,7 +7994,7 @@ fn rollback_utxos_after_abandon(#[case] seed: Seed) { KeyPurpose::ReceiveFunds, idx.try_into().unwrap(), ); - make_address_output(address, utxo_amount) + make_address_output(address.into_object(), utxo_amount) }) .collect_vec(); let block1 = Block::new( @@ -7991,7 +8028,7 @@ fn rollback_utxos_after_abandon(#[case] seed: Seed) { let tx = wallet .create_transaction_to_addresses( DEFAULT_ACCOUNT_INDEX, - [make_address_output(address, utxo_amount)], + [make_address_output(address.into_object(), utxo_amount)], SelectedInputs::Utxos(selected_utxos.clone()), BTreeMap::new(), FeeRate::from_amount_per_kb(Amount::ZERO), diff --git a/wallet/wallet-controller/src/synced_controller.rs b/wallet/wallet-controller/src/synced_controller.rs index 7a4a1ad39..c0e04df28 100644 --- a/wallet/wallet-controller/src/synced_controller.rs +++ b/wallet/wallet-controller/src/synced_controller.rs @@ -543,7 +543,7 @@ where ) -> Result> { self.check_tokens_in_selected_utxo(&selected_utxos).await?; - let output = make_address_output(address, amount); + let output = make_address_output(address.into_object(), amount); self.create_and_send_tx( move |current_fee_rate: FeeRate, consolidate_fee_rate: FeeRate, @@ -656,7 +656,7 @@ where selected_utxo: UtxoOutPoint, change_address: Option>, ) -> Result<(PartiallySignedTransaction, Balances), ControllerError> { - let output = make_address_output(address, amount); + let output = make_address_output(address.into_object(), amount); let utxo_output = fetch_utxo(&self.rpc_client, &selected_utxo, self.wallet).await?; let change_address = if let Some(change_address) = change_address { @@ -944,7 +944,8 @@ where address: Address, amount: Amount, ) -> Result> { - let output = make_address_output_token(address, amount, token_info.token_id()); + let output = + make_address_output_token(address.into_object(), amount, token_info.token_id()); self.create_and_send_token_tx( token_info, move |current_fee_rate: FeeRate, @@ -982,7 +983,8 @@ where amount: Amount, intent: String, ) -> Result<(SignedTransaction, SignedTransactionIntent), ControllerError> { - let output = make_address_output_token(address, amount, token_info.token_id()); + let output = + make_address_output_token(address.into_object(), amount, token_info.token_id()); self.create_token_tx( token_info, move |current_fee_rate: FeeRate, diff --git a/wasm-wrappers/src/lib.rs b/wasm-wrappers/src/lib.rs index ca701452c..d879e4cd8 100644 --- a/wasm-wrappers/src/lib.rs +++ b/wasm-wrappers/src/lib.rs @@ -1128,6 +1128,7 @@ pub fn estimate_transaction_size( let size = tx_size_with_num_inputs_and_outputs(tx_outputs.len(), input_utxos_destinations.len()) + .map_err(Error::TransactionSizeEstimationError)? + outputs_encoded_size(tx_outputs.as_slice()); let inputs_size = inputs.len();