diff --git a/CHANGELOG.md b/CHANGELOG.md index 2f8f150b809..fe5a8d11ce4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,7 @@ - Print an overview of configuration and system information at startup [#4451](https://github.com/hyperledger/besu/pull/4451) ### Bug Fixes +- Restore updating chain head and finalized block during backward sync [#4718](https://github.com/hyperledger/besu/pull/4718) ### Download Links @@ -76,7 +77,6 @@ https://hyperledger.jfrog.io/hyperledger/besu-binaries/besu/22.10.0/besu-22.10.0.tar.gz / sha256: 88fb5df567e4ec3547d7d2970cfef00debbd020c0da66b19166d43779b3b2b85 https://hyperledger.jfrog.io/hyperledger/besu-binaries/besu/22.10.0/besu-22.10.0.zip / sha256: c8e39f7c879409cb9b47f4d3de5e9c521249083830a8c9a45e8a14a319fe195d - ## 22.10.0-RC2 ### Breaking Changes diff --git a/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinator.java b/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinator.java index 9d84402abe1..8e17715bf75 100644 --- a/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinator.java +++ b/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinator.java @@ -390,33 +390,45 @@ private boolean canRetryBlockCreation(final Throwable throwable) { } @Override - public Optional getOrSyncHeaderByHash(final Hash blockHash) { + public Optional getOrSyncHeadByHash(final Hash headHash, final Hash finalizedHash) { final var chain = protocolContext.getBlockchain(); - final var optHeader = chain.getBlockHeader(blockHash); + final var maybeHeadHeader = chain.getBlockHeader(headHash); - if (optHeader.isPresent()) { - debugLambda(LOG, "BlockHeader {} is already present", () -> optHeader.get().toLogString()); + if (maybeHeadHeader.isPresent()) { + debugLambda(LOG, "BlockHeader {} is already present", maybeHeadHeader.get()::toLogString); } else { - debugLambda(LOG, "appending block hash {} to backward sync", blockHash::toHexString); - backwardSyncContext.syncBackwardsUntil(blockHash); + debugLambda(LOG, "Appending new head block hash {} to backward sync", headHash::toHexString); + backwardSyncContext.updateHead(headHash); + backwardSyncContext + .syncBackwardsUntil(headHash) + .thenRun(() -> updateFinalized(finalizedHash)); } - return optHeader; + return maybeHeadHeader; } - @Override - public Optional getOrSyncHeaderByHash( - final Hash blockHash, final Hash finalizedBlockHash) { - final var chain = protocolContext.getBlockchain(); - final var optHeader = chain.getBlockHeader(blockHash); - - if (optHeader.isPresent()) { - debugLambda(LOG, "BlockHeader {} is already present", () -> optHeader.get().toLogString()); - } else { - debugLambda(LOG, "appending block hash {} to backward sync", blockHash::toHexString); - backwardSyncContext.updateHeads(blockHash, finalizedBlockHash); - backwardSyncContext.syncBackwardsUntil(blockHash); + private void updateFinalized(final Hash finalizedHash) { + if (mergeContext + .getFinalized() + .map(BlockHeader::getHash) + .map(finalizedHash::equals) + .orElse(Boolean.FALSE)) { + LOG.debug("Finalized block already set to {}, nothing to do", finalizedHash); + return; } - return optHeader; + + protocolContext + .getBlockchain() + .getBlockHeader(finalizedHash) + .ifPresentOrElse( + finalizedHeader -> { + debugLambda( + LOG, "Setting finalized block header to {}", finalizedHeader::toLogString); + mergeContext.setFinalized(finalizedHeader); + }, + () -> + LOG.warn( + "Internal error, backward sync completed but failed to import finalized block {}", + finalizedHash)); } @Override @@ -693,12 +705,11 @@ private Optional findValidAncestor( @Override public boolean isDescendantOf(final BlockHeader ancestorBlock, final BlockHeader newBlock) { - LOG.debug( - "checking if block {}:{} is ancestor of {}:{}", - ancestorBlock.getNumber(), - ancestorBlock.getBlockHash(), - newBlock.getNumber(), - newBlock.getBlockHash()); + debugLambda( + LOG, + "checking if block {} is ancestor of {}", + ancestorBlock::toLogString, + newBlock::toLogString); // start with self, because descending from yourself is valid Optional parentOf = Optional.of(newBlock); @@ -714,10 +725,11 @@ public boolean isDescendantOf(final BlockHeader ancestorBlock, final BlockHeader && ancestorBlock.getBlockHash().equals(parentOf.get().getBlockHash())) { return true; } else { - LOG.debug( + debugLambda( + LOG, "looped all the way back, did not find ancestor {} of child {}", - ancestorBlock.getBlockHash(), - newBlock.getBlockHash()); + ancestorBlock::toLogString, + newBlock::toLogString); return false; } } diff --git a/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeMiningCoordinator.java b/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeMiningCoordinator.java index 65e96779a4e..79fedf90734 100644 --- a/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeMiningCoordinator.java +++ b/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeMiningCoordinator.java @@ -63,12 +63,10 @@ ForkchoiceResult updateForkChoice( CompletableFuture appendNewPayloadToSync(Block newPayload); - Optional getOrSyncHeaderByHash(Hash blockHash); + Optional getOrSyncHeadByHash(Hash headHash, Hash finalizedHash); boolean isMiningBeforeMerge(); - Optional getOrSyncHeaderByHash(Hash blockHash, Hash finalizedBlockHash); - void addBadBlock(final Block block, Optional maybeCause); boolean isBadBlock(Hash blockHash); diff --git a/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/TransitionCoordinator.java b/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/TransitionCoordinator.java index 1e5a14888b1..ce9e84e7dd2 100644 --- a/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/TransitionCoordinator.java +++ b/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/TransitionCoordinator.java @@ -183,14 +183,8 @@ public CompletableFuture appendNewPayloadToSync(final Block newPayload) { } @Override - public Optional getOrSyncHeaderByHash(final Hash blockHash) { - return mergeCoordinator.getOrSyncHeaderByHash(blockHash); - } - - @Override - public Optional getOrSyncHeaderByHash( - final Hash blockHash, final Hash finalizedBlockHash) { - return mergeCoordinator.getOrSyncHeaderByHash(blockHash, finalizedBlockHash); + public Optional getOrSyncHeadByHash(final Hash headHash, final Hash finalizedHash) { + return mergeCoordinator.getOrSyncHeadByHash(headHash, finalizedHash); } @Override diff --git a/consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinatorTest.java b/consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinatorTest.java index 5b514b7e207..074002ea2ba 100644 --- a/consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinatorTest.java +++ b/consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinatorTest.java @@ -698,7 +698,7 @@ public void assertGetOrSyncForBlockAlreadyPresent() { BlockHeader mockHeader = headerGenerator.parentHash(Hash.fromHexStringLenient("0xdead")).buildHeader(); when(blockchain.getBlockHeader(mockHeader.getHash())).thenReturn(Optional.of(mockHeader)); - var res = coordinator.getOrSyncHeaderByHash(mockHeader.getHash()); + var res = coordinator.getOrSyncHeadByHash(mockHeader.getHash(), Hash.ZERO); assertThat(res).isPresent(); } @@ -710,7 +710,7 @@ public void assertGetOrSyncForBlockNotPresent() { when(backwardSyncContext.syncBackwardsUntil(mockHeader.getBlockHash())) .thenReturn(CompletableFuture.completedFuture(null)); - var res = coordinator.getOrSyncHeaderByHash(mockHeader.getHash()); + var res = coordinator.getOrSyncHeadByHash(mockHeader.getHash(), Hash.ZERO); assertThat(res).isNotPresent(); } diff --git a/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdated.java b/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdated.java index bc88647ed40..64c427da1e8 100644 --- a/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdated.java +++ b/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdated.java @@ -99,24 +99,27 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext) Optional.of(forkChoice.getHeadBlockHash() + " is an invalid block"))); } - Optional newHead = - mergeCoordinator.getOrSyncHeaderByHash(forkChoice.getHeadBlockHash()); + final Optional maybeNewHead = + mergeCoordinator.getOrSyncHeadByHash( + forkChoice.getHeadBlockHash(), forkChoice.getFinalizedBlockHash()); - if (newHead.isEmpty()) { + if (maybeNewHead.isEmpty()) { return syncingResponse(requestId, forkChoice); } + final BlockHeader newHead = maybeNewHead.get(); + maybePayloadAttributes.ifPresentOrElse( this::logPayload, () -> LOG.debug("Payload attributes are null")); if (!isValidForkchoiceState( - forkChoice.getSafeBlockHash(), forkChoice.getFinalizedBlockHash(), newHead.get())) { + forkChoice.getSafeBlockHash(), forkChoice.getFinalizedBlockHash(), newHead)) { logForkchoiceUpdatedCall(INVALID, forkChoice); return new JsonRpcErrorResponse(requestId, JsonRpcError.INVALID_FORKCHOICE_STATE); } // TODO: post-merge cleanup, this should be unnecessary after merge - if (!mergeCoordinator.latestValidAncestorDescendsFromTerminal(newHead.get())) { + if (!mergeCoordinator.latestValidAncestorDescendsFromTerminal(newHead)) { logForkchoiceUpdatedCall(INVALID, forkChoice); return new JsonRpcSuccessResponse( requestId, @@ -124,12 +127,12 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext) INVALID, Hash.ZERO, null, - Optional.of(newHead.get() + " did not descend from terminal block"))); + Optional.of(newHead + " did not descend from terminal block"))); } ForkchoiceResult result = mergeCoordinator.updateForkChoice( - newHead.get(), + newHead, forkChoice.getFinalizedBlockHash(), forkChoice.getSafeBlockHash(), maybePayloadAttributes.map( @@ -149,7 +152,7 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext) maybePayloadAttributes.map( payloadAttributes -> mergeCoordinator.preparePayload( - newHead.get(), + newHead, payloadAttributes.getTimestamp(), payloadAttributes.getPrevRandao(), payloadAttributes.getSuggestedFeeRecipient())); diff --git a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedTest.java b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedTest.java index 3b53a4f8d68..6ad01828fb4 100644 --- a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedTest.java +++ b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedTest.java @@ -111,7 +111,7 @@ public void shouldReturnSyncingIfMissingNewHead() { public void shouldReturnInvalidOnBadTerminalBlock() { BlockHeader mockHeader = new BlockHeaderTestFixture().baseFeePerGas(Wei.ONE).buildHeader(); - when(mergeCoordinator.getOrSyncHeaderByHash(mockHeader.getHash())) + when(mergeCoordinator.getOrSyncHeadByHash(mockHeader.getHash(), Hash.ZERO)) .thenReturn(Optional.of(mockHeader)); when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(false); assertSuccessWithPayloadForForkchoiceResult( @@ -148,7 +148,7 @@ public void shouldReturnSyncingOnHeadNotFound() { @Test public void shouldReturnValidWithoutFinalizedOrPayload() { BlockHeader mockHeader = new BlockHeaderTestFixture().baseFeePerGas(Wei.ONE).buildHeader(); - when(mergeCoordinator.getOrSyncHeaderByHash(mockHeader.getHash())) + when(mergeCoordinator.getOrSyncHeadByHash(mockHeader.getHash(), Hash.ZERO)) .thenReturn(Optional.of(mockHeader)); when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(true); @@ -172,7 +172,7 @@ public void shouldReturnInvalidOnOldTimestamp() { when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(true); when(mergeCoordinator.isDescendantOf(any(), any())).thenReturn(true); when(mergeContext.isSyncing()).thenReturn(false); - when(mergeCoordinator.getOrSyncHeaderByHash(mockHeader.getHash())) + when(mergeCoordinator.getOrSyncHeadByHash(mockHeader.getHash(), parent.getHash())) .thenReturn(Optional.of(mockHeader)); when(mergeCoordinator.updateForkChoice( mockHeader, parent.getHash(), parent.getHash(), Optional.empty())) @@ -202,7 +202,7 @@ public void shouldReturnValidWithNewHeadAndFinalizedNoPayload() { BlockHeader mockParent = builder.number(9L).buildHeader(); BlockHeader mockHeader = builder.number(10L).parentHash(mockParent.getHash()).buildHeader(); when(blockchain.getBlockHeader(any())).thenReturn(Optional.of(mockHeader)); - when(mergeCoordinator.getOrSyncHeaderByHash(mockHeader.getHash())) + when(mergeCoordinator.getOrSyncHeadByHash(mockHeader.getHash(), Hash.ZERO)) .thenReturn(Optional.of(mockHeader)); when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(true); when(mergeCoordinator.isDescendantOf(any(), any())).thenReturn(true); @@ -217,7 +217,7 @@ public void shouldReturnValidWithNewHeadAndFinalizedNoPayload() { @Test public void shouldReturnValidWithoutFinalizedWithPayload() { BlockHeader mockHeader = new BlockHeaderTestFixture().baseFeePerGas(Wei.ONE).buildHeader(); - when(mergeCoordinator.getOrSyncHeaderByHash(mockHeader.getHash())) + when(mergeCoordinator.getOrSyncHeadByHash(mockHeader.getHash(), Hash.ZERO)) .thenReturn(Optional.of(mockHeader)); when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(true); @@ -254,7 +254,7 @@ public void shouldReturnInvalidForkchoiceStateIfFinalizedBlockIsUnknown() { when(blockchain.getBlockHeader(finalizedBlockHash)).thenReturn(Optional.empty()); when(mergeContext.isSyncing()).thenReturn(false); - when(mergeCoordinator.getOrSyncHeaderByHash(newHead.getHash())) + when(mergeCoordinator.getOrSyncHeadByHash(newHead.getHash(), finalizedBlockHash)) .thenReturn(Optional.of(newHead)); var resp = @@ -275,7 +275,7 @@ public void shouldReturnInvalidForkchoiceStateIfFinalizedBlockIsNotAnAncestorOfN when(blockchain.getBlockHeader(newHead.getHash())).thenReturn(Optional.of(newHead)); when(blockchain.getBlockHeader(finalized.getHash())).thenReturn(Optional.of(finalized)); when(mergeContext.isSyncing()).thenReturn(false); - when(mergeCoordinator.getOrSyncHeaderByHash(newHead.getHash())) + when(mergeCoordinator.getOrSyncHeadByHash(newHead.getHash(), finalized.getBlockHash())) .thenReturn(Optional.of(newHead)); when(mergeCoordinator.isDescendantOf(finalized, newHead)).thenReturn(false); @@ -301,7 +301,7 @@ public void shouldReturnInvalidForkchoiceStateIfSafeHeadZeroWithFinalizedBlock() when(blockchain.getBlockHeader(parent.getHash())).thenReturn(Optional.of(parent)); when(mergeContext.isSyncing()).thenReturn(false); - when(mergeCoordinator.getOrSyncHeaderByHash(newHead.getHash())) + when(mergeCoordinator.getOrSyncHeadByHash(newHead.getHash(), parent.getBlockHash())) .thenReturn(Optional.of(newHead)); var resp = @@ -328,7 +328,7 @@ public void shouldReturnInvalidForkchoiceStateIfSafeBlockIsUnknown() { when(blockchain.getBlockHeader(finalized.getHash())).thenReturn(Optional.of(finalized)); when(blockchain.getBlockHeader(safeBlockBlockHash)).thenReturn(Optional.empty()); when(mergeContext.isSyncing()).thenReturn(false); - when(mergeCoordinator.getOrSyncHeaderByHash(newHead.getHash())) + when(mergeCoordinator.getOrSyncHeadByHash(newHead.getHash(), finalized.getBlockHash())) .thenReturn(Optional.of(newHead)); when(mergeCoordinator.isDescendantOf(finalized, newHead)).thenReturn(true); @@ -352,7 +352,7 @@ public void shouldReturnInvalidForkchoiceStateIfSafeBlockIsNotADescendantOfFinal when(blockchain.getBlockHeader(finalized.getHash())).thenReturn(Optional.of(finalized)); when(blockchain.getBlockHeader(safeBlock.getHash())).thenReturn(Optional.of(safeBlock)); when(mergeContext.isSyncing()).thenReturn(false); - when(mergeCoordinator.getOrSyncHeaderByHash(newHead.getHash())) + when(mergeCoordinator.getOrSyncHeadByHash(newHead.getHash(), finalized.getBlockHash())) .thenReturn(Optional.of(newHead)); when(mergeCoordinator.isDescendantOf(finalized, newHead)).thenReturn(true); when(mergeCoordinator.isDescendantOf(finalized, safeBlock)).thenReturn(false); @@ -377,7 +377,7 @@ public void shouldReturnInvalidForkchoiceStateIfSafeBlockIsNotAnAncestorOfNewHea when(blockchain.getBlockHeader(finalized.getHash())).thenReturn(Optional.of(finalized)); when(blockchain.getBlockHeader(safeBlock.getHash())).thenReturn(Optional.of(safeBlock)); when(mergeContext.isSyncing()).thenReturn(false); - when(mergeCoordinator.getOrSyncHeaderByHash(newHead.getHash())) + when(mergeCoordinator.getOrSyncHeadByHash(newHead.getHash(), finalized.getBlockHash())) .thenReturn(Optional.of(newHead)); when(mergeCoordinator.isDescendantOf(finalized, newHead)).thenReturn(true); when(mergeCoordinator.isDescendantOf(finalized, safeBlock)).thenReturn(true); @@ -397,7 +397,7 @@ public void shouldReturnInvalidForkchoiceStateIfSafeBlockIsNotAnAncestorOfNewHea public void shouldIgnoreUpdateToOldHeadAndNotPreparePayload() { BlockHeader mockHeader = new BlockHeaderTestFixture().baseFeePerGas(Wei.ONE).buildHeader(); - when(mergeCoordinator.getOrSyncHeaderByHash(mockHeader.getHash())) + when(mergeCoordinator.getOrSyncHeadByHash(mockHeader.getHash(), Hash.ZERO)) .thenReturn(Optional.of(mockHeader)); when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(true); diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardChain.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardChain.java index 94dfa89ef66..2844c461b04 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardChain.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardChain.java @@ -93,15 +93,14 @@ public synchronized void prependAncestorsHeader(final BlockHeader blockHeader) { headers.put(blockHeader.getHash(), blockHeader); return; } - BlockHeader firstHeader = firstStoredAncestor.get(); + final BlockHeader firstHeader = firstStoredAncestor.get(); headers.put(blockHeader.getHash(), blockHeader); - chainStorage.put(blockHeader.getHash(), firstStoredAncestor.get().getHash()); + chainStorage.put(blockHeader.getHash(), firstHeader.getHash()); firstStoredAncestor = Optional.of(blockHeader); debugLambda( LOG, - "Added header {} on height {} to backward chain led by pivot {} on height {}", + "Added header {} to backward chain led by pivot {} on height {}", blockHeader::toLogString, - blockHeader::getNumber, () -> lastStoredPivot.orElseThrow().toLogString(), firstHeader::getNumber); } @@ -127,16 +126,23 @@ public synchronized void dropFirstHeader() { } public synchronized void appendTrustedBlock(final Block newPivot) { - debugLambda(LOG, "appending trusted block {}", newPivot::toLogString); + debugLambda(LOG, "Appending trusted block {}", newPivot::toLogString); headers.put(newPivot.getHash(), newPivot.getHeader()); blocks.put(newPivot.getHash(), newPivot); if (lastStoredPivot.isEmpty()) { firstStoredAncestor = Optional.of(newPivot.getHeader()); } else { if (newPivot.getHeader().getParentHash().equals(lastStoredPivot.get().getHash())) { + debugLambda( + LOG, + "Added block {} to backward chain led by pivot {} on height {}", + newPivot::toLogString, + lastStoredPivot.get()::toLogString, + firstStoredAncestor.get()::getNumber); chainStorage.put(lastStoredPivot.get().getHash(), newPivot.getHash()); } else { firstStoredAncestor = Optional.of(newPivot.getHeader()); + debugLambda(LOG, "Re-pivoting to new target block {}", newPivot::toLogString); } } lastStoredPivot = Optional.of(newPivot.getHeader()); diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncContext.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncContext.java index bd9bd21b2bc..05724c5d67f 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncContext.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncContext.java @@ -37,7 +37,6 @@ import java.util.Optional; import java.util.concurrent.CompletableFuture; import java.util.concurrent.atomic.AtomicReference; -import java.util.stream.Stream; import com.google.common.annotations.VisibleForTesting; import org.slf4j.Logger; @@ -58,13 +57,9 @@ public class BackwardSyncContext { private final AtomicReference currentBackwardSyncStatus = new AtomicReference<>(); private final BackwardChain backwardChain; private int batchSize = BATCH_SIZE; - private Optional maybeFinalized = Optional.empty(); private Optional maybeHead = Optional.empty(); - private final int maxRetries; - private final long millisBetweenRetries = DEFAULT_MILLIS_BETWEEN_RETRIES; - private final Subscribers badChainListeners = Subscribers.create(); public BackwardSyncContext( @@ -108,16 +103,17 @@ public synchronized boolean isSyncing() { .orElse(Boolean.FALSE); } - public synchronized void updateHeads(final Hash head, final Hash finalizedBlockHash) { - if (Hash.ZERO.equals(finalizedBlockHash)) { - this.maybeFinalized = Optional.empty(); + public synchronized void updateHead(final Hash headHash) { + if (Hash.ZERO.equals(headHash)) { + maybeHead = Optional.empty(); } else { - this.maybeFinalized = Optional.ofNullable(finalizedBlockHash); - } - if (Hash.ZERO.equals(head)) { - this.maybeHead = Optional.empty(); - } else { - this.maybeHead = Optional.ofNullable(head); + maybeHead = Optional.of(headHash); + Optional maybeCurrentStatus = Optional.ofNullable(currentBackwardSyncStatus.get()); + maybeCurrentStatus.ifPresent( + status -> + backwardChain + .getBlock(headHash) + .ifPresent(block -> status.updateTargetHeight(block.getHeader().getNumber()))); } } @@ -125,13 +121,7 @@ public synchronized CompletableFuture syncBackwardsUntil(final Hash newBlo Optional maybeCurrentStatus = Optional.ofNullable(this.currentBackwardSyncStatus.get()); if (isTrusted(newBlockHash)) { return maybeCurrentStatus - .map( - status -> { - backwardChain - .getBlock(newBlockHash) - .ifPresent(block -> status.updateTargetHeight(block.getHeader().getNumber())); - return status.currentFuture; - }) + .map(Status::getCurrentFuture) .orElseGet(() -> CompletableFuture.completedFuture(null)); } backwardChain.addNewHash(newBlockHash); @@ -300,7 +290,7 @@ public void subscribeBadChainListener(final BadChainListener badChainListener) { } // In rare case when we request too many headers/blocks we get response that does not contain all - // data and we might want to retry with smaller batch size + // data, and we might want to retry with smaller batch size public int getBatchSize() { return batchSize; } @@ -349,19 +339,21 @@ protected void possiblyMoveHead(final Block lastSavedBlock) { LOG.debug("Nothing to do with the head"); return; } - if (blockchain.getChainHead().getHash().equals(maybeHead.get())) { + + final Hash head = maybeHead.get(); + if (blockchain.getChainHead().getHash().equals(head)) { LOG.debug("Head is already properly set"); return; } - if (blockchain.contains(maybeHead.get())) { - LOG.debug("Changing head to {}", maybeHead.get().toHexString()); - blockchain.rewindToBlock(maybeHead.get()); + + if (blockchain.contains(head)) { + LOG.debug("Changing head to {}", head); + blockchain.rewindToBlock(head); return; } - if (blockchain.getChainHead().getHash().equals(lastSavedBlock.getHash())) { - LOG.debug("Rewinding head to lastSavedBlock {}", lastSavedBlock.getHash()); - blockchain.rewindToBlock(lastSavedBlock.getHash()); - } + + debugLambda(LOG, "Rewinding head to last saved block {}", lastSavedBlock::toLogString); + blockchain.rewindToBlock(lastSavedBlock.getHash()); } public SyncState getSyncState() { @@ -372,13 +364,6 @@ public synchronized BackwardChain getBackwardChain() { return backwardChain; } - public Optional findMaybeFinalized() { - return Stream.of(maybeFinalized, getProtocolContext().getBlockchain().getFinalized()) - .filter(Optional::isPresent) - .map(Optional::get) - .findFirst(); - } - public Status getStatus() { return currentBackwardSyncStatus.get(); } diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncStep.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncStep.java index 73ac964b530..dd05a02dd0f 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncStep.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncStep.java @@ -59,6 +59,12 @@ protected Hash possibleRestoreOldNodes(final BlockHeader firstAncestor) { @VisibleForTesting protected CompletableFuture> requestHeaders(final Hash hash) { + if (context.getProtocolContext().getBlockchain().contains(hash)) { + LOG.debug( + "Hash {} already present in local blockchain no need to request headers to peers", hash); + return CompletableFuture.completedFuture(List.of()); + } + final int batchSize = context.getBatchSize(); LOG.debug("Requesting headers for hash {}, with batch size {}", hash, batchSize); diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardsSyncAlgorithm.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardsSyncAlgorithm.java index f362c9ec3ad..a87af03bbb3 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardsSyncAlgorithm.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardsSyncAlgorithm.java @@ -73,21 +73,21 @@ public CompletableFuture pickNextStep() { if (!context.isReady()) { return waitForReady(); } - runFinalizedSuccessionRule( - context.getProtocolContext().getBlockchain(), context.findMaybeFinalized()); - final Optional possibleFirstAncestorHeader = + final Optional maybeFirstAncestorHeader = context.getBackwardChain().getFirstAncestorHeader(); - if (possibleFirstAncestorHeader.isEmpty()) { + if (maybeFirstAncestorHeader.isEmpty()) { this.finished = true; - LOG.info("The Backward sync is done"); + LOG.info("Current backward sync session is done"); context.getBackwardChain().clear(); return CompletableFuture.completedFuture(null); } + final MutableBlockchain blockchain = context.getProtocolContext().getBlockchain(); - final BlockHeader firstAncestorHeader = possibleFirstAncestorHeader.get(); + final BlockHeader firstAncestorHeader = maybeFirstAncestorHeader.get(); if (blockchain.contains(firstAncestorHeader.getHash())) { return executeProcessKnownAncestors(); } + if (blockchain.getChainHead().getHeight() > firstAncestorHeader.getNumber()) { debugLambda( LOG, @@ -99,7 +99,7 @@ public CompletableFuture pickNextStep() { if (finalBlockConfirmation.ancestorHeaderReached(firstAncestorHeader)) { debugLambda( LOG, - "Backward sync reached ancestor header with {}, starting Forward sync", + "Backward sync reached ancestor header with {}, starting forward sync", firstAncestorHeader::toLogString); return executeForwardAsync(); } @@ -166,52 +166,6 @@ private void countDownIfReady(final CountDownLatch latch) { } } - @VisibleForTesting - protected void runFinalizedSuccessionRule( - final MutableBlockchain blockchain, final Optional maybeFinalized) { - if (maybeFinalized.isEmpty()) { - LOG.debug("Nothing to validate yet, consensus layer did not provide a new finalized block"); - return; - } - final Hash newFinalized = maybeFinalized.get(); - if (!blockchain.contains(newFinalized)) { - LOG.debug("New finalized block {} is not imported yet", newFinalized); - return; - } - - final Optional maybeOldFinalized = blockchain.getFinalized(); - if (maybeOldFinalized.isPresent()) { - final Hash oldFinalized = maybeOldFinalized.get(); - if (newFinalized.equals(oldFinalized)) { - LOG.debug("We already have this block as finalized"); - return; - } - BlockHeader newFinalizedHeader = - blockchain - .getBlockHeader(newFinalized) - .orElseThrow( - () -> - new BackwardSyncException( - "The header " + newFinalized.toHexString() + "not found")); - BlockHeader oldFinalizedHeader = - blockchain - .getBlockHeader(oldFinalized) - .orElseThrow( - () -> - new BackwardSyncException( - "The header " + oldFinalized.toHexString() + "not found")); - LOG.info( - "Updating finalized {} block to new finalized block {}", - oldFinalizedHeader.toLogString(), - newFinalizedHeader.toLogString()); - } else { - // Todo: should TTD test be here? - LOG.info("Setting new finalized block to {}", newFinalized); - } - - blockchain.setFinalized(newFinalized); - } - private CompletableFuture waitForPeers(final int count) { final WaitForPeersTask waitForPeersTask = WaitForPeersTask.create(context.getEthContext(), count, context.getMetricsSystem()); diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncAlgSpec.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncAlgSpec.java index 91a57bb8636..69ab5470f3a 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncAlgSpec.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncAlgSpec.java @@ -36,7 +36,6 @@ import java.nio.charset.StandardCharsets; import java.util.List; -import java.util.Optional; import java.util.concurrent.CompletableFuture; import java.util.concurrent.TimeUnit; import javax.annotation.Nonnull; @@ -262,93 +261,6 @@ public void shouldRunBackwardStepWhenNotOnLocalHeight() { verify(algorithm).executeBackwardAsync(any()); } - @Test - public void successionShouldIgnoreEmptyFinalized() { - final BackwardsSyncAlgorithm backwardsSyncAlgorithm = - new BackwardsSyncAlgorithm(context, firstHeader -> false); - - Optional finalized = localBlockchain.getFinalized(); - assertThat(finalized).isEmpty(); - - backwardsSyncAlgorithm.runFinalizedSuccessionRule(localBlockchain, Optional.empty()); - - finalized = localBlockchain.getFinalized(); - assertThat(finalized).isEmpty(); - } - - @Test - public void successionShouldSetFinalizedFromEmpty() { - final BackwardsSyncAlgorithm backwardsSyncAlgorithm = - new BackwardsSyncAlgorithm(context, firstHeader -> false); - - Optional finalized = localBlockchain.getFinalized(); - assertThat(finalized).isEmpty(); - - backwardsSyncAlgorithm.runFinalizedSuccessionRule( - localBlockchain, Optional.of(localBlockchain.getChainHead().getHash())); - - finalized = localBlockchain.getFinalized(); - assertThat(finalized).isPresent(); - assertThat(finalized).contains(localBlockchain.getChainHead().getHash()); - } - - @Test - public void successionShouldIgnoreFinalisedWhenNotImportedYet() { - final BackwardsSyncAlgorithm backwardsSyncAlgorithm = - new BackwardsSyncAlgorithm(context, firstHeader -> false); - - Optional finalized = localBlockchain.getFinalized(); - assertThat(finalized).isEmpty(); - - backwardsSyncAlgorithm.runFinalizedSuccessionRule( - localBlockchain, Optional.of(remoteBlockchain.getChainHead().getHash())); - - finalized = localBlockchain.getFinalized(); - assertThat(finalized).isEmpty(); - } - - @Test - public void successionShouldKeepFinalizedWhenNotChanged() { - final BackwardsSyncAlgorithm backwardsSyncAlgorithm = - new BackwardsSyncAlgorithm(context, firstHeader -> false); - - Optional finalized = localBlockchain.getFinalized(); - assertThat(finalized).isEmpty(); - - backwardsSyncAlgorithm.runFinalizedSuccessionRule( - localBlockchain, Optional.of(localBlockchain.getChainHead().getHash())); - backwardsSyncAlgorithm.runFinalizedSuccessionRule( - localBlockchain, Optional.of(localBlockchain.getChainHead().getHash())); - - finalized = localBlockchain.getFinalized(); - assertThat(finalized).isPresent(); - assertThat(finalized).contains(localBlockchain.getChainHead().getHash()); - } - - @Test - public void successionShouldUpdateOldFinalizedToNewFinalized() { - final BackwardsSyncAlgorithm backwardsSyncAlgorithm = - new BackwardsSyncAlgorithm(context, firstHeader -> false); - - Optional finalized = localBlockchain.getFinalized(); - assertThat(finalized).isEmpty(); - - final Hash fin1 = localBlockchain.getBlockByNumber(LOCAL_HEIGHT - 5).orElseThrow().getHash(); - backwardsSyncAlgorithm.runFinalizedSuccessionRule(localBlockchain, Optional.of(fin1)); - - finalized = localBlockchain.getFinalized(); - assertThat(finalized).isPresent(); - assertThat(finalized).contains(fin1); - - final Hash fin2 = localBlockchain.getBlockByNumber(LOCAL_HEIGHT - 3).orElseThrow().getHash(); - - backwardsSyncAlgorithm.runFinalizedSuccessionRule(localBlockchain, Optional.of(fin2)); - - finalized = localBlockchain.getFinalized(); - assertThat(finalized).isPresent(); - assertThat(finalized).contains(fin2); - } - @Test public void shouldStartForwardSyncIfGenesisIsReached() { doReturn(true).when(context).isReady(); diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncContextTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncContextTest.java index cf2c4eb92f8..61b5c235bdb 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncContextTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncContextTest.java @@ -249,16 +249,7 @@ private Block getBlockByNumber(final int number) { @Test public void testUpdatingHead() { - context.updateHeads(null, null); - context.possiblyMoveHead(null); - assertThat(localBlockchain.getChainHeadBlock().getHeader().getNumber()).isEqualTo(LOCAL_HEIGHT); - - context.updateHeads(Hash.ZERO, null); - context.possiblyMoveHead(null); - - assertThat(localBlockchain.getChainHeadBlock().getHeader().getNumber()).isEqualTo(LOCAL_HEIGHT); - - context.updateHeads(localBlockchain.getBlockByNumber(4).orElseThrow().getHash(), null); + context.updateHead(localBlockchain.getBlockByNumber(4).orElseThrow().getHash()); context.possiblyMoveHead(null); assertThat(localBlockchain.getChainHeadBlock().getHeader().getNumber()).isEqualTo(4); diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncStepTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncStepTest.java index 7f9e5b7c5b5..17eb2e45a92 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncStepTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncStepTest.java @@ -163,6 +163,17 @@ public void shouldRequestHeaderWhenAsked() throws Exception { assertThat(blockHeader).isEqualTo(lookingForBlock.getHeader()); } + @Test + public void shouldNotRequestHeaderIfAlreadyPresent() throws Exception { + BackwardSyncStep step = new BackwardSyncStep(context, createBackwardChain(REMOTE_HEIGHT - 1)); + final Block lookingForBlock = getBlockByNumber(LOCAL_HEIGHT); + + final CompletableFuture> future = + step.requestHeaders(lookingForBlock.getHeader().getHash()); + + assertThat(future.get().isEmpty()).isTrue(); + } + @Test public void shouldRequestHeaderBeforeCurrentHeight() throws Exception { extendBlockchain(REMOTE_HEIGHT + 1, context.getProtocolContext().getBlockchain());