Make CDeterministicMN::internalId private to make sure it's set/accessed properly (#3505)

* Make internalId private to make sure it's set/accessed properly

* evo: Make internalId only accessible by the constructor

* Tweak constructors

Co-authored-by: xdustinface <xdustinfacex@gmail.com>

Author: UdjinM6

src/evo/deterministicmns.cpp

@@ -65,6 +65,13 @@ void CDeterministicMNState::ToJson(UniValue& obj) const
     }
 }
 
+uint64_t CDeterministicMN::GetInternalId() const
+{
+    // can't get it if it wasn't set yet
+    assert(internalId != std::numeric_limits<uint64_t>::max());
+    return internalId;
+}
+
 std::string CDeterministicMN::ToString() const
 {
     return strprintf("CDeterministicMN(proTxHash=%s, collateralOutpoint=%s, nOperatorReward=%f, state=%s", proTxHash.ToString(), collateralOutpoint.ToStringShort(), (double)nOperatorReward / 100, pdmnState->ToString());
@@ -359,21 +366,21 @@ CDeterministicMNListDiff CDeterministicMNList::BuildDiff(const CDeterministicMNL
         } else if (fromPtr != toPtr || fromPtr->pdmnState != toPtr->pdmnState) {
             CDeterministicMNStateDiff stateDiff(*fromPtr->pdmnState, *toPtr->pdmnState);
             if (stateDiff.fields) {
-                diffRet.updatedMNs.emplace(toPtr->internalId, std::move(stateDiff));
+                diffRet.updatedMNs.emplace(toPtr->GetInternalId(), std::move(stateDiff));
             }
         }
     });
     ForEachMN(false, [&](const CDeterministicMNCPtr& fromPtr) {
         auto toPtr = to.GetMN(fromPtr->proTxHash);
         if (toPtr == nullptr) {
-            diffRet.removedMns.emplace(fromPtr->internalId);
+            diffRet.removedMns.emplace(fromPtr->GetInternalId());
         }
     });
 
     // added MNs need to be sorted by internalId so that these are added in correct order when the diff is applied later
     // otherwise internalIds will not match with the original list
     std::sort(diffRet.addedMNs.begin(), diffRet.addedMNs.end(), [](const CDeterministicMNCPtr& a, const CDeterministicMNCPtr& b) {
-        return a->internalId < b->internalId;
+        return a->GetInternalId() < b->GetInternalId();
     });
 
     return diffRet;
@@ -438,8 +445,8 @@ void CDeterministicMNList::AddMN(const CDeterministicMNCPtr& dmn, bool fBumpTota
     if (mnMap.find(dmn->proTxHash)) {
         throw(std::runtime_error(strprintf("%s: can't add a duplicate masternode with the same proTxHash=%s", __func__, dmn->proTxHash.ToString())));
     }
-    if (mnInternalIdMap.find(dmn->internalId)) {
-        throw(std::runtime_error(strprintf("%s: can't add a duplicate masternode with the same internalId=%d", __func__, dmn->internalId)));
+    if (mnInternalIdMap.find(dmn->GetInternalId())) {
+        throw(std::runtime_error(strprintf("%s: can't add a duplicate masternode with the same internalId=%d", __func__, dmn->GetInternalId())));
     }
     if (HasUniqueProperty(dmn->pdmnState->addr)) {
         throw(std::runtime_error(strprintf("%s: can't add a masternode with a duplicate address %s", __func__, dmn->pdmnState->addr.ToStringIPPort(false))));
@@ -449,7 +456,7 @@ void CDeterministicMNList::AddMN(const CDeterministicMNCPtr& dmn, bool fBumpTota
     }
 
     mnMap = mnMap.set(dmn->proTxHash, dmn);
-    mnInternalIdMap = mnInternalIdMap.set(dmn->internalId, dmn->proTxHash);
+    mnInternalIdMap = mnInternalIdMap.set(dmn->GetInternalId(), dmn->proTxHash);
     AddUniqueProperty(dmn, dmn->collateralOutpoint);
     if (dmn->pdmnState->addr != CService()) {
         AddUniqueProperty(dmn, dmn->pdmnState->addr);
@@ -460,7 +467,7 @@ void CDeterministicMNList::AddMN(const CDeterministicMNCPtr& dmn, bool fBumpTota
     }
     if (fBumpTotalCount) {
         // nTotalRegisteredCount acts more like a checkpoint, not as a limit,
-        nTotalRegisteredCount = std::max(dmn->internalId + 1, (uint64_t)nTotalRegisteredCount);
+        nTotalRegisteredCount = std::max(dmn->GetInternalId() + 1, (uint64_t)nTotalRegisteredCount);
     }
 }
 
@@ -515,7 +522,7 @@ void CDeterministicMNList::RemoveMN(const uint256& proTxHash)
         DeleteUniqueProperty(dmn, dmn->pdmnState->pubKeyOperator);
     }
     mnMap = mnMap.erase(proTxHash);
-    mnInternalIdMap = mnInternalIdMap.erase(dmn->internalId);
+    mnInternalIdMap = mnInternalIdMap.erase(dmn->GetInternalId());
 }
 
 CDeterministicMNManager::CDeterministicMNManager(CEvoDB& _evoDb) :
@@ -680,9 +687,8 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, const C
                 return _state.DoS(100, false, REJECT_INVALID, "bad-protx-payload");
             }
 
-            auto dmn = std::make_shared<CDeterministicMN>();
+            auto dmn = std::make_shared<CDeterministicMN>(newList.GetTotalRegisteredCount());
             dmn->proTxHash = tx.GetHash();
-            dmn->internalId = newList.GetTotalRegisteredCount();
 
             // collateralOutpoint is either pointing to an external collateral or to the ProRegTx itself
             if (proTx.collateralOutpoint.hash.IsNull()) {
@@ -1028,15 +1034,13 @@ bool CDeterministicMNManager::UpgradeDiff(CDBBatch& batch, const CBlockIndex* pi
     CDeterministicMNListDiff newDiff;
     size_t addedCount = 0;
     for (auto& p : oldDiff.addedMNs) {
-        auto dmn = std::make_shared<CDeterministicMN>(*p.second);
-        dmn->internalId = curMNList.GetTotalRegisteredCount() + addedCount;
+        auto dmn = std::make_shared<CDeterministicMN>(*p.second, curMNList.GetTotalRegisteredCount() + addedCount);
         newDiff.addedMNs.emplace_back(dmn);
-
         addedCount++;
     }
     for (auto& p : oldDiff.removedMns) {
         auto dmn = curMNList.GetMN(p);
-        newDiff.removedMns.emplace(dmn->internalId);
+        newDiff.removedMns.emplace(dmn->GetInternalId());
     }
 
     // applies added/removed MNs
@@ -1055,7 +1059,7 @@ bool CDeterministicMNManager::UpgradeDiff(CDBBatch& batch, const CBlockIndex* pi
         }
 
         newDiff.updatedMNs.emplace(std::piecewise_construct,
-                std::forward_as_tuple(oldMN->internalId),
+                std::forward_as_tuple(oldMN->GetInternalId()),
                 std::forward_as_tuple(*oldMN->pdmnState, *newMN->pdmnState));
     }
 

src/evo/deterministicmns.h

@@ -187,16 +187,30 @@ class CDeterministicMNStateDiff
 
 class CDeterministicMN
 {
+private:
+    uint64_t internalId{std::numeric_limits<uint64_t>::max()};
+
 public:
-    CDeterministicMN() {}
+    CDeterministicMN() = delete; // no default constructor, must specify internalId
+    CDeterministicMN(uint64_t _internalId) : internalId(_internalId)
+    {
+        // only non-initial values
+        assert(_internalId != std::numeric_limits<uint64_t>::max());
+    }
+    // TODO: can be removed in a future version
+    CDeterministicMN(const CDeterministicMN& mn, uint64_t _internalId) : CDeterministicMN(mn) {
+        // only non-initial values
+        assert(_internalId != std::numeric_limits<uint64_t>::max());
+        internalId = _internalId;
+    }
+
     template <typename Stream>
     CDeterministicMN(deserialize_type, Stream& s)
     {
         s >> *this;
     }
 
     uint256 proTxHash;
-    uint64_t internalId{std::numeric_limits<uint64_t>::max()};
     COutPoint collateralOutpoint;
     uint16_t nOperatorReward;
     CDeterministicMNStateCPtr pdmnState;
@@ -226,7 +240,8 @@ class CDeterministicMN
         SerializationOp(s, CSerActionUnserialize(), oldFormat);
     }
 
-public:
+    uint64_t GetInternalId() const;
+
     std::string ToString() const;
     void ToJson(UniValue& obj) const;
 };
@@ -603,7 +618,10 @@ class CDeterministicMNListDiff_OldFormat
         size_t cnt = ReadCompactSize(s);
         for (size_t i = 0; i < cnt; i++) {
             uint256 proTxHash;
-            auto dmn = std::make_shared<CDeterministicMN>();
+            // NOTE: This is a hack and "0" is just a dummy id. The actual internalId is assigned to a copy
+            // of this dmn via corresponding ctor when we convert the diff format to a new one in UpgradeDiff
+            // thus the logic that we must set internalId before dmn is used in any meaningful way is preserved.
+            auto dmn = std::make_shared<CDeterministicMN>(0);
             s >> proTxHash;
             dmn->Unserialize(s, true);
             addedMNs.emplace(proTxHash, dmn);

src/evo/mnauth.cpp

@@ -191,10 +191,10 @@ void CMNAuth::NotifyMasternodeListChanged(bool undo, const CDeterministicMNList&
             return;
         }
         bool doRemove = false;
-        if (diff.removedMns.count(verifiedDmn->internalId)) {
+        if (diff.removedMns.count(verifiedDmn->GetInternalId())) {
             doRemove = true;
         } else {
-            auto it = diff.updatedMNs.find(verifiedDmn->internalId);
+            auto it = diff.updatedMNs.find(verifiedDmn->GetInternalId());
             if (it != diff.updatedMNs.end()) {
                 if ((it->second.fields & CDeterministicMNStateDiff::Field_pubKeyOperator) && it->second.state.pubKeyOperator.GetHash() != pnode->verifiedPubKeyHash) {
                     doRemove = true;