refactor: introduce SignHash type for improved type safety in LLMQ signing #6826
Conversation
|
Note: this pr is currently based on #6825 |
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughAdds a new llmq::SignHash type (src/llmq/signhash.h/cpp) and wires it into the build. Replaces the standalone BuildSignHash function with SignHash and updates CSigBase::buildSignHash() to return SignHash. Call sites across evo, instantsend, llmq (signing, quorums, signing_shares), rpc, and tests now construct SignHash and use .Get() where a uint256 was previously used. Two nested signHash members in CSigSharesNodeState were changed from uint256 to llmq::SignHash. A std::hash specialization and salted hasher for SignHash were added. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (15)
test/lint/lint-circular-dependencies.py (1)
45-45: Validate that this new cycle is expected and unavoidable."core_io -> evo/mnhftx -> llmq/signing -> net_processing -> evo/smldiff -> core_io" still references llmq/signing. If the intent is to reduce signing exposure by introducing SignHash, consider whether this particular cycle can also be broken (e.g., moving verification bits behind narrower interfaces).
Would you like me to propose a dependency minimization plan (file-by-file include graph diff) for the evo/mnhftx → llmq/signing path?
src/llmq/signhash.h (2)
55-65: Move std::hash specialization into namespace std and use a fixed-width read for better distribution.Two issues:
- The specialization should be declared in namespace std (the qualified-id form is accepted by some compilers but declaring inside std is the conventional and less contentious approach).
- Hashing into size_t by memcpy’ing sizeof(size_t) bytes yields platform-dependent entropy (32 vs 64-bit). Prefer always reading 8 bytes (LE64) and casting to size_t.
Apply this diff:
-// Make SignHash hashable for use in unordered_map -template<> -struct std::hash<llmq::SignHash> { - std::size_t operator()(const llmq::SignHash& signHash) const noexcept { - // Use the first 8 bytes of the hash as the hash value - const unsigned char* data = signHash.data(); - std::size_t result; - std::memcpy(&result, data, sizeof(result)); - return result; - } -}; +// Make SignHash hashable for use in unordered_map +namespace std { +template<> +struct hash<llmq::SignHash> { + std::size_t operator()(const llmq::SignHash& signHash) const noexcept { + // Use the first 8 bytes of the hash for a simple hash seed + uint64_t v = 0; + std::memcpy(&v, signHash.data(), sizeof(v)); // data() is byte-aligned + return static_cast<std::size_t>(v); + } +}; +} // namespace stdIf you want stronger distribution, consider a lightweight mixer (e.g., xorshift or wyhash-inspired mix on v) at no appreciable cost.
28-50: Optional: leaner serialization via BaseHash utilities (if available).If util/hash_type.h already provides Serialize/Unserialize for BaseHash-derived types (as it does for other typed hashes in Core), these explicit methods may be redundant.
If redundant in your codebase, remove the custom Serialize/Unserialize here and rely on the BaseHash-provided ones. Otherwise, keeping them is fine.
src/evo/specialtx_filter.cpp (2)
35-51: Minor: unify element-add helpers to avoid vector copies (optional).Both AddScriptElement and AddHashElement allocate a fresh std::vector each time. If addElement ultimately copies into another container, this can be two copies per field.
Consider changing the callback to accept a Span (or similar) and only copy once at the call site requiring owning storage. That would also let you pass MakeUCharSpan(stream) directly without an intermediate vector.
52-59: Maintenance note is appreciated. Add a brief “what fields” table reference (optional).The sync note is good. A short pointer in a comment to the exact function/section in CBloomFilter that must mirror this would further reduce drift risk.
test/functional/p2p_blockfilters.py (1)
311-357: Helper creates valid, minimal AssetLockTx; consider using zero-value OP_RETURN (optional).The construction is straightforward and works for the purpose of filter testing. As a minor convention alignment, OP_RETURN outputs are typically zero-value.
Apply this diff to set OP_RETURN output to zero and keep the locked amount solely in the payload’s credit outputs:
- # Add OP_RETURN output with the locked amount - lock_tx.vout.append(CTxOut(int(amount_locked * COIN), CScript([OP_RETURN, b""]))) + # Add a zero-value OP_RETURN output to carry payload (conventional) + lock_tx.vout.append(CTxOut(0, CScript([OP_RETURN, b""])))src/instantsend/instantsend.cpp (1)
289-291: Minor: keep the strong type until the call site for clarityYou immediately convert SignHash to a uint256. Keeping the SignHash wrapper locally improves readability and type-safety, only unwrapping at the API boundary.
Apply this small refactor:
- uint256 signHash = llmq::SignHash(llmq_params.type, quorum->qc->quorumHash, id, islock->txid).Get(); - batchVerifier.PushMessage(nodeId, hash, signHash, islock->sig.Get(), quorum->qc->quorumPublicKey); + llmq::SignHash signHash(llmq_params.type, quorum->qc->quorumHash, id, islock->txid); + batchVerifier.PushMessage(nodeId, hash, signHash.Get(), islock->sig.Get(), quorum->qc->quorumPublicKey);src/Makefile.am (2)
203-204: Expose evo/specialtx_filter.h in BITCOIN_CORE_HPublicly listing the header is fine. See below about moving the implementation to libbitcoin_common for reuse.
484-485: Consider moving evo/specialtx_filter.cpp to libbitcoin_common to avoid duplication between filtersCBloomFilter lives in libbitcoin_common. If we want both bloom and compact filters to use the exact same extraction logic, putting evo/specialtx_filter.cpp into libbitcoin_common prevents duplication and keeps behavior in sync. Today, it’s compiled into libbitcoin_node, so common can’t directly reuse it without duplication.
Apply this diff to move the implementation to libbitcoin_common:
libbitcoin_node_a_SOURCES = \ ... - evo/specialtx.cpp \ - evo/specialtx_filter.cpp \ + evo/specialtx.cpp \ evo/specialtxman.cpp \ ... libbitcoin_common_a_SOURCES = \ base58.cpp \ bech32.cpp \ chainparams.cpp \ coins.cpp \ common/bloom.cpp \ + evo/specialtx_filter.cpp \ compressor.cpp \ core_read.cpp \ core_write.cpp \ ...This keeps blockfilter.cpp (node) able to call into common at link time, and lets CBloomFilter reuse the same function.
src/llmq/signing.h (1)
118-121: Opportunity: key recovered-sig caches by SignHash instead of uint256Now that SignHash exists, consider migrating caches/maps that logically key by the sign-hash (e.g., hasSigForSessionCache) to use SignHash for additional type safety. This is a nice-to-have and can be incremental.
src/llmq/signing.cpp (3)
189-201: Use signHash.Get() consistently for DB keys to avoid type ambiguity.Here k4 stores a SignHash object as the tuple element, while write path used uint256. While serialization currently matches, it’s safer and clearer to always pass the underlying uint256 for DB keys.
Apply:
- auto k4 = std::make_tuple(std::string("rs_s"), signHash); + auto k4 = std::make_tuple(std::string("rs_s"), signHash.Get());
435-444: Avoid recomputing SignHash in logs.buildSignHash() now constructs a hash each time. Cache it locally to reduce duplicate work in the hot path.
- LogPrint(BCLog::LLMQ, "CSigningManager::%s -- signHash=%s, id=%s, msgHash=%s, node=%d\n", __func__, - recoveredSig->buildSignHash().ToString(), recoveredSig->getId().ToString(), recoveredSig->getMsgHash().ToString(), from); + const SignHash sign_hash{recoveredSig->buildSignHash()}; + LogPrint(BCLog::LLMQ, "CSigningManager::%s -- signHash=%s, id=%s, msgHash=%s, node=%d\n", __func__, + sign_hash.ToString(), recoveredSig->getId().ToString(), recoveredSig->getMsgHash().ToString(), from); @@ - LogPrint(BCLog::LLMQ, "CSigningManager::%s -- already pending reconstructed sig, signHash=%s, id=%s, msgHash=%s, node=%d\n", __func__, - recoveredSig->buildSignHash().ToString(), recoveredSig->getId().ToString(), recoveredSig->getMsgHash().ToString(), from); + LogPrint(BCLog::LLMQ, "CSigningManager::%s -- already pending reconstructed sig, signHash=%s, id=%s, msgHash=%s, node=%d\n", __func__, + sign_hash.ToString(), recoveredSig->getId().ToString(), recoveredSig->getMsgHash().ToString(), from);
278-279: Format specifier mismatch in LogPrint.func is a const char*, but the format string uses %d. Use %s to avoid UB and garbled logs. (Pre-existing, but nearby.)
- LogPrint(BCLog::LLMQ, "CRecoveredSigsDb::%d -- deleted %d entries\n", __func__, toDelete.size()); + LogPrint(BCLog::LLMQ, "CRecoveredSigsDb::%s -- deleted %d entries\n", __func__, toDelete.size());src/test/blockfilter_tests.cpp (1)
356-443: Edge-case tests are valuable; consider adding a Bloom parity check.The compact-filter path is covered; adding analogous CBloomFilter relevance tests would guard parity with CheckSpecialTransactionMatchesAndUpdate.
If you’d like, I can draft a small Bloom filter unit test asserting that the same special-tx elements are considered relevant and update behavior is correct under BLOOM_UPDATE_ALL.
src/common/bloom.cpp (1)
200-202: Unknown type logging: reduce verbosity to avoid log spam.Using LogPrintf here can be noisy if new types appear. Consider demoting to a category-based LogPrint or removing entirely.
- LogPrintf("Unknown special transaction type in Bloom filter check.\n"); + LogPrint(BCLog::NET, "Unknown special transaction type in Bloom filter check.\n");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (19)
src/Makefile.am(4 hunks)src/blockfilter.cpp(2 hunks)src/common/bloom.cpp(1 hunks)src/evo/assetlocktx.cpp(3 hunks)src/evo/mnhftx.cpp(2 hunks)src/evo/specialtx_filter.cpp(1 hunks)src/evo/specialtx_filter.h(1 hunks)src/instantsend/instantsend.cpp(2 hunks)src/llmq/quorums.cpp(2 hunks)src/llmq/signhash.cpp(1 hunks)src/llmq/signhash.h(1 hunks)src/llmq/signing.cpp(8 hunks)src/llmq/signing.h(2 hunks)src/llmq/signing_shares.cpp(9 hunks)src/rpc/quorums.cpp(2 hunks)src/test/blockfilter_tests.cpp(2 hunks)src/test/evo_islock_tests.cpp(2 hunks)test/functional/p2p_blockfilters.py(3 hunks)test/lint/lint-circular-dependencies.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
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/evo/specialtx_filter.hsrc/rpc/quorums.cppsrc/evo/mnhftx.cppsrc/blockfilter.cppsrc/evo/specialtx_filter.cppsrc/llmq/quorums.cppsrc/test/blockfilter_tests.cppsrc/llmq/signing.hsrc/instantsend/instantsend.cppsrc/common/bloom.cppsrc/llmq/signhash.cppsrc/test/evo_islock_tests.cppsrc/llmq/signing_shares.cppsrc/llmq/signhash.hsrc/evo/assetlocktx.cppsrc/llmq/signing.cpp
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/specialtx_filter.hsrc/evo/mnhftx.cppsrc/evo/specialtx_filter.cppsrc/evo/assetlocktx.cpp
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/blockfilter_tests.cppsrc/test/evo_islock_tests.cpp
test/functional/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/p2p_blockfilters.py
🧠 Learnings (2)
📓 Common learnings
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
📚 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/specialtx_filter.hsrc/evo/mnhftx.cppsrc/blockfilter.cppsrc/evo/specialtx_filter.cppsrc/common/bloom.cppsrc/Makefile.amsrc/evo/assetlocktx.cpp
🧬 Code Graph Analysis (14)
src/evo/specialtx_filter.h (1)
src/evo/specialtx_filter.cpp (2)
ExtractSpecialTxFilterElements(57-136)ExtractSpecialTxFilterElements(57-58)
src/evo/mnhftx.cpp (1)
src/evo/mnhftx.h (1)
sig(36-36)
src/blockfilter.cpp (1)
src/evo/specialtx_filter.cpp (2)
ExtractSpecialTxFilterElements(57-136)ExtractSpecialTxFilterElements(57-58)
src/evo/specialtx_filter.cpp (2)
src/common/bloom.cpp (2)
stream(67-67)stream(88-88)src/test/blockfilter_tests.cpp (1)
stream(120-120)
src/test/blockfilter_tests.cpp (1)
src/evo/assetlocktx.h (1)
CAssetLockPayload(27-66)
src/llmq/signing.h (2)
src/llmq/signing_shares.h (4)
nodiscard(136-167)nodiscard(186-193)nodiscard(228-233)nodiscard(237-244)src/llmq/signing.cpp (2)
buildSignHash(840-843)buildSignHash(840-840)
src/instantsend/instantsend.cpp (2)
src/llmq/signhash.h (1)
SignHash(24-44)src/llmq/signhash.cpp (1)
SignHash(15-23)
src/llmq/signhash.cpp (1)
src/llmq/signhash.h (1)
SignHash(24-44)
src/test/evo_islock_tests.cpp (2)
src/llmq/signhash.h (1)
SignHash(24-44)src/llmq/signhash.cpp (1)
SignHash(15-23)
src/llmq/signing_shares.cpp (2)
src/llmq/signhash.h (1)
SignHash(24-44)src/llmq/signhash.cpp (1)
SignHash(15-23)
test/functional/p2p_blockfilters.py (3)
test/functional/test_framework/messages.py (2)
CAssetLockTx(1197-1222)tx_from_hex(230-232)test/functional/test_framework/script_util.py (1)
key_to_p2pkh_script(65-67)test/functional/test_framework/test_framework.py (1)
skip_if_no_wallet(1060-1068)
src/llmq/signhash.h (1)
src/llmq/signhash.cpp (1)
SignHash(15-23)
src/evo/assetlocktx.cpp (1)
src/evo/assetlocktx.h (1)
quorumSig(82-82)
src/llmq/signing.cpp (2)
src/llmq/signhash.h (1)
SignHash(24-44)src/llmq/signhash.cpp (1)
SignHash(15-23)
🔇 Additional comments (47)
src/evo/specialtx_filter.h (1)
13-22: Public API looks clear and minimal — good separation.Forward-declaring CTransaction and exposing only a callback-based extractor keeps dependencies light and prevents unnecessary header coupling. Matches the stated goal of avoiding circular deps.
test/lint/lint-circular-dependencies.py (1)
36-40: Updated circular-dependency expectations align with the refactor.The new cycles referencing llmq/commitment instead of llmq/signing (and removal of older signing-related cycles) are consistent with moving sign-hash calculation out of signing. No red flags here.
src/evo/specialtx_filter.cpp (2)
64-82: Parity with Bloom filter: ProRegTx extraction looks correct.
- Collateral outpoint serialized with SER_NETWORK/PROTOCOL_VERSION.
- Owner/voting KeyIDs and payout script added.
Matches the maintenance note’s intent to keep Bloom and compact-filter behavior in sync.
114-126: AssetLock: correct exclusion of OP_RETURN and inclusion of credit outputs.Including creditOutputs from the payload while excluding OP_RETURN is the right choice for filter usefulness. This aligns with the functional tests.
test/functional/p2p_blockfilters.py (3)
69-71: Good guard for wallet dependency.Ensures the functional test is skipped properly if the wallet is not compiled.
382-437: End-to-end AssetLockTx filter size check is pragmatic.Comparing filter sizes between a block with special tx and a control block is a reasonable signal given GCS decoding isn’t exposed here. Assertions and RPC usage look correct.
438-527: Multiple credit outputs path looks solid.Covers the multi-output case and validates increased filter size. Good complement to unit tests.
src/instantsend/instantsend.cpp (1)
20-20: Include addition aligns with the new SignHash type usageIncluding llmq/signhash.h here is correct and scoped alongside other llmq headers. No concerns.
src/evo/mnhftx.cpp (2)
12-12: Header include is correctllmq/signhash.h inclusion is necessary for the new SignHash construction below. Looks good.
106-108: LGTM: migrated to SignHash cleanlyConstructing SignHash and passing signHash.Get() to VerifyInsecure keeps the strong type locally and unwraps at the BLS boundary. No behavior change; clearer intent.
src/evo/assetlocktx.cpp (2)
10-10: Correct new dependencyAdding llmq/signhash.h is required for the SignHash initializer below. No issues.
149-151: Nice use of C++17 if-initializer with SignHashLimiting signHash’s scope to the condition is neat and safe; VerifyInsecure only needs the underlying uint256 during the call. No lifetime issues here.
src/rpc/quorums.cpp (2)
25-25: Include added appropriatelyllmq/signhash.h is needed for quorum_verify. Placement among llmq headers is consistent.
611-613: LGTM: switched quorum_verify to SignHashRetains strong typing locally and unwraps at the BLS boundary. RPC behavior unchanged; internal clarity improved.
src/llmq/quorums.cpp (3)
11-11: New include is correct and localizedllmq/signhash.h inclusion is required for VerifyRecoveredSig migration. No concerns.
1281-1283: LGTM: VerifyRecoveredSig migrated to SignHashStrongly-typed construction followed by Get() at the verify call site is exactly the intended pattern. No functional changes introduced.
11-11: Verification complete: no stale BuildSignHash & all SignHash call sites include <llmq/signhash.h>All
BuildSignHashreferences have been removed, and every file constructingllmq::SignHash(includingsrc/test/evo_islock_tests.cppandsrc/instantsend/instantsend.cpp) explicitly includes<llmq/signhash.h>.src/blockfilter.cpp (2)
10-10: Include of specialtx_filter is appropriate and localizedBringing in evo/specialtx_filter.h here keeps BlockFilter decoupled from specific special-tx details. No issues.
200-203: Good: delegated extraction for special-transaction elements; ensure parity with bloom filtersThe delegation pattern is clean and keeps BasicFilterElements lean. One concern: changing the content of the BASIC_FILTER by adding extra elements (keys, proTxHash, AssetLock credit outputs, etc.) will produce different cfilters and cfheaders compared to older releases. This can fragment serving peers until the network converges on this version.
- Please document this behavioral change in the release notes and confirm it’s acceptable for BIP158 light clients.
- If strict backward-compat is a requirement for the BASIC_FILTER type, consider introducing a new filter type instead.
Would you verify network impact and client compatibility? If we want to avoid duplication and drift between compact filters and bloom filters, we should have both call the same extractor (rather than “mirror” logic). See Makefile suggestion below for moving evo/specialtx_filter.cpp to libbitcoin_common so CBloomFilter can reuse it directly.
src/test/evo_islock_tests.cpp (2)
8-8: Include of llmq/signhash.h is correctThe test now depends on the new SignHash API; header inclusion is correct.
84-85: Correct SignHash construction and usage in testOrder of parameters matches SignHash’s serialization (llmqType, quorumHash, id, msgHash), and calling .Get() for uint256 comparison is appropriate. LGTM.
src/Makefile.am (2)
256-257: Expose llmq/signhash.h in BITCOIN_CORE_HThis aligns with the API shift to the strong SignHash type. No issues.
517-518: Adding llmq/signhash.cpp to node library is fineSignHash is primarily consumed by llmq/signing and friends in node; placement is reasonable.
src/llmq/signing.h (2)
11-11: Include dependency on signhash.h is required for the new return typeHeader dependency is correct and localizes the type change.
71-71: All C++ call sites correctly use SignHash; no outstanding BuildSignHash referencesI’ve verified that in the C++ codebase:
- There are no remaining calls to the old free function
BuildSignHash(only a Go binding insrc/dashbls/go-bindings/threshold.go, which isn’t affected).- Every use of
buildSignHash()in C++ either
• stores the result in aSignHashand uses its methods directly (e.g..ToString())
• or calls.Get()when a rawuint256is needed (e.g. in caches, BLS-verify, batch writes).No further updates are required here.
src/llmq/signhash.cpp (1)
15-23: Constructor correctly reproduces previous BuildSignHash hashing semanticsThe serialization order and use of CHashWriter(SER_GETHASH, 0) match the prior approach. No runtime overhead, and type safety is improved.
src/llmq/signing.cpp (6)
9-9: Include for SignHash is correct and scoped.Bringing in llmq/signhash.h locally keeps the type usage explicit in this TU. Looks good.
165-167: DB keying by signHash now uses uint256 at the boundary — LGTM.Using signHash.Get() when forming the LevelDB key preserves on-disk compatibility and keeps DB schema unchanged.
213-216: Cache erase uses uint256 — consistent with cache key type.Using signHash.Get() aligns with the cache's uint256 key type. Good.
556-558: Batch verify now feeds the underlying uint256 sign hash — correct.Switching PushMessage to recSig->buildSignHash().Get() matches the verifier’s template type.
600-614: SignHash usage in conflict detection is precise.Comparing signHash.Get() values preserves prior behavior and clarifies intent.
840-843: API change: CSigBase::buildSignHash() now returns SignHash — clean and contained.Encapsulating construction inside the type improves intent and type safety. No behavioral change.
src/llmq/signing_shares.cpp (8)
10-10: Include for SignHash is appropriate.Keeps the translation unit self-sufficient with the new type.
31-33: Keying by uint256 sign hash is preserved — good.Using buildSignHash().Get() to populate SigShareKey avoids accidental map key type drift.
128-132: Session map remains keyed by uint256 — consistent usage of Get().Constructing SignHash once and using Get() avoids recomputation and keeps map keys uniform.
776-778: Precomputing signHash for lookups is correct.Constructing SignHash once and taking .Get() for map access keeps lookups consistent and efficient.
833-835: Verification uses underlying uint256 — aligns with BLS API.Passing signHash.Get() to VerifyInsecure matches library expectations.
1561-1565: Signing uses buildSignHash().Get() — correct across both single-node and threshold paths.This keeps signature inputs stable and type-safe.
Also applies to: 1597-1601
1621-1627: ForceReAnnouncement: session keying by uint256 maintained — good.Computing SignHash(...).Get() here avoids accidental divergence with storage/container key types.
1643-1644: Cleanup on recovered sig uses underlying uint256 — correct.RemoveSigSharesForSession expects uint256; passing .Get() is the right boundary conversion.
src/test/blockfilter_tests.cpp (2)
10-18: Required headers added for special-tx payloads and spans — LGTM.These cover payload construction, network info, and span utilities used in the tests.
201-354: Comprehensive coverage for special transaction fields in filters — well done.The assertions validate inclusion of collateralOutpoint, key IDs, payout scripts, proTxHash, and AssetLock credit scripts, and exclusion of unrelated data. This anchors behavior for ExtractSpecialTxFilterElements.
src/common/bloom.cpp (5)
122-127: Maintenance note is spot on.Explicitly calling out the need to keep this in sync with ExtractSpecialTxFilterElements will save future regressions.
133-145: Provider Register matching and update behavior looks correct.
- Matches collateral outpoint, owner/voting key IDs, and payout script.
- Updates the filter with tx hash under BLOOM_UPDATE_ALL.
This mirrors what clients expect for SPV relevance.
147-158: Service Update handling is consistent with expectations.
- proTxHash match returns relevance immediately.
- scriptOperatorPayout match triggers proTxHash insertion under BLOOM_UPDATE_ALL.
Good balance between relevance and update semantics.
160-171: Registrar Update handling is correct and symmetric with service update.Checks proTxHash first, then key/script with appropriate update behavior.
180-191: Double-check AssetUnlock prevouts against synthetic AssetLock outpointsWe’ve confirmed that:
- Bloom code in
src/common/bloom.cppunconditionally injects synthetic outpoints(txid, idx)for each credit output of an AssetLock viaProcessTxOut(extraOuts[i], tx.GetHash(), i).- The AssetUnlock logic (in
src/evo/assetlocktx.cpp→CheckAssetUnlockTx→GetDataFromUnlockTxinsrc/evo/creditpool.cpp) does iterate overtx.vinand expects each input’sprevoutto reference exactly those synthetic outpoints (i.e. the credit outputs inserted earlier).- No other special prevout mapping is used—AssetUnlock inputs are matched by standard
COutPoint(hash,index).Therefore the synthetic insertion is correct and clients will see relevant matches when filtering for spends of those credit outputs.
No changes needed here.
src/llmq/signhash.h
Outdated
| class SignHash : public BaseHash<uint256> { | ||
| public: | ||
| SignHash() = default; | ||
|
|
||
| /** | ||
| * Constructs a SignHash from the given parameters. | ||
| * This replaces the previous BuildSignHash function. | ||
| */ | ||
| SignHash(Consensus::LLMQType llmqType, const uint256& quorumHash, const uint256& id, const uint256& msgHash); | ||
|
|
||
| /** | ||
| * Get the underlying uint256 hash value. | ||
| */ | ||
| const uint256& Get() const { return m_hash; } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix inheritance: BaseHash must be tagged with the derived type, not uint256.
Using BaseHash defeats the type-tagging (and some compile-time type-safety) this PR aims to introduce. The Tag template parameter should be the wrapper type itself.
Apply this diff:
-#include <llmq/params.h>
+#include <consensus/llmq_types.h>
#include <uint256.h>
-#include <cstring>
-#include <vector>
+#include <cstring>
+#include <cstdint>
+#include <functional>
#include <util/hash_type.h>
namespace llmq
{
@@
-class SignHash : public BaseHash<uint256> {
+class SignHash : public BaseHash<SignHash> {
public:
SignHash() = default;
@@
- SignHash(Consensus::LLMQType llmqType, const uint256& quorumHash, const uint256& id, const uint256& msgHash);
+ explicit SignHash(Consensus::LLMQType llmqType, const uint256& quorumHash, const uint256& id, const uint256& msgHash);Rationale:
- BaseHash expects T to be the tag type, not the underlying hash.
- Include the minimal header that declares Consensus::LLMQType to avoid heavier dependencies.
- Explicit constructor prevents unintended implicit conversions.
- Add missing standard headers required by std::hash specialization and fixed-width integer usage.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class SignHash : public BaseHash<uint256> { | |
| public: | |
| SignHash() = default; | |
| /** | |
| * Constructs a SignHash from the given parameters. | |
| * This replaces the previous BuildSignHash function. | |
| */ | |
| SignHash(Consensus::LLMQType llmqType, const uint256& quorumHash, const uint256& id, const uint256& msgHash); | |
| /** | |
| * Get the underlying uint256 hash value. | |
| */ | |
| const uint256& Get() const { return m_hash; } | |
| #include <consensus/llmq_types.h> | |
| #include <uint256.h> | |
| #include <cstring> | |
| #include <cstdint> | |
| #include <functional> | |
| #include <util/hash_type.h> | |
| namespace llmq { | |
| class SignHash : public BaseHash<SignHash> { | |
| public: | |
| SignHash() = default; | |
| /** | |
| * Constructs a SignHash from the given parameters. | |
| * This replaces the previous BuildSignHash function. | |
| */ | |
| explicit SignHash(Consensus::LLMQType llmqType, const uint256& quorumHash, const uint256& id, const uint256& msgHash); | |
| /** | |
| * Get the underlying uint256 hash value. | |
| */ | |
| const uint256& Get() const { return m_hash; } |
| LOCK(cs_cache); | ||
| hasSigForIdCache.insert(std::make_pair(recSig.getLlmqType(), recSig.getId()), true); | ||
| hasSigForSessionCache.insert(signHash, true); | ||
| hasSigForSessionCache.insert(signHash.Get(), true); |
There was a problem hiding this comment.
consider further refactoring by using SignHash as a type for hasSigForSessionCache, for uniqueSignHashes, as argument in PushMessage, InitSession, as a member of SignSession, etc
|
This pull request has conflicts, please rebase. |
…ping Refactored BuildSignHash function into a dedicated SignHash class that extends BaseHash<uint256>. This change: - Eliminates circular dependencies by removing forward declarations - Improves type safety with dedicated SignHash type vs generic uint256 - Follows existing patterns in the codebase (similar to AssumeutxoHash) - Centralizes sign hash calculation logic in its own translation unit The new SignHash type encapsulates the hash calculation for LLMQ signing operations, making the code more maintainable and type-safe. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Extends usage of the SignHash type to improve type safety: - CSigBase::buildSignHash() now returns SignHash instead of uint256 - Added serialization support for database storage compatibility - Added std::hash specialization for use in unordered_map - Use SignHash directly where possible, only converting to uint256 at API boundaries (BLS verification, database operations) This provides better type safety while maintaining clean abstraction boundaries - the BLS library continues to work with uint256 as it should not depend on higher-level LLMQ types. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
PastaPastaPasta
force-pushed
the
refactor-signhash-type
branch
from
75d6cf4 to
f446223
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/llmq/signing_shares.cpp (1)
129-132: Consistent session map keying with uint256; consider broader SignHash use later.Using signHash.Get() keeps wire/db compatibility. Optional: store SignHash (or a strong typedef) in Session to reduce repeated conversions.
🧹 Nitpick comments (3)
src/llmq/signing_shares.cpp (1)
10-10: Fix include order to satisfy clang-format.Place llmq headers in alphabetical order within the group to resolve the CI formatting error.
Apply this diff:
-#include <llmq/options.h> -#include <llmq/quorums.h> -#include <llmq/commitment.h> +#include <llmq/commitment.h> +#include <llmq/options.h> +#include <llmq/quorums.h> #include <llmq/signhash.h>src/llmq/signing.cpp (2)
189-201: Use uint256 consistently for the “rs_s” key during erase.k4 currently builds the key with SignHash (not .Get()). While serialization is likely identical, using uint256 avoids ambiguity and matches write paths.
Apply this diff:
- auto k4 = std::make_tuple(std::string("rs_s"), signHash); + auto k4 = std::make_tuple(std::string("rs_s"), signHash.Get());Also applies to: 213-216
554-558: Wrap long PushMessage call to satisfy clang-format CI.Reflow the call as multi-line.
Apply this diff:
- batchVerifier.PushMessage(nodeId, recSig->GetHash(), recSig->buildSignHash().Get(), recSig->sig.Get(), quorum->qc->quorumPublicKey); + batchVerifier.PushMessage( + nodeId, + recSig->GetHash(), + recSig->buildSignHash().Get(), + recSig->sig.Get(), + quorum->qc->quorumPublicKey);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
src/Makefile.am(2 hunks)src/evo/assetlocktx.cpp(2 hunks)src/evo/mnhftx.cpp(2 hunks)src/instantsend/instantsend.cpp(2 hunks)src/llmq/quorums.cpp(2 hunks)src/llmq/signhash.cpp(1 hunks)src/llmq/signhash.h(1 hunks)src/llmq/signing.cpp(8 hunks)src/llmq/signing.h(2 hunks)src/llmq/signing_shares.cpp(9 hunks)src/rpc/quorums.cpp(2 hunks)src/test/evo_islock_tests.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- src/llmq/signing.h
- src/evo/assetlocktx.cpp
- src/llmq/quorums.cpp
- src/instantsend/instantsend.cpp
- src/Makefile.am
- src/test/evo_islock_tests.cpp
- src/evo/mnhftx.cpp
- src/rpc/quorums.cpp
- src/llmq/signhash.cpp
- src/llmq/signhash.h
🧰 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/llmq/signing.cppsrc/llmq/signing_shares.cpp
🧠 Learnings (1)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
🧬 Code graph analysis (2)
src/llmq/signing.cpp (2)
src/llmq/signhash.h (1)
SignHash(24-44)src/llmq/signhash.cpp (1)
SignHash(15-23)
src/llmq/signing_shares.cpp (2)
src/llmq/signhash.h (1)
SignHash(24-44)src/llmq/signhash.cpp (1)
SignHash(15-23)
🪛 GitHub Actions: Clang Diff Format Check
src/llmq/signing.cpp
[error] 554-556: Clang-format differences found: wrapped long function call across lines.
[error] 837-846: Clang-format differences found: condensed buildSignHash function formatting.
src/llmq/signing_shares.cpp
[error] 4-9: Clang-format differences found: include ordering changes.
⏰ 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-apple-darwin / Build depends
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (10)
src/llmq/signing_shares.cpp (6)
31-31: Keying now uses uint256 via SignHash::Get() — correct boundary.This aligns the in-memory key (uint256) with the new SignHash type. LGTM.
776-777: Correct materialization of SignHash for lookups.Computing once and using the uint256 for map access is appropriate.
834-835: Verification uses underlying hash — correct.Passing signHash.Get() to VerifyInsecure is expected.
1561-1561: Signing with SignHash::Get() — correct.Both branches properly sign the concrete uint256.
Also applies to: 1597-1597
1621-1621: Re-announcement uses recomputed signHash — OK.Matches the session/signature stores keyed by uint256.
1643-1644: Cleanup uses recoveredSig.buildSignHash().Get() — consistent.Correctly erases by the underlying session key.
src/llmq/signing.cpp (4)
165-178: DB write paths now store by uint256(session) from SignHash — correct.k4 uses signHash.Get() and the cache mirrors that. Looks good.
473-474: Unique-session tracking uses buildSignHash().Get() — correct.Strong type at construction, plain uint256 for set keying. LGTM.
600-610: Equality via SignHash::Get() — correct.Comparison uses the underlying uint256 to detect conflicts. LGTM.
840-843: buildSignHash now returns SignHash — good; adjust formatting per CI.Functionality is correct. The CI flagged formatting; please run clang-format to match the project style if it still fails after other fixes.
This update enhances the consistency and type safety of the LLMQ signing code by: - Replacing uint256 with llmq::SignHash in relevant structures and functions. - Updating InitSession to accept SignHash directly, improving clarity. - Ensuring serialization compatibility for SignHash in database operations. - Adjusting includes for better organization and reducing redundancy. These changes streamline the codebase and reinforce the use of the SignHash type, aligning with previous refactoring efforts for improved maintainability.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/llmq/signing.cpp (1)
176-176: Broaden SignHash usage in caches and maps (follow-up).Consider using SignHash in hasSigForSessionCache, unique sets, and function params to maximize type safety. Requires hasher support.
🧹 Nitpick comments (4)
src/llmq/signing_shares.h (1)
311-312: Storing SignHash in SessionInfo/Session is consistent with the refactor.Good step toward stronger typing. Consider also updating APIs that take a signHash to accept SignHash for full end-to-end type safety (e.g., GetSessionBySignHash/EraseAllForSignHash helpers).
Also applies to: 324-325
src/llmq/signing_shares.cpp (3)
101-115: Avoid copying CSigBase into InitSession.Pass by const reference to prevent copying several uint256s.
Apply:
-static void InitSession(CSigSharesNodeState::Session& s, const llmq::SignHash& signHash, CSigBase from) +static void InitSession(CSigSharesNodeState::Session& s, const llmq::SignHash& signHash, const CSigBase& from)
121-123: Minor: reuse computed SignHash if available.Not critical, but when UpdateKey already computed the hash, consider threading that through to avoid recomputation.
129-133: sessions keyed by uint256 while Session holds SignHash.This is fine for now; long-term, consider keying by SignHash once a salted hasher exists for it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
src/llmq/quorums.cpp(2 hunks)src/llmq/signhash.cpp(1 hunks)src/llmq/signhash.h(1 hunks)src/llmq/signing.cpp(9 hunks)src/llmq/signing.h(2 hunks)src/llmq/signing_shares.cpp(12 hunks)src/llmq/signing_shares.h(2 hunks)src/test/evo_islock_tests.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/llmq/quorums.cpp
- src/test/evo_islock_tests.cpp
- src/llmq/signhash.cpp
- src/llmq/signhash.h
🧰 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/llmq/signing_shares.hsrc/llmq/signing.hsrc/llmq/signing_shares.cppsrc/llmq/signing.cpp
🧠 Learnings (1)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
🧬 Code graph analysis (4)
src/llmq/signing_shares.h (2)
src/llmq/signhash.h (2)
llmq(17-52)SignHash(24-46)src/llmq/signhash.cpp (1)
SignHash(14-22)
src/llmq/signing.h (2)
src/llmq/signing_shares.h (4)
nodiscard(136-167)nodiscard(186-193)nodiscard(228-233)nodiscard(237-244)src/llmq/signing.cpp (2)
buildSignHash(841-841)buildSignHash(841-841)
src/llmq/signing_shares.cpp (2)
src/llmq/signhash.cpp (1)
SignHash(14-22)src/llmq/signhash.h (1)
SignHash(24-46)
src/llmq/signing.cpp (2)
src/llmq/signhash.cpp (1)
SignHash(14-22)src/llmq/signhash.h (1)
SignHash(24-46)
⏰ 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: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (19)
src/llmq/signing.h (3)
11-11: Include of signhash.h is appropriate.Required for the new SignHash return type.
18-18: Include of util/threadinterrupt.h is fine.Matches usage of CThreadInterrupt in this header.
71-71: All callers updated to use SignHash
Verified via grep that every use of buildSignHash now returns SignHash and no code still expects a raw uint256.src/llmq/signing_shares.cpp (9)
7-7: New include is fine.llmq/commitment.h is used in this TU; OK.
10-10: New include is required for SignHash.All good.
31-33: UpdateKey now uses SignHash::Get() correctly.No issues.
351-353: Correct: compare/cache lookups use signHash.Get().Matches existing DB/cache API that expects uint256.
Also applies to: 388-390
834-841: Sporadic VerifyInsecure uses SignHash::Get() correctly.No issues.
1561-1565: Local copy of uint256 signHash is correct for signing.Explicit copy avoids lifetime pitfalls. LGTM.
Also applies to: 1596-1603
1621-1627: OK: Re-announce path derives uint256 signHash via SignHash wrapper.Consistent with internal map key type.
1643-1645: Cleanup uses recoveredSig.buildSignHash().Get(); correct.Matches Erase* methods expecting uint256.
776-781: All SignHash::Get() bindings are safe
Noauto&orconst auto&bindings toSignHash(...).Get()were found in the codebase; all calls use value copies.src/llmq/signing.cpp (7)
9-9: Include signhash.h is needed.LGTM.
165-167: DB keys now use SignHash::Get(); correct.Keeps on-disk format as uint256.
Also applies to: 175-179
195-201: Erase paths updated to use SignHash::Get(); correct.Cache consistency preserved.
Also applies to: 213-216
473-475: Unique set uses buildSignHash().Get() — safe copy.No issues.
611-613: Conflict check compares SignHash::Get(); correct.Type-safe compare while preserving wrapper semantics.
841-841: buildSignHash() implementation updated — good.Centralizes construction in SignHash ctor; noexcept not required.
557-559: CBLSBatchVerifier::PushMessage copies the message hash by value; passingbuildSignHash().Get()is safe. InternalMessagestores its ownuint256copy, so there’s no risk of dangling references.
There was a problem hiding this comment.
fix clang https://github.com/dashpay/dash/actions/runs/17408759830/job/49420182387?pr=6826
see nits, mostly about using {}
src/evo/mnhftx.cpp
Outdated
|
|
||
| const uint256 signHash = llmq::BuildSignHash(llmqType, quorum->qc->quorumHash, requestId, msgHash); | ||
| if (!sig.VerifyInsecure(quorum->qc->quorumPublicKey, signHash)) { | ||
| const llmq::SignHash signHash(llmqType, quorum->qc->quorumHash, requestId, msgHash); |
There was a problem hiding this comment.
nit: use {}
| const llmq::SignHash signHash(llmqType, quorum->qc->quorumHash, requestId, msgHash); | |
| const llmq::SignHash signHash{llmqType, quorum->qc->quorumHash, requestId, msgHash}; |
src/instantsend/instantsend.cpp
Outdated
| return {}; | ||
| } | ||
| uint256 signHash = BuildSignHash(llmq_params.type, quorum->qc->quorumHash, id, islock->txid); | ||
| uint256 signHash = llmq::SignHash(llmq_params.type, quorum->qc->quorumHash, id, islock->txid).Get(); |
There was a problem hiding this comment.
| uint256 signHash = llmq::SignHash(llmq_params.type, quorum->qc->quorumHash, id, islock->txid).Get(); | |
| uint256 signHash = llmq::SignHash{llmq_params.type, quorum->qc->quorumHash, id, islock->txid}.Get(); |
src/llmq/quorums.cpp
Outdated
|
|
||
| uint256 signHash = BuildSignHash(llmqType, quorum->qc->quorumHash, id, msgHash); | ||
| const bool ret = sig.VerifyInsecure(quorum->qc->quorumPublicKey, signHash); | ||
| SignHash signHash(llmqType, quorum->qc->quorumHash, id, msgHash); |
There was a problem hiding this comment.
| SignHash signHash(llmqType, quorum->qc->quorumHash, id, msgHash); | |
| SignHash signHash{llmqType, quorum->qc->quorumHash, id, msgHash}; |
src/llmq/signhash.cpp
Outdated
| m_hash = h.GetHash(); | ||
| } | ||
|
|
||
| } // namespace llmq No newline at end of file |
src/llmq/signhash.h
Outdated
| }; | ||
|
|
||
|
|
||
| #endif // BITCOIN_LLMQ_SIGNHASH_H No newline at end of file |
src/rpc/quorums.cpp
Outdated
|
|
||
| uint256 signHash = llmq::BuildSignHash(llmqType, quorum->qc->quorumHash, id, msgHash); | ||
| return sig.VerifyInsecure(quorum->qc->quorumPublicKey, signHash); | ||
| llmq::SignHash signHash(llmqType, quorum->qc->quorumHash, id, msgHash); |
There was a problem hiding this comment.
| llmq::SignHash signHash(llmqType, quorum->qc->quorumHash, id, msgHash); | |
| llmq::SignHash signHash{llmqType, quorum->qc->quorumHash, id, msgHash}; |
- Replace () with {} for SignHash constructor calls across codebase
- Add missing trailing newlines in signhash files
- Addresses code review feedback for consistent initialization syntax
|
@knst see latest |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/rpc/quorums.cpp (1)
611-612: LGTM: matches prior suggestion; concise and clear.
🧹 Nitpick comments (1)
src/evo/assetlocktx.cpp (1)
149-151: Minor readability: avoid init-statement in if; construct then verify.Apply:
- if (const llmq::SignHash signHash(llmqType, quorum->qc->quorumHash, requestId, msgHash); - quorumSig.VerifyInsecure(quorum->qc->quorumPublicKey, signHash.Get())) { + const llmq::SignHash signHash{llmqType, quorum->qc->quorumHash, requestId, msgHash}; + if (quorumSig.VerifyInsecure(quorum->qc->quorumPublicKey, signHash.Get())) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
src/evo/assetlocktx.cpp(2 hunks)src/evo/mnhftx.cpp(2 hunks)src/instantsend/instantsend.cpp(2 hunks)src/llmq/quorums.cpp(2 hunks)src/llmq/signhash.cpp(1 hunks)src/llmq/signhash.h(1 hunks)src/rpc/quorums.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/instantsend/instantsend.cpp
- src/llmq/quorums.cpp
- src/evo/mnhftx.cpp
🧰 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/signhash.cppsrc/rpc/quorums.cppsrc/evo/assetlocktx.cppsrc/llmq/signhash.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/assetlocktx.cpp
🧠 Learnings (1)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
🧬 Code graph analysis (3)
src/llmq/signhash.cpp (1)
src/llmq/signhash.h (1)
SignHash(24-46)
src/evo/assetlocktx.cpp (1)
src/evo/assetlocktx.h (1)
quorumSig(82-82)
src/llmq/signhash.h (1)
src/llmq/signhash.cpp (1)
SignHash(14-22)
⏰ 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). (8)
- GitHub Check: predict_conflicts
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (3)
src/evo/assetlocktx.cpp (1)
10-10: LGTM: correct dependency inclusion.src/rpc/quorums.cpp (1)
25-25: LGTM: needed header added.src/llmq/signhash.h (1)
24-29: Verify SignHash CRTP template, salted hashing, and includes
Ensure BaseHash is parameterized with SignHash (CRTP), std::hash uses StaticSaltedHasher for DoS resistance, and clean up includes by removing<vector>and adding<cstdint>/<functional>.
| SignHash::SignHash(Consensus::LLMQType llmqType, const uint256& quorumHash, const uint256& id, const uint256& msgHash) | ||
| { | ||
| CHashWriter h(SER_GETHASH, 0); | ||
| h << llmqType; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure deterministic encoding of enum; drop unused include.
- Serialize LLMQType explicitly as uint8_t for stability.
- Remove unused .
Apply:
-#include <vector>
@@
- h << llmqType;
+ h << static_cast<uint8_t>(llmqType);Also applies to: 10-10
🤖 Prompt for AI Agents
In src/llmq/signhash.cpp around line 17, the enum value llmqType is being
serialized without an explicit fixed-size type which risks non-deterministic
encoding; cast/convert llmqType to uint8_t (or static_cast<uint8_t>(llmqType))
when writing to h so it is serialized as a single byte, and remove the
now-unused #include <vector> from the top of the file to clean up imports.
Summary
This PR introduces a dedicated
SignHashtype to replace the genericuint256usage for LLMQ sign hashes, improving type safety and code clarity throughout the signing subsystem.Motivation
Previously,
BuildSignHashwas a standalone function returninguint256, which:Changes
New SignHash Type
SignHashclass extendingBaseHash<uint256>(following existing patterns likeAssumeutxoHash)std::hashspecialization for use as unordered_map keysRefactored APIs
CSigBase::buildSignHash()now returnsSignHashinstead ofuint256SignHashconstructorassetlocktx.cppandquorums.cppType Safety Improvements
uint256via.Get()only at API boundaries (BLS verification, database operations)uint256as it shouldn't depend on higher-level LLMQ typesBenefits
Test plan
test_dash --run_test=evo_islock_tests)🤖 Generated with Claude Code