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.

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

Make checkpoint sync work when finalized state is transitioned with empty slot(s) #7981

Closed
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,5 @@ the [releases page](https://github.com/Consensys/teku/releases).
- Added hidden option `--Xp2p-dumps-to-file-enabled` to enable saving p2p dumps to file.

### Bug Fixes

- Fixed a checkpoint sync issue where Teku couldn't start when the finalized state has been transitioned with empty slot(s)
4 changes: 4 additions & 0 deletions ethereum/spec/src/main/java/tech/pegasys/teku/spec/Spec.java
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,10 @@ public UInt64 computeStartSlotAtEpoch(final UInt64 epoch) {
return atEpoch(epoch).miscHelpers().computeStartSlotAtEpoch(epoch);
}

public UInt64 computeEndSlotAtEpoch(final UInt64 epoch) {
return atEpoch(epoch).miscHelpers().computeEndSlotAtEpoch(epoch);
}

public UInt64 computeEpochAtSlot(final UInt64 slot) {
return atSlot(slot).miscHelpers().computeEpochAtSlot(slot);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,15 @@ public class StateAndBlockSummary implements BeaconBlockSummary {
protected StateAndBlockSummary(final BeaconBlockSummary blockSummary, final BeaconState state) {
checkNotNull(blockSummary);
checkNotNull(state);
this.blockSummary = blockSummary;
this.state = state;
verifyStateAndBlockConsistency();
}

protected void verifyStateAndBlockConsistency() {
checkArgument(
blockSummary.getStateRoot().equals(state.hashTreeRoot()),
"Block state root must match the supplied state");
this.blockSummary = blockSummary;
this.state = state;
}

public static StateAndBlockSummary create(final BeaconState state) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,15 +45,10 @@ private AnchorPoint(
final BeaconState state,
final BeaconBlockSummary blockSummary) {
super(blockSummary, state);
checkArgument(
checkpoint.getRoot().equals(blockSummary.getRoot()), "Checkpoint and block must match");
checkArgument(
checkpoint.getEpochStartSlot(spec).isGreaterThanOrEqualTo(blockSummary.getSlot()),
"Block must be at or prior to the start of the checkpoint epoch");

this.spec = spec;
this.checkpoint = checkpoint;
this.isGenesis = checkpoint.getEpoch().equals(SpecConfig.GENESIS_EPOCH);
verifyAnchor();
}

public static AnchorPoint create(
Expand Down Expand Up @@ -126,6 +121,42 @@ public static AnchorPoint fromInitialBlockAndState(
return new AnchorPoint(spec, checkpoint, state, block);
}

/**
* Skipping verification in the super class. All checks are made in {@link #verifyAnchor()}
* instead
*/
@Override
protected void verifyStateAndBlockConsistency() {}

private void verifyAnchor() {
final UInt64 blockSlot = blockSummary.getSlot();
if (state.getSlot().isGreaterThan(blockSlot)) {
// the finalized state is transitioned with empty slot(s)
checkArgument(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also add verification for state.block_roots (block slot - state slot interval) and state.state_roots (block slot)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added those checks, made it bit more complex, since don't have access to spec in super class.

blockSummary.getStateRoot().equals(state.getLatestBlockHeader().getStateRoot()),
"Block state root must match the latest block header state root in the state");
final int stateAndBlockRootsIndex =
blockSlot.mod(spec.getSlotsPerHistoricalRoot(blockSlot)).intValue();
checkArgument(
blockSummary
.getStateRoot()
.equals(state.getStateRoots().get(stateAndBlockRootsIndex).get()),
"Block state root must match the state root for the block slot %s in the state roots",
blockSlot);
checkArgument(
blockSummary.getRoot().equals(state.getBlockRoots().get(stateAndBlockRootsIndex).get()),
"Block root must match the root for the block slot %s in the block roots in the state",
blockSlot);
} else {
super.verifyStateAndBlockConsistency();
}
checkArgument(
checkpoint.getRoot().equals(blockSummary.getRoot()), "Checkpoint and block must match");
checkArgument(
checkpoint.getEpochStartSlot(spec).isGreaterThanOrEqualTo(blockSlot),
"Block must be at or prior to the start of the checkpoint epoch");
}

public boolean isGenesis() {
return isGenesis;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

package tech.pegasys.teku.spec.datastructures.state;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy;

import java.util.Optional;
Expand All @@ -21,11 +22,19 @@
import tech.pegasys.teku.spec.Spec;
import tech.pegasys.teku.spec.TestSpecFactory;
import tech.pegasys.teku.spec.datastructures.blocks.BeaconBlockAndState;
import tech.pegasys.teku.spec.datastructures.blocks.SignedBlockAndState;
import tech.pegasys.teku.spec.datastructures.state.beaconstate.BeaconState;
import tech.pegasys.teku.spec.logic.common.statetransition.exceptions.EpochProcessingException;
import tech.pegasys.teku.spec.logic.common.statetransition.exceptions.SlotProcessingException;
import tech.pegasys.teku.spec.util.DataStructureUtil;
import tech.pegasys.teku.storage.storageSystem.InMemoryStorageSystemBuilder;
import tech.pegasys.teku.storage.storageSystem.StorageSystem;

public class AnchorPointTest {
private final Spec spec = TestSpecFactory.createDefault();
private final DataStructureUtil dataStructureUtil = new DataStructureUtil(spec);
private final StorageSystem storageSystem =
InMemoryStorageSystemBuilder.create().specProvider(spec).build();

@Test
public void create_withCheckpointPriorToState() {
Expand All @@ -41,4 +50,30 @@ public void create_withCheckpointPriorToState() {
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("Block must be at or prior to the start of the checkpoint epoch");
}

@Test
public void createFromInitialState_WhenFinalizedStateTransitionedWithAnEmptySlot()
throws SlotProcessingException, EpochProcessingException {
storageSystem.chainUpdater().initializeGenesis();

final UInt64 latestBlockHeaderEpoch = UInt64.valueOf(4);
final UInt64 latestBlockHeaderSlot = spec.computeEndSlotAtEpoch(latestBlockHeaderEpoch);
final SignedBlockAndState blockAndState =
storageSystem.chainUpdater().advanceChainUntil(latestBlockHeaderSlot);

// empty slot transition
final BeaconState postState =
spec.processSlots(blockAndState.getState(), latestBlockHeaderSlot.increment());

final AnchorPoint anchor = AnchorPoint.fromInitialState(spec, postState);

// verify finalized anchor
assertThat(anchor.getBlockSlot()).isEqualTo(latestBlockHeaderSlot);
assertThat(anchor.getStateRoot()).isEqualTo(blockAndState.getBlock().getStateRoot());
final Checkpoint expectedCheckpoint =
new Checkpoint(
latestBlockHeaderEpoch.plus(1),
blockAndState.getBlock().asHeader().getMessage().hashTreeRoot());
assertThat(anchor.getCheckpoint()).isEqualTo(expectedCheckpoint);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -325,21 +325,12 @@ public void loadedInitialStateResource(
}
}

public void errorIncompatibleInitialState(final UInt64 epoch) {
log.error(
"Cannot start with provided initial state for the epoch {}, "
+ "checkpoint occurred on the empty slot, which is not yet supported.\n"
+ "If you are using remote checkpoint source, "
+ "please wait for the next epoch to finalize and retry.",
epoch);
}

public void warnInitialStateIgnored() {
log.warn("Not loading specified initial state as chain data already exists.");
}

public void warnFailedToLoadInitialState(final String message) {
log.warn(message);
public void warnFailedToLoadInitialState(final Throwable throwable) {
log.warn("Failed to load initial state", throwable);
}

public void warnOnInitialStateWithSkippedSlots(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1409,14 +1409,14 @@ private Optional<AnchorPoint> tryLoadingAnchorPointFromInitialState(
initialAnchor =
attemptToLoadAnchorPoint(
networkConfiguration.getNetworkBoostrapConfig().getInitialState());
} catch (final InvalidConfigurationException e) {
} catch (final InvalidConfigurationException ex) {
final StateBoostrapConfig stateBoostrapConfig =
networkConfiguration.getNetworkBoostrapConfig();
if (stateBoostrapConfig.isUsingCustomInitialState()
&& !stateBoostrapConfig.isUsingCheckpointSync()) {
throw e;
throw ex;
}
STATUS_LOG.warnFailedToLoadInitialState(e.getMessage());
STATUS_LOG.warnFailedToLoadInitialState(ex);
}

return initialAnchor;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@

package tech.pegasys.teku.services.beaconchain;

import static tech.pegasys.teku.infrastructure.exceptions.ExitConstants.ERROR_EXIT_CODE;
import static tech.pegasys.teku.infrastructure.logging.StatusLogger.STATUS_LOG;
import static tech.pegasys.teku.networks.Eth2NetworkConfiguration.FINALIZED_STATE_URL_PATH;

Expand Down Expand Up @@ -86,10 +85,6 @@ private AnchorPoint getAnchorPoint(Spec spec, String stateResource, String sanit
throws IOException {
STATUS_LOG.loadingInitialStateResource(sanitizedResource);
final BeaconState state = ChainDataLoader.loadState(spec, stateResource);
if (state.getSlot().isGreaterThan(state.getLatestBlockHeader().getSlot())) {
STATUS_LOG.errorIncompatibleInitialState(spec.computeEpochAtSlot(state.getSlot()));
System.exit(ERROR_EXIT_CODE);
}
final AnchorPoint anchor = AnchorPoint.fromInitialState(spec, state);
STATUS_LOG.loadedInitialStateResource(
state.hashTreeRoot(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -858,13 +858,10 @@ private SafeFuture<Optional<StateAndBlockSummary>> getOrRegenerateBlockAndState(
return SafeFuture.completedFuture(maybeEpochState);
}

// if finalized is gone from cache we can still reconstruct that without regenerating
// if finalized is gone from cache we can use the finalized anchor without regenerating
if (finalizedAnchor.getRoot().equals(blockRoot)) {
LOG.trace("epochCache GET finalizedAnchor {}", finalizedAnchor::getSlot);
return SafeFuture.completedFuture(
Optional.of(
StateAndBlockSummary.create(
finalizedAnchor.getBlockSummary(), finalizedAnchor.getState())));
return SafeFuture.completedFuture(Optional.of(finalizedAnchor));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's transitioned anchor, it will be incorrect state for requested block_root

Copy link
Contributor Author

@StefanBratanov StefanBratanov Feb 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AnchorPoint.getStateRoot() still returns the state_root of the finalzied block (last block header), not the state

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method private SafeFuture<Optional<StateAndBlockSummary>> getOrRegenerateBlockAndState( final Bytes32 blockRoot) should return state at block with blockRoot, transitioned state is not this state. This method is used by a lot of different consumers, it could be an issue

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah AnchorPoint.getStateRoot() returns the state_root of the blockSummary, so would be the state associated with the block

Copy link
Contributor Author

@StefanBratanov StefanBratanov Feb 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In any case, StateAndBlockSummary.create(finalizedAnchor.getBlockSummary(), finalizedAnchor.getState()) is same as just returning finalizedAnchor

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On startup we definitely create the Store with the anchor coming from the checkpoint URL. So it is possible that we could return it from here...

}

maybeEpochStates.ifPresent(
Expand Down