Repository navigation
fix!: do fee math in f64 - #88
Merged
Merged
Conversation
`FeeRate`, `Candidate`'s value methods, the waste functions and the `BnbMetric` score were all `f32`. At ~1 BTC values f32's spacing is 8 sats, so `LowestFee::bound` (which cast `selected_value` and `target.value()` separately before subtracting them) could land several sats above the true fee and BnB pruned the cheapest selection. Fees of 2^24 sats or more also scored the same, so BnB could keep the dearer of two selections (bitcoindevkit#82). Move the crate's float layer to f64: `FeeRate` wraps an `Ordf64`, `BnbMetric::score`/`bound` return `Ordf64`, and `FloatExt::ceil` goes through `i64` so it is correct for f64. `LowestFee::bound` also subtracts the selected and target values as integers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
evanlinjin
marked this pull request as ready for review
October 22, 2026 02:40
LLFourn
approved these changes
Oct 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Moves the crate's fee math from
f32tof64. Withf32, rounding at ordinary amounts makesLowestFeemiss the cheapest selection:LowestFee::boundcastselected_valueandtarget.value()tof32separately before subtracting them. At ~1 BTC,f32's spacing is 8 sats, so the bound could come out several sats above the true fee, and BnB pruned the cheapest selection.LowestFee's score cast the fee tof32, so fees of 2^24 sats or more that differ by a few sats scored the same (LowestFee score loses precision for fees above 2^24 sats due to f32 conversion #82).Patching only
boundwould leave every otherf32path rounding, so the whole float layer changes. The next release breaks the API anyway:FeeRatewraps anOrdf64. Its constructors andas_sat_vb/spwutake and returnf64.Candidate::effective_value,value_pwu,effective_value_pwu,implied_feeandfee_per_value,CoinSelector::wasteandDrainWeights::wastereturnf64.BnbMetric::score/bound,run_bnbandbnb_solutionsuseOrdf64.FloatExt::ceilgoes throughi64rather thani32, so it is correct forf64.LowestFee::boundsubtracts the values as integers and needs no helper or final cast.Tests:
bound_is_admissible_at_large_valuescallsboundon a 1 BTC + 99 sat input (on master the bound is 104 while selecting both costs 101).score_tells_apart_fees_above_2_pow_24checks that fees of 2^25 and 2^25 + 1 score differently. The round-count asserts intests/bnb.rsare unchanged.Fixes #82
Replaces #87. Differences from it:
f64rather than adding a privatef64copy ofCandidate::effective_valueand casting the bound back tof32, which left the score and every other caller inf32..max(0.0)clamp from fix: clamp ideal_fee to 0.0 in LowestFee::bound to prevent f32 rounding panic #71 where fix: compute LowestFee::bound in f64 #87 kept theassert!that fix: clamp ideal_fee to 0.0 in LowestFee::bound to prevent f32 rounding panic #71 removed.#[cfg(not(debug_assertions))]).