Add MaxWeight to cap transaction weight during selection#48
Conversation
There was a problem hiding this comment.
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.
|
@murchandamus Thank you for the insight and you have a point. Maybe the best move is to make This will also make bitcoindevkit/bdk-tx#47 implementation cleaner. |
aagbotemi
force-pushed
the
feat/max-weight-metric
branch
3 times, most recently
from
4572958 to
9e7f498
Compare
Thanks @murchandamus for the review.
Thanks @evanlinjin, I took your suggestion and moved the cap onto Target as a selection constraint. |
aagbotemi
force-pushed
the
feat/max-weight-metric
branch
2 times, most recently
from
c63c995 to
92d2425
Compare
| 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" | ||
| ); |
There was a problem hiding this comment.
What's the motivation to remove this?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
In this case, it's better to change the check rather than remove it.
There was a problem hiding this comment.
Yes, I have changed the check
aagbotemi
force-pushed
the
feat/max-weight-metric
branch
from
92d2425 to
0b8b3e5
Compare
| .ok_or_else(|| InsufficientFunds { | ||
| missing: self.excess(target, Drain::NONE).unsigned_abs(), | ||
| .ok_or_else(|| { | ||
| let excess = self.excess(target, Drain::NONE); |
There was a problem hiding this comment.
Excess can be negative because inputs are low in effective value and high in weight -- in which case, we should return MaxWeightExceeded.
There was a problem hiding this comment.
Thanks for the catch. I've fixed it.
| 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" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
aagbotemi
force-pushed
the
feat/max-weight-metric
branch
2 times, most recently
from
0df09d3 to
8a733e2
Compare
aagbotemi
force-pushed
the
feat/max-weight-metric
branch
from
8a733e2 to
543821e
Compare
3 participants
Description
max_weightfield added toTarget, making the maximum transaction weight a selection constraint enforced inis_target_met.Notes for review
is_target_met/is_target_met_with_drain.select_until_target_metnow returnsSelectError { InsufficientFunds, MaxWeightExceeded }so a weight cap failure isn't mislabeled as a funds shortfall.Changelog notice
max_weightfield toTargetselect_until_target_metreturnsSelectError