Make sure to clear setAskFor in Dash submodules #1730
Conversation
Thanks @sidhujag for finding the bug
src/masternode-payments.cpp
Outdated
| // TODO: clear setAskFor for MSG_MASTERNODE_PAYMENT_BLOCK too | ||
|
|
||
| if(pfrom->nVersion < GetMinMasternodePaymentsProto()) return; | ||
|
|
There was a problem hiding this comment.
not needed because if node is misbehaving by sending wrong peer version let setAskFor be not cleared to avoid DDOS .
There was a problem hiding this comment.
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.
| @@ -909,6 +912,8 @@ void CMasternodeMan::ProcessMessage(CNode* pfrom, std::string& strCommand, CData | |||
| CMasternodeVerification mnv; | |||
| vRecv >> mnv; | |||
|
|
|||
| pfrom->setAskFor.erase(mnv.GetHash()); | |||
|
|
|||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yep, makes sense not to process mnv until mn list is synced. Will fix, thanks! 👍
| @@ -803,6 +804,8 @@ void CMasternodeMan::ProcessMessage(CNode* pfrom, std::string& strCommand, CData | |||
|
|
|||
| pfrom->setAskFor.erase(nHash); | |||
|
|
|||
| if(!masternodeSync.IsBlockchainSynced()) return; | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
|
ACK |
* Make sure to clear setAskFor in Dash submodules Thanks @sidhujag for finding the bug * partially revert previous commit * skip mnv until mn list is synced
Thanks @sidhujag for finding the bug