Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove check in fcU if new blocks are descendants of the previous finalized block #3975

Merged
merged 7 commits into from
Jun 24, 2022
Merged

Remove check in fcU if new blocks are descendants of the previous finalized block #3975

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
54ffc50
removed check from fcU if new blocks are descendants of the previous …
daniellehrner Jun 16, 2022
4619a13
Merge branch 'main' into fcu_remove_finalized_descendant_check
daniellehrner Jun 16, 2022
3465867
Merge branch 'main' into fcu_remove_finalized_descendant_check
daniellehrner Jun 17, 2022
d064afd
backwards sync: remove check that block must be a descendant of final…
daniellehrner Jun 21, 2022
cf455a7
Merge branch 'main' into fcu_remove_finalized_descendant_check
daniellehrner Jun 21, 2022
463e73b
Merge branch 'main' into fcu_remove_finalized_descendant_check
daniellehrner Jun 24, 2022
6d332b2
Merge branch 'main' into fcu_remove_finalized_descendant_check
daniellehrner Jun 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 0 additions & 28 deletions ...ge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,38 +274,10 @@ public ForkchoiceResult updateForkChoice(
final Hash safeBlockHash,
final Optional<PayloadAttributes> maybePayloadAttributes) {
MutableBlockchain blockchain = protocolContext.getBlockchain();
Optional<BlockHeader> currentFinalized = mergeContext.getFinalized();
final Optional<BlockHeader> newFinalized = blockchain.getBlockHeader(finalizedBlockHash);

final Optional<Hash> latestValid = getLatestValidAncestor(newHead);

if (currentFinalized.isPresent()
&& newFinalized.isPresent()
&& !isDescendantOf(currentFinalized.get(), newFinalized.get())) {
return ForkchoiceResult.withFailure(
INVALID,
String.format(
"new finalized block %s is not a descendant of current finalized block %s",
finalizedBlockHash, currentFinalized.get().getBlockHash()),
latestValid);
}

// ensure new head is descendant of finalized
Optional<String> descendantError =
newFinalized
.map(Optional::of)
.orElse(currentFinalized)
.filter(finalized -> !isDescendantOf(finalized, newHead))
.map(
finalized ->
String.format(
"new head block %s is not a descendant of current finalized block %s",
newHead.getBlockHash(), finalized.getBlockHash()));

if (descendantError.isPresent()) {
return ForkchoiceResult.withFailure(INVALID, descendantError.get(), latestValid);
}

Optional<BlockHeader> parentOfNewHead = blockchain.getBlockHeader(newHead.getParentHash());
if (parentOfNewHead.isPresent()
&& parentOfNewHead.get().getTimestamp() >= newHead.getTimestamp()) {
Expand Down
131 changes: 0 additions & 131 deletions ...rc/test/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,119 +262,6 @@ public void updateForkChoiceShouldPersistLastFinalizedBlockHash() {
verify(mergeContext).setSafeBlock(lastFinalizedHeader);
}

@Test
public void updateForkChoiceShouldFailIfLastFinalizedNotDescendantOfPreviousFinalized() {
BlockHeader terminalHeader = terminalPowBlock();
coordinator.executeBlock(new Block(terminalHeader, BlockBody.empty()));

BlockHeader prevFinalizedHeader = nextBlockHeader(terminalHeader);
Block prevFinalizedBlock = new Block(prevFinalizedHeader, BlockBody.empty());
coordinator.executeBlock(prevFinalizedBlock);

when(mergeContext.getFinalized()).thenReturn(Optional.of(prevFinalizedHeader));

// not descendant of previous finalized block
BlockHeader lastFinalizedHeader = disjointBlockHeader(prevFinalizedHeader);
Block lastFinalizedBlock = new Block(lastFinalizedHeader, BlockBody.empty());
coordinator.executeBlock(lastFinalizedBlock);

BlockHeader headBlockHeader = nextBlockHeader(lastFinalizedHeader);
Block headBlock = new Block(headBlockHeader, BlockBody.empty());
coordinator.executeBlock(headBlock);
when(blockchain.getBlockHeader(lastFinalizedBlock.getHash()))
.thenReturn(Optional.of(lastFinalizedHeader));
when(blockchain.getBlockHeader(headBlockHeader.getHash()))
.thenReturn(Optional.of(headBlockHeader));
var res =
coordinator.updateForkChoice(
headBlockHeader,
lastFinalizedBlock.getHash(),
lastFinalizedBlock.getHash(),
Optional.empty());
assertThat(res.isValid()).isFalse();
assertThat(res.getStatus()).isEqualTo(ForkchoiceResult.Status.INVALID);

verify(blockchain, never()).setFinalized(lastFinalizedBlock.getHash());
verify(mergeContext, never()).setFinalized(lastFinalizedHeader);
verify(blockchain, never()).setSafeBlock(lastFinalizedBlock.getHash());
verify(mergeContext, never()).setSafeBlock(lastFinalizedHeader);
}

@Test
public void updateForkChoiceShouldFailIfHeadNotDescendantOfLastFinalized() {
BlockHeader terminalHeader = terminalPowBlock();
coordinator.executeBlock(new Block(terminalHeader, BlockBody.empty()));

BlockHeader prevFinalizedHeader = nextBlockHeader(terminalHeader);
Block prevFinalizedBlock = new Block(prevFinalizedHeader, BlockBody.empty());
coordinator.executeBlock(prevFinalizedBlock);

when(mergeContext.getFinalized()).thenReturn(Optional.of(prevFinalizedHeader));

BlockHeader lastFinalizedHeader = nextBlockHeader(prevFinalizedHeader);
Block lastFinalizedBlock = new Block(lastFinalizedHeader, BlockBody.empty());
coordinator.executeBlock(lastFinalizedBlock);

// not descendant of last finalized block
BlockHeader headBlockHeader = disjointBlockHeader(lastFinalizedHeader);
Block headBlock = new Block(headBlockHeader, BlockBody.empty());
coordinator.executeBlock(headBlock);
when(blockchain.getBlockHeader(lastFinalizedBlock.getHash()))
.thenReturn(Optional.of(lastFinalizedHeader));
when(blockchain.getBlockHeader(headBlockHeader.getHash()))
.thenReturn(Optional.of(headBlockHeader));
var res =
coordinator.updateForkChoice(
headBlockHeader,
lastFinalizedBlock.getHash(),
lastFinalizedBlock.getHash(),
Optional.empty());
assertThat(res.isValid()).isFalse();
assertThat(res.getStatus()).isEqualTo(ForkchoiceResult.Status.INVALID);

verify(blockchain, never()).setFinalized(lastFinalizedBlock.getHash());
verify(mergeContext, never()).setFinalized(lastFinalizedHeader);
verify(blockchain, never()).setSafeBlock(lastFinalizedBlock.getHash());
verify(mergeContext, never()).setSafeBlock(lastFinalizedHeader);
}

@Test
public void updateForkChoiceShouldFailIfFinalizedBlockNotFound() {
BlockHeader terminalHeader = terminalPowBlock();
coordinator.executeBlock(new Block(terminalHeader, BlockBody.empty()));

BlockHeader prevFinalizedHeader = nextBlockHeader(terminalHeader);
Block prevFinalizedBlock = new Block(prevFinalizedHeader, BlockBody.empty());
coordinator.executeBlock(prevFinalizedBlock);

when(mergeContext.getFinalized()).thenReturn(Optional.of(prevFinalizedHeader));

BlockHeader lastFinalizedHeader = nextBlockHeader(prevFinalizedHeader);
Block lastFinalizedBlock = new Block(lastFinalizedHeader, BlockBody.empty());
// note this block is not executed, so not known by us

BlockHeader headBlockHeader = nextBlockHeader(lastFinalizedHeader);
Block headBlock = new Block(headBlockHeader, BlockBody.empty());
coordinator.executeBlock(headBlock);
when(blockchain.getBlockHeader(lastFinalizedBlock.getHash())).thenReturn(Optional.empty());
when(blockchain.getBlockHeader(headBlockHeader.getHash()))
.thenReturn(Optional.of(headBlockHeader))
.thenReturn(Optional.of(headBlockHeader));
var res =
coordinator.updateForkChoice(
headBlockHeader,
lastFinalizedBlock.getHash(),
lastFinalizedBlock.getHash(),
Optional.empty());
assertThat(res.isValid()).isFalse();
assertThat(res.getStatus()).isEqualTo(ForkchoiceResult.Status.INVALID);

verify(blockchain, never()).setFinalized(lastFinalizedBlock.getHash());
verify(mergeContext, never()).setFinalized(lastFinalizedHeader);
verify(blockchain, never()).setSafeBlock(lastFinalizedBlock.getHash());
verify(mergeContext, never()).setSafeBlock(lastFinalizedHeader);
}

@Test
public void assertGetOrSyncForBlockAlreadyPresent() {
BlockHeader mockHeader =
Expand Down Expand Up @@ -609,24 +496,6 @@ private BlockHeader nextBlockHeader(
.buildHeader();
}

private BlockHeader disjointBlockHeader(final BlockHeader disjointFromHeader) {
Hash disjointParentHash = Hash.wrap(disjointFromHeader.getParentHash().shiftRight(1));

return headerGenerator
.difficulty(Difficulty.ZERO)
.parentHash(disjointParentHash)
.gasLimit(genesisState.getBlock().getHeader().getGasLimit())
.number(disjointFromHeader.getNumber() + 1)
.stateRoot(genesisState.getBlock().getHeader().getStateRoot())
.baseFeePerGas(
feeMarket.computeBaseFee(
genesisState.getBlock().getHeader().getNumber() + 1,
disjointFromHeader.getBaseFee().orElse(Wei.of(0x3b9aca00)),
0,
15000000l))
.buildHeader();
}

MergeCoordinator terminalAncestorMock(final long chainDepth, final boolean hasTerminalPoW) {
final Difficulty mockTTD = Difficulty.of(1000);
BlockHeaderTestFixture builder = new BlockHeaderTestFixture().baseFeePerGas(Wei.ONE);
Expand Down
2 changes: 1 addition & 1 deletion ...rc/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ private CompletableFuture<Void> prepareBackwardSyncFuture() {
this,
FinalBlockConfirmation.confirmationChain(
FinalBlockConfirmation.genesisConfirmation(blockchain),
FinalBlockConfirmation.finalizedConfirmation(blockchain)))
FinalBlockConfirmation.ancestorConfirmation(blockchain)))
.executeBackwardsSync(null);
}

Expand Down
7 changes: 2 additions & 5 deletions ...main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardsSyncAlgorithm.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,8 @@ public CompletableFuture<Void> pickNextStep() {
() -> firstAncestorHeader.get().toLogString());
}

if (finalBlockConfirmation.finalHeaderReached(firstAncestorHeader.get())) {
LOG.info("Backward sync reached final header, starting Forward sync");
if (finalBlockConfirmation.ancestorHeaderReached(firstAncestorHeader.get())) {
LOG.info("Backward sync reached ancestor header, starting Forward sync");
return executeForwardAsync();
}

Expand Down Expand Up @@ -179,9 +179,6 @@ protected void runFinalizedSuccessionRule(
() ->
new BackwardSyncException(
"The header " + oldFinalized.toHexString() + "not found"));
if (newFinalizedHeader.getNumber() < oldFinalizedHeader.getNumber()) {
throw new BackwardSyncException("Cannot finalize below already finalized...");
}
LOG.info(
"Updating finalized {} block to new finalized block {}",
oldFinalizedHeader.toLogString(),
Expand Down
26 changes: 6 additions & 20 deletions ...main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/FinalBlockConfirmation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,14 @@
package org.hyperledger.besu.ethereum.eth.sync.backwardsync;

import org.hyperledger.besu.ethereum.chain.MutableBlockchain;
import org.hyperledger.besu.ethereum.core.Block;
import org.hyperledger.besu.ethereum.core.BlockHeader;

import java.util.Arrays;
import java.util.Optional;

public interface FinalBlockConfirmation {
boolean finalHeaderReached(final BlockHeader firstHeader);
boolean ancestorHeaderReached(final BlockHeader firstHeader);

public static FinalBlockConfirmation genesisConfirmation(final MutableBlockchain blockchain) {
static FinalBlockConfirmation genesisConfirmation(final MutableBlockchain blockchain) {
final BlockHeader genesisBlockHeader = blockchain.getBlockHeader(0).orElseThrow();
return firstHeader -> {
if (firstHeader.getNumber() > 0) {
Expand All @@ -45,29 +43,17 @@ public static FinalBlockConfirmation genesisConfirmation(final MutableBlockchain
};
}

public static FinalBlockConfirmation finalizedConfirmation(final MutableBlockchain blockchain) {
return firstHeader -> {
final Optional<Block> finalized =
blockchain.getFinalized().flatMap(blockchain::getBlockByHash);
if (finalized.isPresent()
&& (finalized.get().getHeader().getNumber() >= firstHeader.getNumber())) {
throw new BackwardSyncException("Cannot continue below finalized...");
}
if (blockchain.contains(firstHeader.getParentHash())) {
return true;
}
return false;
};
static FinalBlockConfirmation ancestorConfirmation(final MutableBlockchain blockchain) {
return firstHeader -> blockchain.contains(firstHeader.getParentHash());
}

public static FinalBlockConfirmation confirmationChain(
final FinalBlockConfirmation... confirmations) {
static FinalBlockConfirmation confirmationChain(final FinalBlockConfirmation... confirmations) {
return firstHeader -> {
return Arrays.stream(confirmations)
.reduce(
false,
(aBoolean, confirmation2) ->
aBoolean || confirmation2.finalHeaderReached(firstHeader),
aBoolean || confirmation2.ancestorHeaderReached(firstHeader),
(aBoolean, aBoolean2) -> aBoolean || aBoolean2);
};
}
Expand Down
30 changes: 1 addition & 29 deletions ...rc/test/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncAlgSpec.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ public void setUp() throws Exception {
context,
FinalBlockConfirmation.confirmationChain(
FinalBlockConfirmation.genesisConfirmation(localBlockchain),
FinalBlockConfirmation.finalizedConfirmation(localBlockchain))));
FinalBlockConfirmation.ancestorConfirmation(localBlockchain))));
when(context.getProtocolContext().getBlockchain()).thenReturn(localBlockchain);
}

Expand Down Expand Up @@ -323,34 +323,6 @@ public void successionShouldKeepFinalizedWhenNotChanged() {
assertThat(finalized).contains(localBlockchain.getChainHead().getHash());
}

@Test
public void successionShouldThrowWhenFinalizingBellowPreviousFinalized() {
final BackwardsSyncAlgorithm backwardsSyncAlgorithm =
new BackwardsSyncAlgorithm(context, firstHeader -> false);

Optional<Hash> 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());

assertThatThrownBy(
() ->
backwardsSyncAlgorithm.runFinalizedSuccessionRule(
localBlockchain,
Optional.of(
localBlockchain
.getBlockHeader(LOCAL_HEIGHT - 1)
.orElseThrow()
.getBlockHash())))
.isInstanceOf(BackwardSyncException.class)
.hasMessageContaining("Cannot finalize below already finalized");
}

@Test
public void successionShouldUpdateOldFinalizedToNewFinalized() {
final BackwardsSyncAlgorithm backwardsSyncAlgorithm =
Expand Down