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

@@ -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 \

src/llmq/dkgsession.cpp

@@ -24,36 +24,21 @@
 
 namespace llmq
 {
+static std::array<std::atomic<double>, 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<std::string, double> 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);
     }

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<CDeterministicMNCPtr>& mns, const uint256& _myProTxHash, int _quorumIndex);
 
-    std::optional<size_t> GetMyMemberIndex() const { return myIdx; }
+    [[nodiscard]] std::optional<size_t> 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<CFinalCommitment> 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
 

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)

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 <llmq/dkgsession.h>
+#include <util/irange.h>
+
+#include <boost/test/unit_test.hpp>
+
+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()