refactor: allow wallet to select fresh change address to prevent address reuse in coinjoin createdenoms #6870
Conversation
…ess reuse in coinjoin createdenoms
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
Walkthrough
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)src/**/*.{cpp,h,cc,cxx,hpp}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧬 Code graph analysis (1)src/coinjoin/util.cpp (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
🔇 Additional comments (2)
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 |
|
Please abandon this PR, it is not a contrived use case, but a very common one instead. Consider the following uses cases.
We have enough dust in wallets as it is. 🥺 Please don't change the current behaviour. |
…e address handling Added a new command-line option `-coinjoinfreshchange` to allow users to control whether change from CoinJoin denomination creation is sent to a fresh change address or back to the source address. This change improves privacy by preventing address reuse. Updated relevant documentation and code to support this feature.
|
@kxcd Consider the latest commit; behavior would be to retain the current legacy logic unless a user specifies in their config |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
-
doc/release-notes-6870.md(1 hunks) -
src/coinjoin/options.cpp(2 hunks) -
src/coinjoin/options.h(4 hunks) -
src/coinjoin/util.cpp(2 hunks) -
src/wallet/init.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/wallet/init.cppsrc/coinjoin/options.hsrc/coinjoin/options.cppsrc/coinjoin/util.cpp
doc/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the doc directory (documentation)
Files:
doc/release-notes-6870.md
🧬 Code graph analysis (4)
src/wallet/init.cpp (1)
src/dummywallet.cpp (1)
argsman(31-31)
src/coinjoin/options.h (1)
src/coinjoin/options.cpp (4)
Get(15-20)Get(15-15)SetFreshChange(64-68)SetFreshChange(64-64)
src/coinjoin/options.cpp (1)
src/qt/optionsmodel.cpp (2)
Init(50-335)Init(50-50)
src/coinjoin/util.cpp (1)
src/coinjoin/options.h (1)
GetFreshChange(64-64)
🪛 GitHub Actions: Clang Diff Format Check
src/coinjoin/util.cpp
[error] 1-1: Clang-format-diff detected format differences for src/coinjoin/util.cpp. The script reported differences; formatting needs to be applied. Command failed with exit code 1: git diff -U0 origin/develop --
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: arm-linux-build / Build source
🔇 Additional comments (4)
src/wallet/init.cpp (1)
108-108: New wallet arg exposure looks correctFlag is well-scoped under WALLET_COINJOIN and uses DEFAULT_COINJOIN_FRESHCHANGE.
doc/release-notes-6870.md (1)
1-9: Release note is clear and accurateAccurately describes default and behavior when enabled.
src/coinjoin/options.h (1)
34-34: Option surface and state additions look consistentDefault, getters/setters, and atomic storage align with existing options.
Also applies to: 64-65, 73-74, 92-93
src/coinjoin/options.cpp (1)
64-69: Fresh-change flag wiring is correct
Setter, Init() parsing, JSON exposure, and runtime usage align with existing patterns—no issues found.
src/coinjoin/util.cpp
Outdated
|
|
||
| #include <coinjoin/util.h> | ||
| #include <policy/fees.h> | ||
| #include <coinjoin/options.h> |
There was a problem hiding this comment.
❓ Verification inconclusive
Functional change LGTM; fix formatting to unblock CI
- Behavior toggle is correct: only set destChange when fresh-change is disabled; otherwise let wallet choose a fresh change address.
- CI is failing on clang-format for this file. Please run the project’s formatter to resolve.
To fix:
- Run contrib/devtools/clang-format-diff.py as in the CI message, or clang-format the file to project style.
Also, please sanity-check keypool usage with fresh-change enabled (one extra key per tx) to ensure no unexpected keypool depletion during CreateDenoms.
Also applies to: 129-134
Fix formatting in src/coinjoin/util.cpp to unblock CI
- Apply contrib/devtools/clang-format-diff.py (or run clang-format) on this file.
- Sanity-check keypool consumption with fresh-change enabled (one extra key per tx) to avoid depletion during CreateDenoms.
🤖 Prompt for AI Agents
In src/coinjoin/util.cpp around line 7, the file needs clang-format applied and
the keypool consumption logic must account for fresh-change using one extra key
per transaction; run contrib/devtools/clang-format-diff.py or clang-format on
this file to fix formatting, then update the sanity-check that verifies keypool
availability during CreateDenoms to reserve one additional key per tx when
fresh-change is enabled (or decrement the available key count by one per tx) so
the check reflects the true consumption and prevents depletion.
|
I don't see any value in having CJ specific change in a fresh new address tbh. It's pretty obvious that all outputs are yours and the only output with non-denom value is a change. No privacy is gained from using a fresh new address imo. Why would anyone use it? If it's simply for the sake of following "avoid reuse" principle then maybe we could do this only for wallets with |
|
"No privacy is gained from using a fresh new address imo." Agreed; the problem becomes, what if you have some dumb automation tied to the receiving of fund on an address. Your dumb automation just sees, "Oh another transaction came in to our address. Guess we should convert X dash to USDT and send it to the address on file", or whatever their automation is configured to do. |
… avoid_reuse setting Modified CoinJoin denomination creation to utilize the wallet's "avoid_reuse" setting. When enabled, change is sent to a fresh change address to prevent address reuse; otherwise, it defaults to the legacy behavior of returning change to the source address. Removed the now obsolete `-coinjoinfreshchange` option and updated relevant documentation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/coinjoin/options.cpp (1)
65-76: LGTM! Thread-safe singleton initialization.The Init() method correctly initializes the singleton using a static local variable (line 68), which guarantees thread-safe initialization in C++11 and later. The use of std::call_once in Get() (line 17) ensures Init() is called exactly once, and the assert (line 67) catches any programming errors. All command-line arguments are properly validated and clamped to safe ranges.
Optional refactors:
Prefer static_cast over C-style casts: Lines 70-74 use C-style casts
(int). For a C++20 codebase, consider usingstatic_cast<int>()for type safety, or remove the casts ifGetIntArg()already returnsint.Extract validation helper: The repetitive
std::min(std::max(...), ...)pattern on lines 70-74 could be extracted into a helper function to reduce duplication:template<typename T> T ClampValue(T value, T min_val, T max_val) { return std::min(std::max(value, min_val), max_val); }Then use it as:
instance.nCoinJoinSessions = ClampValue(gArgs.GetIntArg("-coinjoinsessions", DEFAULT_COINJOIN_SESSIONS), MIN_COINJOIN_SESSIONS, MAX_COINJOIN_SESSIONS);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
doc/release-notes-6870.md(1 hunks)src/coinjoin/options.cpp(1 hunks)src/coinjoin/util.cpp(2 hunks)src/wallet/init.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- doc/release-notes-6870.md
- src/wallet/init.cpp
- src/coinjoin/util.cpp
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/coinjoin/options.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
Co-authored-by: UdjinM6 <[email protected]>
3 participants
Issue being fixed or feature implemented
Coinjoin CreateDenoms will currently intentionally re-use the input address for change; I see no reason to do this, and it can be non-ideal for certain instances such as if you receive on X and are tracking that address X for further receipts. Say every time you receive on X you send to Y as a part of an exchange or something.
The only time I can image where it's not ideal is if you receive to X multiple times, maybe you receive 1.9 and 0.1 (contrived) in this case, say your first create denoms makes 1.9 -> 1 and 0.9; because that 0.9 is kept in address X, the second 0.1 can be combined with the 0.9 to make another 1 Dash denom.
What was done?
Use fresh change address instead of re-using
How Has This Been Tested?
NOT TESTED (@kwvg please test and confirm functionality of createdenoms)
Breaking Changes
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.