-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: form committees and find majority results #304
Merged
Merged
Changes from 11 commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
42c73c0
feat: form committees and find majority results
bajtos 1660c57
Merge branch 'main' into feat-majorities
bajtos 74661e8
Update lib/committee.js
bajtos fa872f8
refactor: simplify committee evaluation
bajtos 0732097
refactor: committee.measurements are private now
bajtos 96d23a8
Merge branch 'main' into feat-majorities
bajtos 504a159
Merge branch 'main' into feat-majorities
bajtos 5018dbf
refactor: make requiredCommitteeSize configurable
bajtos f9cf793
refactor: calculate committee stats from committees
bajtos 3a91be9
feat: telemetry `committees_too_small`
bajtos 16381a5
add reminders what to change as part of this PR
bajtos d959ecd
Update lib/committee.js
bajtos 3197125
Update lib/committee.js
bajtos 01cdb6d
Update lib/committee.js
bajtos ac582d2
Merge branch 'main' into feat-majorities
bajtos 029db0d
refactor: allow undefined committee evaluation
bajtos d61cef9
fixup! document why we use async generator to iterate accepted measur…
bajtos 889b112
feat: calculate indexer query stats using committees
bajtos b8916a9
feat: calculate daily deals using committees
bajtos 25cde29
feat: cross-check CAR size and checksum
bajtos eb38242
Merge branch 'main' into feat-majorities
bajtos File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,200 @@ | ||
import assert from 'node:assert' | ||
import createDebug from 'debug' | ||
import { getTaskId } from './retrieval-stats.js' | ||
|
||
/** @import {Measurement} from './preprocess.js' */ | ||
/** @import {RetrievalResult, CommitteeCheckError} from './typings.js' */ | ||
|
||
const debug = createDebug('spark:committee') | ||
|
||
/** @typedef {Map<string, Committee>} TaskIdToCommitteeMap */ | ||
|
||
export class Committee { | ||
/** @type {Measurement[]} */ | ||
#measurements | ||
|
||
/** | ||
* @param {Pick<Measurement, 'cid' | 'minerId'>} retrievalTask | ||
*/ | ||
constructor ({ cid, minerId }) { | ||
this.retrievalTask = { minerId, cid } | ||
|
||
this.#measurements = [] | ||
|
||
/** @type {string | CommitteeCheckError} */ | ||
this.indexerResult = 'MAJORITY_NOT_FOUND' | ||
|
||
/** @type {RetrievalResult} */ | ||
this.retrievalResult = 'MAJORITY_NOT_FOUND' | ||
} | ||
|
||
get size () { | ||
return this.#measurements.length | ||
} | ||
|
||
get measurements () { | ||
const ret = [...this.#measurements] | ||
Object.freeze(ret) | ||
return ret | ||
} | ||
|
||
/** | ||
* @param {Measurement} m | ||
*/ | ||
addMeasurement (m) { | ||
assert.strictEqual(m.cid, this.retrievalTask.cid, 'cid must match') | ||
assert.strictEqual(m.minerId, this.retrievalTask.minerId, 'minerId must match') | ||
assert.strictEqual(m.fraudAssessment, 'OK', 'only accepted measurements can be added') | ||
this.#measurements.push(m) | ||
} | ||
|
||
/** | ||
* @param {object} args | ||
* @param {number} args.requiredCommitteeSize | ||
* @returns | ||
*/ | ||
evaluate ({ requiredCommitteeSize }) { | ||
debug( | ||
'Evaluating task %o with a committee of %s measurements', | ||
this.retrievalTask, | ||
this.#measurements.length | ||
) | ||
|
||
if (this.#measurements.length < requiredCommitteeSize) { | ||
debug('→ committee is too small (size=%s required=%s); retrievalResult=COMMITTEE_TOO_SMALL', | ||
this.#measurements.length, | ||
requiredCommitteeSize | ||
) | ||
this.indexerResult = 'COMMITTEE_TOO_SMALL' | ||
this.retrievalResult = 'COMMITTEE_TOO_SMALL' | ||
this.#measurements.forEach(m => { m.fraudAssessment = 'COMMITTEE_TOO_SMALL' }) | ||
bajtos marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return | ||
} | ||
|
||
debug('- searching for majority in indexer results') | ||
/** @type {(keyof Measurement)[]} */ | ||
const indexerResultProps = [ | ||
'providerId', | ||
'indexerResult', | ||
'provider_address', | ||
'protocol' | ||
] | ||
const indexerResultMajority = this.#findMajority(indexerResultProps) | ||
if (indexerResultMajority) { | ||
this.indexerResult = indexerResultMajority.majorityValue.indexerResult | ||
} | ||
|
||
debug('- searching for majority in retrieval results') | ||
/** @type {(keyof Measurement)[]} */ | ||
const retrievalResultProps = [ | ||
...indexerResultProps, | ||
'retrievalResult' | ||
// TODO | ||
// - status_code, | ||
// - timeout, | ||
// - car size | ||
// - car checksum | ||
// - car too large | ||
] | ||
|
||
const retrievalResultMajority = this.#findMajority(retrievalResultProps) | ||
if (retrievalResultMajority) { | ||
this.retrievalResult = retrievalResultMajority.majorityValue.retrievalResult | ||
retrievalResultMajority.minorityMeasurements.forEach(m => { | ||
m.fraudAssessment = 'MINORITY_RESULT' | ||
}) | ||
bajtos marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} else { | ||
this.retrievalResult = 'MAJORITY_NOT_FOUND' | ||
this.#measurements.forEach(m => { m.fraudAssessment = 'MAJORITY_NOT_FOUND' }) | ||
bajtos marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
|
||
/** | ||
* @param {(keyof Measurement)[]} measurementFields | ||
*/ | ||
#findMajority (measurementFields) { | ||
/** @param {Measurement} m */ | ||
const getResult = m => pick(m, ...measurementFields) | ||
|
||
/** @type {Map<string, Measurement[]>} */ | ||
const resultGroups = new Map() | ||
|
||
// 1. Group measurements using the result as the grouping key | ||
for (const m of this.#measurements) { | ||
const key = JSON.stringify(getResult(m)) | ||
let list = resultGroups.get(key) | ||
if (!list) { | ||
list = [] | ||
resultGroups.set(key, list) | ||
} | ||
list.push(m) | ||
} | ||
|
||
if (debug.enabled) { | ||
debug('- results found:') | ||
for (const k of resultGroups.keys()) { | ||
debug(' %o', JSON.parse(k)) | ||
} | ||
} | ||
|
||
// 2. Sort the measurement groups by their size | ||
const keys = Array.from(resultGroups.keys()) | ||
keys.sort( | ||
(a, b) => (resultGroups.get(a)?.length ?? 0) - (resultGroups.get(b)?.length ?? 0) | ||
) | ||
const measurementGroups = keys.map(k => resultGroups.get(k)) | ||
|
||
// 3. Find the majority | ||
const majorityMeasurements = measurementGroups.pop() | ||
const majoritySize = majorityMeasurements.length | ||
const majorityValue = getResult(majorityMeasurements[0]) | ||
|
||
debug('- majority=%s committee-size=%s value=%o', majoritySize, this.size, majorityValue) | ||
if (majoritySize <= this.size / 2) { | ||
debug('→ majority is not absolute; result=MAJORITY_NOT_FOUND') | ||
return undefined | ||
} else { | ||
debug('→ majority agrees on result=%o', majorityValue) | ||
return { | ||
majorityValue, | ||
majorityMeasurements, | ||
minorityMeasurements: measurementGroups.flat() | ||
} | ||
} | ||
} | ||
} | ||
|
||
/** | ||
* @template T | ||
* @template {keyof T} K | ||
* @param {T} obj | ||
* @param {K[]} keys | ||
* @returns {Pick<T, K>} | ||
*/ | ||
function pick (obj, ...keys) { | ||
/** @type {any} */ | ||
const ret = {} | ||
keys.forEach(key => { | ||
ret[key] = obj[key] | ||
}) | ||
return ret | ||
} | ||
|
||
/** | ||
* @param {Iterable<Measurement>} measurements | ||
* @returns {TaskIdToCommitteeMap} | ||
*/ | ||
export function groupMeasurementsToCommittees (measurements) { | ||
/** @type {TaskIdToCommitteeMap} */ | ||
const committeesMap = new Map() | ||
for (const m of measurements) { | ||
const key = getTaskId(m) | ||
let c = committeesMap.get(key) | ||
if (!c) { | ||
c = new Committee(m) | ||
committeesMap.set(key, c) | ||
} | ||
c.addMeasurement(m) | ||
} | ||
return committeesMap | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think there can be a case where
Committee
is initialized, then there's an error duringevaluate()
, then it's state is read and it's falsely interpreted as'MAJORITY_NOT_FOUND'
, while it should rather be something like 'UNKNOWN_ERROR
'?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you are right that this design (a mutable Committee class) allows that to happen in theory. I think it should not happen in practice right now because if there is an error, then an exception is thrown and the entire evaluation is aborted.
I propose to move these two "*Result" properties into a nested object stored in
Commitee.evaluation
property, and let this property beundefined
until the evaluation completes.See 029db0d
A possible next refactoring is to remove the Committee class entirely and modify
Committee.evaluate
into a pure function accepting({requiredCommitteeSize, retrievalTask, measurements})
and returningCommitteeEvaluation
.Such change would push the complexity to
runFraudDetection
, which would need to create the list of committees - an array of{retrievalTask, measurements, evaluation}
objects. This list of committees is later used to extract stats for InfluxDB and spark-stats API.In that light, such refactoring does not seem to be an improvement.
Let me know what you think!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to having the result be
undefined
or something else, which we can differentiate from the previous default stateThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I'm not sure whether removing the class does help, let's see how it goes?