refactor: carry the coin type in CoinSelectionParams - #7651
Conversation
CoinType is threaded through AttemptSelection and ChooseSelectionResult as a defaulted trailing parameter. Upstream keeps rewriting exactly these signatures (bitcoin#26661, bitcoin#27183), so every backport has to re-resolve the Dash-only argument, and a resolution that drops the trailing argument silently falls back to CoinType::ALL_COINS, losing the fully-mixed selection restrictions. Store the coin type in CoinSelectionParams instead, which is built once per selection request and already flows through the whole pipeline. The enum moves from coincontrol.h to coinselection.h so the field can be declared there without pulling the much heavier coincontrol.h into the selection module; coin control is a consumer of the selection layer, so the include direction stays sound.
…onResult The coin type now travels in CoinSelectionParams, so the Dash-only trailing parameter and its eleven call-site arguments are dead weight. Removing them restores both signatures to their upstream Bitcoin Core shape, which lets future coin-selection backports (bitcoin#26661, bitcoin#27183) apply without Dash-specific conflicts and removes the failure mode where a conflict resolution drops the trailing argument and silently selects with CoinType::ALL_COINS.
The three-argument overload only forwards to the four-argument one with CoinType::ONLY_READY_TO_MIX; a default argument expresses the same without a second method. Also include coincontrol.h directly in wallet/coinjoin.cpp, which uses CCoinControl but only received it transitively.
|
🕓 Ready for review — 3 ahead in queue (commit 22441a3) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. WalkthroughThe change moves Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The refactor carries CoinType through per-request selection parameters while preserving ready-to-mix and fully-mixed policies and existing validation and locking behavior; no actionable merge-blocking risk remains, so it is merge-ready after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
2 participants
Issue being fixed or feature implemented
CoinType is threaded through AttemptSelection and ChooseSelectionResult as a defaulted trailing parameter. Upstream keeps rewriting exactly these signatures (bitcoin#26661, bitcoin#27183), so every backport has to re-resolve the Dash-only argument, and a resolution that drops the trailing argument silently falls back to CoinType::ALL_COINS, losing the fully-mixed selection restrictions.
What was done?
Store the coin type in CoinSelectionParams instead, which is built once per selection request and already flows through the whole pipeline. The enum moves from coincontrol.h to coinselection.h so the field can be declared there without pulling the much heavier coincontrol.h into the selection module; coin control is a consumer of the selection layer, so the include direction stays sound.
It drops CoinType threading from AttemptSelection/ChooseSelectionResult
How Has This Been Tested?
Run unit & fuctional tests
Breaking Changes
N/A
Checklist: