Refactor BnB tests
This PR is splitting off some of the improvements made in #28985 and starts addressing the issues raised in #27754.
I aim to completely replace coinselector_tests with coinselection_tests. The goal is to generally use coins created per a nominal effective value so we can get away from testing with CoinSelectionParams that are non-representative and effectuate counterintuitive behavior such as feerate = 0 or cost_of_change = 0
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Code Coverage
For detailed information about the code coverage, see the test coverage report.
Reviews
See the guideline for information on the review process. A summary of reviews will appear here.
Conflicts
No conflicts as of last run.
Pinging @furszy, @achow101, @S3RK, as discussed
Alright, should hopefully be ready to review.
I'm not completely sure about 584e524. The commit description says
Originally these tests verified that at a SelectCoins level that a solution with fewer inputs gets preferred at high feerates, and a solution with more inputs gets preferred at low feerates. This outcome relies on the behavior of BnB, so we move these tests under the umbrella of BnB tests.
It is true that the outcome relies only on the BnB behavior currently but that might not be true in the future. There could be other algorithm clashing with it.
Yeah, so the old tests assumed that because BnB behaved a certain way, we would get a specific overall outcome. The new tests just check that BnB behaves a certain way. We might still want tests that test the overall outcome as a result from the interaction of multiple coin selection tests, but this one seemed clearly to be testing BnB behavior, and it seemed strange to me to be testing that at the level where the results are combined rather than checking that BnB assumptions are fulfilled by BnB.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/29957535397
Hints
Make sure to run all tests locally, according to the documentation.
The failure may happen due to a number of reasons, for example:
-
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
-
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
-
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.