Fix number of inferers considered in the reward cycle #691
Conversation
fernandofcampos
force-pushed
the
PROTO-2851
branch
from
a39e04b
to
ba1eb8c
Compare
fernandofcampos
force-pushed
the
PROTO-2851
branch
from
ba1eb8c
to
c091ee2
Compare
| // Get active inferers for topic | ||
| workerAddresses, err := k.GetActiveInferersForTopic(ctx, topic.Id) | ||
| if err != nil { | ||
| return errorsmod.Wrap(err, "error getting active inferers for topic") | ||
| } |
There was a problem hiding this comment.
To avoid potential unnecessary queries, I'd move this below GetLowestInfererScoreEma since it's just used after getting the lowest score.
| // Check if the inferer with lowest score is active before removing it, because remove will not fail if the inferer is not active | ||
| isActive, err := k.IsActiveInferer(ctx, topic.Id, lowestEmaScore.Address) | ||
| if err != nil { | ||
| return errorsmod.Wrap(err, "error checking if inferer is active") | ||
| } | ||
| if !isActive { | ||
| return errors.New("inferer with lowest score is not active") | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm wondering if it should be handled this way. If we are trying to remove an active inferer that doesn't exist, it's a bug on our end. Maybe it's just a sanity check to avoid disrupting the reward flow, but if this happens, we should be made aware somehow. Lmk your thoughts.
There was a problem hiding this comment.
This is an extra check indeed. "Defense in depth" applies here imo. Could def argue it's excessive, or warranted. imo it's good to have, much like our other excessive checks
| // If there are no lowest inferer score ema, it means it is the first inference for the topic | ||
| if !found { | ||
| lowestEmaScore = previousEmaScore | ||
| err = k.SetLowestInfererScoreEma(ctx, topic.Id, lowestEmaScore) | ||
| if err != nil { | ||
| return errorsmod.Wrap(err, "error setting lowest inferer score ema") | ||
| } | ||
| } |
There was a problem hiding this comment.
I think this can be removed; this condition is already checked in the code below.
It will avoid double persistence.
There was a problem hiding this comment.
Also please double-check if we still need UpdateLowestScoreFromInfererAddresses in this new implementation, I can see some cases that we persist the same lowest score 3 times.
There was a problem hiding this comment.
this "not found" check applies for the very first time the topic begins an epoch
There was a problem hiding this comment.
We still do need UpdateLowestScoreFromInfererAddresses I believe because if we remove an actor, it may be any actor => we don't know the new lowest actor
There was a problem hiding this comment.
this "not found" check applies for the very first time the topic begins an epoch
the condition below is already covering this case: if uint64(len(workerAddresses)) == 0
Purpose of Changes and their Description
k.lowestInfererScoreEmawas not being updated on a new Epoch. Then, after the 32 slots get filled up, and when the 33rd inference arrives, it was checked against the value ink.lowestInfererScoreEmafrom previous Epoch.If the new inferer has a greater score, it gets added and the old one gets removed. But since the old one was from the previous epoch (that is, the worker in the stored
Scorestruct of lowest score), it does not exist anymore, and the function that removes the inferer won't return and error if it does not exist. So we don't remove any inferer, and we add a new one. That's how we end up with 33.This PR effectively resets that stored lowest score (stored by topic) upon the start of a new worker submission window. Therefore, whenever one attempts to move a worker out of the active set, we can be assured the worker is actually a part of the topic's current active set.
Link(s) to Ticket(s) or Issue(s) resolved by this PR
https://linear.app/alloralabs/issue/PROTO-2851/fix-number-of-inferers-considered-in-the-reward-cycle
Are these changes tested and documented?
Unreleasedsection ofCHANGELOG.md?Still Left Todo
Fill this out if this is a Draft PR so others can help.