Skip to content

Commit

Permalink
Merge #2584: [Refactor] Reorder mnw processing and validation
Browse files Browse the repository at this point in the history 
e8559fc [Refactor] make AddWinningMasternode a void function (random-zebra)
5aaa02a [Test] add mnwinner case for non-existent MN. (furszy)
e68773b [Refactor] Fix checks order for mnw processing (random-zebra)

Pull request description:

  Extracted from #2421 (and based on top of it) so #2550 can be merged in between.

ACKs for top commit:
  furszy:
    utACK e8559fc after rebase

Tree-SHA512: 659d4ca43502f8e1e8f6ddbf7c815f6a07a69625f00c8ab71b7d8e9c97b85e90d97b18ccc988dcfd4d345d5f79932128abdaa0f0634380be7d715d3a3e45c6fc
  • Loading branch information
@furszy
furszy committed Nov 8, 2021
2 parents 02a0060 + e8559fc commit c5ef662
Show file tree
Hide file tree
Showing 3 changed files with 71 additions and 64 deletions.
100 changes: 53 additions & 47 deletions src/masternode-payments.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,14 +166,7 @@ bool CMasternodePaymentWinner::IsValid(CNode* pnode, CValidationState& state, in
{
int n = mnodeman.GetMasternodeRank(vinMasternode, nBlockHeight - 100);
if (n < 1 || n > MNPAYMENTS_SIGNATURES_TOTAL) {
//It's common to have masternodes mistakenly think they are in the top 10
// We don't want to print all of these messages, or punish them unless they're way off
std::string strError = strprintf("Masternode not in the top %d (%d)", MNPAYMENTS_SIGNATURES_TOTAL, n);
if (n > MNPAYMENTS_SIGNATURES_TOTAL * 2) {
LogPrint(BCLog::MASTERNODE,"CMasternodePaymentWinner::IsValid - %s\n", strError);
//if (masternodeSync.IsSynced()) Misbehaving(pnode->GetId(), 20);
}
return state.Error(strError);
return state.Error(strprintf("Masternode not in the top %d (%d)", MNPAYMENTS_SIGNATURES_TOTAL, n));
}

// Must be a P2PKH
Expand Down Expand Up @@ -474,48 +467,54 @@ bool CMasternodePayments::ProcessMNWinner(CMasternodePaymentWinner& winner, CNod
return state.Error("mnw old message version");
}

// See if the mnw signer exists, and whether it's a legacy or DMN masternode
const CMasternode* pmn{nullptr};
auto dmn = deterministicMNManager->GetListAtChainTip().GetMNByCollateral(winner.vinMasternode.prevout);
if (dmn == nullptr) {
// legacy masternode
pmn = mnodeman.Find(winner.vinMasternode.prevout);
if (pmn == nullptr) {
// it could be a non-synced masternode. ask for the mnb
LogPrint(BCLog::MASTERNODE, "mnw - unknown masternode %s\n", winner.vinMasternode.prevout.hash.ToString());
// Only ask for missing items after the initial syncing process is complete
// otherwise will think a full sync succeeded when they return a result
if (pfrom && masternodeSync.IsSynced()) mnodeman.AskForMN(pfrom, winner.vinMasternode);
return state.Error("Non-existent mnwinner voter");
}
}
// either deterministic or legacy. not both
assert((dmn && !pmn) || (!dmn && pmn));

// See if the masternode is in the quorum (top-MNPAYMENTS_SIGNATURES_TOTAL)
if (!winner.IsValid(pfrom, state, nHeight)) {
// error cause set internally
return false;
}

// See if this masternode has already voted for this block height
if (!CanVote(winner.vinMasternode.prevout, winner.nBlockHeight)) {
return state.Error("MN already voted");
}

// See if this winner was signed with a dmn or a legacy masternode
bool is_valid_sig = false;
auto mnList = deterministicMNManager->GetListAtChainTip();
auto dmn = mnList.GetMNByCollateral(winner.vinMasternode.prevout);
if (dmn) {
// DMN: Check BLS signature
is_valid_sig = winner.CheckSignature(dmn->pdmnState->pubKeyOperator.Get());
} else {
// Legacy masternode
const CMasternode* pmn = mnodeman.Find(winner.vinMasternode.prevout);
if (pmn == nullptr) {
// it could be a non-synced masternode. ask for the mnb
LogPrint(BCLog::MASTERNODE, "mnw - unknown masternode %s\n", winner.vinMasternode.prevout.hash.ToString());
// Only ask for missing items after the initial syncing process is complete
// otherwise will think a full sync succeeded when they return a result
if (pfrom && masternodeSync.IsSynced()) mnodeman.AskForMN(pfrom, winner.vinMasternode);
return state.Error("Invalid voter or voter mnwinner signature");
}
is_valid_sig = winner.CheckSignature(pmn->pubKeyMasternode.GetID());
}
// Check signature
bool is_valid_sig = dmn ? winner.CheckSignature(dmn->pdmnState->pubKeyOperator.Get())
: winner.CheckSignature(pmn->pubKeyMasternode.GetID());

if (!is_valid_sig) {
LogPrint(BCLog::MASTERNODE, "%s : mnw - invalid signature\n", __func__);
LogPrint(BCLog::MASTERNODE, "%s : mnw - invalid signature for %s masternode: %s\n",
__func__, (dmn ? "deterministic" : "legacy"), winner.vinMasternode.prevout.hash.ToString());
if (pfrom) {
LOCK(cs_main);
Misbehaving(pfrom->GetId(), 20);
}
return state.Error("invalid voter mnwinner signature");
}

if (!AddWinningMasternode(winner)) {
return state.Error("Failed to add mnwinner"); // move state inside AddWinningMasternode
}
// Record vote
RecordWinnerVote(winner.vinMasternode.prevout, winner.nBlockHeight);

// Add winner
AddWinningMasternode(winner);

// Relay only if we are synchronized.
// Makes no sense to relay MNWinners to the peers from where we are syncing them.
Expand Down Expand Up @@ -560,20 +559,11 @@ bool CMasternodePayments::IsScheduled(const CMasternode& mn, int nNotBlockHeight
return false;
}

bool CMasternodePayments::AddWinningMasternode(CMasternodePaymentWinner& winnerIn)
void CMasternodePayments::AddWinningMasternode(CMasternodePaymentWinner& winnerIn)
{
// check winner height
if (winnerIn.nBlockHeight - 100 > mnodeman.GetBestHeight() + 1) {
return error("%s: mnw - invalid height %d > %d", __func__, winnerIn.nBlockHeight - 100, mnodeman.GetBestHeight() + 1);
}

{
LOCK2(cs_mapMasternodePayeeVotes, cs_mapMasternodeBlocks);

if (mapMasternodePayeeVotes.count(winnerIn.GetHash())) {
return false;
}

mapMasternodePayeeVotes[winnerIn.GetHash()] = winnerIn;

if (!mapMasternodeBlocks.count(winnerIn.nBlockHeight)) {
Expand All @@ -586,8 +576,6 @@ bool CMasternodePayments::AddWinningMasternode(CMasternodePaymentWinner& winnerI
ExtractDestination(winnerIn.payee, addr);
LogPrint(BCLog::MASTERNODE, "mnw - Adding winner %s for block %d\n", EncodeDestination(addr), winnerIn.nBlockHeight);
mapMasternodeBlocks[winnerIn.nBlockHeight].AddPayee(winnerIn.payee, 1);

return true;
}

bool CMasternodeBlockPayees::IsTransactionValid(const CTransaction& txNew)
Expand Down Expand Up @@ -765,6 +753,12 @@ void CMasternodePayments::ProcessBlock(int nBlockHeight)
return;
}

// check winner height
if (nBlockHeight - 100 > mnodeman.GetBestHeight() + 1) {
LogPrintf("%s: mnw - invalid height %d > %d", __func__, nBlockHeight - 100, mnodeman.GetBestHeight() + 1);
return;
}

// pay to the oldest MN that still had no payment but its input is old enough and it was active long enough
int nCount = 0;
MasternodeRef pmn = mnodeman.GetNextMasternodeInQueueForPayment(nBlockHeight, true, nCount);
Expand All @@ -789,9 +783,8 @@ void CMasternodePayments::ProcessBlock(int nBlockHeight)
return;
}
}
if (!AddWinningMasternode(newWinner)) {
return;
}

AddWinningMasternode(newWinner);
newWinner.Relay();
LogPrintf("%s: Relayed winner %s\n", __func__, newWinner.GetHash().ToString());
nLastBlockHeight = nBlockHeight;
Expand Down Expand Up @@ -827,6 +820,19 @@ std::string CMasternodePayments::ToString() const
return info.str();
}

bool CMasternodePayments::CanVote(const COutPoint& outMasternode, int nBlockHeight) const
{
LOCK(cs_mapMasternodePayeeVotes);
const auto it = mapMasternodesLastVote.find(outMasternode);
return it == mapMasternodesLastVote.end() || it->second != nBlockHeight;
}

void CMasternodePayments::RecordWinnerVote(const COutPoint& outMasternode, int nBlockHeight)
{
LOCK(cs_mapMasternodePayeeVotes);
mapMasternodesLastVote[outMasternode] = nBlockHeight;
}

bool IsCoinbaseValueValid(const CTransactionRef& tx, CAmount nBudgetAmt, CValidationState& _state)
{
assert(tx->IsCoinBase());
Expand Down
25 changes: 8 additions & 17 deletions src/masternode-payments.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,6 @@ class CMasternodePayments : public CValidationInterface
public:
std::map<uint256, CMasternodePaymentWinner> mapMasternodePayeeVotes;
std::map<int, CMasternodeBlockPayees> mapMasternodeBlocks;
std::map<COutPoint, int> mapMasternodesLastVote; //prevout, nBlockHeight

CMasternodePayments()
{
Expand All @@ -227,7 +226,7 @@ class CMasternodePayments : public CValidationInterface

void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) override;

bool AddWinningMasternode(CMasternodePaymentWinner& winner);
void AddWinningMasternode(CMasternodePaymentWinner& winner);
void ProcessBlock(int nBlockHeight);

void Sync(CNode* node, int nCountNeeded);
Expand All @@ -243,28 +242,20 @@ class CMasternodePayments : public CValidationInterface
bool IsTransactionValid(const CTransaction& txNew, const CBlockIndex* pindexPrev);
bool IsScheduled(const CMasternode& mn, int nNotBlockHeight);

bool CanVote(const COutPoint& outMasternode, int nBlockHeight)
{
LOCK(cs_mapMasternodePayeeVotes);

if (mapMasternodesLastVote.count(outMasternode)) {
if (mapMasternodesLastVote[outMasternode] == nBlockHeight) {
return false;
}
}

//record this masternode voted
mapMasternodesLastVote[outMasternode] = nBlockHeight;
return true;
}

bool ProcessMNWinner(CMasternodePaymentWinner& winner, CNode* pfrom, CValidationState& state);
void ProcessMessageMasternodePayments(CNode* pfrom, std::string& strCommand, CDataStream& vRecv);
std::string GetRequiredPaymentsString(int nBlockHeight);
void FillBlockPayee(CMutableTransaction& txCoinbase, CMutableTransaction& txCoinstake, const CBlockIndex* pindexPrev, bool fProofOfStake) const;
std::string ToString() const;

SERIALIZE_METHODS(CMasternodePayments, obj) { READWRITE(obj.mapMasternodePayeeVotes, obj.mapMasternodeBlocks); }

private:
// keep track of last voted height for mnw signers
std::map<COutPoint, int> mapMasternodesLastVote; //prevout, nBlockHeight

bool CanVote(const COutPoint& outMasternode, int nBlockHeight) const;
void RecordWinnerVote(const COutPoint& outMasternode, int nBlockHeight);
};


Expand Down
10 changes: 10 additions & 0 deletions src/test/mnpayments_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,16 @@ BOOST_FIXTURE_TEST_CASE(mnwinner_test, TestChain100Setup)
std::vector<FakeMasternode> mnList = buildMNList(tipBlock.GetHash(), tipBlock.GetBlockTime(), 40);
std::vector<std::pair<int64_t, MasternodeRef>> mnRank = mnodeman.GetMasternodeRanks(nextBlockHeight - 100);

// Test mnwinner failure for non-existent MN voter.
CTxIn dummyVoter;
CScript dummyPayeeScript;
CKey dummyKey;
dummyKey.MakeNewKey(true);
CValidationState state0;
BOOST_CHECK(!CreateMNWinnerPayment(dummyVoter, nextBlockHeight, dummyPayeeScript,
dummyKey, dummyKey.GetPubKey(), state0));
BOOST_CHECK_MESSAGE(findStrError(state0, "Non-existent mnwinner voter"), state0.GetRejectReason());

// Take the first MN
auto firstMN = findMNData(mnList, mnRank[0].second);
CTxIn mnVinVoter(firstMN.mn.vin);
Expand Down

0 comments on commit c5ef662

Please sign in to comment.