Simplify change policy #14
Merged
Conversation
This file contains 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
It is now just a single threshold which if surpassed implies a change outupt should be added. Fix README examples as well
LLFourn
force-pushed
the
simplify-change-policy
branch
5 times, most recently
from
474d832 to
8c85236
Compare
LLFourn
force-pushed
the
simplify-change-policy
branch
from
8c85236 to
dac0641
Compare
This was referenced Dec 22, 2023
jp1ac4
reviewed
Jan 2, 2024
| value: 0, | ||
| }, | ||
| ); | ||
| if excess > change_policy.min_value as i64 { |
There was a problem hiding this comment.
I'm wondering if this should have been >= instead of > . The min_value_and_waste change policy was previously returning change for excess equal to the min value.
There was a problem hiding this comment.
I think it should be >= indeed.
darosior
added a commit
to wizardsardine/liana
that referenced
this pull request
Jan 13, 2024
5a15c74 commands: return warnings from spend creation (jp1ac4) da474ad spend: add warning when adding change to fee (jp1ac4) 8d84f0d spend: return max possible change from coin selection (jp1ac4) e4d8330 spend: use debug log level in coin selection (jp1ac4) Pull request description: This is a first step towards #811. The GUI will be updated in a follow-up PR. It adds a warning if there is any change/excess available that has been added to the fee, building on the assumption from bitcoindevkit/coin-select#14 that the decision whether to include change depends on whether the excess is over a threshold. The function `select_coins_for_spend()` now returns this threshold so that the caller can check if the possible change amount is above it. I've used an `enum` for the different warnings, but could use strings directly. If the approach looks fine, I'll add some tests. ACKs for top commit: darosior: ACK 5a15c74 Tree-SHA512: 12716b1e299d3154c520667c66aeb7a176a770f4a1d53125a8334d7c5a7e7bb2ce1dcb795ad5998bffc1303d9cb34c651cf8d3ef3dee8210572d75069012bddd
Merged
darosior
added a commit
to wizardsardine/liana
that referenced
this pull request
Apr 15, 2024
43ecd94 spend: change parameter type for rbf (jp1ac4) 6376909 commands: remove rbf rule 4 logic (jp1ac4) 936d7e9 spend: bump bdk_coin_select to 0.3.0 (jp1ac4) Pull request description: This is to resolve #923. The `score` method of the `LowestFee` metric has been fixed upstream and so our temporary partial fix from #867 is no longer required. The `min_fee` parameter of `select_coins_for_spend`, if positive, now ensures that RBF rule 4 is satisfied. Accordingly, it has been renamed to `replaced_fee` and made an `Option`. We could potentially have used the `SpendTxFees` enum as a parameter directly instead of `feerate_vb` and `replaced_fee`, but `feerate_vb` is currently `f32` rather than `u64` and so I kept them as separate parameters. Thanks to how the `replaced_fee` parameter works, the fee iteration approach used in `rbf_psbt` to ensure the replacement satisfies [RBF rule 4](https://github.com/bitcoin/bitcoin/blob/master/doc/policy/mempool-replacements.md#current-replace-by-fee-policy) is no longer required. `base_weight` is no longer stored in `CoinSelector` and instead the output weights are stored in `Target`. This means that the `output_weight` of `DrainWeights` no longer needs to take into account a potential change in output count varint as this is now handled by bdk_coin_select. The min value for change no longer includes the threshold itself and so we have to subtract 1 from our change policy's min value (see bitcoindevkit/coin-select#14 (comment)). We already have a [test](https://github.com/wizardsardine/liana/blob/master/src/commands/mod.rs#L1653-#L1654) that fails without this subtraction as it expects a change output of 5000 sats. ACKs for top commit: darosior: ACK 43ecd94 Tree-SHA512: a064bafef13abefcb8c4b640cfc4017eb288c62891a8b486add33c1499e7061bf262d6aadabbdd4c191ed9b81e3931b382c8c8445e6bb9c1b052380caf14b3f9
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.
This PR makes a simplifying assumption: The decision about whether to add change or not can be reduced to a check whether the excess is over a certain threshold. The change policy is simply how to compute that threshold.
I originally didn't do this because I thought change policies could be more complicated than that -- and they probably could be! But @evanlinjin pointed out that you can easily run the coin selector multiple times with different policies and change output structures if you wanted to compare and contrast different approaches (rather than trying to embed this logic into the change policy itself).
I realized it is actually quite difficult to implement the metrics correctly without knowing the threshold that will case change to be added (and the weight of that change) up front. Knowing this we should be able to make the implementations much simpler.