Fix MasternodeRateCheck #1490
Fix MasternodeRateCheck #1490
Conversation
|
Needs rebase to fix macos travis ci build |
the-schnibble
force-pushed
the
check_rates_only
branch
3 times, most recently
from
73f3624 to
a2a9ec6
Compare
src/governance.cpp
Outdated
| @@ -863,6 +862,8 @@ bool CGovernanceManager::MasternodeRateCheck(const CGovernanceObject& govobj, up | |||
| default: | |||
| break; | |||
| } | |||
|
|
|||
| it->second.fStatusOK = true; | |||
There was a problem hiding this comment.
This line shouldn't be needed because this is set by the last_object_rec constructor (this code only runs for a newly added object).
src/governance.cpp
Outdated
| case UPDATE_FAIL_ONLY: | ||
| if(!fRateOK) { | ||
| pBuffer->AddTimestamp(nTimestamp); | ||
| if(!fRateOK) |
There was a problem hiding this comment.
It looks like the problems you discovered still aren't fixed here.
The break is still missing for the UPDATE_FAIL_ONLY case and all code paths still return true.
Also I don't think UPDATE_TRUE should return true if the rate check fails.
EDIT: OK, I see that there is a path that returns false (I think you should make the break explicit though, not rely on the fall-through behavior which is hard to follow.) I still don't think the UPDATE_TRUE case should be returning true unconditionally.
There was a problem hiding this comment.
Actually, we pass UPDATE_TRUE only when the object has been accepted. Would it better to have a separate method for updating the buffer? I have refactored the code and possibly fixed some other issues with CRateCheckBuffer. Here it is: aaeed1c
the-schnibble
changed the title
the-schnibble
force-pushed
the
check_rates_only
branch
from
c794913 to
742b1d0
Compare
the-schnibble
changed the title
|
needs rebase |
the-schnibble
force-pushed
the
check_rates_only
branch
2 times, most recently
from
219d7f5 to
1729598
Compare
|
Needs another rebase (as expected) |
the-schnibble
changed the title
the-schnibble
force-pushed
the
check_rates_only
branch
2 times, most recently
from
cb02642 to
27781f5
Compare
the-schnibble
changed the title
the-schnibble
force-pushed
the
check_rates_only
branch
from
27781f5 to
fe8d9cc
Compare
| switch(nObjectType) { | ||
| case GOVERNANCE_OBJECT_TRIGGER: | ||
| // Allow 1 trigger per mn per cycle, with a small fudge factor | ||
| pBuffer = &it->second.triggerBuffer; | ||
| buffer = it->second.triggerBuffer; |
There was a problem hiding this comment.
Why this was changed? What's the point of updating local variable only?
There was a problem hiding this comment.
A local variable is used here to check whether the rate limit exceeded if the object is accepted. I moved the buffer updating outside of MasternodeRateCheck to a separate method MasternodeRateUpdate.
Rationale:
MasternodeRateCheckis working only for watchdog's/trigger's owner when he is creating the object initially. In all other cases, the method always returnstruefor all the objects with correct timestamp.CRateCheckBuffereven if check has failed. Bearing in mind (1) it is not a big problem, but we still can drop this object in some cases (object is invalid, we have this object already or watchdog object doesn't fit required time bounds)Explanation of the commits:
fixed an issue with MasternodeRateCheck always returns true
This is simple fix for the issue.
Consequences:
Initial call to
MasternodeRateCheckis no longer updating the buffer when the rate check fails.Taking into account another problem discovered in processing of
MNGOVERNANCEOBJECTwe don't need to update buffer because it won't help to counter spam. But it could cause another problems with governance objects propagation.We shouldn't update buffer unless we accept the object. Otherwise if we reject some object, update the buffer for it's masternode, but other nodes won't update this (because we didn't relay the object), and then this MN can create another valid object which will be accepted by most of nodes, but rejected by us.
UPDATE_TRUEinMasternodeRateCheckis always returns true, just because we always use this flag when object has accepted and this value doesn't matter. In next commit this part of code changed to "return void".additioanal fixes and refactoring
This commit is basically refactoring of the code. It's purposed to make buffer updating outside of
MasternodeRateCheck.Some test results for the PR: https://github.com/the-schnibble/dash-tests/blob/master/results/mn_rate_check.txt
The test itself: https://github.com/the-schnibble/dash-tests/blob/master/mn_rate_check.py
Here node1 is a slightly modified masternode which can create triggers with an arbitrary rate to check that the constraint works for others. The test also checks (counts records in the log) that all objects are relayed, accepted and rejected by nodes as expected (I'm using a private testnet with a reduced interval of superblocks, so the test was waiting for 8.46 minutes to check the "rate limit").