highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-07-22 22:29:28 +02:00
Code Issues Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#35633: wallet: avoid call bumpfeediscount with negative values

Browse Source 
3ae3a94f2b wallet: avoid call bumpfeediscount with negative values (Pol Espinasa)

Pull request description:

  in https://github.com/bitcoin/bitcoin/issues/34232 dergoegge reported an assertion fail in `SetBumpFeeDiscount`.

  **Context**

  The bump-fee discount in the savings that we can have when multiple UTXOs that we selected for our transaction share a common unconfirmed ancestor transaction.

  We know we can have some savings because we first calculate the `summed_bump_fees` and then compare to the `combined_bump_fee`.
  For context:
  - `bump_fees`: Extra fees that the new transaction must pay to contribute to get his ancestor confirmed.
  - `summed_bump_fees`: The sum of each input ancestor `bump_fee`. If we have two inputs with unconfirmed ancestors A and B and both have a `bump_fee = 100` then `summed_bump_fees = 200`.
  - `combined_bump_fee`: Is the total `bump_fees` summed of all inputs, but taking into account shared ancestors. If A and B share the transaction ancestor then `combined_bump_fee = 100` not `200` as the transaction must be bumped only once.

  If `summed_bumpfees` > `combined_bump_fee` we are overestimating the `bump_fee` as we are counting multiple times the same ancestors so we can discount it using `SetBumpFeeDiscount(summed_bump_fees - combined_bump_fees)`.

  **Problem**

  To calculate `summed_bump_fees` and `combined_bump_fee` we use two different fresh MiniMiner snapshots of the mempool. Because they are called in different moments the two snapshots of the mempool might be different. An artificial feerate decrease of an ancestor using `prioritizesettransaction` can make `combined_bump_fee > summed_bump_fees` creating a negative discount. This cause calling `SetBumpFeeDiscount` with a negative vaule triggering an assertion `discount >= 0`.

  **Fix**

  This PR fixes it by ensuring not only that a discount exist but also that is greater the 0.

  **Test**

  It is hard to manually trigger this race condition. dergoegge coded a patch and test to trigger it that can be used to test the fix.
  5320e2fd21

ACKs for top commit:
  achow101:
    ACK 3ae3a94f2b
  pablomartin4btc:
    ACK 3ae3a94f2b

Tree-SHA512: e76693eb66c4883ed3ef5edf1baa972657b0d532487803706312d486b4d4a4997f5dcbc64781a1a21b7b5628b06fcc97a6633382509061734d0f5615e851bef1
This commit is contained in:
Ava Chow
2026-07-14 13:15:52 -07:00
parent c4216e885a 3ae3a94f2b
commit 8d9e4f8dbd
1 changed files with 2 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
src/wallet/spend.cpp
View File

@@ -796,7 +796,8 @@ util::Result<SelectionResult> ChooseSelectionResult(interfaces::Chain& chain, co
return util::Error{_("Failed to calculate bump fees, because unconfirmed UTXOs depend on an enormous cluster of unconfirmed transactions.")};
}
CAmount bump_fee_overestimate = summed_bump_fees - combined_bump_fee.value();
if (bump_fee_overestimate) {
// Avoid negative discount if mempool changed between the two bump fee snapshots.
if (bump_fee_overestimate > 0) {
result.SetBumpFeeDiscount(bump_fee_overestimate);
}
result.RecalculateWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee);