Skip to content

Commit 60401eb

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 d5012c5 commit 60401eb

File tree

9 files changed

+112
-94
lines changed

9 files changed

+112
-94
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: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -222,9 +222,6 @@ class CSigningManager
222222
void RegisterRecoveredSigsListener(CRecoveredSigsListener* l) EXCLUSIVE_LOCKS_REQUIRED(!cs_listeners);
223223
void UnregisterRecoveredSigsListener(CRecoveredSigsListener* l) EXCLUSIVE_LOCKS_REQUIRED(!cs_listeners);
224224

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);
228225
bool HasRecoveredSig(Consensus::LLMQType llmqType, const uint256& id, const uint256& msgHash) const;
229226
bool HasRecoveredSigForId(Consensus::LLMQType llmqType, const uint256& id) const;
230227
bool HasRecoveredSigForSession(const uint256& signHash) const;
@@ -243,6 +240,10 @@ class CSigningManager
243240
void StartWorkerThread(PeerManager& peerman);
244241
void StopWorkerThread();
245242
void InterruptWorkerThread();
243+
244+
CRecoveredSigsDb& GetDb() { return db; }
245+
// Needed for access to GetDb()
246+
friend class CSigSharesManager;
246247
};
247248

248249
template<typename NodesContainer, typename Continue, typename Callback>

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},
@@ -867,6 +869,86 @@ CDeterministicMNCPtr CSigSharesManager::SelectMemberForRecovery(const CQuorumCPt
867869
return v[attempt % v.size()].second;
868870
}
869871

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