Filter attestation with target checkpoint too old (#7137)

Signed-off-by: Paul Harris <[email protected]> Co-authored-by: Paul Harris <[email protected]> Co-authored-by: Lucas Saldanha <[email protected]> Co-authored-by: Stefan Bratanov <[email protected]>
Consensys · May 12, 2023 · 190c768 · 190c768
1 parent e4cfbb5
commit 190c768
Show file tree

Hide file tree

Showing 3 changed files with 80 additions and 16 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,6 +1,7 @@
 # Changelog
 
 ## Upcoming Breaking Changes
+
 - Upgrading source code to Java 17 meaning users will need to upgrade their Java install to at least 17, or use the jdk17 variant of the docker image.
 - The `/eth/v1/debug/beacon/states/:state_id` endpoint has been deprecated in favor of the v2 Altair endpoint `/eth/v2/debug/beacon/states/:state_id`
 - The `/eth/v1/beacon/blocks/:block_id` endpoint has been deprecated in favor of the v2 Altair endpoint `/eth/v2/beacon/blocks/:block_id`
@@ -10,15 +11,19 @@
 - The command argument `--Xdeposit-snapshot-enabled` will be removed, just remove it from commandline/configuration if you use it, updated argument `--deposit-snapshot-enabled` defaults to true now.
 
 ## Current Releases
+
 For information on changes in released versions of Teku, see the [releases page](https://github.com/ConsenSys/teku/releases).
 
 ## Unreleased Changes
 
 ### Breaking Changes
 
 ### Additions and Improvements
- - Set `User-Agent` header to "teku/v<version>" (e.g. teku/v23.4.0) when making builder bid requests to help builders identify clients and versions. Use `--builder-set-user-agent-header=false` to disable. 
- - Included more context when a request to an external signer fails.
- - Added `/eth/v1/beacon/rewards/blocks/{block_id}` rest api endpoint.
+
+- Set `User-Agent` header to "teku/v<version>" (e.g. teku/v23.4.0) when making builder bid requests to help builders identify clients and versions. Use `--builder-set-user-agent-header=false` to disable.
+- Included more context when a request to an external signer fails.
+- Added `/eth/v1/beacon/rewards/blocks/{block_id}` rest api endpoint.
 
 ### Bug Fixes
+
+- Filtering attestations with old target checkpoint
diff --git a/.../src/main/java/tech/pegasys/teku/statetransition/validation/AttestationStateSelector.java b/.../src/main/java/tech/pegasys/teku/statetransition/validation/AttestationStateSelector.java
@@ -16,10 +16,13 @@
 import static tech.pegasys.teku.infrastructure.async.SafeFuture.completedFuture;
 
 import java.util.Optional;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
 import org.apache.tuweni.bytes.Bytes32;
 import tech.pegasys.teku.infrastructure.async.SafeFuture;
 import tech.pegasys.teku.infrastructure.unsigned.UInt64;
 import tech.pegasys.teku.spec.Spec;
+import tech.pegasys.teku.spec.datastructures.forkchoice.ProtoNodeData;
 import tech.pegasys.teku.spec.datastructures.forkchoice.ReadOnlyForkChoiceStrategy;
 import tech.pegasys.teku.spec.datastructures.operations.AttestationData;
 import tech.pegasys.teku.spec.datastructures.state.Checkpoint;
@@ -28,6 +31,7 @@
 import tech.pegasys.teku.storage.client.RecentChainData;
 
 public class AttestationStateSelector {
+  private static final Logger LOG = LogManager.getLogger();
   private final Spec spec;
   private final RecentChainData recentChainData;
 
@@ -60,7 +64,7 @@ public SafeFuture<Optional<BeaconState>> getStateToValidate(
     if (attestationEpoch
         .plus(spec.getSpecConfig(headEpoch).getEpochsPerHistoricalVector())
         .isGreaterThan(headEpoch)) {
-      if (isAncestorOfChainHead(chainHead, targetBlockRoot, attestationSlot)) {
+      if (isAncestorOfChainHead(chainHead.getRoot(), targetBlockRoot, attestationSlot)) {
         return chainHead.getState().thenApply(Optional::of);
       }
     }
@@ -83,6 +87,18 @@ public SafeFuture<Optional<BeaconState>> getStateToValidate(
       // Block became unknown, so ignore it
       return completedFuture(Optional.empty());
     }
+
+    if (isJustificationTooOld(targetBlockRoot, targetBlockSlot.get())) {
+      // we already justified a more recent slot on all compatible heads
+      LOG.debug(
+          "Ignored attestation gossip: attestationData target {}, source {}, head {} , slot {}",
+          attestationData.getTarget().getRoot(),
+          attestationData.getSource().getRoot(),
+          attestationData.getBeaconBlockRoot(),
+          attestationData.getSlot());
+      return completedFuture(Optional.empty());
+    }
+
     final Checkpoint requiredCheckpoint;
     if (targetBlockSlot.get().isLessThan(earliestSlot)) {
       // Target block is from before the earliest slot so just roll it forward.
@@ -100,19 +116,51 @@ public SafeFuture<Optional<BeaconState>> getStateToValidate(
       requiredCheckpoint =
           new Checkpoint(spec.computeEpochAtSlot(earliestSlot), maybeAncestorRoot.get());
     }
+    LOG.debug(
+        "Retrieving checkpoint state for attestationData target {}, source {}, head {} , slot {}; required checkpoint block root {}",
+        attestationData.getTarget().getRoot(),
+        attestationData.getSource().getRoot(),
+        attestationData.getBeaconBlockRoot(),
+        attestationData.getSlot(),
+        requiredCheckpoint.getRoot());
     return recentChainData.retrieveCheckpointState(requiredCheckpoint);
   }
 
   private Boolean isAncestorOfChainHead(
-      final ChainHead chainHead, final Bytes32 targetBlockRoot, final UInt64 attestationSlot) {
+      final Bytes32 headRoot, final Bytes32 blockRoot, final UInt64 blockSlot) {
     return recentChainData
         .getForkChoiceStrategy()
         .orElseThrow()
-        .getAncestor(chainHead.getRoot(), attestationSlot)
-        .map(canonicalRoot -> canonicalRoot.equals(targetBlockRoot))
+        .getAncestor(headRoot, blockSlot)
+        .map(canonicalRoot -> canonicalRoot.equals(blockRoot))
+        .orElse(false);
+  }
+
+  private Boolean isJustifiedCheckpointOfHeadOlderOrEqualToAttestationJustifiedSlot(
+      final ProtoNodeData head, final UInt64 justifiedBlockSlot) {
+    final Checkpoint justifiedCheckpoint = head.getCheckpoints().getJustifiedCheckpoint();
+    final Optional<UInt64> maybeHeadJustifiedSlot =
+        recentChainData.getSlotForBlockRoot(justifiedCheckpoint.getRoot());
+    return maybeHeadJustifiedSlot
+        .map(slot -> slot.isLessThanOrEqualTo(justifiedBlockSlot))
         .orElse(false);
   }
 
+  private boolean isJustificationTooOld(
+      final Bytes32 justifiedRoot, final UInt64 justifiedBlockSlot) {
+
+    return recentChainData.getChainHeads().stream()
+        // must be attesting to a viable chain
+        .filter(head -> isAncestorOfChainHead(head.getRoot(), justifiedRoot, justifiedBlockSlot))
+        // must be attesting to something that progresses justification
+        .filter(
+            head ->
+                isJustifiedCheckpointOfHeadOlderOrEqualToAttestationJustifiedSlot(
+                    head, justifiedBlockSlot))
+        .findFirst()
+        .isEmpty();
+  }
+
   /**
    * Committee information is only guaranteed to be stable up to 1 epoch ahead, if block attested to
    * is too old, we need to roll the corresponding state forward to process the attestation

diff --git a/.../test/java/tech/pegasys/teku/statetransition/validation/AttestationStateSelectorTest.java b/.../test/java/tech/pegasys/teku/statetransition/validation/AttestationStateSelectorTest.java
@@ -138,19 +138,30 @@ void shouldUseHeadStateWhenAttestationIsForAncestorFromEarlierEpochWithinHistori
   }
 
   @Test
-  void shouldUseAncestorAtEarliestSlotWhenBlockIsAFork() {
+  void shouldIgnoreAttestationsWithTargetWhichIsAlreadyJustified() {
+    chainUpdater.updateBestBlock(
+        chainUpdater.advanceChainUntil(spec.computeStartSlotAtEpoch(UInt64.ONE)));
+
     final ChainBuilder forkBuilder = chainBuilder.fork();
-    // Advance chain so finalized checkpoint isn't suitable
-    chainUpdater.updateBestBlock(chainUpdater.advanceChainUntil(25));
 
-    final SignedBlockAndState expected = forkBuilder.generateBlockAtSlot(16);
-    chainUpdater.saveBlock(expected);
-    final SignedBlockAndState forkBlock = forkBuilder.generateBlockAtSlot(24);
+    final SignedBlockAndState forkBlock =
+        forkBuilder.generateBlockAtSlot(spec.computeStartSlotAtEpoch(UInt64.ONE).plus(1));
+
     chainUpdater.saveBlock(forkBlock);
 
-    final SafeFuture<Optional<BeaconState>> result =
-        selectStateFor(UInt64.valueOf(25), forkBlock.getRoot());
-    assertThatSafeFuture(result).isCompletedWithOptionalContaining(expected.getState());
+    // advance chain head to epoch 3 and justify epoch 2
+    chainUpdater.advanceChainUntil(spec.computeStartSlotAtEpoch(UInt64.valueOf(3)));
+    chainUpdater.justifyEpoch(2);
+
+    // Attestation slot of the fork block is before an already justified checkpoint
+    final UInt64 attestationSlot = spec.computeStartSlotAtEpoch(forkBlock.getSlot());
+    final Bytes32 blockRoot = forkBlock.getRoot();
+
+    final SafeFuture<Optional<BeaconState>> result = selectStateFor(attestationSlot, blockRoot);
+
+    final Optional<BeaconState> selectedState = safeJoin(result);
+
+    assertThat(selectedState).isEmpty();
   }
 
   private SafeFuture<Optional<BeaconState>> selectStateFor(