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).
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: