From 7310fc435750085939ec77c6458a34fb949a2d2d Mon Sep 17 00:00:00 2001 From: Alessandro Rezzi Date: Wed, 29 Mar 2023 09:07:35 +0200 Subject: [PATCH 1/3] Enable multisig proposal after v6 --- src/budget/budgetmanager.cpp | 5 ++--- src/budget/budgetproposal.cpp | 12 ++++++------ src/budget/budgetproposal.h | 4 ++-- src/qt/pivx/governancemodel.cpp | 2 +- src/rpc/budget.cpp | 2 +- 5 files changed, 12 insertions(+), 13 deletions(-) diff --git a/src/budget/budgetmanager.cpp b/src/budget/budgetmanager.cpp index 9cf51d3b9133b..19a0c4b462a4a 100644 --- a/src/budget/budgetmanager.cpp +++ b/src/budget/budgetmanager.cpp @@ -324,19 +324,18 @@ bool CBudgetManager::AddProposal(CBudgetProposal& budgetProposal) { AssertLockNotHeld(cs_proposals); // need to lock cs_main here (CheckCollateral) const uint256& nHash = budgetProposal.GetHash(); - + int nCurrentHeight = GetBestHeight(); if (WITH_LOCK(cs_proposals, return mapProposals.count(nHash))) { LogPrint(BCLog::MNBUDGET,"%s: proposal %s already added\n", __func__, nHash.ToString()); return false; } - if (!budgetProposal.IsWellFormed(GetTotalBudget(budgetProposal.GetBlockStart()))) { + if (!budgetProposal.IsWellFormed(GetTotalBudget(budgetProposal.GetBlockStart()), nCurrentHeight)) { LogPrint(BCLog::MNBUDGET,"%s: Invalid budget proposal %s %s\n", __func__, nHash.ToString(), budgetProposal.IsInvalidLogStr()); return false; } std::string strError; - int nCurrentHeight = GetBestHeight(); const uint256& feeTxId = budgetProposal.GetFeeTXHash(); if (!CheckCollateral(feeTxId, nHash, strError, budgetProposal.nTime, nCurrentHeight, false)) { LogPrint(BCLog::MNBUDGET,"%s: invalid budget proposal (%s) collateral id=%s - %s\n", diff --git a/src/budget/budgetproposal.cpp b/src/budget/budgetproposal.cpp index eae6e4cdc4638..69e35a28adf7d 100644 --- a/src/budget/budgetproposal.cpp +++ b/src/budget/budgetproposal.cpp @@ -123,11 +123,11 @@ bool CBudgetProposal::CheckAmount(const CAmount& nTotalBudget) return true; } -bool CBudgetProposal::CheckAddress() +bool CBudgetProposal::CheckAddress(const int nCurrentHeight) { - // !TODO: There might be an issue with multisig in the coinbase on mainnet - // we will add support for it in a future release. - if (address.IsPayToScriptHash()) { + // Multisig is supported only after v6 + bool isV6Enforced = Params().GetConsensus().NetworkUpgradeActive(nCurrentHeight, Consensus::UPGRADE_V6_0); + if (!isV6Enforced && address.IsPayToScriptHash()) { strInvalid = "Multisig is not currently supported."; return false; } @@ -160,9 +160,9 @@ bool CBudgetProposal::CheckStrings() } } -bool CBudgetProposal::IsWellFormed(const CAmount& nTotalBudget) +bool CBudgetProposal::IsWellFormed(const CAmount& nTotalBudget, const int nCurrentHeight) { - return CheckStartEnd() && CheckAmount(nTotalBudget) && CheckAddress(); + return CheckStartEnd() && CheckAmount(nTotalBudget) && CheckAddress(nCurrentHeight); } bool CBudgetProposal::updateExpired(int nCurrentHeight) diff --git a/src/budget/budgetproposal.h b/src/budget/budgetproposal.h index 1766a9e1564d3..cdae5fe38cb4f 100644 --- a/src/budget/budgetproposal.h +++ b/src/budget/budgetproposal.h @@ -41,7 +41,7 @@ class CBudgetProposal bool updateExpired(int nCurrentHeight); bool CheckStartEnd(); bool CheckAmount(const CAmount& nTotalBudget); - bool CheckAddress(); + bool CheckAddress(const int nCurrentHeight); bool CheckStrings(); protected: @@ -71,7 +71,7 @@ class CBudgetProposal // sets fValid and strInvalid, returns fValid bool UpdateValid(int nHeight, int mnCount); // Static checks that should be done only once - sets strInvalid - bool IsWellFormed(const CAmount& nTotalBudget); + bool IsWellFormed(const CAmount& nTotalBudget, const int nCurrentHeight); bool IsValid() const { return fValid; } void SetStrInvalid(const std::string& _strInvalid) { strInvalid = _strInvalid; } std::string IsInvalidReason() const { return strInvalid; } diff --git a/src/qt/pivx/governancemodel.cpp b/src/qt/pivx/governancemodel.cpp index 9c209927fea99..67dcaf67fc076 100644 --- a/src/qt/pivx/governancemodel.cpp +++ b/src/qt/pivx/governancemodel.cpp @@ -245,7 +245,7 @@ OperationResult GovernanceModel::createProposal(const std::string& strProposalNa // Validate proposal CBudgetProposal proposal(strProposalName, strURL, nPaymentCount, scriptPubKey, nAmount, nBlockStart, UINT256_ZERO); - if (!proposal.IsWellFormed(g_budgetman.GetTotalBudget(proposal.GetBlockStart()))) { + if (!proposal.IsWellFormed(g_budgetman.GetTotalBudget(proposal.GetBlockStart()), clientModel->getNumBlocks())) { return {false, strprintf(_("Proposal is not valid %s"), proposal.IsInvalidReason())}; } diff --git a/src/rpc/budget.cpp b/src/rpc/budget.cpp index c1b2323871350..dea8ec1179815 100644 --- a/src/rpc/budget.cpp +++ b/src/rpc/budget.cpp @@ -147,7 +147,7 @@ UniValue preparebudget(const JSONRPCRequest& request) // create transaction 15 minutes into the future, to allow for confirmation time CBudgetProposal proposal(strProposalName, strURL, nPaymentCount, scriptPubKey, nAmount, nBlockStart, UINT256_ZERO); const uint256& nHash = proposal.GetHash(); - if (!proposal.IsWellFormed(g_budgetman.GetTotalBudget(proposal.GetBlockStart()))) + if (!proposal.IsWellFormed(g_budgetman.GetTotalBudget(proposal.GetBlockStart()), GetChainTip()->nHeight)) throw std::runtime_error("Proposal is not valid " + proposal.IsInvalidReason()); CTransactionRef wtx; From 4df4ca82d49f27bca34e2b7d202b1823db735ac9 Mon Sep 17 00:00:00 2001 From: Alessandro Rezzi Date: Wed, 29 Mar 2023 23:15:20 +0200 Subject: [PATCH 2/3] Updated functional test to cover P2SH proposals --- .../tiertwo_governance_sync_basic.py | 62 ++++++++++++++++--- 1 file changed, 52 insertions(+), 10 deletions(-) diff --git a/test/functional/tiertwo_governance_sync_basic.py b/test/functional/tiertwo_governance_sync_basic.py index 0be6354985d72..e2df8992e1f0e 100755 --- a/test/functional/tiertwo_governance_sync_basic.py +++ b/test/functional/tiertwo_governance_sync_basic.py @@ -59,7 +59,7 @@ def check_budget_finalization_sync(self, votesCount, status): for i in range(0, len(self.nodes)): node = self.nodes[i] budFin = node.mnfinalbudget("show") - assert_true(len(budFin) == 1, "MN budget finalization not synced in node" + str(i)) + assert_true(len(budFin) == 1, "MN budget finalization not synced in node " + str(i)) budget = budFin[next(iter(budFin))] assert_equal(budget["VoteCount"], votesCount) assert_equal(budget["Status"], status) @@ -176,6 +176,15 @@ def submit_proposals(self, props): self.log.info("proposal %s broadcast successful!" % entry.name) return props + def create_multisig_addr(self, node): + addr1 = node.getnewaddress() + addr2 = node.getnewaddress() + addr3 = node.getnewaddress() + sig_address_1 = node.validateaddress(addr1) + sig_address_2 = node.validateaddress(addr2) + sig_address_3 = node.validateaddress(addr3) + return node.addmultisigaddress(2, [sig_address_1['pubkey'], sig_address_2['pubkey'], sig_address_3['pubkey']]) + def run_test(self): self.enable_mocktime() self.setup_3_masternodes_network() @@ -208,6 +217,18 @@ def run_test(self): ) self.submit_proposals([firstProposal]) + multisigAddr = self.create_multisig_addr(self.miner) + # Submit multisig proposal + self.log.info("preparing budget proposal..") + multisigProposal = Proposal( + "we-like-multisig", + "https://forum.pivx.org/t/test-multisig", + 2, + multisigAddr, + 600 + ) + self.submit_proposals([multisigProposal]) + # Create 15 more proposals to have a higher tier two net gossip movement props = [] for i in range(15): @@ -223,17 +244,21 @@ def run_test(self): self.stake(7, [self.remoteOne, self.remoteTwo]) # Check proposals existence for i in range(self.num_nodes): - assert_equal(len(self.nodes[i].getbudgetinfo()), 16) + assert_equal(len(self.nodes[i].getbudgetinfo()), 17) # now let's vote for the proposal with the first MN self.log.info("Voting with MN1...") voteResult = self.ownerOne.mnbudgetvote("alias", firstProposal.proposalHash, "yes", self.masternodeOneAlias, True) assert_equal(voteResult["detail"][0]["result"], "success") + multisigVoteResult = self.ownerOne.mnbudgetvote("alias", multisigProposal.proposalHash, "yes", self.masternodeOneAlias, True) + assert_equal(multisigVoteResult["detail"][0]["result"], "success") + # check that the vote was accepted everywhere self.stake(1, [self.remoteOne, self.remoteTwo]) self.check_vote_existence(firstProposal.name, self.mnOneCollateral.hash, "YES", True) - self.log.info("all good, MN1 vote accepted everywhere!") + self.check_vote_existence(multisigProposal.name, self.mnOneCollateral.hash, "YES", True) + self.log.info("all good, MN1 votes accepted everywhere!") # before broadcast the second vote, let's drop the budget data of ownerOne. # so the node is forced to send a single proposal sync when the, now orphan, proposal vote is received. @@ -247,11 +272,15 @@ def run_test(self): voteResult = self.ownerTwo.mnbudgetvote("alias", firstProposal.proposalHash, "yes", self.masternodeTwoAlias, True) assert_equal(voteResult["detail"][0]["result"], "success") + multisigVoteResult = self.ownerTwo.mnbudgetvote("alias", multisigProposal.proposalHash, "yes", self.masternodeTwoAlias, True) + assert_equal(multisigVoteResult["detail"][0]["result"], "success") # check orphan vote proposal re-sync self.log.info("checking orphan vote based proposal re-sync...") time.sleep(5) # wait a bit before check it self.check_proposal_existence(firstProposal.name, firstProposal.proposalHash) self.check_vote_existence(firstProposal.name, self.mnOneCollateral.hash, "YES", True) + self.check_proposal_existence(multisigProposal.name, multisigProposal.proposalHash) + self.check_vote_existence(multisigProposal.name, self.mnOneCollateral.hash, "YES", True) self.log.info("all good, orphan vote based proposal re-sync succeeded") # check that the vote was accepted everywhere @@ -275,19 +304,28 @@ def run_test(self): TotalPayment = firstProposal.amountPerCycle * firstProposal.cycles Allotted = firstProposal.amountPerCycle RemainingPaymentCount = firstProposal.cycles + + multisigBlockEnd = blockStart + multisigProposal.cycles * 145 + multisigTotalPayment = multisigProposal.amountPerCycle * multisigProposal.cycles + multisigAllotted = multisigProposal.amountPerCycle + multisigRemainingPaymentCount = multisigProposal.cycles expected_budget = [ self.get_proposal_obj(firstProposal.name, firstProposal.link, firstProposal.proposalHash, firstProposal.feeTxId, blockStart, blockEnd, firstProposal.cycles, RemainingPaymentCount, firstProposal.paymentAddr, 1, 3, 0, 0, satoshi_round(TotalPayment), satoshi_round(firstProposal.amountPerCycle), - True, True, satoshi_round(Allotted), satoshi_round(Allotted)) + True, True, satoshi_round(Allotted), satoshi_round(Allotted)), + self.get_proposal_obj(multisigProposal.name, multisigProposal.link, multisigProposal.proposalHash, multisigProposal.feeTxId, blockStart, + multisigBlockEnd, multisigProposal.cycles, multisigRemainingPaymentCount, multisigProposal.paymentAddr, 1, + 2, 0, 0, satoshi_round(multisigTotalPayment), satoshi_round(multisigProposal.amountPerCycle), + True, True, satoshi_round(multisigAllotted), satoshi_round(multisigAllotted + Allotted)), ] self.check_budgetprojection(expected_budget) # Quick block count check. - assert_equal(self.ownerOne.getblockcount(), 279) + assert_equal(self.ownerOne.getblockcount(), 282) self.log.info("starting budget finalization sync test..") - self.stake(2, [self.remoteOne, self.remoteTwo]) + self.stake(1, [self.remoteOne, self.remoteTwo]) # assert that there is no budget finalization first. assert_equal(len(self.ownerOne.mnfinalbudget("show")), 0) @@ -337,10 +375,14 @@ def run_test(self): addrInfo = self.miner.listreceivedbyaddress(0, False, False, firstProposal.paymentAddr) assert_equal(addrInfo[0]["amount"], firstProposal.amountPerCycle) + addrInfo = self.miner.listreceivedbyaddress(0, False, False, multisigProposal.paymentAddr) + assert_equal(addrInfo[0]["amount"], multisigProposal.amountPerCycle) + self.log.info("budget proposal paid!, all good") # Check that the proposal info returns updated payment count expected_budget[0]["RemainingPaymentCount"] -= 1 + expected_budget[1]["RemainingPaymentCount"] -= 1 self.check_budgetprojection(expected_budget) self.stake(1, [self.remoteOne, self.remoteTwo]) @@ -356,7 +398,7 @@ def run_test(self): self.wait_until_mnsync_finished() self.check_budgetprojection(expected_budget) for i in range(self.num_nodes): - assert_equal(len(self.nodes[i].getbudgetinfo()), 16) + assert_equal(len(self.nodes[i].getbudgetinfo()), 17) self.log.info("resync (1): budget data resynchronized successfully!") @@ -377,7 +419,7 @@ def run_test(self): self.log.info("syncing node..") self.wait_until_mnsync_finished() for i in range(self.num_nodes): - assert_equal(len(self.nodes[i].getbudgetinfo()), 16) + assert_equal(len(self.nodes[i].getbudgetinfo()), 17) self.log.info("resync (2): budget data resynchronized successfully!") # Let's now verify the remote budget data relay. @@ -392,7 +434,7 @@ def run_test(self): time.sleep(5) # wait a little bit self.log.info("Checking budget sync..") for i in range(self.num_nodes): - assert_equal(len(self.nodes[i].getbudgetinfo()), 16) + assert_equal(len(self.nodes[i].getbudgetinfo()), 17) self.check_vote_existence(firstProposal.name, self.mnOneCollateral.hash, "YES", True) self.check_vote_existence(firstProposal.name, self.mnTwoCollateral.hash, "YES", True) self.check_vote_existence(firstProposal.name, self.proRegTx1, "YES", True) @@ -424,7 +466,7 @@ def run_test(self): assert_equal(len(self.miner.mnfinalbudget("show")), 1) blocks_to_mine = nextSuperBlockHeight + 200 - self.miner.getblockcount() self.log.info("Mining %d more blocks to check expired budget removal..." % blocks_to_mine) - self.stake(blocks_to_mine - 1, [self.remoteTwo]) + self.stake(blocks_to_mine, [self.remoteTwo]) # finalized budget must still be there self.miner.checkbudgets() assert_equal(len(self.miner.mnfinalbudget("show")), 1) From 11c279c3b7ef05117f5796edac2b056fe18b12c3 Mon Sep 17 00:00:00 2001 From: Alessandro Rezzi Date: Wed, 29 Mar 2023 23:31:14 +0200 Subject: [PATCH 3/3] Add CheckString for proposals --- src/budget/budgetproposal.cpp | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/budget/budgetproposal.cpp b/src/budget/budgetproposal.cpp index 69e35a28adf7d..b100be5042770 100644 --- a/src/budget/budgetproposal.cpp +++ b/src/budget/budgetproposal.cpp @@ -5,6 +5,7 @@ #include "budget/budgetproposal.h" #include "chainparams.h" +#include "logging.h" #include "script/standard.h" #include "utilstrencodings.h" @@ -146,9 +147,6 @@ bool CBudgetProposal::CheckAddress(const int nCurrentHeight) return true; } -/* TODO: Add this to IsWellFormed() for the next hard-fork - * This will networkly reject malformed proposal names and URLs - */ bool CBudgetProposal::CheckStrings() { if (strProposalName != SanitizeString(strProposalName)) { @@ -157,11 +155,17 @@ bool CBudgetProposal::CheckStrings() } if (strURL != SanitizeString(strURL)) { strInvalid = "Proposal URL contains illegal characters."; + return false; } + return true; } bool CBudgetProposal::IsWellFormed(const CAmount& nTotalBudget, const int nCurrentHeight) { + bool isV6Enforced = Params().GetConsensus().NetworkUpgradeActive(nCurrentHeight, Consensus::UPGRADE_V6_0); + if (isV6Enforced && !CheckStrings()) { + return false; + } return CheckStartEnd() && CheckAmount(nTotalBudget) && CheckAddress(nCurrentHeight); }