fix: address review feedback for CoinJoin promotion/demotion (PR #7052)#44
Conversation
Add IsSpent/IsLockedCoin filtering to both CountCoinsByDenomination() and SelectFullyMixedForPromotion() to prevent concurrent CoinJoin sessions from selecting the same UTXOs. This matches the existing filtering pattern used in the denomination selector. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add bounds check for outpoint.n before accessing wtx.tx->vout in JoinExistingQueue and StartNewQueue promotion paths. This prevents potential out-of-bounds access if wallet state changes between UTXO selection and input construction. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Wrap m_chainman.ActiveChain().Tip() accesses in LOCK(::cs_main) in ProcessDSVIN and AddEntry to prevent race conditions during V24 activation state checks. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The full-session finalization path in CheckPool was creating the final transaction without verifying the minimum standard mixer count. This could allow a session with only promotion/demotion entries (which don't provide privacy) to finalize. Add the same GetStandardEntriesCount() check that the timeout path already uses. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…InOuts Replace the inline promotion/demotion validation in IsValidInOuts() with calls to the dedicated CoinJoin::ValidatePromotionEntry() and CoinJoin::ValidateDemotionEntry() functions. These functions were already declared, implemented, and tested but never called from production code. This ensures the tested code IS the production code. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The member stores inputs for both promotion AND demotion operations, so the name m_vecPromotionInputs was misleading. Rename to m_vecRebalanceInputs to better reflect its dual purpose. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
PastaPastaPasta
merged commit 332186c
into
PastaPastaPasta:feat/coinjoin-promotion
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc50997142
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (fConsumeCollateralRet) *fConsumeCollateralRet = true; | ||
| return false; | ||
| } | ||
| nExpectedInputDenom = CoinJoin::GetLargerAdjacentDenom(nSessionDenom); |
There was a problem hiding this comment.
This path now sets nExpectedInputDenom from GetLargerAdjacentDenom(nSessionDenom) without checking for 0, so a demotion-shaped entry (1 input, 10 outputs) in a session already at the largest denom can pass structure/output validation and then accept a non-denominated input (AmountToDenomination == 0) if values balance. Before this commit, the explicit nLargerAdjacentDenom == 0 guard rejected this case, so this is a regression that allows an invalid demotion mode and weakens the mixing rules for largest-denom sessions.
Useful? React with 👍 / 👎.
2 participants
Address review feedback from PR dashpay#7052 code review.
Changes
fix: enforce standard mixer minimum on full-session finalization
The full-session finalization path in
CheckPoolwas creating the final transaction without verifying the minimum standard mixer count. This could allow a session with only promotion/demotion entries (which don't provide privacy) to finalize. Adds the sameGetStandardEntriesCount()check that the timeout path already uses.fix: acquire cs_main for V24 activation checks in server
Two locations access
ActiveChain().Tip()without holdingcs_main, creating a race with tip updates/reorgs. Wraps both inLOCK(::cs_main).fix: add bounds checking for outpoint.n in coinjoin client
Adds
outpoint.n < wtx.tx->vout.size()bounds check before accessing wallet transaction outputs, preventing potential out-of-bounds access from corrupted outpoints.fix: add IsLockedCoin/IsSpent check in SelectFullyMixedForPromotion
SelectFullyMixedForPromotion()was iteratingsetWalletUTXOwithout checkingIsLockedCoin()orIsSpent(), enabling double-reservation of coins.refactor: use ValidatePromotionEntry/ValidateDemotionEntry in IsValidInOuts
Replaces inline promotion/demotion validation in
IsValidInOuts()with calls to the dedicatedCoinJoin::ValidatePromotionEntry()andCoinJoin::ValidateDemotionEntry()functions. These were already declared, implemented, and tested but never called from production code.refactor: rename m_vecPromotionInputs to m_vecRebalanceInputs
The member stores inputs for both promotion AND demotion operations, so
m_vecPromotionInputswas misleading. Renamed tom_vecRebalanceInputsto better reflect its dual purpose.