From 9222f57a35ee749ba18287605a47c200d0690ff5 Mon Sep 17 00:00:00 2001 From: furszy Date: Sun, 20 Jun 2021 11:05:39 -0300 Subject: [PATCH 1/4] Check budget payment tx validity on the finalization with the highest amount of votes only. --- src/budget/budgetmanager.cpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/budget/budgetmanager.cpp b/src/budget/budgetmanager.cpp index 9fb1be8bd98fd..59754e9261c34 100644 --- a/src/budget/budgetmanager.cpp +++ b/src/budget/budgetmanager.cpp @@ -615,19 +615,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__); } } From 4975951fa23d4cb922289d125b2a71e0d556ce84 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 21 Jun 2021 18:33:44 -0300 Subject: [PATCH 2/4] Only accept finalization votes from enabled MNs. --- src/budget/budgetmanager.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/budget/budgetmanager.cpp b/src/budget/budgetmanager.cpp index 59754e9261c34..77b2345d90791 100644 --- a/src/budget/budgetmanager.cpp +++ b/src/budget/budgetmanager.cpp @@ -1026,7 +1026,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)); From eceec6b1b0cc86de5858273ec4e83d67960e8690 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 21 Jun 2021 22:23:45 -0300 Subject: [PATCH 3/4] [Test] Add test coverage for invalid budget finalization. --- src/rpc/budget.cpp | 90 ++++++++- test/functional/test_runner.py | 1 + .../tiertwo_governance_invalid_budget.py | 172 ++++++++++++++++++ 3 files changed, 262 insertions(+), 1 deletion(-) create mode 100755 test/functional/tiertwo_governance_invalid_budget.py diff --git a/src/rpc/budget.cpp b/src/rpc/budget.cpp index 071a894a6b3b0..66be677c2ac32 100644 --- a/src/rpc/budget.cpp +++ b/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 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 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 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 } }; diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 35270af321f5b..560431fcc6e22 100755 --- a/test/functional/test_runner.py +++ b/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 = [ diff --git a/test/functional/tiertwo_governance_invalid_budget.py b/test/functional/tiertwo_governance_invalid_budget.py new file mode 100755 index 0000000000000..80b3ea5d93684 --- /dev/null +++ b/test/functional/tiertwo_governance_invalid_budget.py @@ -0,0 +1,172 @@ +#!/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"] == "fin_budget_sent") + budgetFinHash = res["id"] + assert (budgetFinHash != "") + time.sleep(1) + + self.log.info("Voting for invalid budget finalization...") + self.minerA.mnfinalbudget("vote-many", budgetFinHash) + self.stake_and_ping(self.minerAPos, 2, [self.mn1, self.mn2]) + budFin = self.minerB.mnfinalbudget("show") + budget = budFin[next(iter(budFin))] + assert_equal(budget["VoteCount"], 2) + + # 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() \ No newline at end of file From 29e11d1c2206c182edea3c5007b374f405428607 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 21 Jun 2021 22:57:40 -0300 Subject: [PATCH 4/4] Budget finalization, compare budget payments with existent proposals. --- src/budget/budgetmanager.cpp | 27 ++++++++++++++++--- src/budget/budgetmanager.h | 4 +-- .../tiertwo_governance_invalid_budget.py | 12 ++------- 3 files changed, 27 insertions(+), 16 deletions(-) diff --git a/src/budget/budgetmanager.cpp b/src/budget/budgetmanager.cpp index 77b2345d90791..aff42f7e37c6c 100644 --- a/src/budget/budgetmanager.cpp +++ b/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 vBudget = GetBudget(); + std::map 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); @@ -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(); @@ -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) { diff --git a/src/budget/budgetmanager.h b/src/budget/budgetmanager.h index 04e775502bd7d..5477fccf47216 100644 --- a/src/budget/budgetmanager.h +++ b/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); diff --git a/test/functional/tiertwo_governance_invalid_budget.py b/test/functional/tiertwo_governance_invalid_budget.py index 80b3ea5d93684..acaceb35035fb 100755 --- a/test/functional/tiertwo_governance_invalid_budget.py +++ b/test/functional/tiertwo_governance_invalid_budget.py @@ -84,17 +84,9 @@ def run_test(self): 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"] == "fin_budget_sent") - budgetFinHash = res["id"] - assert (budgetFinHash != "") - time.sleep(1) + assert(res["result"] == "error") # not accepted - self.log.info("Voting for invalid budget finalization...") - self.minerA.mnfinalbudget("vote-many", budgetFinHash) - self.stake_and_ping(self.minerAPos, 2, [self.mn1, self.mn2]) - budFin = self.minerB.mnfinalbudget("show") - budget = budFin[next(iter(budFin))] - assert_equal(budget["VoteCount"], 2) + self.log.info("Good, invalid budget not accepted.") # Stake up until the block before the superblock. skip_blocks = next_superblock - self.minerA.getblockcount() - 1