#36255 Refactor CoinGrinder tests
https://github.com/bitcoin/bitcoin/pull/36255 · · +194/-274 in 2 files, 11 commits · labels: CI failed · draft
Goal
- Consolidate CoinGrinder tests into the modern test suite to reduce confusion across multiple test files
- Test coin selection with realistic feerates and effective values rather than obsolete dummy parameters
This PR migrates CoinGrinder tests from the legacy `coinselector_tests.cpp` to `coinselection_tests.cpp`. The migrated tests use effective values and realistic feerates instead of synthetic parameters, and simplifies the test for search exhaustion.
Problem: CoinGrinder unit tests were residing in the older coin selector test suite using obsolete dummy parameters that do not reflect production coin selection behavior, fragmenting wallet coin selection testing across multiple files.
Category: Wallet (#65 of 84)
P3 · cleanup
- P3 because it cleans up internal unit tests without impacting wallet users or node safety
- Better test fidelity and consolidation help maintainers navigate coin selection tests
Worthwhile internal test suite consolidation and maintenance. As yancyribbens noted, moving tests out of `coinselector_tests.cpp` helps resolve confusion between multiple test suites, and using realistic feerates and effective values improves test fidelity, though it does not impact end users or wallet safety.
Membership: Changes unit test files under src/wallet/test/ covering CoinGrinder coin selection.
Factors: security/stability 0, bug 0, performance 0, user value 0, leverage 1
Reviewability: Stale: CI failing
- CI is failing on the head commit
Continuous integration is reporting failure on the head commit.
Author status: Active; opened draft for initial colleague review with intentions to make minor tweaks.
Open concerns:
- Simplifying helper functions (HaveEquivalentInputs vs ranges permutation / direct comparisons)
- Replacing floating-point amount arithmetic with integer satoshi expressions
- Structuring test cases to run independently without shared assertion macros hiding failure lines
Agreement: Strong
- Strong concept support for consolidating tests and retiring the legacy test file
- Concept approval welcoming test cleanup to reduce confusion between test suites (yancyribbens)
- Concept approval with a branch of test simplification suggestions (l0rinc)
Strong concept support from reviewers who welcome retiring legacy coin selection test files, accompanied by simplification suggestions from l0rinc.
Both reviewers gave Concept ACKs with positive rationale, and review comments are focused on helpful refactoring suggestions rather than objections to the change.
- yancyribbens Concept ACKed: 'Thanks for following up on this. It would be nice to see the coinselector test file removed entirely in the future since having multiple test files has caused confusion.'
- l0rinc Concept ACKed and provided branch with test simplification ideas.
Review verdicts (DrahtBot): 0
- Concept ACK: l0rinc, yancyribbens
Dependencies
Enables:
- Eventual removal of legacy coinselector_tests.cpp
Files
468 lines under test/bench/ci.
- src/wallet/test/coinselector_tests.cpp +0/-253
- src/wallet/test/coinselection_tests.cpp +194/-21
Card
PR #36255 migrates the CoinGrinder unit tests from the older coinselector_tests.cpp into coinselection_tests.cpp, updating them to test with realistic feerates and effective values. This improves test suite maintainability and helps move toward retiring the redundant legacy coin selection test file. Reviewers support the concept with helpful code simplifications proposed, though CI is currently failing on head.