bitcoin icon indicating copy to clipboard operation
bitcoin copied to clipboard

Refactor BnB tests

Open murchandamus opened this issue 2 years ago • 23 comments

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

murchandamus avatar Mar 01 '24 22:03 murchandamus

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.

DrahtBot avatar Mar 01 '24 22:03 DrahtBot

Pinging @furszy, @achow101, @S3RK, as discussed

murchandamus avatar Apr 09 '24 12:04 murchandamus

Alright, should hopefully be ready to review.

murchandamus avatar Jun 27 '24 17:06 murchandamus

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.

murchandamus avatar Jun 27 '24 17:06 murchandamus

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

DrahtBot avatar Sep 11 '24 11:09 DrahtBot