Closed nflaig closed 5 months ago
In Lighthouse the url
field is required
Based on reviewing other clients (and if I got this correctly)
url
field (ref)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.
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.
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 (https://github.com/ChainSafe/lodestar/issues/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
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
As per spec, importing remote keys does not require to provide the
url
as part of the remote signer definition as only thepubkey
is required.https://github.com/ethereum/keymanager-APIs/blob/f99e41560b0db7cb922baa3be52735d611280f8b/types/import_remote_signer_definition.yaml#L2
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
url
is optional and should it be required insteadThe 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.