[Backport] Align budget payee validity to highest voted budget check.

src/budget/budgetmanager.cpp

@@ -185,7 +185,7 @@ std::string CBudgetManager::GetFinalizedBudgetStatus(const uint256& nHash) const
     return retBadHashes + " -- " + retBadPayeeOrAmount;
 }
 
-bool CBudgetManager::AddFinalizedBudget(CFinalizedBudget& finalizedBudget)
+bool CBudgetManager::AddFinalizedBudget(CFinalizedBudget& finalizedBudget, CNode* pfrom)
 {
     AssertLockNotHeld(cs_budgets);    // need to lock cs_main here (CheckCollateral)
     const uint256& nHash = finalizedBudget.GetHash();
@@ -216,6 +216,25 @@ bool CBudgetManager::AddFinalizedBudget(CFinalizedBudget& finalizedBudget)
         return false;
     }
 
+    // Compare budget payments with existent proposals, don't care on the order, just verify proposals existence.
+    std::vector<CBudgetProposal> vBudget = GetBudget();
+    std::map<uint256, CBudgetProposal> mapWinningProposals;
+    for (const CBudgetProposal& p: vBudget) { mapWinningProposals.emplace(p.GetHash(), p); }
+    if (!finalizedBudget.CheckProposals(mapWinningProposals)) {
+        finalizedBudget.SetStrInvalid("Invalid proposals");
+        LogPrint(BCLog::MNBUDGET,"%s: Budget finalization does not match with winning proposals\n", __func__);
+        // just for now (until v6), request proposals sync in case we are missing one of them.
+        if (pfrom) {
+            for (const auto& propId : finalizedBudget.GetProposalsHashes()) {
+                if (!g_budgetman.HaveProposal(propId)) {
+                    pfrom->PushInventory(CInv(MSG_BUDGET_PROPOSAL, propId));
+                }
+            }
+        }
+        return false;
+    }
+
+    // Add budget finalization.
     SetBudgetProposalsStr(finalizedBudget);
     {
         LOCK(cs_budgets);
@@ -615,19 +634,19 @@ TrxValidationStatus CBudgetManager::IsTransactionValid(const CTransaction& txNew
     bool fThreshold = false;
     {
         LOCK(cs_budgets);
-        for (const auto& it: mapFinalizedBudgets) {
-            const CFinalizedBudget* pfb = &(it.second);
-            const int nVoteCount = pfb->GetVoteCount();
-            LogPrint(BCLog::MNBUDGET,"%s: checking %s (%s): votes %d (threshold %d)\n",
-                    __func__, pfb->GetName(), pfb->GetProposalsStr(), nVoteCount, nCountThreshold);
-            if (nVoteCount > nCountThreshold) {
+        // Get the finalized budget with the highest amount of votes..
+        const CFinalizedBudget* highestVotesBudget = GetBudgetWithHighestVoteCount(nBlockHeight);
+        if (highestVotesBudget) {
+            // Need to surpass the threshold
+            if (highestVotesBudget->GetVoteCount() > nCountThreshold) {
                 fThreshold = true;
-                if (pfb->IsTransactionValid(txNew, nBlockHash, nBlockHeight) == TrxValidationStatus::Valid) {
+                if (highestVotesBudget->IsTransactionValid(txNew, nBlockHash, nBlockHeight) ==
+                    TrxValidationStatus::Valid) {
                     return TrxValidationStatus::Valid;
                 }
-                // tx not valid. keep looking.
-                LogPrint(BCLog::MNBUDGET, "%s: ignoring budget. Out of range or tx not valid.\n", __func__);
             }
+            // tx not valid
+            LogPrint(BCLog::MNBUDGET, "%s: ignoring budget. Out of range or tx not valid.\n", __func__);
         }
     }
 
@@ -979,14 +998,14 @@ int CBudgetManager::ProcessProposalVote(CBudgetVote& vote, CNode* pfrom)
     return 0;
 }
 
-int CBudgetManager::ProcessFinalizedBudget(CFinalizedBudget& finalbudget)
+int CBudgetManager::ProcessFinalizedBudget(CFinalizedBudget& finalbudget, CNode* pfrom)
 {
     const uint256& nHash = finalbudget.GetHash();
     if (HaveFinalizedBudget(nHash)) {
         masternodeSync.AddedBudgetItem(nHash);
         return 0;
     }
-    if (!AddFinalizedBudget(finalbudget)) {
+    if (!AddFinalizedBudget(finalbudget, pfrom)) {
         return 0;
     }
     finalbudget.Relay();
@@ -1026,7 +1045,7 @@ int CBudgetManager::ProcessFinalizedBudgetVote(CFinalizedBudgetVote& vote, CNode
     }
 
     std::string strError;
-    if (UpdateFinalizedBudget(vote, pfrom, strError)) {
+    if (pmn->IsEnabled() && UpdateFinalizedBudget(vote, pfrom, strError)) {
         vote.Relay();
         masternodeSync.AddedBudgetItem(vote.GetHash());
         LogPrint(BCLog::MNBUDGET, "fbvote - new finalized budget vote - %s from masternode %s\n", vote.GetHash().ToString(), HexStr(pmn->pubKeyMasternode));
@@ -1080,7 +1099,7 @@ int CBudgetManager::ProcessMessageInner(CNode* pfrom, std::string& strCommand, C
         if (!finalbudget.ParseBroadcast(vRecv)) {
             return 20;
         }
-        return ProcessFinalizedBudget(finalbudget);
+        return ProcessFinalizedBudget(finalbudget, pfrom);
     }
 
     if (strCommand == NetMsgType::FINALBUDGETVOTE) {

src/budget/budgetmanager.h

@@ -98,7 +98,7 @@ class CBudgetManager
     int ProcessBudgetVoteSync(const uint256& nProp, CNode* pfrom);
     int ProcessProposal(CBudgetProposal& proposal);
     int ProcessProposalVote(CBudgetVote& proposal, CNode* pfrom);
-    int ProcessFinalizedBudget(CFinalizedBudget& finalbudget);
+    int ProcessFinalizedBudget(CFinalizedBudget& finalbudget, CNode* pfrom);
     int ProcessFinalizedBudgetVote(CFinalizedBudgetVote& vote, CNode* pfrom);
 
     // functions returning a pointer in the map. Need cs_proposals/cs_budgets locked from the caller
@@ -118,7 +118,7 @@ class CBudgetManager
     bool IsBudgetPaymentBlock(int nBlockHeight) const;
     bool IsBudgetPaymentBlock(int nBlockHeight, int& nCountThreshold) const;
     bool AddProposal(CBudgetProposal& budgetProposal);
-    bool AddFinalizedBudget(CFinalizedBudget& finalizedBudget);
+    bool AddFinalizedBudget(CFinalizedBudget& finalizedBudget, CNode* pfrom = nullptr);
     uint256 SubmitFinalBudget();
 
     bool UpdateProposal(const CBudgetVote& vote, CNode* pfrom, std::string& strError);

src/rpc/budget.cpp

@@ -623,6 +623,93 @@ UniValue mnfinalbudgetsuggest(const JSONRPCRequest& request)
     return (budgetHash.IsNull()) ? NullUniValue : budgetHash.ToString();
 }
 
+UniValue createrawmnfinalbudget(const JSONRPCRequest& request)
+{
+    if (request.fHelp || request.params.size() > 4)
+        throw std::runtime_error(
+                "createrawmnfinalbudget\n"
+                "\nTry to submit the raw budget finalization\n"
+                "returns the budget hash if it was broadcasted sucessfully"
+                "\nArguments:\n"
+                "1. \"budgetname\"    (string, required) finalization name\n"
+                "2. \"blockstart\"    (numeric, required) superblock height\n"
+                "3. \"proposals\"     (string, required) A json array of json objects\n"
+                "     [\n"
+                "       {\n"
+                "         \"proposalid\":\"id\",  (string, required) The proposal id\n"
+                "         \"payee\":n,         (hex, required) The payee script\n"
+                "         \"amount\":n            (numeric, optional) The payee amount\n"
+                "       }\n"
+                "       ,...\n"
+                "     ]\n"
+                "4. \"feetxid\"    (string, optional) the transaction fee hash\n"
+                ""
+                "\nResult:\n"
+                "{\n"
+                    "\"result\"     (string) Budget suggest broadcast or error\n"
+                    "\"id\"         (string) id of the fee tx or the finalized budget\n"
+                "}\n"
+                ); // future: add examples.
+
+    if (!Params().IsRegTestNet()) {
+        throw JSONRPCError(RPC_MISC_ERROR, "command available only for RegTest network");
+    }
+
+    // future: add inputs error checking..
+    std::string budName = request.params[0].get_str();
+    int nBlockStart = request.params[1].get_int();
+    std::vector<CTxBudgetPayment> vecTxBudgetPayments;
+    UniValue budgetVec = request.params[2].get_array();
+    for (unsigned int idx = 0; idx < budgetVec.size(); idx++) {
+        const UniValue& prop = budgetVec[idx].get_obj();
+        uint256 propId = ParseHashO(prop, "proposalid");
+        std::vector<unsigned char> scriptData(ParseHexO(prop, "payee"));
+        CScript payee = CScript(scriptData.begin(), scriptData.end());
+        CAmount amount = AmountFromValue(find_value(prop, "amount"));
+        vecTxBudgetPayments.emplace_back(propId, payee, amount);
+    }
+
+    Optional<uint256> txFeeId = nullopt;
+    if (request.params.size() > 3) {
+        txFeeId = ParseHashV(request.params[3], "parameter 4");
+    }
+
+    if (!txFeeId) {
+        CFinalizedBudget tempBudget(budName, nBlockStart, vecTxBudgetPayments, UINT256_ZERO);
+        const uint256& budgetHash = tempBudget.GetHash();
+
+        // create fee tx
+        CTransactionRef wtx;
+        CReserveKey keyChange(pwalletMain);
+        if (!pwalletMain->CreateBudgetFeeTX(wtx, budgetHash, keyChange, true)) {
+            throw std::runtime_error("Can't make collateral transaction");
+        }
+        // Send the tx to the network
+        const CWallet::CommitResult& res = pwalletMain->CommitTransaction(wtx, keyChange, g_connman.get());
+        UniValue ret(UniValue::VOBJ);
+        if (res.status == CWallet::CommitStatus::OK) {
+            ret.pushKV("result", "tx_fee_sent");
+            ret.pushKV("id", wtx->GetHash().ToString());
+        } else {
+            ret.pushKV("result", "error");
+        }
+        return ret;
+    }
+
+    UniValue ret(UniValue::VOBJ);
+    // Collateral tx already exists, see if it's mature enough.
+    CFinalizedBudget fb(budName, nBlockStart, vecTxBudgetPayments, *txFeeId);
+    if (g_budgetman.AddFinalizedBudget(fb)) {
+        fb.Relay();
+        ret.pushKV("result", "fin_budget_sent");
+        ret.pushKV("id", fb.GetHash().ToString());
+    } else {
+        // future: add proper error
+        ret.pushKV("result", "error");
+    }
+    return ret;
+}
+
 UniValue mnfinalbudget(const JSONRPCRequest& request)
 {
     std::string strCommand;
@@ -821,7 +908,8 @@ static const CRPCCommand commands[] =
     { "budget",             "checkbudgets",           &checkbudgets,           true  },
 
     /* Not shown in help */
-    { "hidden",             "mnfinalbudgetsuggest",   &mnfinalbudgetsuggest,   true  },
+    { "hidden",             "mnfinalbudgetsuggest",   &mnfinalbudgetsuggest,   true, },
+    { "hidden",             "createrawmnfinalbudget", &createrawmnfinalbudget, true  }
 
 };
 

test/functional/test_runner.py

@@ -144,6 +144,7 @@
     'tiertwo_governance_reorg.py',              # ~ 361 sec
     'tiertwo_masternode_activation.py',
     'tiertwo_masternode_ping.py',
+    'tiertwo_governance_invalid_budget.py',
 ]
 
 SAPLING_SCRIPTS = [

test/functional/tiertwo_governance_invalid_budget.py

@@ -0,0 +1,164 @@
+#!/usr/bin/env python3
+# 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.
+
+from test_framework.test_framework import PivxTestFramework
+from test_framework.util import (
+    assert_equal,
+    p2p_port,
+)
+
+import os
+import time
+
+class GovernanceInvalidBudgetTest(PivxTestFramework):
+
+    def set_test_params(self):
+        self.setup_clean_chain = True
+        # 4 nodes:
+        # - 1 miner/mncontroller
+        # - 2 remote mns
+        # - 1 other node to stake a forked chain
+        self.num_nodes = 4
+        self.extra_args = [["-sporkkey=932HEevBSujW2ud7RfB1YF91AFygbBRQj3de3LyaCRqNzKKgWXi"],
+                           [],
+                           ["-listen", "-externalip=127.0.0.1"],
+                           ["-listen", "-externalip=127.0.0.1"],
+                           ]
+        self.enable_mocktime()
+
+        self.minerAPos = 0
+        self.minerBPos = 1
+        self.remoteOnePos = 1
+        self.remoteTwoPos = 2
+
+        self.masternodeOneAlias = "mnOne"
+        self.masternodeTwoAlias = "mntwo"
+
+        self.mnOnePrivkey = "9247iC59poZmqBYt9iDh9wDam6v9S1rW5XekjLGyPnDhrDkP4AK"
+        self.mnTwoPrivkey = "92Hkebp3RHdDidGZ7ARgS4orxJAGyFUPDXNqtsYsiwho1HGVRbF"
+
+    def run_test(self):
+        self.minerA = self.nodes[self.minerAPos]     # also controller of mn1 and mn2
+        self.minerB = self.nodes[self.minerBPos]
+        self.mn1 = self.nodes[self.remoteOnePos]
+        self.mn2 = self.nodes[self.remoteTwoPos]
+        self.setupContext()
+
+        # Create a proposal and vote on it
+        next_superblock = self.minerA.getnextsuperblock()
+        payee = self.minerA.getnewaddress()
+        self.log.info("Creating a proposal to be paid at block %d" % next_superblock)
+        proposalFeeTxId = self.minerA.preparebudget("test1", "https://test1.org", 2,
+                                               next_superblock, payee, 300)
+        self.stake_and_ping(self.minerAPos, 3, [self.mn1, self.mn2])
+        proposalHash = self.minerA.submitbudget("test1", "https://test1.org", 2,
+                                           next_superblock, payee, 300, proposalFeeTxId)
+        time.sleep(1)
+        self.stake_and_ping(self.minerAPos, 7, [self.mn1, self.mn2])
+        self.log.info("Vote for the proposal and check projection...")
+        self.minerA.mnbudgetvote("alias", proposalHash, "yes", self.masternodeOneAlias)
+        self.minerA.mnbudgetvote("alias", proposalHash, "yes", self.masternodeTwoAlias)
+        time.sleep(1)
+        self.stake_and_ping(self.minerAPos, 1, [self.mn1, self.mn2])
+        projection = self.minerB.getbudgetprojection()[0]
+        assert_equal(projection["Name"], "test1")
+        assert_equal(projection["Hash"], proposalHash)
+        assert_equal(projection["Yeas"], 2)
+
+        # Create invalid finalized budget and vote on it
+        self.log.info("Creating invalid budget finalization...")
+        self.stake_and_ping(self.minerAPos, 5, [self.mn1, self.mn2])
+
+        budgetname = "invalid finalization"
+        blockstart = self.minerA.getnextsuperblock()
+        proposals = []
+        badPropId = "aa0061d705de36385c37701e7632408bd9d2876626b1299a17f7dc818c0ad285"
+        badPropPayee = "8c988f1a4a4de2161e0f50aac7f17e7f9555caa4"
+        badPropAmount = 500
+        proposals.append({"proposalid": badPropId, "payee": badPropPayee, "amount": badPropAmount})
+        res = self.minerA.createrawmnfinalbudget(budgetname, blockstart, proposals)
+        assert(res["result"] == "tx_fee_sent")
+        feeBudgetId = res["id"]
+        time.sleep(1)
+        self.stake_and_ping(self.minerAPos, 4, [self.mn1, self.mn2])
+        res = self.minerA.createrawmnfinalbudget(budgetname, blockstart, proposals, feeBudgetId)
+        assert(res["result"] == "error") # not accepted
+
+        self.log.info("Good, invalid budget not accepted.")
+
+        # Stake up until the block before the superblock.
+        skip_blocks = next_superblock - self.minerA.getblockcount() - 1
+        self.stake_and_ping(self.minerAPos, skip_blocks, [self.mn1, self.mn2])
+
+        # mine the superblock and check payment, it must not pay to the invalid finalization.
+        self.log.info("Checking superblock...")
+        self.stake_and_ping(self.nodes.index(self.minerA), 1, [])
+        assert_equal(self.minerA.getblockcount(), next_superblock)
+        coinstake = self.minerA.getrawtransaction(self.minerA.getblock(self.minerA.getbestblockhash())["tx"][1], True)
+        budget_payment_out = coinstake["vout"][-1]
+        assert(budget_payment_out["scriptPubKey"]["hex"] != badPropPayee)
+        assert(budget_payment_out["value"] != badPropAmount)
+
+        self.log.info("All good.")
+
+    def send_3_pings(self, mn_list):
+        self.advance_mocktime(30)
+        self.send_pings(mn_list)
+        self.stake_and_ping(self.minerAPos, 1, mn_list)
+        self.advance_mocktime(30)
+        self.send_pings(mn_list)
+        time.sleep(2)
+
+    def setupContext(self):
+        # First mine 250 PoW blocks (50 with minerB, 200 with minerA)
+        self.log.info("Generating 259 blocks...")
+        for _ in range(2):
+            for _ in range(25):
+                self.mocktime = self.generate_pow(self.minerBPos, self.mocktime)
+            self.sync_blocks()
+            for _ in range(100):
+                self.mocktime = self.generate_pow(self.minerAPos, self.mocktime)
+            self.sync_blocks()
+        # Then stake 9 blocks with minerA
+        self.stake_and_ping(self.minerAPos, 9, [])
+        for n in self.nodes:
+            assert_equal(n.getblockcount(), 259)
+
+        # Setup Masternodes
+        self.log.info("Masternodes setup...")
+        ownerdir = os.path.join(self.options.tmpdir, "node%d" % self.minerAPos, "regtest")
+        self.mnOneCollateral = self.setupMasternode(self.minerA, self.minerA, self.masternodeOneAlias,
+                                                    ownerdir, self.remoteOnePos, self.mnOnePrivkey)
+        self.mnTwoCollateral = self.setupMasternode(self.minerA, self.minerA, self.masternodeTwoAlias,
+                                                    ownerdir, self.remoteTwoPos, self.mnTwoPrivkey)
+
+        # Activate masternodes
+        self.log.info("Masternodes activation...")
+        self.stake_and_ping(self.minerAPos, 1, [])
+        time.sleep(3)
+        self.advance_mocktime(10)
+        remoteOnePort = p2p_port(self.remoteOnePos)
+        remoteTwoPort = p2p_port(self.remoteTwoPos)
+        self.mn1.initmasternode(self.mnOnePrivkey, "127.0.0.1:"+str(remoteOnePort))
+        self.mn2.initmasternode(self.mnTwoPrivkey, "127.0.0.1:"+str(remoteTwoPort))
+        self.stake_and_ping(self.minerAPos, 1, [])
+        self.wait_until_mnsync_finished()
+        self.controller_start_masternode(self.minerA, self.masternodeOneAlias)
+        self.controller_start_masternode(self.minerA, self.masternodeTwoAlias)
+        self.wait_until_mn_preenabled(self.mnOneCollateral, 40)
+        self.wait_until_mn_preenabled(self.mnOneCollateral, 40)
+        self.send_3_pings([self.mn1, self.mn2])
+        self.wait_until_mn_enabled(self.mnOneCollateral, 120, [self.mn1, self.mn2])
+        self.wait_until_mn_enabled(self.mnOneCollateral, 120, [self.mn1, self.mn2])
+
+        # activate sporks
+        self.log.info("Masternodes enabled. Activating sporks.")
+        self.activate_spork(self.minerAPos, "SPORK_8_MASTERNODE_PAYMENT_ENFORCEMENT")
+        self.activate_spork(self.minerAPos, "SPORK_9_MASTERNODE_BUDGET_ENFORCEMENT")
+        self.activate_spork(self.minerAPos, "SPORK_13_ENABLE_SUPERBLOCKS")
+
+
+if __name__ == '__main__':
+    GovernanceInvalidBudgetTest().main()