Add waste metric for coin selection#435
Add waste metric for coin selection#435benthecarman wants to merge 1 commit intobitcoindevkit:masterfrom
Conversation
| } | ||
| } | ||
|
|
||
| struct WasteMetric; |
There was a problem hiding this comment.
This looks like a good direction for improving coin selection, I haven't looked deeply into the code/algorithm but I do have a couple rust style suggestions.
- Unless you plan on adding additional fields to the
WasteMetricstruct I think you can do without it. - And if the idea for
calculate_wasteis as a helper function to implementations of theCoinSelectionAlgorithmtrait then it may be better to makecalculate_wastea "default implementation" on theCoinSelectionAlgorithmtrait.
If you take this approach then possibly CoinSelectionResult could include a waste metric field.
There was a problem hiding this comment.
The waste metric requires a long term fee rate (expected average fee rate of the wallet) so idk if it can be directly be added to CoinSelectionResult.
There was a problem hiding this comment.
Just to clarify, the "Long Term Feerate Estimate" (LTFRE) is not the average feerate on the wallet, it's more of a minimum feerate that you expect to be able to spend at in the future. The thinking is that if you expect to be able to spend an input at a lower feerate in the future, you should delay spending it to then, hence, anything above that feerate is considered "wasteful". Since you always need to use at least one input, two solutions that each use one input of the same type will see their score increase by the same amount, so it doesn't affect their prioritization. However, if you e.g. have different output types or compare solutions with multiple inputs, you'll prefer the smaller input weight per this heuristic.
rajarshimaitra
left a comment
rajarshimaitra
left a comment
There was a problem hiding this comment.
Thanks for the pull.
tested and concept ACK.
Can you please provide some more doc details within the code on waste metric? Is it used by core too? I would like to learn more about the rationale behind the waste calculations and see how that can be used to make selction-algo choices efficient.
| impl WasteMetric { | ||
| fn calculate_waste( | ||
| selected: Vec<WeightedUtxo>, | ||
| cost_of_change: Option<u64>, | ||
| amount_needed: u64, | ||
| fee_rate: FeeRate, | ||
| long_term_fee_rate: FeeRate, | ||
| ) -> i64 { |
There was a problem hiding this comment.
Instead of returning i64 why not return just return Self? And have i64 or whatever as member of WasteMetric. If we are defining a struct anyway, lets use it. :)
In this way we can also use this inside CoinSelectionResult later. This seems to me like a very nice tool to compare different selection algorithm and we can expose a simple API to users with some default configs (like long term fee rate etc).
|
Some good suggestions came up in our team chat today from @LLFourn and @afilini about his PR.
Also @rajarshimaitra reminded us that tomorrows PR review club will be on this topic, so good for anyone interested to attend. |
|
moving this one to DRAFT until we figure out how to integrate it. |
|
Hey @benthecarman, are you still working on this? May I continue your work? |
Feel free to! |
Although |
|
@csralvall That makes sense.. Eventually we would want to use the waste function to compare potential wastes between different coin selections.. It makes sense that it can be different than what BnB uses internally.. I think I understand the approach you are suggesting, although would be more clear when I see the code.. Feel free to update the PR (or make new one) and I would review.. |
I recently wrote a blog post about this, let me know if you have open questions, I'd be happy to incorporate feedback: https://murch.one/posts/waste-metric/
Yeah, that's actually a good point. So far, I don't see a reason why the two would diverge, but another contributor recently suggested that we could introduce additional heuristics based on privacy and confirmation-reliability to aid in prioritization of SelectionResults. These additional heuristics might only apply to the choice between the solutions from different algorithms rather than internally to BnB. |
|
Hey, we are in the process of releasing BDK 1.0, which will under the hood work quite differently from the current BDK. For this reason, I'm closing all the PRs that don't really apply anymore. If you think this is a mistake, feel free to rebase on master and re-open! (Specifically, the coin selection is being rewritten from scratch) |
6 participants
Implementation of the coin selection waste metric from bitcoin/bitcoin#22009
Later this can be used in the wallet to select the most optimal coin selection algo.
I plan on adding more of the tests but wanted to get this out there as I am new to rust and I am guessing I have some things I need to fix.