refactor: change GetMasternodeQuorumNodes return type from unordered_set to vector #6898
Conversation
…set to vector Updated the GetMasternodeQuorumNodes method to return a vector of NodeId instead of an unordered_set. This change simplifies the logic for filtering and transforming nodes, enhancing readability and maintainability. Additionally, adjusted related code in ProcessSigShare to accommodate the new return type.
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughThis pull request refactors the internal representation of quorum member node collections by replacing Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Rationale: The changes involve a public API modification affecting multiple call sites, container type changes with different semantic implications (set → vector introduces potential duplicates and ordering changes), and cross-file dependencies. While the logic modifications are not densely complex, the reviewer must verify that removing implicit deduplication from the set data structure does not introduce unintended duplicates in downstream logic, and confirm that all callers of 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 (3)
🧰 Additional context used📓 Path-based instructions (1)src/**/*.{cpp,h,cc,cxx,hpp}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🔇 Additional comments (5)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
| }; | ||
|
|
||
| for (NodeId id : m_nodes | ||
| | std::views::filter(IsMasternodeQuorumNode) |
There was a problem hiding this comment.
is std::views::filter & std::views::transform library or language feature?
I have concerns that this approach in long-term may make compilation time slower, if that's library feature; but I have no input at the moment.
There was a problem hiding this comment.
@coderabbitai please investigate and respond to this question
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
C++20 ranges views compilation time impact performance
💡 Result:
Short answer: C++20 ranges/views commonly increase compile times (sometimes noticeably) but usually have no runtime cost if the compiler inlines and optimizes the pipeline; the trade-off is heavier template/headers and more work for the compiler. Suggested mitigations: limit exposure (compile boundaries/PCH), prefer MSVC or up-to-date toolchains, avoid overly-deep templated pipelines in hot build paths, and use type-erasure or hand-written loops where compile-time cost matters. [1][2][3][4]
Supporting points and sources
- Compile-time impact: std::ranges and range-v3 are template‑heavy and cause larger headers/longer compile times; reports show anything from small (≈10%) to large slowdowns and examples where including in C++20 mode expanded include chains and slowed compilation. [2][3][4]
- Runtime cost: views are designed to be zero‑overhead (lazy, inlinable pipelines) and in typical cases compile to equivalent iterator/loop code; benchmarks often show similar runtime performance to equivalent iterator-based code. [1][5]
- Variability: compile-time and runtime behavior depend heavily on compiler (GCC/Clang vs MSVC), standard-library implementation, and how complex the composed views/closures are — some implementations (range-v3, older libstdc++) were worse for compile time than modern std::ranges implementations. [2][3]
- Mitigations: use precompiled headers, hide ranges-heavy code behind translation-unit boundaries (compile-time walls), update compilers/stdlibs, avoid huge composed pipelines in headers, or use runtime type-erasure for APIs where compile cost dominates. Tools like -ftime-report (GCC) can show where compile time is spent. [6][3]
Key references
- Practical benchmarks / discussion (build & runtime comparisons). [1]
- Fluent C++ on limitations and compile-time cost of range-v3. [2]
- Reports about larger headers and measurable compile-time increases when C++20 ranges are enabled. [3]
- Design/behaviour discussion of std::ranges (constness/caching, performance implications). [4]
- Benchmarks showing runtime parity with iterator/algorithm code. [5]
- GCC compile-time analysis / tooling (-ftime-report). [6]
If you want, I can: (a) run or find specific microbenchmarks comparing compile time and runtime on GCC/Clang/MSVC for your code size, or (b) give concrete recipes (PCH, TU boundaries, example refactors) to reduce build-time impact.
@knst Great question! Let me address both aspects:
Library vs Language Feature
std::views::filter and std::views::transform are library features, specifically part of the C++20 Ranges library in the standard library. They are not core language features but rather template-based abstractions provided by <ranges>.
Compilation Time Impact
Your concern about compilation time is well-founded. Based on current research:
The Reality:
- C++20 ranges/views commonly increase compile times (sometimes noticeably) but usually have no runtime cost if the compiler inlines and optimizes the pipeline
- Reports show anything from small (≈10%) to large slowdowns depending on the complexity of the pipeline and compiler version
- The template-heavy nature of ranges means more work for the compiler during instantiation
Mitigating Factors for This Specific Case:
- The usage in this PR is relatively simple (just filter + transform), not a deeply nested pipeline
- It's in a
.cppfile, not a header, so the compilation impact is localized tonet.cpponly - Modern compilers (up-to-date GCC, Clang, MSVC) handle ranges better than older versions
Trade-offs:
- Compilation time: Likely a modest increase in
net.cppcompile time - Runtime performance: Views are designed to be zero-overhead (lazy, inlinable pipelines) and in typical cases compile to equivalent iterator/loop code
- Code readability: More declarative and easier to understand the intent
Recommendation:
Given that this is localized to a single translation unit and uses a simple pipeline, the compilation time impact should be manageable. If it becomes a concern during actual builds, the team could consider measuring it or reverting to the traditional loop approach. The readability benefits likely outweigh the modest compilation time cost in this case.
4 participants
Issue being fixed or feature implemented
Updated the GetMasternodeQuorumNodes method to return a vector of NodeId instead of an unordered_set. This change simplifies the logic for filtering and transforming nodes, enhancing readability and maintainability. Additionally, adjusted related code in ProcessSigShare to accommodate the new return type.
It's better to use a vector over a set, as all users are just iterating through this. They don't actually benefit from the O(1) lookup, so let's get the better benefits of cache locality.
What was done?
How Has This Been Tested?
Breaking Changes
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.