Repository navigation
fix: compute LowestFee::bound in f64 - #87
Closed
chukwudiikeh wants to merge 1 commit into
Closed
chukwudiikeh wants to merge 1 commit into
chukwudiikeh wants to merge 1 commit into
Conversation
Merged
Member
|
Thanks for tracking this down and for the measurements in #87! The fix is in #88. It takes the approach (subtract as integers, compute in |
evanlinjin
added a commit
that referenced
this pull request
Oct 22, 2026
9531dc0 fix!: do fee math in f64 (志宇) Pull request description: Moves the crate's fee math from `f32` to `f64`. With `f32`, rounding at ordinary amounts makes `LowestFee` miss the cheapest selection: - `LowestFee::bound` cast `selected_value` and `target.value()` to `f32` separately 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 to `f32`, so fees of 2^24 sats or more that differ by a few sats scored the same (#82). Patching only `bound` would leave every other `f32` path rounding, so the whole float layer changes. The next release breaks the API anyway: - `FeeRate` wraps an `Ordf64`. Its constructors and `as_sat_vb`/`spwu` take and return `f64`. - `Candidate::effective_value`, `value_pwu`, `effective_value_pwu`, `implied_fee` and `fee_per_value`, `CoinSelector::waste` and `DrainWeights::waste` return `f64`. - `BnbMetric::score`/`bound`, `run_bnb` and `bnb_solutions` use `Ordf64`. - `FloatExt::ceil` goes through `i64` rather than `i32`, so it is correct for `f64`. - `LowestFee::bound` subtracts the values as integers and needs no helper or final cast. Tests: `bound_is_admissible_at_large_values` calls `bound` on a 1 BTC + 99 sat input (on master the bound is 104 while selecting both costs 101). `score_tells_apart_fees_above_2_pow_24` checks that fees of 2^25 and 2^25 + 1 score differently. The round-count asserts in `tests/bnb.rs` are unchanged. Fixes #82 Replaces #87. Differences from it: - Converts the crate to `f64` rather than adding a private `f64` copy of `Candidate::effective_value` and casting the bound back to `f32`, which left the score and every other caller in `f32`. - Based on current master: keeps the `.max(0.0)` clamp from #71 where #87 kept the `assert!` that #71 removed. - Uses deterministic unit repros instead of a 158-line brute-force file (7 × 20,000 cases, about 30s, not gated with `#[cfg(not(debug_assertions))]`). Top commit has no ACKs. Tree-SHA512: da05ca081de5ee4749ab380ddaf3350499fb4e436afa0b67b192629bed125f7e62b6dc8ce4ebb2aa79f94dd248e32bb88bfd28456f59f67ba25cb041798984c2
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.
Follow-up on Issue #82
LowestFee::boundcastselected_valueandtarget.value()tof32separately before subtracting them, so the bound lost sats at large UTXO values. A bound above the true score made branch and bound prune the cheapest selection, and a negative one hitassert!(ideal_fee >= 0.0).This PR does the subtraction in integers, as suggested in the issue, and does the rest of the bound arithmetic in
f64, casting tof32only at the end, so the public score type is unchanged. It addstests/large_values.rs, which compares branch and bound against brute force over 20,000 random cases per scenario: on master, UTXOs of 0.2–500 BTC gave 1,588 suboptimal selections and 3,763 panics, and with this PR every scenario gives 0. The integer subtraction alone still left failures, becausescale * to_resize.valuewas computed inf32. The test takes about 30s in release mode; I can cut the case count if that's too slow for CI.Fixed
LowestFeeno longer picks a more expensive selection or panics when UTXO values are large.