Minimal fixes to address NCFP-10: Do not allow "red" in-queue verifiers anymore.

EggPool commented 4 years ago

Reference:
https://github.com/n-y-z-o/nyzoVerifier/issues/22

By voting Yes to ncfp-10, current cycle acknowledged the issue with always red verifiers being able to remain in queue with no actual verifier running, just spamming nodejoins once in "nodes" file.

This patch does

persist the new communicationFailureCount metric in nodes file.
rebalance increment/reset of failure count by reseting communicationFailureCount on successful answer only, never on incoming messages alone.
log nodejoins messages in a more verbose way, so external scripts can use that info (ip and short-id) to auto-ban or rate limit spamming nodes.
adds some context in changelog.md

This version has been tested since several days on several verifiers (both in-cycle and in-queue).
v596 and 597 have been integrated since so this PR can be merge with no conflict.

A preview of what the eligible (> 30 days) nyzo queue would look like if present at https://nyzo.today/queue from a modded verifier.
https://nyzo.today/dropped is a searchable table of "dropped" verifiers (nodes that are in the original nodes file but dropped from the modded version due to being consistently inactive for too long)

The difference with current nyzo queue is striking, and no false positive was reported so far.
This is just a first step to address huge number of verifiers abusing current code, but in no way a definitive answer.
This only patches today's most urging issue in the most minimal and side effect proof way.

itsMarcoSolis commented 4 years ago

By comparing the queue before and after applying this fix it is evident that a major improvement in the joining process has been achieved. Not a single member of the community has reported a false positive so it's safe to asume this change won't impact the cycle on a negative manner. I hope the core devs can merge this PR soon.

bh00ker commented 4 years ago

I fully support this proposed PR. Been following closely. I think it addresses NCFP-10 quite well.

m-burner commented 4 years ago

I'm against this PR. We need a vote from the whole community. If someone decides to make changes that cut 3/4 of the queue, a general decision is needed.

bh00ker commented 4 years ago

I'm against this PR. We need a vote from the whole community. If someone decides to make changes that cut 3/4 of the queue, a general decision is needed.

We voted that red verifiers shouldnt be allowed to participate in the queue / lottery. This addresses it, and well I might add.

orisica commented 4 years ago

Great work, about time we remove fake verifiers from the queue and resume Proof Of Diversity which is in accordance with cycle vote.

Syd-ai commented 4 years ago

Great work, it will definitely grants a better diversity to nyzo cycle. Looking forward for a merge 🙏

setanimals commented 4 years ago

Supported! TY Sy for all the great work on this.

l-u-f-o commented 4 years ago

Reviewed the code and test results. This PR has my full support. The cycle tx result of NCFP-10 also shows support by the community.

nsejourne commented 4 years ago

I want this PR to be merged !

krypdkat commented 4 years ago

LGTM :+1: tested for 4 days. No issue.

I think you should resolve the conflict, squash all commits into 1 and move the ChangeLog to somewhere else to make it easier for the dev to build the next version upon your PR :slightly_smiling_face:

n-y-z-o / nyzoVerifier

Minimal fixes to address NCFP-10: Do not allow "red" in-queue verifiers anymore. #25