From f5f8bd9df4b54baabeba391c30ef75d929f3b40d Mon Sep 17 00:00:00 2001 From: random-zebra Date: Tue, 22 Dec 2020 14:59:30 +0100 Subject: [PATCH 1/6] Consensus: enforce proposal max payments (6 main-net / 20 test-net) --- src/budget/budgetproposal.cpp | 7 +++++++ src/chainparams.cpp | 2 ++ src/consensus/params.h | 1 + src/rpc/budget.cpp | 6 ++++++ 4 files changed, 16 insertions(+) diff --git a/src/budget/budgetproposal.cpp b/src/budget/budgetproposal.cpp index f04e2cc9b45d..9073a6a6bf02 100644 --- a/src/budget/budgetproposal.cpp +++ b/src/budget/budgetproposal.cpp @@ -100,6 +100,13 @@ bool CBudgetProposal::CheckStartEnd() return false; } + // !TODO: remove (and alwyas use new rules) when all proposals submitted before v5 enforcement are expired. + bool fNewRules = Params().GetConsensus().NetworkUpgradeActive(nBlockStart, Consensus::UPGRADE_V5_0); + if (fNewRules && GetTotalPaymentCount() > Params().GetConsensus().nMaxProposalPayments) { + strInvalid = "Invalid payment count"; + return false; + } + return true; } diff --git a/src/chainparams.cpp b/src/chainparams.cpp index 3bf95f645e82..ff7cecef0fec 100644 --- a/src/chainparams.cpp +++ b/src/chainparams.cpp @@ -147,6 +147,7 @@ class CMainParams : public CChainParams consensus.nTargetTimespanV2 = 30 * 60; consensus.nTargetSpacing = 1 * 60; consensus.nTimeSlotLength = 15; + consensus.nMaxProposalPayments = 6; // spork keys consensus.strSporkPubKey = "0410050aa740d280b134b40b40658781fc1116ba7700764e0ce27af3e1737586b3257d19232e0cb5084947f5107e44bcd577f126c9eb4a30ea2807b271d2145298"; @@ -281,6 +282,7 @@ class CTestNetParams : public CMainParams consensus.nTargetTimespanV2 = 30 * 60; consensus.nTargetSpacing = 1 * 60; consensus.nTimeSlotLength = 15; + consensus.nMaxProposalPayments = 20; // spork keys consensus.strSporkPubKey = "04677c34726c491117265f4b1c83cef085684f36c8df5a97a3a42fc499316d0c4e63959c9eca0dba239d9aaaf72011afffeb3ef9f51b9017811dec686e412eb504"; diff --git a/src/consensus/params.h b/src/consensus/params.h index 9ca97cb809f6..591d91e7416e 100644 --- a/src/consensus/params.h +++ b/src/consensus/params.h @@ -105,6 +105,7 @@ struct Params { int64_t nTargetTimespanV2; int64_t nTargetSpacing; int nTimeSlotLength; + int nMaxProposalPayments; // spork keys std::string strSporkPubKey; diff --git a/src/rpc/budget.cpp b/src/rpc/budget.cpp index 9eebc048e571..66e488d3b576 100644 --- a/src/rpc/budget.cpp +++ b/src/rpc/budget.cpp @@ -64,6 +64,12 @@ void checkBudgetInputs(const UniValue& params, std::string &strProposalName, std if (nPaymentCount < 1) throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid payment count, must be more than zero."); + int nMaxPayments = Params().GetConsensus().nMaxProposalPayments; + if (nPaymentCount > nMaxPayments) { + throw JSONRPCError(RPC_INVALID_PARAMETER, + strprintf("Invalid payment count, must be <= %d", nMaxPayments)); + } + CBlockIndex* pindexPrev = GetChainTip(); if (!pindexPrev) throw JSONRPCError(RPC_IN_WARMUP, "Try again after active chain is loaded"); From 2b53142ee20a697b90f2cdf4ad396f1a9a1c1f24 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Tue, 22 Dec 2020 15:42:01 +0100 Subject: [PATCH 2/6] Consensus: enforce that proposal BlockStart must be a superblock --- src/budget/budgetproposal.cpp | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/budget/budgetproposal.cpp b/src/budget/budgetproposal.cpp index 9073a6a6bf02..0ee4914963a3 100644 --- a/src/budget/budgetproposal.cpp +++ b/src/budget/budgetproposal.cpp @@ -8,6 +8,7 @@ #include "masternodeman.h" CBudgetProposal::CBudgetProposal(): + nAlloted(0), fValid(true), strInvalid(""), strProposalName("unknown"), @@ -27,6 +28,7 @@ CBudgetProposal::CBudgetProposal(const std::string& name, const CAmount& amount, int blockstart, const uint256& nfeetxhash): + nAlloted(0), fValid(true), strInvalid(""), strProposalName(name), @@ -38,14 +40,10 @@ CBudgetProposal::CBudgetProposal(const std::string& name, nTime(0) { const int nBlocksPerCycle = Params().GetConsensus().nBudgetCycleBlocks; + // !todo: remove this when v5 rules are enforced (nBlockStart is always = to nCycleStart) int nCycleStart = nBlockStart - nBlockStart % nBlocksPerCycle; - // Right now single payment proposals have nBlockEnd have a cycle too early! - // switch back if it break something else - // calculate the end of the cycle for this vote, add half a cycle (vote will be deleted after that block) - // nBlockEnd = nCycleStart + GetBudgetPaymentCycleBlocks() * nPaymentCount + GetBudgetPaymentCycleBlocks() / 2; - - // Calculate the end of the cycle for this vote, vote will be deleted after next cycle + // calculate the expiration block nBlockEnd = nCycleStart + (nBlocksPerCycle + 1) * paycount; } @@ -90,8 +88,13 @@ bool CBudgetProposal::IsHeavilyDownvoted(bool fNewRules) bool CBudgetProposal::CheckStartEnd() { - if (nBlockStart < 0) { - strInvalid = "Invalid Proposal"; + // !TODO: remove (and always use new rules) when all proposals submitted before v5 enforcement are expired. + bool fNewRules = Params().GetConsensus().NetworkUpgradeActive(nBlockStart, Consensus::UPGRADE_V5_0); + + if (nBlockStart < 0 || + // block start must be a superblock + (fNewRules && (nBlockStart % Params().GetConsensus().nBudgetCycleBlocks) != 0)) { + strInvalid = "Invalid nBlockStart"; return false; } @@ -100,8 +103,6 @@ bool CBudgetProposal::CheckStartEnd() return false; } - // !TODO: remove (and alwyas use new rules) when all proposals submitted before v5 enforcement are expired. - bool fNewRules = Params().GetConsensus().NetworkUpgradeActive(nBlockStart, Consensus::UPGRADE_V5_0); if (fNewRules && GetTotalPaymentCount() > Params().GetConsensus().nMaxProposalPayments) { strInvalid = "Invalid payment count"; return false; From 3aaa51049a28cd691afe85dec09874d161872106 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Tue, 22 Dec 2020 16:28:38 +0100 Subject: [PATCH 3/6] Tests: check invalid RPC inputs in rpc_budget test --- test/functional/rpc_budget.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/test/functional/rpc_budget.py b/test/functional/rpc_budget.py index a430c727832f..0d88298b00d4 100755 --- a/test/functional/rpc_budget.py +++ b/test/functional/rpc_budget.py @@ -36,6 +36,10 @@ def run_test(self): assert_raises_rpc_error(-8, "Invalid payment count, must be more than zero.", self.nodes[0].preparebudget, name, scheme + url, 0, nextsuperblock, address, cycleamount) + self.log.info("Test with invalid (21) cycles") + assert_raises_rpc_error(-8, "Invalid payment count, must be <= 20", self.nodes[0].preparebudget, + name, scheme + url, 21, nextsuperblock, address, cycleamount) + self.log.info("Test with invalid block start") assert_raises_rpc_error(-8, "Invalid block start", self.nodes[0].preparebudget, name, scheme + url, numcycles, nextsuperblock - 12, address, cycleamount) @@ -47,8 +51,9 @@ def run_test(self): name, scheme + url, numcycles, nextsuperblock, "DBREvBPNQguwuC4YMoCG5FoH1sA2YntvZm", cycleamount) self.log.info("Test with too low amount") - assert_raises_rpc_error(-8, "Invalid amount - Payment of 9.00 is less than minimum 10 PIV allowed", self.nodes[0].preparebudget, - name, scheme + url, numcycles, nextsuperblock, address, 9) + invalid_amt = 9.99999999 + assert_raises_rpc_error(-8, "Invalid amount - Payment of %.8f is less than minimum 10 PIV allowed" % invalid_amt, self.nodes[0].preparebudget, + name, scheme + url, numcycles, nextsuperblock, address, invalid_amt) self.log.info("Test with too high amount") assert_raises_rpc_error(-8, "Invalid amount - Payment of 648001.00 more than max of 648000.00", self.nodes[0].preparebudget, From a005fa71f9a9ae7aafb6bf92562eabc8ad3da230 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Thu, 24 Dec 2020 02:47:45 +0100 Subject: [PATCH 4/6] Tests: check getbudgetinfo / getbudgetprojection updated output --- .../tiertwo_governance_sync_basic.py | 54 ++++++++++++++++++- 1 file changed, 53 insertions(+), 1 deletion(-) diff --git a/test/functional/tiertwo_governance_sync_basic.py b/test/functional/tiertwo_governance_sync_basic.py index cc52827fbbac..07982bdc0456 100755 --- a/test/functional/tiertwo_governance_sync_basic.py +++ b/test/functional/tiertwo_governance_sync_basic.py @@ -7,6 +7,7 @@ from test_framework.util import ( assert_equal, assert_true, + Decimal, ) import time @@ -60,6 +61,39 @@ def check_vote_existence(self, proposalName, mnCollateralHash, voteType): found = True assert_true(found, "Error checking vote existence in node " + str(i)) + def get_proposal_obj(self, Name, URL, Hash, FeeHash, BlockStart, BlockEnd, + TotalPaymentCount, RemainingPaymentCount, PaymentAddress, + Ratio, Yeas, Nays, Abstains, TotalPayment, MonthlyPayment, + IsEstablished, IsValid, Allotted, TotalBudgetAllotted, IsInvalidReason = ""): + obj = {} + obj["Name"] = Name + obj["URL"] = URL + obj["Hash"] = Hash + obj["FeeHash"] = FeeHash + obj["BlockStart"] = BlockStart + obj["BlockEnd"] = BlockEnd + obj["TotalPaymentCount"] = TotalPaymentCount + obj["RemainingPaymentCount"] = RemainingPaymentCount + obj["PaymentAddress"] = PaymentAddress + obj["Ratio"] = Ratio + obj["Yeas"] = Yeas + obj["Nays"] = Nays + obj["Abstains"] = Abstains + obj["TotalPayment"] = TotalPayment + obj["MonthlyPayment"] = MonthlyPayment + obj["IsEstablished"] = IsEstablished + obj["IsValid"] = IsValid + if IsInvalidReason != "": + obj["IsInvalidReason"] = IsInvalidReason + obj["Alloted"] = Allotted + obj["TotalBudgetAlloted"] = TotalBudgetAllotted + return obj + + def check_budgetprojection(self, expected): + for i in range(self.num_nodes): + assert_equal(self.nodes[i].getbudgetprojection(), expected) + self.log.info("Budget projection valid for node %d" % i) + def run_test(self): self.enable_mocktime() self.setup_2_masternodes_network() @@ -106,7 +140,7 @@ def run_test(self): # let's wait a little bit and see if all nodes are sync time.sleep(1) self.check_proposal_existence(firstProposalName, proposalHash) - self.log.info("proposal broadcast succeed!") + self.log.info("proposal broadcast successful!") # Proposal is established after 5 minutes. Mine 7 blocks # Proposal needs to be on the chain > 5 min. @@ -131,6 +165,20 @@ def run_test(self): self.check_vote_existence(firstProposalName, self.mnTwoTxHash, "YES") self.log.info("all good, MN2 vote accepted everywhere!") + # Now check the budget + blockStart = nextSuperBlockHeight + blockEnd = blockStart + firstProposalCycles * 145 + TotalPayment = firstProposalAmountPerCycle * firstProposalCycles + Allotted = firstProposalAmountPerCycle + RemainingPaymentCount = firstProposalCycles + expected_budget = [ + self.get_proposal_obj(firstProposalName, firstProposalLink, proposalHash, proposalFeeTxId, blockStart, + blockEnd, firstProposalCycles, RemainingPaymentCount, firstProposalAddress, 1, + 2, 0, 0, Decimal(str(TotalPayment)), Decimal(str(firstProposalAmountPerCycle)), + True, True, Decimal(str(Allotted)), Decimal(str(Allotted))) + ] + self.check_budgetprojection(expected_budget) + # Quick block count check. assert_equal(self.ownerOne.getblockcount(), 276) @@ -164,6 +212,10 @@ def run_test(self): self.log.info("budget proposal paid!, all good") + # Check that the proposal info returns updated payment count + expected_budget[0]["RemainingPaymentCount"] -= 1 + self.check_budgetprojection(expected_budget) + From 72abccfc07836b23ec981287a080df4ecc11b3de Mon Sep 17 00:00:00 2001 From: random-zebra Date: Thu, 24 Dec 2020 03:55:58 +0100 Subject: [PATCH 5/6] scripted-diff: Fix "alloted" typo -BEGIN VERIFY SCRIPT- sed -i 's/alloted/allotted/g' src/*/*.h src/*/*.cpp test/functional/*.py ; sed -i 's/Alloted/Allotted/g' src/*/*.h src/*/*.cpp test/functional/*.py ; -END VERIFY SCRIPT- --- src/budget/budgetproposal.cpp | 4 ++-- src/budget/budgetproposal.h | 6 +++--- src/rpc/budget.cpp | 8 ++++---- test/functional/tiertwo_governance_sync_basic.py | 4 ++-- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/budget/budgetproposal.cpp b/src/budget/budgetproposal.cpp index 0ee4914963a3..ab77c665bf03 100644 --- a/src/budget/budgetproposal.cpp +++ b/src/budget/budgetproposal.cpp @@ -8,7 +8,7 @@ #include "masternodeman.h" CBudgetProposal::CBudgetProposal(): - nAlloted(0), + nAllotted(0), fValid(true), strInvalid(""), strProposalName("unknown"), @@ -28,7 +28,7 @@ CBudgetProposal::CBudgetProposal(const std::string& name, const CAmount& amount, int blockstart, const uint256& nfeetxhash): - nAlloted(0), + nAllotted(0), fValid(true), strInvalid(""), strProposalName(name), diff --git a/src/budget/budgetproposal.h b/src/budget/budgetproposal.h index 2f1824925c31..1754d3f245a4 100644 --- a/src/budget/budgetproposal.h +++ b/src/budget/budgetproposal.h @@ -22,7 +22,7 @@ static const int64_t BUDGET_VOTE_UPDATE_MIN = 60 * 60; class CBudgetProposal { private: - CAmount nAlloted; + CAmount nAllotted; bool fValid; std::string strInvalid; @@ -87,8 +87,8 @@ class CBudgetProposal int GetNays() const { return GetVoteCount(CBudgetVote::VOTE_NO); } int GetAbstains() const { return GetVoteCount(CBudgetVote::VOTE_ABSTAIN); }; CAmount GetAmount() const { return nAmount; } - void SetAllotted(CAmount nAllotedIn) { nAlloted = nAllotedIn; } - CAmount GetAllotted() const { return nAlloted; } + void SetAllotted(CAmount nAllottedIn) { nAllotted = nAllottedIn; } + CAmount GetAllotted() const { return nAllotted; } void CleanAndRemove(); diff --git a/src/rpc/budget.cpp b/src/rpc/budget.cpp index 66e488d3b576..9bd8d81f491f 100644 --- a/src/rpc/budget.cpp +++ b/src/rpc/budget.cpp @@ -45,7 +45,7 @@ void budgetToJSON(const CBudgetProposal* pbudgetProposal, UniValue& bObj, int nC bObj.pushKV("IsValid", fValid); if (!fValid) bObj.pushKV("IsInvalidReason", pbudgetProposal->IsInvalidReason()); - bObj.pushKV("Alloted", ValueFromAmount(pbudgetProposal->GetAllotted())); + bObj.pushKV("Allotted", ValueFromAmount(pbudgetProposal->GetAllotted())); } void checkBudgetInputs(const UniValue& params, std::string &strProposalName, std::string &strURL, @@ -544,8 +544,8 @@ UniValue getbudgetprojection(const JSONRPCRequest& request) " \"IsEstablished\": true|false, (boolean) Established (true) or (false)\n" " \"IsValid\": true|false, (boolean) Valid (true) or Invalid (false)\n" " \"IsInvalidReason\": \"xxxx\", (string) Error message, if any\n" - " \"Alloted\": xxx.xxx, (numeric) Amount alloted in current period\n" - " \"TotalBudgetAlloted\": xxx.xxx (numeric) Total alloted\n" + " \"Allotted\": xxx.xxx, (numeric) Amount allotted in current period\n" + " \"TotalBudgetAllotted\": xxx.xxx (numeric) Total allotted\n" " }\n" " ,...\n" "]\n" @@ -562,7 +562,7 @@ UniValue getbudgetprojection(const JSONRPCRequest& request) UniValue bObj(UniValue::VOBJ); budgetToJSON(&p, bObj, g_budgetman.GetBestHeight()); nTotalAllotted += p.GetAllotted(); - bObj.pushKV("TotalBudgetAlloted", ValueFromAmount(nTotalAllotted)); + bObj.pushKV("TotalBudgetAllotted", ValueFromAmount(nTotalAllotted)); ret.push_back(bObj); } diff --git a/test/functional/tiertwo_governance_sync_basic.py b/test/functional/tiertwo_governance_sync_basic.py index 07982bdc0456..4adc1eaf0309 100755 --- a/test/functional/tiertwo_governance_sync_basic.py +++ b/test/functional/tiertwo_governance_sync_basic.py @@ -85,8 +85,8 @@ def get_proposal_obj(self, Name, URL, Hash, FeeHash, BlockStart, BlockEnd, obj["IsValid"] = IsValid if IsInvalidReason != "": obj["IsInvalidReason"] = IsInvalidReason - obj["Alloted"] = Allotted - obj["TotalBudgetAlloted"] = TotalBudgetAllotted + obj["Allotted"] = Allotted + obj["TotalBudgetAllotted"] = TotalBudgetAllotted return obj def check_budgetprojection(self, expected): From bdfaaed277981dce9618b7a335edd744780aebaf Mon Sep 17 00:00:00 2001 From: random-zebra Date: Tue, 22 Dec 2020 17:15:53 +0100 Subject: [PATCH 6/6] [BUG] Fix total budget on testnet the cycle is 144 blocks, not 146. Use the available GetBlockValue/nBudgetCycleBlocks, instead of hardcoding the constants. Note: GetTotalBudget should consider the possibility of changes to the block value, between consecutive superblocks. Since the chain already passed the last reduction, and the block value is fixed at 5, we can leave this as is for now. --- src/budget/budgetmanager.cpp | 44 ++++------------------------------- src/test/budget_tests.cpp | 3 ++- test/functional/rpc_budget.py | 5 ++-- 3 files changed, 9 insertions(+), 43 deletions(-) diff --git a/src/budget/budgetmanager.cpp b/src/budget/budgetmanager.cpp index 87228855a354..eec014627d5d 100644 --- a/src/budget/budgetmanager.cpp +++ b/src/budget/budgetmanager.cpp @@ -749,47 +749,11 @@ std::string CBudgetManager::GetRequiredPaymentsString(int nBlockHeight) CAmount CBudgetManager::GetTotalBudget(int nHeight) { - if (Params().NetworkID() == CBaseChainParams::TESTNET) { - CAmount nSubsidy = 500 * COIN; - return ((nSubsidy / 100) * 10) * 146; - } - - //get block value and calculate from that - CAmount nSubsidy = 0; - const Consensus::Params& consensus = Params().GetConsensus(); - const bool isPoSActive = consensus.NetworkUpgradeActive(nHeight, Consensus::UPGRADE_POS); - if (nHeight >= 151200 && !isPoSActive) { - nSubsidy = 50 * COIN; - } else if (isPoSActive && nHeight <= 302399) { - nSubsidy = 50 * COIN; - } else if (nHeight <= 345599 && nHeight >= 302400) { - nSubsidy = 45 * COIN; - } else if (nHeight <= 388799 && nHeight >= 345600) { - nSubsidy = 40 * COIN; - } else if (nHeight <= 431999 && nHeight >= 388800) { - nSubsidy = 35 * COIN; - } else if (nHeight <= 475199 && nHeight >= 432000) { - nSubsidy = 30 * COIN; - } else if (nHeight <= 518399 && nHeight >= 475200) { - nSubsidy = 25 * COIN; - } else if (nHeight <= 561599 && nHeight >= 518400) { - nSubsidy = 20 * COIN; - } else if (nHeight <= 604799 && nHeight >= 561600) { - nSubsidy = 15 * COIN; - } else if (nHeight <= 647999 && nHeight >= 604800) { - nSubsidy = 10 * COIN; - } else if (consensus.NetworkUpgradeActive(nHeight, Consensus::UPGRADE_ZC_V2)) { - nSubsidy = 10 * COIN; - } else { - nSubsidy = 5 * COIN; - } + // 20% of the block value + CAmount nSubsidy = GetBlockValue(nHeight) / 5; - // Amount of blocks in a months period of time (using 1 minutes per) = (60*24*30) - if (nHeight <= 172800) { - return 648000 * COIN; - } else { - return ((nSubsidy / 100) * 10) * 1440 * 30; - } + // multiplied by the number of blocks in a cycle (144 on testnet, 30*1440 on mainnet) + return nSubsidy * Params().GetConsensus().nBudgetCycleBlocks; } void CBudgetManager::AddSeenProposalVote(const CBudgetVote& vote) diff --git a/src/test/budget_tests.cpp b/src/test/budget_tests.cpp index b5bfcf3baf85..d62051286f99 100644 --- a/src/test/budget_tests.cpp +++ b/src/test/budget_tests.cpp @@ -23,7 +23,8 @@ BOOST_AUTO_TEST_CASE(budget_value) { SelectParams(CBaseChainParams::TESTNET); int nHeightTest = Params().GetConsensus().vUpgrades[Consensus::UPGRADE_ZC_V2].nActivationHeight + 1; - CheckBudgetValue(nHeightTest, "testnet", 7300*COIN); + CheckBudgetValue(nHeightTest-1, "testnet", 7200*COIN); + CheckBudgetValue(nHeightTest, "testnet", 144*COIN); SelectParams(CBaseChainParams::MAIN); nHeightTest = Params().GetConsensus().vUpgrades[Consensus::UPGRADE_ZC_V2].nActivationHeight + 1; diff --git a/test/functional/rpc_budget.py b/test/functional/rpc_budget.py index 0d88298b00d4..93e87c923edd 100755 --- a/test/functional/rpc_budget.py +++ b/test/functional/rpc_budget.py @@ -56,8 +56,9 @@ def run_test(self): name, scheme + url, numcycles, nextsuperblock, address, invalid_amt) self.log.info("Test with too high amount") - assert_raises_rpc_error(-8, "Invalid amount - Payment of 648001.00 more than max of 648000.00", self.nodes[0].preparebudget, - name, scheme + url, numcycles, nextsuperblock, address, 648001) + invalid_amt = 50 * 144 + 0.00000001 + assert_raises_rpc_error(-8, "Invalid amount - Payment of %.8f more than max of 7200.00" % invalid_amt, self.nodes[0].preparebudget, + name, scheme + url, numcycles, nextsuperblock, address, invalid_amt) self.log.info("Test without URL scheme")