Panda print only when crossing TTD the first time (#4194)

* add oldState to MergeStateHandler, move Pandas to Transition watcher Signed-off-by: garyschulte <[email protected]>
hyperledger · Jul 29, 2022 · 9b2fcde · 9b2fcde
1 parent 0d7eaf1
commit 9b2fcde
Show file tree

Hide file tree

Showing 12 changed files with 81 additions and 22 deletions.
diff --git a/besu/src/main/java/org/hyperledger/besu/controller/BesuControllerBuilder.java b/besu/src/main/java/org/hyperledger/besu/controller/BesuControllerBuilder.java
@@ -21,7 +21,6 @@
 import org.hyperledger.besu.config.GenesisConfigOptions;
 import org.hyperledger.besu.consensus.merge.FinalizedBlockHashSupplier;
 import org.hyperledger.besu.consensus.merge.MergeContext;
-import org.hyperledger.besu.consensus.merge.PandaPrinter;
 import org.hyperledger.besu.consensus.qbft.pki.PkiBlockCreationConfiguration;
 import org.hyperledger.besu.crypto.NodeKey;
 import org.hyperledger.besu.datatypes.Hash;
@@ -379,7 +378,6 @@ public BesuController build() {
     final EthContext ethContext = new EthContext(ethPeers, ethMessages, snapMessages, scheduler);
     final boolean fastSyncEnabled = !SyncMode.isFullSync(syncConfig.getSyncMode());
     final SyncState syncState = new SyncState(blockchain, ethPeers, fastSyncEnabled, checkpoint);
-    syncState.subscribeTTDReached(new PandaPrinter());
 
     final TransactionPool transactionPool =
         TransactionPoolFactory.createTransactionPool(

diff --git a/besu/src/main/java/org/hyperledger/besu/controller/TransitionBesuControllerBuilder.java b/besu/src/main/java/org/hyperledger/besu/controller/TransitionBesuControllerBuilder.java
@@ -15,6 +15,7 @@
 package org.hyperledger.besu.controller;
 
 import org.hyperledger.besu.config.GenesisConfigFile;
+import org.hyperledger.besu.consensus.merge.PandaPrinter;
 import org.hyperledger.besu.consensus.merge.PostMergeContext;
 import org.hyperledger.besu.consensus.merge.TransitionBackwardSyncContext;
 import org.hyperledger.besu.consensus.merge.TransitionContext;
@@ -180,7 +181,7 @@ private void initTransitionWatcher(
 
     PostMergeContext postMergeContext = protocolContext.getConsensusContext(PostMergeContext.class);
     postMergeContext.observeNewIsPostMergeState(
-        (isPoS, difficultyStoppedAt) -> {
+        (isPoS, priorState, difficultyStoppedAt) -> {
           if (isPoS) {
             // if we transitioned to post-merge, stop and disable any mining
             composedCoordinator.getPreMergeObject().disable();
@@ -190,6 +191,11 @@ private void initTransitionWatcher(
                 .getBlockchain()
                 .setBlockChoiceRule((newBlockHeader, currentBlockHeader) -> -1);
 
+            if (priorState.filter(prior -> !prior).isPresent()) {
+              // only print pandas if we had a prior merge state, and it was false
+              PandaPrinter.printOnFirstCrossing();
+            }
+
           } else if (composedCoordinator.isMiningBeforeMerge()) {
             // if our merge state is set to mine pre-merge and we are mining, start mining
             composedCoordinator.getPreMergeObject().enable();

diff --git a/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/PandaPrinter.java b/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/PandaPrinter.java
@@ -16,8 +16,6 @@
 
 package org.hyperledger.besu.consensus.merge;
 
-import org.hyperledger.besu.plugin.services.BesuEvents.TTDReachedListener;
-
 import java.io.BufferedReader;
 import java.io.IOException;
 import java.io.InputStream;
@@ -28,11 +26,11 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-public class PandaPrinter implements TTDReachedListener {
+public class PandaPrinter {
 
   private static final Logger LOG = LoggerFactory.getLogger(PandaPrinter.class);
   private static final String pandaBanner = PandaPrinter.loadBanner();
-  private final AtomicBoolean beenDisplayed = new AtomicBoolean();
+  private static final AtomicBoolean beenDisplayed = new AtomicBoolean();
 
   private static String loadBanner() {
     Class<PandaPrinter> c = PandaPrinter.class;
@@ -50,10 +48,19 @@ private static String loadBanner() {
     return resultStringBuilder.toString();
   }
 
-  @Override
-  public void onTTDReached(final boolean reached) {
-    if (reached && beenDisplayed.compareAndSet(false, true)) {
+  public static boolean printOnFirstCrossing() {
+    boolean shouldPrint = beenDisplayed.compareAndSet(false, true);
+    if (shouldPrint) {
       LOG.info("\n" + pandaBanner);
     }
+    return shouldPrint;
+  }
+
+  static boolean hasDisplayed() {
+    return beenDisplayed.get();
+  }
+
+  static void resetForTesting() {
+    beenDisplayed.set(false);
   }
 }
diff --git a/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/PostMergeContext.java b/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/PostMergeContext.java
@@ -57,7 +57,12 @@ public class PostMergeContext implements MergeContext {
 
   @VisibleForTesting
   PostMergeContext() {
-    this.terminalTotalDifficulty = new AtomicReference<>(Difficulty.ZERO);
+    this(Difficulty.ZERO);
+  }
+
+  @VisibleForTesting
+  PostMergeContext(final Difficulty difficulty) {
+    this.terminalTotalDifficulty = new AtomicReference<>(difficulty);
     this.syncState = new AtomicReference<>();
   }
 
@@ -100,7 +105,8 @@ public void setIsPostMerge(final Difficulty totalDifficulty) {
     if (oldState.isEmpty() || oldState.get() != newState) {
       newMergeStateCallbackSubscribers.forEach(
           newMergeStateCallback ->
-              newMergeStateCallback.mergeStateChanged(newState, Optional.of(totalDifficulty)));
+              newMergeStateCallback.mergeStateChanged(
+                  newState, oldState, Optional.of(totalDifficulty)));
     }
   }
 

diff --git a/...erge/src/main/java/org/hyperledger/besu/consensus/merge/TransitionBestPeerComparator.java b/...erge/src/main/java/org/hyperledger/besu/consensus/merge/TransitionBestPeerComparator.java
@@ -56,7 +56,9 @@ public TransitionBestPeerComparator(final Difficulty configuredTerminalTotalDiff
 
   @Override
   public void mergeStateChanged(
-      final boolean isPoS, final Optional<Difficulty> difficultyStoppedAt) {
+      final boolean isPoS,
+      final Optional<Boolean> oldState,
+      final Optional<Difficulty> difficultyStoppedAt) {
     if (isPoS && difficultyStoppedAt.isPresent()) {
       terminalTotalDifficulty.set(difficultyStoppedAt.get());
     }

diff --git a/consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/PandaPrinterTest.java b/consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/PandaPrinterTest.java
@@ -16,13 +16,48 @@
 
 package org.hyperledger.besu.consensus.merge;
 
+import static org.assertj.core.api.Assertions.assertThat;
+
+import org.hyperledger.besu.ethereum.core.Difficulty;
+
 import org.junit.Test;
 
 public class PandaPrinterTest {
 
+  final MergeStateHandler fauxTransitionHandler =
+      (isPoS, priorState, ttd) -> {
+        if (isPoS && priorState.filter(prior -> !prior).isPresent())
+          PandaPrinter.printOnFirstCrossing();
+      };
+
   @Test
   public void printsPanda() {
-    PandaPrinter panda = new PandaPrinter();
-    panda.onTTDReached(true);
+    PandaPrinter.resetForTesting();
+    assertThat(PandaPrinter.printOnFirstCrossing()).isTrue();
+    assertThat(PandaPrinter.printOnFirstCrossing()).isFalse();
+  }
+
+  @Test
+  public void doesNotPrintAtInit() {
+    PandaPrinter.resetForTesting();
+    var mergeContext = new PostMergeContext(Difficulty.ONE);
+    mergeContext.observeNewIsPostMergeState(fauxTransitionHandler);
+
+    assertThat(PandaPrinter.hasDisplayed()).isFalse();
+    mergeContext.setIsPostMerge(Difficulty.ONE);
+    assertThat(PandaPrinter.hasDisplayed()).isFalse();
+  }
+
+  @Test
+  public void printsWhenCrossingOnly() {
+    PandaPrinter.resetForTesting();
+    var mergeContext = new PostMergeContext(Difficulty.ONE);
+    mergeContext.observeNewIsPostMergeState(fauxTransitionHandler);
+
+    assertThat(PandaPrinter.hasDisplayed()).isFalse();
+    mergeContext.setIsPostMerge(Difficulty.ZERO);
+    assertThat(PandaPrinter.hasDisplayed()).isFalse();
+    mergeContext.setIsPostMerge(Difficulty.ONE);
+    assertThat(PandaPrinter.hasDisplayed()).isTrue();
   }
 }
diff --git a/consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/PostMergeContextTest.java b/consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/PostMergeContextTest.java
@@ -173,7 +173,9 @@ private static class MergeStateChangeCollector implements MergeStateHandler {
 
     @Override
     public void mergeStateChanged(
-        final boolean isPoS, final Optional<Difficulty> difficultyStoppedAt) {
+        final boolean isPoS,
+        final Optional<Boolean> oldState,
+        final Optional<Difficulty> difficultyStoppedAt) {
       stateChanges.add(isPoS);
     }
 

diff --git a/.../src/test/java/org/hyperledger/besu/consensus/merge/TransitionBestPeerComparatorTest.java b/.../src/test/java/org/hyperledger/besu/consensus/merge/TransitionBestPeerComparatorTest.java
@@ -60,7 +60,7 @@ public void assertHandlesNewTTD() {
     assertThat(comparator.compare(a, b)).isEqualTo(-1);
 
     // update TTD with actual value
-    comparator.mergeStateChanged(true, Optional.of(Difficulty.of(5002)));
+    comparator.mergeStateChanged(true, Optional.empty(), Optional.of(Difficulty.of(5002)));
     assertThat(comparator.compare(a, b)).isEqualTo(1);
   }
 }
diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/consensus/merge/MergeStateHandler.java b/ethereum/eth/src/main/java/org/hyperledger/besu/consensus/merge/MergeStateHandler.java
@@ -22,5 +22,8 @@
 
 public interface MergeStateHandler {
 
-  void mergeStateChanged(final boolean isPoS, final Optional<Difficulty> difficultyStoppedAt);
+  void mergeStateChanged(
+      final boolean isPoS,
+      final Optional<Boolean> priorState,
+      final Optional<Difficulty> difficultyStoppedAt);
 }
diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/MergePeerFilter.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/MergePeerFilter.java
@@ -88,7 +88,9 @@ public void onNewForkchoiceMessage(
 
   @Override
   public void mergeStateChanged(
-      final boolean isPoS, final Optional<Difficulty> difficultyStoppedAt) {
+      final boolean isPoS,
+      final Optional<Boolean> oldState,
+      final Optional<Difficulty> difficultyStoppedAt) {
     if (isPoS && difficultyStoppedAt.isPresent()) {
       LOG.debug("terminal difficulty set to {}", difficultyStoppedAt.get().getValue());
       long lockStamp = this.powTerminalDifficultyLock.writeLock();

diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/state/SyncState.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/state/SyncState.java
@@ -177,8 +177,6 @@ public boolean isInSync(final long syncTolerance) {
   }
 
   public void setReachedTerminalDifficulty(final boolean stoppedAtTerminalDifficulty) {
-    // TODO: this is an inexpensive way to stop sync when we reach TTD,
-    //      we should revisit when merge sync is better defined
     this.reachedTerminalDifficulty = Optional.of(stoppedAtTerminalDifficulty);
     ttdReachedListeners.forEach(listener -> listener.onTTDReached(stoppedAtTerminalDifficulty));
   }

diff --git a/...m/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/EthProtocolManagerTest.java b/...m/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/EthProtocolManagerTest.java
@@ -235,7 +235,7 @@ public void disconnectNewPoWPeers() {
       ethManager.processMessage(EthProtocol.ETH63, new DefaultMessage(stakePeer, stakePeerStatus));
 
       mergePeerFilter.mergeStateChanged(
-          true, Optional.of(blockchain.getChainHead().getTotalDifficulty()));
+          true, Optional.empty(), Optional.of(blockchain.getChainHead().getTotalDifficulty()));
       mergePeerFilter.onNewForkchoiceMessage(
           Hash.EMPTY, Optional.of(Hash.hash(Bytes.of(1))), Hash.EMPTY);
       mergePeerFilter.onNewForkchoiceMessage(