Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make sure to clear setAskFor in Dash submodules #1730

Merged
merged 3 commits into from
Nov 22, 2017
Merged

Make sure to clear setAskFor in Dash submodules #1730

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e38513f
Make sure to clear setAskFor in Dash submodules
UdjinM6 Nov 19, 2017
7a7d2f4
partially revert previous commit
UdjinM6 Nov 20, 2017
858764a
skip mnv until mn list is synced
UdjinM6 Nov 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 13 additions & 12 deletions src/governance.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,20 +148,20 @@ void CGovernanceManager::ProcessMessage(CNode* pfrom, std::string& strCommand, C
{
// MAKE SURE WE HAVE A VALID REFERENCE TO THE TIP BEFORE CONTINUING

CGovernanceObject govobj;
vRecv >> govobj;

uint256 nHash = govobj.GetHash();

pfrom->setAskFor.erase(nHash);

if(!masternodeSync.IsMasternodeListSynced()) {
LogPrint("gobject", "MNGOVERNANCEOBJECT -- masternode list not synced\n");
return;
}

CGovernanceObject govobj;
vRecv >> govobj;

uint256 nHash = govobj.GetHash();
std::string strHash = nHash.ToString();

pfrom->setAskFor.erase(nHash);

LogPrint("gobject", "MNGOVERNANCEOBJECT -- Received object: %s\n", strHash);

if(!AcceptObjectMessage(nHash)) {
Expand Down Expand Up @@ -232,22 +232,23 @@ void CGovernanceManager::ProcessMessage(CNode* pfrom, std::string& strCommand, C
// A NEW GOVERNANCE OBJECT VOTE HAS ARRIVED
else if (strCommand == NetMsgType::MNGOVERNANCEOBJECTVOTE)
{
CGovernanceVote vote;
vRecv >> vote;

uint256 nHash = vote.GetHash();

pfrom->setAskFor.erase(nHash);

// Ignore such messages until masternode list is synced
if(!masternodeSync.IsMasternodeListSynced()) {
LogPrint("gobject", "MNGOVERNANCEOBJECTVOTE -- masternode list not synced\n");
return;
}

CGovernanceVote vote;
vRecv >> vote;

LogPrint("gobject", "MNGOVERNANCEOBJECTVOTE -- Received vote: %s\n", vote.ToString());

uint256 nHash = vote.GetHash();
std::string strHash = nHash.ToString();

pfrom->setAskFor.erase(nHash);

if(!AcceptVoteMessage(nHash)) {
LogPrint("gobject", "MNGOVERNANCEOBJECTVOTE -- Received unrequested vote object: %s, hash: %s, peer = %d\n",
vote.ToString(), strHash, pfrom->GetId());
Expand Down
15 changes: 8 additions & 7 deletions src/instantx.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,6 @@ void CInstantSend::ProcessMessage(CNode* pfrom, std::string& strCommand, CDataSt
if(fLiteMode) return; // disable all Dash specific functionality
if(!sporkManager.IsSporkActive(SPORK_2_INSTANTSEND_ENABLED)) return;

// Ignore any InstantSend messages until masternode list is synced
if(!masternodeSync.IsMasternodeListSynced()) return;

// NOTE: NetMsgType::TXLOCKREQUEST is handled via ProcessMessage() in main.cpp

if (strCommand == NetMsgType::TXLOCKVOTE) // InstantSend Transaction Lock Consensus Votes
Expand All @@ -57,17 +54,21 @@ void CInstantSend::ProcessMessage(CNode* pfrom, std::string& strCommand, CDataSt
CTxLockVote vote;
vRecv >> vote;


uint256 nVoteHash = vote.GetHash();

pfrom->setAskFor.erase(nVoteHash);

// Ignore any InstantSend messages until masternode list is synced
if(!masternodeSync.IsMasternodeListSynced()) return;

LOCK(cs_main);
#ifdef ENABLE_WALLET
if (pwalletMain)
LOCK(pwalletMain->cs_wallet);
#endif
LOCK(cs_instantsend);

uint256 nVoteHash = vote.GetHash();

pfrom->setAskFor.erase(nVoteHash);

if(mapTxLockVotes.count(nVoteHash)) return;
mapTxLockVotes.insert(std::make_pair(nVoteHash, vote));

Expand Down
13 changes: 8 additions & 5 deletions src/masternode-payments.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,9 +305,6 @@ int CMasternodePayments::GetMinMasternodePaymentsProto() {

void CMasternodePayments::ProcessMessage(CNode* pfrom, std::string& strCommand, CDataStream& vRecv, CConnman& connman)
{
// Ignore any payments messages until masternode list is synced
if(!masternodeSync.IsMasternodeListSynced()) return;

if(fLiteMode) return; // disable all Dash specific functionality

if (strCommand == NetMsgType::MASTERNODEPAYMENTSYNC) { //Masternode Payments Request Sync
Expand All @@ -333,15 +330,21 @@ void CMasternodePayments::ProcessMessage(CNode* pfrom, std::string& strCommand,

} else if (strCommand == NetMsgType::MASTERNODEPAYMENTVOTE) { // Masternode Payments Vote for the Winner


CMasternodePaymentVote vote;
vRecv >> vote;

if(pfrom->nVersion < GetMinMasternodePaymentsProto()) return;

uint256 nHash = vote.GetHash();

pfrom->setAskFor.erase(nHash);

// TODO: clear setAskFor for MSG_MASTERNODE_PAYMENT_BLOCK too

if(pfrom->nVersion < GetMinMasternodePaymentsProto()) return;

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not needed because if node is misbehaving by sending wrong peer version let setAskFor be not cleared to avoid DDOS .

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, fixed. As a side note, this should probably be moved to the top of CMasternodePayments::ProcessMessage to avoid serving outdated nodes completely but that's out of the scope for this PR.

// Ignore any payments messages until masternode list is synced
if(!masternodeSync.IsMasternodeListSynced()) return;

{
LOCK(cs_mapMasternodePaymentVotes);
if(mapMasternodePaymentVotes.count(nHash)) {
Expand Down
7 changes: 6 additions & 1 deletion src/masternodeman.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -771,7 +771,6 @@ std::pair<CService, std::set<uint256> > CMasternodeMan::PopScheduledMnbRequestCo
void CMasternodeMan::ProcessMessage(CNode* pfrom, std::string& strCommand, CDataStream& vRecv, CConnman& connman)
{
if(fLiteMode) return; // disable all Dash specific functionality
if(!masternodeSync.IsBlockchainSynced()) return;

if (strCommand == NetMsgType::MNANNOUNCE) { //Masternode Broadcast

Expand All @@ -780,6 +779,8 @@ void CMasternodeMan::ProcessMessage(CNode* pfrom, std::string& strCommand, CData

pfrom->setAskFor.erase(mnb.GetHash());

if(!masternodeSync.IsBlockchainSynced()) return;

LogPrint("masternode", "MNANNOUNCE -- Masternode announce, masternode=%s\n", mnb.vin.prevout.ToStringShort());

int nDos = 0;
Expand All @@ -803,6 +804,8 @@ void CMasternodeMan::ProcessMessage(CNode* pfrom, std::string& strCommand, CData

pfrom->setAskFor.erase(nHash);

if(!masternodeSync.IsBlockchainSynced()) return;
Copy link

@sidhujag sidhujag Nov 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think this should be IsMasternodeListSynced instead of IsBlockchainSynced?, a MN ping should generally happen after you have the MN in question no? I do see that AskForMN is called at the end of this block of code if the MN is not found for some reason. Is it used for your created but not yet broadcast MN for some reason?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Processing pings while in mn list sync state is ok. DSEG actually sends last ping invs together with mnb invs and (in general) there should be not that much of an overhead here imo. I woudn't touch it (at least for now).


LogPrint("masternode", "MNPING -- Masternode ping, masternode=%s\n", mnp.vin.prevout.ToStringShort());

// Need LOCK2 here to ensure consistent locking order because the CheckAndUpdate call below locks cs_main
Expand Down Expand Up @@ -909,6 +912,8 @@ void CMasternodeMan::ProcessMessage(CNode* pfrom, std::string& strCommand, CData
CMasternodeVerification mnv;
vRecv >> mnv;

pfrom->setAskFor.erase(mnv.GetHash());

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean mWeAskedForVerification? Where is MNVERIFY requested with an INV? Can you show how this MNVERIFY ended up calling setAskFor.insert() on the client side?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We relay mnv via inv in few places and like for all other cases, when the other node has no data yet, it asks us by sending inv back https://github.com/dashpay/dash/blob/master/src/net_processing.cpp#L1419 which updates setAskFor https://github.com/dashpay/dash/blob/master/src/net.cpp#L2761. So once we finally get data on the other node, we should clear setAskFor there. Or am I missing smth?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh I see ok this one was a little convoluted and was a harder path to trace but found it's pushing INV here: https://github.com/dashpay/dash/blob/master/src/masternode.h#L405 so that relays PushInventory to all connected nodes which sets up setAskFor.insert later. That is a good catch and wasn't sure of it when I was implementing I did see it but was not sure. Thanks.

Copy link

@sidhujag sidhujag Nov 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think you need a

if(!masternodeSync.IsMasternodeListSynced()) return;

after line 915 where you added the setAskFor.erase()? I'm not sure if a MN message (case 2 & 3) can be verified if the list isn't synced?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, makes sense not to process mnv until mn list is synced. Will fix, thanks! 👍

if(mnv.vchSig1.empty()) {
// CASE 1: someone asked me to verify myself /IP we are using/
SendVerifyReply(pfrom, mnv, connman);
Expand Down