Skip to content

fix(coin_selection): avoid per-utxo fee rounding#479

Open
benthecarman wants to merge 1 commit intobitcoindevkit:masterfrom
benthecarman:rounding-coin-select
Open

fix(coin_selection): avoid per-utxo fee rounding#479
benthecarman wants to merge 1 commit intobitcoindevkit:masterfrom
benthecarman:rounding-coin-select

Conversation

@benthecarman
Copy link
Copy Markdown
Contributor

@benthecarman benthecarman commented May 5, 2026

select_sorted_utxos computed fee_rate * weight per utxo and summed the results. Because FeeRate * Weight ceils to whole sats, each input dropped up to 0.999 sats of fractional fee. The error grew linearly with the input count: at 1 sat/vb on 271-wu inputs (67.75 sat each), four inputs underpay by 3 sats; ten underpay by 7 sats; large selections drift further.

Accumulate input weight during selection and compute the fee once on the total. Flooring now happens at most once for the whole tx, so the result equals the fee a sender would compute by weighing the finished transaction and never falls short of the requested fee rate.

This was found when comparing fee calculation differences between LDK and BDK.

Description

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

Bugfixes:

  • I've added tests to reproduce the issue which are now passing

select_sorted_utxos computed fee_rate * weight per utxo and summed
the results. Because FeeRate * Weight floors to whole sats, each
input dropped up to 0.999 sats of fractional fee. The error grew
linearly with the input count: at 1 sat/vb on 271-wu inputs
(67.75 sat each), four inputs underpay by 3 sats; ten underpay by
7 sats; large selections drift further.

Accumulate input weight during selection and compute the fee once
on the total. Flooring now happens at most once for the whole tx,
so the result equals the fee a sender would compute by weighing
the finished transaction and never falls short of the requested
fee rate.

This was found when comparing fee calculation differences between LDK
and BDK.
@codecov
Copy link
Copy Markdown

codecov Bot commented May 5, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.05%. Comparing base (a9ad3b9) to head (aa3e4b1).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #479   +/-   ##
=======================================
  Coverage   80.05%   80.05%           
=======================================
  Files          24       24           
  Lines        5360     5360           
  Branches      244      244           
=======================================
  Hits         4291     4291           
  Misses        990      990           
  Partials       79       79           
Flag Coverage Δ
rust 80.05% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

benthecarman added a commit to benthecarman/ldk-node that referenced this pull request May 5, 2026
Update rust-lightning to commit 2313bd5 along with the matching
bitcoin-payment-instructions fork. The new splice API computes its
fee estimate independently of the BDK coin selection it drives, so
any surplus from BDK's higher reservation flows into the new
funding output instead of staying in change.

Bridge the gap in `select_confirmed_utxos`:

  * Strip 5 WU per foreign input when calling `add_foreign_utxo`
    so BDK doesn't double-count the empty script_sig byte and
    witness-count varint that LDK's `satisfaction_weight` already
    includes.

  * Bump the change output up by the residual rounding surplus
    from BDK's per-component fee ceilings — goes away once
    bitcoindevkit/bdk_wallet#479 ships.

Generated with the help of Claude Opus 4.7.
benthecarman added a commit to benthecarman/ldk-node that referenced this pull request May 5, 2026
Update rust-lightning to commit 2313bd5 along with the matching
bitcoin-payment-instructions fork. The new splice API computes its
fee estimate independently of the BDK coin selection it drives, so
any surplus from BDK's higher reservation flows into the new
funding output instead of staying in change.

Bridge the gap in `select_confirmed_utxos`:

  * Strip 5 WU per foreign input when calling `add_foreign_utxo`
    so BDK doesn't double-count the empty script_sig byte and
    witness-count varint that LDK's `satisfaction_weight` already
    includes.

  * Bump the change output up by the residual rounding surplus
    from BDK's per-component fee ceilings — goes away once
    bitcoindevkit/bdk_wallet#479 ships.

Generated with the help of Claude Opus 4.7.
@ValuedMammal
Copy link
Copy Markdown
Collaborator

I see what you mean about avoiding per-utxo fee rounding, just confused by the "floors to whole sats" comment since bitcoin's impl Mul<FeeRate> for Weight states that it uses a ceiling computation, so rounding up instead of down?

@benthecarman
Copy link
Copy Markdown
Contributor Author

Ah yeah, that should be ceils

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants