Skip to content

Commit

Permalink
Merge #2567: [BUG][TierTwo] MNB process refactor
Browse files Browse the repository at this point in the history 
4a67ede ProcessMNB: Unify IsInputAssociatedWithPubkey check inside MNMan::CheckInputs. (furszy)
bcd7f05 ProcessMNB: Only emplace valid mnb in the seen map. (furszy)
8f6cd1a Move CMasternodeBroadcast::AddAndRelayMNB to the Masternode manager class. (furszy)
2d7effd Bugfix: missing error if mnb cannot be added for DMN prevalence. (furszy)
059fd0e Move mnb check inputs to masternode manager. (furszy)
17a3b14 MasternodeMan: Do not access activeMasternode from 'CMasternodeBroadcast:CheckInputsAndAdd' function. (furszy)
aad9139 CMasternode: decouple CheckInputs from CheckInputsAndAdd (furszy)
0c0e4c0 CMasternode::IsInputAssociatedWithPubkey use coins cache instead of disk lookup. (furszy)
100f81f MN sync: unify duplicated sync timeout code in one single function. (furszy)

Pull request description:

  Have refactored the mnb validation process, placing the code where it should had been in the first place (masternode manager and not be an internal masternode function that is called from and internally calls to the manager..).

  Doing it found a bug over the MN-DMN compatibility code:
  if the mnb received is from an active DMN, it needs to not be relayed to the network as it's an invalid mnb that no node will accept. Active DMNs have prevalence over regular MNs.

  And improved an un-performant, on disk, tx look up to check the mnb pubkey association with the collateral key. Use the coins cache instead.

ACKs for top commit:
  Fuzzbawls:
    Code ACK 4a67ede
  random-zebra:
    utACK 4a67ede

Tree-SHA512: 58af66effa51177a771e984003d833783516fd9d22e2b745f4296475bde21b4cf07301739b055630091967cefb5ad7ac6f9b0b9b0869f945200baefad571a015
  • Loading branch information
@random-zebra
random-zebra committed Oct 11, 2021
2 parents f70c72d + 4a67ede commit ccb2a43
Show file tree
Hide file tree
Showing 6 changed files with 117 additions and 116 deletions.
21 changes: 11 additions & 10 deletions src/masternode-sync.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,15 @@ void CMasternodeSync::Process()
});
}

void CMasternodeSync::syncTimeout(const std::string& reason)
{
LogPrintf("%s - ERROR - Sync has failed on %s, will retry later\n", __func__, reason);
RequestedMasternodeAssets = MASTERNODE_SYNC_FAILED;
RequestedMasternodeAttempt = 0;
lastFailure = GetTime();
nCountFailures++;
}

bool CMasternodeSync::SyncWithNode(CNode* pnode, bool fLegacyMnObsolete)
{
CNetMsgMaker msgMaker(pnode->GetSendVersion());
Expand Down Expand Up @@ -352,11 +361,7 @@ bool CMasternodeSync::SyncWithNode(CNode* pnode, bool fLegacyMnObsolete)
if (lastMasternodeList == 0 &&
(RequestedMasternodeAttempt >= MASTERNODE_SYNC_THRESHOLD * 3 || GetTime() - nAssetSyncStarted > MASTERNODE_SYNC_TIMEOUT * 5)) {
if (sporkManager.IsSporkActive(SPORK_8_MASTERNODE_PAYMENT_ENFORCEMENT)) {
LogPrintf("CMasternodeSync::Process - ERROR - Sync has failed on %s, will retry later\n", "MASTERNODE_SYNC_LIST");
RequestedMasternodeAssets = MASTERNODE_SYNC_FAILED;
RequestedMasternodeAttempt = 0;
lastFailure = GetTime();
nCountFailures++;
syncTimeout("MASTERNODE_SYNC_LIST");
} else {
SwitchToNextAsset();
}
Expand Down Expand Up @@ -389,11 +394,7 @@ bool CMasternodeSync::SyncWithNode(CNode* pnode, bool fLegacyMnObsolete)
if (lastMasternodeWinner == 0 &&
(RequestedMasternodeAttempt >= MASTERNODE_SYNC_THRESHOLD * 3 || GetTime() - nAssetSyncStarted > MASTERNODE_SYNC_TIMEOUT * 5)) {
if (sporkManager.IsSporkActive(SPORK_8_MASTERNODE_PAYMENT_ENFORCEMENT)) {
LogPrintf("CMasternodeSync::Process - ERROR - Sync has failed on %s, will retry later\n", "MASTERNODE_SYNC_MNW");
RequestedMasternodeAssets = MASTERNODE_SYNC_FAILED;
RequestedMasternodeAttempt = 0;
lastFailure = GetTime();
nCountFailures++;
syncTimeout("MASTERNODE_SYNC_MNW");
} else {
SwitchToNextAsset();
}
Expand Down
3 changes: 3 additions & 0 deletions src/masternode-sync.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,9 @@ class CMasternodeSync

// Check if an update is needed
void CheckAndUpdateSyncStatus();

// Mark sync timeout
void syncTimeout(const std::string& reason);
};

#endif
86 changes: 0 additions & 86 deletions src/masternode.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -214,24 +214,6 @@ bool CMasternode::IsValidNetAddr() const
(IsReachable(addr) && addr.IsRoutable());
}

bool CMasternode::IsInputAssociatedWithPubkey() const
{
CScript payee;
payee = GetScriptForDestination(pubKeyCollateralAddress.GetID());

CTransactionRef txVin;
uint256 hash;
if(GetTransaction(vin.prevout.hash, txVin, hash, true)) {
for (const CTxOut& out : txVin->vout) {
if (out.nValue == Params().GetConsensus().nMNCollateralAmt &&
out.scriptPubKey == payee)
return true;
}
}

return false;
}

CMasternodeBroadcast::CMasternodeBroadcast() :
CMasternode()
{ }
Expand Down Expand Up @@ -486,74 +468,6 @@ bool CMasternodeBroadcast::CheckAndUpdate(int& nDos, int nChainHeight)
return true;
}

bool CMasternodeBroadcast::CheckInputsAndAdd(int nChainHeight, int& nDoS)
{
// we are a masternode with the same vin (i.e. already activated) and this mnb is ours (matches our Masternode privkey)
// so nothing to do here for us
if (fMasterNode && activeMasternode.vin != nullopt &&
vin.prevout == activeMasternode.vin->prevout &&
pubKeyMasternode == activeMasternode.pubKeyMasternode &&
activeMasternode.GetStatus() == ACTIVE_MASTERNODE_STARTED) {
return true;
}

// incorrect ping or its sigTime
if(lastPing.IsNull() || !lastPing.CheckAndUpdate(nDoS, nChainHeight, false, true)) {
return false;
}

// search existing Masternode list
CMasternode* pmn = mnodeman.Find(vin.prevout);
if (pmn != NULL) {
// nothing to do here if we already know about this masternode and it's enabled
if (pmn->IsEnabled()) return true;
// if it's not enabled, remove old MN first and continue
else
mnodeman.Remove(pmn->vin.prevout);
}

const Coin& collateralUtxo = pcoinsTip->AccessCoin(vin.prevout);
if (collateralUtxo.IsSpent()) {
LogPrint(BCLog::MASTERNODE,"mnb - vin %s spent\n", vin.prevout.ToString());
return false;
}

LogPrint(BCLog::MASTERNODE, "mnb - Accepted Masternode entry\n");
const int utxoHeight = collateralUtxo.nHeight;
int collateralUtxoDepth = nChainHeight - utxoHeight + 1;
if (collateralUtxoDepth < MasternodeCollateralMinConf()) {
LogPrint(BCLog::MASTERNODE,"mnb - Input must have at least %d confirmations\n", MasternodeCollateralMinConf());
// maybe we miss few blocks, let this mnb to be checked again later
mnodeman.mapSeenMasternodeBroadcast.erase(GetHash());
masternodeSync.mapSeenSyncMNB.erase(GetHash());
return false;
}

// verify that sig time is legit in past
// should be at least not earlier than block when 1000 PIV tx got MASTERNODE_MIN_CONFIRMATIONS
CBlockIndex* pConfIndex = WITH_LOCK(cs_main, return chainActive[utxoHeight + MasternodeCollateralMinConf() - 1]); // block where tx got MASTERNODE_MIN_CONFIRMATIONS
if (pConfIndex->GetBlockTime() > sigTime) {
LogPrint(BCLog::MASTERNODE,"mnb - Bad sigTime %d for Masternode %s (%i conf block is at %d)\n",
sigTime, vin.prevout.hash.ToString(), MasternodeCollateralMinConf(), pConfIndex->GetBlockTime());
return false;
}

LogPrint(BCLog::MASTERNODE,"mnb - Got NEW Masternode entry - %s - %lli \n", vin.prevout.hash.ToString(), sigTime);
CMasternode mn(*this);
mnodeman.Add(mn);

// if it matches our Masternode privkey, then we've been remotely activated
if (pubKeyMasternode == activeMasternode.pubKeyMasternode && protocolVersion == PROTOCOL_VERSION) {
activeMasternode.EnableHotColdMasterNode(vin, addr);
}

bool isLocal = (addr.IsRFC1918() || addr.IsLocal()) && !Params().IsRegTestNet();

if (!isLocal) Relay();

return true;
}

void CMasternodeBroadcast::Relay()
{
CInv inv(MSG_MASTERNODE_ANNOUNCE, GetHash());
Expand Down
4 changes: 0 additions & 4 deletions src/masternode.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -222,9 +222,6 @@ class CMasternode : public CSignedMessage

bool IsValidNetAddr() const;

/// Is the input associated with collateral public key? (and there is 10000 PIV - checking if valid masternode)
bool IsInputAssociatedWithPubkey() const;

/*
* This is used only by the compatibility code for DMN, which don't share the public key (but the keyid).
* Used by the payment-logic to include the necessary information in a temporary MasternodeRef object
Expand Down Expand Up @@ -253,7 +250,6 @@ class CMasternodeBroadcast : public CMasternode
CMasternodeBroadcast(const CMasternode& mn);

bool CheckAndUpdate(int& nDoS, int nChainHeight);
bool CheckInputsAndAdd(int chainHeight, int& nDos);

uint256 GetHash() const;

Expand Down
116 changes: 100 additions & 16 deletions src/masternodeman.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -731,6 +731,68 @@ std::vector<std::pair<int64_t, MasternodeRef>> CMasternodeMan::GetMasternodeRank
return vecMasternodeScores;
}

bool CMasternodeMan::CheckInputs(CMasternodeBroadcast& mnb, int nChainHeight, int& nDoS)
{
// incorrect ping or its sigTime
if(mnb.lastPing.IsNull() || !mnb.lastPing.CheckAndUpdate(nDoS, nChainHeight, false, true)) {
return false;
}

// search existing Masternode list
CMasternode* pmn = Find(mnb.vin.prevout);
if (pmn != nullptr) {
// nothing to do here if we already know about this masternode and it's enabled
if (pmn->IsEnabled()) return true;
// if it's not enabled, remove old MN first and continue
else
mnodeman.Remove(pmn->vin.prevout);
}

const Coin& collateralUtxo = pcoinsTip->AccessCoin(mnb.vin.prevout);
if (collateralUtxo.IsSpent()) {
LogPrint(BCLog::MASTERNODE,"mnb - vin %s spent\n", mnb.vin.prevout.ToString());
return false;
}

// Check collateral value
if (collateralUtxo.out.nValue != Params().GetConsensus().nMNCollateralAmt) {
LogPrint(BCLog::MASTERNODE,"mnb - invalid amount for mnb collateral %s\n", mnb.vin.prevout.ToString());
nDoS = 33;
return false;
}

// Check collateral association with mnb pubkey
CScript payee = GetScriptForDestination(mnb.pubKeyCollateralAddress.GetID());
if (collateralUtxo.out.scriptPubKey != payee) {
LogPrint(BCLog::MASTERNODE,"mnb - collateral %s not associated with mnb pubkey\n", mnb.vin.prevout.ToString());
nDoS = 33;
return false;
}

LogPrint(BCLog::MASTERNODE, "mnb - Accepted Masternode entry\n");
const int utxoHeight = (int) collateralUtxo.nHeight;
int collateralUtxoDepth = nChainHeight - utxoHeight + 1;
if (collateralUtxoDepth < MasternodeCollateralMinConf()) {
LogPrint(BCLog::MASTERNODE,"mnb - Input must have at least %d confirmations\n", MasternodeCollateralMinConf());
// maybe we miss few blocks, let this mnb to be checked again later
mapSeenMasternodeBroadcast.erase(mnb.GetHash());
masternodeSync.mapSeenSyncMNB.erase(mnb.GetHash());
return false;
}

// verify that sig time is legit in past
// should be at least not earlier than block when 1000 PIV tx got MASTERNODE_MIN_CONFIRMATIONS
CBlockIndex* pConfIndex = WITH_LOCK(cs_main, return chainActive[utxoHeight + MasternodeCollateralMinConf() - 1]); // block where tx got MASTERNODE_MIN_CONFIRMATIONS
if (pConfIndex->GetBlockTime() > mnb.sigTime) {
LogPrint(BCLog::MASTERNODE,"mnb - Bad sigTime %d for Masternode %s (%i conf block is at %d)\n",
mnb.sigTime, mnb.vin.prevout.hash.ToString(), MasternodeCollateralMinConf(), pConfIndex->GetBlockTime());
return false;
}

// Good input
return true;
}

int CMasternodeMan::ProcessMNBroadcast(CNode* pfrom, CMasternodeBroadcast& mnb)
{
const uint256& mnbHash = mnb.GetHash();
Expand All @@ -745,26 +807,48 @@ int CMasternodeMan::ProcessMNBroadcast(CNode* pfrom, CMasternodeBroadcast& mnb)
return nDoS;
}

// make sure the vout that was signed is related to the transaction that spawned the Masternode
// - this is expensive, so it's only done once per Masternode
if (!mnb.IsInputAssociatedWithPubkey()) {
LogPrint(BCLog::MASTERNODE, "%s : mnb - Got mismatched pubkey and vin\n", __func__);
return 33;
// If we are a masternode with the same vin (i.e. already activated) and this mnb is ours (matches our Masternode pubkey)
// nothing to do here for us
if (fMasterNode && activeMasternode.vin != nullopt &&
mnb.vin.prevout == activeMasternode.vin->prevout &&
mnb.pubKeyMasternode == activeMasternode.pubKeyMasternode &&
activeMasternode.GetStatus() == ACTIVE_MASTERNODE_STARTED) {
mapSeenMasternodeBroadcast.emplace(mnbHash, mnb);
return 0;
}

// now that did the basic mnb checks, can add it.
// make sure it's still unspent
if (!CheckInputs(mnb, chainHeight, nDoS)) {
return nDoS; // error set internally
}

// now that did the mnb checks, can add it.
mapSeenMasternodeBroadcast.emplace(mnbHash, mnb);

// make sure it's still unspent
// - this is checked later by .check() in many places and by ThreadCheckObfuScationPool()
if (mnb.CheckInputsAndAdd(chainHeight, nDoS)) {
// use this as a peer
g_connman->AddNewAddress(CAddress(mnb.addr, NODE_NETWORK), pfrom->addr, 2 * 60 * 60);
masternodeSync.AddedMasternodeList(mnbHash);
} else {
LogPrint(BCLog::MASTERNODE,"mnb - Rejected Masternode entry %s\n", mnb.vin.prevout.hash.ToString());
return nDoS;
// All checks performed, add it
LogPrint(BCLog::MASTERNODE,"%s - Got NEW Masternode entry - %s - %lli \n", __func__,
mnb.vin.prevout.hash.ToString(), mnb.sigTime);
CMasternode mn(mnb);
if (!Add(mn)) {
LogPrint(BCLog::MASTERNODE, "%s - Rejected Masternode entry %s\n", __func__,
mnb.vin.prevout.hash.ToString());
return 0;
}

// if it matches our MN pubkey, then we've been remotely activated
if (mnb.pubKeyMasternode == activeMasternode.pubKeyMasternode && mnb.protocolVersion == PROTOCOL_VERSION) {
activeMasternode.EnableHotColdMasterNode(mnb.vin, mnb.addr);
}
// Relay
bool isLocal = (mnb.addr.IsRFC1918() || mnb.addr.IsLocal()) && !Params().IsRegTestNet();
if (!isLocal) mnb.Relay();

// Add it as a peer
g_connman->AddNewAddress(CAddress(mnb.addr, NODE_NETWORK), pfrom->addr, 2 * 60 * 60);

// Update sync status
masternodeSync.AddedMasternodeList(mnbHash);

// All good
return 0;
}
Expand Down Expand Up @@ -970,7 +1054,7 @@ int64_t CMasternodeMan::GetLastPaid(const MasternodeRef& mn, int count_enabled,
CHashWriter ss(SER_GETHASH, PROTOCOL_VERSION);
ss << mn->vin;
ss << mn->sigTime;
uint256 hash = ss.GetHash();
const uint256& hash = ss.GetHash();

// use a deterministic offset to break a tie -- 2.5 minutes
int64_t nOffset = UintToArith256(hash).GetCompact(false) % 150;
Expand Down
3 changes: 3 additions & 0 deletions src/masternodeman.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,9 @@ class CMasternodeMan
// Relay a MN
void BroadcastInvMN(CMasternode* mn, CNode* pfrom);

// Validation
bool CheckInputs(CMasternodeBroadcast& mnb, int nChainHeight, int& nDoS);

public:
// Keep track of all broadcasts I've seen
std::map<uint256, CMasternodeBroadcast> mapSeenMasternodeBroadcast;
Expand Down

0 comments on commit ccb2a43

Please sign in to comment.