From 7420ef77911956e170035988c6368d502ec112a9 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Tue, 2 Feb 2021 18:40:48 +0100 Subject: [PATCH 1/4] [RPC] fix vote hashes and masternode collateral outpoints - nHash is supposed to be the vote hash, not the outpoint hash - mnId is the collateral outpoint (not just the outpoint.hash) --- src/budget/budgetvote.cpp | 4 ++-- src/budget/finalizedbudgetvote.cpp | 2 +- src/rpc/budget.cpp | 2 +- test/functional/tiertwo_governance_sync_basic.py | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/budget/budgetvote.cpp b/src/budget/budgetvote.cpp index e0a7c7a21bbe..28014959bd42 100644 --- a/src/budget/budgetvote.cpp +++ b/src/budget/budgetvote.cpp @@ -53,8 +53,8 @@ std::string CBudgetVote::GetStrMessage() const UniValue CBudgetVote::ToJSON() const { UniValue bObj(UniValue::VOBJ); - bObj.pushKV("mnId", vin.prevout.hash.ToString()); - bObj.pushKV("nHash", vin.prevout.GetHash().ToString()); + bObj.pushKV("mnId", vin.prevout.ToStringShort()); + bObj.pushKV("nHash", GetHash().ToString()); bObj.pushKV("Vote", GetVoteString()); bObj.pushKV("nTime", nTime); bObj.pushKV("fValid", fValid); diff --git a/src/budget/finalizedbudgetvote.cpp b/src/budget/finalizedbudgetvote.cpp index bee7a6645179..3fdb634f3042 100644 --- a/src/budget/finalizedbudgetvote.cpp +++ b/src/budget/finalizedbudgetvote.cpp @@ -44,7 +44,7 @@ uint256 CFinalizedBudgetVote::GetHash() const UniValue CFinalizedBudgetVote::ToJSON() const { UniValue bObj(UniValue::VOBJ); - bObj.pushKV("nHash", vin.prevout.GetHash().ToString()); + bObj.pushKV("nHash", GetHash().ToString()); bObj.pushKV("nTime", (int64_t) nTime); bObj.pushKV("fValid", fValid); return bObj; diff --git a/src/rpc/budget.cpp b/src/rpc/budget.cpp index eb71826afaef..55abe32ef26b 100644 --- a/src/rpc/budget.cpp +++ b/src/rpc/budget.cpp @@ -481,7 +481,7 @@ UniValue getbudgetvotes(const JSONRPCRequest& request) "\nResult:\n" "[\n" " {\n" - " \"mnId\": \"xxxx\", (string) Hash of the masternode's collateral transaction\n" + " \"mnId\": \"xxxx-x\", (string) Masternode's outpoint collateral transaction (hash-n)\n" " \"nHash\": \"xxxx\", (string) Hash of the vote\n" " \"Vote\": \"YES|NO\", (string) Vote cast ('YES' or 'NO')\n" " \"nTime\": xxxx, (numeric) Time in seconds since epoch the vote was cast\n" diff --git a/test/functional/tiertwo_governance_sync_basic.py b/test/functional/tiertwo_governance_sync_basic.py index 4adc1eaf0309..3ff5412cfa93 100755 --- a/test/functional/tiertwo_governance_sync_basic.py +++ b/test/functional/tiertwo_governance_sync_basic.py @@ -56,7 +56,7 @@ def check_vote_existence(self, proposalName, mnCollateralHash, voteType): assert(len(votesInfo) > 0) found = False for voteInfo in votesInfo: - if (voteInfo["mnId"] == mnCollateralHash) : + if (voteInfo["mnId"].split("-")[0] == mnCollateralHash) : assert_equal(voteInfo["Vote"], voteType) found = True assert_true(found, "Error checking vote existence in node " + str(i)) From 715c9ad72f6ab599394b7b72fd371007aa385ea0 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Tue, 2 Feb 2021 18:59:35 +0100 Subject: [PATCH 2/4] [BUG] Fix CBudgetProposal/CFinalizedBudget::GetVotesHashes These functions are supposed to return a vector of vote-hashes. They instead return a vector of mnIds(collateral)-hashes. --- src/budget/budgetproposal.cpp | 2 +- src/budget/finalizedbudget.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/budget/budgetproposal.cpp b/src/budget/budgetproposal.cpp index ab77c665bf03..e99c305155e3 100644 --- a/src/budget/budgetproposal.cpp +++ b/src/budget/budgetproposal.cpp @@ -306,7 +306,7 @@ std::vector CBudgetProposal::GetVotesHashes() const { std::vector vRet; for (const auto& it: mapVotes) { - vRet.push_back(it.first); + vRet.push_back(it.second.GetHash()); } return vRet; } diff --git a/src/budget/finalizedbudget.cpp b/src/budget/finalizedbudget.cpp index 60256d46adf7..164de9860a73 100644 --- a/src/budget/finalizedbudget.cpp +++ b/src/budget/finalizedbudget.cpp @@ -289,7 +289,7 @@ std::vector CFinalizedBudget::GetVotesHashes() const { std::vector vRet; for (const auto& it: mapVotes) { - vRet.push_back(it.first); + vRet.push_back(it.second.GetHash()); } return vRet; } From 14670743d16d7bfe05ed10d7c468bfa0eab207a1 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Tue, 2 Feb 2021 19:08:42 +0100 Subject: [PATCH 3/4] [Refactoring] Index mapVotes by mnIds (outpoints) instead of using the hash of the whole outpoint object. --- src/budget/budgetproposal.cpp | 12 ++++++------ src/budget/budgetproposal.h | 2 +- src/budget/finalizedbudget.cpp | 12 ++++++------ src/budget/finalizedbudget.h | 2 +- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/budget/budgetproposal.cpp b/src/budget/budgetproposal.cpp index e99c305155e3..b961b589aed3 100644 --- a/src/budget/budgetproposal.cpp +++ b/src/budget/budgetproposal.cpp @@ -215,11 +215,11 @@ bool CBudgetProposal::IsPassing(int nBlockStartBudget, int nBlockEndBudget, int bool CBudgetProposal::AddOrUpdateVote(const CBudgetVote& vote, std::string& strError) { std::string strAction = "New vote inserted:"; - const uint256& hash = vote.GetVin().prevout.GetHash(); + const COutPoint& mnId = vote.GetVin().prevout; const int64_t voteTime = vote.GetTime(); - if (mapVotes.count(hash)) { - const int64_t& oldTime = mapVotes[hash].GetTime(); + if (mapVotes.count(mnId)) { + const int64_t& oldTime = mapVotes[mnId].GetTime(); if (oldTime > voteTime) { strError = strprintf("new vote older than existing vote - %s\n", vote.GetHash().ToString()); LogPrint(BCLog::MNBUDGET, "%s: %s\n", __func__, strError); @@ -240,7 +240,7 @@ bool CBudgetProposal::AddOrUpdateVote(const CBudgetVote& vote, std::string& strE return false; } - mapVotes[hash] = vote; + mapVotes[mnId] = vote; LogPrint(BCLog::MNBUDGET, "%s: %s %s\n", __func__, strAction.c_str(), vote.GetHash().ToString().c_str()); return true; @@ -271,10 +271,10 @@ void CBudgetProposal::SetSynced(bool synced) void CBudgetProposal::CleanAndRemove() { LogPrint(BCLog::MNBUDGET, "Cleaning budget votes for %s. Before: YES=%d, NO=%d\n", GetName(), GetYeas(), GetNays()); - std::map::iterator it = mapVotes.begin(); + auto it = mapVotes.begin(); while (it != mapVotes.end()) { - CMasternode* pmn = mnodeman.Find(it->second.GetVin().prevout); + CMasternode* pmn = mnodeman.Find(it->first); (*it).second.SetValid(pmn != nullptr); ++it; } diff --git a/src/budget/budgetproposal.h b/src/budget/budgetproposal.h index 1754d3f245a4..72205e1076d5 100644 --- a/src/budget/budgetproposal.h +++ b/src/budget/budgetproposal.h @@ -34,7 +34,7 @@ class CBudgetProposal bool CheckAddress(); protected: - std::map mapVotes; + std::map mapVotes; std::string strProposalName; std::string strURL; int nBlockStart; diff --git a/src/budget/finalizedbudget.cpp b/src/budget/finalizedbudget.cpp index 164de9860a73..0c9deacc82d2 100644 --- a/src/budget/finalizedbudget.cpp +++ b/src/budget/finalizedbudget.cpp @@ -53,12 +53,12 @@ bool CFinalizedBudget::ParseBroadcast(CDataStream& broadcast) bool CFinalizedBudget::AddOrUpdateVote(const CFinalizedBudgetVote& vote, std::string& strError) { - const uint256& hash = vote.GetVin().prevout.GetHash(); + const COutPoint& mnId = vote.GetVin().prevout; const int64_t voteTime = vote.GetTime(); std::string strAction = "New vote inserted:"; - if (mapVotes.count(hash)) { - const int64_t oldTime = mapVotes[hash].GetTime(); + if (mapVotes.count(mnId)) { + const int64_t oldTime = mapVotes[mnId].GetTime(); if (oldTime > voteTime) { strError = strprintf("new vote older than existing vote - %s\n", vote.GetHash().ToString()); LogPrint(BCLog::MNBUDGET, "%s: %s\n", __func__, strError); @@ -80,7 +80,7 @@ bool CFinalizedBudget::AddOrUpdateVote(const CFinalizedBudgetVote& vote, std::st return false; } - mapVotes[hash] = vote; + mapVotes[mnId] = vote; LogPrint(BCLog::MNBUDGET, "%s: %s %s\n", __func__, strAction.c_str(), vote.GetHash().ToString().c_str()); return true; } @@ -166,10 +166,10 @@ bool CFinalizedBudget::CheckProposals(const std::map& // Remove votes from masternodes which are not valid/existent anymore void CFinalizedBudget::CleanAndRemove() { - std::map::iterator it = mapVotes.begin(); + auto it = mapVotes.begin(); while (it != mapVotes.end()) { - CMasternode* pmn = mnodeman.Find(it->second.GetVin().prevout); + CMasternode* pmn = mnodeman.Find(it->first); (*it).second.SetValid(pmn != nullptr); ++it; } diff --git a/src/budget/finalizedbudget.h b/src/budget/finalizedbudget.h index 7cc2a4a26970..c17a0532f726 100644 --- a/src/budget/finalizedbudget.h +++ b/src/budget/finalizedbudget.h @@ -40,7 +40,7 @@ class CFinalizedBudget bool CheckName(); protected: - std::map mapVotes; + std::map mapVotes; std::string strBudgetName; int nBlockStart; std::vector vecBudgetPayments; From 11922723e37f8eed676c40d3c50fd05cd702a075 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Tue, 2 Feb 2021 19:09:53 +0100 Subject: [PATCH 4/4] [Cleanup] Remove CBaseOutPoint::GetHash() now unused --- src/primitives/transaction.cpp | 5 ----- src/primitives/transaction.h | 2 -- 2 files changed, 7 deletions(-) diff --git a/src/primitives/transaction.cpp b/src/primitives/transaction.cpp index 31338ac22d14..02c33dc9dfaf 100644 --- a/src/primitives/transaction.cpp +++ b/src/primitives/transaction.cpp @@ -20,11 +20,6 @@ std::string BaseOutPoint::ToStringShort() const return strprintf("%s-%u", hash.ToString().substr(0,64), n); } -uint256 BaseOutPoint::GetHash() const -{ - return Hash(BEGIN(hash), END(hash), BEGIN(n), END(n)); -} - std::string COutPoint::ToString() const { return strprintf("COutPoint(%s, %u)", hash.ToString().substr(0,10), n); diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h index db134b7134ea..cc77d7c21b11 100644 --- a/src/primitives/transaction.h +++ b/src/primitives/transaction.h @@ -74,8 +74,6 @@ class BaseOutPoint size_t DynamicMemoryUsage() const { return 0; } - uint256 GetHash() const; - }; /** An outpoint - a combination of a transaction hash and an index n into its vout */