Skip to content

Commit

Permalink
Merge #2440: [Backport] Align budget payee validity to highest voted …
Browse files Browse the repository at this point in the history
…budget check.

29e11d1 Budget finalization, compare budget payments with existent proposals. (furszy)
eceec6b [Test] Add test coverage for invalid budget finalization. (furszy)
4975951 Only accept finalization votes from enabled MNs. (furszy)
9222f57 Check budget payment tx validity on the finalization with the highest amount of votes only. (furszy)

Pull request description:

  Backports 96a4fad, 18a5bc5 and 21f6d0e into v5.2.

ACKs for top commit:
  random-zebra:
    utACK 29e11d1
  Fuzzbawls:
    ACK 29e11d1

Tree-SHA512: 29afcf8578185119ed18536ee57b68319795db46503d67ddc6e042611f5e455fc77bb7cdc37a95643048ae9b44c49d20351782c77bff0fc71926bf33018b41af
  • Loading branch information
Fuzzbawls committed Jun 22, 2021
2 parents 92139ca + 29e11d1 commit f381577
Show file tree
Hide file tree
Showing 5 changed files with 289 additions and 17 deletions.
47 changes: 33 additions & 14 deletions src/budget/budgetmanager.cpp
Expand Up @@ -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();
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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__);
}
}

Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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) {
Expand Down
4 changes: 2 additions & 2 deletions src/budget/budgetmanager.h
Expand Up @@ -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
Expand All @@ -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);
Expand Down
90 changes: 89 additions & 1 deletion src/rpc/budget.cpp
Expand Up @@ -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;
Expand Down Expand Up @@ -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 }

};

Expand Down
1 change: 1 addition & 0 deletions test/functional/test_runner.py
Expand Up @@ -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 = [
Expand Down
164 changes: 164 additions & 0 deletions 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()

0 comments on commit f381577

Please sign in to comment.