Implement a way to sync only needed winners#1028
Implement a way to sync only needed winners#1028UdjinM6 merged 3 commits intodashpay:v0.12.1.xfrom UdjinM6:msgWinnerBlock
Conversation
…stead of/after preliminary Sync.
tgflynn
left a comment
tgflynn
left a comment
There was a problem hiding this comment.
I don't really get the point of RequestLowDataPaymentBlocks. It doesn't look like it guarantees that all votes are counted so why only count some of them ?
Moreover I don't see why votes for past blocks are needed at all, since they can't influence any decisions the node will now make.
| CMasternodePayee(CScript payee, uint256 hashIn) : | ||
| scriptPubKey(payee), | ||
| nVotes(1), | ||
| vecVoteHashes(hashIn) |
There was a problem hiding this comment.
This looks wrong. I'm surprised it even compiles. I think you want the default constructor here.
There was a problem hiding this comment.
eeek... I have no idea how this slipped in, will fix
|
|
||
| template <typename Stream, typename Operation> | ||
| inline void SerializationOp(Stream& s, Operation ser_action, int nType, int nVersion) { | ||
| READWRITE(*(CScriptBase*)(&scriptPubKey)); |
There was a problem hiding this comment.
I know you didn't change this but this looks weird. Why all the casting ? Shouldn't READWRITE(scriptPubKey) suffice ?
| public: | ||
| CMasternodePayee() : | ||
| scriptPubKey(), | ||
| nVotes(0), |
There was a problem hiding this comment.
I don't think nVotes is needed now. I would remove it and just return vecVoteHashes.size().
| { | ||
| public: | ||
| private: | ||
| CScript scriptPubKey; |
There was a problem hiding this comment.
Shouldn't vin (ie. COutPoint) be used here to insure uniqueness ?
There was a problem hiding this comment.
Yep, that would be better but we only have CScript payee in CMasternodePaymentWinner and switching from payee to vin would break backward compatibility. This is still quite unique though because this one corresponds to pubKeyCollateralAddress.
| BOOST_FOREACH(CMasternodePayee& payee, it->second.vecPayees) { | ||
| if(payee.GetVoteCount() >= MNPAYMENTS_SIGNATURES_REQUIRED) { | ||
| fFound = true; | ||
| break; |
There was a problem hiding this comment.
It looks like the break here will make nTotalVotes innacurate
There was a problem hiding this comment.
yep, but it will not be used if fFound is true anyway
|
The point of RequestLowDataPaymentBlocks is to find "payment blocks" which have not enough votes to identify the winner. The reason it uses avg rather than max for votes threshold (which is the same criteria as in IsEnoughData) is because it's quite common to miss few votes for some old blocks or due to some voting issues (some MNs do not find themselves in top10 i.e. do not cast a vote at all). Votes for past blocks are needed because you need to construct history of payments in some way. I would agree however that having new UpdateLastPaid in place (i.e. scanning back blockchain) could probably also help to optimize logic for selecting which "payment blocks" to request but I would leave this out of the scope of this PR. |
|
utACK |
Followup for the recent winner_block PR (#1028) and name confusion discovered during code review. "Mostly" because also: - CMasternodeBlockPayees::GetPayee -> CMasternodeBlockPayees::GetBestPayee which describes what this function is actually doing a bit better imo; - fixing constructor CMasternodePaymentVote empty constructor
2 participants
The idea is to ask node we are trying to sync from only for future payment blocks and for blocks where payment info is missing/incomplete for us instead of asking for the whole set of winners every time we sync. This (hopefully) should significantly lower cpu/net load if node was offline for not too long. For nodes which were offline for a period of time comparable to nMinBlocksToStore (~1 week on mainnet) there should be (almost) no difference.