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

Make churner produce params for all specified quorums #288

Merged
merged 3 commits into from
Mar 1, 2024

Conversation

ian-shim
Copy link
Contributor

@ian-shim ian-shim commented Feb 28, 2024

Why are these changes needed?

Currently, operator software will call either RegisterOperatorWithChurn or RegisterOperator to opt into quorums. But when there are multiple quorums where only a subset of the quorums are full, this call won't work because churner won't produce a kick params for the quorum which aren't full. The contract code to register with churn handles the case where only a subset quorums are full, but it enforces that kick params are specified for all quorums.
This PR makes churner produce kick params even for quorums that aren't full.

Checks

  • I've made sure the lint is passing in this PR.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(

@ian-shim ian-shim force-pushed the churner-include-all-quorums branch 3 times, most recently from d5229f9 to 4e8ca7d Compare February 28, 2024 17:24
@@ -89,14 +93,54 @@ func teardown() {
func TestChurner(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test was not testing anything because no operators have been registered.
Churner simply exited without any OperatorsToChurn

@ian-shim ian-shim changed the title churner register all quorums Make churner produce params for all specified quorums Feb 28, 2024
@ian-shim ian-shim marked this pull request as ready for review February 28, 2024 17:29
@ian-shim ian-shim force-pushed the churner-include-all-quorums branch from 4e8ca7d to b1678f4 Compare February 28, 2024 17:35
api/proto/churner/churner.proto Outdated Show resolved Hide resolved
api/proto/churner/churner.proto Show resolved Hide resolved
@ian-shim ian-shim force-pushed the churner-include-all-quorums branch from 239fe3d to 3d587e3 Compare February 29, 2024 00:12
@ian-shim ian-shim force-pushed the churner-include-all-quorums branch from 3d587e3 to 441c768 Compare February 29, 2024 00:14
@ian-shim ian-shim requested a review from jianoaix February 29, 2024 00:14
if uint32(len(operatorStakes[quorumID])) < operatorSetParams.MaxOperatorCount {
// quorum is not full, so we can continue
continue
if len(operatorStakes[quorumID]) == 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. So this would cause registration to fail if there was one empty quorum and another quorum which was full. Can't we handle this case by just passing in an empty operators to churn object?

  2. Shouldn't we also pass in an empty object whenever the quorum isn't full? It doesn't look like the contracts require the object to be populated in that case, but maybe there's an issue I'm not seeing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that idea.
@0x0aa0 can you chime in here?
Can we have zero address for operator address in operatorKickParams in case the quorum isn't full?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

@0x0aa0 0x0aa0 Feb 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(edited) Yes this does appear to work

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@0x0aa0 , but in these situations, the call to deregisterOperator couldn't be occurring...

Copy link
Collaborator

@mooselumph mooselumph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just one question

@ian-shim ian-shim merged commit 313bd6c into Layr-Labs:master Mar 1, 2024
4 checks passed
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 this pull request may close these issues.

4 participants