wallet: avoid call bumpfeediscount with negative values
What the developer wrote
Authored by Pol Espinasa
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>
What changed, and why it matters
This commit fixes a crash bug in Bitcoin Core's wallet coin selection. When the wallet tried to estimate fees for spending unconfirmed coins, it took two separate snapshots of the mempool. If the mempool changed between those snapshots—such as when someone artificially lowered a transaction's fee priority—the second fee could be higher than the first. That produced a negative 'discount' value, which then triggered an internal assertion that the value must be zero or positive, causing the wallet process to crash. The fix simply skips applying the discount when it would be negative.
Apply the patch. It is a minimal, correct change. Consider whether SetBumpFeeDiscount should also defensively reject negative inputs, but the primary fix is sufficient to prevent the crash described.
Security signals we found
Assertion failure from negative value passed to fee-discount helper
Race between two mempool snapshots used in the same coin-selection calculation
prioritisetransaction can flip the sign of the computed discount
Wallet-local denial of service via crafted mempool state
Evidence from the diff
In src/wallet/spend.cpp, ChooseSelectionResult computes bump_fee_overestimate as summed_bump_fees minus combined_bump_fee. Both values come from independent MiniMiner mempool snapshots. If the mempool state changes between snapshots (e.g., via prioritisetransaction reducing an ancestor feerate), combined_bump_fee can exceed summed_bump_fees, making bump_fee_overestimate negative. The original code checked if (bump_fee_overestimate), which is true for any nonzero value including negatives, and then passed the negative value to SetBumpFeeDiscount, whose underlying logic asserts the value is >= 0. The patch changes the guard to if (bump_fee_overestimate > 0), preventing the assertion failure.
Changed components
src/wallet/spend.cppChooseSelectionResultSetBumpFeeDiscountMiniMiner mempool snapshotInspect captured patch +2 / −1
diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index dba7b882..e92546db 100644
--- a/src/wallet/spend.cpp
+++ b/src/wallet/spend.cpp
@@ -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);
Why this scored 57/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.