Merge #2492: [Consensus] v5.3 network upgrade.

1355b20 Validation: Add new multi-inputs and multi-empty-outputs coinstake rules. (furszy)
599bd42 Test: Add coinstake multi-inputs and multi-empty-outputs test coverage. (furszy)
a195c2e [Test] Introduce PoS testing suite. (furszy)
e98c1e8 Add and guard new max time window for mnb and mnp sigtime. (furszy)
7f8fb12 Introduce v5.3 network upgrade. (furszy)

Pull request description:

  Grouped the consensus changes for the coming v5.3 upgrade.

  There is no enforcement height defined to not set it until everything is ready but.. we aren't far from be able to enforce it on testnet and start testing the release deeply, only have to merge #2411 and can be done.

ACKs for top commit:
  random-zebra:
    ACK 1355b20
  Fuzzbawls:
    ACK 1355b20

Tree-SHA512: 9282857e54f5a03ef43d9098a376bd403729304fb8a12be9a68c7dca4b6b29aba444f60158df1d50715f0b40db34225771f39770d05c9f9be16226cb4cb8975b

src/Makefile.test.include

@@ -84,7 +84,9 @@ BITCOIN_TEST_SUITE = \
 if ENABLE_WALLET
 BITCOIN_TEST_SUITE += \
   wallet/test/wallet_test_fixture.cpp \
-  wallet/test/wallet_test_fixture.h
+  wallet/test/wallet_test_fixture.h \
+  wallet/test/pos_test_fixture.cpp \
+  wallet/test/pos_test_fixture.h
 endif
 
 # test_pivx binary #
@@ -174,7 +176,8 @@ SAPLING_TESTS +=\
   test/librust/sapling_rpc_wallet_tests.cpp \
   test/librust/sapling_wallet_tests.cpp \
   wallet/test/wallet_shielded_balances_tests.cpp \
-  wallet/test/wallet_sapling_transactions_validations_tests.cpp
+  wallet/test/wallet_sapling_transactions_validations_tests.cpp \
+  wallet/test/pos_validations_tests.cpp
 endif
 
 test_test_pivx_SOURCES = $(BITCOIN_TEST_SUITE) $(BITCOIN_TESTS) $(SAPLING_TESTS) $(JSON_TEST_FILES) $(RAW_TEST_FILES)

src/chainparams.cpp

@@ -201,6 +201,8 @@ class CMainParams : public CChainParams
         consensus.vUpgrades[Consensus::UPGRADE_V4_0].nActivationHeight          = 2153200;
         consensus.vUpgrades[Consensus::UPGRADE_V5_0].nActivationHeight          = 2700500;
         consensus.vUpgrades[Consensus::UPGRADE_V5_2].nActivationHeight          = 2927000;
+        consensus.vUpgrades[Consensus::UPGRADE_V5_3].nActivationHeight =
+                Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT;
         consensus.vUpgrades[Consensus::UPGRADE_V6_0].nActivationHeight =
                 Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT;
 
@@ -340,6 +342,8 @@ class CTestNetParams : public CChainParams
         consensus.vUpgrades[Consensus::UPGRADE_V4_0].nActivationHeight          = 201;
         consensus.vUpgrades[Consensus::UPGRADE_V5_0].nActivationHeight          = 201;
         consensus.vUpgrades[Consensus::UPGRADE_V5_2].nActivationHeight          = 262525;
+        consensus.vUpgrades[Consensus::UPGRADE_V5_3].nActivationHeight =
+                Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT;
         consensus.vUpgrades[Consensus::UPGRADE_V6_0].nActivationHeight =
                 Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT;
 
@@ -472,6 +476,7 @@ class CRegTestParams : public CChainParams
                 Consensus::NetworkUpgrade::ALWAYS_ACTIVE;
         consensus.vUpgrades[Consensus::UPGRADE_V5_0].nActivationHeight          = 300;
         consensus.vUpgrades[Consensus::UPGRADE_V5_2].nActivationHeight          = 300;
+        consensus.vUpgrades[Consensus::UPGRADE_V5_3].nActivationHeight          = 251;
         consensus.vUpgrades[Consensus::UPGRADE_V6_0].nActivationHeight =
                 Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT;
 

src/consensus/params.h

@@ -35,6 +35,7 @@ enum UpgradeIndex : uint32_t {
     UPGRADE_V4_0,
     UPGRADE_V5_0,
     UPGRADE_V5_2,
+    UPGRADE_V5_3,
     UPGRADE_V6_0,
     UPGRADE_TESTDUMMY,
     // NOTE: Also add new upgrades to NetworkUpgradeInfo in upgrades.cpp

src/consensus/upgrades.cpp

@@ -31,31 +31,35 @@ const struct NUInfo NetworkUpgradeInfo[Consensus::MAX_NETWORK_UPGRADES] = {
         },
         {
                 /*.strName =*/ "Zerocoin_v2",
-                /*.strInfo =*/ "new zerocoin serials and zPOS start",
+                /*.strInfo =*/ "New zerocoin serials and zPOS start",
         },
         {
                 /*.strName =*/ "BIP65",
                 /*.strInfo =*/ "CLTV (BIP65) activation - start block v5",
         },
         {
                 /*.strName =*/ "Zerocoin_Public",
-                /*.strInfo =*/ "activation of zerocoin public spends (spend v3)",
+                /*.strInfo =*/ "Activation of zerocoin public spends (spend v3)",
         },
         {
                 /*.strName =*/ "PIVX_v3.4",
-                /*.strInfo =*/ "new 256-bit stake modifier - start block v6",
+                /*.strInfo =*/ "New 256-bit stake modifier - start block v6",
         },
         {
                 /*.strName =*/ "PIVX_v4.0",
-                /*.strInfo =*/ "new message sigs - start block v7 - time protocol - zc spend v4",
+                /*.strInfo =*/ "New message sigs - start block v7 - time protocol - zc spend v4",
         },
         {
                 /*.strName =*/ "v5_shield",
                 /*.strInfo =*/ "Sapling Shield - start block v8 - start transaction v3",
         },
         {
                 /*.strName =*/ "PIVX_v5.2",
-                /*.strInfo =*/ "new cold-staking rules",
+                /*.strInfo =*/ "New cold-staking rules",
+        },
+        {
+                /*.strName =*/ "PIVX_v5.3",
+                /*.strInfo =*/ "New staking rules",
         },
         {
                 /*.strName =*/ "v6_evo",

src/masternode-payments.cpp

@@ -161,11 +161,11 @@ std::string CMasternodePaymentWinner::GetStrMessage() const
     return vinMasternode.prevout.ToStringShort() + std::to_string(nBlockHeight) + HexStr(payee);
 }
 
-bool CMasternodePaymentWinner::IsValid(CNode* pnode, std::string& strError)
+bool CMasternodePaymentWinner::IsValid(CNode* pnode, std::string& strError, int chainHeight)
 {
     int n = mnodeman.GetMasternodeRank(vinMasternode, nBlockHeight - 100);
-
-    if (n < 1 || n > MNPAYMENTS_SIGNATURES_TOTAL) {
+    bool guard = Params().GetConsensus().NetworkUpgradeActive(chainHeight, Consensus::UPGRADE_V5_3);
+    if ((guard && n < 1) || n > MNPAYMENTS_SIGNATURES_TOTAL) {
         //It's common to have masternodes mistakenly think they are in the top 10
         // We don't want to print all of these messages, or punish them unless they're way off
         if (n > MNPAYMENTS_SIGNATURES_TOTAL * 2) {
@@ -176,6 +176,12 @@ bool CMasternodePaymentWinner::IsValid(CNode* pnode, std::string& strError)
         return false;
     }
 
+    // Must be a P2PKH
+    if (guard && !payee.IsPayToPublicKeyHash()) {
+        LogPrint(BCLog::MASTERNODE, "%s - payee must be a P2PKH\n", __func__);
+        return false;
+    }
+
     return true;
 }
 
@@ -219,9 +225,9 @@ bool IsBlockValueValid(int nHeight, CAmount& nExpectedValue, CAmount nMinted, CA
         }
     }
 
-    // !todo: remove after V6 enforcement and default it to true
-    const bool isV6UpgradeEnforced = consensus.NetworkUpgradeActive(nHeight, Consensus::UPGRADE_V6_0);
-    return (!isV6UpgradeEnforced || nMinted >= 0) && nMinted <= nExpectedValue;
+    // !todo: remove after V5.3 enforcement and default it to true
+    const bool isUpgradeEnforced = consensus.NetworkUpgradeActive(nHeight, Consensus::UPGRADE_V5_3);
+    return (!isUpgradeEnforced || nMinted >= 0) && nMinted <= nExpectedValue;
 }
 
 bool IsBlockPayeeValid(const CBlock& block, const CBlockIndex* pindexPrev)
@@ -326,7 +332,7 @@ bool CMasternodePayments::GetLegacyMasternodeTxOut(int nHeight, std::vector<CTxO
     if (!GetBlockPayee(nHeight, payee)) {
         //no masternode detected
         const Consensus::Params& consensus = Params().GetConsensus();
-        const uint256& hash = consensus.NetworkUpgradeActive(nHeight, Consensus::UPGRADE_V6_0) ?
+        const uint256& hash = consensus.NetworkUpgradeActive(nHeight, Consensus::UPGRADE_V5_3) ?
                               mnodeman.GetHashAtHeight(nHeight - 1) : consensus.hashGenesisBlock;
         MasternodeRef winningNode = mnodeman.GetCurrentMasterNode(hash);
         if (winningNode) {
@@ -457,7 +463,7 @@ void CMasternodePayments::ProcessMessageMasternodePayments(CNode* pfrom, std::st
         }
 
         std::string strError = "";
-        if (!winner.IsValid(pfrom, strError)) {
+        if (!winner.IsValid(pfrom, strError, nHeight)) {
             // if(strError != "") LogPrint(BCLog::MASTERNODE,"mnw - invalid message - %s\n", strError);
             return;
         }

src/masternode-payments.h

@@ -177,7 +177,7 @@ class CMasternodePaymentWinner : public CSignedMessage
     std::string GetStrMessage() const override;
     CTxIn GetVin() const { return vinMasternode; };
 
-    bool IsValid(CNode* pnode, std::string& strError);
+    bool IsValid(CNode* pnode, std::string& strError, int chainHeight);
     void Relay();
 
     void AddPayee(const CScript& payeeIn)

src/masternode.cpp

@@ -62,6 +62,13 @@ int MasternodeRemovalSeconds()
     return Params().IsRegTestNet() ? MASTERNODE_REMOVAL_SECONDS_REGTEST : MASTERNODE_REMOVAL_SECONDS;
 }
 
+// Used for sigTime < maxTimeWindow
+int64_t GetMaxTimeWindow(int chainHeight)
+{
+    bool isV5_3 = Params().GetConsensus().NetworkUpgradeActive(chainHeight, Consensus::UPGRADE_V5_3);
+    return GetAdjustedTime() + (isV5_3 ? (60 * 2) : (60 * 60));
+}
+
 
 CMasternode::CMasternode() :
         CSignedMessage()
@@ -136,7 +143,7 @@ std::string CMasternode::GetStrMessage() const
 //
 // When a new masternode broadcast is sent, update our information
 //
-bool CMasternode::UpdateFromNewBroadcast(CMasternodeBroadcast& mnb)
+bool CMasternode::UpdateFromNewBroadcast(CMasternodeBroadcast& mnb, int chainHeight)
 {
     if (mnb.sigTime > sigTime) {
         // TODO: lock cs. Need to be careful as mnb.lastPing.CheckAndUpdate locks cs_main internally.
@@ -147,7 +154,7 @@ bool CMasternode::UpdateFromNewBroadcast(CMasternodeBroadcast& mnb)
         protocolVersion = mnb.protocolVersion;
         addr = mnb.addr;
         int nDoS = 0;
-        if (mnb.lastPing.IsNull() || (!mnb.lastPing.IsNull() && mnb.lastPing.CheckAndUpdate(nDoS, false))) {
+        if (mnb.lastPing.IsNull() || (!mnb.lastPing.IsNull() && mnb.lastPing.CheckAndUpdate(nDoS, chainHeight, false))) {
             lastPing = mnb.lastPing;
             mnodeman.mapSeenMasternodePing.emplace(lastPing.GetHash(), lastPing);
         }
@@ -393,10 +400,10 @@ bool CMasternodeBroadcast::CheckDefaultPort(CService service, std::string& strEr
     return true;
 }
 
-bool CMasternodeBroadcast::CheckAndUpdate(int& nDos)
+bool CMasternodeBroadcast::CheckAndUpdate(int& nDos, int nChainHeight)
 {
     // make sure signature isn't in the future (past is OK)
-    if (sigTime > GetAdjustedTime() + 60 * 60) {
+    if (sigTime > GetMaxTimeWindow(nChainHeight)) {
         LogPrint(BCLog::MASTERNODE,"mnb - Signature rejected, too far into the future %s\n", vin.prevout.hash.ToString());
         nDos = 1;
         return false;
@@ -444,7 +451,7 @@ bool CMasternodeBroadcast::CheckAndUpdate(int& nDos)
         return false;
 
     // incorrect ping or its sigTime
-    if(lastPing.IsNull() || !lastPing.CheckAndUpdate(nDos, false, true)) {
+    if(lastPing.IsNull() || !lastPing.CheckAndUpdate(nDos, nChainHeight, false, true)) {
         return false;
     }
 
@@ -470,7 +477,7 @@ bool CMasternodeBroadcast::CheckAndUpdate(int& nDos)
     if (pmn->pubKeyCollateralAddress == pubKeyCollateralAddress && !pmn->IsBroadcastedWithin(MasternodeBroadcastSeconds())) {
         //take the newest entry
         LogPrint(BCLog::MASTERNODE,"mnb - Got updated entry for %s\n", vin.prevout.hash.ToString());
-        if (pmn->UpdateFromNewBroadcast((*this))) {
+        if (pmn->UpdateFromNewBroadcast((*this), nChainHeight)) {
             if (pmn->IsEnabled()) Relay();
         }
         masternodeSync.AddedMasternodeList(GetHash());
@@ -491,7 +498,7 @@ bool CMasternodeBroadcast::CheckInputsAndAdd(int nChainHeight, int& nDoS)
     }
 
     // incorrect ping or its sigTime
-    if(lastPing.IsNull() || !lastPing.CheckAndUpdate(nDoS, false, true)) {
+    if(lastPing.IsNull() || !lastPing.CheckAndUpdate(nDoS, nChainHeight, false, true)) {
         return false;
     }
 
@@ -589,9 +596,9 @@ std::string CMasternodePing::GetStrMessage() const
     return vin.ToString() + blockHash.ToString() + std::to_string(sigTime);
 }
 
-bool CMasternodePing::CheckAndUpdate(int& nDos, bool fRequireAvailable, bool fCheckSigTimeOnly)
+bool CMasternodePing::CheckAndUpdate(int& nDos, int nChainHeight, bool fRequireAvailable, bool fCheckSigTimeOnly)
 {
-    if (sigTime > GetAdjustedTime() + 60 * 60) {
+    if (sigTime > GetMaxTimeWindow(nChainHeight)) {
         LogPrint(BCLog::MNPING,"%s: Signature rejected, too far into the future %s\n", __func__, vin.prevout.hash.ToString());
         nDos = 30;
         return false;

src/masternode.h

@@ -58,7 +58,7 @@ class CMasternodePing : public CSignedMessage
     const CTxIn GetVin() const { return vin; };
     bool IsNull() const { return blockHash.IsNull() || vin.prevout.IsNull(); }
 
-    bool CheckAndUpdate(int& nDos, bool fRequireAvailable = true, bool fCheckSigTimeOnly = false);
+    bool CheckAndUpdate(int& nDos, int nChainHeight, bool fRequireAvailable = true, bool fCheckSigTimeOnly = false);
     void Relay();
 
     CMasternodePing& operator=(const CMasternodePing& other) = default;
@@ -156,7 +156,7 @@ class CMasternode : public CSignedMessage
         Unserialize(s);
     }
 
-    bool UpdateFromNewBroadcast(CMasternodeBroadcast& mnb);
+    bool UpdateFromNewBroadcast(CMasternodeBroadcast& mnb, int chainHeight);
 
     CMasternode::state GetActiveState() const;
 
@@ -243,7 +243,7 @@ class CMasternodeBroadcast : public CMasternode
     CMasternodeBroadcast(CService newAddr, CTxIn newVin, CPubKey newPubkey, CPubKey newPubkey2, int protocolVersionIn, const CMasternodePing& _lastPing);
     CMasternodeBroadcast(const CMasternode& mn);
 
-    bool CheckAndUpdate(int& nDoS);
+    bool CheckAndUpdate(int& nDoS, int nChainHeight);
     bool CheckInputsAndAdd(int chainHeight, int& nDos);
 
     uint256 GetHash() const;

src/masternodeman.cpp

@@ -713,7 +713,7 @@ int CMasternodeMan::ProcessMNBroadcast(CNode* pfrom, CMasternodeBroadcast& mnb)
     }
 
     int nDoS = 0;
-    if (!mnb.CheckAndUpdate(nDoS)) {
+    if (!mnb.CheckAndUpdate(nDoS, GetBestHeight())) {
         return nDoS;
     }
 
@@ -747,7 +747,7 @@ int CMasternodeMan::ProcessMNPing(CNode* pfrom, CMasternodePing& mnp)
     if (mapSeenMasternodePing.count(mnpHash)) return 0; //seen
 
     int nDoS = 0;
-    if (mnp.CheckAndUpdate(nDoS)) return 0;
+    if (mnp.CheckAndUpdate(nDoS, GetBestHeight())) return 0;
 
     if (nDoS > 0) {
         // if anything significant failed, mark that node
@@ -894,7 +894,7 @@ void CMasternodeMan::UpdateMasternodeList(CMasternodeBroadcast& mnb)
         CMasternode mn(mnb);
         Add(mn);
     } else {
-        pmn->UpdateFromNewBroadcast(mnb);
+        pmn->UpdateFromNewBroadcast(mnb, GetBestHeight());
     }
 }
 

src/test/CMakeLists.txt

@@ -88,6 +88,8 @@ set(BITCOIN_TEST_SUITE
         ${CMAKE_CURRENT_SOURCE_DIR}/librust/sapling_test_fixture.cpp
         ${CMAKE_SOURCE_DIR}/src/wallet/test/wallet_test_fixture.h
         ${CMAKE_SOURCE_DIR}/src/wallet/test/wallet_test_fixture.cpp
+        ${CMAKE_SOURCE_DIR}/src/wallet/test/pos_test_fixture.h
+        ${CMAKE_SOURCE_DIR}/src/wallet/test/pos_test_fixture.cpp
         ${CMAKE_CURRENT_SOURCE_DIR}/librust/utiltest.h
         ${CMAKE_CURRENT_SOURCE_DIR}/librust/utiltest.cpp
         ${CMAKE_CURRENT_SOURCE_DIR}/librust/json_test_vectors.h
@@ -175,6 +177,7 @@ set(BITCOIN_TESTS
         ${CMAKE_SOURCE_DIR}/src/wallet/test/crypto_tests.cpp
         ${CMAKE_SOURCE_DIR}/src/wallet/test/wallet_shielded_balances_tests.cpp
         ${CMAKE_SOURCE_DIR}/src/wallet/test/wallet_sapling_transactions_validations_tests.cpp
+        ${CMAKE_SOURCE_DIR}/src/wallet/test/pos_validations_tests.cpp
         )
 
 set(test_test_pivx_SOURCES ${BITCOIN_TEST_SUITE} ${BITCOIN_TESTS} ${JSON_TEST_FILES})

src/test/budget_tests.cpp

@@ -136,7 +136,7 @@ BOOST_FIXTURE_TEST_CASE(block_value_undermint, RegTestingSetup)
     CAmount nBudgetAmtRet = 0;
     // under-minting blocks are invalid after v6
     BOOST_CHECK(IsBlockValueValid(nHeight, nExpectedRet, -1, nBudgetAmtRet));
-    UpdateNetworkUpgradeParameters(Consensus::UPGRADE_V6_0, Consensus::NetworkUpgrade::ALWAYS_ACTIVE);
+    UpdateNetworkUpgradeParameters(Consensus::UPGRADE_V5_3, Consensus::NetworkUpgrade::ALWAYS_ACTIVE);
     BOOST_CHECK(!IsBlockValueValid(nHeight, nExpectedRet, -1, nBudgetAmtRet));
 }
 

src/validation.cpp

@@ -2935,6 +2935,22 @@ bool ContextualCheckBlock(const CBlock& block, CValidationState& state, CBlockIn
         }
     }
 
+    bool fActiveV5_3 = chainparams.GetConsensus().NetworkUpgradeActive(nHeight, Consensus::UPGRADE_V5_3);
+    if (fActiveV5_3 && block.IsProofOfStake()) {
+        CTransactionRef csTx = block.vtx[1];
+        if (csTx->vin.size() > 1) {
+            return state.DoS(100, false, REJECT_INVALID, "bad-cs-multi-inputs", false,
+                             "invalid multi-inputs coinstake");
+        }
+
+        // Prevent multi-empty-outputs
+        for (int i=1; i<csTx->vout.size(); i++ ) {
+            if (csTx->vout[i].IsEmpty()) {
+                return state.DoS(100, false, REJECT_INVALID, "bad-txns-vout-empty");
+            }
+        }
+    }
+
     return true;
 }
 

src/wallet/test/pos_test_fixture.cpp

@@ -0,0 +1,40 @@
+// Copyright (c) 2021 The PIVX developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or https://www.opensource.org/licenses/mit-license.php.
+
+#include "wallet/test/pos_test_fixture.h"
+#include "wallet/wallet.h"
+
+#include <boost/test/unit_test.hpp>
+
+TestPoSChainSetup::TestPoSChainSetup() : TestChainSetup(0)
+{
+    initZKSNARKS(); // init zk-snarks lib
+
+    bool fFirstRun;
+    pwalletMain = std::make_unique<CWallet>("testWallet", CWalletDBWrapper::CreateMock());
+    pwalletMain->LoadWallet(fFirstRun);
+    RegisterValidationInterface(pwalletMain.get());
+
+    {
+        LOCK(pwalletMain->cs_wallet);
+        pwalletMain->SetMinVersion(FEATURE_SAPLING);
+        gArgs.ForceSetArg("-keypool", "5");
+        pwalletMain->SetupSPKM(true);
+
+        // import coinbase key used to generate the 100-blocks chain
+        BOOST_CHECK(pwalletMain->AddKeyPubKey(coinbaseKey, coinbaseKey.GetPubKey()));
+    }
+
+    int posActivation = Params().GetConsensus().vUpgrades[Consensus::UPGRADE_POS].nActivationHeight - 1;
+    for (int i = 0; i < posActivation; i++) {
+        CBlock b = CreateAndProcessBlock({}, coinbaseKey);
+        coinbaseTxns.emplace_back(*b.vtx[0]);
+    }
+}
+
+TestPoSChainSetup::~TestPoSChainSetup()
+{
+    SyncWithValidationInterfaceQueue();
+    UnregisterValidationInterface(pwalletMain.get());
+}