Clarify expected behavior if remote signer URL is not provided

[nflaig]

As per spec, importing remote keys does not require to provide the url as part of the remote signer definition as only the pubkey is required.

keymanager-APIs/types/import_remote_signer_definition.yaml

Line 2 in f99e415

required: [pubkey]

At least in Lodestar, we require the url to be present and to be a valid URL, otherwise the key will not be imported due to an error.

What I would like to clarify is the following

• is it by mistake that the url is optional and should it be required instead
• or is the validator client expected to infer the URL from CLI args such as --externalSigner.url

The later is not always possible, at least in Lodestar, as the --externalSigner.url flag is optional and is not strictly required to use a remote signer. Using the keymanager API also allows to import remote signer keys from different remote signers while the CLI flag has a 1:1 relationship.

As the URL will be required when the validator client has to send a remote signing request , the spec should at a minimum clarify the expected behavior if the remote signer URL is not provided.

[dapplion]

In Lighthouse the url field is required

https://github.com/sigp/lighthouse/blob/3058b96f2560f1da04ada4f9d8ba8e5651794ff6/common/eth2/src/lighthouse_vc/std_types.rs#L139-L142

[nflaig]

Based on reviewing other clients (and if I got this correctly)

• Nimbus requires url field (ref)
• Teku has it marked as optional (ref)
• Prysm allows to provide url field but logs a warning and only supports URL from CLI flag (ref)

And as noted in Lodestar and Lighthouse the url field is required. This means 3 out of 5 clients are not spec compliant right now.

We could set it as required in the spec and it wouldn't really require a change on server side but api client implementations that currently assume that the URL can be omitted would require a breaking change but since those api clients already don't work with most server implementations it might be for the best.

[rolfyone]

it was quite intentionally optional... Making it required i'd class as a breaking change. I do understand what you're saying about some clients being required now, but that doesn't validate considering breaking the api...

To me the clear answer is the clients marking it required should probably become compliant with the spec, and if that means they 400 when a url isn't present, at least it's consistent.

I'm fairly sure the reason that it's 'optional' is because a default url from another source is one of the flows.

[nflaig]

I'm fairly sure the reason that it's 'optional' is because a default url from another source is one of the flows.

Might be that I am not fully aware of the use case of this API but so far I have just seen it being misused to fix some other shortcomings clients have, e.g. keeping keys between validator client and remote signer in sync (ChainSafe/lodestar#6624).

For example, Diva has implemented a custom reload script to import missing keys but it does not provide the url right now which is why it fails with Lodestar and also does not remove any keys, meaning if a key is removed from Diva it will cause noisy errors as Lodestar will still try to handle duties for the validator.

In my opinion, the remote keys api only makes sense if you either don't have a 1:1 setup or the remote signer is not configured via CLI.

And if a client has a remote signer configured via CLI flag (e.g. --externalSigner.url) it should make sure to keep the keys in sync itself by updating the in-memory validator store based on response from GET /api/v1/eth2/publicKeys API of the remote signer.

As of right now, only Nimbus keeps the keys in sync but I feel like this would remove a headache for tooling around remote signers if all clients could handle this internally.

Curious what other teams think about this

[nflaig]

and if that means they 400 when a url isn't present, at least it's consistent.

Agreed that it's probably not worth a spec change, and keeping it as optional leaves the option open for the implementation to get it from another source.

If the implementation returns a 400 if the url is not provided it is probably not strictly spec compliant but at least in case of Lodestar it is better than setting a default value which might not be transparent to the user.

This clarifies the expected behavior, although it could be more well-defined in the spec, but likely not worth the effort / coordination considering the limited use cases for this api