wallet: assert BnB's internally calculated waste is the same as GetSelectionWaste #24530

pull glozow wants to merge 1 commits into bitcoin:master from glozow:2022-03-bnb-waste changing 2 files +2 −1

glozow commented at 12:32 PM on March 11, 2022: member

#22009 introduced a GetSelectionWaste() function to determine how much "waste" a coin selection solution has, and is a mirror of the waste calculated inside of SelectCoinsBnB(). It would be bad for these two waste metrics to deviate, since it could negatively affect how often we select the BnB solution. Add an assertion to help tests catch a potential accidental change.

[wallet] assert BnB internally calculated waste is the same as GetSelectionWaste()

These two implementations of waste calculation should never deviate.
Still keep the SelectCoinsBnB internal calculation because incremental
calculate-as-you-go is much more performant than calling
GetSelectionWaste() over and over again.

ec7d73628a

glozow commented at 12:33 PM on March 11, 2022: member

At first, I thought it would be a good idea to use GetSelectionWaste() for the current selection inside SelectCoinsBnB, but @achow101 explained that this would involve a lot of repeated work going over the selected coins and hurt the performance.
fanquake added the label Wallet on Mar 11, 2022
achow101 commented at 2:29 PM on March 11, 2022: member

ACK ec7d73628a6397fca3b5b852d4e97ff918b6d3a6
achow101 requested review from Xekyo on Mar 11, 2022
Xekyo commented at 3:31 PM on March 11, 2022: member

I was wondering whether this would break when BnB does not find a solution, but realized that it returns earlier in this case. Change looks good to me.

ACK ec7d73628a6397fca3b5b852d4e97ff918b6d3a6
achow101 merged this on Mar 11, 2022
achow101 closed this on Mar 11, 2022
glozow deleted the branch on Mar 11, 2022
DrahtBot locked this on Mar 11, 2023

Contributors

Labels

Linked (view graph)

#22009 wallet: Decide which coin selection solution to use based on waste metric