From 0f3e00ce03dc2b0ff25fd718ab94d37c8c8c0827 Mon Sep 17 00:00:00 2001 From: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com> Date: Tue, 6 Sep 2022 19:32:53 +0200 Subject: [PATCH] refactor: create an enum for DKGError, instead of passing around potentially invalid strings (#4998) * refactor: create an enum for DKGError, instead of passing around potentially invalid strings This also enables us to utilize an std::array instead of a std::map This also removes the CCriticalSection and instead utilizes atomic doubles This also adds safety to the dkgsimerror rpc rejecting invalid types * test: add some tests for DKGError --- src/Makefile.test.include | 1 + src/llmq/dkgsession.cpp | 45 +++++++++++++------------------------ src/llmq/dkgsession.h | 35 ++++++++++++++++++++++++----- src/rpc/rpcquorums.cpp | 12 ++++++---- src/test/llmq_dkg_tests.cpp | 27 ++++++++++++++++++++++ 5 files changed, 81 insertions(+), 39 deletions(-) create mode 100644 src/test/llmq_dkg_tests.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 7bb902a69e..279e94309e 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -257,6 +257,7 @@ BITCOIN_TESTS =\ test/key_tests.cpp \ test/lcg.h \ test/limitedmap_tests.cpp \ + test/llmq_dkg_tests.cpp \ test/logging_tests.cpp \ test/dbwrapper_tests.cpp \ test/validation_tests.cpp \ diff --git a/src/llmq/dkgsession.cpp b/src/llmq/dkgsession.cpp index fc99fbe6ff..8495e5fe0f 100644 --- a/src/llmq/dkgsession.cpp +++ b/src/llmq/dkgsession.cpp @@ -24,36 +24,21 @@ namespace llmq { +static std::array, int(DKGError::type::_COUNT)> simDkgErrorMap{}; -// Supported error types: -// - contribution-omit -// - contribution-lie -// - complain-lie -// - justify-lie -// - justify-omit -// - commit-omit -// - commit-lie - -static CCriticalSection cs_simDkgError; -static std::map simDkgErrorMap; - -void SetSimulatedDKGErrorRate(const std::string& type, double rate) +void SetSimulatedDKGErrorRate(DKGError::type type, double rate) { - LOCK(cs_simDkgError); - simDkgErrorMap[type] = rate; + if (int(type) >= DKGError::type::_COUNT) return; + simDkgErrorMap[int(type)] = rate; } -static double GetSimulatedErrorRate(const std::string& type) +double GetSimulatedErrorRate(DKGError::type type) { - LOCK(cs_simDkgError); - auto it = simDkgErrorMap.find(type); - if (it != simDkgErrorMap.end()) { - return it->second; - } - return 0; + if (int(type) >= DKGError::type::_COUNT) return 0; + return simDkgErrorMap[int(type)]; } -bool CDKGSession::ShouldSimulateError(const std::string& type) const +bool CDKGSession::ShouldSimulateError(DKGError::type type) const { if (params.type != Consensus::LLMQType::LLMQ_TEST) { return false; @@ -162,7 +147,7 @@ void CDKGSession::SendContributions(CDKGPendingMessages& pendingMessages) logger.Batch("sending contributions"); - if (ShouldSimulateError("contribution-omit")) { + if (ShouldSimulateError(DKGError::type::CONTRIBUTION_OMIT)) { logger.Batch("omitting"); return; } @@ -181,7 +166,7 @@ void CDKGSession::SendContributions(CDKGPendingMessages& pendingMessages) const auto& m = members[i]; CBLSSecretKey skContrib = m_sk_contributions[i]; - if (i != myIdx && ShouldSimulateError("contribution-lie")) { + if (i != myIdx && ShouldSimulateError(DKGError::type::CONTRIBUTION_LIE)) { logger.Batch("lying for %s", m->dmn->proTxHash.ToString()); skContrib.MakeNewKey(); } @@ -314,7 +299,7 @@ void CDKGSession::ReceiveMessage(const CDKGContribution& qc, bool& retBan) if (!qc.contributions->Decrypt(*myIdx, WITH_LOCK(activeMasternodeInfoCs, return *activeMasternodeInfo.blsKeyOperator), skContribution, PROTOCOL_VERSION)) { logger.Batch("contribution from %s could not be decrypted", member->dmn->proTxHash.ToString()); complain = true; - } else if (member->idx != myIdx && ShouldSimulateError("complain-lie")) { + } else if (member->idx != myIdx && ShouldSimulateError(DKGError::type::COMPLAIN_LIE)) { logger.Batch("lying/complaining for %s", member->dmn->proTxHash.ToString()); complain = true; } @@ -691,7 +676,7 @@ void CDKGSession::SendJustification(CDKGPendingMessages& pendingMessages, const CBLSSecretKey skContribution = m_sk_contributions[i]; - if (i != myIdx && ShouldSimulateError("justify-lie")) { + if (i != myIdx && ShouldSimulateError(DKGError::type::JUSTIFY_LIE)) { logger.Batch("lying for %s", m->dmn->proTxHash.ToString()); skContribution.MakeNewKey(); } @@ -699,7 +684,7 @@ void CDKGSession::SendJustification(CDKGPendingMessages& pendingMessages, const qj.contributions.emplace_back(i, skContribution); } - if (ShouldSimulateError("justify-omit")) { + if (ShouldSimulateError(DKGError::type::JUSTIFY_OMIT)) { logger.Batch("omitting"); return; } @@ -941,7 +926,7 @@ void CDKGSession::SendCommitment(CDKGPendingMessages& pendingMessages) return; } - if (ShouldSimulateError("commit-omit")) { + if (ShouldSimulateError(DKGError::type::COMMIT_OMIT)) { logger.Batch("omitting"); return; } @@ -979,7 +964,7 @@ void CDKGSession::SendCommitment(CDKGPendingMessages& pendingMessages) qc.quorumVvecHash = ::SerializeHash(*vvec); int lieType = -1; - if (ShouldSimulateError("commit-lie")) { + if (ShouldSimulateError(DKGError::type::COMMIT_LIE)) { lieType = GetRandInt(5); logger.Batch("lying on commitment. lieType=%d", lieType); } diff --git a/src/llmq/dkgsession.h b/src/llmq/dkgsession.h index d5f37b6237..f95e2d0e6c 100644 --- a/src/llmq/dkgsession.h +++ b/src/llmq/dkgsession.h @@ -210,6 +210,30 @@ public: bool someoneComplain{false}; }; +class DKGError { +public: + enum type { + COMPLAIN_LIE = 0, + COMMIT_OMIT, + COMMIT_LIE, + CONTRIBUTION_OMIT, + CONTRIBUTION_LIE, + JUSTIFY_OMIT, + JUSTIFY_LIE, + _COUNT + }; + static constexpr DKGError::type from_string(std::string_view in) { + if (in == "complain-lie") return COMPLAIN_LIE; + if (in == "commit-omit") return COMMIT_OMIT; + if (in == "commit-lie") return COMMIT_LIE; + if (in == "contribution-omit") return CONTRIBUTION_OMIT; + if (in == "contribution-lie") return CONTRIBUTION_LIE; + if (in == "justify-lie") return JUSTIFY_LIE; + if (in == "justify-omit") return JUSTIFY_OMIT; + return _COUNT; + } +}; + /** * The DKG session is a single instance of the DKG process. It is owned and called by CDKGSessionHandler, which passes * received DKG messages to the session. The session is not persistent and will loose it's state (the whole object is @@ -281,7 +305,7 @@ public: bool Init(const CBlockIndex* pQuorumBaseBlockIndex, const std::vector& mns, const uint256& _myProTxHash, int _quorumIndex); - std::optional GetMyMemberIndex() const { return myIdx; } + [[nodiscard]] std::optional GetMyMemberIndex() const { return myIdx; } /** * The following sets of methods are for the first 4 phases handled in the session. The flow of message calls @@ -325,16 +349,16 @@ public: // Phase 5: aggregate/finalize std::vector FinalizeCommitments(); - bool AreWeMember() const { return !myProTxHash.IsNull(); } + [[nodiscard]] bool AreWeMember() const { return !myProTxHash.IsNull(); } void MarkBadMember(size_t idx); void RelayInvToParticipants(const CInv& inv) const; public: - CDKGMember* GetMember(const uint256& proTxHash) const; + [[nodiscard]] CDKGMember* GetMember(const uint256& proTxHash) const; private: - bool ShouldSimulateError(const std::string& type) const; + [[nodiscard]] bool ShouldSimulateError(DKGError::type type) const; }; class CDKGLogger : public CBatchedLogger @@ -346,7 +370,8 @@ public: CBatchedLogger(BCLog::LLMQ_DKG, strprintf("QuorumDKG(type=%s, quorumIndex=%d, height=%d, member=%d, func=%s)", _llmqTypeName, _quorumIndex, _height, _areWeMember, _func)){}; }; -void SetSimulatedDKGErrorRate(const std::string& type, double rate); +void SetSimulatedDKGErrorRate(DKGError::type type, double rate); +double GetSimulatedErrorRate(DKGError::type type); } // namespace llmq diff --git a/src/rpc/rpcquorums.cpp b/src/rpc/rpcquorums.cpp index 7afb0a6b75..00ca8edae5 100644 --- a/src/rpc/rpcquorums.cpp +++ b/src/rpc/rpcquorums.cpp @@ -573,16 +573,20 @@ static UniValue quorum_dkgsimerror(const JSONRPCRequest& request) { quorum_dkgsimerror_help(request); - std::string type = request.params[0].get_str(); + std::string type_str = request.params[0].get_str(); double rate = ParseDoubleV(request.params[1], "rate"); if (rate < 0 || rate > 1) { throw JSONRPCError(RPC_INVALID_PARAMETER, "invalid rate. Must be between 0 and 1"); } - llmq::SetSimulatedDKGErrorRate(type, rate); - - return UniValue(); + if (const llmq::DKGError::type type = llmq::DKGError::from_string(type_str); + type == llmq::DKGError::type::_COUNT) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "invalid type. See DKGError class implementation"); + } else { + llmq::SetSimulatedDKGErrorRate(type, rate); + return UniValue(); + } } static void quorum_getdata_help(const JSONRPCRequest& request) diff --git a/src/test/llmq_dkg_tests.cpp b/src/test/llmq_dkg_tests.cpp new file mode 100644 index 0000000000..3f37a29ba6 --- /dev/null +++ b/src/test/llmq_dkg_tests.cpp @@ -0,0 +1,27 @@ +// Copyright (c) 2022 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 +#include + +#include + +BOOST_AUTO_TEST_SUITE(llmq_dkg_tests) + +BOOST_AUTO_TEST_CASE(llmq_dkgerror) +{ + using namespace llmq; + for (auto i : irange::range(int(llmq::DKGError::type::_COUNT))) { + BOOST_ASSERT(GetSimulatedErrorRate(llmq::DKGError::type(i)) == 0.0); + SetSimulatedDKGErrorRate(llmq::DKGError::type(i), 1.0); + BOOST_ASSERT(GetSimulatedErrorRate(llmq::DKGError::type(i)) == 1.0); + } + BOOST_ASSERT(GetSimulatedErrorRate(llmq::DKGError::type::_COUNT) == 0.0); + SetSimulatedDKGErrorRate(llmq::DKGError::type::_COUNT, 1.0); + BOOST_ASSERT(GetSimulatedErrorRate(llmq::DKGError::type::_COUNT) == 0.0); +} + + + +BOOST_AUTO_TEST_SUITE_END()