Add MaxWeight to cap transaction weight during selection by aagbotemi · Pull Request #48 · bitcoindevkit/coin-select

aagbotemi · 2026-06-25T12:36:57Z

Description

max_weight field added to Target, making the maximum transaction weight a selection constraint enforced in is_target_met.

Notes for review

The weight cap lives on Target and flows through is_target_met/is_target_met_with_drain.
select_until_target_met now returns SelectError { InsufficientFunds, MaxWeightExceeded } so a weight cap failure isn't mislabeled as a funds shortfall.

Changelog notice

Added max_weight field to Target
select_until_target_met returns SelectError

murchandamus

I’m not actively contributing to this repository, so please feel free to ignore me. I noticed that this PR only adds MaxWeight for BnB, but a maximum weight would be especially relevant in the context of creating TRUC transactions that are limited to 10,000 vB or 1000 vB respectively, so it would be relevant to all appropriate for all coinselection algorithms.

evanlinjin · 2026-06-27T02:07:49Z

@murchandamus Thank you for the insight and you have a point. Maybe the best move is to make MaxWeight part of the Target as it is a selection constraint.

This will also make bitcoindevkit/bdk-tx#47 implementation cleaner.

aagbotemi · 2026-06-27T12:10:41Z

I’m not actively contributing to this repository, so please feel free to ignore me. I noticed that this PR only adds MaxWeight for BnB, but a maximum weight would be especially relevant in the context of creating TRUC transactions that are limited to 10,000 vB or 1000 vB respectively, so it would be relevant to all appropriate for all coinselection algorithms.

Thanks @murchandamus for the review.

Maybe the best move is to make MaxWeight part of the Target as it is a selection constraint.

This will also make bitcoindevkit/bdk-tx#47 implementation cleaner.

Thanks @evanlinjin, I took your suggestion and moved the cap onto Target as a selection constraint.

evanlinjin · 2026-06-27T14:41:31Z

-            debug_assert_eq!(
-                self.is_target_met(target),
-                self.is_target_met_with_drain(
-                    target,
-                    Drain {
-                        weights: change_policy.drain_weights,
-                        value: excess as u64
-                    }
-                ),
-                "if the target is met without a drain it must be met after adding the drain"
-            );


What's the motivation to remove this?

I intended to use excess. Now that max_weight is on Target, is_target_met and is_target_met_with_drain both check the cap, and they can differ on weight. The replacement check only the value it was meant to guard, independent of weight.

In this case, it's better to change the check rather than remove it.

Yes, I have changed the check

evanlinjin · 2026-06-28T00:35:02Z

-            .ok_or_else(|| InsufficientFunds {
-                missing: self.excess(target, Drain::NONE).unsigned_abs(),
+            .ok_or_else(|| {
+                let excess = self.excess(target, Drain::NONE);


Excess can be negative because inputs are low in effective value and high in weight -- in which case, we should return MaxWeightExceeded.

Thanks for the catch. I've fixed it.

evanlinjin · 2026-06-28T00:39:13Z

-            debug_assert_eq!(
-                self.is_target_met(target),
-                self.is_target_met_with_drain(
+            debug_assert!(
+                self.excess(
                    target,
                    Drain {
                        weights: change_policy.drain_weights,
-                        value: excess as u64
-                    }
-                ),
-                "if the target is met without a drain it must be met after adding the drain"
+                        value: excess as u64,
+                    },
+                ) >= 0,
+                "if the target value is met without a drain it must be met after adding the drain"


Shouldn't the check be: if the target is met without a drain, it must be met after adding the drain, unless if max weight is exceeded?

This is wrong. Adding a drain output increases the weight of the transaction so we cannot guarantee that it'll be within our max weight target (as the debug_assert implies).

Good. We can either keep a value-only check (excess(target, drain) >= 0, scoped to value only) or drop the assert, since with drain.value = excess here it's always true. Open to your thoughts.

evanlinjin · 2026-06-30T13:43:22Z

We need a bit more thought looking at how adding max_weight to Target changes that other parts of the codebase.

The logic of is_selection_possible is now wrong. Previously, Target was a purely monotone predicate -- it's fields calculated excess which can monotonously be increased by adding more effective inputs. Putting max_weight in Target changed this property as max_weight puts an upper bound on the weights of the inputs added. To "fix" it, we need to do a more expensive knapsack algorithm -- is this the best move?
How does the max_weight parameter affect the LowestFee metric? I've noticed that proptests are not updated to test with different max_weight values.

evanlinjin · 2026-07-02T05:42:11Z

@aagbotemi I'm going to have a go at finding the right solution. The problem is a lot more complex than initially thought.

aagbotemi · 2026-07-02T16:25:42Z

I went down some rabbit hole. Making is_selection_possible exact under a weight cap turns it into a knapsack-style problem where we have to search subsets. Any exact approach is too heavy for a simple pre-check, and BnB already does that kind of search. The idea I was working with earlier is to have is_selection_possible ignore max_weight. Then false still means truly impossible, true means possible ignoring the cap, and selection itself reports MaxWeightExceeded. I also wrote a proptest that brute-forces every subset to verify any solution we pick here. Interested to hear findings.

murchandamus reviewed Jun 25, 2026

View reviewed changes

aagbotemi force-pushed the feat/max-weight-metric branch 3 times, most recently from 4572958 to 9e7f498 Compare June 27, 2026 12:00

aagbotemi force-pushed the feat/max-weight-metric branch 2 times, most recently from c63c995 to 92d2425 Compare June 27, 2026 14:12

evanlinjin reviewed Jun 27, 2026

View reviewed changes

Comment thread src/coin_selector.rs Outdated

aagbotemi force-pushed the feat/max-weight-metric branch from 92d2425 to 0b8b3e5 Compare June 27, 2026 16:50

aagbotemi requested a review from evanlinjin June 27, 2026 17:02

evanlinjin reviewed Jun 28, 2026

View reviewed changes

aagbotemi mentioned this pull request Jun 28, 2026

Add configurable max_weight to SelectorParams bitcoindevkit/bdk-tx#47

Draft

4 tasks

aagbotemi force-pushed the feat/max-weight-metric branch 2 times, most recently from 0df09d3 to 8a733e2 Compare June 28, 2026 22:50

feat!: enforce max_weight in Target

543821e

aagbotemi force-pushed the feat/max-weight-metric branch from 8a733e2 to 543821e Compare June 28, 2026 23:02

evanlinjin mentioned this pull request Jul 2, 2026

Add Target::max_weight, built on the metric-decides-change refactor (#49) #51

Draft

Uh oh!

Conversation

aagbotemi commented Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Notes for review

Changelog notice

Uh oh!

murchandamus left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evanlinjin commented Jun 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aagbotemi commented Jun 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evanlinjin Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evanlinjin commented Jun 30, 2026

Uh oh!

evanlinjin commented Jul 2, 2026

Uh oh!

aagbotemi commented Jul 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aagbotemi commented Jun 25, 2026 •

edited

Loading

murchandamus left a comment •

edited

Loading

evanlinjin commented Jun 27, 2026 •

edited

Loading

evanlinjin Jun 30, 2026 •

edited

Loading