wallet: avoid call bumpfeediscount with negative values #35633

pull polespinasa wants to merge 1 commits into bitcoin:master from polespinasa:2026-07-01-setbumpfeediscount changing 1 files +2 −1

polespinasa commented at 3:19 PM on July 1, 2026: member
in #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. https://github.com/polespinasa/bitcoin/commit/5320e2fd215734ff3b7c59b07108ca5c5f7e075f
DrahtBot added the label Wallet on Jul 1, 2026
DrahtBot commented at 3:20 PM on July 1, 2026: contributor



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35633.



Reviews

See the guideline and AI policy for information on the review process.

Type Reviewers

ACK achow101, pablomartin4btc

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.



Conflicts

No conflicts as of last run.
polespinasa commented at 3:21 PM on July 1, 2026: member

@dergoegge wanted to use your commit for the test but something was failing in the imports :)

Type	Reviewers
ACK	achow101, pablomartin4btc

in src/wallet/spend.cpp:799 in ccfa09e0c3 outdated

 795 | @@ -796,7 +796,7 @@ util::Result<SelectionResult> ChooseSelectionResult(interfaces::Chain& chain, co
 796 |              return util::Error{_("Failed to calculate bump fees, because unconfirmed UTXOs depend on an enormous cluster of unconfirmed transactions.")};
 797 |          }
 798 |          CAmount bump_fee_overestimate = summed_bump_fees - combined_bump_fee.value();
 799 | -        if (bump_fee_overestimate) {
 800 | +        if (bump_fee_overestimate > 0) {

pablomartin4btc commented at 6:45 AM on July 7, 2026:

I think a short comment on the > 0 condition would help explain the subtle intent... without it, it just looks like a truthy-vs-explicit style preference.

        // Avoid negative discount if mempool changed between the two bump fee snapshots.
        if (bump_fee_overestimate > 0) {

polespinasa commented at 3:06 PM on July 7, 2026:

done

pablomartin4btc commented at 6:51 AM on July 7, 2026: member

Concept ACK

It would be nice to incorporate the test into the PR, if possible.

wallet: avoid call bumpfeediscount with negative values

ChooseSelectionResult computes the bump-fee discount as: summed_bump_fees - combined_bump_fee
Where summed_bump_fees is the sum of per-UTXO ancestor bump fees and combined_bump_fee is the
true combined cost taking into account shared ancestors.

Both variables use creates a fresh MiniMiner snapshot of the mempool. Because of that
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.
This cause calling bumpfeediscount with a negative vaule triggering an assertion >= 0.

This commit fixes this by only calling bumpfeediscount when the discount is strictly positive.

Co-authored-by: dergoegge <n.goeggi@gmail.com>

3ae3a94f2b

polespinasa force-pushed on Jul 7, 2026
polespinasa commented at 3:07 PM on July 7, 2026: member

It would be nice to incorporate the test into the PR, if possible.

I don't think that is possible, the situation is racy and the test that dergoegge proposed needed to modify the source code to pause it and force the condition.
achow101 commented at 6:34 PM on July 7, 2026: member

ACK 3ae3a94f2bdc572cdf1b5223419a25650291ab56
DrahtBot requested review from pablomartin4btc on Jul 7, 2026
pablomartin4btc commented at 2:16 PM on July 8, 2026: member

ACK 3ae3a94f2bdc572cdf1b5223419a25650291ab56

Contributors

Labels

Linked (view graph)

#34232 wallet: SetBumpFeeDiscount(const CAmount): Assertion `discount >= 0' failed

wallet: avoid call bumpfeediscount with negative values #35633

Code Coverage & Benchmarks

Reviews

Conflicts