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

add api key bls remote signer #1128

Merged
merged 3 commits into from
Jan 18, 2025
Merged

Conversation

shrimalmadhur
Copy link
Contributor

Why are these changes needed?

Checks

  • 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.
  • I've checked the new test coverage and the coverage percentage didn't drop.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(

Comment on lines +190 to +192
if blsRemoteSignerEnabled && blsSignerAPIKey == "" {
return nil, fmt.Errorf("BLS signer API key is required if BLS remote signer is enabled")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this requirement predicated on a new version of Cerberus that also requires API key? I've got remote BLS running on my testnet now with Cerberus. When I upgrade my node, do I need to simultaneously upgrade Cerberus? If I upgrade Cerberus first, will my node fail to remote sign until upgraded with this change?

Copy link
Contributor Author

@shrimalmadhur shrimalmadhur Jan 18, 2025

Choose a reason for hiding this comment

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

yes basically you will have to clean import on the new version of cerberus - since it sets up a DB and then persists some metadata for the keys - if you are already running cerberus. You will have to upgrade both together
not ideal but given only few folks (2-3 folks outside us) are testing right now on testnet it's an okay process. wdyt?

Basically steps

  • Upgrade Cerberus (with postgres)
  • Reimport key which will give you API key
  • Add that in .env on eigenda with latest eigenda release
  • restart eigenda

there could be missed batches in between which won't be able to sign due to this inconsistency

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, as long as current testnet users are in the loop, seems ok.

If possible might make sense to disable remote singing on node as first step - reverting to local BLS, to allow for Cerberus upgrade validation outside production path. Or maybe recommend users spin up new Cerberus alongside old Cerberus on new port so that they can import and validate new Cerberus is ready before pulling the switch. Then update .env and bump node release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's a good point. I will write an upgrade guide in release notes with these options and share. good call on that.

@shrimalmadhur shrimalmadhur marked this pull request as ready for review January 18, 2025 00:28
@shrimalmadhur shrimalmadhur merged commit 7850088 into master Jan 18, 2025
9 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.

2 participants