diff --git a/build.gradle b/build.gradle index 92b9179611e..60e81b2bbe7 100644 --- a/build.gradle +++ b/build.gradle @@ -541,6 +541,7 @@ def slashingProtectionInterchangeRefTestBaseUrl = 'https://github.com/eth-client def refTestDownloadDir = "${buildDir}/refTests/${refTestVersion}" def blsRefTestDownloadDir = "${buildDir}/blsRefTests/${blsRefTestVersion}" def slashingProtectionInterchangeRefTestDownloadDir = "${buildDir}/slashingProtectionInterchangeRefTests/${slashingProtectionInterchangeRefTestVersion}" +def compTestDownloadDir = "${buildDir}/compRefTests/${refTestVersion}" def refTestExpandDir = "${project.rootDir}/eth-reference-tests/src/referenceTest/resources/consensus-spec-tests/" def downloadFile(String url, String token, File outputFile) { @@ -657,8 +658,19 @@ tasks.register('downloadSlashingProtectionInterchangeRefTests', Download) { overwrite false } +// comptests.tar.gz is only published alongside tagged consensus-specs releases (not the nightly +// vector-generation workflow), so it is skipped for nightly builds. +tasks.register('downloadCompTests', Download) { + onlyIf { !nightly } + src([ + "${refTestBaseUrl}/${refTestVersion}/comptests.tar.gz" + ]) + dest "${compTestDownloadDir}/comptests.tar.gz" + overwrite false +} + tasks.register('downloadRefTests') { - dependsOn downloadEthRefTests, downloadBlsRefTests, downloadSlashingProtectionInterchangeRefTests + dependsOn downloadEthRefTests, downloadBlsRefTests, downloadSlashingProtectionInterchangeRefTests, downloadCompTests } tasks.register('cleanRefTestsGeneral', Delete) { @@ -719,11 +731,31 @@ tasks.register('expandRefTestsSlashingProtectionInterchange', Copy) { into "${refTestExpandDir}/tests/slashing-protection-interchange" } +// comptests.tar.gz bundles the whole consensus-specs tests/ tree (pyspec source included, not +// just vectors), so only the fork_choice_compliance subtree is extracted here rather than the +// full ~3GB archive. +tasks.register('cleanRefTestsForkChoiceCompliance', Delete) { + delete fileTree(refTestExpandDir) { + include "tests/**/fork_choice_compliance/**" + } +} + +tasks.register('expandRefTestsForkChoiceCompliance', Copy) { + dependsOn cleanRefTestsForkChoiceCompliance, downloadCompTests + onlyIf { !nightly } + from { + tarTree("${compTestDownloadDir}/comptests.tar.gz").matching { + include "tests/**/fork_choice_compliance/**" + } + } + into refTestExpandDir +} + tasks.register('expandRefTests') { - dependsOn expandRefTestsGeneral, expandRefTestsMainnet, expandRefTestsMinimal, expandRefTestsBls, expandRefTestsSlashingProtectionInterchange + dependsOn expandRefTestsGeneral, expandRefTestsMainnet, expandRefTestsMinimal, expandRefTestsBls, expandRefTestsSlashingProtectionInterchange, expandRefTestsForkChoiceCompliance } tasks.register('cleanRefTests') { - dependsOn cleanRefTestsGeneral, cleanRefTestsMainnet, cleanRefTestsMinimal, cleanRefTestsBls, cleanRefTestsSlashingProtectionInterchange + dependsOn cleanRefTestsGeneral, cleanRefTestsMainnet, cleanRefTestsMinimal, cleanRefTestsBls, cleanRefTestsSlashingProtectionInterchange, cleanRefTestsForkChoiceCompliance } tasks.register('deploy') {} diff --git a/eth-reference-tests/src/referenceTest/java/tech/pegasys/teku/reference/phase0/forkchoice/ForkChoiceTestExecutor.java b/eth-reference-tests/src/referenceTest/java/tech/pegasys/teku/reference/phase0/forkchoice/ForkChoiceTestExecutor.java index e074feecb67..d2d78704253 100644 --- a/eth-reference-tests/src/referenceTest/java/tech/pegasys/teku/reference/phase0/forkchoice/ForkChoiceTestExecutor.java +++ b/eth-reference-tests/src/referenceTest/java/tech/pegasys/teku/reference/phase0/forkchoice/ForkChoiceTestExecutor.java @@ -26,6 +26,7 @@ import java.io.IOException; import java.nio.file.Path; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.HashSet; @@ -40,6 +41,7 @@ import org.apache.tuweni.bytes.Bytes32; import org.apache.tuweni.ssz.SSZ; import org.assertj.core.api.Condition; +import org.junit.jupiter.api.Assertions; import org.opentest4j.TestAbortedException; import tech.pegasys.teku.bls.BLSSignature; import tech.pegasys.teku.bls.BLSSignatureVerifier; @@ -75,7 +77,6 @@ import tech.pegasys.teku.spec.datastructures.forkchoice.FastConfirmationStore; import tech.pegasys.teku.spec.datastructures.forkchoice.ForkChoiceNode; import tech.pegasys.teku.spec.datastructures.forkchoice.ForkChoicePayloadStatus; -import tech.pegasys.teku.spec.datastructures.forkchoice.ProtoNodeData; import tech.pegasys.teku.spec.datastructures.forkchoice.ReadOnlyForkChoiceStrategy; import tech.pegasys.teku.spec.datastructures.forkchoice.ReadOnlyStore; import tech.pegasys.teku.spec.datastructures.forkchoice.VoteUpdater; @@ -536,15 +537,12 @@ private void applyAttestation( forkChoice.onAttestation(validatableAttestation); assertThat(result).isCompleted(); final AttestationProcessingResult processingResult = safeJoin(result); - // A current-slot attestation is valid but deferred by fork choice (stored and applied on the - // next tick). The fast confirmation vectors apply such attestations, so treat deferral as an - // accepted outcome. - final boolean acceptedByForkChoice = - processingResult.isSuccessful() - || processingResult.getStatus() - == AttestationProcessingResult.Status.DEFER_FORK_CHOICE_PROCESSING; - assertThat(acceptedByForkChoice) - .withFailMessage(processingResult.getInvalidReason()) + // If a current-slot attestation is valid but deferred by fork choice (stored and applied on the + // next tick), reference tests seem to expect it to be considered invalid, so we will not + // consider + // it a successful attestation + assertThat(processingResult.isSuccessful()) + .withFailMessage("%s failed with processing result: %s", attestationName, processingResult) .isEqualTo(valid); } @@ -847,27 +845,53 @@ private void applyChecks( case "viable_for_head_roots_and_weights" -> { final List> viableHeadRootsAndWeightsData = get(checks, checkType); - final Map viableHeadRootsAndWeights = + + final Set viableHeadRootsAndWeights = viableHeadRootsAndWeightsData.stream() - .collect( - Collectors.toMap( - entry -> Bytes32.fromHexString((String) entry.get("root")), - entry -> UInt64.valueOf(entry.get("weight").toString()))); - final Map chainHeadRootsAndWeights = + .map( + entry -> + new HeadRootAndWeight( + Bytes32.fromHexString((String) entry.get("root")), + UInt64.valueOf(entry.get("weight").toString()), + Optional.ofNullable((Integer) entry.get("payload_status")) + .map(this::convertToPayloadStatus))) + .collect(Collectors.toSet()); + final Set chainHeadRootsAndWeights = recentChainData .getForkChoiceStrategy() .map(ReadOnlyForkChoiceStrategy::getChainHeads) .orElse(Collections.emptyList()) .stream() - .collect(Collectors.toMap(ProtoNodeData::getRoot, ProtoNodeData::getWeight)); - - assertThat(chainHeadRootsAndWeights.keySet()) - .containsAll(viableHeadRootsAndWeights.keySet()); - - for (Bytes32 root : viableHeadRootsAndWeights.keySet()) { - UInt64 weight = viableHeadRootsAndWeights.get(root); - UInt64 actualWeight = chainHeadRootsAndWeights.get(root); - assertThat(actualWeight).describedAs("block %s's weight", root).isEqualTo(weight); + .map( + protoNodeData -> + new HeadRootAndWeight( + protoNodeData.getRoot(), + protoNodeData.getWeight(), + Optional.ofNullable(protoNodeData.getPayloadStatus()))) + .collect(Collectors.toSet()); + + for (HeadRootAndWeight headRootAndWeight : viableHeadRootsAndWeights) { + boolean notPresent = + chainHeadRootsAndWeights.stream() + .noneMatch( + (chainHeadRootAndWeight) -> { + if (headRootAndWeight.root.equals(chainHeadRootAndWeight.root) + && headRootAndWeight.weight.equals(chainHeadRootAndWeight.weight)) { + // an unset payload status means we don't need to check if payload + // status is correct + if (headRootAndWeight.payloadStatus.isPresent()) { + return headRootAndWeight.payloadStatus.equals( + chainHeadRootAndWeight.payloadStatus); + } else { + return true; + } + } + return false; + }); + Assertions.assertFalse( + notPresent, + String.format( + "Unable to find %s in %s", headRootAndWeight, chainHeadRootsAndWeights)); } } @@ -1142,4 +1166,14 @@ public BlsSetting getBlsSetting() { return BlsSetting.forCode(blsSetting); } } + + private ForkChoicePayloadStatus convertToPayloadStatus(final int payloadStatus) { + return Arrays.stream(ForkChoicePayloadStatus.values()) + .filter(fcps -> fcps.getValue() == payloadStatus) + .findAny() + .get(); + } + + private record HeadRootAndWeight( + Bytes32 root, UInt64 weight, Optional payloadStatus) {} } diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/datastructures/attestation/AttestationSource.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/datastructures/attestation/AttestationSource.java new file mode 100644 index 00000000000..531eaf6d380 --- /dev/null +++ b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/datastructures/attestation/AttestationSource.java @@ -0,0 +1,19 @@ +/* + * Copyright Consensys Software Inc., 2026 + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + */ + +package tech.pegasys.teku.spec.datastructures.attestation; + +public enum AttestationSource { + GOSSIP, + BLOCK +} diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/util/ForkChoiceUtil.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/util/ForkChoiceUtil.java index 7ff8d617448..8178bd85dfc 100644 --- a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/util/ForkChoiceUtil.java +++ b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/util/ForkChoiceUtil.java @@ -28,6 +28,7 @@ import tech.pegasys.teku.spec.config.SpecConfig; import tech.pegasys.teku.spec.config.SpecConfigAltair; import tech.pegasys.teku.spec.config.SpecConfigBellatrix; +import tech.pegasys.teku.spec.datastructures.attestation.AttestationSource; import tech.pegasys.teku.spec.datastructures.attestation.ValidatableAttestation; import tech.pegasys.teku.spec.datastructures.blobs.versions.deneb.BlobSidecar; import tech.pegasys.teku.spec.datastructures.blocks.BeaconBlock; @@ -536,13 +537,15 @@ private AttestationProcessingResult validateOnAttestation( UInt64 currentEpoch = miscHelpers.computeEpochAtSlot(getCurrentSlot(store)); final ReadOnlyForkChoiceStrategy forkChoiceStrategy = store.getForkChoiceStrategy(); - return validateOnAttestation(forkChoiceStrategy, currentEpoch, attestationData); + return validateOnAttestation( + forkChoiceStrategy, currentEpoch, attestationData, AttestationSource.GOSSIP); } public AttestationProcessingResult validateOnAttestation( final ReadOnlyForkChoiceStrategy forkChoiceStrategy, final UInt64 currentEpoch, - final AttestationData attestationData) { + final AttestationData attestationData, + final AttestationSource attestationSource) { final Checkpoint target = attestationData.getTarget(); // Use GENESIS_EPOCH for previous when genesis to avoid underflow @@ -551,9 +554,11 @@ public AttestationProcessingResult validateOnAttestation( ? currentEpoch.minus(UInt64.ONE) : SpecConfig.GENESIS_EPOCH; - if (!target.getEpoch().equals(previousEpoch) && !target.getEpoch().equals(currentEpoch)) { - return AttestationProcessingResult.invalid( - "Attestations must be from the current or previous epoch"); + if (attestationSource == AttestationSource.GOSSIP) { + if (!target.getEpoch().equals(previousEpoch) && !target.getEpoch().equals(currentEpoch)) { + return AttestationProcessingResult.invalid( + "Attestations must be from the current or previous epoch"); + } } if (!target.getEpoch().equals(miscHelpers.computeEpochAtSlot(attestationData.getSlot()))) { @@ -923,10 +928,10 @@ public boolean shouldNotifyForkChoiceUpdatedOnBlock() { } public boolean shouldApplyProposerBoost( + final ForkChoiceReorgContext context, final Bytes32 proposerBoostRoot, final ReadOnlyForkChoiceStrategy forkChoiceStrategy, - final UInt64 reorgThreshold, - final BeaconState justifiedState) { + final UInt64 reorgThreshold) { return true; } diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/versions/gloas/util/ForkChoiceUtilGloas.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/versions/gloas/util/ForkChoiceUtilGloas.java index d8ca5b111b3..7e7b317de16 100644 --- a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/versions/gloas/util/ForkChoiceUtilGloas.java +++ b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/versions/gloas/util/ForkChoiceUtilGloas.java @@ -270,24 +270,20 @@ public Optional toVersionGloas() { * If the boosted block's parent was weak and from the previous slot, boost only applies if there * are no timely equivocations from the same proposer. * - *

Implementation note: the proposer-equivocation branch is intentionally not implemented yet. - * The current code records both block timeliness flags, but it does not yet consume the PTC - * timeliness bit here to suppress proposer boost on same-proposer equivocations. Because that - * branch is still deferred, the weak-parent check has no effect on the return value and is - * intentionally skipped here. + *

Spec mapping: {@code should_apply_proposer_boost(store)}. * + * @param context the fork choice reorg context, used to look up blocks and PTC timeliness * @param proposerBoostRoot the current proposer boost root, empty if none * @param forkChoiceStrategy the fork choice strategy for looking up block data * @param reorgThreshold the threshold for the head weakness check - * @param justifiedState unused until the proposer-equivocation branch is implemented * @return true if proposer boost should be applied */ @Override public boolean shouldApplyProposerBoost( + final ForkChoiceReorgContext context, final Bytes32 proposerBoostRoot, final ReadOnlyForkChoiceStrategy forkChoiceStrategy, - final UInt64 reorgThreshold, - final BeaconState justifiedState) { + final UInt64 reorgThreshold) { final Optional maybeParentRoot = forkChoiceStrategy.blockParentRoot(proposerBoostRoot); final Optional maybeBlockSlot = forkChoiceStrategy.blockSlot(proposerBoostRoot); if (maybeParentRoot.isEmpty() || maybeBlockSlot.isEmpty()) { @@ -299,21 +295,57 @@ public boolean shouldApplyProposerBoost( if (maybeParentSlot.isEmpty()) { return true; } + final UInt64 parentSlot = maybeParentSlot.get(); // Apply proposer boost if parent is not from the previous slot - if (maybeParentSlot.get().increment().isLessThan(blockSlot)) { + if (parentSlot.increment().isLessThan(blockSlot)) { return true; } - // TODO-GLOAS: implement the Gloas equivocation suppression branch from - // should_apply_proposer_boost - // using recorded PTC timeliness instead of routing a predicate through ForkChoice. - // The complication is that we need to have a good interaction with gossip datastructures to - // detect equivocations. Spec should probably be updated. - // NOTE: there is no point in implementing the following check without implementing - // equivocation. - // # Apply proposer boost if `parent` is not weak - // if not is_head_weak(store, parent_root): - // return True - return true; + final ReadOnlyStore store = context.getStore(); + // Apply proposer boost if `parent` is not weak + if (!isHeadWeak(store, parentRoot, reorgThreshold)) { + return true; + } + // If `parent` is weak and from the previous slot, apply + // proposer boost if there are no early equivocations + return !hasTimelyEquivocatingSiblingProposal( + context, forkChoiceStrategy, parentRoot, parentSlot); + } + + /** + * Spec mapping: the {@code equivocations} list comprehension inside {@code + * should_apply_proposer_boost}. + * + *

Looks for another block proposed at the same slot as {@code parentRoot}, by the same + * proposer, whose PTC-timeliness bit is set. Finding one means the parent's proposer equivocated + * in a way that was seen in time by the PTC, which should suppress proposer boost on an + * otherwise-weak parent. + */ + private boolean hasTimelyEquivocatingSiblingProposal( + final ForkChoiceReorgContext context, + final ReadOnlyForkChoiceStrategy forkChoiceStrategy, + final Bytes32 parentRoot, + final UInt64 parentSlot) { + final ReadOnlyStore store = context.getStore(); + final Optional maybeParentProposerIndex = + store.getBlockIfAvailable(parentRoot).map(block -> block.getMessage().getProposerIndex()); + if (maybeParentProposerIndex.isEmpty()) { + return false; + } + final UInt64 parentProposerIndex = maybeParentProposerIndex.get(); + return forkChoiceStrategy.getBlockRootsAtSlot(parentSlot).stream() + .filter(siblingRoot -> !siblingRoot.equals(parentRoot)) + .anyMatch( + siblingRoot -> + context + .getBlockTimeliness(siblingRoot) + .map(BlockTimeliness::isTimelyPtc) + .orElse(false) + && store + .getBlockIfAvailable(siblingRoot) + .map( + block -> + block.getMessage().getProposerIndex().equals(parentProposerIndex)) + .orElse(false)); } /** diff --git a/ethereum/spec/src/test/java/tech/pegasys/teku/spec/logic/common/util/ForkChoiceUtilTest.java b/ethereum/spec/src/test/java/tech/pegasys/teku/spec/logic/common/util/ForkChoiceUtilTest.java index dd26b9ca26d..7b5ddb764cc 100644 --- a/ethereum/spec/src/test/java/tech/pegasys/teku/spec/logic/common/util/ForkChoiceUtilTest.java +++ b/ethereum/spec/src/test/java/tech/pegasys/teku/spec/logic/common/util/ForkChoiceUtilTest.java @@ -40,6 +40,7 @@ import tech.pegasys.teku.spec.SpecMilestone; import tech.pegasys.teku.spec.SpecVersion; import tech.pegasys.teku.spec.TestSpecFactory; +import tech.pegasys.teku.spec.datastructures.attestation.AttestationSource; import tech.pegasys.teku.spec.datastructures.blobs.versions.deneb.BlobSidecar; import tech.pegasys.teku.spec.datastructures.blocks.BlockCheckpoints; import tech.pegasys.teku.spec.datastructures.blocks.SignedBeaconBlock; @@ -52,8 +53,10 @@ import tech.pegasys.teku.spec.datastructures.forkchoice.ReadOnlyStore; import tech.pegasys.teku.spec.datastructures.forkchoice.TestStoreFactory; import tech.pegasys.teku.spec.datastructures.forkchoice.TestStoreImpl; +import tech.pegasys.teku.spec.datastructures.operations.AttestationData; import tech.pegasys.teku.spec.datastructures.state.Checkpoint; import tech.pegasys.teku.spec.datastructures.state.beaconstate.BeaconState; +import tech.pegasys.teku.spec.datastructures.util.AttestationProcessingResult; import tech.pegasys.teku.spec.logic.common.statetransition.availability.AvailabilityChecker; import tech.pegasys.teku.spec.logic.common.statetransition.availability.AvailabilityCheckerFactory; import tech.pegasys.teku.spec.logic.common.statetransition.exceptions.EpochProcessingException; @@ -624,6 +627,69 @@ void shouldOverrideFcuCheckProposerPreState_shouldReturnFalseWhenValidatorDiscon .isFalse(); } + @Test + void + validateOnAttestation_blockSource_shouldRejectAttestationWhenTargetEpochDoesNotMatchSlotEpoch() { + // Regression test: previously the "target epoch must equal compute_epoch_at_slot(slot)" + // check was incorrectly gated behind AttestationSource.GOSSIP, so attestations embedded in + // blocks with a mismatched slot/target epoch were accepted and their votes counted toward + // fork choice. Per the spec, this structural check is unconditional. + final int slotsPerEpoch = spec.getGenesisSpecConfig().getSlotsPerEpoch(); + final UInt64 currentEpoch = UInt64.valueOf(10); + final UInt64 attestationSlot = UInt64.valueOf(currentEpoch.longValue() * slotsPerEpoch); + // Target epoch is the previous epoch, so it would pass the current/previous-epoch recency + // check, but it does not match compute_epoch_at_slot(attestationSlot) == currentEpoch. + final Checkpoint mismatchedTarget = dataStructureUtil.randomCheckpoint(currentEpoch.minus(1)); + final AttestationData attestationData = + new AttestationData( + attestationSlot, + UInt64.ZERO, + dataStructureUtil.randomBytes32(), + dataStructureUtil.randomCheckpoint(currentEpoch.minus(2)), + mismatchedTarget); + final ReadOnlyForkChoiceStrategy strategy = mock(ReadOnlyForkChoiceStrategy.class); + + final AttestationProcessingResult result = + forkChoiceUtil.validateOnAttestation( + strategy, currentEpoch, attestationData, AttestationSource.BLOCK); + + assertThat(result.isInvalid()).isTrue(); + assertThat(result.getInvalidReason()) + .contains("Attestation slot must be within specified epoch"); + // The attestation should be rejected before any fork choice lookups are attempted. + verify(strategy, never()).contains(any()); + } + + @Test + void validateOnAttestation_blockSource_shouldSkipRecencyCheckButStillEnforceEpochConsistency() { + // AttestationSource.BLOCK is only meant to skip the current/previous-epoch recency check + // (which does not apply to attestations already embedded in a finalized-chain block). + // Here the target epoch is neither current nor previous, but it is internally consistent + // with the attestation slot, so validation should proceed past the epoch checks. + final int slotsPerEpoch = spec.getGenesisSpecConfig().getSlotsPerEpoch(); + final UInt64 currentEpoch = UInt64.valueOf(10); + final UInt64 oldEpoch = UInt64.ZERO; + final UInt64 attestationSlot = UInt64.valueOf(oldEpoch.longValue() * slotsPerEpoch); + final Checkpoint consistentTarget = dataStructureUtil.randomCheckpoint(oldEpoch); + final AttestationData attestationData = + new AttestationData( + attestationSlot, + UInt64.ZERO, + dataStructureUtil.randomBytes32(), + dataStructureUtil.randomCheckpoint(oldEpoch), + consistentTarget); + final ReadOnlyForkChoiceStrategy strategy = mock(ReadOnlyForkChoiceStrategy.class); + when(strategy.contains(consistentTarget.getRoot())).thenReturn(false); + + final AttestationProcessingResult result = + forkChoiceUtil.validateOnAttestation( + strategy, currentEpoch, attestationData, AttestationSource.BLOCK); + + // Falls through to the unknown-block check rather than being rejected for being outside the + // current/previous epoch, confirming the recency check was skipped as intended. + assertThat(result).isEqualTo(AttestationProcessingResult.UNKNOWN_BLOCK); + } + private ReadOnlyStore mockStore( final long currentSlot, final Bytes32... blocksWithNonDefaultPayloads) { final ReadOnlyStore store = mock(ReadOnlyStore.class); diff --git a/ethereum/spec/src/test/java/tech/pegasys/teku/spec/logic/versions/gloas/util/ForkChoiceUtilGloasTest.java b/ethereum/spec/src/test/java/tech/pegasys/teku/spec/logic/versions/gloas/util/ForkChoiceUtilGloasTest.java index 9787b98c31d..ac152a3ad96 100644 --- a/ethereum/spec/src/test/java/tech/pegasys/teku/spec/logic/versions/gloas/util/ForkChoiceUtilGloasTest.java +++ b/ethereum/spec/src/test/java/tech/pegasys/teku/spec/logic/versions/gloas/util/ForkChoiceUtilGloasTest.java @@ -362,11 +362,12 @@ void getFullPayloadVoteHint_matchesAttestationIndex() { @Test void shouldApplyProposerBoost_returnsTrue_whenProposerBoostRootIsUnknown() { final Bytes32 boostRoot = dataStructureUtil.randomBytes32(); + final ForkChoiceReorgContext context = mock(ForkChoiceReorgContext.class); final ReadOnlyForkChoiceStrategy strategy = mock(ReadOnlyForkChoiceStrategy.class); when(strategy.blockParentRoot(boostRoot)).thenReturn(Optional.empty()); assertThat( forkChoiceUtil.shouldApplyProposerBoost( - boostRoot, strategy, UInt64.valueOf(100), justifiedState)) + context, boostRoot, strategy, UInt64.valueOf(100))) .isTrue(); } @@ -374,6 +375,7 @@ void shouldApplyProposerBoost_returnsTrue_whenProposerBoostRootIsUnknown() { void shouldApplyProposerBoost_returnsTrue_whenParentNotFromPreviousSlot() { final Bytes32 boostRoot = dataStructureUtil.randomBytes32(); final Bytes32 parentRoot = dataStructureUtil.randomBytes32(); + final ForkChoiceReorgContext context = mock(ForkChoiceReorgContext.class); final ReadOnlyForkChoiceStrategy strategy = mock(ReadOnlyForkChoiceStrategy.class); when(strategy.blockParentRoot(boostRoot)).thenReturn(Optional.of(parentRoot)); when(strategy.blockSlot(boostRoot)).thenReturn(Optional.of(UInt64.valueOf(5))); @@ -381,7 +383,7 @@ void shouldApplyProposerBoost_returnsTrue_whenParentNotFromPreviousSlot() { assertThat( forkChoiceUtil.shouldApplyProposerBoost( - boostRoot, strategy, UInt64.valueOf(100), justifiedState)) + context, boostRoot, strategy, UInt64.valueOf(100))) .isTrue(); } @@ -389,30 +391,92 @@ void shouldApplyProposerBoost_returnsTrue_whenParentNotFromPreviousSlot() { void shouldApplyProposerBoost_returnsTrue_whenParentIsNotWeak() { final Bytes32 boostRoot = dataStructureUtil.randomBytes32(); final Bytes32 parentRoot = dataStructureUtil.randomBytes32(); + final ForkChoiceReorgContext context = mock(ForkChoiceReorgContext.class); final ReadOnlyForkChoiceStrategy strategy = mock(ReadOnlyForkChoiceStrategy.class); + final ReadOnlyStore store = mock(ReadOnlyStore.class); + when(context.getStore()).thenReturn(store); when(strategy.blockParentRoot(boostRoot)).thenReturn(Optional.of(parentRoot)); when(strategy.blockSlot(boostRoot)).thenReturn(Optional.of(gloasSlot.plus(1))); when(strategy.blockSlot(parentRoot)).thenReturn(Optional.of(gloasSlot)); // consecutive - // The weak-parent branch is currently deferred together with proposer equivocation handling. + // Justified/head state are unavailable, so isHeadWeak fails closed to "not weak". + when(store.getJustifiedStateIfAvailable()).thenReturn(Optional.empty()); + when(store.getBlockStateIfAvailable(parentRoot)).thenReturn(Optional.empty()); + assertThat(forkChoiceUtil.shouldApplyProposerBoost(context, boostRoot, strategy, UInt64.ZERO)) + .isTrue(); + } + + @Test + void shouldApplyProposerBoost_returnsTrue_whenParentIsWeakButNoTimelyEquivocatingSibling() { + final Bytes32 boostRoot = dataStructureUtil.randomBytes32(); + final Bytes32 parentRoot = dataStructureUtil.randomBytes32(); + final ForkChoiceReorgContext context = weakParentContext(boostRoot, parentRoot); + final ReadOnlyForkChoiceStrategy strategy = context.getStore().getForkChoiceStrategy(); + when(strategy.getBlockRootsAtSlot(gloasSlot)).thenReturn(List.of(parentRoot)); + assertThat( forkChoiceUtil.shouldApplyProposerBoost( - boostRoot, strategy, UInt64.ZERO, justifiedState)) + context, boostRoot, strategy, UInt64.valueOf(100))) .isTrue(); } @Test - void shouldApplyProposerBoost_returnsTrue_whenParentIsWeakAndEquivocationBranchIsDeferred() { + void shouldApplyProposerBoost_returnsFalse_whenParentIsWeakWithTimelyEquivocatingSibling() { final Bytes32 boostRoot = dataStructureUtil.randomBytes32(); final Bytes32 parentRoot = dataStructureUtil.randomBytes32(); + final Bytes32 siblingRoot = dataStructureUtil.randomBytes32(); + final UInt64 parentProposerIndex = UInt64.valueOf(7); + final ForkChoiceReorgContext context = weakParentContext(boostRoot, parentRoot); + final ReadOnlyStore store = context.getStore(); + final ReadOnlyForkChoiceStrategy strategy = store.getForkChoiceStrategy(); + + final SignedBeaconBlock signedParentBlock = mock(SignedBeaconBlock.class); + final BeaconBlock parentBeaconBlock = mock(BeaconBlock.class); + when(signedParentBlock.getMessage()).thenReturn(parentBeaconBlock); + when(parentBeaconBlock.getProposerIndex()).thenReturn(parentProposerIndex); + when(store.getBlockIfAvailable(parentRoot)).thenReturn(Optional.of(signedParentBlock)); + + final SignedBeaconBlock signedSiblingBlock = mock(SignedBeaconBlock.class); + final BeaconBlock siblingBeaconBlock = mock(BeaconBlock.class); + when(signedSiblingBlock.getMessage()).thenReturn(siblingBeaconBlock); + when(siblingBeaconBlock.getProposerIndex()).thenReturn(parentProposerIndex); + when(store.getBlockIfAvailable(siblingRoot)).thenReturn(Optional.of(signedSiblingBlock)); + + when(strategy.getBlockRootsAtSlot(gloasSlot)).thenReturn(List.of(parentRoot, siblingRoot)); + when(context.getBlockTimeliness(siblingRoot)) + .thenReturn(Optional.of(new BlockTimeliness(true, true))); + + assertThat( + forkChoiceUtil.shouldApplyProposerBoost( + context, boostRoot, strategy, UInt64.valueOf(100))) + .isFalse(); + } + + /** + * Builds a {@link ForkChoiceReorgContext} where {@code boostRoot}'s parent ({@code parentRoot}) + * is from the previous slot and scores as weak (zero attestation weight against a large + * threshold), so {@code shouldApplyProposerBoost} proceeds to the equivocation check. + */ + private ForkChoiceReorgContext weakParentContext( + final Bytes32 boostRoot, final Bytes32 parentRoot) { + final BeaconState headState = dataStructureUtil.randomBeaconState(gloasSlot); + final ProtoNodeData parentNode = mock(ProtoNodeData.class); final ReadOnlyForkChoiceStrategy strategy = mock(ReadOnlyForkChoiceStrategy.class); + final ReadOnlyStore store = mock(ReadOnlyStore.class); + final ForkChoiceReorgContext context = mock(ForkChoiceReorgContext.class); + + when(context.getStore()).thenReturn(store); + when(store.getForkChoiceStrategy()).thenReturn(strategy); + when(store.getJustifiedStateIfAvailable()).thenReturn(Optional.of(justifiedState)); + when(store.getBlockStateIfAvailable(parentRoot)).thenReturn(Optional.of(headState)); + when(store.getVote(ArgumentMatchers.any())).thenReturn(VoteTracker.DEFAULT); + when(store.getProposerBoostRoot()).thenReturn(Optional.empty()); + when(parentNode.getWeight()).thenReturn(UInt64.ZERO); + when(strategy.getBlockData(parentRoot, ForkChoicePayloadStatus.PAYLOAD_STATUS_PENDING)) + .thenReturn(Optional.of(parentNode)); when(strategy.blockParentRoot(boostRoot)).thenReturn(Optional.of(parentRoot)); when(strategy.blockSlot(boostRoot)).thenReturn(Optional.of(gloasSlot.plus(1))); when(strategy.blockSlot(parentRoot)).thenReturn(Optional.of(gloasSlot)); // consecutive - // The equivocation suppression branch is intentionally not implemented yet. - assertThat( - forkChoiceUtil.shouldApplyProposerBoost( - boostRoot, strategy, UInt64.valueOf(100), justifiedState)) - .isTrue(); + return context; } @Test diff --git a/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/block/BlockManager.java b/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/block/BlockManager.java index 477cc6d2f6d..7ee1ba719e8 100644 --- a/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/block/BlockManager.java +++ b/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/block/BlockManager.java @@ -178,7 +178,13 @@ public SafeFuture validateAndImportBlock( // block failed gossip validation, let's drop it from the pool, so it won't be served // via RPC anymore. This should not be done on ignore result (i.e. duplicate blocks // could cause an unwanted drop) - case REJECT -> blockEventsListener.removeAllForBlock(block.getSlotAndBlockRoot()); + case REJECT -> { + blockEventsListener.removeAllForBlock(block.getSlotAndBlockRoot()); + // This attempt never reached import, so any timeliness recorded from its raw + // arrival shouldn't stick around to affect a later, separate import attempt for + // the same block (e.g. if it's subsequently re-fetched by root). + recentChainData.invalidateUnconfirmedBlockTimeliness(block); + } case IGNORE -> {} } }); @@ -313,7 +319,16 @@ private SafeFuture handleBlockImport( result -> { if (result.isSuccessful()) { LOG.trace("Imported block: {}", block); + // Successful import confirms (and, if necessary, refreshes) the block's + // timeliness recording, so it no longer matters whether an earlier attempt for + // this block was premature or otherwise didn't succeed. + recentChainData.confirmBlockTimeliness(block); } else { + // This attempt didn't result in a successful import. Discard any unconfirmed + // timeliness recording tied to it so a later, successful attempt (e.g. a retry + // from the pending/future block pool) can record fresh, accurate timeliness + // instead of being stuck with this attempt's possibly premature/invalid value. + recentChainData.invalidateUnconfirmedBlockTimeliness(block); switch (result.getFailureReason()) { case UNKNOWN_PARENT -> { // Add to the pending pool so it is triggered once the parent is imported diff --git a/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/forkchoice/ForkChoice.java b/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/forkchoice/ForkChoice.java index 1a38aa5b956..d3639a0a553 100644 --- a/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/forkchoice/ForkChoice.java +++ b/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/forkchoice/ForkChoice.java @@ -53,6 +53,7 @@ import tech.pegasys.teku.spec.SpecMilestone; import tech.pegasys.teku.spec.cache.CapturingIndexedAttestationCache; import tech.pegasys.teku.spec.cache.IndexedAttestationCache; +import tech.pegasys.teku.spec.datastructures.attestation.AttestationSource; import tech.pegasys.teku.spec.datastructures.attestation.ValidatableAttestation; import tech.pegasys.teku.spec.datastructures.blobs.versions.deneb.BlobSidecar; import tech.pegasys.teku.spec.datastructures.blocks.BeaconBlock; @@ -68,6 +69,7 @@ import tech.pegasys.teku.spec.datastructures.forkchoice.ForkChoicePayloadStatus; import tech.pegasys.teku.spec.datastructures.forkchoice.InvalidCheckpointException; import tech.pegasys.teku.spec.datastructures.forkchoice.ProtoNodeData; +import tech.pegasys.teku.spec.datastructures.forkchoice.ReadOnlyForkChoiceStrategy; import tech.pegasys.teku.spec.datastructures.forkchoice.ReadOnlyStore; import tech.pegasys.teku.spec.datastructures.forkchoice.SlotAndForkChoiceNode; import tech.pegasys.teku.spec.datastructures.forkchoice.VoteTracker; @@ -506,14 +508,21 @@ private Optional updateHeadTransaction( final BeaconState justifiedState, final Checkpoint finalizedCheckpoint, final Checkpoint justifiedCheckpoint) { - if (forkChoiceLateBlockReorgEnabled) { - recentChainData.getStore().computeBalanceThresholds(justifiedState); - } + // Balance thresholds are used both by the opt-in late-block-reorg feature and, unconditionally, + // by shouldApplyProposerBoost's weak-parent check below, so compute them regardless of the + // forkChoiceLateBlockReorgEnabled flag. + recentChainData.getStore().computeBalanceThresholds(justifiedState); final VoteUpdater transaction = recentChainData.startVoteUpdate(); final List justifiedEffectiveBalances = spec.getBeaconStateUtil(justifiedState.getSlot()) .getEffectiveActiveUnslashedBalances(justifiedState); + final Optional proposerBoostRoot = recentChainData.getStore().getProposerBoostRoot(); + final UInt64 proposerBoostAmount = + proposerBoostRoot.isPresent() && shouldApplyProposerBoost(proposerBoostRoot.get()) + ? spec.getProposerBoostAmount(justifiedState) + : UInt64.ZERO; + // If a runtime exception occurs while updating protoarray, we could skip the transaction // commit. // There is no clean way to solve it unless we move to a fully transactional protoarray update. @@ -526,8 +535,8 @@ private Optional updateHeadTransaction( finalizedCheckpoint, justifiedCheckpoint, justifiedEffectiveBalances, - recentChainData.getStore().getProposerBoostRoot(), - spec.getProposerBoostAmount(justifiedState)); + proposerBoostRoot, + proposerBoostAmount); try { recentChainData.updateHead(headNode.node(), nodeSlot.orElse(headNode.slot())); @@ -541,6 +550,24 @@ private Optional updateHeadTransaction( return recentChainData.getChainHead(); } + /** + * Spec mapping: {@code should_apply_proposer_boost(store)}. Resolves the fork-versioned {@link + * ForkChoiceUtil} for the boosted block's own slot so pre-Gloas forks keep the unconditional + * default while Gloas can suppress boost on a weak, same-slot-equivocated parent. + */ + private boolean shouldApplyProposerBoost(final Bytes32 proposerBoostRoot) { + final ReadOnlyStore store = recentChainData.getStore(); + final ReadOnlyForkChoiceStrategy forkChoiceStrategy = store.getForkChoiceStrategy(); + final Optional maybeBoostBlockSlot = forkChoiceStrategy.blockSlot(proposerBoostRoot); + if (maybeBoostBlockSlot.isEmpty()) { + return true; + } + return spec.atSlot(maybeBoostBlockSlot.get()) + .getForkChoiceUtil() + .shouldApplyProposerBoost( + recentChainData, proposerBoostRoot, forkChoiceStrategy, store.getReorgThreshold()); + } + /** * Import a block to the store. The supplied blockSlotState must already have empty slots * processed to the same slot as the block. @@ -771,8 +798,14 @@ private BlockImportResult importBlockAndState( computeEarliestBlobSidecarsSlot( recentChainData.getStore(), dataAndValidationResult, block.getMessage()); - final Optional preImportHead = - recentChainData.getChainHead().map(ChainHead::getForkChoiceNode); + // Per spec's on_block, "head" here must be get_head(store) computed fresh, immediately + // before this block is added to the store. recentChainData.getChainHead() is only a cache + // that gets refreshed by explicit updateHead()/processHead() calls, so it can lag behind the + // ForkChoiceStrategy's live vote/weight state whenever votes are applied through a path that + // doesn't refresh it (e.g. onAttestation/onAttesterSlashing, or the reference test harness + // applying skipped old-epoch attestation weights directly). Recomputing the head here avoids + // using a stale node when deciding whether to set the proposer boost root. + final ForkChoiceNode preImportHead = findNewChainHead(forkChoiceStrategy).node(); forkChoiceUtil.applyBlockToStore( transaction, @@ -783,12 +816,7 @@ private BlockImportResult importBlockAndState( earliestBlobSidecarsSlot); final boolean shouldUpdateProposerBoostRoot = - preImportHead - .filter( - forkChoiceNode -> - shouldUpdateProposerBoostRoot( - block, forkChoiceNode, forkChoiceStrategy, transaction)) - .isPresent(); + shouldUpdateProposerBoostRoot(block, preImportHead, forkChoiceStrategy, transaction); if (shouldUpdateProposerBoostRoot) { transaction.setProposerBoostRoot(block.getRoot()); } @@ -1264,7 +1292,8 @@ private boolean validateBlockAttestation( final IndexedAttestationLight attestation) { return spec.atSlot(attestation.data().getSlot()) .getForkChoiceUtil() - .validateOnAttestation(forkChoiceStrategy, currentEpoch, attestation.data()) + .validateOnAttestation( + forkChoiceStrategy, currentEpoch, attestation.data(), AttestationSource.BLOCK) .isSuccessful(); } 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 55c4bc9ff7f..7e954854567 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 @@ -78,7 +78,10 @@ public SafeFuture> getStateToValidate( attestationEpoch .plus(spec.getSpecConfig(headEpoch).getEpochsPerHistoricalVector()) .isGreaterThan(headEpoch); - if (isWithinHistoricalEpochs && isAncestorOfChainHead(chainHead.getRoot(), targetBlockRoot)) { + if (isWithinHistoricalEpochs + && isAncestorOfChainHead(chainHead.getRoot(), targetBlockRoot) + && hasMatchingShufflingDependentRoot( + chainHead.getRoot(), targetBlockRoot, attestationEpoch)) { appliedSelectorRule.labels("ancestor_of_head").inc(); return chainHead.getState().thenApply(Optional::of); } @@ -105,10 +108,12 @@ public SafeFuture> getStateToValidate( if (isWithinHistoricalEpochs) { // if it's an ancestor of any chain head within historic slots, use that chain head. final Optional maybeChainHeadData = - recentChainData.getChainHeads().stream() + recentChainData.getChainHeadsIncludingNonViable().stream() .filter( head -> - isAncestorOfChainHead(head.getRoot(), targetBlockRoot, targetBlockSlot.get())) + isAncestorOfChainHead(head.getRoot(), targetBlockRoot, targetBlockSlot.get()) + && hasMatchingShufflingDependentRoot( + head.getRoot(), targetBlockRoot, attestationEpoch)) .findFirst() .flatMap( protoNodeData -> { @@ -200,6 +205,45 @@ private Boolean isAncestorOfChainHead( .orElse(false); } + /** + * Checks that {@code candidateRoot} (a chain head, or a fork's chain head) shares the same + * shuffling-determining history as {@code targetRoot} for {@code epoch}, before we reuse {@code + * candidateRoot}'s state to compute {@code targetRoot}'s committees. + * + *

Structural ancestry alone (i.e. {@code targetRoot} lies on the chain leading to {@code + * candidateRoot}) is not sufficient: the two branches can still have diverged after {@code + * targetRoot} but before the block whose RANDAO reveal fixes the shuffling seed for {@code epoch} + * (the "shuffling dependent root", per the {@code get_shuffling_dependent_root} spec helper). If + * they diverged before that point, {@code candidateRoot}'s state has a different seed and will + * produce different committees than {@code targetRoot}'s own branch would, so its state must not + * be reused. + */ + private boolean hasMatchingShufflingDependentRoot( + final Bytes32 candidateRoot, final Bytes32 targetRoot, final UInt64 epoch) { + final Optional maybeForkChoiceStrategy = + recentChainData.getForkChoiceStrategy(); + if (maybeForkChoiceStrategy.isEmpty()) { + return false; + } + final ReadOnlyForkChoiceStrategy forkChoiceStrategy = maybeForkChoiceStrategy.get(); + final UInt64 minSeedLookahead = UInt64.valueOf(spec.getSpecConfig(epoch).getMinSeedLookahead()); + if (epoch.isLessThanOrEqualTo(minSeedLookahead)) { + // The shuffling seed for this epoch is derived from the genesis RANDAO mix on every + // branch, so there is nothing to diverge on yet. + return true; + } + // The committee shuffling for `epoch` is fixed as of the start of `epoch - + // MIN_SEED_LOOKAHEAD`, i.e. the dependent root is the ancestor at the slot immediately + // before that boundary. + final UInt64 dependentSlot = + spec.getEarliestQueryableSlotForBeaconCommitteeInTargetEpoch(epoch).minusMinZero(1); + final Optional candidateDependentRoot = + forkChoiceStrategy.getAncestor(candidateRoot, dependentSlot); + final Optional targetDependentRoot = + forkChoiceStrategy.getAncestor(targetRoot, dependentSlot); + return candidateDependentRoot.isPresent() && candidateDependentRoot.equals(targetDependentRoot); + } + private Boolean isJustifiedCheckpointOfHeadOlderOrEqualToAttestationJustifiedSlot( final ProtoNodeData head, final UInt64 justifiedBlockSlot) { final Checkpoint justifiedCheckpoint = head.getCheckpoints().getJustifiedCheckpoint(); @@ -213,7 +257,7 @@ private Boolean isJustifiedCheckpointOfHeadOlderOrEqualToAttestationJustifiedSlo private boolean isJustificationTooOld( final Bytes32 justifiedRoot, final UInt64 justifiedBlockSlot) { - return recentChainData.getChainHeads().stream() + return recentChainData.getChainHeadsIncludingNonViable().stream() // must be attesting to a viable chain .filter(head -> isAncestorOfChainHead(head.getRoot(), justifiedRoot, justifiedBlockSlot)) // must be attesting to something that progresses justification diff --git a/storage/src/main/java/tech/pegasys/teku/storage/client/BlockTimelinessTracker.java b/storage/src/main/java/tech/pegasys/teku/storage/client/BlockTimelinessTracker.java index 2a655832a01..99f8f3c1d13 100644 --- a/storage/src/main/java/tech/pegasys/teku/storage/client/BlockTimelinessTracker.java +++ b/storage/src/main/java/tech/pegasys/teku/storage/client/BlockTimelinessTracker.java @@ -25,26 +25,41 @@ import tech.pegasys.teku.spec.logic.common.util.ForkChoiceUtil; import tech.pegasys.teku.spec.logic.common.util.ForkChoiceUtil.BlockTimeliness; -/** Runtime storage for record_block_timeliness. */ +/** + * Runtime storage for record_block_timeliness. + * + *

Timeliness is first recorded speculatively (unconfirmed) as soon as a block is observed, + * before we know whether that particular import attempt will succeed. An unconfirmed recording may + * be refreshed or discarded by later events for the same block, so a premature observation (e.g. a + * block gossiped slightly before its slot starts) or one tied to an attempt that is ultimately + * rejected/deferred does not permanently pin an incorrect value. Once a block is actually, + * successfully imported, its timeliness recording is confirmed and becomes final. + */ class BlockTimelinessTracker { private static final Logger LOG = LogManager.getLogger(); private final Spec spec; private final Supplier genesisTimeMillisSupplier; - private final Map blockTimeliness; + private final Map blockTimeliness; BlockTimelinessTracker( final Spec spec, final Supplier genesisTimeMillisSupplier, - final Map blockTimeliness) { + final Map blockTimeliness) { this.spec = spec; this.genesisTimeMillisSupplier = genesisTimeMillisSupplier; this.blockTimeliness = blockTimeliness; } + /** + * Records an observation of block timeliness from an arrival time (e.g. gossip receipt). As long + * as the block hasn't yet been confirmed (see {@link #confirmBlockTimeliness}), this overwrites + * any previous, unconfirmed observation - so a later, more accurate signal always wins over a + * stale one left behind by a premature or unsuccessful earlier attempt. + */ public void setBlockTimelinessFromArrivalTime( final SignedBeaconBlock block, final UInt64 arrivalTimeMillis) { - if (blockTimeliness.get(block.getRoot()) != null) { + if (isConfirmed(block.getRoot())) { return; } if (spec.atSlot(block.getSlot()) @@ -54,7 +69,9 @@ public void setBlockTimelinessFromArrivalTime( return; } blockTimeliness.put( - block.getRoot(), computeBlockTimelinessFromArrivalTime(block, arrivalTimeMillis)); + block.getRoot(), + new TimelinessRecord( + computeBlockTimelinessFromArrivalTime(block, arrivalTimeMillis), false)); } /** @@ -63,11 +80,54 @@ public void setBlockTimelinessFromArrivalTime( */ public void setBlockTimelinessAfterDataAvailability( final SignedBeaconBlock block, final UInt64 dataAvailableTimeMillis) { - if (blockTimeliness.get(block.getRoot()) != null) { + if (isConfirmed(block.getRoot())) { return; } blockTimeliness.put( - block.getRoot(), computeBlockTimelinessFromArrivalTime(block, dataAvailableTimeMillis)); + block.getRoot(), + new TimelinessRecord( + computeBlockTimelinessFromArrivalTime(block, dataAvailableTimeMillis), false)); + } + + /** + * Discards any not-yet-confirmed timeliness recording for a block. Called whenever an import + * attempt concludes without the block being successfully imported, so that if the block is later + * imported through a separate attempt (e.g. retried from a pending/future block pool, or + * re-fetched by root), that later attempt starts from a clean slate instead of being stuck with a + * stale, possibly premature or invalid observation from the earlier attempt. A confirmed + * recording (from a previous successful import) is never discarded. + */ + public void invalidateUnconfirmedTimeliness(final Bytes32 root) { + final TimelinessRecord existing = blockTimeliness.get(root); + if (existing != null && !existing.confirmed()) { + blockTimeliness.remove(root); + } + } + + /** + * Confirms the timeliness recording for a block that has just been successfully imported. If an + * unconfirmed observation is already present, it is promoted as the final value. Otherwise (no + * observation was ever recorded for this block, e.g. it was imported directly via RPC with no + * prior gossip arrival), timeliness is computed fresh using {@code fallbackTimeMillis}. Once + * confirmed, the recording is final and will not be changed by any later call. + */ + public void confirmBlockTimeliness( + final SignedBeaconBlock block, final UInt64 fallbackTimeMillis) { + final Bytes32 root = block.getRoot(); + final TimelinessRecord existing = blockTimeliness.get(root); + if (existing == null) { + blockTimeliness.put( + root, + new TimelinessRecord( + computeBlockTimelinessFromArrivalTime(block, fallbackTimeMillis), true)); + } else if (!existing.confirmed()) { + blockTimeliness.put(root, new TimelinessRecord(existing.timeliness(), true)); + } + } + + private boolean isConfirmed(final Bytes32 root) { + final TimelinessRecord existing = blockTimeliness.get(root); + return existing != null && existing.confirmed(); } BlockTimeliness computeBlockTimelinessFromArrivalTime( @@ -105,10 +165,17 @@ BlockTimeliness computeBlockTimelinessFromArrivalTime( } public Optional getBlockTimeliness(final Bytes32 root) { - return Optional.ofNullable(blockTimeliness.get(root)); + return Optional.ofNullable(blockTimeliness.get(root)).map(TimelinessRecord::timeliness); } public boolean isBlockLate(final Bytes32 root) { - return ForkChoiceUtil.isHeadLate(Optional.ofNullable(blockTimeliness.get(root))); + return ForkChoiceUtil.isHeadLate(getBlockTimeliness(root)); } + + /** + * @param timeliness the recorded timeliness value + * @param confirmed whether this recording is tied to a block that has been successfully imported. + * Confirmed recordings are final; unconfirmed ones may still be refreshed or discarded. + */ + record TimelinessRecord(BlockTimeliness timeliness, boolean confirmed) {} } diff --git a/storage/src/main/java/tech/pegasys/teku/storage/client/RecentChainData.java b/storage/src/main/java/tech/pegasys/teku/storage/client/RecentChainData.java index c0937b4d21e..365245b8608 100644 --- a/storage/src/main/java/tech/pegasys/teku/storage/client/RecentChainData.java +++ b/storage/src/main/java/tech/pegasys/teku/storage/client/RecentChainData.java @@ -836,6 +836,12 @@ public List getChainHeads() { .orElse(Collections.emptyList()); } + public List getChainHeadsIncludingNonViable() { + return getForkChoiceStrategy() + .map((s) -> s.getChainHeads(true)) + .orElse(Collections.emptyList()); + } + public List getAllBlockRootsAtSlot(final UInt64 slot) { return getForkChoiceStrategy() .map(forkChoiceStrategy -> forkChoiceStrategy.getBlockRootsAtSlot(slot)) @@ -876,6 +882,23 @@ public void setBlockTimelinessAfterDataAvailability( blockTimelinessTracker.setBlockTimelinessAfterDataAvailability(block, dataAvailableTimeMillis); } + /** + * Discards any not-yet-confirmed timeliness recording for this block, so that if it's later + * successfully imported via a separate attempt, that attempt isn't stuck with a stale value left + * behind by this one. + */ + public void invalidateUnconfirmedBlockTimeliness(final SignedBeaconBlock block) { + blockTimelinessTracker.invalidateUnconfirmedTimeliness(block.getRoot()); + } + + /** + * Confirms (finalizes) the timeliness recording for a block that has just been successfully + * imported, refreshing it from a possibly stale/premature earlier observation if necessary. + */ + public void confirmBlockTimeliness(final SignedBeaconBlock block) { + blockTimelinessTracker.confirmBlockTimeliness(block, store.getTimeInMillis()); + } + @Override public Optional getBlockTimeliness(final Bytes32 root) { return blockTimelinessTracker.getBlockTimeliness(root); diff --git a/storage/src/test/java/tech/pegasys/teku/storage/client/BlockTimelinessTrackerTest.java b/storage/src/test/java/tech/pegasys/teku/storage/client/BlockTimelinessTrackerTest.java index 29d395c78af..54857bf5800 100644 --- a/storage/src/test/java/tech/pegasys/teku/storage/client/BlockTimelinessTrackerTest.java +++ b/storage/src/test/java/tech/pegasys/teku/storage/client/BlockTimelinessTrackerTest.java @@ -56,27 +56,115 @@ void shouldReportTimelinessIfSet() { } @Test - void shouldKeepFirstTimelyObservation() { + void shouldRefreshUnconfirmedObservationWithLaterArrival() { + // A block observation isn't confirmed until the block is actually, successfully imported, so + // a later observation (e.g. from a later import attempt) refreshes an earlier one rather than + // being stuck with it - this stops a premature or ultimately unsuccessful first arrival from + // permanently pinning an incorrect timeliness value. tracker.setBlockTimelinessFromArrivalTime( signedBlockAndState.getBlock(), computeTime(slot, 500)); tracker.setBlockTimelinessFromArrivalTime( signedBlockAndState.getBlock(), computeTime(slot, 3000)); + assertThat(tracker.getBlockTimeliness(signedBlockAndState.getRoot())) + .isPresent() + .hasValueSatisfying(timeliness -> assertThat(timeliness.isTimelyAttestation()).isFalse()); + } + + @Test + void shouldKeepConfirmedObservationEvenAfterLaterArrival() { + tracker.setBlockTimelinessFromArrivalTime( + signedBlockAndState.getBlock(), computeTime(slot, 500)); + tracker.confirmBlockTimeliness(signedBlockAndState.getBlock(), computeTime(slot, 500)); + + // Once confirmed (i.e. the block was successfully imported), the recording is final and a + // later, unrelated arrival observation must not be able to change it. + tracker.setBlockTimelinessFromArrivalTime( + signedBlockAndState.getBlock(), computeTime(slot, 3000)); + + assertThat(tracker.getBlockTimeliness(signedBlockAndState.getRoot())) + .isPresent() + .hasValueSatisfying(timeliness -> assertThat(timeliness.isTimelyAttestation()).isTrue()); + } + + @Test + void confirmBlockTimelinessShouldPromoteExistingUnconfirmedObservation() { + tracker.setBlockTimelinessFromArrivalTime( + signedBlockAndState.getBlock(), computeTime(slot, 500)); + + tracker.confirmBlockTimeliness(signedBlockAndState.getBlock(), computeTime(slot, 3000)); + + // The unconfirmed (timely) observation is promoted as-is; the fallback time passed to + // confirmBlockTimeliness is ignored since an observation was already present. + assertThat(tracker.getBlockTimeliness(signedBlockAndState.getRoot())) + .isPresent() + .hasValueSatisfying(timeliness -> assertThat(timeliness.isTimelyAttestation()).isTrue()); + } + + @Test + void confirmBlockTimelinessShouldComputeFreshWhenNoPriorObservationExists() { + // e.g. a block imported directly via RPC with no prior gossip arrival ever recorded. + tracker.confirmBlockTimeliness(signedBlockAndState.getBlock(), computeTime(slot, 500)); + + assertThat(tracker.getBlockTimeliness(signedBlockAndState.getRoot())) + .isPresent() + .hasValueSatisfying(timeliness -> assertThat(timeliness.isTimelyAttestation()).isTrue()); + } + + @Test + void invalidateUnconfirmedTimelinessShouldDiscardUnconfirmedObservation() { + tracker.setBlockTimelinessFromArrivalTime( + signedBlockAndState.getBlock(), computeTime(slot, 500)); + + tracker.invalidateUnconfirmedTimeliness(signedBlockAndState.getRoot()); + + assertThat(tracker.getBlockTimeliness(signedBlockAndState.getRoot())).isEmpty(); + } + + @Test + void invalidateUnconfirmedTimelinessShouldNotDiscardConfirmedObservation() { + tracker.setBlockTimelinessFromArrivalTime( + signedBlockAndState.getBlock(), computeTime(slot, 500)); + tracker.confirmBlockTimeliness(signedBlockAndState.getBlock(), computeTime(slot, 500)); + + tracker.invalidateUnconfirmedTimeliness(signedBlockAndState.getRoot()); + assertThat(tracker.getBlockTimeliness(signedBlockAndState.getRoot())) .isPresent() .hasValueSatisfying(timeliness -> assertThat(timeliness.isTimelyAttestation()).isTrue()); } @Test - void shouldKeepFirstLateObservation() { + void invalidateThenRetryShouldAllowFreshTimelinessToBeRecorded() { + // Simulates a block gossiped prematurely (e.g. just before its slot, within clock disparity + // tolerance), deferred, and then later retried once its slot has genuinely started. tracker.setBlockTimelinessFromArrivalTime( signedBlockAndState.getBlock(), computeTime(slot, 2100)); + tracker.invalidateUnconfirmedTimeliness(signedBlockAndState.getRoot()); + + // The retried attempt records a fresh, timely observation instead of being stuck with the + // stale, late-looking premature one. tracker.setBlockTimelinessFromArrivalTime( signedBlockAndState.getBlock(), computeTime(slot, 500)); + tracker.confirmBlockTimeliness(signedBlockAndState.getBlock(), computeTime(slot, 500)); assertThat(tracker.getBlockTimeliness(signedBlockAndState.getRoot())) .isPresent() - .hasValueSatisfying(timeliness -> assertThat(timeliness.isTimelyAttestation()).isFalse()); + .hasValueSatisfying(timeliness -> assertThat(timeliness.isTimelyAttestation()).isTrue()); + } + + @Test + void setBlockTimelinessIfAbsentShouldNotOverwriteExistingConfirmedObservation() { + tracker.setBlockTimelinessFromArrivalTime( + signedBlockAndState.getBlock(), computeTime(slot, 500)); + tracker.confirmBlockTimeliness(signedBlockAndState.getBlock(), computeTime(slot, 500)); + + tracker.setBlockTimelinessFromArrivalTime( + signedBlockAndState.getBlock(), computeTime(slot, 3000)); + + assertThat(tracker.getBlockTimeliness(signedBlockAndState.getRoot())) + .isPresent() + .hasValueSatisfying(timeliness -> assertThat(timeliness.isTimelyAttestation()).isTrue()); } @Test