Deprecate Xsnapsync-bft-enabled option , to be removed in future release

[pullurib]

PR description

Adding deprecation notice and marking for removal in future release.

Fixed Issue(s)

#7924

[macfarla]

needs a changelog entry - this will be a breaking change

[macfarla]

you'll also want to remove
hidden = true,
so the option shows in the help

[siladu]

IMO better to avoid the breaking change and make this an alias and call out the deprecation of the X version, e.g. see

besu/besu/src/main/java/org/hyperledger/besu/cli/options/storage/DiffBasedSubStorageOptions.java

Lines 58 to 69 in 27592b5

	// TODO --Xbonsai-limit-trie-logs-enabled and --Xbonsai-trie-log-pruning-enabled are deprecated,
	// remove in a future release
	@SuppressWarnings("ExperimentalCliOptionMustBeCorrectlyDisplayed")
	@Option(
	names = {
	LIMIT_TRIE_LOGS_ENABLED,
	"--Xbonsai-limit-trie-logs-enabled", // deprecated
	"--Xbonsai-trie-log-pruning-enabled" // deprecated
	},
	fallbackValue = "true",
	description = "Limit the number of trie logs that are retained. (default: ${DEFAULT-VALUE})")
	private Boolean limitTrieLogsEnabled = DEFAULT_LIMIT_TRIE_LOGS_ENABLED;

Then can add entry to changelog under "Upcoming Breaking Changes"

[pullurib]

Thanks for the suggestions. So currently I will

• add comments in the code to indicate deprecation in future release
• Add changelog entry for upcoming breaking changes
• Update documentation to indicate this option deprecation soon and that it will be enabled by default then

[matthew1001]

Yes I think I we should remove the option, not promote it to non-experimental.

So I can see the case for making this PR a changelog entry to mark it deprecated, followed by a PR after the next release or two have gone out, which basically reverses the PR I originally added the new flag under.

[macfarla]

ok so if I don't specify this option, does my QBFT network work with SNAP/CHECKPOINT sync ? Have you tried it? Can you add a test for this too

take a look at this code in BesuCommand -

  private void validateConsensusSyncCompatibilityOptions() {
    // snap and checkpoint are experimental for BFT
    if ((genesisConfigOptionsSupplier.get().isIbftLegacy()
            || genesisConfigOptionsSupplier.get().isIbft2()
            || genesisConfigOptionsSupplier.get().isQbft())
        && !unstableSynchronizerOptions.isSnapSyncBftEnabled()) {
      final String errorSuffix = "can't be used with BFT networks";
      if (SyncMode.CHECKPOINT.equals(syncMode)) {
        throw new ParameterException(
            commandLine, String.format("%s %s", "Checkpoint sync", errorSuffix));
      }
      if (syncMode == SyncMode.SNAP) {
        throw new ParameterException(commandLine, String.format("%s %s", "Snap sync", errorSuffix));
      }
    }
  }

[matthew1001]

ok so if I don't specify this option, does my QBFT network work with SNAP/CHECKPOINT sync ? Have you tried it?

Yeah I've tested all combinations of QBFT/IBFT/SNAP/CHECKPOINT and can confirm that they all work. @pullurib could you look at which tests already exist for QBFT sync modes and see if we can add one or two for SNAP and CHECKPOINT please?

[matthew1001]

Suggest that since we're not removing the validation logic in this PR and we add additional tests under the PR where we do remove it if that's OK with you @macfarla

[pullurib]

Sure, I'll take a look and add any missing tests in a following PR which would remove the option and cleanup the validation checks .

[macfarla]

Suggest that since we're not removing the validation logic in this PR and we add additional tests under the PR where we do remove it if that's OK with you @macfarla

Disagree - deprecation is a signal to users that they should stop using this flag. However if I'm a user, I can't stop using this flag until SNAP & BFT work together without it. It will be confusing for users to deprecate the flag first without removing the part of the code that prevents startup with SNAP & BFT unless you specify this flag.

I would suggest

1. remove the restriction on SNAP+BFT - fine to do this in a separate PR - effectively this makes the --Xsnap-bft-enabled flag useless but users don't need to do anything
2. signal deprecation of the flag - users can remove use of the flag during the deprecation period
3. remove the flag - breaking change

[macfarla]

this is what I get right now if I try SNAP + BFT without using this flag

2024-12-03 09:06:37.550+10:00 | main | INFO  | Besu | Starting Besu
2024-12-03 09:06:38.049+10:00 | main | ERROR | Besu | Failed to start Besu Snap sync can't be used with BFT networks
Snap sync can't be used with BFT networks

To display full help:
besu [COMMAND] --help

[matthew1001]

Apologies, I thought this was just a deprecation announce via changelog but I'm fine with changing the logic at the same time and making the flag redundant (but valid). Sound OK to you @pullurib ?

[pullurib]

Sounds good, @matthew1001 . Thanks for clarifying @macfarla . I'll go ahead and include those changes too in this PR .

[macfarla]

looking good. an extra level of niceness is if I specify this option, to log a warning to say it's now deprecated and ignored

requested changes have been made.

[macfarla]

AT failure -
https://github.com/hyperledger/besu/actions/runs/13622026362/job/38135471845?pr=7930

> Task :acceptance-tests:tests:acceptanceTestNotPrivacy
BftSyncAcceptanceTest > shouldSyncValidatorNode(String, BftAcceptanceTestParameterization, SyncMode) > 1: ibft2 with FULL sync FAILED
    java.lang.AssertionError: 
    Expecting actual:
      -1
    to be greater than or equal to:
      1
        at org.hyperledger.besu.tests.acceptance.bft.BftSyncAcceptanceTest.shouldSyncValidatorNode(BftSyncAcceptanceTest.java:79)
BftSyncAcceptanceTest > shouldSyncValidatorNode(String, BftAcceptanceTestParameterization, SyncMode) > 3: ibft2 with SNAP sync FAILED
    java.lang.AssertionError: 
    Expecting actual:
      -1
    to be greater than or equal to:
      1
        at org.hyperledger.besu.tests.acceptance.bft.BftSyncAcceptanceTest.shouldSyncValidatorNode(BftSyncAcceptanceTest.java:79)

[pullurib]

AT failure - https://github.com/hyperledger/besu/actions/runs/13622026362/job/38135471845?pr=7930

> Task :acceptance-tests:tests:acceptanceTestNotPrivacy
BftSyncAcceptanceTest > shouldSyncValidatorNode(String, BftAcceptanceTestParameterization, SyncMode) > 1: ibft2 with FULL sync FAILED
    java.lang.AssertionError: 
    Expecting actual:
      -1
    to be greater than or equal to:
      1
        at org.hyperledger.besu.tests.acceptance.bft.BftSyncAcceptanceTest.shouldSyncValidatorNode(BftSyncAcceptanceTest.java:79)
BftSyncAcceptanceTest > shouldSyncValidatorNode(String, BftAcceptanceTestParameterization, SyncMode) > 3: ibft2 with SNAP sync FAILED
    java.lang.AssertionError: 
    Expecting actual:
      -1
    to be greater than or equal to:
      1
        at org.hyperledger.besu.tests.acceptance.bft.BftSyncAcceptanceTest.shouldSyncValidatorNode(BftSyncAcceptanceTest.java:79)

Looks like a timing issue - has been passing locally. Added some delay to let the new node come up and sync.

[macfarla]

I don't love fixing the failing test with a sleep tbh, is there another option

[pullurib]

I don't love fixing the failing test with a sleep tbh, is there another option

Updated with conditions and timeouts