Provide meaningful rejection reasons (#4179)

data/beaconrestapi/src/test/java/tech/pegasys/teku/beaconrestapi/handlers/v1/beacon/PostAttesterSlashingTest.java

@@ -62,7 +62,7 @@ void shouldReturnBadRequestIfAttesterSlashingIsInvalid() throws Exception {
         new AttesterSlashing(dataStructureUtil.randomAttesterSlashing());
     when(context.body()).thenReturn(jsonProvider.objectToJSON(slashing));
     when(provider.postAttesterSlashing(slashing))
-        .thenReturn(SafeFuture.completedFuture(InternalValidationResult.REJECT));
+        .thenReturn(SafeFuture.completedFuture(InternalValidationResult.reject("Nope")));
     handler.handle(context);
 
     verify(provider).postAttesterSlashing(slashing);

data/beaconrestapi/src/test/java/tech/pegasys/teku/beaconrestapi/handlers/v1/beacon/PostProposerSlashingTest.java

@@ -62,7 +62,7 @@ void shouldReturnBadRequestIfProposerSlashingIsInvalid() throws Exception {
         new ProposerSlashing(dataStructureUtil.randomProposerSlashing());
     when(context.body()).thenReturn(jsonProvider.objectToJSON(slashing));
     when(provider.postProposerSlashing(slashing))
-        .thenReturn(SafeFuture.completedFuture(InternalValidationResult.REJECT));
+        .thenReturn(SafeFuture.completedFuture(InternalValidationResult.reject("Oops")));
     handler.handle(context);
 
     verify(provider).postProposerSlashing(slashing);

data/beaconrestapi/src/test/java/tech/pegasys/teku/beaconrestapi/handlers/v1/beacon/PostVoluntaryExitTest.java

@@ -62,7 +62,7 @@ void shouldReturnBadRequest_ifVoluntaryExitInvalid() throws Exception {
         new SignedVoluntaryExit(dataStructureUtil.randomSignedVoluntaryExit());
     when(context.body()).thenReturn(jsonProvider.objectToJSON(exit));
     when(provider.postVoluntaryExit(exit))
-        .thenReturn(SafeFuture.completedFuture(InternalValidationResult.REJECT));
+        .thenReturn(SafeFuture.completedFuture(InternalValidationResult.reject("Oh dear")));
     handler.handle(context);
 
     verify(provider).postVoluntaryExit(exit);

ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/operations/validation/ProposerSlashingValidator.java

@@ -71,7 +71,8 @@ public enum ProposerSlashingInvalidReason implements OperationInvalidReason {
     PROPOSER_INDICES_DIFFERENT("Header proposer indices don't match"),
     SAME_HEADER("Headers are not different"),
     INVALID_PROPOSER("Invalid proposer index"),
-    PROPOSER_NOT_SLASHABLE("Proposer is not slashable");
+    PROPOSER_NOT_SLASHABLE("Proposer is not slashable"),
+    INVALID_SIGNATURE("Slashing fails signature verification");
 
     private final String description;
 

ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/operations/validation/VoluntaryExitValidator.java

@@ -88,7 +88,8 @@ public enum ExitInvalidReason implements OperationInvalidReason {
     VALIDATOR_INACTIVE("Validator is not active"),
     EXIT_INITIATED("Validator has already initiated exit"),
     SUBMITTED_TOO_EARLY("Specified exit epoch is still in the future"),
-    VALIDATOR_TOO_YOUNG("Validator has not been active long enough");
+    VALIDATOR_TOO_YOUNG("Validator has not been active long enough"),
+    INVALID_SIGNATURE("Signature is invalid");
 
     private final String description;
 

ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/OperationPool.java

@@ -53,7 +53,8 @@ public SszList<T> getItemsForBlock(BeaconState stateAtBlockSlot) {
     // evicting the oldest entries when the size is exceeded as we only ever access via iteration.
     return operations.stream()
         .limit(schema.getMaxLength())
-        .filter(item -> operationValidator.validateForStateTransition(stateAtBlockSlot, item))
+        .filter(
+            item -> operationValidator.validateForStateTransition(stateAtBlockSlot, item).isEmpty())
         .collect(schema.collector());
   }
 

ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/synccommittee/SyncCommitteeMessageValidator.java

@@ -16,7 +16,7 @@
 import static java.util.stream.Collectors.toList;
 import static tech.pegasys.teku.statetransition.validation.InternalValidationResult.ACCEPT;
 import static tech.pegasys.teku.statetransition.validation.InternalValidationResult.IGNORE;
-import static tech.pegasys.teku.statetransition.validation.InternalValidationResult.REJECT;
+import static tech.pegasys.teku.statetransition.validation.InternalValidationResult.reject;
 import static tech.pegasys.teku.util.config.Constants.VALID_SYNC_COMMITTEE_MESSAGE_SET_SIZE;
 
 import java.util.List;
@@ -70,10 +70,10 @@ public SafeFuture<InternalValidationResult> validate(
     final Optional<SyncCommitteeUtil> maybeSyncCommitteeUtil =
         spec.getSyncCommitteeUtil(message.getSlot());
     if (maybeSyncCommitteeUtil.isEmpty()) {
-      LOG.trace(
-          "Rejecting sync committee message because the fork active at slot {} does not support sync committees",
-          message.getSlot());
-      return SafeFuture.completedFuture(REJECT);
+      return SafeFuture.completedFuture(
+          reject(
+              "Rejecting sync committee message because the fork active at slot %s does not support sync committees",
+              message.getSlot()));
     }
     final SyncCommitteeUtil syncCommitteeUtil = maybeSyncCommitteeUtil.get();
 
@@ -135,8 +135,9 @@ private SafeFuture<InternalValidationResult> validateWithState(
     // committee, i.e. state.validators[sync_committee_message.validator_index].pubkey in
     // state.current_sync_committee.pubkeys.
     if (assignedSubcommittees.isEmpty()) {
-      LOG.trace("Rejecting sync committee message because validator is not in the sync committee");
-      return SafeFuture.completedFuture(REJECT);
+      return SafeFuture.completedFuture(
+          reject(
+              "Rejecting sync committee message because validator is not in the sync committee"));
     }
 
     // For messages received via gossip, it has to be unique based on the subnet it was on
@@ -162,15 +163,15 @@ private SafeFuture<InternalValidationResult> validateWithState(
         && !assignedSubcommittees
             .getAssignedSubcommittees()
             .contains(validateableMessage.getReceivedSubnetId().getAsInt())) {
-      LOG.trace("Rejecting sync committee message because subnet id is incorrect");
-      return SafeFuture.completedFuture(REJECT);
+      return SafeFuture.completedFuture(
+          reject("Rejecting sync committee message because subnet id is incorrect"));
     }
 
     final Optional<BLSPublicKey> maybeValidatorPublicKey =
         spec.getValidatorPubKey(state, message.getValidatorIndex());
     if (maybeValidatorPublicKey.isEmpty()) {
-      LOG.trace("Rejecting sync committee message because the validator index is unknown");
-      return SafeFuture.completedFuture(REJECT);
+      return SafeFuture.completedFuture(
+          reject("Rejecting sync committee message because the validator index is unknown"));
     }
 
     // [REJECT] The message is valid for the message beacon_block_root for the validator
@@ -183,13 +184,11 @@ private SafeFuture<InternalValidationResult> validateWithState(
         .thenApply(
             signatureValid -> {
               if (!signatureValid) {
-                LOG.trace("Rejecting sync committee message because the signature is invalid");
-                return REJECT;
+                return reject("Rejecting sync committee message because the signature is invalid");
               }
               if (!seenIndices.addAll(uniquenessKeys)) {
-                LOG.trace(
+                return reject(
                     "Ignoring sync committee message as a duplicate was processed during validation");
-                return IGNORE;
               }
               return ACCEPT;
             });

ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/validation/AggregateAttestationValidator.java

@@ -14,9 +14,12 @@
 package tech.pegasys.teku.statetransition.validation;
 
 import static java.lang.Math.toIntExact;
+import static tech.pegasys.teku.infrastructure.async.SafeFuture.completedFuture;
 import static tech.pegasys.teku.spec.datastructures.util.BeaconStateUtil.compute_epoch_at_slot;
 import static tech.pegasys.teku.spec.datastructures.util.BeaconStateUtil.compute_signing_root;
 import static tech.pegasys.teku.spec.datastructures.util.BeaconStateUtil.get_domain;
+import static tech.pegasys.teku.statetransition.validation.InternalValidationResult.ignore;
+import static tech.pegasys.teku.statetransition.validation.InternalValidationResult.reject;
 import static tech.pegasys.teku.util.config.Constants.DOMAIN_SELECTION_PROOF;
 import static tech.pegasys.teku.util.config.Constants.VALID_AGGREGATE_SET_SIZE;
 
@@ -81,13 +84,11 @@ public SafeFuture<InternalValidationResult> validate(final ValidateableAttestati
         new AggregatorIndexAndEpoch(
             aggregateAndProof.getIndex(), compute_epoch_at_slot(aggregateSlot));
     if (receivedAggregatorIndexAndEpochs.contains(aggregatorIndexAndEpoch)) {
-      LOG.trace("Ignoring duplicate aggregate");
-      return SafeFuture.completedFuture(InternalValidationResult.IGNORE);
+      return completedFuture(ignore("Ignoring duplicate aggregate"));
     }
 
     if (receivedValidAggregations.contains(attestation.hash_tree_root())) {
-      LOG.trace("Ignoring duplicate aggregate based on hash tree root");
-      return SafeFuture.completedFuture(InternalValidationResult.IGNORE);
+      return completedFuture(ignore("Ignoring duplicate aggregate based on hash tree root"));
     }
 
     final BatchSignatureVerifier signatureVerifier = new BatchSignatureVerifier();
@@ -96,15 +97,15 @@ public SafeFuture<InternalValidationResult> validate(final ValidateableAttestati
             aggregateInternalValidationResult -> {
               if (aggregateInternalValidationResult.isNotProcessable()) {
                 LOG.trace("Rejecting aggregate because attestation failed validation");
-                return SafeFuture.completedFuture(aggregateInternalValidationResult);
+                return completedFuture(aggregateInternalValidationResult);
               }
 
               return recentChainData
                   .retrieveBlockState(aggregate.getData().getBeacon_block_root())
                   .thenCompose(
                       maybeState ->
                           maybeState.isEmpty()
-                              ? SafeFuture.completedFuture(Optional.empty())
+                              ? completedFuture(Optional.empty())
                               : attestationValidator.resolveStateForAttestation(
                                   aggregate, maybeState.get()))
                   .thenApply(
@@ -118,8 +119,7 @@ public SafeFuture<InternalValidationResult> validate(final ValidateableAttestati
                         final Optional<BLSPublicKey> aggregatorPublicKey =
                             spec.getValidatorPubKey(state, aggregateAndProof.getIndex());
                         if (aggregatorPublicKey.isEmpty()) {
-                          LOG.trace("Rejecting aggregate with invalid index");
-                          return InternalValidationResult.REJECT;
+                          return reject("Rejecting aggregate with invalid index");
                         }
 
                         if (!isSelectionProofValid(
@@ -128,8 +128,7 @@ public SafeFuture<InternalValidationResult> validate(final ValidateableAttestati
                             state,
                             aggregatorPublicKey.get(),
                             aggregateAndProof.getSelection_proof())) {
-                          LOG.trace("Rejecting aggregate with incorrect selection proof");
-                          return InternalValidationResult.REJECT;
+                          return reject("Rejecting aggregate with incorrect selection proof");
                         }
 
                         final List<Integer> beaconCommittee =
@@ -144,37 +143,31 @@ public SafeFuture<InternalValidationResult> validate(final ValidateableAttestati
                             .getValidatorsUtil()
                             .isAggregator(
                                 aggregateAndProof.getSelection_proof(), aggregatorModulo)) {
-                          LOG.trace(
+                          return reject(
                               "Rejecting aggregate because selection proof does not select validator as aggregator");
-                          return InternalValidationResult.REJECT;
                         }
                         if (!beaconCommittee.contains(
                             toIntExact(aggregateAndProof.getIndex().longValue()))) {
-                          LOG.trace(
-                              "Rejecting aggregate because attester is not in committee. Should have been one of {}",
+                          return reject(
+                              "Rejecting aggregate because attester is not in committee. Should have been one of %s",
                               beaconCommittee);
-                          return InternalValidationResult.REJECT;
                         }
 
                         if (!validateSignature(
                             signatureVerifier, signedAggregate, state, aggregatorPublicKey.get())) {
-                          LOG.trace("Rejecting aggregate with invalid signature");
-                          return InternalValidationResult.REJECT;
+                          return reject("Rejecting aggregate with invalid signature");
                         }
 
                         if (!signatureVerifier.batchVerify()) {
-                          LOG.trace("Rejecting aggregate with invalid batch signature");
-                          return InternalValidationResult.REJECT;
+                          return reject("Rejecting aggregate with invalid batch signature");
                         }
 
                         if (!receivedAggregatorIndexAndEpochs.add(aggregatorIndexAndEpoch)) {
-                          LOG.trace("Ignoring duplicate aggregate");
-                          return InternalValidationResult.IGNORE;
+                          return ignore("Ignoring duplicate aggregate");
                         }
 
                         if (!receivedValidAggregations.add(attestation.hash_tree_root())) {
-                          LOG.trace("Ignoring duplicate aggregate based on hash tree root");
-                          return InternalValidationResult.IGNORE;
+                          return ignore("Ignoring duplicate aggregate based on hash tree root");
                         }
 
                         return aggregateInternalValidationResult;

ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/validation/AttestationValidator.java

@@ -99,8 +99,9 @@ private InternalValidationResult addAndCheckFirstValidAttestation(final Attestat
   private InternalValidationResult singleAttestationChecks(final Attestation attestation) {
     // The attestation is unaggregated -- that is, it has exactly one participating validator
     // (len([bit for bit in attestation.aggregation_bits if bit == 0b1]) == 1).
-    if (attestation.getAggregation_bits().getBitCount() != 1) {
-      return InternalValidationResult.REJECT;
+    final int bitCount = attestation.getAggregation_bits().getBitCount();
+    if (bitCount != 1) {
+      return InternalValidationResult.reject("Attestation has %s bits set instead of 1", bitCount);
     }
 
     // The attestation is the first valid attestation received for the participating validator for
@@ -120,7 +121,10 @@ SafeFuture<InternalValidationResult> singleOrAggregateAttestationChecks(
     final AttestationData data = attestation.getData();
     // The attestation's epoch matches its target
     if (!data.getTarget().getEpoch().equals(spec.computeEpochAtSlot(data.getSlot()))) {
-      return completedFuture(InternalValidationResult.REJECT);
+      return completedFuture(
+          InternalValidationResult.reject(
+              "Attestation slot %s is not from target epoch %s",
+              data.getSlot(), data.getTarget().getEpoch()));
     }
 
     // attestation.data.slot is within the last ATTESTATION_PROPAGATION_SLOT_RANGE slots (within a
@@ -157,15 +161,20 @@ SafeFuture<InternalValidationResult> singleOrAggregateAttestationChecks(
               if (data.getIndex()
                   .isGreaterThanOrEqualTo(
                       spec.getCommitteeCountPerSlot(state, data.getTarget().getEpoch()))) {
-                return completedFuture(InternalValidationResult.REJECT);
+                return completedFuture(
+                    InternalValidationResult.reject(
+                        "Committee index %s is out of range", data.getIndex()));
               }
 
               // The attestation's committee index (attestation.data.index) is for the correct
               // subnet.
               if (receivedOnSubnetId.isPresent()
                   && spec.computeSubnetForAttestation(state, attestation)
                       != receivedOnSubnetId.getAsInt()) {
-                return completedFuture(InternalValidationResult.REJECT);
+                return completedFuture(
+                    InternalValidationResult.reject(
+                        "Attestation received on incorrect subnet (%s) for specified committee index (%s)",
+                        attestation.getData().getIndex(), receivedOnSubnetId.getAsInt()));
               }
 
               // The check below is not specified in the Eth2 networking spec, yet an attestation
@@ -174,15 +183,20 @@ SafeFuture<InternalValidationResult> singleOrAggregateAttestationChecks(
               final List<Integer> committee =
                   spec.getBeaconCommittee(state, data.getSlot(), data.getIndex());
               if (committee.size() != attestation.getAggregation_bits().size()) {
-                return completedFuture(InternalValidationResult.REJECT);
+                return completedFuture(
+                    InternalValidationResult.reject(
+                        "Aggregation bit size %s is greater than committee size %s",
+                        attestation.getAggregation_bits().size(), committee.size()));
               }
 
               return spec.isValidIndexedAttestation(
                       state, validateableAttestation, signatureVerifier)
                   .thenApply(
                       signatureResult -> {
                         if (!signatureResult.isSuccessful()) {
-                          return InternalValidationResult.REJECT;
+                          return InternalValidationResult.reject(
+                              "Attestation is not a valid indexed attestation: %s",
+                              signatureResult.getInvalidReason());
                         }
 
                         // The attestation's target block is an ancestor of the block named in the
@@ -195,7 +209,8 @@ SafeFuture<InternalValidationResult> singleOrAggregateAttestationChecks(
                                 ancestorOfLMDVote ->
                                     ancestorOfLMDVote.equals(data.getTarget().getRoot()))
                             .orElse(false)) {
-                          return InternalValidationResult.REJECT;
+                          return InternalValidationResult.reject(
+                              "Attestation LMD vote block does not descend from target block");
                         }
 
                         // The current finalized_checkpoint is an ancestor of the block defined by
@@ -210,7 +225,8 @@ SafeFuture<InternalValidationResult> singleOrAggregateAttestationChecks(
                                 ancestorOfLMDVote ->
                                     ancestorOfLMDVote.equals(finalizedCheckpoint.getRoot()))
                             .orElse(false)) {
-                          return InternalValidationResult.REJECT;
+                          return InternalValidationResult.reject(
+                              "Attestation block root does not descent from finalized checkpoint");
                         }
 
                         // Save committee shuffling seed since the state is available and