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

Ejector #493

Merged
merged 13 commits into from
Apr 26, 2024
Merged

Ejector #493

merged 13 commits into from
Apr 26, 2024

Conversation

jianoaix
Copy link
Contributor

@jianoaix jianoaix commented Apr 18, 2024

Why are these changes needed?

Make an Ejector API for automated ejection of nonsigners based on objective onchain nonsigning rates and protocol SLA: https://docs.eigenlayer.xyz/eigenda/operator-guides/requirements/protocol-SLA.

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 :(

@jianoaix jianoaix marked this pull request as draft April 18, 2024 05:33
@mooselumph mooselumph self-requested a review April 18, 2024 18:18
@jianoaix jianoaix marked this pull request as ready for review April 20, 2024 00:13
@jianoaix jianoaix requested review from dmanc and 0x0aa0 April 23, 2024 23:05
@mooselumph mooselumph requested a review from ian-shim April 24, 2024 04:55
@jianoaix jianoaix requested a review from antojoseph April 24, 2024 17:29
Copy link
Contributor

@ian-shim ian-shim left a comment

Choose a reason for hiding this comment

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

👏👏👏

One general question. It's fine for this PR as we want this functionality asap, but I'm not sure if dataapi is the right place to host this method. dataapi is meant to serve (readonly) data, and we eventually want to make it available for external parties to run as well.

"github.com/Layr-Labs/eigensdk-go/logging"
)

// The caller should ensure "stakeShare" is in range (0, 1].
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

stakeShareToSLA converts stake share to the enforced SLA metric. 
`stakeShare` must be in range (0, 1].

so that it gets registered by godoc

}
}

func (e *ejector) eject(ctx context.Context, nonsigningRate *OperatorsNonsigningPercentage, mode string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

mode isn't used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it'll be used after fixing the TODO to update metrics

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually this isn't needed even after fixing TODO -- removed it!

@@ -0,0 +1,124 @@
package dataapi
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a test for ejector

// @Failure 404 {object} ErrorResponse "error: Not found"
// @Failure 500 {object} ErrorResponse "error: Server error"
// @Router /ejector/ejection [get]
func (s *server) EjectOperatorsHandler(c *gin.Context) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a test on server_test

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.

I few questions. I'd encourage us to try to merge this today, even if some of the smaller issues need a followup PR.

Comment on lines 15 to 20
case stakeShare > 0.1:
return 0.975
case stakeShare > 0.05:
return 0.95
default:
return 0.9
Copy link
Collaborator

Choose a reason for hiding this comment

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

Update to new SLA

@@ -292,6 +301,68 @@ func (s *server) Shutdown() error {
return nil
}

// EjectOperatorsHandler godoc
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the plan for how this will be automated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

call it from grafana

Comment on lines 30 to 32
sla := stakeShareToSLA(stakeShare)
perf := (1 - sla) / nonsigningRate
return perf / (1.0 + perf)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a bit uncertain about whether this scoring will give us the right ordering across SLA classes. Interested to know if there was a rationale for this particular scoring. I'll put some further thought into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The breakdown will be:

  • 1 - sla is the error budget,
  • nonsingingRate / (1 - sla) is how badly it has burnt the error budget
  • the inverse is indicating perf
  • then normalize it

@jianoaix
Copy link
Contributor Author

👏👏👏

One general question. It's fine for this PR as we want this functionality asap, but I'm not sure if dataapi is the right place to host this method. dataapi is meant to serve (readonly) data, and we eventually want to make it available for external parties to run as well.

a control plane restructuring will be needed later. For now this will be permissioned at upper layer as well as inside the server (with the secrete token)

@jianoaix jianoaix merged commit 8f24e8a into Layr-Labs:master Apr 26, 2024
5 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.

3 participants