-
Notifications
You must be signed in to change notification settings - Fork 1.2k
refactor: introduce SignHash type for improved type safety in LLMQ signing #6826
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
0465463
f446223
ac9e5b7
b34fd1b
fd6ad86
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| // Copyright (c) 2025 The Dash Core developers | ||
| // Distributed under the MIT software license, see the accompanying | ||
| // file COPYING or http://www.opensource.org/licenses/mit-license.php. | ||
|
|
||
| #include <llmq/signhash.h> | ||
|
|
||
| #include <hash.h> | ||
| #include <serialize.h> | ||
|
|
||
| #include <vector> | ||
|
|
||
| namespace llmq { | ||
|
|
||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Ensure deterministic encoding of enum; drop unused include.
Apply: -#include <vector>
@@
- h << llmqType;
+ h << static_cast<uint8_t>(llmqType);Also applies to: 10-10 🤖 Prompt for AI Agents |
||
| h << quorumHash; | ||
| h << id; | ||
| h << msgHash; | ||
| m_hash = h.GetHash(); | ||
| } | ||
|
|
||
| } // namespace llmq | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| // Copyright (c) 2025 The Dash Core developers | ||
| // Distributed under the MIT software license, see the accompanying | ||
| // file COPYING or http://www.opensource.org/licenses/mit-license.php. | ||
|
|
||
| #ifndef BITCOIN_LLMQ_SIGNHASH_H | ||
| #define BITCOIN_LLMQ_SIGNHASH_H | ||
|
|
||
| #include <llmq/params.h> | ||
| #include <uint256.h> | ||
|
|
||
| #include <cstring> | ||
| #include <vector> | ||
|
|
||
| #include <saltedhasher.h> | ||
| #include <util/hash_type.h> | ||
|
|
||
| namespace llmq { | ||
|
|
||
| /** | ||
| * SignHash is a strongly-typed wrapper for the hash used in LLMQ signing operations. | ||
| * It encapsulates the hash calculation for quorum signatures, replacing the need for | ||
| * BuildSignHash function and avoiding circular dependencies. | ||
| */ | ||
| class SignHash : public BaseHash<uint256> | ||
| { | ||
| public: | ||
| SignHash() = default; | ||
| using BaseHash<uint256>::BaseHash; | ||
|
|
||
| /** | ||
| * 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; } | ||
|
|
||
| // Serialization support | ||
| template <typename Stream> | ||
| void Serialize(Stream& s) const | ||
| { | ||
| s << m_hash; | ||
| } | ||
|
|
||
| template <typename Stream> | ||
| void Unserialize(Stream& s) | ||
| { | ||
| s >> m_hash; | ||
| } | ||
| }; | ||
|
|
||
| // Salted hasher for llmq::SignHash that reuses the salted hasher for uint256 | ||
| struct SignHashSaltedHasher { | ||
| std::size_t operator()(const SignHash& signHash) const noexcept { return StaticSaltedHasher{}(signHash.Get()); } | ||
| }; | ||
|
|
||
| } // namespace llmq | ||
|
|
||
| // 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; | ||
| } | ||
| }; | ||
|
|
||
|
|
||
| #endif // BITCOIN_LLMQ_SIGNHASH_H |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |
|
|
||
| #include <llmq/commitment.h> | ||
| #include <llmq/quorums.h> | ||
| #include <llmq/signhash.h> | ||
| #include <llmq/signing_shares.h> | ||
|
|
||
| #include <bls/bls_batchverifier.h> | ||
|
|
@@ -161,7 +162,7 @@ void CRecoveredSigsDb::WriteRecoveredSig(const llmq::CRecoveredSig& recSig) | |
|
|
||
| // store by signHash | ||
| auto signHash = recSig.buildSignHash(); | ||
| auto k4 = std::make_tuple(std::string("rs_s"), signHash); | ||
| auto k4 = std::make_tuple(std::string("rs_s"), signHash.Get()); | ||
| batch.Write(k4, (uint8_t)1); | ||
|
|
||
| // store by current time. Allows fast cleanup of old recSigs | ||
|
|
@@ -173,7 +174,7 @@ void CRecoveredSigsDb::WriteRecoveredSig(const llmq::CRecoveredSig& recSig) | |
| { | ||
| LOCK(cs_cache); | ||
| hasSigForIdCache.insert(std::make_pair(recSig.getLlmqType(), recSig.getId()), true); | ||
| hasSigForSessionCache.insert(signHash, true); | ||
| hasSigForSessionCache.insert(signHash.Get(), true); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. consider further refactoring by using |
||
| hasSigForHashCache.insert(recSig.GetHash(), true); | ||
| } | ||
| } | ||
|
|
@@ -190,7 +191,7 @@ void CRecoveredSigsDb::RemoveRecoveredSig(CDBBatch& batch, Consensus::LLMQType l | |
| auto k1 = std::make_tuple(std::string("rs_r"), recSig.getLlmqType(), recSig.getId()); | ||
| auto k2 = std::make_tuple(std::string("rs_r"), recSig.getLlmqType(), recSig.getId(), recSig.getMsgHash()); | ||
| auto k3 = std::make_tuple(std::string("rs_h"), recSig.GetHash()); | ||
| auto k4 = std::make_tuple(std::string("rs_s"), signHash); | ||
| auto k4 = std::make_tuple(std::string("rs_s"), signHash.Get()); | ||
| batch.Erase(k1); | ||
| batch.Erase(k2); | ||
| if (deleteHashKey) { | ||
|
|
@@ -211,7 +212,7 @@ void CRecoveredSigsDb::RemoveRecoveredSig(CDBBatch& batch, Consensus::LLMQType l | |
|
|
||
| LOCK(cs_cache); | ||
| hasSigForIdCache.erase(std::make_pair(recSig.getLlmqType(), recSig.getId())); | ||
| hasSigForSessionCache.erase(signHash); | ||
| hasSigForSessionCache.erase(signHash.Get()); | ||
| if (deleteHashKey) { | ||
| hasSigForHashCache.erase(recSig.GetHash()); | ||
| } | ||
|
|
@@ -469,7 +470,7 @@ void CSigningManager::CollectPendingRecoveredSigsToVerify( | |
|
|
||
| bool alreadyHave = db.HasRecoveredSigForHash(recSig->GetHash()); | ||
| if (!alreadyHave) { | ||
| uniqueSignHashes.emplace(nodeId, recSig->buildSignHash()); | ||
| uniqueSignHashes.emplace(nodeId, recSig->buildSignHash().Get()); | ||
| retSigShares[nodeId].emplace_back(recSig); | ||
| } | ||
| ns.erase(ns.begin()); | ||
|
|
@@ -553,7 +554,8 @@ bool CSigningManager::ProcessPendingRecoveredSigs(PeerManager& peerman) | |
| } | ||
|
|
||
| const auto& quorum = quorums.at(std::make_pair(recSig->getLlmqType(), recSig->getQuorumHash())); | ||
| batchVerifier.PushMessage(nodeId, recSig->GetHash(), recSig->buildSignHash(), recSig->sig.Get(), quorum->qc->quorumPublicKey); | ||
| batchVerifier.PushMessage(nodeId, recSig->GetHash(), recSig->buildSignHash().Get(), recSig->sig.Get(), | ||
| quorum->qc->quorumPublicKey); | ||
| verifyCount++; | ||
| } | ||
| } | ||
|
|
@@ -605,7 +607,7 @@ void CSigningManager::ProcessRecoveredSig(const std::shared_ptr<const CRecovered | |
| CRecoveredSig otherRecoveredSig; | ||
| if (db.GetRecoveredSigById(llmqType, recoveredSig->getId(), otherRecoveredSig)) { | ||
| auto otherSignHash = otherRecoveredSig.buildSignHash(); | ||
| if (signHash != otherSignHash) { | ||
| if (signHash.Get() != otherSignHash.Get()) { | ||
| // this should really not happen, as each masternode is participating in only one vote, | ||
| // even if it's a member of multiple quorums. so a majority is only possible on one quorum and one msgHash per id | ||
| LogPrintf("CSigningManager::%s -- conflicting recoveredSig for signHash=%s, id=%s, msgHash=%s, otherSignHash=%s\n", __func__, | ||
|
|
@@ -836,20 +838,8 @@ void CSigningManager::WorkThreadMain(PeerManager& peerman) | |
| } | ||
| } | ||
|
|
||
| uint256 CSigBase::buildSignHash() const | ||
| { | ||
| return BuildSignHash(llmqType, quorumHash, id, msgHash); | ||
| } | ||
| SignHash CSigBase::buildSignHash() const { return SignHash(llmqType, quorumHash, id, msgHash); } | ||
|
|
||
| uint256 BuildSignHash(Consensus::LLMQType llmqType, const uint256& quorumHash, const uint256& id, const uint256& msgHash) | ||
| { | ||
| CHashWriter h(SER_GETHASH, 0); | ||
| h << llmqType; | ||
| h << quorumHash; | ||
| h << id; | ||
| h << msgHash; | ||
| return h.GetHash(); | ||
| } | ||
|
|
||
| bool IsQuorumActive(Consensus::LLMQType llmqType, const CQuorumManager& qman, const uint256& quorumHash) | ||
| { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.