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

feat: Commit boost API - Get Public Keys #1031

Merged
merged 7 commits into from
Oct 24, 2024

Conversation

usmansaleem
Copy link
Contributor

@usmansaleem usmansaleem commented Oct 20, 2024

PR Description

feat: Commit boost API - Get Public Keys. Implementing commit boost API as defined at https://commit-boost.github.io/commit-boost-client/api

Note: This PR does not return compressed SECP public key. This will be handled in PR #1033

  • CLI options.
  • --commit-boost-api-enabled=<true|false>. To enable commit boost API
  • --proxy-keystores-path. Path to the directory that will read and store encrypted proxy keys in v4 (BLS) and v3 (SECP) formats.
  • --proxy-keystores-password-file. The path to file that contains password to encrypt and decrypt proxy keystores.
  • Implement route and handlers for /signer/v1/get_pubkeys
  • Load proxy signers from local directories

Fixed Issue(s)

See #1017

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

Testing

  • I thought about testing these changes in a realistic/non-local environment.

@usmansaleem usmansaleem marked this pull request as ready for review October 20, 2024 10:20
@usmansaleem usmansaleem requested a review from jframe October 20, 2024 23:16
@usmansaleem usmansaleem self-assigned this Oct 20, 2024
@usmansaleem usmansaleem added doc-change-required Indicates an issue or PR that requires doc to be updated TeamCerberus Under active development by TeamCerberus @Consensys labels Oct 20, 2024
@@ -164,6 +165,7 @@ private static class NetworkCliCompletionCandidates extends ArrayList<String> {
@Mixin private PicoKeystoresParameters keystoreParameters;
@Mixin private PicoCliAwsSecretsManagerParameters awsSecretsManagerParameters;
@Mixin private PicoCliGcpSecretManagerParameters gcpSecretManagerParameters;
@Mixin private PicoCommitBoostApiParameters commitBoostApiParameters;
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not quite right to add this into the eth2 API as it has secp signing as well. I think it would be better to add this to the default or base command rather than a subcommand as that's where we want all the other functionality eventually too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the upcoming GenerateProxyKey requires Spec (to calculate genesis fork version) and gvr to compute the root. The spec and gvr is specific to "eth2" mode, hence the commit boost API is specific to eth2 mode as well and won't work in eth1 mode at all.

I can still move it to base level, but we will only be using it in eth2 mode.

// obtain DefaultArtifactSignerProvider as that is the only one we are dealing in eth2 mode.
final ArtifactSignerProvider artifactSignerProvider =
artifactSignerProviders.stream()
.filter(provider -> provider instanceof DefaultArtifactSignerProvider)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does commit boost only work with the DefaultArtifactSignerProvider?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

commit boost only works in eth2 mode. The DefaultArtifactSignerProvider will load both v3 and v4 proxy signers for each of the main BLS signer. The SECPArtifactSigner that is used in eth1 mode (the eth address is used as identifier there) is not applicable for commit boost routes and handlers.

&& identifierPath.toFile().isDirectory()) {
final Path v4Dir = identifierPath.resolve("v4");

if (v4Dir.toFile().canRead() && v4Dir.toFile().isDirectory()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are separate directories for each proxy key version needed? Pretty sure the file formats have a version

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 allow easier loading using existing BLS and SECP bulkloading utility classes. Otherwise, I would have to parse json, check the version, and then invoke BLS or SECP decryption logic. Since the proxy keys Generate logic (the other PR) is controlled programmatically, the user is not involved in placing these files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

each delegator (BLS Signer) can have BLS Proxy Signers and SECP Proxy Signers. Hence:

/commit_boost_keystores_dir
..../0xblskey0..n/
.........../0xblskey0..n/v3/0xproxy_secp_key0...n
.........../0xblskey0..n/v4/0xproxy_bls_key0...n

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as discussed, updated to use BLS and SECP256K1 as subdirectories for each signer public key.

@usmansaleem usmansaleem requested a review from jframe October 24, 2024 03:58
@usmansaleem usmansaleem enabled auto-merge (squash) October 24, 2024 06:11
@usmansaleem usmansaleem merged commit 68db6b7 into Consensys:master Oct 24, 2024
6 checks passed
@usmansaleem usmansaleem deleted the commit_boost_support branch October 24, 2024 08:36
@alexandratran alexandratran removed the doc-change-required Indicates an issue or PR that requires doc to be updated label Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TeamCerberus Under active development by TeamCerberus @Consensys
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants