Fix/wallet fee calculation #1916

OBorce · 2025-04-29T19:08:53Z

The initial bug was caused by the calculation of the TX size with 0 inputs, and the bug got triggered when a TX was constructed with 100+ inputs which caused the compact length to have 1 more byte then before.
This was fixed by passing the number of inputs to the tx_size_with_outputs.
Currently as before the fee of a TX can be up to 16 bytes more per change output because of the unknown amount of change that will be returned, and the calculation always uses Amount::MAX.
The fees test has been rewritten to have both a TX with many inputs up to 1k and a sweep TX which uses a separate implementation which doesn't have a change output and always pays correctly the exact fee.
In the new test a rounding error was discovered, and fixed which happens in case of individual input fee calculation vs total fee calculation using entire tx size.
So now we still calculate the fee per input for the selection algorithm to know which input has how much effective value, but at the end after the inputs have been selected we recalculate the fee and add the difference back as change. (it should always be up to number of inputs in atoms, which is not a lot but still was crushing the test)
Also added the missing limit for tx size/weight, which now should prevent creating a TX with size over 1MB retrieved from chain_config.

- 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

ImplOfAnImpl · 2025-04-30T08:07:53Z

wallet/src/wallet/tests.rs

@@ -1447,7 +1462,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,


typo - diffference

ImplOfAnImpl · 2025-04-30T08:08:10Z

wallet/src/wallet/tests.rs

-    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


beforhand -> beforehand

ImplOfAnImpl · 2025-04-30T08:29:12Z

wallet/src/account/mod.rs

+            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


ImplOfAnImpl · 2025-05-07T05:42:59Z

common/src/size_estimation/mod.rs

    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");
-    serialization::Encode::encoded_size(&tx)
+    let size = serialization::Encode::encoded_size(&tx);


Put empty tx size calculation to a constant? (in a lazy_static block)
(Can be local to this function)

ImplOfAnImpl · 2025-05-07T05:51:20Z

common/src/size_estimation/mod.rs

+    let output_compact_size_diff =
+        serialization::Encode::encoded_size(&CompactSize {
+            value: num_outputs as u64,
+        }) - serialization::Encode::encoded_size(&CompactSize { value: 0 });


Plz calculate the fixed part (serialization::Encode::encoded_size(&CompactSize { value: 0 })) only once.

Also:

The codec used u32 for the length of the vector/slice instead of u64 - https://github.com/paritytech/parity-scale-codec/blob/0a0295a465ac40fd2d38359220fdcc3f3de94cae/src/codec.rs#L1090 (not that it makes much of a difference though).

It may make sense to get rid of the CompactSize and use Compact::<u32>::compact_len here instead (like Compact::<u32>::compact_len(&(num_outputs as u32)) - Compact::<u32>::compact_len(&0)).
(probably first checking that num_outputs actually fits into u32)

ImplOfAnImpl · 2025-05-07T10:24:32Z

wallet/src/account/mod.rs

        let selection_result = select_coins(
            utxos,
            (amount_to_be_paid_in_currency_with_fees - preselected_amount).unwrap_or(Amount::ZERO),
            PayFee::PayFeeWithThisCurrency,
            cost_of_change,
            selection_algo,
+            self.chain_config.max_tx_size_for_mempool(),
        )?;


I wonder, why do we need to call select_coins in the case when amount_to_be_paid_in_currency_with_fees == preselected_amount?

Also, select_coins has this code:

if selection_target == Amount::ZERO && pay_fees == PayFee::DoNotPayFeeWithThisCurrency { return Ok(SelectionResult::new(selection_target)); }

Why doesn't it ignore zero amount in the PayFeeWithThisCurrency case too?

It looks like we select coins for the change even when we don't need any change.

ImplOfAnImpl · 2025-05-07T10:51:33Z

wallet/src/account/mod.rs

-            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(


I would say that check_outputs_and_add_change no longer checks outputs, so it's better to rename it.
(Or maybe remove it completely and put its body directly into select_inputs_for_send_request. Since it's not really reusable IMO).

ImplOfAnImpl · 2025-05-07T11:00:43Z

wallet/src/account/mod.rs

+                            (inputs.amount, inputs.fee, inputs.total_input_size)
+                        });
+
+                let (size_of_change, cost_of_change) = output_change_size_and_fees(


There is a tight coupling between output_change_size_and_fees and check_outputs_and_add_change - the former accepts Option<&Address<Destination>> and assumes PublicKeyHash if it's None. And the latter then calls key_chain.next_unused_address if change_addresses doesn't contain the corresponding currency, and next_unused_address is assumed to always return PublicKeyHash I guess, which is not a great assumption either.

I wonder if we could refactor it in this PR. E.g. modify change_addresses in advance, calling key_chain.next_unused_address if necessary, so that the map already contains all possible currencies.
(If not, then it's better to at least add some comments to output_change_size_and_fees and check_outputs_and_add_change, mentioning this interdependency).

ImplOfAnImpl · 2025-05-07T12:04:18Z

wallet/src/account/mod.rs

+        // 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;
+            }
+        };


This looks hacky.

select_inputs_for_send_request operates on pay_fee_with_currency, so I guess you should pass it here instead of assuming that it's always Coin.
(or if you think it's redundant, then maybe select_inputs_for_send_request should no longer pretend that it can handle other currencies to pay fees with).

The code in select_inputs_for_send_request, where it iterates over "non-pay_fee_with_currency" currencies and updates the total tx size and fee, doesn't make sense anymore, because those values will always be zeroes now.

So,
a) I wonder what was wrong with the previous approach. Why can't we return the sizes/fees on per-currency basis as before, and make the caller combine the result (like it still does)?
b) If the old approach is not good anymore, then maybe this function's return type should be changed to something else that better reflects the data that is being collected?

ImplOfAnImpl · 2025-05-07T12:04:55Z

wallet/src/wallet/tests.rs

+// 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


Successfully

OBorce requested review from erubboli and ImplOfAnImpl as code owners April 29, 2025 19:08

OBorce force-pushed the fix/wallet-fee-calculation branch from 05abe2e to 6b1fe65 Compare May 1, 2025 09:52

OBorce added 3 commits May 6, 2025 12:29

fix tx fee calculation for large number of inputs

0e44ac9

- 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

add tests for max tx weight

c3ca958

fix comments

5a4271e

OBorce force-pushed the fix/wallet-fee-calculation branch from 6b1fe65 to 5a4271e Compare May 6, 2025 10:30

ImplOfAnImpl approved these changes May 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/wallet fee calculation #1916

Fix/wallet fee calculation #1916

OBorce commented Apr 29, 2025 •

edited

Loading

ImplOfAnImpl Apr 30, 2025

ImplOfAnImpl Apr 30, 2025

ImplOfAnImpl Apr 30, 2025

ImplOfAnImpl May 7, 2025

ImplOfAnImpl May 7, 2025

ImplOfAnImpl May 7, 2025

ImplOfAnImpl May 7, 2025

ImplOfAnImpl May 7, 2025

ImplOfAnImpl May 7, 2025

ImplOfAnImpl May 7, 2025

Fix/wallet fee calculation #1916

Are you sure you want to change the base?

Fix/wallet fee calculation #1916

Conversation

OBorce commented Apr 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OBorce commented Apr 29, 2025 •

edited

Loading