#36255 Refactor CoinGrinder tests

full analysis

https://github.com/bitcoin/bitcoin/pull/36255 · murchandamus · +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

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.

Data

dossier JSON · extract JSON · model openrouter/google/gemini-3.8-flash, generated 2026-09-17T15:59, confidence high, input hash d388a3c3e8bda0bc