Repository navigation
fix: clamp ideal_fee to 0.0 in LowestFee::bound to prevent f32 rounding panic - #71
Merged
evanlinjin merged 1 commit intoOct 21, 2026
Conversation
5 tasks done
evanlinjin
requested changes
Oct 20, 2026
evanlinjin
left a comment
evanlinjin
left a comment
Member
There was a problem hiding this comment.
Thanks for working on this. Can approve once these changes are made
Comment on lines
272
to
+276
| // `scale` could be 0 even if `is_funded` is `false` due to the latter being based on | ||
| // rounded-up vbytes. | ||
| let ideal_fee = scale.0 * to_resize.value as f32 + cs.selected_value() as f32 | ||
| - target.value() as f32; | ||
| assert!(ideal_fee >= 0.0); | ||
| let ideal_fee = (scale.0 * to_resize.value as f32 + cs.selected_value() as f32 | ||
| - target.value() as f32) | ||
| .max(0.0); |
Member
There was a problem hiding this comment.
nit: The comment above still only explains why scale can be 0. Now that the assert! is gone, the reason for the .max(0.0) isn't written anywhere. Suggest a line on it:
Suggested change
| // `scale` could be 0 even if `is_funded` is `false` due to the latter being based on | |
| // rounded-up vbytes. | |
| let ideal_fee = scale.0 * to_resize.value as f32 + cs.selected_value() as f32 | |
| - target.value() as f32; | |
| assert!(ideal_fee >= 0.0); | |
| let ideal_fee = (scale.0 * to_resize.value as f32 + cs.selected_value() as f32 | |
| - target.value() as f32) | |
| .max(0.0); | |
| // `scale` could be 0 even if `is_funded` is `false` due to the latter being based on | |
| // rounded-up vbytes. f32 rounding can also push the sum slightly below zero; 0 is | |
| // always an admissible lower bound for a fee, so clamp. | |
| let ideal_fee = (scale.0 * to_resize.value as f32 + cs.selected_value() as f32 | |
| - target.value() as f32) | |
| .max(0.0); |
Arowolokehinde
force-pushed
the
fix/lowest-fee-bound-f32-panic
branch
from
October 21, 2026 12:33
42995cf to
b954eb5
Compare
Contributor
Author
|
Thanks for the review. Both comments have been addressed. I have updated the test with the changes you suggested and also the CoinSelector::new and bound signatures after rebasing onto master. Locally, all tests pass and fmt and clippy are clean. |
evanlinjin
force-pushed
the
fix/lowest-fee-bound-f32-panic
branch
from
October 21, 2026 21:13
b954eb5 to
5e1c018
Compare
Merged
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.
f32 rounding causes the fractional relaxation to land slightly below zero on valid inputs, triggering assert!(ideal_fee >= 0.0). Clamping to 0.0 is always admissible since fees are never negative.
Added a regression test using the concrete values from the issue report.
Closes #57