core: Remove fraud detection logic - #572
Conversation
* Validate measurment timings during preprocess * Remove isNaN check for ttfb value
|
Looks like there's multiple type issues atm |
…ation/spark-evaluate into chore/remove-fraud-detection
| prepareProviderRetrievalResultStats: async () => {} | ||
| }) | ||
|
|
||
| let point = telemetry.find(p => p.name === 'retrieval_stats_honest') |
There was a problem hiding this comment.
tooling probably consumes retrieval_stats_honest - what if we mirror all properties from retrieval_stats_all to retrieval_stats_honest, we we know now that all stats are honest?
There was a problem hiding this comment.
Good point, I've reintroduced retrieval_stats_honest in 2d2e291 for backwards compatibility of existing tools.
juliangruber
left a comment
juliangruber
left a comment
There was a problem hiding this comment.
Needs @bajtos's approval
bajtos
left a comment
bajtos
left a comment
There was a problem hiding this comment.
LGTM!
After you ship this change, you can also simplify the code in spark-0k-checker that's picking tasks from the round based using a deterministically random algorithm, see https://github.com/CheckerNetwork/spark-0k-checker/blob/61a41564122dd2f994e164b3d8690c29dfda1007/lib/tasker.js#L127-L162
| // PERFORMANCE: Avoid duplicating the array of measurements because there are | ||
| // hundreds of thousands of them. All the function groupMeasurementsToCommittees | ||
| // needs is to iterate over the accepted measurements once. | ||
| const iterateAcceptedMeasurements = function * () { | ||
| for (const m of measurements) { | ||
| // Mark all measurements as accepted by default. | ||
| m.taskingEvaluation = 'OK' | ||
| yield m | ||
| } | ||
| } |
There was a problem hiding this comment.
- The performance aspect is no longer true as we expect to have 100x-1000x fewer measurements in the 0k architecture.
- The goal of the iterator was to provide a read-only view into a subset of measurements. In my mind, iterators are expected to only read the data source. I find it very surprising that iterating over accepted measurements changes
m.taskEvaluationfield.
Proposal: I assume your goal here is to mark all measurements as accepted. Let's use a simple for loop.
| // PERFORMANCE: Avoid duplicating the array of measurements because there are | |
| // hundreds of thousands of them. All the function groupMeasurementsToCommittees | |
| // needs is to iterate over the accepted measurements once. | |
| const iterateAcceptedMeasurements = function * () { | |
| for (const m of measurements) { | |
| // Mark all measurements as accepted by default. | |
| m.taskingEvaluation = 'OK' | |
| yield m | |
| } | |
| } | |
| // Mark all measurements as accepted by default. | |
| for (const m of measurements) { | |
| m.taskingEvaluation = 'OK' | |
| } |
| const { committees, timings: fraudDetectionTimings } = await runFraudDetection({ | ||
| const committees = Array.from(evaluationCommittees.values()) | ||
| logger.log( | ||
| 'EVALUATE ROUND %s: Evaluated %s measurements.\n', |
There was a problem hiding this comment.
| 'EVALUATE ROUND %s: Evaluated %s measurements.\n', | |
| 'EVALUATE ROUND %s: Accepted all %s measurements.\n', |
| } | ||
| logger.log( | ||
| 'EVALUATE ROUND %s: Evaluated %s measurements.\n%o', | ||
| 'EVALUATE ROUND %s: Evaluated %s measurements.\n', |
There was a problem hiding this comment.
This is the same log as above. Let's remove it completely.
Also, please remove the /** @type line 62/72 above.
| point.intField('fraud_detection_duration_ms', fraudDetectionDuration) | ||
| for (const { influxField, duration } of fraudDetectionTimings) { | ||
| point.intField(`fraud_detection_timings_${influxField}_ms`, duration) | ||
| } | ||
| for (const [type, count] of Object.entries(evaluationOutcomes)) { | ||
| point.intField(`measurements_${type}`, count) | ||
| } |
There was a problem hiding this comment.
I think this change may break some Grafana charts. I agree that we no longer need to monitor these metrics. After you ship this PR and some rounds are evaluated, please review the Grafana dashboard(s) and remove the charts that visualise these metrics and no longer have data.
4 participants
This pull request simplifies the evaluation process in
lib/evaluate.jsby removing therunFraudDetectionfunction and replacing it with a streamlined approach for grouping and evaluating measurements. By removing the fraud detection all measurements are marked as valid and are accepted by the evaluation mechanism. It also updates related telemetry and test cases to reflect these changes.Simplifications in Evaluation Logic:
runFraudDetectionfunction and replaced it with an inline implementation that directly processes measurements marking them as valid using a generator function (iterateAcceptedMeasurements) to avoid array duplication. (lib/evaluate.js, [1] [2] [3]REQUIRED_COMMITTEE_SIZEfrom 30 to 1. (lib/evaluate.js, lib/evaluate.jsL3-R12)Updates to Telemetry:
fraud_detection_duration_msandretrieval_stats_honest, consolidating all retrieval stats underretrieval_stats_all. (lib/evaluate.js, [1] [2]Test Case Adjustments:
test/evaluate.jsto reflect the removal ofrunFraudDetectionand the consolidation of telemetry points. This includes changes to expected telemetry data and test descriptions. (test/evaluate.js, [1] [2] [3] [4] [5]Blocked by #569
Closes #574