From a51b2439e9db63fff8b136e9b3841d432e66d312 Mon Sep 17 00:00:00 2001 From: Paul Harris Date: Tue, 28 Nov 2023 07:09:08 +1000 Subject: [PATCH] Implemented get_proposer_head for late block reorg (#7753) Implemented get_proposer_head function, and tested results via the reftests. It's going to be more efficient to get the justified state once, so have refactored isHeadWeak and isParentStrong. The ordering of checks was cheapest -> most expensive, we can avoid fetching state if any of the lighter tests fail. partially addresses #6595 Signed-off-by: Paul Harris --- .../forkchoice/ForkChoiceTestExecutor.java | 12 +- .../forkchoice/ReadOnlyStore.java | 4 +- .../forkchoice/TestStoreImpl.java | 8 +- .../teku/storage/client/RecentChainData.java | 69 ++++++++++ .../pegasys/teku/storage/store/Store.java | 123 ++++++------------ .../teku/storage/store/StoreTransaction.java | 8 +- .../pegasys/teku/storage/store/StoreTest.java | 28 ++-- 7 files changed, 137 insertions(+), 115 deletions(-) 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 5ba20ccd6e1..058caca916e 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 @@ -464,14 +464,10 @@ private void applyChecks( case "get_proposer_head" -> { final Bytes32 expectedProposerHead = getBytes32(checks, checkType); - final Optional boostedRoot = recentChainData.getBestBlockRoot(); - if (expectedProposerHead.isZero()) { - assertThat(boostedRoot).describedAs("get_proposer_head").isEmpty(); - } else { - assertThat(boostedRoot) - .describedAs("get_proposer_head") - .contains(expectedProposerHead); - } + final Bytes32 root = + recentChainData.getProposerHead( + expectedProposerHead, recentChainData.getHeadSlot().increment()); + assertThat(root).describedAs("get_proposer_head").isEqualTo(expectedProposerHead); } case "should_override_forkchoice_update" -> { diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/datastructures/forkchoice/ReadOnlyStore.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/datastructures/forkchoice/ReadOnlyStore.java index 9e07bf30e31..5d9080c089c 100644 --- a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/datastructures/forkchoice/ReadOnlyStore.java +++ b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/datastructures/forkchoice/ReadOnlyStore.java @@ -136,10 +136,10 @@ SafeFuture> retrieveCheckpointState( Checkpoint checkpoint, BeaconState latestStateAtEpoch); // implements is_head_weak from fork-choice Consensus Spec - SafeFuture> isHeadWeak(final Bytes32 root); + boolean isHeadWeak(BeaconState justifiedState, Bytes32 root); // implements is_parent_strong from fork-choice Consensus Spec - SafeFuture> isParentStrong(final Bytes32 parentRoot); + boolean isParentStrong(BeaconState justifiedState, Bytes32 parentRoot); // implements is_ffg_competitive from Consensus Spec Optional isFfgCompetitive(final Bytes32 headRoot, final Bytes32 parentRoot); diff --git a/ethereum/spec/src/testFixtures/java/tech/pegasys/teku/spec/datastructures/forkchoice/TestStoreImpl.java b/ethereum/spec/src/testFixtures/java/tech/pegasys/teku/spec/datastructures/forkchoice/TestStoreImpl.java index 574e0149882..3d6fe727c85 100644 --- a/ethereum/spec/src/testFixtures/java/tech/pegasys/teku/spec/datastructures/forkchoice/TestStoreImpl.java +++ b/ethereum/spec/src/testFixtures/java/tech/pegasys/teku/spec/datastructures/forkchoice/TestStoreImpl.java @@ -247,13 +247,13 @@ public SafeFuture> retrieveCheckpointState( } @Override - public SafeFuture> isHeadWeak(Bytes32 root) { - return SafeFuture.completedFuture(Optional.empty()); + public boolean isHeadWeak(BeaconState justifiedState, Bytes32 root) { + return false; } @Override - public SafeFuture> isParentStrong(Bytes32 parentRoot) { - return SafeFuture.completedFuture(Optional.empty()); + public boolean isParentStrong(BeaconState justifiedState, Bytes32 parentRoot) { + return false; } @Override 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 27654860a93..c76c02fc245 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 @@ -21,6 +21,7 @@ import java.util.NavigableMap; import java.util.Optional; import java.util.TreeMap; +import java.util.concurrent.CancellationException; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicBoolean; import org.apache.logging.log4j.LogManager; @@ -619,6 +620,74 @@ public List getAllBlockRootsAtSlot(final UInt64 slot) { .orElse(Collections.emptyList()); } + public Bytes32 getProposerHead(final Bytes32 headRoot, final UInt64 slot) { + // if proposer boost is still active, don't attempt to override head + final boolean isProposerBoostActive = + store.getProposerBoostRoot().map(root -> !root.equals(headRoot)).orElse(false); + final boolean isShufflingStable = spec.atSlot(slot).getForkChoiceUtil().isShufflingStable(slot); + final boolean isFinalizationOk = + spec.atSlot(slot).getForkChoiceUtil().isFinalizationOk(store, slot); + final boolean isProposingOnTime = isProposingOnTime(slot); + final boolean isHeadLate = isBlockLate(headRoot); + final Optional maybeHead = store.getBlockIfAvailable(headRoot); + + // to return parent root, we need all of (isHeadLate, isShufflingStable, isFfgCompetetive, + // isFinalizationOk, isProposingOnTime, isSingleSlotReorg, isHeadWeak, isParentStrong) + + // cheap checks of that list: + // (isHeadLate, isShufflingStable, isFinalizationOk, isProposingOnTime); + // and isProposerBoostActive (assert condition); + // finally need head block to make further checks + if (!isHeadLate + || !isShufflingStable + || !isFinalizationOk + || !isProposingOnTime + || isProposerBoostActive + || maybeHead.isEmpty()) { + return headRoot; + } + + final SignedBeaconBlock head = maybeHead.get(); + final boolean isFfgCompetitive = + store.isFfgCompetitive(headRoot, head.getParentRoot()).orElse(false); + + final Optional maybeParentSlot = getSlotForBlockRoot(head.getParentRoot()); + final boolean isParentSlotOk = + maybeParentSlot.map(uInt64 -> uInt64.increment().equals(head.getSlot())).orElse(false); + final boolean isCurrentTimeOk = head.getSlot().increment().equals(slot); + final boolean isSingleSlotReorg = isParentSlotOk && isCurrentTimeOk; + + // from the initial list, check + // isFfgCompetitive, isSingleSlotReorg + if (!isFfgCompetitive || !isSingleSlotReorg) { + return headRoot; + } + + final SafeFuture> future = + store.retrieveCheckpointState(store.getJustifiedCheckpoint()); + try { + final Optional maybeJustifiedState = future.join(); + // to make further checks, we would need the justified state, return headRoot if we don't have + // it. + if (maybeJustifiedState.isEmpty()) { + return headRoot; + } + final boolean isHeadWeak = store.isHeadWeak(maybeJustifiedState.get(), headRoot); + final boolean isParentStrong = + store.isParentStrong(maybeJustifiedState.get(), head.getParentRoot()); + // finally, the parent must be strong, and the current head must be weak. + if (isHeadWeak && isParentStrong) { + return head.getParentRoot(); + } + } catch (Exception exception) { + if (!(exception instanceof CancellationException)) { + LOG.error("Failed to get justified checkpoint", exception); + } + } + + return headRoot; + } + public void setBlockTimelinessFromArrivalTime( final SignedBeaconBlock block, final UInt64 arrivalTime) { blockTimelinessTracker.setBlockTimelinessFromArrivalTime(block, arrivalTime); diff --git a/storage/src/main/java/tech/pegasys/teku/storage/store/Store.java b/storage/src/main/java/tech/pegasys/teku/storage/store/Store.java index e9ae37ef04a..d424072b521 100644 --- a/storage/src/main/java/tech/pegasys/teku/storage/store/Store.java +++ b/storage/src/main/java/tech/pegasys/teku/storage/store/Store.java @@ -535,94 +535,55 @@ public Optional getBlockIfAvailable(final Bytes32 blockRoot) } @Override - public SafeFuture> isHeadWeak(final Bytes32 root) { - + public boolean isHeadWeak(final BeaconState justifiedState, final Bytes32 root) { final Optional maybeBlockData = getBlockDataFromForkChoiceStrategy(root); - if (maybeBlockData.isEmpty()) { - LOG.trace("isHeadWeak {}: no block data in protoArray, returning false", root); - return SafeFuture.completedFuture(Optional.empty()); - } - - final UInt64 headWeight = maybeBlockData.get().getWeight(); - - final SafeFuture> futureMaybeJustifiedState = - retrieveCheckpointState(justifiedCheckpoint); - - return futureMaybeJustifiedState - .thenApply( - maybeJustifiedState -> - maybeJustifiedState.map( - justifiedState -> { - final SpecVersion specVersion = spec.atSlot(justifiedState.getSlot()); - final BeaconStateAccessors beaconStateAccessors = - specVersion.beaconStateAccessors(); - final UInt64 reorgThreashold = - beaconStateAccessors.calculateCommitteeFraction( - justifiedState, - specVersion.getConfig().getReorgHeadWeightThreshold()); - final boolean result = headWeight.isLessThan(reorgThreashold); - - LOG.trace( - "isHeadWeak {}: headWeight: {}, reorgThreshold: {}, result: {}", - root, - headWeight, - reorgThreashold, - result); - return result; - })) - .exceptionallyCompose( - err -> { - LOG.error( - "Failed to retrieve justified state when checking head weight for block {}.", + return maybeBlockData + .map( + blockData -> { + final UInt64 headWeight = blockData.getWeight(); + + final SpecVersion specVersion = spec.atSlot(justifiedState.getSlot()); + final BeaconStateAccessors beaconStateAccessors = specVersion.beaconStateAccessors(); + final UInt64 reorgThreshold = + beaconStateAccessors.calculateCommitteeFraction( + justifiedState, specVersion.getConfig().getReorgHeadWeightThreshold()); + final boolean result = headWeight.isLessThan(reorgThreshold); + + LOG.trace( + "isHeadWeak {}: headWeight: {}, reorgThreshold: {}, result: {}", root, - err); - return SafeFuture.completedFuture(Optional.empty()); - }); + headWeight, + reorgThreshold, + result); + return result; + }) + .orElse(false); } @Override - public SafeFuture> isParentStrong(final Bytes32 parentRoot) { + public boolean isParentStrong(final BeaconState justifiedState, final Bytes32 parentRoot) { final Optional maybeBlockData = getBlockDataFromForkChoiceStrategy(parentRoot); - if (maybeBlockData.isEmpty()) { - LOG.trace("isParentStrong {}: no block data in protoArray, returning false", parentRoot); - return SafeFuture.completedFuture(Optional.empty()); - } - - final UInt64 parentWeight = maybeBlockData.get().getWeight(); - - final SafeFuture> futureMaybeJustifiedState = - retrieveCheckpointState(justifiedCheckpoint); - - return futureMaybeJustifiedState - .thenApply( - maybeJustifiedState -> - maybeJustifiedState.map( - justifiedState -> { - final SpecVersion specVersion = spec.atSlot(justifiedState.getSlot()); - final BeaconStateAccessors beaconStateAccessors = - specVersion.beaconStateAccessors(); - final UInt64 parentThreshold = - beaconStateAccessors.calculateCommitteeFraction( - justifiedState, - specVersion.getConfig().getReorgParentWeightThreshold()); - final boolean result = parentWeight.isGreaterThan(parentThreshold); - - LOG.trace( - "isParentStrong {}: parentWeight: {}, parentThreshold: {}, result: {}", - parentRoot, - parentWeight, - parentThreshold, - result); - return result; - })) - .exceptionallyCompose( - err -> { - LOG.error( - "Failed to retrieve justified state when checking weight for parent {}.", + return maybeBlockData + .map( + blockData -> { + final UInt64 parentWeight = blockData.getWeight(); + + final SpecVersion specVersion = spec.atSlot(justifiedState.getSlot()); + final BeaconStateAccessors beaconStateAccessors = specVersion.beaconStateAccessors(); + final UInt64 parentThreshold = + beaconStateAccessors.calculateCommitteeFraction( + justifiedState, specVersion.getConfig().getReorgParentWeightThreshold()); + final boolean result = parentWeight.isGreaterThan(parentThreshold); + + LOG.debug( + "isParentStrong {}: parentWeight: {}, parentThreshold: {}, result: {}", parentRoot, - err); - return SafeFuture.completedFuture(Optional.empty()); - }); + parentWeight, + parentThreshold, + result); + return result; + }) + .orElse(false); } @Override diff --git a/storage/src/main/java/tech/pegasys/teku/storage/store/StoreTransaction.java b/storage/src/main/java/tech/pegasys/teku/storage/store/StoreTransaction.java index 595e502b1f5..51d5286bbbd 100644 --- a/storage/src/main/java/tech/pegasys/teku/storage/store/StoreTransaction.java +++ b/storage/src/main/java/tech/pegasys/teku/storage/store/StoreTransaction.java @@ -452,13 +452,13 @@ public SafeFuture> retrieveCheckpointState( } @Override - public SafeFuture> isHeadWeak(Bytes32 root) { - return store.isHeadWeak(root); + public boolean isHeadWeak(BeaconState justifiedState, Bytes32 root) { + return store.isHeadWeak(justifiedState, root); } @Override - public SafeFuture> isParentStrong(Bytes32 parentRoot) { - return store.isParentStrong(parentRoot); + public boolean isParentStrong(BeaconState justifiedState, Bytes32 parentRoot) { + return store.isParentStrong(justifiedState, parentRoot); } @Override diff --git a/storage/src/test/java/tech/pegasys/teku/storage/store/StoreTest.java b/storage/src/test/java/tech/pegasys/teku/storage/store/StoreTest.java index 3e2170c0458..cdb241b071d 100644 --- a/storage/src/test/java/tech/pegasys/teku/storage/store/StoreTest.java +++ b/storage/src/test/java/tech/pegasys/teku/storage/store/StoreTest.java @@ -100,8 +100,7 @@ public void isHeadWeak_withoutNodeData() { processChainHeadWithMockForkChoiceStrategy( (store, blockAndState) -> { final Bytes32 root = blockAndState.getRoot(); - final SafeFuture> isHeadWeakFuture = store.isHeadWeak(root); - assertThat(safeJoin(isHeadWeakFuture)).isEmpty(); + assertThat(store.isHeadWeak(justifiedState(store), root)).isFalse(); }); } @@ -112,8 +111,7 @@ public void isHeadWeak_withSufficientWeightIsFalse() { final Bytes32 root = blockAndState.getRoot(); setProtoNodeDataForBlock(blockAndState, UInt64.valueOf("2400000001"), UInt64.MAX_VALUE); - final SafeFuture> isHeadWeakFuture = store.isHeadWeak(root); - assertThat(safeJoin(isHeadWeakFuture)).contains(false); + assertThat(store.isHeadWeak(justifiedState(store), root)).isFalse(); }); } @@ -124,8 +122,7 @@ public void isHeadWeak_Boundary() { final Bytes32 root = blockAndState.getRoot(); setProtoNodeDataForBlock(blockAndState, UInt64.valueOf("2399999999"), UInt64.MAX_VALUE); - final SafeFuture> isHeadWeakFuture = store.isHeadWeak(root); - assertThat(safeJoin(isHeadWeakFuture)).contains(true); + assertThat(store.isHeadWeak(justifiedState(store), root)).isTrue(); }); } @@ -136,8 +133,7 @@ public void isHeadWeak_withLowWeightIsTrue() { final Bytes32 root = blockAndState.getRoot(); setProtoNodeDataForBlock(blockAndState, UInt64.valueOf("1000000000"), UInt64.MAX_VALUE); - final SafeFuture> isHeadWeakFuture = store.isHeadWeak(root); - assertThat(safeJoin(isHeadWeakFuture)).contains(true); + assertThat(store.isHeadWeak(justifiedState(store), root)).isTrue(); }); } @@ -146,8 +142,7 @@ public void isParentStrong_withoutNodeData() { processChainHeadWithMockForkChoiceStrategy( (store, blockAndState) -> { final Bytes32 root = blockAndState.getBlock().getParentRoot(); - final SafeFuture> isParentStrongFuture = store.isParentStrong(root); - assertThat(safeJoin(isParentStrongFuture)).isEmpty(); + assertThat(store.isParentStrong(justifiedState(store), root)).isFalse(); }); } @@ -157,8 +152,7 @@ public void isParentStrong_withSufficientWeight() { (store, blockAndState) -> { final Bytes32 root = blockAndState.getBlock().getParentRoot(); setProtoNodeDataForBlock(blockAndState, UInt64.ZERO, UInt64.valueOf("19200000001")); - final SafeFuture> isParentStrongFuture = store.isParentStrong(root); - assertThat(safeJoin(isParentStrongFuture)).contains(true); + assertThat(store.isParentStrong(justifiedState(store), root)).isTrue(); }); } @@ -168,8 +162,7 @@ public void isParentStrong_wityBoundaryWeight() { (store, blockAndState) -> { final Bytes32 root = blockAndState.getBlock().getParentRoot(); setProtoNodeDataForBlock(blockAndState, UInt64.ZERO, UInt64.valueOf("19200000000")); - final SafeFuture> isParentStrongFuture = store.isParentStrong(root); - assertThat(safeJoin(isParentStrongFuture)).contains(false); + assertThat(store.isParentStrong(justifiedState(store), root)).isFalse(); }); } @@ -179,8 +172,7 @@ public void isParentStrong_wityZeroWeight() { (store, blockAndState) -> { final Bytes32 root = blockAndState.getBlock().getParentRoot(); setProtoNodeDataForBlock(blockAndState, UInt64.ZERO, UInt64.ZERO); - final SafeFuture> isParentStrongFuture = store.isParentStrong(root); - assertThat(safeJoin(isParentStrongFuture)).contains(false); + assertThat(store.isParentStrong(justifiedState(store), root)).isFalse(); }); } @@ -525,6 +517,10 @@ public void shouldKeepOnlyMostRecentBlocksInBlockCache() { } } + private BeaconState justifiedState(final UpdatableStore store) { + return safeJoin(store.retrieveCheckpointState(store.getJustifiedCheckpoint())).orElseThrow(); + } + private void testApplyChangesWhenTransactionCommits(final boolean withInterleavedTransaction) { final UpdatableStore store = createGenesisStore(); final UInt64 epoch3 = UInt64.valueOf(4);