Skip to content

Commit 0052fca

Browse files
committed
refactor: move AsyncSignIfMember() to CSigSharesManager
Giving write access of CRecoveredSigsDb to an external manager isn't great but we have to do this if we want to drop CActiveMasternodeManager from CSigSharesManager Review with `git log -p -n1 --color-moved=dimmed_zebra`.
1 parent 489cf6e commit 0052fca

File tree

9 files changed

+114
-99
lines changed

9 files changed

+114
-99
lines changed

src/chainlock/signing.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99

1010
#include <chainlock/clsig.h>
1111
#include <instantsend/instantsend.h>
12+
#include <llmq/signing_shares.h>
1213
#include <masternode/sync.h>
1314
#include <spork.h>
1415

@@ -140,7 +141,7 @@ void ChainLockSigner::TrySignChainTip(const llmq::CInstantSendManager& isman)
140141
lastSignedMsgHash = msgHash;
141142
}
142143

143-
m_sigman.AsyncSignIfMember(Params().GetConsensus().llmqTypeChainLocks, m_shareman, requestId, msgHash);
144+
m_shareman.AsyncSignIfMember(Params().GetConsensus().llmqTypeChainLocks, m_sigman, requestId, msgHash);
144145
}
145146

146147
void ChainLockSigner::EraseFromBlockHashTxidMap(const uint256& hash)

src/instantsend/signing.cpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313

1414
#include <chainlock/chainlock.h>
1515
#include <llmq/quorums.h>
16+
#include <llmq/signing_shares.h>
1617
#include <masternode/sync.h>
1718
#include <spork.h>
1819

@@ -330,7 +331,7 @@ bool InstantSendSigner::TrySignInputLocks(const CTransaction& tx, bool fRetroact
330331
WITH_LOCK(cs_input_requests, inputRequestIds.emplace(id));
331332
LogPrint(BCLog::INSTANTSEND, "%s -- txid=%s: trying to vote on input %s with id %s. fRetroactive=%d\n",
332333
__func__, tx.GetHash().ToString(), in.prevout.ToStringShort(), id.ToString(), fRetroactive);
333-
if (m_sigman.AsyncSignIfMember(llmqType, m_shareman, id, tx.GetHash(), {}, fRetroactive)) {
334+
if (m_shareman.AsyncSignIfMember(llmqType, m_sigman, id, tx.GetHash(), {}, fRetroactive)) {
334335
LogPrint(BCLog::INSTANTSEND, "%s -- txid=%s: voted on input %s with id %s\n", __func__,
335336
tx.GetHash().ToString(), in.prevout.ToStringShort(), id.ToString());
336337
}
@@ -388,6 +389,6 @@ void InstantSendSigner::TrySignInstantSendLock(const CTransaction& tx)
388389
txToCreatingInstantSendLocks.emplace(tx.GetHash(), &e.first->second);
389390
}
390391

391-
m_sigman.AsyncSignIfMember(llmqType, m_shareman, id, tx.GetHash(), quorum->m_quorum_base_block_index->GetBlockHash());
392+
m_shareman.AsyncSignIfMember(llmqType, m_sigman, id, tx.GetHash(), quorum->m_quorum_base_block_index->GetBlockHash());
392393
}
393394
} // namespace instantsend

src/llmq/ehf_signals.cpp

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,17 +3,19 @@
33
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
44

55
#include <llmq/ehf_signals.h>
6-
#include <llmq/quorums.h>
7-
#include <llmq/commitment.h>
86

97
#include <chainparams.h>
108
#include <consensus/validation.h>
119
#include <deploymentstatus.h>
12-
#include <evo/mnhftx.h>
1310
#include <index/txindex.h> // g_txindex
1411
#include <primitives/transaction.h>
1512
#include <validation.h>
1613

14+
#include <evo/mnhftx.h>
15+
#include <llmq/commitment.h>
16+
#include <llmq/quorums.h>
17+
#include <llmq/signing_shares.h>
18+
1719
namespace llmq {
1820
CEHFSignalsHandler::CEHFSignalsHandler(ChainstateManager& chainman, CMNHFManager& mnhfman, CSigningManager& sigman,
1921
CSigSharesManager& shareman, const CQuorumManager& qman) :
@@ -76,7 +78,7 @@ void CEHFSignalsHandler::trySignEHFSignal(int bit, const CBlockIndex* const pind
7678
const uint256 msgHash = mnhfPayload.PrepareTx().GetHash();
7779

7880
WITH_LOCK(cs, ids.insert(requestId));
79-
sigman.AsyncSignIfMember(llmqType, shareman, requestId, msgHash, quorum->qc->quorumHash, false, true);
81+
shareman.AsyncSignIfMember(llmqType, sigman, requestId, msgHash, quorum->qc->quorumHash, false, true);
8082
}
8183

8284
MessageProcessingResult CEHFSignalsHandler::HandleNewRecoveredSig(const CRecoveredSig& recoveredSig)

src/llmq/signing.cpp

Lines changed: 0 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -666,80 +666,6 @@ void CSigningManager::UnregisterRecoveredSigsListener(CRecoveredSigsListener* l)
666666
recoveredSigsListeners.erase(itRem, recoveredSigsListeners.end());
667667
}
668668

669-
bool CSigningManager::AsyncSignIfMember(Consensus::LLMQType llmqType, CSigSharesManager& shareman, const uint256& id,
670-
const uint256& msgHash, const uint256& quorumHash, bool allowReSign,
671-
bool allowDiffMsgHashSigning)
672-
{
673-
if (m_mn_activeman == nullptr) return false;
674-
if (m_mn_activeman->GetProTxHash().IsNull()) return false;
675-
676-
const auto quorum = [&]() {
677-
if (quorumHash.IsNull()) {
678-
// This might end up giving different results on different members
679-
// This might happen when we are on the brink of confirming a new quorum
680-
// This gives a slight risk of not getting enough shares to recover a signature
681-
// But at least it shouldn't be possible to get conflicting recovered signatures
682-
// TODO fix this by re-signing when the next block arrives, but only when that block results in a change of the quorum list and no recovered signature has been created in the mean time
683-
const auto &llmq_params_opt = Params().GetLLMQ(llmqType);
684-
assert(llmq_params_opt.has_value());
685-
return SelectQuorumForSigning(llmq_params_opt.value(), m_chainstate.m_chain, qman, id);
686-
} else {
687-
return qman.GetQuorum(llmqType, quorumHash);
688-
}
689-
}();
690-
691-
if (!quorum) {
692-
LogPrint(BCLog::LLMQ, "CSigningManager::%s -- failed to select quorum. id=%s, msgHash=%s\n", __func__, id.ToString(), msgHash.ToString());
693-
return false;
694-
}
695-
696-
if (!quorum->IsValidMember(m_mn_activeman->GetProTxHash())) {
697-
return false;
698-
}
699-
700-
{
701-
bool hasVoted = db.HasVotedOnId(llmqType, id);
702-
if (hasVoted) {
703-
uint256 prevMsgHash;
704-
db.GetVoteForId(llmqType, id, prevMsgHash);
705-
if (msgHash != prevMsgHash) {
706-
if (allowDiffMsgHashSigning) {
707-
LogPrintf("CSigningManager::%s -- already voted for id=%s and msgHash=%s. Signing for different msgHash=%s\n",
708-
__func__, id.ToString(), prevMsgHash.ToString(), msgHash.ToString());
709-
hasVoted = false;
710-
} else {
711-
LogPrintf("CSigningManager::%s -- already voted for id=%s and msgHash=%s. Not voting on conflicting msgHash=%s\n",
712-
__func__, id.ToString(), prevMsgHash.ToString(), msgHash.ToString());
713-
return false;
714-
}
715-
} else if (allowReSign) {
716-
LogPrint(BCLog::LLMQ, "CSigningManager::%s -- already voted for id=%s and msgHash=%s. Resigning!\n", __func__,
717-
id.ToString(), prevMsgHash.ToString());
718-
} else {
719-
LogPrint(BCLog::LLMQ, "CSigningManager::%s -- already voted for id=%s and msgHash=%s. Not voting again.\n", __func__,
720-
id.ToString(), prevMsgHash.ToString());
721-
return false;
722-
}
723-
}
724-
725-
if (db.HasRecoveredSigForId(llmqType, id)) {
726-
// no need to sign it if we already have a recovered sig
727-
return true;
728-
}
729-
if (!hasVoted) {
730-
db.WriteVoteForId(llmqType, id, msgHash);
731-
}
732-
}
733-
734-
if (allowReSign) {
735-
// make us re-announce all known shares (other nodes might have run into a timeout)
736-
shareman.ForceReAnnouncement(quorum, llmqType, id, msgHash);
737-
}
738-
shareman.AsyncSign(quorum, id, msgHash);
739-
740-
return true;
741-
}
742-
743669
bool CSigningManager::HasRecoveredSig(Consensus::LLMQType llmqType, const uint256& id, const uint256& msgHash) const
744670
{
745671
return db.HasRecoveredSig(llmqType, id, msgHash);

src/llmq/signing.h

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -209,22 +209,20 @@ class CSigningManager
209209
EXCLUSIVE_LOCKS_REQUIRED(!cs_pending, !cs_listeners);
210210
bool ProcessPendingRecoveredSigs(PeerManager& peerman)
211211
EXCLUSIVE_LOCKS_REQUIRED(!cs_pending, !cs_listeners); // called from the worker thread of CSigSharesManager
212-
public:
213-
// TODO - should not be public!
212+
213+
// Used by CSigSharesManager
214+
CRecoveredSigsDb& GetDb() { return db; }
214215
void ProcessRecoveredSig(const std::shared_ptr<const CRecoveredSig>& recoveredSig, PeerManager& peerman)
215216
EXCLUSIVE_LOCKS_REQUIRED(!cs_pending, !cs_listeners);
216217

217-
private:
218-
void Cleanup(); // called from the worker thread of CSigSharesManager
218+
// Needed for access to GetDb() and ProcessRecoveredSig()
219+
friend class CSigSharesManager;
219220

220221
public:
221222
// public interface
222223
void RegisterRecoveredSigsListener(CRecoveredSigsListener* l) EXCLUSIVE_LOCKS_REQUIRED(!cs_listeners);
223224
void UnregisterRecoveredSigsListener(CRecoveredSigsListener* l) EXCLUSIVE_LOCKS_REQUIRED(!cs_listeners);
224225

225-
bool AsyncSignIfMember(Consensus::LLMQType llmqType, CSigSharesManager& shareman, const uint256& id,
226-
const uint256& msgHash, const uint256& quorumHash = uint256(), bool allowReSign = false,
227-
bool allowDiffMsgHashSigning = false);
228226
bool HasRecoveredSig(Consensus::LLMQType llmqType, const uint256& id, const uint256& msgHash) const;
229227
bool HasRecoveredSigForId(Consensus::LLMQType llmqType, const uint256& id) const;
230228
bool HasRecoveredSigForSession(const uint256& signHash) const;
@@ -236,8 +234,8 @@ class CSigningManager
236234
private:
237235
std::thread workThread;
238236
CThreadInterrupt workInterrupt;
237+
void Cleanup(); // called from the worker thread of CSigSharesManager
239238
void WorkThreadMain(PeerManager& peerman) EXCLUSIVE_LOCKS_REQUIRED(!cs_pending, !cs_listeners);
240-
;
241239

242240
public:
243241
void StartWorkerThread(PeerManager& peerman);

src/llmq/signing_shares.cpp

Lines changed: 85 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
#include <util/thread.h>
2222
#include <util/time.h>
2323
#include <util/underlying.h>
24+
#include <validation.h>
2425

2526
#include <cxxtimer.hpp>
2627

@@ -179,10 +180,11 @@ void CSigSharesNodeState::RemoveSession(const uint256& signHash)
179180

180181
//////////////////////
181182

182-
CSigSharesManager::CSigSharesManager(CConnman& connman, CSigningManager& _sigman, PeerManager& peerman,
183-
const CActiveMasternodeManager& mn_activeman, const CQuorumManager& _qman,
184-
const CSporkManager& sporkman) :
183+
CSigSharesManager::CSigSharesManager(CConnman& connman, CChainState& chainstate, CSigningManager& _sigman,
184+
PeerManager& peerman, const CActiveMasternodeManager& mn_activeman,
185+
const CQuorumManager& _qman, const CSporkManager& sporkman) :
185186
m_connman{connman},
187+
m_chainstate{chainstate},
186188
sigman{_sigman},
187189
m_peerman{peerman},
188190
m_mn_activeman{mn_activeman},
@@ -866,6 +868,86 @@ CDeterministicMNCPtr CSigSharesManager::SelectMemberForRecovery(const CQuorumCPt
866868
return v[attempt % v.size()].second;
867869
}
868870

871+
bool CSigSharesManager::AsyncSignIfMember(Consensus::LLMQType llmqType, CSigningManager& sigman, const uint256& id,
872+
const uint256& msgHash, const uint256& quorumHash, bool allowReSign,
873+
bool allowDiffMsgHashSigning)
874+
{
875+
AssertLockNotHeld(cs_pendingSigns);
876+
877+
if (m_mn_activeman.GetProTxHash().IsNull()) return false;
878+
879+
const auto quorum = [&]() {
880+
if (quorumHash.IsNull()) {
881+
// This might end up giving different results on different members
882+
// This might happen when we are on the brink of confirming a new quorum
883+
// This gives a slight risk of not getting enough shares to recover a signature
884+
// But at least it shouldn't be possible to get conflicting recovered signatures
885+
// TODO fix this by re-signing when the next block arrives, but only when that block results in a change of
886+
// the quorum list and no recovered signature has been created in the mean time
887+
const auto& llmq_params_opt = Params().GetLLMQ(llmqType);
888+
assert(llmq_params_opt.has_value());
889+
return SelectQuorumForSigning(llmq_params_opt.value(), m_chainstate.m_chain, qman, id);
890+
} else {
891+
return qman.GetQuorum(llmqType, quorumHash);
892+
}
893+
}();
894+
895+
if (!quorum) {
896+
LogPrint(BCLog::LLMQ, "CSigningManager::%s -- failed to select quorum. id=%s, msgHash=%s\n", __func__,
897+
id.ToString(), msgHash.ToString());
898+
return false;
899+
}
900+
901+
if (!quorum->IsValidMember(m_mn_activeman.GetProTxHash())) {
902+
return false;
903+
}
904+
905+
{
906+
auto& db = sigman.GetDb();
907+
bool hasVoted = db.HasVotedOnId(llmqType, id);
908+
if (hasVoted) {
909+
uint256 prevMsgHash;
910+
db.GetVoteForId(llmqType, id, prevMsgHash);
911+
if (msgHash != prevMsgHash) {
912+
if (allowDiffMsgHashSigning) {
913+
LogPrintf("%s -- already voted for id=%s and msgHash=%s. Signing for different " /* Continued */
914+
"msgHash=%s\n",
915+
__func__, id.ToString(), prevMsgHash.ToString(), msgHash.ToString());
916+
hasVoted = false;
917+
} else {
918+
LogPrintf("%s -- already voted for id=%s and msgHash=%s. Not voting on " /* Continued */
919+
"conflicting msgHash=%s\n",
920+
__func__, id.ToString(), prevMsgHash.ToString(), msgHash.ToString());
921+
return false;
922+
}
923+
} else if (allowReSign) {
924+
LogPrint(BCLog::LLMQ, "%s -- already voted for id=%s and msgHash=%s. Resigning!\n", __func__,
925+
id.ToString(), prevMsgHash.ToString());
926+
} else {
927+
LogPrint(BCLog::LLMQ, "%s -- already voted for id=%s and msgHash=%s. Not voting again.\n", __func__,
928+
id.ToString(), prevMsgHash.ToString());
929+
return false;
930+
}
931+
}
932+
933+
if (db.HasRecoveredSigForId(llmqType, id)) {
934+
// no need to sign it if we already have a recovered sig
935+
return true;
936+
}
937+
if (!hasVoted) {
938+
db.WriteVoteForId(llmqType, id, msgHash);
939+
}
940+
}
941+
942+
if (allowReSign) {
943+
// make us re-announce all known shares (other nodes might have run into a timeout)
944+
ForceReAnnouncement(quorum, llmqType, id, msgHash);
945+
}
946+
AsyncSign(quorum, id, msgHash);
947+
948+
return true;
949+
}
950+
869951
void CSigSharesManager::CollectSigSharesToRequest(std::unordered_map<NodeId, Uint256HashMap<CSigSharesInv>>& sigSharesToRequest)
870952
{
871953
AssertLockHeld(cs);

src/llmq/signing_shares.h

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -405,6 +405,7 @@ class CSigSharesManager : public CRecoveredSigsListener
405405
FastRandomContext rnd GUARDED_BY(cs);
406406

407407
CConnman& m_connman;
408+
CChainState& m_chainstate;
408409
CSigningManager& sigman;
409410
PeerManager& m_peerman;
410411
const CActiveMasternodeManager& m_mn_activeman;
@@ -415,9 +416,9 @@ class CSigSharesManager : public CRecoveredSigsListener
415416
std::atomic<uint32_t> recoveredSigsCounter{0};
416417

417418
public:
418-
explicit CSigSharesManager(CConnman& connman, CSigningManager& _sigman, PeerManager& peerman,
419-
const CActiveMasternodeManager& mn_activeman, const CQuorumManager& _qman,
420-
const CSporkManager& sporkman);
419+
explicit CSigSharesManager(CConnman& connman, CChainState& chainstate, CSigningManager& _sigman,
420+
PeerManager& peerman, const CActiveMasternodeManager& mn_activeman,
421+
const CQuorumManager& _qman, const CSporkManager& sporkman);
421422
~CSigSharesManager() override;
422423

423424
CSigSharesManager() = delete;
@@ -439,6 +440,11 @@ class CSigSharesManager : public CRecoveredSigsListener
439440

440441
static CDeterministicMNCPtr SelectMemberForRecovery(const CQuorumCPtr& quorum, const uint256& id, int attempt);
441442

443+
bool AsyncSignIfMember(Consensus::LLMQType llmqType, CSigningManager& sigman, const uint256& id,
444+
const uint256& msgHash, const uint256& quorumHash = uint256(), bool allowReSign = false,
445+
bool allowDiffMsgHashSigning = false)
446+
EXCLUSIVE_LOCKS_REQUIRED(!cs_pendingSigns);
447+
442448
private:
443449
// all of these return false when the currently processed message should be aborted (as each message actually contains multiple messages)
444450
bool ProcessMessageSigSesAnn(const CNode& pfrom, const CSigSesAnn& ann);

src/masternode/active/context.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,8 @@ ActiveContext::ActiveContext(ChainstateManager& chainman, CConnman& connman, CDe
2626
cj_server{std::make_unique<CCoinJoinServer>(chainman, connman, dmnman, dstxman, mn_metaman, mempool, peerman,
2727
mn_activeman, mn_sync, *llmq_ctx.isman)},
2828
gov_signer{std::make_unique<GovernanceSigner>(connman, dmnman, govman, mn_activeman, chainman, mn_sync)},
29-
shareman{std::make_unique<llmq::CSigSharesManager>(connman, *llmq_ctx.sigman, peerman, mn_activeman, *llmq_ctx.qman,
30-
sporkman)},
29+
shareman{std::make_unique<llmq::CSigSharesManager>(connman, chainman.ActiveChainstate(), *llmq_ctx.sigman, peerman,
30+
mn_activeman, *llmq_ctx.qman, sporkman)},
3131
ehf_sighandler{
3232
std::make_unique<llmq::CEHFSignalsHandler>(chainman, mnhfman, *llmq_ctx.sigman, *shareman, *llmq_ctx.qman)},
3333
cl_signer{std::make_unique<chainlock::ChainLockSigner>(chainman.ActiveChainstate(), *llmq_ctx.clhandler,

src/rpc/quorums.cpp

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -464,8 +464,7 @@ static UniValue quorum_sign_helper(const JSONRPCRequest& request, Consensus::LLM
464464
fSubmit = ParseBoolV(request.params[3], "submit");
465465
}
466466
if (fSubmit) {
467-
return llmq_ctx.sigman->AsyncSignIfMember(llmqType, *CHECK_NONFATAL(node.active_ctx)->shareman, id, msgHash,
468-
quorumHash);
467+
return CHECK_NONFATAL(node.active_ctx)->shareman->AsyncSignIfMember(llmqType, *llmq_ctx.sigman, id, msgHash, quorumHash);
469468
} else {
470469
const auto pQuorum = [&]() {
471470
if (quorumHash.IsNull()) {

0 commit comments

Comments
 (0)