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

6612: Remove deprecated sync modes and related helper methods #7309

Merged
merged 4 commits into from
Jul 12, 2024
Merged

6612: Remove deprecated sync modes and related helper methods #7309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
88e1e20
6612: Remove deprecated sync modes and related helper methods
Matilda-Clerke Jul 12, 2024
1d26b90
run spotless
Matilda-Clerke Jul 12, 2024
60f8a9d
run spotless
Matilda-Clerke Jul 12, 2024
acce6c6
Merge branch 'main' into 6612-remove-deprecated-sync-modes-x_snap-and…
Matilda-Clerke Jul 12, 2024
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
10 changes: 5 additions & 5 deletions besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1566,11 +1566,11 @@ private void validateConsensusSyncCompatibilityOptions() {
|| genesisConfigOptionsSupplier.get().isQbft())
&& !unstableSynchronizerOptions.isSnapSyncBftEnabled()) {
final String errorSuffix = "can't be used with BFT networks";
if (SyncMode.CHECKPOINT.equals(syncMode) || SyncMode.X_CHECKPOINT.equals(syncMode)) {
if (SyncMode.CHECKPOINT.equals(syncMode)) {
throw new ParameterException(
commandLine, String.format("%s %s", "Checkpoint sync", errorSuffix));
}
if (syncMode == SyncMode.SNAP || syncMode == SyncMode.X_SNAP) {
if (syncMode == SyncMode.SNAP) {
throw new ParameterException(commandLine, String.format("%s %s", "Snap sync", errorSuffix));
}
}
Expand Down Expand Up @@ -1753,7 +1753,7 @@ && isOptionSet(commandLine, "--sync-min-peers")) {
CommandLineUtils.failIfOptionDoesntMeetRequirement(
commandLine,
"--Xcheckpoint-post-merge-enabled can only be used with CHECKPOINT sync-mode",
SyncMode.isCheckpointSync(getDefaultSyncModeIfNotSet()),
getDefaultSyncModeIfNotSet() == SyncMode.CHECKPOINT,
singletonList("--Xcheckpoint-post-merge-enabled"));

CommandLineUtils.failIfOptionDoesntMeetRequirement(
Expand Down Expand Up @@ -2043,10 +2043,10 @@ private PrivacyParameters privacyParameters() {
if (syncMode == SyncMode.FAST) {
throw new ParameterException(commandLine, String.format("%s %s", "Fast sync", errorSuffix));
}
if (syncMode == SyncMode.SNAP || syncMode == SyncMode.X_SNAP) {
if (syncMode == SyncMode.SNAP) {
throw new ParameterException(commandLine, String.format("%s %s", "Snap sync", errorSuffix));
}
if (syncMode == SyncMode.CHECKPOINT || syncMode == SyncMode.X_CHECKPOINT) {
if (syncMode == SyncMode.CHECKPOINT) {
throw new ParameterException(
commandLine, String.format("%s %s", "Checkpoint sync", errorSuffix));
}
Expand Down
4 changes: 1 addition & 3 deletions besu/src/main/java/org/hyperledger/besu/controller/BesuController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@
*/
package org.hyperledger.besu.controller;

import static org.hyperledger.besu.ethereum.eth.sync.SyncMode.isCheckpointSync;

import org.hyperledger.besu.cli.config.EthNetworkConfig;
import org.hyperledger.besu.config.GenesisConfigFile;
import org.hyperledger.besu.config.GenesisConfigOptions;
Expand Down Expand Up @@ -361,7 +359,7 @@ public BesuControllerBuilder fromGenesisFile(
// wrap with TransitionBesuControllerBuilder if we have a terminal total difficulty:
if (configOptions.getTerminalTotalDifficulty().isPresent()) {
// Enable start with vanilla MergeBesuControllerBuilder for PoS checkpoint block
if (isCheckpointSync(syncMode) && isCheckpointPoSBlock(configOptions)) {
if (syncMode == SyncMode.CHECKPOINT && isCheckpointPoSBlock(configOptions)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (syncMode == SyncMode.CHECKPOINT && isCheckpointPoSBlock(configOptions)) {
if (SyncMode.CHECKPOINT == syncMode && isCheckpointPoSBlock(configOptions)) {

nit: this way around avoids a null pointer if syncMode were ever to be null (which shouldn't happen to be honest)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought if syncMode were null, it should just evaluate null == SyncMode.CHECKPOINT, which should be ok and return false as expected, right?

Copy link
Contributor

@siladu siladu Jul 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, I was getting confused with nullableField.equals(value) idiom throwing a null pointer.
I should have suggested SyncMode.CHECKPOINT.equals(syncMode).

You're right, both will return false so it's not a big deal, mostly stylistic.

return new MergeBesuControllerBuilder().genesisConfigFile(genesisConfigFile);
} else {
// TODO this should be changed to vanilla MergeBesuControllerBuilder and the Transition*
Expand Down
6 changes: 3 additions & 3 deletions besu/src/main/java/org/hyperledger/besu/controller/BesuControllerBuilder.java
View file
Open in desktop
siladu marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -708,8 +708,8 @@ public BesuController build() {

ethPeers.setTrailingPeerRequirementsSupplier(synchronizer::calculateTrailingPeerRequirements);

if (SyncMode.isSnapSync(syncConfig.getSyncMode())
|| SyncMode.isCheckpointSync(syncConfig.getSyncMode())) {
if (syncConfig.getSyncMode() == SyncMode.SNAP
|| syncConfig.getSyncMode() == SyncMode.CHECKPOINT) {
synchronizer.subscribeInSync((b) -> ethPeers.snapServerPeersNeeded(!b));
ethPeers.snapServerPeersNeeded(true);
} else {
Expand Down Expand Up @@ -1157,7 +1157,7 @@ protected List<PeerValidator> createPeerValidators(final ProtocolSchedule protoc

final CheckpointConfigOptions checkpointConfigOptions =
genesisConfigOptions.getCheckpointOptions();
if (SyncMode.isCheckpointSync(syncConfig.getSyncMode()) && checkpointConfigOptions.isValid()) {
if (syncConfig.getSyncMode() == SyncMode.CHECKPOINT && checkpointConfigOptions.isValid()) {
validators.add(
new CheckpointBlocksPeerValidator(
protocolSchedule,
Expand Down
2 changes: 1 addition & 1 deletion besu/src/test/java/org/hyperledger/besu/cli/BesuCommandTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1107,7 +1107,7 @@ public void syncMode_invalid() {
assertThat(commandOutput.toString(UTF_8)).isEmpty();
assertThat(commandErrorOutput.toString(UTF_8))
.contains(
"Invalid value for option '--sync-mode': expected one of [FULL, FAST, SNAP, CHECKPOINT, X_SNAP, X_CHECKPOINT] (case-insensitive) but was 'bogus'");
"Invalid value for option '--sync-mode': expected one of [FULL, FAST, SNAP, CHECKPOINT] (case-insensitive) but was 'bogus'");
}

@Test
Expand Down
2 changes: 1 addition & 1 deletion ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthPeers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,7 +525,7 @@ private void ethPeerStatusExchanged(final EthPeer peer) {

peer.chainState().updateHeightEstimate(peerHeadBlockHeader.getNumber());
CompletableFuture<Void> isServingSnapFuture;
if (SyncMode.isCheckpointSync(syncMode) || SyncMode.isSnapSync(syncMode)) {
if (syncMode == SyncMode.SNAP || syncMode == SyncMode.CHECKPOINT) {
// even if we have finished the snap sync, we still want to know if the peer is a snap
// server
isServingSnapFuture =
Expand Down
6 changes: 3 additions & 3 deletions ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/DefaultSynchronizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,8 @@ public DefaultSynchronizer(
this::calculateTrailingPeerRequirements,
metricsSystem);

if (SyncMode.isSnapSync(syncConfig.getSyncMode())
|| SyncMode.isCheckpointSync(syncConfig.getSyncMode())) {
if (syncConfig.getSyncMode() == SyncMode.SNAP
|| syncConfig.getSyncMode() == SyncMode.CHECKPOINT) {
SnapServerChecker.createAndSetSnapServerChecker(ethContext, metricsSystem);
}

Expand Down Expand Up @@ -145,7 +145,7 @@ public DefaultSynchronizer(
worldStateStorageCoordinator,
syncState,
clock);
} else if (SyncMode.isCheckpointSync(syncConfig.getSyncMode())) {
} else if (syncConfig.getSyncMode() == SyncMode.CHECKPOINT) {
this.fastSyncFactory =
() ->
CheckpointDownloaderFactory.createCheckpointDownloader(
Expand Down
26 changes: 2 additions & 24 deletions ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/SyncMode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,35 +27,13 @@ public enum SyncMode {
// Perform snapsync
SNAP,
// Perform snapsync but starting from a checkpoint instead of starting from genesis
CHECKPOINT,
// Deprecated and will be removed in 24.4.0 (X_SNAP and X_CHECKPOINT)
X_SNAP,
X_CHECKPOINT;
CHECKPOINT;

public String normalize() {
if (this.toString().startsWith("X_")) {
// removes X_ at the beginning
return StringUtils.capitalize(this.toString().substring(2).toLowerCase(Locale.ROOT));
}

return StringUtils.capitalize(this.toString().toLowerCase(Locale.ROOT));
}

public static boolean isFullSync(final SyncMode syncMode) {
return !EnumSet.of(
SyncMode.FAST,
SyncMode.SNAP,
SyncMode.X_SNAP,
SyncMode.CHECKPOINT,
SyncMode.X_CHECKPOINT)
.contains(syncMode);
}

public static boolean isCheckpointSync(final SyncMode syncMode) {
return X_CHECKPOINT.equals(syncMode) || CHECKPOINT.equals(syncMode);
}

public static boolean isSnapSync(final SyncMode syncMode) {
return X_SNAP.equals(syncMode) || SNAP.equals(syncMode);
return !EnumSet.of(SyncMode.FAST, SyncMode.SNAP, SyncMode.CHECKPOINT).contains(syncMode);
}
}
Loading