From 190c7687ff8149578ca44e16ad015c4583a2f8e4 Mon Sep 17 00:00:00 2001 From: Enrico Del Fante Date: Sat, 13 May 2023 00:38:19 +0200 Subject: [PATCH] Filter attestation with target checkpoint too old (#7137) Signed-off-by: Paul Harris Co-authored-by: Paul Harris Co-authored-by: Lucas Saldanha Co-authored-by: Stefan Bratanov --- CHANGELOG.md | 11 +++- .../validation/AttestationStateSelector.java | 56 +++++++++++++++++-- .../AttestationStateSelectorTest.java | 29 +++++++--- 3 files changed, 80 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 537cf258d2f..9e99289fb86 100644 --- 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,6 +11,7 @@ - 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 @@ -17,8 +19,11 @@ For information on changes in released versions of Teku, see the [releases page] ### Breaking Changes ### Additions and Improvements - - Set `User-Agent` header to "teku/v" (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" (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/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/validation/AttestationStateSelector.java b/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/validation/AttestationStateSelector.java index 5425be0dfae..4615e81d115 100644 --- a/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/validation/AttestationStateSelector.java +++ b/ethereum/statetransition/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> 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> 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> 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 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/ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/validation/AttestationStateSelectorTest.java b/ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/validation/AttestationStateSelectorTest.java index e8d3422a888..a62a2fb68b9 100644 --- a/ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/validation/AttestationStateSelectorTest.java +++ b/ethereum/statetransition/src/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> 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> result = selectStateFor(attestationSlot, blockRoot); + + final Optional selectedState = safeJoin(result); + + assertThat(selectedState).isEmpty(); } private SafeFuture> selectStateFor(