fix: multiple 5403 followups (#5424)

## Issue being fixed or feature implemented
- CBLSLazyWrapper is doing to much and not enough at the same time
- nVersion assignment in CDeterministicMNState(Diff) is incomplete
- pubKeyOperator deserialization needs nVersion but nVersion is deser-ed
much later
- protx rpcs are implicitly converting pubKeyOperator (by forcing
nVersion=2), they shouldn't do that

## What was done?
pls see individual commits

## How Has This Been Tested?
- [x] run tests locally 
- [x] reindex on testnet:
  - [x] with and without `--assumevalid=0` to the tip
  - [x] with 19.1 almost to the forkpoint, then with this version
- [x] reindex on mainnet:
  - [x] with and without `--assumevalid=0` to the tip
  - [x] with 19.1 to height 1100000+, then with this version

## Breaking Changes
might need reindexing if you were running develop on testnet already

## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_

src/bls/bls.h

@@ -434,6 +434,7 @@ class CBLSLazyWrapper
         if (r.bufValid) {
             vecBytes = r.vecBytes;
         } else {
+            vecBytes.resize(BLSObject::SerSize);
             std::fill(vecBytes.begin(), vecBytes.end(), 0);
         }
         objInitialized = r.objInitialized;
@@ -456,6 +457,7 @@ class CBLSLazyWrapper
     {
         std::unique_lock<std::mutex> l(mutex);
         if (!objInitialized && !bufValid) {
+            vecBytes.resize(BLSObject::SerSize);
             std::fill(vecBytes.begin(), vecBytes.end(), 0);
         } else if (!bufValid || (bufLegacyScheme != specificLegacyScheme)) {
             vecBytes = obj.ToByteVector(specificLegacyScheme);
@@ -508,21 +510,14 @@ class CBLSLazyWrapper
         if (!objInitialized) {
             obj.SetByteVector(vecBytes, bufLegacyScheme);
             if (!obj.IsValid()) {
-                // If setting of BLS object using one scheme failed, then we need to attempt again with the opposite scheme.
-                // This is due to the fact that LazyBLSWrapper receives a serialised buffer but attempts to create actual BLS object when needed.
-                // That could happen when the fork has been activated and the enforced scheme has switched.
-                obj.SetByteVector(vecBytes, !bufLegacyScheme);
-                if (obj.IsValid()) {
-                    bufLegacyScheme = !bufLegacyScheme;
-                }
+                bufValid = false;
+                return invalidObj;
             }
             if (!obj.CheckMalleable(vecBytes, bufLegacyScheme)) {
                 bufValid = false;
-                objInitialized = false;
-                obj = invalidObj;
-            } else {
-                objInitialized = true;
+                return invalidObj;
             }
+            objInitialized = true;
         }
         return obj;
     }
@@ -547,6 +542,7 @@ class CBLSLazyWrapper
     {
         std::unique_lock<std::mutex> l(mutex);
         if (!objInitialized && !bufValid) {
+            vecBytes.resize(BLSObject::SerSize);
             std::fill(vecBytes.begin(), vecBytes.end(), 0);
             hash.SetNull();
         } else if (!bufValid) {
@@ -562,6 +558,16 @@ class CBLSLazyWrapper
         return hash;
     }
 
+    bool IsLegacy() const
+    {
+        return bufLegacyScheme;
+    }
+
+    void SetLegacy(bool specificLegacyScheme)
+    {
+        bufLegacyScheme = specificLegacyScheme;
+    }
+
     std::string ToString() const
     {
         return Get().ToString(bufLegacyScheme);

src/evo/deterministicmns.cpp

@@ -482,7 +482,6 @@ void CDeterministicMNList::UpdateMN(const CDeterministicMN& oldDmn, const std::s
 {
     auto dmn = std::make_shared<CDeterministicMN>(oldDmn);
     auto oldState = dmn->pdmnState;
-    dmn->pdmnState = pdmnState;
 
     // All mnUniquePropertyMap's updates must be atomic.
     // Using this temporary map as a checkpoint to roll back to in case of any issues.
@@ -504,13 +503,14 @@ void CDeterministicMNList::UpdateMN(const CDeterministicMN& oldDmn, const std::s
                 oldDmn.proTxHash.ToString(), pdmnState->pubKeyOperator.ToString())));
     }
     if (dmn->nType == MnType::HighPerformance) {
-        if (!UpdateUniqueProperty(*dmn, oldState->platformNodeID, dmn->pdmnState->platformNodeID)) {
+        if (!UpdateUniqueProperty(*dmn, oldState->platformNodeID, pdmnState->platformNodeID)) {
             mnUniquePropertyMap = mnUniquePropertyMapSaved;
             throw(std::runtime_error(strprintf("%s: Can't update a masternode %s with a duplicate platformNodeID=%s", __func__,
-                                               dmn->proTxHash.ToString(), dmn->pdmnState->platformNodeID.ToString())));
+                                               oldDmn.proTxHash.ToString(), pdmnState->platformNodeID.ToString())));
         }
     }
 
+    dmn->pdmnState = pdmnState;
     mnMap = mnMap.set(oldDmn.proTxHash, dmn);
 }
 
@@ -862,8 +862,10 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, const C
                 // reset all operator related fields and put MN into PoSe-banned state in case the operator key changes
                 newState->ResetOperatorFields();
                 newState->BanIfNotBanned(nHeight);
+                // we update pubKeyOperator here, make sure state version matches
+                newState->nVersion = proTx.nVersion;
+                newState->pubKeyOperator = proTx.pubKeyOperator;
             }
-            newState->pubKeyOperator = proTx.pubKeyOperator;
             newState->keyIDVoting = proTx.keyIDVoting;
             newState->scriptPayout = proTx.scriptPayout;
 

src/evo/dmnstate.cpp

@@ -25,16 +25,17 @@ std::string CDeterministicMNState::ToString() const
         operatorPayoutAddress = EncodeDestination(dest);
     }
 
-    return strprintf("CDeterministicMNState(nRegisteredHeight=%d, nLastPaidHeight=%d, nPoSePenalty=%d, nPoSeRevivedHeight=%d, nPoSeBanHeight=%d, nRevocationReason=%d, "
+    return strprintf("CDeterministicMNState(nVersion=%d, nRegisteredHeight=%d, nLastPaidHeight=%d, nPoSePenalty=%d, nPoSeRevivedHeight=%d, nPoSeBanHeight=%d, nRevocationReason=%d, "
                      "ownerAddress=%s, pubKeyOperator=%s, votingAddress=%s, addr=%s, payoutAddress=%s, operatorPayoutAddress=%s)",
-                     nRegisteredHeight, nLastPaidHeight, nPoSePenalty, nPoSeRevivedHeight, nPoSeBanHeight, nRevocationReason,
+                     nVersion, nRegisteredHeight, nLastPaidHeight, nPoSePenalty, nPoSeRevivedHeight, nPoSeBanHeight, nRevocationReason,
                      EncodeDestination(PKHash(keyIDOwner)), pubKeyOperator.ToString(), EncodeDestination(PKHash(keyIDVoting)), addr.ToStringIPPort(false), payoutAddress, operatorPayoutAddress);
 }
 
 void CDeterministicMNState::ToJson(UniValue& obj, MnType nType) const
 {
     obj.clear();
     obj.setObject();
+    obj.pushKV("version", nVersion);
     obj.pushKV("service", addr.ToStringIPPort(false));
     obj.pushKV("registeredHeight", nRegisteredHeight);
     obj.pushKV("lastPaidHeight", nLastPaidHeight);
@@ -65,6 +66,9 @@ void CDeterministicMNStateDiff::ToJson(UniValue& obj, MnType nType) const
 {
     obj.clear();
     obj.setObject();
+    if (fields & Field_nVersion) {
+        obj.pushKV("version", state.nVersion);
+    }
     if (fields & Field_addr) {
         obj.pushKV("service", state.addr.ToStringIPPort(false));
     }

src/evo/dmnstate.h

@@ -245,7 +245,8 @@ class CDeterministicMNState
 
     void ResetOperatorFields()
     {
-        pubKeyOperator.Set(CBLSPublicKey(), bls::bls_legacy_scheme.load());
+        nVersion = CProRegTx::LEGACY_BLS_VERSION;
+        pubKeyOperator = CBLSLazyPublicKey();
         addr = CService();
         scriptOperatorPayout = CScript();
         nRevocationReason = CProUpRevTx::REASON_NOT_SPECIFIED;
@@ -343,20 +344,28 @@ class CDeterministicMNStateDiff
 #define DMN_STATE_DIFF_LINE(f) if (a.f != b.f) { state.f = b.f; fields |= Field_##f; }
         DMN_STATE_DIFF_ALL_FIELDS
 #undef DMN_STATE_DIFF_LINE
+        if (fields & Field_pubKeyOperator) { state.nVersion = b.nVersion; fields |= Field_nVersion; }
     }
 
     void ToJson(UniValue& obj, MnType nType) const;
 
     SERIALIZE_METHODS(CDeterministicMNStateDiff, obj)
     {
+        // NOTE: reading pubKeyOperator requires nVersion
+        bool read_pubkey{false};
         READWRITE(VARINT(obj.fields));
 #define DMN_STATE_DIFF_LINE(f) \
         if (strcmp(#f, "pubKeyOperator") == 0 && (obj.fields & Field_pubKeyOperator)) {\
+            SER_READ(obj, read_pubkey = true); \
             READWRITE(CBLSLazyPublicKeyVersionWrapper(const_cast<CBLSLazyPublicKey&>(obj.state.pubKeyOperator), obj.state.nVersion == CProRegTx::LEGACY_BLS_VERSION)); \
         } else if (obj.fields & Field_##f) READWRITE(obj.state.f);
 
         DMN_STATE_DIFF_ALL_FIELDS
 #undef DMN_STATE_DIFF_LINE
+        if (read_pubkey) {
+            SER_READ(obj, obj.fields |= Field_nVersion);
+            SER_READ(obj, obj.state.pubKeyOperator.SetLegacy(obj.state.nVersion == CProRegTx::LEGACY_BLS_VERSION));
+        }
     }
 
     void ApplyToState(CDeterministicMNState& target) const

src/evo/providertx.cpp

@@ -26,6 +26,9 @@ bool CProRegTx::IsTriviallyValid(bool is_bls_legacy_scheme, TxValidationState& s
     if (keyIDOwner.IsNull() || !pubKeyOperator.Get().IsValid() || keyIDVoting.IsNull()) {
         return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-key-null");
     }
+    if (pubKeyOperator.IsLegacy() != (nVersion == LEGACY_BLS_VERSION)) {
+        return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-operator-pubkey");
+    }
     if (!scriptPayout.IsPayToPublicKeyHash() && !scriptPayout.IsPayToScriptHash()) {
         return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-payee");
     }
@@ -117,6 +120,9 @@ bool CProUpRegTx::IsTriviallyValid(bool is_bls_legacy_scheme, TxValidationState&
     if (!pubKeyOperator.Get().IsValid() || keyIDVoting.IsNull()) {
         return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-key-null");
     }
+    if (pubKeyOperator.IsLegacy() != (nVersion == LEGACY_BLS_VERSION)) {
+        return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-operator-pubkey");
+    }
     if (!scriptPayout.IsPayToPublicKeyHash() && !scriptPayout.IsPayToScriptHash()) {
         return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-payee");
     }

src/evo/simplifiedmns.cpp

@@ -58,14 +58,15 @@ std::string CSimplifiedMNListEntry::ToString() const
         operatorPayoutAddress = EncodeDestination(dest);
     }
 
-    return strprintf("CSimplifiedMNListEntry(nType=%d, proRegTxHash=%s, confirmedHash=%s, service=%s, pubKeyOperator=%s, votingAddress=%s, isValid=%d, payoutAddress=%s, operatorPayoutAddress=%s, platformHTTPPort=%d, platformNodeID=%s)",
-                     ToUnderlying(nType), proRegTxHash.ToString(), confirmedHash.ToString(), service.ToString(false), pubKeyOperator.Get().ToString(), EncodeDestination(PKHash(keyIDVoting)), isValid, payoutAddress, operatorPayoutAddress, platformHTTPPort, platformNodeID.ToString());
+    return strprintf("CSimplifiedMNListEntry(nVersion=%d, nType=%d, proRegTxHash=%s, confirmedHash=%s, service=%s, pubKeyOperator=%s, votingAddress=%s, isValid=%d, payoutAddress=%s, operatorPayoutAddress=%s, platformHTTPPort=%d, platformNodeID=%s)",
+                     nVersion, ToUnderlying(nType), proRegTxHash.ToString(), confirmedHash.ToString(), service.ToString(false), pubKeyOperator.ToString(), EncodeDestination(PKHash(keyIDVoting)), isValid, payoutAddress, operatorPayoutAddress, platformHTTPPort, platformNodeID.ToString());
 }
 
 void CSimplifiedMNListEntry::ToJson(UniValue& obj, bool extended) const
 {
     obj.clear();
     obj.setObject();
+    obj.pushKV("nVersion", nVersion);
     obj.pushKV("proRegTxHash", proRegTxHash.ToString());
     obj.pushKV("confirmedHash", confirmedHash.ToString());
     obj.pushKV("service", service.ToString(false));

src/rpc/evo.cpp

@@ -183,9 +183,7 @@ static CKeyID ParsePubKeyIDFromAddress(const std::string& strAddress, const std:
 static CBLSPublicKey ParseBLSPubKey(const std::string& hexKey, const std::string& paramName, bool specific_legacy_bls_scheme = false)
 {
     CBLSPublicKey pubKey;
-    bool is_bls_legacy_scheme = !llmq::utils::IsV19Active(::ChainActive().Tip());
-    bool use_bls_scheme = specific_legacy_bls_scheme || is_bls_legacy_scheme;
-    if (!pubKey.SetHexStr(hexKey, use_bls_scheme)) {
+    if (!pubKey.SetHexStr(hexKey, specific_legacy_bls_scheme)) {
         throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must be a valid BLS public key, not %s", paramName, hexKey));
     }
     return pubKey;
@@ -608,7 +606,7 @@ static UniValue protx_register_common_wrapper(const JSONRPCRequest& request,
     }
 
     bool isV19active = llmq::utils::IsV19Active(WITH_LOCK(cs_main, return ::ChainActive().Tip();));
-    if (mnType == MnType::HighPerformance && !isV19active) {
+    if (isHPMNrequested && !isV19active) {
         throw JSONRPCError(RPC_INVALID_REQUEST, "HPMN aren't allowed yet");
     }
 
@@ -618,12 +616,9 @@ static UniValue protx_register_common_wrapper(const JSONRPCRequest& request,
     tx.nVersion = 3;
     tx.nType = TRANSACTION_PROVIDER_REGISTER;
 
+    const bool use_legacy = isV19active ? specific_legacy_bls_scheme : true;
+
     CProRegTx ptx;
-    if (specific_legacy_bls_scheme && !isHPMNrequested) {
-        ptx.nVersion = CProRegTx::LEGACY_BLS_VERSION;
-    } else {
-        ptx.nVersion = CProRegTx::GetVersion(isV19active);
-    }
     ptx.nType = mnType;
 
     if (isFundRegister) {
@@ -660,7 +655,10 @@ static UniValue protx_register_common_wrapper(const JSONRPCRequest& request,
     }
 
     ptx.keyIDOwner = ParsePubKeyIDFromAddress(request.params[paramIdx + 1].get_str(), "owner address");
-    ptx.pubKeyOperator.Set(ParseBLSPubKey(request.params[paramIdx + 2].get_str(), "operator BLS address", ptx.nVersion == CProRegTx::LEGACY_BLS_VERSION), ptx.nVersion == CProRegTx::LEGACY_BLS_VERSION);
+    ptx.pubKeyOperator.Set(ParseBLSPubKey(request.params[paramIdx + 2].get_str(), "operator BLS address", use_legacy), use_legacy);
+    ptx.nVersion = use_legacy ? CProRegTx::LEGACY_BLS_VERSION : CProRegTx::BASIC_BLS_VERSION;
+    CHECK_NONFATAL(ptx.pubKeyOperator.IsLegacy() == (ptx.nVersion == CProRegTx::LEGACY_BLS_VERSION));
+
     CKeyID keyIDVoting = ptx.keyIDOwner;
 
     if (request.params[paramIdx + 3].get_str() != "") {
@@ -903,7 +901,6 @@ static UniValue protx_update_service_common_wrapper(const JSONRPCRequest& reques
     }
 
     CProUpServTx ptx;
-    ptx.nVersion = CProUpServTx::GetVersion(llmq::utils::IsV19Active(::ChainActive().Tip()));
     ptx.nType = mnType;
     ptx.proTxHash = ParseHashV(request.params[0], "proTxHash");
 
@@ -942,6 +939,7 @@ static UniValue protx_update_service_common_wrapper(const JSONRPCRequest& reques
     if (dmn->nType != mnType) {
         throw std::runtime_error(strprintf("masternode with proTxHash %s is not a %s", ptx.proTxHash.ToString(), GetMnType(mnType).description));
     }
+    ptx.nVersion = dmn->pdmnState->nVersion;
 
     if (keyOperator.GetPublicKey() != dmn->pdmnState->pubKeyOperator.Get()) {
         throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("the operator key does not belong to the registered public key"));
@@ -1028,24 +1026,28 @@ static UniValue protx_update_registrar_wrapper(const JSONRPCRequest& request, co
     EnsureWalletIsUnlocked(wallet.get());
 
     CProUpRegTx ptx;
-    if (specific_legacy_bls_scheme) {
-        ptx.nVersion = CProUpRegTx::LEGACY_BLS_VERSION;
-    } else {
-        ptx.nVersion = CProUpRegTx::GetVersion(llmq::utils::IsV19Active(::ChainActive().Tip()));
-    }
     ptx.proTxHash = ParseHashV(request.params[0], "proTxHash");
 
     auto dmn = deterministicMNManager->GetListAtChainTip().GetMN(ptx.proTxHash);
     if (!dmn) {
         throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("masternode %s not found", ptx.proTxHash.ToString()));
     }
-    ptx.pubKeyOperator = dmn->pdmnState->pubKeyOperator;
     ptx.keyIDVoting = dmn->pdmnState->keyIDVoting;
     ptx.scriptPayout = dmn->pdmnState->scriptPayout;
 
+    const bool use_legacy = llmq::utils::IsV19Active(::ChainActive().Tip()) ? specific_legacy_bls_scheme : true;
+
     if (request.params[1].get_str() != "") {
-        ptx.pubKeyOperator.Set(ParseBLSPubKey(request.params[1].get_str(), "operator BLS address", ptx.nVersion == CProUpRegTx::LEGACY_BLS_VERSION), ptx.nVersion == CProRegTx::LEGACY_BLS_VERSION);
+        // new pubkey
+        ptx.pubKeyOperator.Set(ParseBLSPubKey(request.params[1].get_str(), "operator BLS address", use_legacy), use_legacy);
+    } else {
+        // same pubkey, reuse as is
+        ptx.pubKeyOperator = dmn->pdmnState->pubKeyOperator;
     }
+
+    ptx.nVersion = use_legacy ? CProUpRegTx::LEGACY_BLS_VERSION : CProUpRegTx::BASIC_BLS_VERSION;
+    CHECK_NONFATAL(ptx.pubKeyOperator.IsLegacy() == (ptx.nVersion == CProUpRegTx::LEGACY_BLS_VERSION));
+
     if (request.params[2].get_str() != "") {
         ptx.keyIDVoting = ParsePubKeyIDFromAddress(request.params[2].get_str(), "voting address");
     }