Allow infinite participants by not storing them

[juliangruber]

Prevent the contract from becoming dysfunctional by limiting the total number of participants per round.

When the limit is reached, no new scores will be accepted. This is a problem, because then the round will never finish -> rewards will never be paid and the round stays in contract storage. I'm inclined to count rounds with too many participants as invalid, and not pay anything. This is an attack vector, but it's somewhat fair, since everyone gets paid the same in this case. Another option is to randomly pick participants that make it and ignore the rest. This way you can't attack the contract leading to no pay outs. Otoh, if you swarm it with too many nodes, you increase the chances of you receiving the majority of payouts.

The limit is set to 200k participants currently.

This PR changes the strategy to have multiple reward calls, one per evaluation share submitted. This way, round scores / addresses don't need to be stored at all, which was previously the case as only once all had been submitted we called reward(). I think I'll try that next.

This again reduces gas cost notably.

Before:

[PASS] test_AddMeasurements() (gas: 1462378)
[PASS] test_AdvanceRound() (gas: 1526346)
[PASS] test_SetNextRoundLength() (gas: 1527995)
[PASS] test_SetScores() (gas: 1532795)
[PASS] test_SetScoresEmptyRound() (gas: 1527475)
[PASS] test_SetScoresFractions() (gas: 1584622)
[PASS] test_SetScoresMultipleCalls() (gas: 1990782)
[PASS] test_SetScoresMultipleParticipants() (gas: 1622395)
[PASS] test_SetScoresNotEvaluator() (gas: 1460011)
[PASS] test_SetScoresTooBig() (gas: 1618884)
[PASS] test_SetScoresUnfinishedRound() (gas: 1458138)
[PASS] test_setRoundReward() (gas: 1455441)
[PASS] test_setRoundRewardNotAdmin() (gas: 1458796)

After:

[PASS] test_AddMeasurements() (gas: 1371968)
[PASS] test_AdvanceRound() (gas: 1438075)
[PASS] test_SetNextRoundLength() (gas: 1442070)
[PASS] test_SetScores() (gas: 1432273)
[PASS] test_SetScoresEmptyRound() (gas: 1437637)
[PASS] test_SetScoresFractions() (gas: 1479858)
[PASS] test_SetScoresMultipleCalls() (gas: 1829204)
[PASS] test_SetScoresMultipleParticipants() (gas: 1513397)
[PASS] test_SetScoresNotEvaluator() (gas: 1369601)
[PASS] test_SetScoresTooBig() (gas: 1439384)
[PASS] test_SetScoresUnfinishedRound() (gas: 1367728)
[PASS] test_setRoundReward() (gas: 1365031)
[PASS] test_setRoundRewardNotAdmin() (gas: 1368386)

After this PR, storage is not unbounded any more 🥳 (Except for open unevaluated rounds)

The diff is larger, I suggest to focus on reading the resulting source code https://github.com/Meridian-IE/impact-evaluator/blob/e6b44b7bb659be5eaa7f6337867c7325e07dc3b0/src/ImpactEvaluator.sol.

If there are too many participants, gas costs could still explode - but for the evaluator and not the contract itself. Therefore we should add safeguards there.

[bajtos]

This PR changes the strategy to have multiple reward calls, one per evaluation share submitted.

This new design introduces several new edge cases where only a subset of participants receive the rewards.

Few examples:

• The evaluation service goes rogue, calls setScores() assigning 0.9*MAX_SCORE for one participant, then calls setScores() for other participants. The first participant gets most of the round rewards, everybody else does not receive anything because the subsequent setScores() calls overflow the limit.
  • If we have multiple parties submitting evaluation results, then this allows one dishonest party to claim all rewards.
• The evaluation service submits scores for the first half of participants and then goes down. We pay partial rewards. Can we "heal" this situation by re-running the evaluation function and submitting the missing scores only when the contract state doesn't keep track of individual scores?

It makes me wonder if the cost of these complications (and possible bugs we will have to fix) is worth the savings on gas fees.

Anyhow, I am not opposed to the changes proposed here, just want us to be aware of the ramifications.

[juliangruber]

This PR changes the strategy to have multiple reward calls, one per evaluation share submitted.

This new design introduces several new edge cases where only a subset of participants receive the rewards.

Few examples:

• The evaluation service goes rogue, calls setScores() assigning 0.9*MAX_SCORE for one participant, then calls setScores() for other participants. The first participant gets most of the round rewards, everybody else does not receive anything because the subsequent setScores() calls overflow the limit.

This danger already exists in the current version, the evaluation service can call setScores once where one participant gets 90% and the rest receives 10%. I don't think this PR changes anything about the situation.

• If we have multiple parties submitting evaluation results, then this allows one dishonest party to claim all rewards.

We currently don't yet support multiple evaluation services, we need a proper strategy for this feature in general.

• The evaluation service submits scores for the first half of participants and then goes down. We pay partial rewards. Can we "heal" this situation by re-running the evaluation function and submitting the missing scores only when the contract state doesn't keep track of individual scores?

The evaluation service can now fetch .openRoundIndexes(), so it can tell which rounds are open. Then it can get .openRounds(roundIndex) to get the .totalScore, through which it can check how many scores have already been submitted. It can compare that with its internal state to see if evaluation results are missing. Does that sound ok to you?

It makes me wonder if the cost of these complications (and possible bugs we will have to fix) is worth the savings on gas fees.

Saving gas fees isn't just to save our cost, it's also to prevent the contract from not being able to run any more. With the previous contract, if a malicious party adds 200k participants (just generate 200k addresses and submit 200k measurements) to a round, our contract will be blocked forever (its state is too large, no further transaction is payable) and we can't do anything about it.

[bajtos]

This danger already exists in the current version, the evaluation service can call setScores once where one participant gets 90% and the rest receives 10%. I don't think this PR changes anything about the situation.

IIUC the different between the current and the proposed versions, when there are two setScores calls:

• The current version does not pay any rewards to anybody (evaluation is invalid)
• The proposed version pays 90% of round rewards to the first participant

Anyhow, as long as we are in full control of the evaluation service, this issue does not matter.

I am fine to continue in the direction you proposed here.

The evaluation service can now fetch .openRoundIndexes(), so it can tell which rounds are open. Then it can get .openRounds(roundIndex) to get the .totalScore, through which it can check how many scores have already been submitted. It can compare that with its internal state to see if evaluation results are missing. Does that sound ok to you?

SGTM 👍🏻

The underlying assumption is that the evaluation service runs deterministically and processes the measurements always in the same order.

Saving gas fees isn't just to save our cost, it's also to prevent the contract from not being able to run any more. With the previous contract, if a malicious party adds 200k participants (just generate 200k addresses and submit 200k measurements) to a round, our contract will be blocked forever (its state is too large, no further transaction is payable) and we can't do anything about it.

Ouch, I was not aware of that problem.

I agree we need to fix that!