refactor: couple simplifications for coinjoin's client implementation#7470
refactor: couple simplifications for coinjoin's client implementation#7470knst wants to merge 5 commits into
Conversation
|
✅ Final review complete — no blockers (commit a19dee0) |
WalkthroughCoinJoin masternode selection and used-node tracking now belong to Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/coinjoin/client.cpp (1)
901-935: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winFix
size_tunderflow and optimize unused masternode selection.If the number of enabled masternodes drops below the tracked used masternode count,
nCountNotExcludedwill underflow. This bypasses the< 1early exit, allocates and shuffles a vector unnecessarily, and logs a massive garbage integer for the remaining node count.Additionally, the current implementation copies and shuffles all enabled masternodes just to pick one unused node, requiring
O(N)random generations and swaps. Instead, filter the unused masternodes into a smaller vector and pick one element randomly using a singleGetRandcall.⚡ Proposed fix and optimization
CDeterministicMNCPtr CCoinJoinClientSession::GetRandomNotUsedMasternode() { auto mnList = m_dmnman.GetListAtChainTip(); size_t nCountEnabled = mnList.GetCounts().enabled(); - size_t nCountNotExcluded{nCountEnabled - m_mn_metaman.GetUsedMasternodesCount()}; + size_t nUsedCount = m_mn_metaman.GetUsedMasternodesCount(); + size_t nCountNotExcluded = (nCountEnabled > nUsedCount) ? (nCountEnabled - nUsedCount) : 0; WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::%s -- %d enabled masternodes, %d masternodes to choose from\n", __func__, nCountEnabled, nCountNotExcluded); if (nCountNotExcluded < 1) { return nullptr; } - // fill a vector - std::vector<CDeterministicMNCPtr> vpMasternodesShuffled; - vpMasternodesShuffled.reserve(nCountEnabled); - mnList.ForEachMNShared(/*onlyValid=*/true, - [&vpMasternodesShuffled](const auto& dmn) { vpMasternodesShuffled.emplace_back(dmn); }); - - // shuffle pointers - Shuffle(vpMasternodesShuffled.begin(), vpMasternodesShuffled.end(), FastRandomContext()); - - // loop through - using direct O(1) lookup instead of creating a set copy - for (const auto& dmn : vpMasternodesShuffled) { - if (m_mn_metaman.IsUsedMasternode(dmn->proTxHash)) { - continue; - } - - WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::%s -- found, masternode=%s\n", __func__, - dmn->proTxHash.ToString()); - return dmn; - } + std::vector<CDeterministicMNCPtr> unusedMasternodes; + unusedMasternodes.reserve(nCountNotExcluded); + mnList.ForEachMNShared(/*onlyValid=*/true, [&](const auto& dmn) { + if (!m_mn_metaman.IsUsedMasternode(dmn->proTxHash)) { + unusedMasternodes.emplace_back(dmn); + } + }); - WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::%s -- failed\n", __func__); - return nullptr; + if (unusedMasternodes.empty()) { + WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::%s -- failed\n", __func__); + return nullptr; + } + + size_t randomIndex = GetRand<size_t>(unusedMasternodes.size()); + auto dmn = unusedMasternodes[randomIndex]; + + WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::%s -- found, masternode=%s\n", __func__, + dmn->proTxHash.ToString()); + return dmn; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/coinjoin/client.cpp` around lines 901 - 935, Update CCoinJoinClientSession::GetRandomNotUsedMasternode to avoid size_t underflow by handling used-masternode counts greater than or equal to the enabled count before calculating the remaining count. Collect only valid, unused masternodes into the candidate vector, return nullptr when it is empty, and select one candidate with a single GetRand call instead of shuffling all enabled masternodes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/coinjoin/client.cpp`:
- Around line 901-935: Update CCoinJoinClientSession::GetRandomNotUsedMasternode
to avoid size_t underflow by handling used-masternode counts greater than or
equal to the enabled count before calculating the remaining count. Collect only
valid, unused masternodes into the candidate vector, return nullptr when it is
empty, and select one candidate with a single GetRand call instead of shuffling
all enabled masternodes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d17b456f-8d7a-4bd5-b98d-36d0202dee77
📒 Files selected for processing (6)
src/coinjoin/client.cppsrc/coinjoin/client.hsrc/interfaces/coinjoin.hsrc/qt/optionsdialog.cppsrc/qt/overviewpage.cppsrc/qt/overviewpage.h
💤 Files with no reviewable changes (2)
- src/interfaces/coinjoin.h
- src/qt/optionsdialog.cpp
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The CoinJoin ownership, logging, and constexpr changes are coherent, and the five-commit stack has clean permanent-history structure. However, the Qt cache move drops the prior options-dialog invalidation, so enabling Advanced CoinJoin UI during an active mix can expose a zero or stale progress bar until another block or balance change.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— dash-core-commit-history (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/qt/overviewpage.cpp`:
- [BLOCKING] src/qt/overviewpage.cpp:512-513: Invalidate progress when enabling the advanced UI
OptionsModel emits AdvancedCJUIChanged synchronously during mapper->submit(), and this handler changes the visibility mode before calling coinJoinStatus(true). During an active mix, coinJoinStatus only recomputes progress when the block height differs from nCachedNumBlocks; after a normal timer tick those values are equal. Because updateCoinJoinProgress() returns before calculating the advanced progress while advanced mode is disabled, enabling it can reveal the UI's default 0 value or an older value. Before this PR, OptionsDialog reset the client cache to max after submitting options, forcing the next one-second timer tick to recompute; wallet->markDirty() does not invalidate the new page-local cache or emit balanceChanged when balances are unchanged. Invalidate nCachedNumBlocks here or directly refresh the progress.
… its implementation
…nomResult in some instances - one instance of strAutoDenomResult was never read: getSessionStatuses() reads each session's GetStatus(), not the manager's copy - CheckAutomaticBackup collapsed to WalletCJLogPrint - Manager::DoAutomaticDenominating has better logging
6158ac1 to
a19dee0
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
Independent inspection confirms the prior Advanced CoinJoin UI cache blocker is fixed: coinJoinStatus(true) now resets nCachedNumBlocks to std::numeric_limits::max() (src/qt/overviewpage.cpp:524) before the height-throttle and progress-cache checks, and updateAdvancedCJUI() invokes this forced path synchronously, so no stale/default progress can be exposed. The five-commit CoinJoin/Qt refactor stack is clean and independently reviewable; one low-confidence commit-subject-length nitpick is retained since it is factually accurate (96 and 95 characters) though non-blocking.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— dash-core-commit-history (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— dash-core-commit-history (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (completed),claude-sonnet-5— general (completed)
💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
Independent fresh generation-g3 verification at exact head a19dee0 confirms the CoinJoin/Qt refactor stack has no correctness, security, or performance defects. The generation-1 Advanced CoinJoin UI cache blocker is fixed, and CodeRabbit's unused-masternode subtraction concern is pre-existing and outside this PR. One commit-history disclosure suggestion remains: commit b968667 is labeled as a structural move but also introduces the forced cache-invalidation behavior fix without mentioning it.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— dash-core-commit-history (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— dash-core-commit-history (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— dash-core-commit-history (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (completed)
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/qt/overviewpage.cpp`:
- [SUGGESTION] src/qt/overviewpage.cpp:522-524: Commit b968667 mixes a labeled refactor with an undisclosed behavior fix
Commit b968667b6553dce72d751583d06431c23ea7cf1a is titled as moving nCachedNumBlocks into OverviewPage, but its diff also adds the new `if (fForce) nCachedNumBlocks = std::numeric_limits<int>::max();` behavior that fixes stale progress after an Advanced CoinJoin UI toggle. Because Dash merges the commit stack without squashing, please amend the commit message to disclose the forced-invalidation fix so future blame and bisect users do not encounter a behavior change hidden behind a pure-refactor subject.
| // Forced updates are triggered by mode changes (advanced UI, CoinJoin or privacy toggled), | ||
| // the status has to be re-rendered even if the block height didn't change | ||
| if (fForce) nCachedNumBlocks = std::numeric_limits<int>::max(); |
There was a problem hiding this comment.
🟡 Suggestion: Commit b968667 mixes a labeled refactor with an undisclosed behavior fix
Commit b968667 is titled as moving nCachedNumBlocks into OverviewPage, but its diff also adds the new if (fForce) nCachedNumBlocks = std::numeric_limits<int>::max(); behavior that fixes stale progress after an Advanced CoinJoin UI toggle. Because Dash merges the commit stack without squashing, please amend the commit message to disclose the forced-invalidation fix so future blame and bisect users do not encounter a behavior change hidden behind a pure-refactor subject.
source: ['claude']
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If these PRs merge firstThis PR will likely need a rebase:
|
2 participants
Issue being fixed or feature implemented
That's a follow-up for #7259
What was done?
CCoinJoinClientManagerwhich is not used externally. Internal usages are replaced to longs on place.CCoinJoinClientSessionstill has localstrAutoDenomResultfor the same purpose.nCachedNumBlocksand related logic from coinjoin's interfaces::Coinjoin::Client directly toOverviewPage, the same ascachedNumISLocksand other relevant membersHow Has This Been Tested?
Run unit & functional tests
Breaking Changes
N/A
Checklist: