wallet: avoid knapsack when there's no change #17246
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
I don't think this is really the right approach to allowing subtractFeeFromOutputs. Instead of passing along subtractFeeFromOutputs to BnB, I think it makes more sense to just change the effective_values of each input to be nValue and change the non_input_fees to 0, BnB doesn't need to know whether subtractFeeFromOutputs is being used. I can work on this as an alternative if you'd like. |
|
Happy to see an alternative approach. Ideally though, I think BnB should know about |
BnB should already be minimizing transaction fees as part of reducing "waste". |
|
If the effective value of inputs is equal to their nominal value then, afaik, waste reduction only tries to find the smallest possible set of coins. It won't optimize for which coins are cheapest to spend based on (witness) size. |
|
I looked more closely at your changes, and it seems like you've actually done what I suggested, just in a more verbose way. It could be done with fewer changes. |
|
That's probably because I went all over the place while trying to get this to work, and chased down a few other rabbit holes (like trying to make BnB find a solution with |
This allows skipping the knapsack solver when we can't generate a change address.
6c73d97 to
d29efe6
Compare
|
Closing in favor of #17290, which is simpler and achieves more:
|
…t fee from outputs b007efd Allow BnB when subtract fee from outputs (Andrew Chow) db15e71 Use BnB when preset inputs are selected (Andrew Chow) Pull request description: Currently we explicitly disable BnB when there are preset inputs selected or when the subtract fee from outputs option is enabled. This PR enables BnB for both cases. Kind of an alternative to #17246 (implements the subtract fee from outputs part of it) and borrows a test from there too. ACKs for top commit: instagibbs: reACK b007efd Sjors: re-ACK b007efd Tree-SHA512: 933276b09b2fa2ab43db7f0b98762f06f6f5fa8606195f96aca9fa1cb71ae4ee7156028dd482b1cada82ddd0996a9daf12ea5c152589fdf192cd96cbc51e99df
…subtract fee from outputs b007efd Allow BnB when subtract fee from outputs (Andrew Chow) db15e71 Use BnB when preset inputs are selected (Andrew Chow) Pull request description: Currently we explicitly disable BnB when there are preset inputs selected or when the subtract fee from outputs option is enabled. This PR enables BnB for both cases. Kind of an alternative to bitcoin#17246 (implements the subtract fee from outputs part of it) and borrows a test from there too. ACKs for top commit: instagibbs: reACK bitcoin@b007efd Sjors: re-ACK b007efd Tree-SHA512: 933276b09b2fa2ab43db7f0b98762f06f6f5fa8606195f96aca9fa1cb71ae4ee7156028dd482b1cada82ddd0996a9daf12ea5c152589fdf192cd96cbc51e99df
…subtract fee from outputs b007efd Allow BnB when subtract fee from outputs (Andrew Chow) db15e71 Use BnB when preset inputs are selected (Andrew Chow) Pull request description: Currently we explicitly disable BnB when there are preset inputs selected or when the subtract fee from outputs option is enabled. This PR enables BnB for both cases. Kind of an alternative to bitcoin#17246 (implements the subtract fee from outputs part of it) and borrows a test from there too. ACKs for top commit: instagibbs: reACK bitcoin@b007efd Sjors: re-ACK b007efd Tree-SHA512: 933276b09b2fa2ab43db7f0b98762f06f6f5fa8606195f96aca9fa1cb71ae4ee7156028dd482b1cada82ddd0996a9daf12ea5c152589fdf192cd96cbc51e99df
Builds on top of #17219
Avoid knapsack when there's no change, by enabling BnB when subtracting fees from outputs. Gotchas:
subtractFeesFromOutputsis generally used to spend either all coins or a selected bunch. But there is an edge case where e.g. it can choose between two 0.01 BTC inputs, one SegWit and one legacy, and it won't care.