Merge #2915: [Test] [DMNs] Split special transaction validation

607ba81 test: add trivial validation invalid tests (ale)
2c87e78 test: add trivial validation tests (ale)
325a204 Verify match between type and tx version in GetValidatedTxPayload (ale)
bf373bf Refactor special_tx_validation (ale)
78c7d26 Add SPECIAL_TX_TYPE member to payload classes (ale)
357f572 Add IsTriviallyValid for special payloads (ale)

Pull request description:

  First 4  commits are a (almost) move only  refactor:
  The self contained part of special transaction validation (i.e the part based only on specialtx info and not on full chain and masternode list) has been moved to functions `IsTriviallyValid`.

  Last 2 commits improve unit test coverage:
  I have added two files with a list of special txs which are either valid or invalid.  Test consist in correctly deserializing the txs and verifying that valid/invalid ones are indeed considered valid/invalid.

ACKs for top commit: 607ba81
  Liquid369:
    tACK 607ba81
  Duddino:
    utACK 607ba81
  Fuzzbawls:
    ACK 607ba81

Tree-SHA512: 59a810d49c17943e2dfc10d95e7886922b42cfa2c91b6f8d77d64aa2a386fdebe137c11eb8f93188d79fcd0bae42aceeb2fc3d63af2a3abc1b2ac1cf103d31ea

src/Makefile.test.include

@@ -52,6 +52,8 @@ JSON_TEST_FILES = \
   test/data/tx_invalid.json \
   test/data/tx_valid.json \
   test/data/sighash.json \
+  test/data/specialtx_valid.json \
+  test/data/specialtx_invalid.json \
   test/data/merkle_roots_sapling.json \
   test/data/merkle_serialization_sapling.json \
   test/data/merkle_witness_serialization_sapling.json \

src/evo/providertx.cpp

@@ -56,6 +56,49 @@ void ProRegPL::ToJson(UniValue& obj) const
     obj.pushKV("inputsHash", inputsHash.ToString());
 }
 
+bool ProRegPL::IsTriviallyValid(CValidationState& state) const
+{
+    if (nVersion == 0 || nVersion > ProRegPL::CURRENT_VERSION) {
+        return state.DoS(100, false, REJECT_INVALID, "bad-protx-version");
+    }
+    if (nType != 0) {
+        return state.DoS(100, false, REJECT_INVALID, "bad-protx-type");
+    }
+    if (nMode != 0) {
+        return state.DoS(100, false, REJECT_INVALID, "bad-protx-mode");
+    }
+
+    if (keyIDOwner.IsNull() || keyIDVoting.IsNull()) {
+        return state.DoS(10, false, REJECT_INVALID, "bad-protx-key-null");
+    }
+    if (!pubKeyOperator.IsValid()) {
+        return state.DoS(10, false, REJECT_INVALID, "bad-protx-operator-key-invalid");
+    }
+    // we may support other kinds of scripts later, but restrict it for now
+    if (!scriptPayout.IsPayToPublicKeyHash()) {
+        return state.DoS(10, false, REJECT_INVALID, "bad-protx-payee");
+    }
+    if (!scriptOperatorPayout.empty() && !scriptOperatorPayout.IsPayToPublicKeyHash()) {
+        return state.DoS(10, false, REJECT_INVALID, "bad-protx-operator-payee");
+    }
+
+    CTxDestination payoutDest;
+    if (!ExtractDestination(scriptPayout, payoutDest)) {
+        // should not happen as we checked script types before
+        return state.DoS(10, false, REJECT_INVALID, "bad-protx-payee-dest");
+    }
+    // don't allow reuse of payout key for other keys (don't allow people to put the payee key onto an online server)
+    if (payoutDest == CTxDestination(keyIDOwner) ||
+        payoutDest == CTxDestination(keyIDVoting)) {
+        return state.DoS(10, false, REJECT_INVALID, "bad-protx-payee-reuse");
+    }
+
+    if (nOperatorReward > 10000) {
+        return state.DoS(10, false, REJECT_INVALID, "bad-protx-operator-reward");
+    }
+    return true;
+}
+
 std::string ProUpServPL::ToString() const
 {
     CTxDestination dest;
@@ -79,6 +122,14 @@ void ProUpServPL::ToJson(UniValue& obj) const
     obj.pushKV("inputsHash", inputsHash.ToString());
 }
 
+bool ProUpServPL::IsTriviallyValid(CValidationState& state) const
+{
+    if (nVersion == 0 || nVersion > ProUpServPL::CURRENT_VERSION) {
+        return state.DoS(100, false, REJECT_INVALID, "bad-protx-version");
+    }
+    return true;
+}
+
 std::string ProUpRegPL::ToString() const
 {
     CTxDestination dest;
@@ -103,6 +154,29 @@ void ProUpRegPL::ToJson(UniValue& obj) const
     obj.pushKV("inputsHash", inputsHash.ToString());
 }
 
+bool ProUpRegPL::IsTriviallyValid(CValidationState& state) const
+{
+    if (nVersion == 0 || nVersion > ProUpRegPL::CURRENT_VERSION) {
+        return state.DoS(100, false, REJECT_INVALID, "bad-protx-version");
+    }
+    if (nMode != 0) {
+        return state.DoS(100, false, REJECT_INVALID, "bad-protx-mode");
+    }
+
+    if (!pubKeyOperator.IsValid()) {
+        return state.DoS(10, false, REJECT_INVALID, "bad-protx-operator-key-invalid");
+    }
+    if (keyIDVoting.IsNull()) {
+        return state.DoS(10, false, REJECT_INVALID, "bad-protx-voting-key-null");
+    }
+    // !TODO: enable other scripts
+    if (!scriptPayout.IsPayToPublicKeyHash()) {
+        return state.DoS(10, false, REJECT_INVALID, "bad-protx-payee");
+    }
+
+    return true;
+}
+
 std::string ProUpRevPL::ToString() const
 {
     return strprintf("ProUpRevPL(nVersion=%d, proTxHash=%s, nReason=%d)",
@@ -119,6 +193,20 @@ void ProUpRevPL::ToJson(UniValue& obj) const
     obj.pushKV("inputsHash", inputsHash.ToString());
 }
 
+bool ProUpRevPL::IsTriviallyValid(CValidationState& state) const
+{
+    if (nVersion == 0 || nVersion > ProUpRevPL::CURRENT_VERSION) {
+        return state.DoS(100, false, REJECT_INVALID, "bad-protx-version");
+    }
+
+    // pl.nReason < ProUpRevPL::REASON_NOT_SPECIFIED is always `false` since
+    // pl.nReason is unsigned and ProUpRevPL::REASON_NOT_SPECIFIED == 0
+    if (nReason > ProUpRevPL::REASON_LAST) {
+        return state.DoS(100, false, REJECT_INVALID, "bad-protx-reason");
+    }
+    return true;
+}
+
 bool GetProRegCollateral(const CTransactionRef& tx, COutPoint& outRet)
 {
     if (tx == nullptr) {

src/evo/providertx.h

@@ -6,17 +6,21 @@
 #define PIVX_PROVIDERTX_H
 
 #include "bls/bls_wrapper.h"
-#include "primitives/transaction.h"
 #include "netaddress.h"
+#include "primitives/transaction.h"
+#include <consensus/validation.h>
 
 #include <univalue.h>
 
+class CValidationState;
+
 // Provider-Register tx payload
 
 class ProRegPL
 {
 public:
     static const uint16_t CURRENT_VERSION = 1;
+    static constexpr int16_t SPECIALTX_TYPE = CTransaction::TxType::PROREG;
 
 public:
     uint16_t nVersion{CURRENT_VERSION};                         // message version
@@ -60,6 +64,7 @@ class ProRegPL
 
     std::string ToString() const;
     void ToJson(UniValue& obj) const;
+    bool IsTriviallyValid(CValidationState& state) const;
 };
 
 // Provider-Update-Service tx payload
@@ -68,6 +73,7 @@ class ProUpServPL
 {
 public:
     static const uint16_t CURRENT_VERSION = 1;
+    static constexpr int16_t SPECIALTX_TYPE = CTransaction::TxType::PROUPSERV;
 
 public:
     uint16_t nVersion{CURRENT_VERSION}; // message version
@@ -89,13 +95,15 @@ class ProUpServPL
 public:
     std::string ToString() const;
     void ToJson(UniValue& obj) const;
+    bool IsTriviallyValid(CValidationState& state) const;
 };
 
 // Provider-Update-Registrar tx payload
 class ProUpRegPL
 {
 public:
     static const uint16_t CURRENT_VERSION = 1;
+    static constexpr int16_t SPECIALTX_TYPE = CTransaction::TxType::PROUPREG;
 
 public:
     uint16_t nVersion{CURRENT_VERSION}; // message version
@@ -119,13 +127,15 @@ class ProUpRegPL
 public:
     std::string ToString() const;
     void ToJson(UniValue& obj) const;
+    bool IsTriviallyValid(CValidationState& state) const;
 };
 
 // Provider-Update-Revoke tx payload
 class ProUpRevPL
 {
 public:
     static const uint16_t CURRENT_VERSION = 1;
+    static constexpr int16_t SPECIALTX_TYPE = CTransaction::TxType::PROUPREV;
 
     // these are just informational and do not have any effect on the revocation
     enum RevocationReason {
@@ -155,6 +165,7 @@ class ProUpRevPL
 public:
     std::string ToString() const;
     void ToJson(UniValue& obj) const;
+    bool IsTriviallyValid(CValidationState& state) const;
 };
 
 

src/evo/specialtx_validation.cpp

@@ -108,46 +108,11 @@ static bool CheckCollateralOut(const CTxOut& out, const ProRegPL& pl, CValidatio
 // Provider Register Payload
 static bool CheckProRegTx(const CTransaction& tx, const CBlockIndex* pindexPrev, const CCoinsViewCache* view, CValidationState& state)
 {
-    assert(tx.nType == CTransaction::TxType::PROREG);
 
     ProRegPL pl;
-    if (!GetTxPayload(tx, pl)) {
-        return state.DoS(100, false, REJECT_INVALID, "bad-protx-payload");
-    }
-
-    if (pl.nVersion == 0 || pl.nVersion > ProRegPL::CURRENT_VERSION) {
-        return state.DoS(100, false, REJECT_INVALID, "bad-protx-version");
-    }
-    if (pl.nType != 0) {
-        return state.DoS(100, false, REJECT_INVALID, "bad-protx-type");
-    }
-    if (pl.nMode != 0) {
-        return state.DoS(100, false, REJECT_INVALID, "bad-protx-mode");
-    }
-
-    if (pl.keyIDOwner.IsNull() || pl.keyIDVoting.IsNull()) {
-        return state.DoS(10, false, REJECT_INVALID, "bad-protx-key-null");
-    }
-    if (!pl.pubKeyOperator.IsValid()) {
-        return state.DoS(10, false, REJECT_INVALID, "bad-protx-operator-key-invalid");
-    }
-    // we may support other kinds of scripts later, but restrict it for now
-    if (!pl.scriptPayout.IsPayToPublicKeyHash()) {
-        return state.DoS(10, false, REJECT_INVALID, "bad-protx-payee");
-    }
-    if (!pl.scriptOperatorPayout.empty() && !pl.scriptOperatorPayout.IsPayToPublicKeyHash()) {
-        return state.DoS(10, false, REJECT_INVALID, "bad-protx-operator-payee");
-    }
-
-    CTxDestination payoutDest;
-    if (!ExtractDestination(pl.scriptPayout, payoutDest)) {
-        // should not happen as we checked script types before
-        return state.DoS(10, false, REJECT_INVALID, "bad-protx-payee-dest");
-    }
-    // don't allow reuse of payout key for other keys (don't allow people to put the payee key onto an online server)
-    if (payoutDest == CTxDestination(pl.keyIDOwner) ||
-            payoutDest == CTxDestination(pl.keyIDVoting)) {
-        return state.DoS(10, false, REJECT_INVALID, "bad-protx-payee-reuse");
+    if (!GetValidatedTxPayload(tx, pl, state)) {
+        // pass the state returned by the function above
+        return false;
     }
 
     // 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
@@ -157,10 +122,6 @@ static bool CheckProRegTx(const CTransaction& tx, const CBlockIndex* pindexPrev,
         return false;
     }
 
-    if (pl.nOperatorReward > 10000) {
-        return state.DoS(10, false, REJECT_INVALID, "bad-protx-operator-reward");
-    }
-
     if (pl.collateralOutpoint.hash.IsNull()) {
         // collateral included in the proReg tx
         if (pl.collateralOutpoint.n >= tx.vout.size()) {
@@ -229,15 +190,11 @@ static bool CheckProRegTx(const CTransaction& tx, const CBlockIndex* pindexPrev,
 // Provider Update Service Payload
 static bool CheckProUpServTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValidationState& state)
 {
-    assert(tx.nType == CTransaction::TxType::PROUPSERV);
 
     ProUpServPL pl;
-    if (!GetTxPayload(tx, pl)) {
-        return state.DoS(100, false, REJECT_INVALID, "bad-protx-payload");
-    }
-
-    if (pl.nVersion == 0 || pl.nVersion > ProUpServPL::CURRENT_VERSION) {
-        return state.DoS(100, false, REJECT_INVALID, "bad-protx-version");
+    if (!GetValidatedTxPayload(tx, pl, state)) {
+        // pass the state returned by the function above
+        return false;
     }
 
     if (!CheckService(pl.addr, state)) {
@@ -286,29 +243,11 @@ static bool CheckProUpServTx(const CTransaction& tx, const CBlockIndex* pindexPr
 // Provider Update Registrar Payload
 static bool CheckProUpRegTx(const CTransaction& tx, const CBlockIndex* pindexPrev, const CCoinsViewCache* view, CValidationState& state)
 {
-    assert(tx.nType == CTransaction::TxType::PROUPREG);
 
     ProUpRegPL pl;
-    if (!GetTxPayload(tx, pl)) {
-        return state.DoS(100, false, REJECT_INVALID, "bad-protx-payload");
-    }
-
-    if (pl.nVersion == 0 || pl.nVersion > ProUpRegPL::CURRENT_VERSION) {
-        return state.DoS(100, false, REJECT_INVALID, "bad-protx-version");
-    }
-    if (pl.nMode != 0) {
-        return state.DoS(100, false, REJECT_INVALID, "bad-protx-mode");
-    }
-
-    if (!pl.pubKeyOperator.IsValid()) {
-        return state.DoS(10, false, REJECT_INVALID, "bad-protx-operator-key-invalid");
-    }
-    if (pl.keyIDVoting.IsNull()) {
-        return state.DoS(10, false, REJECT_INVALID, "bad-protx-voting-key-null");
-    }
-    // !TODO: enable other scripts
-    if (!pl.scriptPayout.IsPayToPublicKeyHash()) {
-        return state.DoS(10, false, REJECT_INVALID, "bad-protx-payee");
+    if (!GetValidatedTxPayload(tx, pl, state)) {
+        // pass the state returned by the function above
+        return false;
     }
 
     CTxDestination payoutDest;
@@ -382,21 +321,11 @@ static bool CheckProUpRegTx(const CTransaction& tx, const CBlockIndex* pindexPre
 // Provider Update Revoke Payload
 static bool CheckProUpRevTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValidationState& state)
 {
-    assert(tx.nType == CTransaction::TxType::PROUPREV);
 
     ProUpRevPL pl;
-    if (!GetTxPayload(tx, pl)) {
-        return state.DoS(100, false, REJECT_INVALID, "bad-protx-payload");
-    }
-
-    if (pl.nVersion == 0 || pl.nVersion > ProUpRevPL::CURRENT_VERSION) {
-        return state.DoS(100, false, REJECT_INVALID, "bad-protx-version");
-    }
-
-    // pl.nReason < ProUpRevPL::REASON_NOT_SPECIFIED is always `false` since
-    // pl.nReason is unsigned and ProUpRevPL::REASON_NOT_SPECIFIED == 0
-    if (pl.nReason > ProUpRevPL::REASON_LAST) {
-        return state.DoS(100, false, REJECT_INVALID, "bad-protx-reason");
+    if (!GetValidatedTxPayload(tx, pl, state)) {
+        // pass the state returned by the function above
+        return false;
     }
 
     if (!CheckInputsHash(tx, pl, state)) {
@@ -656,3 +585,15 @@ uint256 CalcTxInputsHash(const CTransaction& tx)
     }
     return hw.GetHash();
 }
+
+template <typename T>
+bool GetValidatedTxPayload(const CTransaction& tx, T& obj, CValidationState& state)
+{
+    if (tx.nType != T::SPECIALTX_TYPE) {
+        return state.DoS(100, false, REJECT_INVALID, "bad-protx-type");
+    }
+    if (!GetTxPayload(tx, obj)) {
+        return state.DoS(100, false, REJECT_INVALID, "bad-protx-payload");
+    }
+    return obj.IsTriviallyValid(state);
+}

src/evo/specialtx_validation.h

@@ -38,4 +38,7 @@ bool VerifyLLMQCommitment(const llmq::CFinalCommitment& qfc, const CBlockIndex*
 
 uint256 CalcTxInputsHash(const CTransaction& tx);
 
+template <typename T>
+bool GetValidatedTxPayload(const CTransaction& tx, T& obj, CValidationState& state);
+
 #endif // PIVX_SPECIALTX_H

src/test/CMakeLists.txt

@@ -14,6 +14,8 @@ set(JSON_TEST_FILES
         ${CMAKE_CURRENT_SOURCE_DIR}/data/tx_invalid.json
         ${CMAKE_CURRENT_SOURCE_DIR}/data/tx_valid.json
         ${CMAKE_CURRENT_SOURCE_DIR}/data/sighash.json
+        ${CMAKE_CURRENT_SOURCE_DIR}/data/specialtx_valid.json
+        ${CMAKE_CURRENT_SOURCE_DIR}/data/specialtx_invalid.json
         ${CMAKE_CURRENT_SOURCE_DIR}/data/merkle_roots_sapling.json
         ${CMAKE_CURRENT_SOURCE_DIR}/data/merkle_serialization_sapling.json
         ${CMAKE_CURRENT_SOURCE_DIR}/data/merkle_witness_serialization_sapling.json