Multiple BN HTTP resolver #13433
Multiple BN HTTP resolver #13433
Conversation
validator/client/service.go
Outdated
| @@ -237,7 +239,7 @@ func (v *ValidatorService) Start() { | |||
| } | |||
|
|
|||
| v.validator = valStruct | |||
| go run(v.ctx, v.validator) | |||
| go run(v.ctx, v.validator, hosts) | |||
There was a problem hiding this comment.
if there are get host and update host functions on the validator, what about just adding hosts to the validator client? instead of passing it down as arguments?
There was a problem hiding this comment.
Ah, this makes a lot of sense. Instead of updating the host in the goroutine, we can have an UpdateHost() function without arguments that updates the host internally.
validator/client/runner.go
Outdated
| } | ||
| go func() { | ||
| // deadline set for 1 epoch from call to not overlap. | ||
| epochDeadline := v.SlotDeadline(slot + params.BeaconConfig().SlotsPerEpoch - 1) |
There was a problem hiding this comment.
this deadline needs to change as we don't have a check if it's epoch start so this deadline will be wrong. I think it might be better to just have a default value.
validator/client/runner.go
Outdated
| if url == v.RetrieveHost() { | ||
| next := (i + 1) % len(hosts) | ||
| log.Infof("Beacon node at %s is not responding, switching to %s", url, hosts[next]) | ||
| v.UpdateHost(hosts[next]) |
There was a problem hiding this comment.
don't you need to check health again after the host is updated and break until the next ticker if it is not healthy?
validator/client/runner.go
Outdated
|
|
||
| km, err := v.Keymanager() | ||
| if err != nil { | ||
| log.WithError(err).Fatal("Could not get keymanager") |
There was a problem hiding this comment.
something doesn't feel right here about logging a fatal, it's needed for the push proposer settings but not sure if the whole thing should break if it can't get the keymanager
There was a problem hiding this comment.
I would say not being able to obtain the keymanager is very bad, but yeah maybe just log an error and return? Not 100% sure. This should realistically never happen anyway.
Co-authored-by: Radosław Kapka <rkapka@wp.pl>
What type of PR is this?
Other
What does this PR do? Why is it needed?
This pull request aims to enhance the HTTP client to support multiple endpoints that the nodes can switch between when necessary.
The endpoints host:port will be specified through a flag. For instance, the flag could be set to
"127.0.0.1:4000, 127.0.0.1:4001"to define two endpoints.We are adding this as we have a similar setup for GRPC that we are going to eventually deprecate. And therefore require it for the HTTP client.
Other notes for review
Currently the number of files changed is massive as I had to regenerate gomock with another library, when this PR merges #13639 these should clear up.