Avoid registration of non-active validators #7423
Conversation
validator/client/src/main/java/tech/pegasys/teku/validator/client/ValidatorRegistrator.java
Outdated
Show resolved
Hide resolved
|
Finished reworking it. Now everything in batches. Tried to keep all existing logic. |
|
Still testing, don't merge |
|
as discussed on standup:
it also happens on startup via |
|
Hmm good point, so we make duplicate calls. |
|
Yeah, nice catch, detaching it to |
|
Rework finished, some notes:
|
| @@ -300,7 +301,36 @@ public SafeFuture<Void> prepareBeaconProposer( | |||
|
|
|||
| public SafeFuture<Void> registerValidators( | |||
| SszList<SignedValidatorRegistration> validatorRegistrations) { | |||
| return validatorApiChannel.registerValidators(validatorRegistrations); | |||
| return validatorApiChannel | |||
There was a problem hiding this comment.
I have 2 doubts here:
1- putting filtering here means that it will only be applied when registering from a remote VC, and it will not be applied in single process.
I see this ValidatorDataProvider as a very thin layer. We should not have business logic unless we really want to differentiate between in process and remote VC handling.
2- if we filter upfront it means that validators in unknown status will not go in our BN data structure. Which means that, if VC doesn't keep calling the registration endpoint, the validator could become known at some point in future. It is an edge case but in theory we loose this info.
There was a problem hiding this comment.
sorry for 2-. It is totally irrelevant considering what we are trying to achieve with this PR :)
| @@ -69,8 +69,11 @@ public void handleRequest(final RestApiRequest request) throws JsonProcessingExc | |||
| .handle( | |||
| (__, error) -> { | |||
| if (error != null) { | |||
| return AsyncApiResponse.respondWithError( | |||
| SC_INTERNAL_SERVER_ERROR, error.getMessage()); | |||
| final String message = | |||
There was a problem hiding this comment.
what about using ExceptionUtil::getRootCauseMessage or add another method there if there is nothing currently fitting the need
| @Override | ||
| public void onNewValidatorStatuses( | ||
| final Map<BLSPublicKey, ValidatorStatus> newValidatorStatuses) { | ||
| Map<BLSPublicKey, ValidatorStatus> oldValidatorStatuses = |
| for (Map.Entry<BLSPublicKey, ValidatorStatus> entry : newValidatorStatuses.entrySet()) { | ||
| BLSPublicKey key = entry.getKey(); | ||
| ValidatorStatus newStatus = entry.getValue(); | ||
| ValidatorStatus oldStatus = oldValidatorStatuses.get(key); | ||
|
|
There was a problem hiding this comment.
entry and other locals -> final
| } | ||
|
|
||
| private void printValidatorStatusSummary(Map<BLSPublicKey, ValidatorStatus> validatorStatuses) { | ||
| Map<ValidatorStatus, AtomicInteger> validatorStatusCount = new HashMap<>(); |
| } | ||
|
|
||
| private void printValidatorStatusesOneByOne( | ||
| Map<BLSPublicKey, ValidatorStatus> validatorStatuses) { |
|
|
||
| private void printValidatorStatusesOneByOne( | ||
| Map<BLSPublicKey, ValidatorStatus> validatorStatuses) { | ||
| for (BLSPublicKey publicKey : validators.getPublicKeys()) { |
|
Reworked to use subscribers model, so |
| import tech.pegasys.teku.spec.schemas.ApiSchemas; | ||
| import tech.pegasys.teku.spec.signatures.Signer; | ||
|
|
||
| public class ValidatorRegistrationSigningService { |
There was a problem hiding this comment.
seems like not a real Service. How about ValidatorRegistrationCreator?
| metricsSystem, validators, validatorApiChannel, asyncRunner); | ||
| final ValidatorStatusLogger validatorStatusLogger = new ValidatorStatusLogger(validators); | ||
| validatorStatusProvider.subscribeNewValidatorStatuses( | ||
| validatorStatusLogger::onNewValidatorStatuses); |
There was a problem hiding this comment.
what about calling the method onRefreshedValidatorStatuses? otherwise it could seem that it is called only when you have a new status. The same applies to the subscription interface
| @@ -115,11 +128,44 @@ void shouldUpdateMetricsForValidatorStatuses() { | |||
| assertThat(gauge.getValue(ValidatorStatus.active_ongoing.name())) | |||
| .isEqualTo(OptionalDouble.of(0)); | |||
|
|
|||
| logger.checkValidatorStatusChanges(); | |||
| provider.onSlot(spec.computeStartSlotAtEpoch(UInt64.ONE).plus(1)); | |||
There was a problem hiding this comment.
maybe you should change the name of this test. It is testing that onSlot we do the job of updating statuses, and you could add the check that the subscriber is called.
PR Description
OwnedValidators.getActiveValidators()name because it's misleading. I'd rename to exclude word active, but it will lead to changes in several classes, so I skipped it to omit unnecessary changes in this PR and could do a separate PR with this change.I appreciate any ideas how could I test it thoroughly, because it's a sensitive part of the code.
Fixed Issue(s)
Fixes #7382
Fixes #6290
Documentation
doc-change-requiredlabel to this PR if updates are required.Changelog