Merge #2345: [TierTwo] Masternode collateral auto-locking + more tests

4fd564c [Tests] Add EvoNotificationInterface in the TestingSetup fixture (random-zebra)
f6aa6ad [BUG] Check masternode/budget payments during block connection (random-zebra)
57fa99f [Tests] Introduce tiertwo_reorg_mempool test (random-zebra)
9a1bf27 [BUG] Add special tx processing to RollforwardBlock (random-zebra)
5d6bcef [RPC][Tests] Include proTx data in json formatted transactions (random-zebra)
93e8453 [Tests] Update deterministicmns test checking collateral locking (random-zebra)
cc132a8 [Wallet] Implement auto-locking of masternode collaterals (random-zebra)
c857636 [Tests] Introduce new functional test tiertwo_deterministicmns.py (random-zebra)

Pull request description:

  Another one coming from #2267
  This PR introduces a "auto-locking" feature for masternode collaterals:

  - at startup, the wallet is scanned, and any masternode collateral coin found gets locked. Can be disabled setting `-mnconflock=0` (although, there is no need for a `masternode.conf` anymore, so we might want to consider defining and using a new flag here, and removing `mnconflock` after 6.0).

  - during runtime, when a ProRegTx is processed by `CWallet::AddToWalletIfInvolvingMe`, the wallet checks for ownership of the collateral, and automatically locks it in case.

  Here, we also add two more functional tests (`tiertwo_deterministicmns.py`, `tiertwo_reorg_mempool.py`), update the other tests, checking the auto-locking feature, and fix a couple bugs:

  - Missing special tx processing in RollforwardBlock

  - Validity of tiertwo payments can (and must) be verified only during block connection, as we keep the deterministic state only for the active chain, not for all possible chaintips.

  Builds on top of:
  - [x] #2308
  - [x] #2309

ACKs for top commit:
  furszy:
    re-ACK 4fd564c after the tiny fix and merging..

Tree-SHA512: b0f05d9265c02fe90baf48e5dcf66b34f078b90ffd0eef1488606013f84996b2ebaa7d70725c59db2f7456105a6eff2ba08da966858f81c4f19d014a72cd7692

src/evo/deterministicmns.cpp

@@ -7,6 +7,7 @@
 
 #include "base58.h"
 #include "chainparams.h"
+#include "consensus/upgrades.h"
 #include "core_io.h"
 #include "evo/specialtx.h"
 #include "guiinterface.h"
@@ -762,6 +763,7 @@ CDeterministicMNList CDeterministicMNManager::GetListForBlock(const CBlockIndex*
 {
     LOCK(cs);
 
+    // Return early before enforcement
     if (!IsDIP3Enforced(pindex->nHeight)) {
         return {};
     }
@@ -792,7 +794,13 @@ CDeterministicMNList CDeterministicMNManager::GetListForBlock(const CBlockIndex*
 
         CDeterministicMNListDiff diff;
         if (!evoDb.Read(std::make_pair(DB_LIST_DIFF, pindex->GetBlockHash()), diff)) {
-            // no snapshot and no diff on disk means that it's the initial snapshot
+            // no snapshot and no diff on disk means that it's initial snapshot (empty list)
+            // If we get here, then this must be the block before the enforcement of DIP3.
+            if (!IsActivationHeight(pindex->nHeight + 1, Params().GetConsensus(), Consensus::UPGRADE_V6_0)) {
+                std::string err = strprintf("No masternode list data found for block %s at height %d. "
+                                            "Possible corrupt database.", pindex->GetBlockHash().ToString(), pindex->nHeight);
+                throw std::runtime_error(err);
+            }
             snapshot = CDeterministicMNList(pindex->GetBlockHash(), -1, 0);
             mnListsCache.emplace(pindex->GetBlockHash(), snapshot);
             break;

src/evo/deterministicmns.h

@@ -586,6 +586,7 @@ class CDeterministicMNManager
     bool BuildNewListFromBlock(const CBlock& block, const CBlockIndex* pindexPrev, CValidationState& state, CDeterministicMNList& mnListRet, bool debugLogs);
     void DecreasePoSePenalties(CDeterministicMNList& mnList);
 
+    // to return a valid list, it must have been built first, so never call it with a block not-yet connected (e.g. from CheckBlock).
     CDeterministicMNList GetListForBlock(const CBlockIndex* pindex);
     CDeterministicMNList GetListAtChainTip();
 

src/evo/providertx.cpp

@@ -246,3 +246,20 @@ void ProRegPL::ToJson(UniValue& obj) const
     obj.pushKV("operatorReward", (double)nOperatorReward / 100);
     obj.pushKV("inputsHash", inputsHash.ToString());
 }
+
+bool GetProRegCollateral(const CTransactionRef& tx, COutPoint& outRet)
+{
+    if (tx == nullptr) {
+        return false;
+    }
+    if (!tx->IsSpecialTx() || tx->nType != CTransaction::TxType::PROREG) {
+        return false;
+    }
+    ProRegPL pl;
+    if (!GetTxPayload(*tx, pl)) {
+        return false;
+    }
+    outRet = pl.collateralOutpoint.hash.IsNull() ? COutPoint(tx->GetHash(), pl.collateralOutpoint.n)
+                                                 : pl.collateralOutpoint;
+    return true;
+}

src/evo/providertx.h

@@ -70,4 +70,8 @@ class ProRegPL
 
 bool CheckProRegTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValidationState& state);
 
+// If tx is a ProRegTx, return the collateral outpoint in outRet.
+bool GetProRegCollateral(const CTransactionRef& tx, COutPoint& outRet);
+
+
 #endif  //PIVX_PROVIDERTX_H

src/init.cpp

@@ -22,6 +22,7 @@
 #include "checkpoints.h"
 #include "compat/sanity.h"
 #include "consensus/upgrades.h"
+#include "evo/deterministicmns.h"
 #include "evo/evonotificationinterface.h"
 #include "fs.h"
 #include "httpserver.h"
@@ -1860,6 +1861,7 @@ bool AppInitMain()
     strBudgetMode = gArgs.GetArg("-budgetvotemode", "auto");
 
 #ifdef ENABLE_WALLET
+    // !TODO: remove after complete transition to DMN
     // use only the first wallet here. This section can be removed after transition to DMN
     if (gArgs.GetBoolArg("-mnconflock", DEFAULT_MNCONFLOCK) && !vpwallets.empty() && vpwallets[0]) {
         LOCK(vpwallets[0]->cs_wallet);
@@ -1873,6 +1875,14 @@ bool AppInitMain()
                       mne.getAlias(), mne.getTxHash(), mne.getOutputIndex());
         }
     }
+
+    // automatic lock for DMN
+    if (gArgs.GetBoolArg("-mnconflock", DEFAULT_MNCONFLOCK)) {
+        const auto& mnList = deterministicMNManager->GetListAtChainTip();
+        for (CWallet* pwallet : vpwallets) {
+            pwallet->ScanMasternodeCollateralsAndLock(mnList);
+        }
+    }
 #endif
 
     // lite mode disables all Masternode related functionality

src/test/evo_deterministicmns_tests.cpp

@@ -19,6 +19,7 @@
 #include "script/sign.h"
 #include "spork.h"
 #include "validation.h"
+#include "validationinterface.h"
 
 #include <boost/test/unit_test.hpp>
 
@@ -192,13 +193,20 @@ BOOST_FIXTURE_TEST_CASE(dip3_protx, TestChain400Setup)
 
     CBlockIndex* chainTip = chainActive.Tip();
     int nHeight = chainTip->nHeight;
-    UpdateNetworkUpgradeParameters(Consensus::UPGRADE_V6_0, nHeight);
+    UpdateNetworkUpgradeParameters(Consensus::UPGRADE_V6_0, nHeight + 2);
+
+    // load empty list (last block before enforcement)
+    CreateAndProcessBlock({}, coinbaseKey);
+    chainTip = chainActive.Tip();
+    BOOST_CHECK_EQUAL(chainTip->nHeight, ++nHeight);
+
+    // force mnsync complete and enable spork 8
     masternodeSync.RequestedMasternodeAssets = MASTERNODE_SYNC_FINISHED;
-    // enable SPORK_8
     int64_t nTime = GetTime() - 10;
     const CSporkMessage& sporkMnPayment = CSporkMessage(SPORK_8_MASTERNODE_PAYMENT_ENFORCEMENT, nTime + 1, nTime);
     sporkManager.AddOrUpdateSporkMessage(sporkMnPayment);
     BOOST_CHECK(sporkManager.IsSporkActive(SPORK_8_MASTERNODE_PAYMENT_ENFORCEMENT));
+
     int port = 1;
 
     std::vector<uint256> dmnHashes;
@@ -232,8 +240,7 @@ BOOST_FIXTURE_TEST_CASE(dip3_protx, TestChain400Setup)
         CreateAndProcessBlock({tx}, coinbaseKey);
         chainTip = chainActive.Tip();
         BOOST_CHECK_EQUAL(chainTip->nHeight, nHeight + 1);
-
-        deterministicMNManager->UpdatedBlockTip(chainTip);
+        SyncWithValidationInterfaceQueue();
         BOOST_CHECK(deterministicMNManager->GetListAtChainTip().HasMN(txid));
 
         // Add change to the utxos map
@@ -252,10 +259,10 @@ BOOST_FIXTURE_TEST_CASE(dip3_protx, TestChain400Setup)
     // Mine 20 blocks, checking MN reward payments
     std::map<uint256, int> mapPayments;
     for (size_t i = 0; i < 20; i++) {
+        SyncWithValidationInterfaceQueue();
         auto dmnExpectedPayee = deterministicMNManager->GetListAtChainTip().GetMNPayee();
         CBlock block = CreateAndProcessBlock({}, coinbaseKey);
         chainTip = chainActive.Tip();
-        deterministicMNManager->UpdatedBlockTip(chainTip);
         BOOST_ASSERT(!block.vtx.empty());
         BOOST_CHECK(IsMNPayeeInBlock(block, dmnExpectedPayee->pdmnState->scriptPayout));
         mapPayments[dmnExpectedPayee->proTxHash]++;
@@ -356,8 +363,7 @@ BOOST_FIXTURE_TEST_CASE(dip3_protx, TestChain400Setup)
         CreateAndProcessBlock(txns, coinbaseKey);
         chainTip = chainActive.Tip();
         BOOST_CHECK_EQUAL(chainTip->nHeight, nHeight + 1);
-
-        deterministicMNManager->UpdatedBlockTip(chainTip);
+        SyncWithValidationInterfaceQueue();
         auto mnList = deterministicMNManager->GetListAtChainTip();
         for (size_t j = 0; j < 3; j++) {
             BOOST_CHECK(mnList.HasMN(txns[j].GetHash()));
@@ -369,10 +375,10 @@ BOOST_FIXTURE_TEST_CASE(dip3_protx, TestChain400Setup)
     // Mine 30 blocks, checking MN reward payments
     mapPayments.clear();
     for (size_t i = 0; i < 30; i++) {
+        SyncWithValidationInterfaceQueue();
         auto dmnExpectedPayee = deterministicMNManager->GetListAtChainTip().GetMNPayee();
         CBlock block = CreateAndProcessBlock({}, coinbaseKey);
         chainTip = chainActive.Tip();
-        deterministicMNManager->UpdatedBlockTip(chainTip);
         BOOST_ASSERT(!block.vtx.empty());
         BOOST_CHECK(IsMNPayeeInBlock(block, dmnExpectedPayee->pdmnState->scriptPayout));
         mapPayments[dmnExpectedPayee->proTxHash]++;
@@ -403,8 +409,11 @@ BOOST_FIXTURE_TEST_CASE(dip3_protx, TestChain400Setup)
     pblock->vtx[0] = MakeTransactionRef(invalidCoinbaseTx);
     pblock->hashMerkleRoot = BlockMerkleRoot(*pblock);
     CValidationState state;
-    BOOST_CHECK_MESSAGE(!ProcessNewBlock(state, nullptr, pblock, nullptr), "Error, invalid block paying to an already paid DMN passed");
-    BOOST_CHECK(WITH_LOCK(cs_main, return chainActive.Height()) == nHeight); // no block connected
+    ProcessNewBlock(state, nullptr, pblock, nullptr);
+    // block not connected
+    chainTip = WITH_LOCK(cs_main, return chainActive.Tip());
+    BOOST_CHECK(chainTip->nHeight == nHeight);
+    BOOST_CHECK(chainTip->GetBlockHash() != pblock->GetHash());
 
     UpdateNetworkUpgradeParameters(Consensus::UPGRADE_V6_0, Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT);
 }

src/test/test_pivx.cpp

@@ -11,6 +11,7 @@
 #include "guiinterface.h"
 #include "evo/deterministicmns.h"
 #include "evo/evodb.h"
+#include "evo/evonotificationinterface.h"
 #include "miner.h"
 #include "net_processing.h"
 #include "rpc/server.h"
@@ -81,6 +82,12 @@ TestingSetup::TestingSetup(const std::string& chainName) : BasicTestingSetup(cha
         // our unit tests aren't testing multiple parts of the code at once.
         GetMainSignals().RegisterBackgroundSignalScheduler(scheduler);
 
+        // Register EvoNotificationInterface
+        g_connman = std::unique_ptr<CConnman>(new CConnman(0x1337, 0x1337)); // Deterministic randomness for tests.
+        connman = g_connman.get();
+        pEvoNotificationInterface = new EvoNotificationInterface(*connman);
+        RegisterValidationInterface(pEvoNotificationInterface);
+
         // Ideally we'd move all the RPC tests to the functional testing framework
         // instead of unit tests, but for now we need these here.
         RegisterAllCoreRPCCommands(tableRPC);
@@ -100,8 +107,6 @@ TestingSetup::TestingSetup(const std::string& chainName) : BasicTestingSetup(cha
         nScriptCheckThreads = 3;
         for (int i=0; i < nScriptCheckThreads-1; i++)
             threadGroup.create_thread(&ThreadScriptCheck);
-        g_connman = std::unique_ptr<CConnman>(new CConnman(0x1337, 0x1337)); // Deterministic randomness for tests.
-        connman = g_connman.get();
         RegisterNodeSignals(GetNodeSignals());
 }
 
@@ -114,6 +119,7 @@ TestingSetup::~TestingSetup()
         UnregisterAllValidationInterfaces();
         GetMainSignals().UnregisterBackgroundSignalScheduler();
         UnloadBlockIndex();
+        delete pEvoNotificationInterface;
         delete pcoinsTip;
         delete pcoinsdbview;
         delete pblocktree;

src/test/test_pivx.h

@@ -50,11 +50,13 @@ struct BasicTestingSetup {
  * and wallet (if enabled) setup.
  */
 class CConnman;
+class EvoNotificationInterface;
 struct TestingSetup: public BasicTestingSetup
 {
     CCoinsViewDB *pcoinsdbview;
     boost::thread_group threadGroup;
     CConnman* connman;
+    EvoNotificationInterface* pEvoNotificationInterface;
     CScheduler scheduler;
 
     TestingSetup(const std::string& chainName = CBaseChainParams::MAIN);

src/tiertwo_networksync.cpp

@@ -170,6 +170,12 @@ void CMasternodeSync::RequestDataTo(CNode* pnode, const char* msg, bool forceReq
 
 void CMasternodeSync::SyncRegtest(CNode* pnode)
 {
+    // skip mn list and winners sync if legacy mn are obsolete
+    if (deterministicMNManager->LegacyMNObsolete() &&
+            (RequestedMasternodeAssets == MASTERNODE_SYNC_LIST || RequestedMasternodeAssets == MASTERNODE_SYNC_MNW)) {
+        RequestedMasternodeAssets = MASTERNODE_SYNC_BUDGET;
+    }
+
     // Initial sync, verify that the other peer answered to all of the messages successfully
     if (RequestedMasternodeAssets == MASTERNODE_SYNC_SPORKS) {
         RequestDataTo(pnode, NetMsgType::GETSPORKS, false);

src/validation.cpp

@@ -1717,6 +1717,15 @@ static bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockInd
                          REJECT_INVALID, "bad-blk-amount");
     }
 
+    // Masternode/Budget payments
+    // !TODO: after transition to DMN is complete, check this also during IBD
+    if (!fInitialBlockDownload) {
+        if (!IsBlockPayeeValid(block, pindex->pprev)) {
+            mapRejectedBlocks.emplace(block.GetHash(), GetTime());
+            return state.DoS(0, false, REJECT_INVALID, "bad-cb-payee", false, "Couldn't find masternode/budget payment");
+        }
+    }
+
     // For blocks v10+: Check that the coinbase pays the exact amount
     if (isPoSActive && pindex->nVersion >= 10 && !IsCoinbaseValueValid(block.vtx[0], nBudgetAmt, state)) {
         // pass the state returned by the function above
@@ -2834,12 +2843,6 @@ bool CheckBlock(const CBlock& block, CValidationState& state, bool fCheckPOW, bo
             // set Cold Staking Spork
             fColdStakingActive = !sporkManager.IsSporkActive(SPORK_19_COLDSTAKING_MAINTENANCE);
 
-            // check masternode/budget payment
-            // !TODO: after transition to DMN is complete, check this also during IBD
-            if (!IsBlockPayeeValid(block, pindexPrev)) {
-                mapRejectedBlocks.emplace(block.GetHash(), GetTime());
-                return state.DoS(0, false, REJECT_INVALID, "bad-cb-payee", false, "Couldn't find masternode/budget payment");
-            }
         } else {
             LogPrintf("%s: Masternode/Budget payment checks skipped on sync\n", __func__);
         }
@@ -3743,6 +3746,13 @@ static bool RollforwardBlock(const CBlockIndex* pindex, CCoinsViewCache& inputs,
         // Pass check = true as every addition may be an overwrite.
         AddCoins(inputs, *tx, pindex->nHeight, true, fSkipInvalid);
     }
+
+    CValidationState state;
+    if (!ProcessSpecialTxsInBlock(block, pindex, state, false /*fJustCheck*/)) {
+        return error("%s: Special tx processing failed for block %s with %s",
+                     __func__, pindex->GetBlockHash().ToString(), FormatStateMessage(state));
+    }
+
     return true;
 }
 

src/wallet/wallet.cpp

@@ -9,6 +9,7 @@
 
 #include "budget/budgetmanager.h"
 #include "coincontrol.h"
+#include "evo/deterministicmns.h"
 #include "init.h"
 #include "guiinterfaceutil.h"
 #include "masternode.h"
@@ -963,7 +964,7 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
 {
     LOCK(cs_wallet);
     CWalletDB walletdb(*dbw, "r+", fFlushOnClose);
-    uint256 hash = wtxIn.GetHash();
+    const uint256& hash = wtxIn.GetHash();
 
     // Inserts only if not already there, returns tx inserted or tx found
     std::pair<std::map<uint256, CWalletTx>::iterator, bool> ret = mapWallet.emplace(hash, wtxIn);
@@ -1164,6 +1165,9 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const CWallet
             }
         }
 
+        // If this is a ProRegTx and the wallet owns the collateral, lock the corresponding coin
+        LockIfMyCollateral(ptx);
+
         bool isFromMe = IsFromMe(ptx);
         if (fExisted || IsMine(ptx) || isFromMe || (saplingNoteData && !saplingNoteData->empty())) {
 
@@ -4153,6 +4157,47 @@ void CWallet::AutoCombineDust(CConnman* connman)
     }
 }
 
+void CWallet::LockOutpointIfMine(const CTransactionRef& ptx, const COutPoint& c)
+{
+    AssertLockHeld(cs_wallet);
+    CTxOut txout;
+    if (ptx && c.hash == ptx->GetHash() && c.n < ptx->vout.size()) {
+        // the collateral is an output of this tx
+        txout = ptx->vout[c.n];
+    } else {
+        // the collateral is a reference to an utxo inside this wallet
+        const auto& it = mapWallet.find(c.hash);
+        if (it != mapWallet.end()) {
+            txout = it->second.tx->vout[c.n];
+        }
+    }
+    if (!txout.IsNull() && IsMine(txout) != ISMINE_NO && !IsSpent(c)) {
+        LockCoin(c);
+    }
+}
+
+// Called during Init
+void CWallet::ScanMasternodeCollateralsAndLock(const CDeterministicMNList& mnList)
+{
+    LOCK(cs_wallet);
+
+    LogPrintf("Locking masternode collaterals...\n");
+    mnList.ForEachMN(false, [&](const CDeterministicMNCPtr& dmn) {
+        LockOutpointIfMine(nullptr, dmn->collateralOutpoint);
+    });
+}
+
+// Called from AddToWalletIfInvolvingMe
+void CWallet::LockIfMyCollateral(const CTransactionRef& ptx)
+{
+    AssertLockHeld(cs_wallet);
+
+    COutPoint o;
+    if (GetProRegCollateral(ptx, o)) {
+        LockOutpointIfMine(ptx, o);
+    }
+}
+
 std::string CWallet::GetWalletHelpString(bool showDebug)
 {
     std::string strUsage = HelpMessageGroup(_("Wallet options:"));