Minimize storage

[juliangruber]

This "radical" PR removes as much internal storage as possible, relying on Events as a method of cost saving and also reducing complexity. See gas comparison, before:

[PASS] test_AddMeasurements() (gas: 1911384)
[PASS] test_AdvanceRound() (gas: 1901709)
[PASS] test_SetNextRoundLength() (gas: 1899204)
[PASS] test_SetScores() (gas: 2007204)
[PASS] test_SetScoresEmptyRound() (gas: 1945811)
[PASS] test_SetScoresFractions() (gas: 2069759)
[PASS] test_SetScoresMultipleParticipants() (gas: 2125921)
[PASS] test_SetScoresNotEvaluator() (gas: 1855247)
[PASS] test_setRoundReward() (gas: 1850238)
[PASS] test_setRoundRewardNotAdmin() (gas: 1853482)

after:

[PASS] test_AddMeasurements() (gas: 1166367)
[PASS] test_AdvanceRound() (gas: 1230516)
[PASS] test_SetNextRoundLength() (gas: 1228349)
[PASS] test_SetScores() (gas: 1222263)
[PASS] test_SetScoresEmptyRound() (gas: 1190229)
[PASS] test_SetScoresFractions() (gas: 1269296)
[PASS] test_SetScoresMultipleParticipants() (gas: 1302958)
[PASS] test_SetScoresNotEvaluator() (gas: 1164214)
[PASS] test_setRoundReward() (gas: 1159622)
[PASS] test_setRoundRewardNotAdmin() (gas: 1162977)

In some calls, this is almost a 50% cost reduction. And since we only keep track of the current open rounds (ie rounds that have started but for which evaluation results haven't been submitted yet), which should on average just be 2, the contract's state won't grow.

Unless I missed something, the only impact this will have on our current running system is that we can't track per-round measurement count from the IE any more. This shouldn't be a problem, as we also have this state in the measurement and evaluate services. Listening for and querying events is already the main method of getting data out of the system. I also removed the method getParticipantScores, if a participant wants to see their score in a round they can look up the matching setScores() transaction.

[bajtos]

I like the idea to reduce state size and gas costs 👏🏻

I don't have enough knowledge about smart contracts to be able to fully review the changes. Perhaps @AmeanAsad can help us?

Before we continue, I would like us to be sure that we can use past events to reconstruct the state (e.g. current round) - see my comment below.

[bajtos]

@juliangruber @patrickwoodhead I think this is such a major change that we should run it through another security audit, WDYT?

[juliangruber]

@juliangruber @patrickwoodhead I think this is such a major change that we should run it through another security audit, WDYT?

Agreed, once this and other changes have been merged, and running in stating for a bit, we should consider another audit

[bajtos]

@juliangruber could you please merge main and resolve the conflicts before we do the final review?

[AmeanAsad]

Hey @juliangruber trying to understand the openRounds concept here. Is there a scenario where we have two open rounds where evaluations are not submitted at the same time? Shouldn't the round process be synchronous?

The scenario im imagining is if the evaluations are being processed for one round and participants start submitting measurements for the following round?

[juliangruber]

whitespace changes from formatter

[juliangruber]

we need to temporarily keep these now, for the case where the evaluator submits scores over multiple calls, and we need to merge them afterwards

[juliangruber]

Hey @juliangruber trying to understand the openRounds concept here. Is there a scenario where we have two open rounds where evaluations are not submitted at the same time? Shouldn't the round process be synchronous?

The scenario im imagining is if the evaluations are being processed for one round and participants start submitting measurements for the following round?

This can happen if the evaluation service errors/fails, but the new rounds are still created so that the system can continue functioning. Plus, you will always have at least two open rounds when you switch to the next one, since it takes a bit of time for evaluation results (which always come in after the round ends) to come in

[juliangruber]

I hope this correctly cleans up the space. If it doesn't, we can convert openRounds to a ring buffer, and access it like openRounds[currentRoundIndex % MAX_OPEN_ROUNDS]. This way storage will never grow, but of course we introduce a limit to open rounds, which could be problematic if the evaluate service fails for multiple rounds.

I think the ring buffer will be safer and we have to prioritize that. If a few rounds don't pay rewards, it's not the end of the world. If the contact becomes too expensive to run, it is.

Wdyt @AmeanAsad @bajtos?

[bajtos]

I had a similar question on my mind. What happens when the evaluation service goes down on Friday evening, and we don't notice until Monday morning? There will be no evaluations submitted for more than 100 rounds.

The ring buffer will not help here unless the evaluation service comes back within MAX_OPEN_ROUNDS and submits evaluations for older rounds that haven't been evaluated yet. (IIRC, we call the second part "self-healing"; it's not implemented yet, and not even on the roadmap AFAICT.)

Thoughts?

[juliangruber]

Agreed, but is that worth it making the good case significantly more expensive, if we can't properly clean up this way?

I need to do more research on how to free up space with nested structures

[juliangruber]

Let's keep this as is, I think it's good enough

[juliangruber]

I'm going to merge and deploy this now, so that we can start gathering usage data.

Hindsight review would be appreciated @bajtos @AmeanAsad 🙏