fix: correct masternode operator key BLS scheme handling (desync + cross-scheme uniqueness)

doc/release-notes-7473.md

@@ -0,0 +1,25 @@
+Notable changes
+===============
+
+* Masternode operator key uniqueness is now enforced per key rather than per BLS
+  encoding. After the v24 hard fork activates, registering or updating to an
+  operator public key already used by another masternode is rejected regardless of
+  which BLS scheme (legacy or basic) either side is encoded under, closing a gap
+  where the same key could be claimed twice under different encodings.
+
+Updated RPCs
+------------
+
+* After the v24 hard fork activates, a legacy (LegacyBLS) masternode migrates to the
+  basic BLS scheme *in place* when its state version is raised, keeping the same
+  operator key: the stored operator public key is re-encoded from the legacy to the
+  basic scheme, with no key rotation required. `protx update_service` and
+  `protx update_registrar` build the corresponding basic-scheme migration
+  transaction for a legacy masternode, and the masternode is no longer PoSe-banned by
+  the change. The `..._legacy` RPC variants keep the masternode on the legacy scheme.
+  Previously a legacy masternode could not move to the basic scheme without
+  effectively rotating its operator key, and `update_registrar` clamped the
+  transaction version silently.
+
+These changes are gated on the v24 deployment and have no effect until it activates
+(v24 is not yet scheduled on mainnet or testnet).

src/evo/deterministicmns.h

@@ -443,6 +443,33 @@ class CDeterministicMNList
         return GetMN(p->first);
     }
 
+    /**
+     * Is this operator public key already held by a masternode other than `self`, under *either* BLS
+     * encoding?
+     *
+     * mnUniquePropertyMap is keyed by GetUniquePropertyHash(), which serializes its argument, and a
+     * BLS key serializes differently under the legacy and basic schemes. So one public key sits in
+     * one of two possible slots and a single lookup sees only one of them, which is why operator key
+     * uniqueness is otherwise enforced per encoding rather than per key. There are exactly two
+     * schemes, so probing both is O(1) rather than a scan.
+     *
+     * Both slots are probed even when one resolves to `self`: where a cross-scheme duplicate pair
+     * already exists, returning early on a self-match would miss the other member.
+     *
+     * Pass uint256() as `self` to exclude nothing.
+     */
+    [[nodiscard]] bool HasOperatorKeyUnderAnyScheme(const CBLSPublicKey& pubkey, const uint256& self) const
+    {
+        for (const bool legacy_scheme : {true, false}) {
+            CBLSLazyPublicKey wrapped;
+            wrapped.Set(pubkey, legacy_scheme);
+            if (!HasUniqueProperty(wrapped)) continue;
+            const auto holder = GetUniquePropertyMN(wrapped);
+            if (holder && holder->proTxHash != self) return true;
+        }
+        return false;
+    }
+
     // Compare two masternode lists for equality, ignoring non-deterministic members.
     // Non-deterministic members (nTotalRegisteredCount, internalId) can differ between
     // nodes due to different sync histories, but don't affect consensus validity.
@@ -551,7 +578,16 @@ class CDeterministicMNList
     template <typename T>
     [[nodiscard]] bool UpdateUniqueProperty(const CDeterministicMN& dmn, const T& oldValue, const T& newValue)
     {
-        if (oldValue == newValue) {
+        // A BLS operator key can keep the same point while its serialized encoding (legacy<->basic)
+        // changes on a version transition. The map is keyed by GetUniquePropertyHash(), so only that
+        // hash reveals the entry must be re-keyed to the new scheme; CBLSLazyPublicKey::operator==
+        // compares the point and ignores the scheme, so it would wrongly short-circuit. Compare the
+        // serialized hashes for BLS keys and the plain value for every other unique property.
+        if constexpr (std::is_same_v<std::decay_t<T>, CBLSLazyPublicKey>) {
+            if (GetUniquePropertyHash(oldValue) == GetUniquePropertyHash(newValue)) {
+                return true;
+            }
+        } else if (oldValue == newValue) {
             return true;
         }
         static const T nullValue{};

src/evo/dmnstate.h

@@ -236,6 +236,16 @@ class CDeterministicMNStateDiff
                     member.get(state) = member.get(b);
                     fields |= member.mask;
                 }
+            } else if constexpr (BaseType::mask == Field_pubKeyOperator) {
+                // CBLSLazyPublicKey::operator== compares the underlying key and ignores its BLS
+                // encoding, but a scheme migration re-encodes the same key (legacy->basic). That must
+                // be captured in the diff -- GetHash() is scheme-dependent -- or a diff-reconstructed
+                // list keeps the old encoding while an online-built list has the new one, and the two
+                // diverge (mnUniquePropertyMap included).
+                if (member.get(a).GetHash() != member.get(b).GetHash()) {
+                    member.get(state) = member.get(b);
+                    fields |= member.mask;
+                }
             } else {
                 if (member.get(a) != member.get(b)) {
                     member.get(state) = member.get(b);

src/evo/specialtxman.cpp

@@ -41,6 +41,13 @@ static bool AddNetInfoEntries(const std::shared_ptr<NetInfoInterface>& net_info,
     return true;
 }
 
+// Raising a masternode's state version out of the legacy BLS scheme re-encodes its operator key and
+// moves it to a new scheme-dependent unique-property slot; the collision guards key off this.
+static bool IsSchemeMigration(int old_version, int new_version)
+{
+    return old_version == ProTxVersion::LegacyBLS && new_version > ProTxVersion::LegacyBLS;
+}
+
 static bool SetStateVersion(CDeterministicMNState& state_mn, uint16_t nVersion, MnType nType,
                             BlockValidationState& state)
 {
@@ -51,6 +58,19 @@ static bool SetStateVersion(CDeterministicMNState& state_mn, uint16_t nVersion,
         state_mn.payouts = LegacyPayoutAsList(state_mn.scriptPayout);
         state_mn.scriptPayout.clear();
     }
+
+    // Keep the operator key's BLS encoding a deterministic function of nVersion, matching the SML and
+    // on-disk serialization, so the stored key and the (scheme-dependent) unique-property index use
+    // the same scheme on every node whether the list was built online or reloaded from a snapshot.
+    // Set() rather than SetLegacy(): the latter only flips the flag and leaves the cached
+    // serialization in the old encoding, so a reloaded node would decode a different key. This runs
+    // before the early return because callers pre-set nVersion, so the version may already match here
+    // while the key still needs re-encoding.
+    if (state_mn.pubKeyOperator != CBLSLazyPublicKey()) {
+        const CBLSPublicKey& pubkey{state_mn.pubKeyOperator.Get()};
+        state_mn.pubKeyOperator.Set(pubkey, nVersion == ProTxVersion::LegacyBLS);
+    }
+
     if (state_mn.nVersion == nVersion && state_mn.netInfo->CanStorePlatform() == needs_extended) {
         return true;
     }
@@ -384,6 +404,15 @@ bool CSpecialTxProcessor::RebuildListFromBlock(const CBlock& block, gsl::not_nul
             }
             dmn->pdmnState = dmnState;
 
+            // CheckProRegTx ran against pindexPrev, so transactions in this same block are invisible
+            // to each other and two of them could claim one operator key under different encodings.
+            // Re-probe the list as rebuilt so far. AddMN() reports a duplicate by throwing, which
+            // would escape block assembly, so reject cleanly here instead.
+            if (is_v24_deployed &&
+                newList.HasOperatorKeyUnderAnyScheme(dmn->pdmnState->pubKeyOperator.Get(), /*self=*/uint256())) {
+                return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-dup-key");
+            }
+
             newList.AddMN(dmn);
 
             if (debugLogs) {
@@ -461,6 +490,17 @@ bool CSpecialTxProcessor::RebuildListFromBlock(const CBlock& block, gsl::not_nul
                 }
             }
 
+            // Migrating a legacy masternode to the basic scheme re-encodes its stored key
+            // (SetStateVersion), moving it to the basic-scheme slot. Per-transaction checks ran
+            // against pindexPrev, so re-check against the list as rebuilt so far: if another
+            // masternode holds this key under either encoding, the re-key in UpdateMN() would throw
+            // out of block assembly.
+            if (is_v24_deployed && IsSchemeMigration(current_version, target_version) &&
+                newList.HasOperatorKeyUnderAnyScheme(dmn->pdmnState->pubKeyOperator.Get(),
+                                                     /*self=*/opt_proTx->proTxHash)) {
+                return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-dup-key");
+            }
+
             newList.UpdateMN(opt_proTx->proTxHash, newState);
             if (debugLogs) {
                 LogPrintf("%s -- MN %s updated at height %d: %s\n", __func__, opt_proTx->proTxHash.ToString(), nHeight,
@@ -481,19 +521,36 @@ bool CSpecialTxProcessor::RebuildListFromBlock(const CBlock& block, gsl::not_nul
             const bool operator_changed{newState->pubKeyOperator != opt_proTx->pubKeyOperator};
             const uint16_t target_version{is_v24_deployed ? std::max<uint16_t>(old_version, opt_proTx->nVersion)
                                                           : (operator_changed ? opt_proTx->nVersion : old_version)};
+
+            // Per-transaction checks ran against pindexPrev, so an earlier transaction in this same
+            // block is invisible to them. Re-evaluate against the list as rebuilt so far: this update
+            // moves the operator key to a new unique-property slot if it rotates the key or crosses
+            // the legacy->basic boundary (which re-encodes the key), and if that slot is held by
+            // another masternode the re-key in UpdateMN() would throw out of block assembly. Reject
+            // cleanly. Scoped to those two cases so a pre-existing cross-scheme pair's non-migrating
+            // routine update is not blocked.
+            {
+                const bool migrating{IsSchemeMigration(old_version, target_version)};
+                if (is_v24_deployed && (operator_changed || migrating) &&
+                    newList.HasOperatorKeyUnderAnyScheme(opt_proTx->pubKeyOperator.Get(),
+                                                         /*self=*/opt_proTx->proTxHash)) {
+                    return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-dup-key");
+                }
+            }
+
             if (operator_changed) {
                 // reset all operator related fields and put MN into PoSe-banned state in case the operator key changes
                 newState->ResetOperatorFields();
                 newState->BanIfNotBanned(nHeight);
                 newState->pubKeyOperator = opt_proTx->pubKeyOperator;
             }
             newState->keyIDVoting = opt_proTx->keyIDVoting;
+            // SetStateVersion() re-encodes the operator key to target_version's scheme, so the stored
+            // key stays consistent with its version whether it was carried in this payload (possibly
+            // under a different version's encoding) or migrated in place.
             if (!SetStateVersion(*newState, target_version, dmn->nType, state)) {
                 return false;
             }
-            if (operator_changed) {
-                newState->pubKeyOperator.SetLegacy(target_version == ProTxVersion::LegacyBLS);
-            }
             if (target_version >= ProTxVersion::ExtAddr) {
                 newState->payouts = opt_proTx->nVersion >= ProTxVersion::ExtAddr
                     ? opt_proTx->payouts
@@ -1126,6 +1183,15 @@ bool CheckProRegTx(const CTransaction& tx, gsl::not_null<const CBlockIndex*> pin
             return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-dup-key");
         }
 
+        // The check above only sees the operator key under the encoding this payload happens to use,
+        // so it misses a key an existing masternode holds under the other one. A ProRegTx never
+        // proves ownership of the operator key, so that gap lets anyone claim a masternode's key.
+        // Nothing is excluded here: a duplicate key is never allowed, even for a ProTx replacing an
+        // existing masternode.
+        if (is_v24_active && mnList.HasOperatorKeyUnderAnyScheme(opt_ptx->pubKeyOperator.Get(), /*self=*/uint256())) {
+            return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-dup-key");
+        }
+
         // never allow duplicate platformNodeIds for EvoNodes
         if (opt_ptx->nType == MnType::Evo) {
             if (mnList.HasUniqueProperty(opt_ptx->platformNodeID)) {
@@ -1192,6 +1258,16 @@ bool CheckProUpServTx(const CTransaction& tx, gsl::not_null<const CBlockIndex*>
         return false;
     }
 
+    // A service update carries no operator key, but raising a legacy masternode to the basic scheme
+    // re-encodes its stored key, moving it to the basic-scheme unique-property slot. If another
+    // masternode already holds that key under either encoding, the re-key in UpdateMN() would throw
+    // out of block assembly, so reject the migration cleanly here.
+    if (DeploymentActiveAfter(pindexPrev, chainman, Consensus::DEPLOYMENT_V24) &&
+        IsSchemeMigration(dmn->pdmnState->nVersion, opt_ptx->nVersion) &&
+        mnList.HasOperatorKeyUnderAnyScheme(dmn->pdmnState->pubKeyOperator.Get(), /*self=*/opt_ptx->proTxHash)) {
+        return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-dup-key");
+    }
+
     // don't allow updating to addresses already used by other MNs
     for (const auto& entry : opt_ptx->netInfo->GetEntries()) {
         if (const auto service_opt{entry.GetAddrPort()}) {
@@ -1261,6 +1337,20 @@ bool CheckProUpRegTx(const CTransaction& tx, gsl::not_null<const CBlockIndex*> p
         return false;
     }
 
+    // This update moves the masternode's operator key to a new unique-property slot when it either
+    // rotates the key or crosses the legacy->basic scheme boundary (which re-encodes the key). Reject
+    // if that target slot is already held by another masternode -- under either encoding -- so the
+    // re-key in UpdateMN() cannot collide and throw out of block assembly. Scoped to those two cases
+    // so a pre-existing cross-scheme pair's non-migrating routine update is not blocked.
+    if (DeploymentActiveAfter(pindexPrev, chainman, Consensus::DEPLOYMENT_V24)) {
+        const bool key_changed{!(opt_ptx->pubKeyOperator == dmn->pdmnState->pubKeyOperator)};
+        const bool migrating{IsSchemeMigration(dmn->pdmnState->nVersion, opt_ptx->nVersion)};
+        if ((key_changed || migrating) &&
+            mnList.HasOperatorKeyUnderAnyScheme(opt_ptx->pubKeyOperator.Get(), /*self=*/opt_ptx->proTxHash)) {
+            return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-dup-key");
+        }
+    }
+
     const auto owner_payouts = GetOwnerPayouts(*opt_ptx);
     if (!IsPayoutListTriviallyValid(owner_payouts, dmn->pdmnState->keyIDOwner, opt_ptx->keyIDVoting, state)) return false;
 
@@ -1286,6 +1376,8 @@ bool CheckProUpRegTx(const CTransaction& tx, gsl::not_null<const CBlockIndex*> p
             return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-dup-key");
         }
     }
+    // Cross-scheme duplicates for this update are rejected earlier (see the collision guard after the
+    // version-change check), which also covers the key-less migration case.
 
     if (!DeploymentDIP0003Enforced(pindexPrev->nHeight, Params().GetConsensus())) {
         if (dmn->pdmnState->keyIDOwner != opt_ptx->keyIDVoting) {

src/node/miner.cpp

@@ -378,6 +378,23 @@ bool BlockAssembler::TestPackageTransactions(const CTxMemPool::setEntries& packa
             return false;
         }
 
+        // A special transaction that was valid when it entered the mempool can be invalidated by
+        // intervening state, most sharply by a fork activating a rule it violates, and nothing
+        // evicts it. Selection otherwise trusts mempool validity, so the entry would be picked into
+        // every template and TestBlockValidity would then reject the whole block, leaving an honest
+        // miner unable to produce one at all. Recheck here, at package granularity: the caller adds
+        // every member of a package that passes, so dropping one member while keeping its
+        // descendants would itself yield an invalid template. check_sigs is off because signatures
+        // were verified on entry and cannot have changed; note CheckMNHFTx() verifies its quorum
+        // signature regardless, which is cheap enough here given how rare MNHF signals are.
+        if (it->GetTx().IsSpecialTxVersion()) {
+            TxValidationState tx_state;
+            if (!m_chain_helper.special_tx->CheckSpecialTx(it->GetTx(), m_chainstate.m_chain.Tip(),
+                                                           m_chainstate.CoinsTip(), /*check_sigs=*/false, tx_state)) {
+                return false;
+            }
+        }
+
         const auto& txid = it->GetTx().GetHash();
         if (!m_isman.IsInstantSendEnabled() || m_isman.IsLocked(txid)) {
             continue;

src/node/miner.h

@@ -208,7 +208,7 @@ class BlockAssembler
       * Increments nPackagesSelected / nDescendantsUpdated with corresponding
       * statistics from the package selection (for logging statistics). */
     void addPackageTxs(const CTxMemPool& mempool, int& nPackagesSelected, int& nDescendantsUpdated,
-                       const CBlockIndex* pindexPrev) EXCLUSIVE_LOCKS_REQUIRED(mempool.cs);
+                       const CBlockIndex* pindexPrev) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, mempool.cs);
 
     // helper functions for addPackageTxs()
     /** Remove confirmed (inBlock) entries from given set */
@@ -219,7 +219,7 @@ class BlockAssembler
       * locktime
       * These checks should always succeed, and they're here
       * only as an extra check in case of suboptimal node configuration */
-    bool TestPackageTransactions(const CTxMemPool::setEntries& package) const;
+    bool TestPackageTransactions(const CTxMemPool::setEntries& package) const EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
     /** Sort the package in an order that is valid to appear in a block */
     void SortForBlock(const CTxMemPool::setEntries& package, std::vector<CTxMemPool::txiter>& sortedEntries);
 };

src/rpc/evo.cpp

@@ -1246,6 +1246,18 @@ static RPCHelpMan protx_update_registrar_wrapper(const bool specific_legacy_bls_
         ptx.pubKeyOperator = dmn->pdmnState->pubKeyOperator;
     }
 
+    // A legacy masternode migrates to the basic scheme via a (non-legacy) registrar update, keeping
+    // its operator key (migration) or supplying a new one (rotation). ptx.nVersion is already set from
+    // the deployment state above -- LegacyBLS for the legacy-BLS RPC variant, basic otherwise -- so
+    // only the key encoding needs fixing up here: a reused key is stored in the legacy encoding, so
+    // re-encode it to the basic scheme to keep the stored key consistent with its version (and the
+    // assertion below holding). A freshly parsed key already matches the requested scheme, and basic
+    // masternodes are untouched.
+    if (!use_legacy && ptx.pubKeyOperator != CBLSLazyPublicKey() && ptx.pubKeyOperator.IsLegacy()) {
+        const CBLSPublicKey& pubkey{ptx.pubKeyOperator.Get()};
+        ptx.pubKeyOperator.Set(pubkey, /*specificLegacyScheme=*/false);
+    }
+
     CHECK_NONFATAL(ptx.pubKeyOperator.IsLegacy() == (ptx.nVersion == ProTxVersion::LegacyBLS));
 
     if (!request.params[2].get_str().empty()) {