Skip to content
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

IndexRegistry does not update currentOperatorIndex when an operator is deregistered, in certain cases #126

Closed
wadealexc opened this issue Dec 29, 2023 · 5 comments · Fixed by #139
Assignees

Comments

@wadealexc
Copy link
Collaborator

Came across this while working on documentation:

uint32 newOperatorCount = _decreaseOperatorCount(quorumNumber);
bytes32 lastOperatorId = _popLastOperator(quorumNumber, newOperatorCount);
if (operatorId != lastOperatorId) {
_assignOperatorToIndex({
operatorId: lastOperatorId,
quorumNumber: quorumNumber,
index: indexOfOperatorToRemove
});

If operatorId == lastOperatorId, we never assign currentOperatorIndex[operatorId].

To fix this, we can change _popLastOperator or add an else condition

@wadealexc wadealexc self-assigned this Dec 29, 2023
@wadealexc
Copy link
Collaborator Author

Sidenote, I feel like I better understand what the IndexRegistry is doing, now that I've written a bunch of docs (esp about the BLSSigChecker).

And now going back through the IndexRegistry, I'm regretting the quantity of helper functions I added when I did refactoring a few months ago 😬

@wadealexc
Copy link
Collaborator Author

wadealexc commented Dec 29, 2023

Also sidenote, thinking about a fix for this issue, I'm remembering having had this conversation already.

I think the gist was that we don't have an "index does not exist" index - and that operators that have never registered have currentOperatorIndex of 0 by default... so we could set the value to 0 in this case, but that's also not technically correct, because there is actually an Operator with that index somewhere?

I'm not sure what the solution is, but maybe some context on how currentOperatorIndex is used offchain would help?

@wadealexc
Copy link
Collaborator Author

actually, noting here that currentOperatorIndex for the deregistering operator is actually just never updated, in any case -- same reasons as above, I guess. Not sure what we would give it, because there's no right answer.

@wadealexc
Copy link
Collaborator Author

wadealexc commented Dec 29, 2023

Ideally we would have INDEX_DOES_NOT_EXIST, but in order to keep things consistent, it'd need to equal 0 so that never-registered operators have it by default.

We'd then need to 1-index everything in the IndexRegistry, and that's definitely too much work to do at this stage.

@ChaoticWalrus
Copy link
Contributor

We discussed some offline, but to summarize my views, I think this behavior is OK (pretty much for the reasons you've outlined above), just needs documentation on the proper usage -- I'd hate to see someone integrate with this code incorrectly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants