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.

Add an optional `Target::max_weight` cap on the resulting transaction weight (WU) — relevant e.g. for TRUC/BIP-431 packages capped at 10,000 or 1,000 vB. It's a feasibility constraint (sibling of the value target: a lower bound on value, this an upper bound on weight). Enforcement: - `CoinSelector::is_within_max_weight` — the anti-monotone weight predicate, kept separate from the monotone value-only target check. - `select_until_target_met` returns `SelectError::MaxWeightExceeded` instead of silently returning an over-cap selection. - `LowestFee`: `drain_value` refuses a change output that would bust the cap; `score` rejects any over-cap selection; `bound` hard-prunes subtrees whose lightest (no-drain) form already busts the cap. A cap-agnostic `fee_score` is split out for the bound's internal estimates so they stay admissible. The economic change decision keeps the existing bound proof valid under the cap, so the hard-prune suffices (no extra changeless term). Naming: `Target` now has two constraints with opposite monotonicity — the value target (lower bound, monotone) and `max_weight` (upper bound, anti-monotone) — so they get separate predicates. Rename the value-only predicates to say so: `is_target_met` -> `is_target_value_met`, `is_target_met_with_drain` -> `is_target_value_met_with_drain`, and `is_selection_possible` -> `can_meet_target_value`. Folding `max_weight` into `is_target_met` (as in bitcoindevkit#48) would make it anti-monotone and break the BnB bound's admissibility. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Now that `Target` carries two constraints of *opposite* monotonicity — the value target (a lower bound, monotone) and `max_weight` (an upper bound, anti-monotone) — the value-only predicates should say so in their names: - `is_target_met` -> `is_funded` - `is_target_met_with_drain` -> `is_funded_with_drain` - `is_selection_possible` -> `is_fundable` "funded"/"fundable" is money-centric, so it naturally excludes the weight cap (a tx being funded is orthogonal to being within a weight limit), and the `-ed`/`-able` pair cleanly distinguishes "this selection covers the value" from "these candidates *can* cover the value". Pure rename, no behaviour change. (Folding `max_weight` into `is_target_met` instead — as bitcoindevkit#48 does — would make it anti-monotone and break the admissibility of `LowestFee`'s BnB bound.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

evanlinjin · 2026-07-04T14:43:33Z

@aagbotemi I opened #51 as a replacement. The core issue is that I believe this PR's implementation is incorrect and the tests don't catch it because LowestFee BnB is never actually run with max_weight set.

It turns out that the value and weight predicates should be considered separately to avoid BnB pruning too aggressively and basing #51 on #49 really simplified the implementation.

… refactor (#49) 0ef98e5 refactor: address review feedback on max_weight (志宇) c294763 perf: reserve the extra input's weight in LowestFee's change-recovery bound (LLFourn) d7d286a perf: make LowestFee's BnB bound max_weight-aware (志宇) 9c6a2c3 refactor!: rename value-only predicates to is_funded/is_fundable (志宇) 79699b8 test: exercise Target::max_weight and add exact feasibility oracle (志宇) 56698ea feat!: add Target::max_weight and enforce it in selection (志宇) 8825078 chore: Enforce MSRV (志宇) Pull request description: Alternative to #48 for capping transaction weight during coin selection — e.g. TRUC / BIP-431 packages limited to 10,000 / 1,000 vB. The key design choice: this is **built on the metric-decides-change refactor (#49)**, because that's what keeps the weight cap from breaking `LowestFee`'s branch-and-bound bound (see [*Why build on #49*](#why-build-on-49)). Four commits: (1) logic, (2) tests, (3) a pure rename of the value-feasibility predicates, (4) an optional bound speedup. ## Approach - `Target::max_weight: Option<u64>` — a feasibility constraint that mirrors the value target: value is a *lower* bound, `max_weight` an *upper* bound. - These two have **opposite monotonicity**, so they stay separate predicates: the value check stays monotone (`is_funded`), the cap is a distinct anti-monotone check (`is_within_max_weight`). That separation is what preserves the monotonicity the BnB bounds rely on. - The cap is enforced only where a selection is committed: - `LowestFee` (which owns the change decision post-#49): refuses change that would bust the cap, rejects any over-cap selection, and prunes subtrees whose lightest (no-drain) form already busts it. - `select_until_target_met` now returns `SelectError::MaxWeightExceeded` instead of silently returning an over-cap selection. ## Why build on #49 `LowestFee`'s branch-and-bound rests on one claim about its bound: **a selection with a worthwhile change output can't be beaten by a descendant that adds inputs to become changeless.** Whether that holds depends on *who* decides the change policy. **Before #49**, the change policy is decoupled from the metric, so a selection can carry a change output that doesn't actually minimize the fee. Pile *non-effective* inputs onto it — each costs more in fee than it adds in value, shrinking the excess — until the change vanishes, and the resulting changeless selection can score *better*. The bound trusted the with-change score as a floor for the subtree; now it's too high, BnB prunes the branch holding the real optimum, and you get a **suboptimal** selection. (This is the inadmissibility #48 runs into.) **After #49**, the metric owns the decision: change is added only when it strictly lowers the long-term fee (`change_value > spend_fee`). Now dropping a worthwhile change can never help. With `A` a worthwhile-change selection and `B = A + inputs` (added value `v ≥ 0`) made changeless: ``` LTF_A = (selected_A − target − change_value) + spend_fee // recovers the change LTF_B = selected_B − target // burns the excess LTF_B − LTF_A = v + change_value − spend_fee > 0 // change_value > spend_fee, v ≥ 0 ``` `B` always costs more, so the bound stays admissible. **And that's exactly what `max_weight` needs.** The cap enforces itself by *refusing* a change output that would bust it — i.e. it can force a descendant to be changeless. The proof never asks *why* `B` is changeless, only that `A`'s change was worthwhile. So on the #49 base the cap needs nothing more than a weight hard-prune — no bound rewrite, no lost optimality. Fold it into the value predicate on the old base instead, and the suboptimality above comes right back. ## Commit 3: renamed predicates `Target` now carries two constraints of opposite monotonicity, so the value-only predicates are renamed to say so — "funded/fundable" is money-centric and naturally excludes weight, and `-ed`/`-able` distinguishes "this selection covers the value" from "these candidates *can* cover it": `is_target_met` → `is_funded`, `is_target_met_with_drain` → `is_funded_with_drain`, `is_selection_possible` → `is_fundable`. ## Commit 4: cap-aware bound (optional speedup) Correctness only needs the hard-prune above. Commit 4 additionally teaches the *bound* about the cap — pruning subtrees that are still under-cap but provably can't reach the value target without busting it. It never changes the result (and stays admissible — the `ensure_bound_is_not_too_tight` proptest passes), but it's a large win in the tight-cap regime a safety rail actually lives in. Measured BnB rounds to completion on a variable-weight pool, cap-aware bound **off → on**: | `max_weight` | rounds off → on | |---|---| | too tight (infeasible) | whole tree (millions, never terminates) → **0** | | binds the optimum | ~13.5k → **33–359** | | tight, optimum still fits | ~5.3k → ~2.4k | | loose (cap doesn't bite) | ~5.3k → ~5.3k (no-op) | The headline is the first row: for a too-tight cap the bound proves infeasibility instantly, instead of enumerating the entire subtree before giving up. ## Testing - `max_weight` is threaded through the `LowestFee` proptests, so BnB is checked against exhaustive search *with the cap active*. - A new `bnb_respects_max_weight` proptest cross-checks BnB feasibility against an independent exhaustive oracle — this is what validates the weight hard-prune. ## Breaking changes - `Target { .. }` literals now require a `max_weight` field. - The predicates renamed above (`is_target_met` → `is_funded`, etc.). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: efd9e426ceca0ae2f1ac9fdbba5012184f9826c830d02171f5ebc13866da9c25fedc418b589a77c9739e3c07ce00baa0cc93e7cf2a317f152f37d55345a2ffa5

evanlinjin · 2026-07-07T11:31:58Z

Closing as #51 is merged.

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

Merged

evanlinjin closed this Jul 7, 2026

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!

evanlinjin commented Jul 4, 2026

Uh oh!

evanlinjin commented Jul 7, 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