refactor: break fraudAssessment into evaluations #442

bajtos · 2025-01-09T14:22:18Z

Break Measurement.fraudAssessment into two new fields:

taskingEvaluation for the tasking-related checks
Example: DUP_INET_GROUP
consensusEvaluation 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.

However, this pull request is intended to be pure refactoring with no change in the functionality. It should simply expand the check fraudAssessment === 'OK' into taskingEvaluation === 'OK' && consensusEvaluation === 'MAJORITY_RESULT' 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

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]>

bajtos

🧵 TODO items blocking the conversion from "draft" to "ready" 👇🏻

lib/evaluate.js

bin/evaluate-measurements.js

lib/round.js

bajtos

More TODOs 👇🏻

lib/platform-stats.js

bajtos · 2025-01-09T14:37:32Z

lib/retrieval-stats.js

@@ -81,8 +81,9 @@ export const buildRetrievalStats = (measurements, telemetryPoint) => {
    const endAt = m.end_at
    const ttfb = startAt && firstByteAt && (firstByteAt - startAt)
    const duration = startAt && endAt && (endAt - startAt)
+    const isAccepted = m.taskingEvaluation === 'OK' && m.majorityEvaluation === 'OK'


TODO: check if we need to add a test

Done in c08c10a

To double check: We will now be calling measurements that will be rewarded "accepted measurements", right?

Hmm, this is a good call! 🤔

I want to call measurements that will be rewarded "measurementsToReward"; that's what I use inside the main evaluation loop.

Everywhere else, we need to decide whether we are interested in measurements that passed tasking evaluation but may be in the minority, or whether we are interested in measurements that will be rewarded (passed the tasking evalution and are in majority).

In that light, maybe it would be better to avoid using the term "accepted measurements" altogether. Or qualify it as "measurements accepted by tasking"?

I don't have a good answer 😞

lib/retrieval-stats.js

Signed-off-by: Miroslav Bajtoš <[email protected]>

lib/retrieval-stats.js

bin/dry-run.js

bin/evaluate-measurements.js

Co-authored-by: Julian Gruber <[email protected]>

Signed-off-by: Miroslav Bajtoš <[email protected]>

juliangruber

Great work!

lib/evaluate.js

lib/platform-stats.js

lib/preprocess.js

juliangruber · 2025-01-10T16:52:53Z

lib/retrieval-stats.js

@@ -81,8 +81,9 @@ export const buildRetrievalStats = (measurements, telemetryPoint) => {
    const endAt = m.end_at
    const ttfb = startAt && firstByteAt && (firstByteAt - startAt)
    const duration = startAt && endAt && (endAt - startAt)
+    const isAccepted = m.taskingEvaluation === 'OK' && m.majorityEvaluation === 'OK'


To double check: We will now be calling measurements that will be rewarded "accepted measurements", right?

juliangruber · 2025-01-10T16:55:13Z

Feel free to land this work next week during my vacation, without further review by me

Co-authored-by: Julian Gruber <[email protected]>

Signed-off-by: Miroslav Bajtoš <[email protected]>

NikolasHaimerl

LGTM, I'll approve once Julians changes have been addressed.

…nDetails Signed-off-by: Miroslav Bajtoš <[email protected]>

…ment-step2

bajtos · 2025-01-21T14:08:11Z

@juliangruber @NikolasHaimerl the pull request is ready for the final review 👀

lib/committee.js

lib/typings.d.ts

Signed-off-by: Miroslav Bajtoš <[email protected]>

lib/committee.js

…und, retrievalMajorityFound Signed-off-by: Miroslav Bajtoš <[email protected]>

bajtos · 2025-01-23T09:47:30Z

@NikolasHaimerl would you like to review the final version or can I go ahead and land the PR?

pyropy · 2025-01-23T13:19:03Z

test/evaluate.js

@@ -241,6 +241,43 @@ describe('evaluate', async function () {
    assertPointFieldValue(point, 'total_nodes', '3i')
  })



Small nitpick -- should rename this file from evaluate.js to evaluate.test.js to make things consistent?

Yeah, we have a bit of inconsistency in file naming. I am fine to rename it. Let's keep such changes out of the scope of this pull request, please.

pyropy · 2025-01-23T13:22:33Z

lib/preprocess.js

@@ -22,8 +22,10 @@ export class Measurement {
    // Note: providerId is recorded by spark-publish but we don't use it for evaluations yet
    this.providerId = pointerize(m.provider_id)
    this.spark_version = pointerize(m.spark_version)
-    /** @type {import('./typings.js').FraudAssesment} */
-    this.fraudAssessment = null
+    /** @type {import('./typings.js').TaskingEvaluation} */


Since initial value is set to null by default we might should set property type to be optional, maybe something like:

/** @type {import('./typings.js').TaskingEvaluation?} */

Agreed 👍

This pull request is not making this any worse, so let's leave such improvement out of the scope, please.

bajtos requested review from juliangruber and NikolasHaimerl January 9, 2025 14:22

fixup! remove unused import

9e59ed6

Signed-off-by: Miroslav Bajtoš <[email protected]>

bajtos commented Jan 9, 2025

View reviewed changes

lib/evaluate.js Outdated Show resolved Hide resolved

lib/evaluate.js Outdated Show resolved Hide resolved

bin/evaluate-measurements.js Outdated Show resolved Hide resolved

lib/round.js Show resolved Hide resolved

This was referenced Jan 9, 2025

refactor: remove unused param honestMeasurements #443

Merged

fix: fetch-recent-miner-measurements & committees #396

Closed

bajtos commented Jan 9, 2025

View reviewed changes

Merge branch 'main' into refactor-fraud-assessment-step2

07be2cc

bajtos mentioned this pull request Jan 10, 2025

Include non-majority measurements in retrieval result breakdown #446

Open

bajtos added 5 commits January 10, 2025 10:16

fixup! add FIXME comments

1392633

Signed-off-by: Miroslav Bajtoš <[email protected]>

test: we don't reward measurements that are not in majority

11d896e

Signed-off-by: Miroslav Bajtoš <[email protected]>

Merge branch 'main' into refactor-fraud-assessment-step2

8731b08

test: counts only majority measurements as accepted

01dc39f

Signed-off-by: Miroslav Bajtoš <[email protected]>

test: records histogram of "score per inet group"

c08c10a

Signed-off-by: Miroslav Bajtoš <[email protected]>

bajtos marked this pull request as ready for review January 10, 2025 09:43

NikolasHaimerl reviewed Jan 10, 2025

View reviewed changes

lib/retrieval-stats.js Outdated Show resolved Hide resolved

juliangruber reviewed Jan 10, 2025

View reviewed changes

bin/dry-run.js Outdated Show resolved Hide resolved

bin/evaluate-measurements.js Outdated Show resolved Hide resolved

bajtos and others added 3 commits January 10, 2025 15:23

Update bin/dry-run.js

aa08724

Co-authored-by: Julian Gruber <[email protected]>

fix evaluate-measurements

f8a0505

Signed-off-by: Miroslav Bajtoš <[email protected]>

preserve behaviour in bin/evaluate-measurements.js

16938d8

Signed-off-by: Miroslav Bajtoš <[email protected]>

bajtos requested review from juliangruber and NikolasHaimerl January 10, 2025 14:54

juliangruber requested changes Jan 10, 2025

View reviewed changes

bajtos and others added 2 commits January 13, 2025 17:23

Apply suggestions from code review

52390bd

Co-authored-by: Julian Gruber <[email protected]>

fixup! removeu unnecessary check

1aba267

Signed-off-by: Miroslav Bajtoš <[email protected]>

NikolasHaimerl reviewed Jan 15, 2025

View reviewed changes

Merge branch 'main' into refactor-fraud-assessment-step2

a6a84bc

bajtos requested a review from juliangruber January 15, 2025 12:41

bajtos added 2 commits January 21, 2025 15:03

Rename to consensusEvaluation, MAJORITY_RESULT and commitee.evaluatio…

7a5db2d

…nDetails Signed-off-by: Miroslav Bajtoš <[email protected]>

Merge remote-tracking branch 'origin/main' into refactor-fraud-assess…

a88ffba

…ment-step2

bajtos requested a review from NikolasHaimerl January 21, 2025 14:08

juliangruber requested changes Jan 21, 2025

View reviewed changes

lib/committee.js Outdated Show resolved Hide resolved

lib/typings.d.ts Outdated Show resolved Hide resolved

bajtos added 2 commits January 21, 2025 15:58

rename to ConsensusCheckError

85c882d

Signed-off-by: Miroslav Bajtoš <[email protected]>

rename to ConsensusEvaluationDetails

c9d91eb

Signed-off-by: Miroslav Bajtoš <[email protected]>

bajtos requested a review from juliangruber January 21, 2025 14:59

juliangruber requested changes Jan 21, 2025

View reviewed changes

lib/committee.js Outdated Show resolved Hide resolved

rename to ConsensusNotFoundReason, CommitteeDecision, indexMajorityFo…

3680486

…und, retrievalMajorityFound Signed-off-by: Miroslav Bajtoš <[email protected]>

bajtos requested a review from pyropy as a code owner January 22, 2025 09:02

Merge branch 'main' into refactor-fraud-assessment-step2

e141918

bajtos requested a review from juliangruber January 22, 2025 09:04

juliangruber approved these changes Jan 22, 2025

View reviewed changes

pyropy reviewed Jan 23, 2025

View reviewed changes

NikolasHaimerl approved these changes Jan 24, 2025

View reviewed changes

Merge branch 'main' into refactor-fraud-assessment-step2

eee1d77

bajtos mentioned this pull request Jan 27, 2025

Include minority retrieval results in the output of evaluate-measurements.js script #458

Open

pyropy approved these changes Jan 27, 2025

View reviewed changes

bajtos merged commit 0f6d5bf into main Jan 27, 2025
6 checks passed

bajtos deleted the refactor-fraud-assessment-step2 branch January 27, 2025 09:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: break fraudAssessment into evaluations #442

refactor: break fraudAssessment into evaluations #442

bajtos commented Jan 9, 2025 •

edited

Loading

bajtos left a comment

bajtos left a comment

bajtos Jan 9, 2025

bajtos Jan 10, 2025

juliangruber Jan 10, 2025

bajtos Jan 13, 2025

juliangruber left a comment

juliangruber Jan 10, 2025

juliangruber commented Jan 10, 2025

NikolasHaimerl left a comment

bajtos commented Jan 21, 2025

bajtos commented Jan 23, 2025

pyropy Jan 23, 2025

bajtos Jan 23, 2025

pyropy Jan 23, 2025

bajtos Jan 24, 2025

		@@ -241,6 +241,43 @@ describe('evaluate', async function () {
		assertPointFieldValue(point, 'total_nodes', '3i')
		})

refactor: break fraudAssessment into evaluations #442

refactor: break fraudAssessment into evaluations #442

Conversation

bajtos commented Jan 9, 2025 • edited Loading

bajtos left a comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliangruber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliangruber commented Jan 10, 2025

NikolasHaimerl left a comment

Choose a reason for hiding this comment

bajtos commented Jan 21, 2025

bajtos commented Jan 23, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Jan 9, 2025 •

edited

Loading