Merge #6729: feat: prohibit new legacy scheme masternodes, restrict ProTx version changes with DEPLOYMENT_V23

af10784 doc: add release notes for version restrictions (Kittywhiskers Van Gogh)
991f14d evo: prohibit new legacy scheme masternode registrations (Kittywhiskers Van Gogh)
abf96b5 evo: prohibit extended address versioning on unsupported transactions (Kittywhiskers Van Gogh)
de7f928 evo: prohibit upgrading from LegacyBLS to ExtAddr support directly (Kittywhiskers Van Gogh)
e430e6e evo: prohibit any ProTx with legacy BLS version after basic BLS upgrade (Kittywhiskers Van Gogh)
dc687a9 rpc: constrain ProTx version for ProUpRevTx based on legacy status (Kittywhiskers Van Gogh)
23c5152 trivial: use consistent variable name, use brackets (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Depends on #6665

  * Depends on #6723

  * Please refer to comments from [dash#6665](#6665) for prior discussion on the contents of the pull request ([comment](#6665 (comment)), [comment](#6665 (comment)), [comment](#6665 (comment)))

  * Complementing the deprecation in [dash#6723](#6723), after `DEPLOYMENT_V23` is activated
    * Registration of **new** masternodes  (i.e. `ProRegTx`) with the legacy scheme (`LegacyBLS`) will no longer be allowed. Existing masternodes are not affected and can continue to operate and participate.

    * Masternodes that are already using the basic scheme (`BasicBLS`) or higher may no longer **downgrade** to the legacy scheme.

  * Additional guardrails have been introduced to complement [dash#6665](#6665), which reserves a new version for extended addresses (`ExtAddr`), affecting `ProRegTx` and `ProUpServTx`, applicable after `DEPLOYMENT_V23` is activated
    * Masternodes **must** migrate to the basic scheme (`BasicBLS`) _before_ attempting to utilize extended addresses (`ExtAddr`), legacy scheme nodes may not attempt a direct upgrade.

    * Special transactions other than `ProRegTx` or `ProUpServTx` may not bear the version reserved for extended addresses (`ExtAddr`). Note that `IsVersionChangeValid()` does not extend to `ProRegTx` as it _creates_ a new entry and therefore doesn't have a masternode state version to compare against (i.e. there's no version to _change_), so the restriction in `IsVersionChangeValid()` only _de facto_ applies to `ProUpServTx`.

  * Future version updates must be conscious of updates to the masternode state ([source](https://github.com/dashpay/dash/blob/d9f52acecb94d57be91b5e17d478d5c909df3633/src/evo/deterministicmns.cpp#L887-L888)), example code for what changes may be required are available [here](#6665 (comment)).

  ## Breaking Changes

  * `protx revoke` will no longer default to using the highest possible version of `ProUpRevTx` and will now _clamp_ the version to `LegacyBLS` if the masternode uses the legacy scheme.

  ## Checklist

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)**
  - [x] 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)_

ACKs for top commit:
  PastaPastaPasta:
    utACK af10784
  UdjinM6:
    utACK af10784

Tree-SHA512: fe788c33397f9e351377777946d5c0498569f68f22d308cce03f903fb3e149bd594ce2caafc50fd62611029563e2841bc42151579b21ba42fdf87cbf7762f716

Author: PastaPastaPasta

doc/release-notes-6729.md

@@ -0,0 +1,12 @@
+Notable Changes
+---------------
+
+* Dash Core will no longer permit the registration of new legacy scheme masternodes after the deployment of the v23
+  fork. Existing basic scheme masternodes will also be prohibited from downgrading to the legacy scheme after the
+  deployment is active.
+
+Updated RPCs
+----------------
+
+* `protx revoke` will now use the legacy scheme version for legacy masternodes instead of the defaulting to the
+   highest `ProUpRevTx` version.

src/evo/deterministicmns.cpp

@@ -957,6 +957,41 @@ static std::optional<ProTx> GetValidatedPayload(const CTransaction& tx, gsl::not
     return opt_ptx;
 }
 
+/**
+ * Validates potential changes to masternode state version by ProTx transaction version
+ * @param[in]  pindexPrev    Previous block index to validate DEPLOYMENT_V23 activation
+ * @param[in]  tx_type       Special transaction type
+ * @param[in]  state_version Current masternode state version
+ * @param[in]  tx_version    Proposed transaction version
+ * @param[out] state         This may be set to an Error state if any error occurred processing them
+ * @returns                  true if version change is valid or DEPLOYMENT_V23 is not active
+ */
+bool IsVersionChangeValid(gsl::not_null<const CBlockIndex*> pindexPrev, const uint16_t tx_type,
+                          const uint16_t state_version, const uint16_t tx_version, TxValidationState& state)
+{
+    if (!DeploymentActiveAfter(pindexPrev, Params().GetConsensus(), Consensus::DEPLOYMENT_V23)) {
+        // New restrictions only apply after v23 deployment
+        return true;
+    }
+
+    if (state_version >= ProTxVersion::BasicBLS && tx_version == ProTxVersion::LegacyBLS) {
+        // Don't allow legacy scheme versioned transactions after upgrading to basic scheme
+        return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-protx-version-downgrade");
+    }
+
+    if (state_version == ProTxVersion::LegacyBLS && tx_version > ProTxVersion::BasicBLS) {
+        // Nodes using the legacy scheme must first upgrade to the basic scheme before upgrading further
+        return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-protx-version-upgrade");
+    }
+
+    if (tx_type != TRANSACTION_PROVIDER_UPDATE_SERVICE && tx_version == ProTxVersion::ExtAddr) {
+        // Only new entries (ProRegTx) and service updates (ProUpServTx) can use ExtAddr versioning
+        return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-protx-version-tx-type");
+    }
+
+    return true;
+}
+
 bool CheckProRegTx(CDeterministicMNManager& dmnman, const CTransaction& tx, gsl::not_null<const CBlockIndex*> pindexPrev, TxValidationState& state, const CCoinsViewCache& view, bool check_sigs)
 {
     const auto opt_ptx = GetValidatedPayload<CProRegTx>(tx, pindexPrev, state);
@@ -965,6 +1000,13 @@ bool CheckProRegTx(CDeterministicMNManager& dmnman, const CTransaction& tx, gsl:
         return false;
     }
 
+    const bool is_v23_active{DeploymentActiveAfter(pindexPrev, Params().GetConsensus(), Consensus::DEPLOYMENT_V23)};
+
+    // No longer allow legacy scheme masternode registration
+    if (is_v23_active && opt_ptx->nVersion < ProTxVersion::BasicBLS) {
+        return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-protx-version-disallowed");
+    }
+
     // It's allowed to set addr to 0, which will put the MN into PoSe-banned state and require a ProUpServTx to be issues later
     // If any of both is set, it must be valid however
     if (!opt_ptx->netInfo->IsEmpty() && !CheckService(*opt_ptx, state)) {
@@ -1099,11 +1141,16 @@ bool CheckProUpServTx(CDeterministicMNManager& dmnman, const CTransaction& tx, g
     }
 
     auto mnList = dmnman.GetListForBlock(pindexPrev);
-    auto mn = mnList.GetMN(opt_ptx->proTxHash);
-    if (!mn) {
+    auto dmn = mnList.GetMN(opt_ptx->proTxHash);
+    if (!dmn) {
         return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-protx-hash");
     }
 
+    if (!IsVersionChangeValid(pindexPrev, tx.nType, dmn->pdmnState->nVersion, opt_ptx->nVersion, state)) {
+        // pass the state returned by the function above
+        return false;
+    }
+
     // don't allow updating to addresses already used by other MNs
     for (const NetInfoEntry& entry : opt_ptx->netInfo->GetEntries()) {
         if (const auto& service_opt{entry.GetAddrPort()}; service_opt.has_value()) {
@@ -1124,7 +1171,7 @@ bool CheckProUpServTx(CDeterministicMNManager& dmnman, const CTransaction& tx, g
     }
 
     if (opt_ptx->scriptOperatorPayout != CScript()) {
-        if (mn->nOperatorReward == 0) {
+        if (dmn->nOperatorReward == 0) {
             // don't allow setting operator reward payee in case no operatorReward was set
             return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-operator-payee");
         }
@@ -1138,7 +1185,7 @@ bool CheckProUpServTx(CDeterministicMNManager& dmnman, const CTransaction& tx, g
         // pass the state returned by the function above
         return false;
     }
-    if (check_sigs && !CheckHashSig(*opt_ptx, mn->pdmnState->pubKeyOperator.Get(), state)) {
+    if (check_sigs && !CheckHashSig(*opt_ptx, dmn->pdmnState->pubKeyOperator.Get(), state)) {
         // pass the state returned by the function above
         return false;
     }
@@ -1166,6 +1213,11 @@ bool CheckProUpRegTx(CDeterministicMNManager& dmnman, const CTransaction& tx, gs
         return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-protx-hash");
     }
 
+    if (!IsVersionChangeValid(pindexPrev, tx.nType, dmn->pdmnState->nVersion, opt_ptx->nVersion, state)) {
+        // pass the state returned by the function above
+        return false;
+    }
+
     // don't allow reuse of payee key for other keys (don't allow people to put the payee key onto an online server)
     if (payoutDest == CTxDestination(PKHash(dmn->pdmnState->keyIDOwner)) || payoutDest == CTxDestination(PKHash(opt_ptx->keyIDVoting))) {
         return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-payee-reuse");
@@ -1221,8 +1273,14 @@ bool CheckProUpRevTx(CDeterministicMNManager& dmnman, const CTransaction& tx, gs
 
     auto mnList = dmnman.GetListForBlock(pindexPrev);
     auto dmn = mnList.GetMN(opt_ptx->proTxHash);
-    if (!dmn)
+    if (!dmn) {
         return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-protx-hash");
+    }
+
+    if (!IsVersionChangeValid(pindexPrev, tx.nType, dmn->pdmnState->nVersion, opt_ptx->nVersion, state)) {
+        // pass the state returned by the function above
+        return false;
+    }
 
     if (!CheckInputsHash(tx, *opt_ptx, state)) {
         // pass the state returned by the function above

src/rpc/evo.cpp

@@ -1269,14 +1269,16 @@ static RPCHelpMan protx_revoke()
     EnsureWalletIsUnlocked(*pwallet);
 
     CProUpRevTx ptx;
-    ptx.nVersion = ProTxVersion::GetMaxFromDeployment<CProUpRevTx>(WITH_LOCK(::cs_main, return chainman.ActiveChain().Tip()));
-
     ptx.proTxHash = ParseHashV(request.params[0], "proTxHash");
+
     auto dmn = dmnman.GetListAtChainTip().GetMN(ptx.proTxHash);
     if (!dmn) {
         throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("masternode %s not found", ptx.proTxHash.ToString()));
     }
 
+    ptx.nVersion = ProTxVersion::GetMaxFromDeployment<CProUpRevTx>(WITH_LOCK(::cs_main, return chainman.ActiveChain().Tip()),
+                                                                   /*is_basic_override=*/dmn->pdmnState->nVersion >= ProTxVersion::BasicBLS);
+
     CBLSSecretKey keyOperator = ParseBLSSecretKey(request.params[1].get_str(), "operatorKey");
 
     if (!request.params[2].isNull()) {