refactor: break fraudAssessment into evaluations #442
Conversation
Break `Measurement.fraudAssessment` into two new fields: - `taskingEvaluation` for the tasking-related checks Example: DUP_INET_GROUP - `majorityEvaluation` for the result of the majority seeking process based on committees. Example: MINORITY_RESULT After this change, places filtering "accepted" measurements have to explicitly spell out how they define "accepted". - Some places are interested in tasking evaluation results only and consider minority results as "accepted" too. Example: RSR calculated from individual measurements. - Other places are stricter and want only measurements in majority. Example: which measurements to reward. Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
| /** @type {import('./preprocess.js').Measurement[]} */ | ||
| /** @type {string[]} */ | ||
| this.measurementBatches = [] | ||
| /** @type {Measurement[]} */ | ||
| this.measurements = [] |
There was a problem hiding this comment.
The field measurementBatches stores a list of CIDs, see here:
spark-evaluate/lib/preprocess.js
Line 119 in 6709840
Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
bin/evaluate-measurements.js
Outdated
| @@ -98,22 +98,24 @@ async function processRound (roundIndex, measurements, resultCounts) { | |||
| }) | |||
|
|
|||
| for (const m of round.measurements) { | |||
| if (m.fraudAssessment !== 'OK') continue | |||
| if (m.taskingEvaluation !== 'OK') continue | |||
There was a problem hiding this comment.
This is also a change in behavior. Previously, majority failures would be rejected here, now they are kept.
Before I continue with the review, can we please resolve this and the above comment? It looks like this question affects most of the changes.
There was a problem hiding this comment.
Great catch, I'll fix this place to preserve the current behaviour and add a FIXME comment similar to the one on lines 109-111 below.
I'd also like to point out that this is a helper script for SPs to inspect recent Spark measurements, it's known to be flawed (see #396), and yet nobody complained about that bug yet - so again, I think the impact of a breaking change in this file is low.
There was a problem hiding this comment.
I re-read the PR description and now see that this change in behavior might be intentional. Are you confident that all changes in behavior in this PR are intentional?
(...)
Before I continue with the review, can we please resolve this and the above comment? It looks like this question affects most of the changes.
I started this PR with the intention to combine refactoring with changes fixing the conditions determining which measurements are considered as accepted. I quickly realise that would make it difficult to reason about the changes in the future, so I decided to pivot and change this pull request into pure refactoring with no change in functionality.
I updated the pull request description to clearly spell out this intention.
I am 90% confident that this pull request does not introduce any change in the functionality in the lib files. I wasn't paying as much attention to bin files, but I think you caught the remaining places to fix and this PR is not introducing any unintentional changes now.
There was a problem hiding this comment.
I searched for all occurences of fraudAssessment === in the pull request and verified that they are all expanded to taskingEvaluation === 'OK' && majorityEvaluation === 'OK'.
I found one place I had to fix - see 16938d8
Co-authored-by: Julian Gruber <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
Break
Measurement.fraudAssessmentinto two new fields:taskingEvaluationfor the tasking-related checksExample: DUP_INET_GROUP
majorityEvaluationfor the result of the majority seeking process based on committees.Example: MINORITY_RESULT
After this change, places filtering "accepted" measurements have to explicitly spell out how they define "accepted".
Some places are interested in tasking evaluation results only and consider minority results as "accepted" too. Example: RSR calculated from individual measurements.
Other places are stricter and want only measurements in majority. Example: which measurements to reward.
However, this pull request is intended to be pure refactoring with no change in the functionality. It should simply expand the check
fraudAssessment === 'OK'intotaskingEvaluation === 'OK' && majorityEvaluation === 'OK'and surface the places where we may want to include minority measurements too. Such changes can be implement in follow-up pull requests.This is a follow-up for #396 (comment)
See also #439