fix!: use the same version along all protx special transactions

src/evo/deterministicmns.h

@@ -7,15 +7,12 @@
 
 #include <evo/dmn_types.h>
 #include <evo/dmnstate.h>
-#include <evo/providertx.h>
 #include <evo/types.h>
 
-#include <arith_uint256.h>
 #include <clientversion.h>
 #include <consensus/params.h>
 #include <crypto/common.h>
 #include <saltedhasher.h>
-#include <scheduler.h>
 #include <sync.h>
 
 #include <gsl/pointers.h>

src/evo/dmnstate.h

@@ -8,7 +8,6 @@
 #include <bls/bls.h>
 #include <crypto/sha256.h>
 #include <evo/providertx.h>
-#include <netaddress.h>
 #include <pubkey.h>
 #include <script/script.h>
 #include <util/helpers.h>

src/evo/netinfo.cpp

@@ -5,7 +5,7 @@
 #include <evo/netinfo.h>
 
 #include <chainparams.h>
-#include <evo/providertx.h>
+#include <evo/types.h>
 #include <netbase.h>
 #include <span.h>
 #include <util/check.h>

src/evo/providertx.cpp

@@ -13,32 +13,6 @@
 #include <hash.h>
 #include <script/standard.h>
 #include <tinyformat.h>
-#include <validation.h>
-
-namespace ProTxVersion {
-template <typename T>
-[[nodiscard]] uint16_t GetMaxFromDeployment(gsl::not_null<const CBlockIndex*> pindexPrev,
-                                            const ChainstateManager& chainman, std::optional<bool> is_basic_override)
-{
-    constexpr bool is_extaddr_eligible{std::is_same_v<std::decay_t<T>, CProRegTx> || std::is_same_v<std::decay_t<T>, CProUpServTx>};
-    return ProTxVersion::GetMax(
-        is_basic_override ? *is_basic_override
-                          : DeploymentActiveAfter(pindexPrev, chainman.GetConsensus(), Consensus::DEPLOYMENT_V19),
-        is_extaddr_eligible ? DeploymentActiveAfter(pindexPrev, chainman, Consensus::DEPLOYMENT_V24) : false);
-}
-template uint16_t GetMaxFromDeployment<CProRegTx>(gsl::not_null<const CBlockIndex*> pindexPrev,
-                                                  const ChainstateManager& chainman,
-                                                  std::optional<bool> is_basic_override);
-template uint16_t GetMaxFromDeployment<CProUpServTx>(gsl::not_null<const CBlockIndex*> pindexPrev,
-                                                     const ChainstateManager& chainman,
-                                                     std::optional<bool> is_basic_override);
-template uint16_t GetMaxFromDeployment<CProUpRegTx>(gsl::not_null<const CBlockIndex*> pindexPrev,
-                                                    const ChainstateManager& chainman,
-                                                    std::optional<bool> is_basic_override);
-template uint16_t GetMaxFromDeployment<CProUpRevTx>(gsl::not_null<const CBlockIndex*> pindexPrev,
-                                                    const ChainstateManager& chainman,
-                                                    std::optional<bool> is_basic_override);
-} // namespace ProTxVersion
 
 template <typename ProTx>
 bool IsNetInfoTriviallyValid(const ProTx& proTx, TxValidationState& state)
@@ -64,10 +38,9 @@ bool IsNetInfoTriviallyValid(const ProTx& proTx, TxValidationState& state)
     return true;
 }
 
-bool CProRegTx::IsTriviallyValid(gsl::not_null<const CBlockIndex*> pindexPrev, const ChainstateManager& chainman,
-                                 TxValidationState& state) const
+bool CProRegTx::IsTriviallyValid(TxValidationState& state) const
 {
-    if (nVersion == 0 || nVersion > ProTxVersion::GetMaxFromDeployment<decltype(*this)>(pindexPrev, chainman)) {
+    if (nVersion == 0 || nVersion > ProTxVersion::ExtAddr) {
         return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-protx-version");
     }
     if (nVersion < ProTxVersion::BasicBLS && nType == MnType::Evo) {
@@ -162,10 +135,9 @@ std::string CProRegTx::ToString() const
                           : strprintf(", platformP2PPort=%d, platformHTTPPort=%d", platformP2PPort, platformHTTPPort)));
 }
 
-bool CProUpServTx::IsTriviallyValid(gsl::not_null<const CBlockIndex*> pindexPrev, const ChainstateManager& chainman,
-                                    TxValidationState& state) const
+bool CProUpServTx::IsTriviallyValid(TxValidationState& state) const
 {
-    if (nVersion == 0 || nVersion > ProTxVersion::GetMaxFromDeployment<decltype(*this)>(pindexPrev, chainman)) {
+    if (nVersion == 0 || nVersion > ProTxVersion::ExtAddr) {
         return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-protx-version");
     }
     if (nVersion < ProTxVersion::BasicBLS && nType == MnType::Evo) {
@@ -207,10 +179,9 @@ std::string CProUpServTx::ToString() const
                           : strprintf(", platformP2PPort=%d, platformHTTPPort=%d", platformP2PPort, platformHTTPPort)));
 }
 
-bool CProUpRegTx::IsTriviallyValid(gsl::not_null<const CBlockIndex*> pindexPrev, const ChainstateManager& chainman,
-                                   TxValidationState& state) const
+bool CProUpRegTx::IsTriviallyValid(TxValidationState& state) const
 {
-    if (nVersion == 0 || nVersion > ProTxVersion::GetMaxFromDeployment<decltype(*this)>(pindexPrev, chainman)) {
+    if (nVersion == 0 || nVersion > ProTxVersion::ExtAddr) {
         return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-protx-version");
     }
     if (nMode != 0) {
@@ -241,10 +212,9 @@ std::string CProUpRegTx::ToString() const
         nVersion, proTxHash.ToString(), pubKeyOperator.ToString(), EncodeDestination(PKHash(keyIDVoting)), payee);
 }
 
-bool CProUpRevTx::IsTriviallyValid(gsl::not_null<const CBlockIndex*> pindexPrev, const ChainstateManager& chainman,
-                                   TxValidationState& state) const
+bool CProUpRevTx::IsTriviallyValid(TxValidationState& state) const
 {
-    if (nVersion == 0 || nVersion > ProTxVersion::GetMaxFromDeployment<decltype(*this)>(pindexPrev, chainman)) {
+    if (nVersion == 0 || nVersion > ProTxVersion::ExtAddr) {
         return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-protx-version");
     }
 

src/evo/providertx.h

@@ -9,6 +9,7 @@
 #include <evo/dmn_types.h>
 #include <evo/netinfo.h>
 #include <evo/specialtx.h>
+#include <evo/types.h>
 #include <primitives/transaction.h>
 #include <util/std23.h>
 
@@ -17,46 +18,12 @@
 #include <netaddress.h>
 #include <pubkey.h>
 
-#include <univalue.h>
 #include <gsl/pointers.h>
+#include <univalue.h>
 
-class CBlockIndex;
-class ChainstateManager;
 class TxValidationState;
 struct RPCResult;
 
-namespace ProTxVersion {
-enum : uint16_t {
-    LegacyBLS = 1,
-    BasicBLS  = 2,
-    ExtAddr   = 3,
-};
-
-/** Get highest permissible ProTx version based on flags set. */
-[[nodiscard]] constexpr uint16_t GetMax(const bool is_basic_scheme_active, const bool is_extended_addr)
-{
-    if (is_basic_scheme_active) {
-        if (is_extended_addr) {
-            // Requires *both* forks to be active to use extended addresses. is_basic_scheme_active could
-            // be set to false due to RPC specialization, so we must evaluate is_extended_addr *last* to
-            // avoid accidentally upgrading a legacy BLS node to basic BLS due to v24 activation.
-            return ProTxVersion::ExtAddr;
-        }
-        return ProTxVersion::BasicBLS;
-    }
-    return ProTxVersion::LegacyBLS;
-}
-
-/** Get highest permissible ProTx version based on deployment status
- *  Note: The override is needed because some RPCs need to use deployment status information for everything *except*
- *        the BLS version upgrade since they are specializations for a specific BLS version. This is a one-off.
- *  TODO: Resolve this oddity. Consider deprecating legacy BLS-only RPCs so we can remove them eventually.
- */
-template <typename T>
-[[nodiscard]] uint16_t GetMaxFromDeployment(gsl::not_null<const CBlockIndex*> pindexPrev, const ChainstateManager& chainman,
-                                            std::optional<bool> is_basic_override = std::nullopt);
-} // namespace ProTxVersion
-
 class CProRegTx
 {
 public:
@@ -125,8 +92,11 @@ class CProRegTx
     [[nodiscard]] static RPCResult GetJsonHelp(const std::string& key, bool optional);
     [[nodiscard]] UniValue ToJson() const;
 
-    bool IsTriviallyValid(gsl::not_null<const CBlockIndex*> pindexPrev, const ChainstateManager& chainman,
-                          TxValidationState& state) const;
+    /**
+     * Note: this check validates only some trivial consensus rules
+     * Use `CheckProRegTx` or GetValidatedPayload<T> helper for full validation
+     */
+    bool IsTriviallyValid(TxValidationState& state) const;
 };
 
 class CProUpServTx
@@ -187,8 +157,11 @@ class CProUpServTx
     [[nodiscard]] static RPCResult GetJsonHelp(const std::string& key, bool optional);
     [[nodiscard]] UniValue ToJson() const;
 
-    bool IsTriviallyValid(gsl::not_null<const CBlockIndex*> pindexPrev, const ChainstateManager& chainman,
-                          TxValidationState& state) const;
+    /**
+     * Note: this check validates only some trivial consensus rules
+     * Use `CheckProUpServTx` or GetValidatedPayload<T> helper for full validation
+     */
+    bool IsTriviallyValid(TxValidationState& state) const;
 };
 
 class CProUpRegTx
@@ -235,8 +208,11 @@ class CProUpRegTx
     [[nodiscard]] static RPCResult GetJsonHelp(const std::string& key, bool optional);
     [[nodiscard]] UniValue ToJson() const;
 
-    bool IsTriviallyValid(gsl::not_null<const CBlockIndex*> pindexPrev, const ChainstateManager& chainman,
-                          TxValidationState& state) const;
+    /**
+     * Note: this check validates only some trivial consensus rules
+     * Use `CheckProUpRegTx` or GetValidatedPayload<T> helper for full validation
+     */
+    bool IsTriviallyValid(TxValidationState& state) const;
 };
 
 class CProUpRevTx
@@ -286,8 +262,11 @@ class CProUpRevTx
     [[nodiscard]] static RPCResult GetJsonHelp(const std::string& key, bool optional);
     [[nodiscard]] UniValue ToJson() const;
 
-    bool IsTriviallyValid(gsl::not_null<const CBlockIndex*> pindexPrev, const ChainstateManager& chainman,
-                          TxValidationState& state) const;
+    /**
+     * Note: this check validates only some trivial consensus rules
+     * Use `CheckProUpRevTx` or GetValidatedPayload<T> helper for full validation
+     */
+    bool IsTriviallyValid(TxValidationState& state) const;
 };
 
 template <typename ProTx>

src/evo/simplifiedmns.h

@@ -6,14 +6,14 @@
 #define BITCOIN_EVO_SIMPLIFIEDMNS_H
 
 #include <bls/bls.h>
+#include <consensus/validation.h>
 #include <evo/dmn_types.h>
 #include <evo/netinfo.h>
-#include <evo/providertx.h>
-#include <util/helpers.h>
-
+#include <evo/types.h>
 #include <merkleblock.h>
 #include <netaddress.h>
 #include <pubkey.h>
+#include <util/helpers.h>
 
 #include <gsl/pointers.h>
 

src/evo/smldiff.h

@@ -8,9 +8,7 @@
 #include <bls/bls.h>
 #include <evo/dmn_types.h>
 #include <evo/netinfo.h>
-#include <evo/providertx.h>
 #include <evo/simplifiedmns.h>
-
 #include <merkleblock.h>
 #include <netaddress.h>
 #include <pubkey.h>

src/evo/specialtxman.cpp

@@ -859,8 +859,8 @@ static bool CheckHashSig(const ProTx& proTx, const CBLSPublicKey& pubKey, TxVali
 }
 
 template <typename ProTx>
-static std::optional<ProTx> GetValidatedPayload(const CTransaction& tx, gsl::not_null<const CBlockIndex*> pindexPrev,
-                                                const ChainstateManager& chainman, TxValidationState& state)
+std::optional<ProTx> GetValidatedPayload(const CTransaction& tx, gsl::not_null<const CBlockIndex*> pindexPrev,
+                                         const ChainstateManager& chainman, TxValidationState& state)
 {
     if (tx.nType != ProTx::SPECIALTX_TYPE) {
         state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-protx-type");
@@ -872,7 +872,11 @@ static std::optional<ProTx> GetValidatedPayload(const CTransaction& tx, gsl::not
         state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-protx-payload");
         return std::nullopt;
     }
-    if (!opt_ptx->IsTriviallyValid(pindexPrev, chainman, state)) {
+    if (opt_ptx->nVersion > DeploymentToProtxVersion(pindexPrev, chainman)) {
+        state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-protx-version");
+        return std::nullopt;
+    }
+    if (!opt_ptx->IsTriviallyValid(state)) {
         // pass the state returned by the function above
         return std::nullopt;
     }
@@ -882,15 +886,13 @@ static std::optional<ProTx> GetValidatedPayload(const CTransaction& tx, gsl::not
 /**
  * Validates potential changes to masternode state version by ProTx transaction version
  * @param[in]  pindexPrev    Previous block index to validate DEPLOYMENT_V24 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_V24 is not active
  */
-static bool IsVersionChangeValid(gsl::not_null<const CBlockIndex*> pindexPrev, const uint16_t tx_type,
-                                 const uint16_t state_version, const uint16_t tx_version,
-                                 const ChainstateManager& chainman, TxValidationState& state)
+static bool IsVersionChangeValid(gsl::not_null<const CBlockIndex*> pindexPrev, const uint16_t state_version,
+                                 const uint16_t tx_version, const ChainstateManager& chainman, TxValidationState& state)
 {
     if (!DeploymentActiveAfter(pindexPrev, chainman, Consensus::DEPLOYMENT_V24)) {
         // New restrictions only apply after v24 deployment
@@ -907,11 +909,6 @@ static bool IsVersionChangeValid(gsl::not_null<const CBlockIndex*> pindexPrev, c
         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;
 }
 
@@ -1077,7 +1074,7 @@ bool CheckProUpServTx(const CTransaction& tx, gsl::not_null<const CBlockIndex*>
         return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-protx-hash");
     }
 
-    if (!IsVersionChangeValid(pindexPrev, tx.nType, dmn->pdmnState->nVersion, opt_ptx->nVersion, chainman, state)) {
+    if (!IsVersionChangeValid(pindexPrev, dmn->pdmnState->nVersion, opt_ptx->nVersion, chainman, state)) {
         // pass the state returned by the function above
         return false;
     }
@@ -1152,7 +1149,7 @@ bool CheckProUpRegTx(const CTransaction& tx, gsl::not_null<const CBlockIndex*> p
         return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-protx-hash");
     }
 
-    if (!IsVersionChangeValid(pindexPrev, tx.nType, dmn->pdmnState->nVersion, opt_ptx->nVersion, chainman, state)) {
+    if (!IsVersionChangeValid(pindexPrev, dmn->pdmnState->nVersion, opt_ptx->nVersion, chainman, state)) {
         // pass the state returned by the function above
         return false;
     }
@@ -1219,7 +1216,7 @@ bool CheckProUpRevTx(const CTransaction& tx, gsl::not_null<const CBlockIndex*> p
         return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-protx-hash");
     }
 
-    if (!IsVersionChangeValid(pindexPrev, tx.nType, dmn->pdmnState->nVersion, opt_ptx->nVersion, chainman, state)) {
+    if (!IsVersionChangeValid(pindexPrev, dmn->pdmnState->nVersion, opt_ptx->nVersion, chainman, state)) {
         // pass the state returned by the function above
         return false;
     }

src/evo/specialtxman.h

@@ -93,6 +93,15 @@ class CSpecialTxProcessor
 };
 
 
+/**
+ * This helper does some trivial validations that doesn't depends on collateral and
+ * masternode list. Use CheckPro*Tx for the full validation of transaction,
+ * including bls signatures, list of masternodes and collateral
+ */
+template <typename ProTx>
+std::optional<ProTx> GetValidatedPayload(const CTransaction& tx, gsl::not_null<const CBlockIndex*> pindexPrev,
+                                         const ChainstateManager& chainman, TxValidationState& state);
+
 bool CheckProRegTx(const CTransaction& tx, gsl::not_null<const CBlockIndex*> pindexPrev,
                    CDeterministicMNManager& dmnman, const CCoinsViewCache& view, const ChainstateManager& chainman,
                    TxValidationState& state, bool check_sigs);

src/evo/types.h

@@ -11,4 +11,28 @@ class CDeterministicMN;
 
 using CDeterministicMNCPtr = std::shared_ptr<const CDeterministicMN>;
 
+namespace ProTxVersion {
+enum : uint16_t {
+    LegacyBLS = 1,
+    BasicBLS = 2,
+    ExtAddr = 3,
+};
+
+/** Get highest permissible ProTx version based on flags set. */
+[[nodiscard]] constexpr uint16_t GetMax(const bool is_basic_scheme_active, const bool is_extended_addr)
+{
+    if (is_basic_scheme_active) {
+        if (is_extended_addr) {
+            // Requires *both* forks to be active to use extended addresses. is_basic_scheme_active could
+            // be set to false due to RPC specialization, so we must evaluate is_extended_addr *last* to
+            // avoid accidentally upgrading a legacy BLS node to basic BLS due to v24 activation.
+            return ProTxVersion::ExtAddr;
+        }
+        return ProTxVersion::BasicBLS;
+    }
+    return ProTxVersion::LegacyBLS;
+}
+
+} // namespace ProTxVersion
+
 #endif // BITCOIN_EVO_TYPES_H

src/llmq/commitment.cpp

@@ -5,6 +5,7 @@
 #include <llmq/commitment.h>
 
 #include <evo/deterministicmns.h>
+#include <evo/dmnstate.h>
 #include <evo/specialtx.h>
 #include <llmq/options.h>
 #include <llmq/utils.h>