Support optional cheater detection #564
Conversation
Create option for aggregating without cheater detection Some renaming
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #564 +/- ##
==========================================
+ Coverage 81.72% 81.78% +0.05%
==========================================
Files 30 30
Lines 3076 3086 +10
==========================================
+ Hits 2514 2524 +10
Misses 562 562
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I assumed that this would be a runtime thing, not compile time. Sorry for not clearly communicating that. (To do at runtime it could check if the verifying shares map is empty, and disable it if it is)
However, this is a valid way to do it. I'm not sure about it though. One advantage of being compile-time is that it's harder to accidentally disable it (assuming we enable it by default, which this PR doesn't do yet, but it's a simple change).
On the other hand we could find a way to do it at runtime while making it harder to disable by accident. For example, with a new aggregate_without_cheater_detection() function (that would call an e.g. refactored out private aggregate_inner() function that is also called by aggregate()).
But I'm not sure if we need to do it at runtime.
What do you think?
|
is it PR title vague or I'm reading it wrong? pr gates existing cheater detection under a feature gate but titled as like cheater detection functionality? |
Yes, if you look at the matching issue, it's about making cheater detection optional. I've updated the PR title to match it. |
There was a problem hiding this comment.
I comitted a suggestion and there are some comments.
The PR will unbreak after #551 merges
Adds cheater detection feature which gives the option to use cheater detection in the aggregate function or not.
This closes #355