feat: implement KeyManager API #1246
Conversation
lib/beacon_api/utils.ex
Outdated
| @@ -31,6 +31,12 @@ defmodule BeaconApi.Utils do | |||
| "0x" <> Base.encode16(binary, case: :lower) | |||
| end | |||
|
|
|||
| def hex_decode("0x" <> binary) do | |||
| with {:ok, decoded} <- Base.decode16(binary, case: :lower) do | |||
There was a problem hiding this comment.
The reason we use this hex_decode utility is because we assume it's correctly stored in our genserver. If that's an assumption we make, instead of using a with here we should call this hex_decode! and match {:ok, decoded} = Base.decode16(binary, case: lower).
The reason for this is that if this decoding fails it will return an error tuple, that we are ignoring in the other function. If that's the case, we should error out directly.
There was a problem hiding this comment.
Actually, this is used to decode the user input. I have updated its definition to the following:
def hex_decode("0x" <> binary), do: Base.decode16(binary, case: :lower)
def hex_decode(binary), do: {:error, "Not valid pubkey: #{inspect(binary)}"}I have also updated the caller function to handle the result properly:
with {:ok, pubkey} <- Utils.hex_decode(pubkey),
..., do:
...
else
{:error, reason} ->
Logger.error("[Keystore] Error removing key. Reason: #{reason}")
end
lib/libp2p_port.ex
Outdated
| @impl GenServer | ||
| def handle_call(:get_keystores, _from, %{validators: []} = state), | ||
| do: {:reply, [], state} |
There was a problem hiding this comment.
Why is this clause necessary? If the validators list is empty, the enum.map will work, and return an empty list too.
There was a problem hiding this comment.
You are right! It is not necessary.
lib/libp2p_port.ex
Outdated
|
|
||
| @impl GenServer | ||
| def handle_call({:delete_validator, pk}, _from, %{validators: validators} = state) do | ||
| case Map.fetch(validators, pk) do |
There was a problem hiding this comment.
Let's use pubkey instead of pk to distinguish these from private keys.
| first_validator = validators |> Map.values() |> List.first() | ||
| validator = Validator.new({first_validator.slot, first_validator.root, keystore}) |
There was a problem hiding this comment.
Nothing to be done in this PR, but I think this shows how weird it is to have these values tied to individual validators. We'll get those values from the store when the other PR is merged.
There was a problem hiding this comment.
Totally! This will be easily handled once we have the other PR ready.
| @type state :: %__MODULE__{ | ||
| slot: Types.slot(), | ||
| epoch: Types.epoch(), | ||
| root: Types.root(), | ||
| duties: Duties.duties(), | ||
| validator: validator(), | ||
| index: non_neg_integer() | nil, | ||
| keystore: Keystore.t(), |
There was a problem hiding this comment.
The Validator is in a weird state (obviously not because of this PR ^^) but this will be a lot better in the coming days. Lot of this is going to change in the refactor I'm working on, I already removed slot, epoch, root and even duties from here, it should be easier by then hopefully.
There was a problem hiding this comment.
I agree. I just hope that this doesn't add too much noise to your PR!
network_params.yaml
Outdated
| validator_count: 1 | ||
| - el_type: geth | ||
| cl_type: lambda | ||
| cl_image: lambda_ethereum_consensus:latest | ||
| use_separate_vc: false | ||
| count: 1 | ||
| validator_count: 32 | ||
| cl_max_mem: 4096 No newline at end of file | ||
| validator_count: 63 |
There was a problem hiding this comment.
np: do we want to have 1 in every lighthouse node and 63 (62 would be sufficient) in ours as the new default config?
There was a problem hiding this comment.
My bad. I changed them only for testing and should have restored these parameters. Nice catch!
Motivation
To be able to integrate with EthDocker.
Description
Implements the Local Key Manager set of endpoints: https://ethereum.github.io/keymanager-APIs/#/Local%20Key%20Manager.
5000as the default port.GET api/openapi.Keystorestruct to hold the keystore data.Validatorstruct.Now we can list, add and delete Validators from
Libp2pPort.Tracked Issues
To stay in sync with the team, I've limited this PR to the changes above, which add the basic structure of the Keystore. I've created the following issues for tasks that are outside the scope of this PR.
Observations
validator-port. If you have already cloned it, a pull may be needed.validator-portwith Kurtosis, thekeymanager-enabledparameter must be set in thenetwork_params.yamlas specified in their docs.Closes #1256