When block not found, throw instead of defaulting to the potentially …

…wrong schedule This error is likely to happen and a followup PR should update getByBlockNumber to use getByBlockHeader Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
jframe · Dec 8, 2022 · 2756a9d · 2756a9d
1 parent 607ac75
commit 2756a9d
Show file tree

Hide file tree

Showing 2 changed files with 19 additions and 10 deletions.
diff --git a/.../merge/src/main/java/org/hyperledger/besu/consensus/merge/TransitionProtocolSchedule.java b/.../merge/src/main/java/org/hyperledger/besu/consensus/merge/TransitionProtocolSchedule.java
@@ -121,11 +121,18 @@ public ProtocolSpec getByBlockHeaderFromTransitionUtils(
   @Override
   public ProtocolSpec getByBlockNumber(final long number) {
 
-    return Optional.ofNullable(protocolContext)
-        .map(ProtocolContext::getBlockchain)
-        .flatMap(blockchain -> blockchain.getBlockByNumber(number))
-        .map(Block::getHeader)
-        .map(timestampSchedule::getByBlockHeader)
+    Block block =
+        Optional.ofNullable(protocolContext)
+            .map(ProtocolContext::getBlockchain)
+            .flatMap(blockchain -> blockchain.getBlockByNumber(number))
+            .orElseThrow(
+                () ->
+                    new IllegalStateException(
+                        String.format(
+                            "Cannot determine protocol schedule based on block number %s since block does not exist in the chain",
+                            number)));
+
+    return Optional.ofNullable(timestampSchedule.getByBlockHeader(block.getHeader()))
         .orElseGet(
             () ->
                 transitionUtils.dispatchFunctionAccordingToMergeState(

diff --git a/...ge/src/test/java/org/hyperledger/besu/consensus/merge/TransitionProtocolScheduleTest.java b/...ge/src/test/java/org/hyperledger/besu/consensus/merge/TransitionProtocolScheduleTest.java
@@ -15,6 +15,7 @@
 package org.hyperledger.besu.consensus.merge;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyNoInteractions;
@@ -173,13 +174,14 @@ public void getByBlockNumber_returnsTimestampScheduleIfPresent() {
   }
 
   @Test
-  public void getByBlockNumber_delegatesToMergeScheduleWhenBlockNotFound() {
+  public void getByBlockNumber_throwsWhenBlockNotFound() {
     when(blockchain.getBlockByNumber(BLOCK_NUMBER)).thenReturn(Optional.empty());
-    when(mergeContext.isPostMerge()).thenReturn(false);
-
-    transitionProtocolSchedule.getByBlockNumber(BLOCK_NUMBER);
 
-    verifyPreMergeProtocolScheduleReturned();
+    assertThatThrownBy(() -> transitionProtocolSchedule.getByBlockNumber(BLOCK_NUMBER))
+        .isInstanceOf(IllegalStateException.class)
+        .hasMessage(
+            "Cannot determine protocol schedule based on block number %s since block does not exist in the chain",
+            BLOCK_NUMBER);
   }
 
   @Test