Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Mar 11, 2022

Conversation

glozow
Copy link
Member

@glozow glozow commented Mar 11, 2022

#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.

…ectionWaste()

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.
@glozow
Copy link
Member Author

glozow commented Mar 11, 2022

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.

@achow101
Copy link
Member

ACK ec7d736

Copy link
Contributor

@murchandamus murchandamus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ec7d736

@achow101 achow101 merged commit c109e7d into bitcoin:master Mar 11, 2022
@glozow glozow deleted the 2022-03-bnb-waste branch March 11, 2022 17:14
@bitcoin bitcoin locked and limited conversation to collaborators Mar 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants