chore: consolidate oft-repeated type definitions to {evo,llmq}/types.h
#6839
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
kwvg
force-pushed
the
types_h
branch
3 times, most recently
from
e88f9cd to
afc9844
Compare
|
This pull request has conflicts, please rebase. |
kwvg
changed the title
llmq/types.hevo/types.h
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/llmq/utils.cpp (2)
515-554: Bug: sentinel value 0 corrupts skipList encoding when first skipped index is 0 (and wrap-around deltas can go negative)
- Using 0 as “uninitialized” collides with a valid first index of 0.
- Computing idx - firstSkippedIndex as size_t under wrap-around yields huge values; later decoding expects small non-negative deltas.
This mis-encodes skipList and breaks GetQuorumQuarterMembersBySnapshot decoding in edge cases.
Proposed fix: use a distinct sentinel and modulo-safe deltas.
- std::vector<int> skipList; - size_t firstSkippedIndex = 0; + std::vector<int> skipList; + size_t firstSkippedIndex = std::numeric_limits<size_t>::max(); size_t idx{0}; @@ - if (skip) { - if (firstSkippedIndex == 0) { - firstSkippedIndex = idx; - skipList.push_back(idx); - } else { - skipList.push_back(idx - firstSkippedIndex); - } - } + if (skip) { + if (firstSkippedIndex == std::numeric_limits<size_t>::max()) { + firstSkippedIndex = idx; + skipList.push_back(static_cast<int>(idx)); + } else { + // modulo-safe forward distance in ring [0, N) + const auto N = sortedCombinedMnsList.size(); + const auto delta = (idx + N - firstSkippedIndex) % N; + skipList.push_back(static_cast<int>(delta)); + } + }Also update the decoder accordingly (see next comment).
675-701: Fix: decode skipList with modulo arithmetic and correct typo
- MODE_SKIPPING_ENTRIES decoding assumes non-wrapping deltas; apply modulo to support ring indexing and the encoding change above.
- Typo: processesdSkipList → processedSkipList.
- size_t first_entry_index{0}; - std::vector<int> processesdSkipList; - for (const auto& s : snapshot.mnSkipList) { - if (first_entry_index == 0) { - first_entry_index = s; - processesdSkipList.push_back(s); - } else { - processesdSkipList.push_back(first_entry_index + s); - } - } + bool first = true; + size_t base = 0; + std::vector<int> processedSkipList; + const auto N = sortedCombinedMns.size(); + for (const auto& s : snapshot.mnSkipList) { + if (first) { + base = static_cast<size_t>(s) % N; + processedSkipList.push_back(static_cast<int>(base)); + first = false; + } else { + const auto abs_idx = (base + static_cast<size_t>(s)) % N; + processedSkipList.push_back(static_cast<int>(abs_idx)); + } + } @@ - auto itsk = processesdSkipList.begin(); + auto itsk = processedSkipList.begin();
🧹 Nitpick comments (14)
src/evo/creditpool.cpp (2)
71-73: Switch to Uint256LruHashMap is good; make dependency explicitcreditpool.cpp uses Uint256LruHashMap but relies on a transitive include. Include evo/types.h here to decouple from headers order.
#include <evo/specialtx.h> +#include <evo/types.h>
73-74: Prefer consistent LOCK usage styleElsewhere in this file you use LOCK as a separate statement; consider the same here for consistency and to avoid depending on C++17-if-initializer + macro semantics.
- if (LOCK(cache_mutex); block_data_cache.get(block_index->GetBlockHash(), blockData)) { + LOCK(cache_mutex); + if (block_data_cache.get(block_index->GetBlockHash(), blockData)) { return blockData; }src/llmq/dkgsession.h (1)
10-13: Consistent hashing/alias usage; consider set alias, too.Includes and usage align with the new centralized types. If you plan full unification, consider adding a Uint256HashSet (or similar) alias to evo/types.h and using it for relayMembers.
Also applies to: 18-18
src/evo/creditpool.h (1)
17-19: Remove redundant unordered_lru_cache include
src/evo/creditpool.h can drop#include <unordered_lru_cache.h>—evo/types.h already includes it and definesUint256LruHashMap.src/llmq/blockprocessor.h (1)
13-18: Remove redundant include of<unordered_lru_cache.h>
TheUint256LruHashMapalias is provided by<evo/types.h>, so the extra header can be dropped.src/evo/cbtx.cpp (1)
7-13: Includes: good; add missing explicit dependency on sync.h.File uses Mutex/LOCK but doesn’t include <sync.h> directly. Avoid relying on transitive includes.
Apply:
#include <deploymentstatus.h> #include <node/blockstorage.h> #include <evo/specialtx.h> #include <evo/types.h> +#include <sync.h>Also applies to: 15-15
src/instantsend/signing.h (2)
68-71: Pointer stability risk remains for map-of-pointers; consider avoiding raw pointers.txToCreatingInstantSendLocks stores InstantSendLock* pointing into creatingInstantSendLocks; rehash of creatingInstantSendLocks can invalidate these pointers. Not new, but worth addressing.
- Short-term: ensure no insert/erase on creatingInstantSendLocks occurs between pointer capture and use, or reserve buckets up-front.
- Safer: store stable iterators by switching storage to std::list and mapping to list::iterator.
Example declaration-only change:
-Uint256HashMap<InstantSendLock> creatingInstantSendLocks GUARDED_BY(cs_creating); -Uint256HashMap<InstantSendLock*> txToCreatingInstantSendLocks GUARDED_BY(cs_creating); +std::list<InstantSendLock> creatingInstantSendLocks GUARDED_BY(cs_creating); +Uint256HashMap<std::list<InstantSendLock>::iterator> txToCreatingInstantSendLocks GUARDED_BY(cs_creating);Would you like a follow-up patch for the .cpp adjustments?
8-8: Add saltedhasher include for explicit dependency.This header declares function parameters using StaticSaltedHasher; add the include to avoid transitive reliance.
#include <evo/types.h> +#include <saltedhasher.h> #include <instantsend/lock.h>src/llmq/utils.cpp (3)
202-207: Fix: formatting break causes CI failureClang diff check flags this region. Please run clang-format (or apply the repo’s format script) to unblock CI.
228-231: Prefer WITH_LOCK over “if (LOCK(...); cond)” to avoid macro-dependent compilation gotchasUsing LOCK in an if-initializer relies on macro expansion details and is easy to break. Use WITH_LOCK for the condition or scope a plain LOCK in a block.
Apply one of:
- if (LOCK(cs_indexed_members); mapIndexedQuorumMembers.empty()) { - InitQuorumsCache(mapIndexedQuorumMembers); - } + if (WITH_LOCK(cs_indexed_members, mapIndexedQuorumMembers.empty())) { + InitQuorumsCache(mapIndexedQuorumMembers); + }- } else if (LOCK(cs_indexed_members); mapIndexedQuorumMembers[llmqType].get(std::pair(pCycleQuorumBaseBlockIndex->GetBlockHash(), quorumIndex), quorumMembers)) { + } else if (WITH_LOCK(cs_indexed_members, mapIndexedQuorumMembers[llmqType].get(std::pair(pCycleQuorumBaseBlockIndex->GetBlockHash(), quorumIndex), quorumMembers))) {Also applies to: 252-258
968-983: Question: do we need these explicit template instantiations here?They’re only useful if InitQuorumsCache is intentionally defined in this TU and called for these exact CacheType instantiations elsewhere. If not, remove to reduce object size and compile time; if yes, consider adding matching extern template declarations in a header for clarity.
src/evo/types.h (1)
8-12: Consider including <uint256.h> to avoid reliance on transitive includesAliases compile only when uint256 is complete at instantiation sites. If some TU includes evo/types.h but not uint256.h, builds may fail. Including uint256.h here avoids that.
#include <unordered_lru_cache.h> #include <memory> #include <unordered_map> +#include <uint256.h>src/instantsend/instantsend.cpp (1)
235-237: Ensure evo/types.h is visible in this TUIf instantsend.h doesn’t include evo/types.h, add it here to satisfy the new aliases.
#include <instantsend/signing.h> #include <llmq/commitment.h> #include <llmq/quorums.h> +#include <evo/types.h>src/llmq/quorums.h (1)
243-249: Caches migrated to Uint256LruHashMap — consistent with PR goals.
Readability improves; semantics unchanged.Consider a helper alias to de-duplicate the map<LLMQType, Uint256LruHashMap> pattern:
+// in a header (e.g., evo/types.h) if broadly useful +template <typename T, size_t Max = 30000> +using QuorumTypeLruMap = std::map<Consensus::LLMQType, Uint256LruHashMap<T, Max>>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
src/Makefile.am(1 hunks)src/chainlock/chainlock.h(2 hunks)src/coinjoin/client.h(1 hunks)src/evo/cbtx.cpp(2 hunks)src/evo/creditpool.cpp(1 hunks)src/evo/creditpool.h(2 hunks)src/evo/deterministicmns.h(2 hunks)src/evo/mnhftx.h(2 hunks)src/evo/types.h(1 hunks)src/instantsend/db.cpp(2 hunks)src/instantsend/db.h(3 hunks)src/instantsend/instantsend.cpp(3 hunks)src/instantsend/instantsend.h(4 hunks)src/instantsend/signing.h(2 hunks)src/llmq/blockprocessor.h(2 hunks)src/llmq/commitment.h(1 hunks)src/llmq/dkgsession.cpp(2 hunks)src/llmq/dkgsession.h(1 hunks)src/llmq/dkgsessionmgr.cpp(2 hunks)src/llmq/quorums.h(2 hunks)src/llmq/signing.h(4 hunks)src/llmq/signing_shares.cpp(6 hunks)src/llmq/signing_shares.h(5 hunks)src/llmq/snapshot.h(2 hunks)src/llmq/utils.cpp(3 hunks)src/llmq/utils.h(1 hunks)src/net.cpp(2 hunks)src/qt/masternodelist.h(1 hunks)src/spork.h(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/evo/specialtx.h : Special transactions use payload extensions defined in src/evo/specialtx.h
Applied to files:
src/Makefile.amsrc/evo/cbtx.cppsrc/evo/mnhftx.hsrc/evo/deterministicmns.h
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
Applied to files:
src/net.cppsrc/qt/masternodelist.h
📚 Learning: 2024-12-29T17:43:41.755Z
Learnt from: kwvg
PR: dashpay/dash#6504
File: src/llmq/quorums.cpp:224-224
Timestamp: 2024-12-29T17:43:41.755Z
Learning: The `CQuorumManager` is fully initialized by `LLMQContext`, addressing any concerns about the manager’s initialization sequence.
Applied to files:
src/llmq/signing.h
📚 Learning: 2025-01-07T18:50:44.838Z
Learnt from: knst
PR: dashpay/dash#6511
File: src/evo/deterministicmns.cpp:1369-1373
Timestamp: 2025-01-07T18:50:44.838Z
Learning: The functions `MigrateDBIfNeeded` and `MigrateDBIfNeeded2` in `src/evo/deterministicmns.cpp` are temporary and will be removed in a future version. Refactoring suggestions for these functions should be avoided.
Applied to files:
src/evo/deterministicmns.h
📚 Learning: 2025-01-02T21:50:00.967Z
Learnt from: kwvg
PR: dashpay/dash#6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the `CInstantSendManager`. Previously, this was handled globally; now it's handled as a class member in `LLMQContext`, but the concurrency control remains consistent.
Applied to files:
src/instantsend/instantsend.h
🪛 GitHub Actions: Clang Diff Format Check
src/llmq/utils.cpp
[error] 200-207: Clang format differences found. The clang-format-diff check failed (exit code 1) due to formatting changes in src/llmq/utils.cpp around line 200. Run 'clang-format -i' to fix the formatting.
🔇 Additional comments (55)
src/Makefile.am (1)
207-207: Exporting evo/types.h via BITCOIN_CORE_H looks correctThis ensures downstream modules rebuild on changes to the new shared types header. No issues spotted.
src/evo/mnhftx.h (2)
8-18: Header reorg is fine; necessary deps presentAdding evo/types.h and unordered_lru_cache.h here is appropriate for the new aliases. No functional concerns.
103-104: Cache type alias migration LGTMReplacing unordered_lru_cache<uint256,...> with Uint256LruHashMap improves readability and avoids hasher duplication; thread-safety annotation is retained.
src/qt/masternodelist.h (1)
12-12: Good consolidation: rely on CDeterministicMNCPtr from evo/types.hSwitching to the central alias keeps the UI header lean and consistent across modules. No issues spotted.
src/llmq/commitment.h (1)
13-16: No include cycle detected in evo/types.h
evo/types.h forward-declares llmq::CFinalCommitment and defines CFinalCommitmentPtr without including llmq/commitment.h.src/coinjoin/client.h (1)
13-16: Include and alias cleanup looks good.Using evo/types.h and removing duplicate forward/alias definitions reduces churn and keeps the public surface consistent.
src/spork.h (1)
17-17: OK to include evo/types.h here.Keeps spork containers consistent with the project-wide aliasing.
src/llmq/snapshot.h (1)
14-19: LRU cache alias: verify constructor compatibility.Switch to Uint256LruHashMap is fine; confirm the alias ultimately wraps unordered_lru_cache<uint256, T, StaticSaltedHasher> so the size-only constructor (32) remains valid and hashing stays salted.
Suggested guard in evo/types.h (or a dedicated cpp):
static_assert(std::is_constructible_v<Uint256LruHashMap<int>, size_t>, "Uint256LruHashMap must expose size_t-capacity constructor");Also applies to: 221-221
src/chainlock/chainlock.h (1)
17-17: StaticSaltedHasher presence verified Uint256HashMap alias retains StaticSaltedHasher for secure hashing.src/llmq/dkgsession.cpp (2)
7-16: Include reorg + use of evo/types.h is fine.No functional changes; keeps this TU consistent with the new centralized types.
Also applies to: 24-25
474-474: Local alias usage LGTM.Using Uint256HashMap for protoMap is a clean drop-in replacement.
src/llmq/utils.h (1)
12-17: Alias migration confirmed: CDeterministicMNCPtr is defined only in src/evo/types.h; no duplicate aliases remain.src/evo/creditpool.h (1)
115-115: Alias swap to Uint256LruHashMap aligns with PR goal.Member initializer with capacity preserved; thread-safety annotation retained. No concerns.
src/llmq/blockprocessor.h (1)
55-55: Internal cache type migrated to Uint256LruHashMap: LGTM.No API changes; mutex/annotation unchanged. Migration is consistent with types.h consolidation.
src/instantsend/db.cpp (2)
149-152: try_emplace usage is appropriate.Moving the shared_ptr and avoiding an extra lookup is fine here.
119-137: Approve alias swap to Uint256HashMap—no remaining std::unordered_map references
All callers ofRemoveConfirmedInstantSendLocksnow useUint256HashMap<InstantSendLockPtr>, and no instances of the oldstd::unordered_map<uint256, InstantSendLockPtr>remain.src/net.cpp (2)
43-45: Including evo/types.h early is fine and makes the aliases available.No ordering hazards visible with existing includes.
3398-3403: Switch to Uint256HashMap is consistent; semantics preserved.
emplace,count, andoperator[]usage remain valid. The stored value matches the inline comment (/*fInbound*/bool).src/llmq/dkgsessionmgr.cpp (2)
14-22: Header reorg + evo/types.h include: OK.No duplicate/conflicting includes; keeps llmq/evo deps localized.
101-102: Default LRU cache parameters unchanged The Uint256LruHashMap alias uses MaxSize=0 and TruncateThreshold=0 by default, identical to unordered_lru_cache’s defaults.src/instantsend/db.h (3)
13-13: New evo/types.h include is necessary for aliases.Good placement near other core headers.
40-42: Cache types migrated to Uint256LruHashMap with explicit capacity: good.Capacity (10000) preserved; visibility and guards unchanged.
105-105: Update docstring for RemoveConfirmedInstantSendLocks return typeApply the following diff in src/instantsend/db.h:
- * @return returns an unordered_map of the hash of the IS Locks and a pointer object to the IS Locks for all IS Locks which were removed + * @return returns a map keyed by uint256 of IS lock hashes to pointers for all removed IS Lockssrc/evo/cbtx.cpp (1)
63-63: Resolve: alias preserves InitQuorumsCache semantics. Uint256LruHashMap is a direct alias to unordered_lru_cache with the same constructor signature (taking a single size_t max-size, defaulting TruncateThreshold to 0), and InitQuorumsCache emplaces each entry with the intended keepOldConnections/keepOldKeys value, so capacity and initialization behavior remain unchanged.src/instantsend/signing.h (1)
8-8: Importing evo/types.h here is correct.Centralizing uint256-keyed containers via the alias improves consistency.
src/evo/deterministicmns.h (2)
18-23: Include order/readability: evo/types.h addition is appropriate; ensure forward decls exist in types.h.Since CDeterministicMNCPtr is now defined in evo/types.h, that header must forward-declare CDeterministicMN (and any llmq pointer targets) to avoid circular includes.
If missing, add forward declarations inside evo/types.h:
+class CDeterministicMN; +namespace llmq { class CQuorum; struct CFinalCommitment; }
631-633: Cache maps migrated to Uint256HashMap: LGTM.Thread-safety annotations unchanged; semantics align with prior unordered_map usage.
src/evo/types.h (1)
21-33: LGTM: centralizing frequently used aliasesThe pointer and 256-bit keyed container aliases are clear and consistent with existing patterns.
src/instantsend/instantsend.cpp (3)
233-241: Signature and map alias change: OKSwitch to Uint256HashMap for pend/recSigs is compatible and improves consistency.
692-713: LGTM: using Uint256HashMap for toDelete reduces boilerplateNo behavioral changes; keeps the salted hasher.
722-744: LGTM: nested Uint256HashMap for conflicts is clearRetains outer key as CBlockIndex* while standardizing inner map.
src/llmq/signing.h (5)
15-17: Good include consolidation; evo/types.h early is correct.
Ensures alias visibility before use and avoids cyclic deps.
20-21: GSL include placement looks fine.
No ABI impact; consistent with rest of file.
34-40: Forward decls + DEFAULT_MAX_RECOVERED_SIGS_AGE added appropriately.
Clearer public surface; constant choice matches existing semantics.
168-169: pendingReconstructedRecoveredSigs: migration to Uint256HashMap looks good.
No ordering assumptions; guarded by cs_pending as before.
118-120: Alias uses StaticSaltedHasher; no action needed.
BothUint256LruHashMapandUint256HashMapaliases insrc/evo/types.huseStaticSaltedHasher, preserving DoS-resistance.src/llmq/quorums.h (3)
8-11: Header hygiene improvements.
Bringing protocol/saltedhasher/thread/time in here reduces hidden deps.
15-18: Adding evo/types.h and llmq/params.h is appropriate.
Enables use of new aliases without local forward decls.
252-252: quorumBaseBlockIndexCache size param explicit — good.
Fits mainnet sizing comment; no concerns.src/llmq/signing_shares.cpp (6)
15-24: Includes reorganized; types.h/signing.h added — OK.
Resolves alias dependencies and keeps TU self-sufficient.
958-1006: CollectSigSharesToSend: migrated to Uint256HashMap.
Semantics preserved; try_emplace use is fine.
1012-1012: proTxToNode switched to Uint256HashMap.
Keeps hashing consistent across the module.
1053-1055: CollectSigSharesToAnnounce: signature updated to Uint256HashMap.
No behavior change; aligns with consolidation.
1113-1117: SendMessages locals: migrated map types.
All downstream usage matches the new aliases.
861-956: CollectSigSharesToRequest signature verified.
Header declaration (void CollectSigSharesToRequest(std::unordered_map<NodeId, Uint256HashMap<CSigSharesInv>>& sigSharesToRequest)) matches the implementation; the pointer-to-mapped-value pattern remains safe.src/llmq/signing_shares.h (5)
15-18: Includes reordering + evo/types.h import look good.No transitive include smell spotted; alias-dependent types are declared after the include.
333-333: Session map alias swap is consistent.Type substitution aligns with the PR’s consolidation goal; pointer lifetimes for values referenced by sessionByRecvId are unchanged from prior behavior.
384-388: State maps migrated to Uint256HashMap under cs — OK.Lock annotations still cover accesses; no behavior change expected.
160-160: Alias verification complete: Uint256HashMap is defined asstd::unordered_map<uint256, T, StaticSaltedHasher>, so the salted hasher and allstd::unordered_mapsemantics (operator[], iteration, invalidation rules, ABI) are preserved.
480-487: All CollectSigSharesTo methods and their call sites use the Uint256HashMap aliases and are correctly guarded by LOCK(cs).*src/instantsend/instantsend.h (5)
15-15: Importing evo/types.h here is appropriate.Keeps alias visibility local to the header’s public surface.
70-73: Pending lock maps: alias swap looks correct.Semantics and locking remain unchanged; readability improves.
83-83: nonLockedTxs switched to Uint256HashMap — OK.No change to NonLockedTxInfo or related sets; concurrency guards intact.
90-90: timingsTxSeen migrated to Uint256HashMap — OK.Consistent with the rest of the PR.
116-117: All invocations use a cached snapshot, not the live maps. No call site passes the member maps (pendingInstantSendLocks or pendingNoTxInstantSendLocks) directly to the overload—all usages occur within the no-arg wrapper that copies under cs_pendingLocks and then passes the localpend.
| mutable Mutex cs; | ||
|
|
||
| std::unordered_map<uint256, CSporkMessage, StaticSaltedHasher> mapSporksByHash GUARDED_BY(cs); | ||
| Uint256HashMap<CSporkMessage> mapSporksByHash GUARDED_BY(cs); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure Uint256HashMap uses StaticSaltedHasher (DoS hardening) and preserves serialization.
Please confirm that Uint256HashMap is a type alias to std::unordered_map<uint256, T, StaticSaltedHasher>. This preserves salt-based hashing and existing (unordered) serialization semantics used by CFlatDB.
Add a compile-time guard in evo/types.h:
+#include <type_traits>
+#include <unordered_map>
+static_assert(std::is_same_v<Uint256HashMap<int>,
+ std::unordered_map<uint256, int, StaticSaltedHasher>>,
+ "Uint256HashMap must use StaticSaltedHasher");And verify with:
🏁 Script executed:
#!/bin/bash
# Confirm alias includes StaticSaltedHasher
rg -nP 'using\s+Uint256HashMap\s*=\s*std::unordered_map<\s*uint256\s*,[^>]*StaticSaltedHasher' src/evo/types.h
# Ensure (de)serialization compiles against unordered_map overloads
rg -nP '<<\s*mapSporksByHash|>>\s*mapSporksByHash' src/spork.hLength of output: 215
Add compile-time guard in src/evo/types.h
+ #include <type_traits>
+ static_assert(std::is_same_v<Uint256HashMap<int>,
+ std::unordered_map<uint256, int, StaticSaltedHasher>>,
+ "Uint256HashMap must use StaticSaltedHasher");Alias already uses StaticSaltedHasher (types.h:24) and serialization streams mapSporksByHash (spork.h:182,194).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/spork.h around line 170, mapSporksByHash is declared using Uint256HashMap
and relies on the StaticSaltedHasher alias defined in src/evo/types.h (around
line 24); add a compile-time guard in src/evo/types.h that asserts the map/hash
alias matches StaticSaltedHasher so serialization uses the expected hasher.
Specifically, add a static_assert (or equivalent SFINAE check) in
src/evo/types.h that verifies the typedef/alias used by Uint256HashMap is the
StaticSaltedHasher (or that a feature macro indicating salted hasher support is
defined), with a clear error message instructing to update the alias if it
differs.
|
This pull request has conflicts, please rebase. |
…ashSet, Uint256LruHashMap c461dcc fix: review comments (Konstantin Akimov) 46a28e6 fmt: apply clang-format (Konstantin Akimov) c452f88 refactor: leftover usages that has not been replaced by automatic script (Konstantin Akimov) 85e6af4 refactor: use Uint256LruHashMap all overcodebase (Konstantin Akimov) c8c6d29 refactor: use Uint256HashMap all over codebase (Konstantin Akimov) 3aaf330 refactor: use Uint256HashSet all over codebase (Konstantin Akimov) d7cd9ab refactor: define helpers Uint256HashMap, Uint256HashSet, Uint256LruHashMap for code simplification (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented Instead typing `std::unordered_map<uint256, CDeterministicMNList, StaticSaltedHasher> mnListsCache` better to just write `Uint256HashMap<CDeterministicMNList> mnListsCache` to minimize a boiler code. This PR aims to replace #6839, but this PR reduces scope significantly, no extra header is introduced. ## What was done? Main idea is authored by kwvg's PR: template <typename T> using Uint256HashMap = std::unordered_map<uint256, T, StaticSaltedHasher>; These helpers are used to prepare a PR: find src/ -name '*.cpp' -exec sed -i 's/std::unordered_set<uint256, StaticSaltedHasher>/Uint256HashSet/' {} \; find src/ -name '*.cpp' -exec sed -i 's/std::unordered_map<uint256, \([a-zA-Z0-9:]*\), StaticSaltedHasher>/Uint256HashMap<\1>/' {} \; find src/ -name '*.cpp' -exec sed -i 's/unordered_lru_cache<uint256, \([a-zA-Z0-9:]*\), StaticSaltedHasher>/Uint256LruHashMap<\1>/' {} \; Leftover changes are done manually. Please, note, that compilation time is not affected by including `<unordered_set>` and `<unordered_map>` because they are already included indirectly by chain `<saltedhasher.h> -> <hash.h> -> <serialize.h>` ## How Has This Been Tested? Grep all over codebase by `StaticSaltedHasher` ## Breaking Changes N/A ## Checklist: - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone ACKs for top commit: kwvg: utACK c461dcc Tree-SHA512: 2d8d0722554f5f89102d4d5380bfda01f2214cf8da82dbf300753e373a0b78ef6248ca626bd07f5bb467c5670ae9733d34aa020a1d268d3074dbc77fc5fc953b
|
Superseded by #6851 |
|
This pull request has conflicts, please rebase. |
kwvg
changed the title
evo/types.h{evo,llmq}/types.h
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/llmq/dkgsession.h (1)
14-15: Redundant forward declaration; consider removingYou still have a separate “class CDeterministicMN;” (Line 26). It’s redundant now that evo/types.h is included; drop it to avoid drift.
src/llmq/signing.h (1)
35-41: Const default: prefer inline constexpr for header-scoped constantsInline variables avoid multiple TU copies and reflect C++20 style.
-// Keep recovered signatures for a week. This is a "-maxrecsigsage" option default. -static constexpr int64_t DEFAULT_MAX_RECOVERED_SIGS_AGE{60 * 60 * 24 * 7}; +// Keep recovered signatures for a week. This is a "-maxrecsigsage" option default. +inline constexpr int64_t DEFAULT_MAX_RECOVERED_SIGS_AGE{60 * 60 * 24 * 7};src/llmq/quorums.h (1)
33-33: Optional: Drop redundant forward declaration of CDeterministicMN.evo/types.h already provides the forward declaration; keeping both is harmless but redundant.
-class CDeterministicMN;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/Makefile.am(2 hunks)src/coinjoin/client.h(1 hunks)src/evo/deterministicmns.h(1 hunks)src/evo/types.h(1 hunks)src/governance/governance.h(2 hunks)src/llmq/commitment.h(1 hunks)src/llmq/dkgsession.h(1 hunks)src/llmq/quorums.h(1 hunks)src/llmq/signing.h(2 hunks)src/llmq/signing_shares.h(1 hunks)src/llmq/types.h(1 hunks)src/llmq/utils.cpp(1 hunks)src/llmq/utils.h(1 hunks)src/qt/masternodelist.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/llmq/commitment.h
- src/Makefile.am
- src/coinjoin/client.h
- src/llmq/utils.cpp
- src/llmq/utils.h
- src/llmq/signing_shares.h
- src/qt/masternodelist.h
🧰 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/llmq/types.hsrc/evo/types.hsrc/governance/governance.hsrc/evo/deterministicmns.hsrc/llmq/signing.hsrc/llmq/dkgsession.hsrc/llmq/quorums.h
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/evo/types.hsrc/evo/deterministicmns.h
🧠 Learnings (3)
📚 Learning: 2025-07-23T09:30:34.631Z
Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.h:5-6
Timestamp: 2025-07-23T09:30:34.631Z
Learning: Dash Core uses BITCOIN_ prefix for header guards as the standard convention, inherited from Bitcoin Core. Only a few BLS-specific files in src/bls/ use DASH_ prefix. The vast majority of files (385+) use BITCOIN_ prefix.
Applied to files:
src/evo/deterministicmns.h
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/evo/specialtx.h : Special transactions use payload extensions defined in src/evo/specialtx.h
Applied to files:
src/evo/deterministicmns.h
📚 Learning: 2024-12-29T17:43:41.755Z
Learnt from: kwvg
PR: dashpay/dash#6504
File: src/llmq/quorums.cpp:224-224
Timestamp: 2024-12-29T17:43:41.755Z
Learning: The `CQuorumManager` is fully initialized by `LLMQContext`, addressing any concerns about the manager’s initialization sequence.
Applied to files:
src/llmq/signing.h
🧬 Code graph analysis (4)
src/llmq/types.h (1)
src/llmq/quorums.cpp (1)
CQuorum(77-79)
src/evo/types.h (1)
src/evo/deterministicmns.h (1)
CDeterministicMN(41-119)
src/governance/governance.h (1)
src/evo/deterministicmns.h (1)
CDeterministicMNList(128-189)
src/llmq/signing.h (1)
src/llmq/types.h (1)
llmq(10-17)
⏰ 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). (2)
- GitHub Check: linux64_multiprocess-test / Test source
- GitHub Check: linux64_tsan-test / Test source
🔇 Additional comments (9)
src/evo/types.h (1)
10-14: Centralized DMN pointer alias looks goodForward-decl + shared_ptr alias is clean and minimizes deps. Guard and include are correct.
src/llmq/dkgsession.h (1)
14-15: Correct dependency: include evo/types.h where CDeterministicMNCPtr is usedThis resolves the alias cleanly across LLMQ code.
src/evo/deterministicmns.h (2)
10-13: Include placement is consistent with module-first orderingAdding evo/{dmn_types,providertx,types}.h at the top adheres to the stated include-order practice.
10-13: Cyclic-safety noteIncluding evo/types.h (which forward-declares CDeterministicMN) in the header that defines it is fine; no ODR/cycle issues here.
Please ensure no translation unit includes evo/types.h and tries to use CDeterministicMNCPtr without also including deterministicmns.h before needing the full definition (typical, but worth a quick compile check on slow builders).
src/governance/governance.h (1)
178-179: Verify non-null invariant before dereferencing lastMNListForVotingKeysBoth Serialize and Unserialize dereference the shared_ptr. Confirm it’s always initialized before use (e.g., in GovernanceStore ctor) to avoid UB.
src/llmq/types.h (1)
10-19: LLMQ pointer/handle aliases centralized correctlyForward decls + pointer aliases under llmq:: keep headers light and decoupled. Guard and includes are correct.
src/llmq/signing.h (1)
11-13: Correct: include llmq/types.h for CQuorumCPtrPrevents leaking aliases elsewhere and keeps this header minimal.
src/llmq/quorums.h (2)
21-22: gsl/pointers include placement is fine.Required for gsl::not_null usages below. No issues.
11-15: No circular includes detected
Verified neitherllmq/types.hnorevo/types.hincludesllmq/quorums.h.
| class CInv; | ||
| class PeerManager; | ||
|
|
||
| class CDeterministicMNList; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Header self-sufficiency: include CDeterministicMNList definition
This header dereferences lastMNListForVotingKeys in inline Serialize/Unserialize (Lines 194, 214), which requires the full CDeterministicMNList definition. A forward decl alone is unsafe and relies on transitive includes.
Add the include for deterministicmns.h here.
Add near other project headers:
#include <evo/deterministicmns.h>🤖 Prompt for AI Agents
In src/governance/governance.h around line 27, the file only forward-declares
CDeterministicMNList but later dereferences lastMNListForVotingKeys in inline
Serialize/Unserialize methods (lines ~194 and ~214), so the full type definition
is required; add the project header include for deterministicmns.h (e.g.,
#include <evo/deterministicmns.h>) alongside the other project headers near the
top of the file so the inline serialization code has the complete
CDeterministicMNList definition.
Motivation
Consolidating frequently defined types to module-specific headers should prevent redefinition (often required to avoid undesirable dependency chains) and make code easier to read.
Additional Information
Depends on refactor: drop
fMasternodeModeglobal, create new context for masternode mode-only logic (ActiveContext), move spun-off signers and EHF signals handler #6828Depends on refactor: introduce new helpers Uint256HashMap, Uint256HashSet, Uint256LruHashMap #6851 (supersedes portions of this PR)
Breaking Changes
None expected.
Checklist