Deprecate FastBlocks flag

[marcindsobczak]

Is your feature request related to a problem? Please describe.
There are two flags in Sync config - FastSync and FastBlocks. Second one is causing confusion sometimes - nodes are not able to sync if FastSync is true, but FastBlocks not. I'm not familiar with any use case when one of this flags is true and the other is false.

Describe the solution you'd like
Keep FastBlocks as part of the config to not break user setups, but don't use it on code - use FastSync instead

Describe alternatives you've considered
Keep it as it is

Additional context
I recently spent some time to figure out the cause of not syncing with message
Unable to find beacon header at height 1. This is unexpected, forcing a new beacon sync
Lack of FastBlocks = true was the reason

[obasekiosa]

Keeping FastBlocks in config while not using it in code might be confusing for cases (however unlikely) where both are set to different values.

Also, maybe adding a warning message that FastBlock isn't being used and would be deprecated from the config.

or/and

to override FastBlocks value to that of FastSync

@MarekM25
@marcindsobczak

[marcindsobczak]

Our nodes are not able to sync using FastSync if FastBlocks are disabled. So we can deprecate FastBlocks and use FastSync in all places where FastBlocks are used right now. There is no point in having it set to different values.
To not break users setups, we should not throw if there will be flag FastBlocks, just ignore it

[LukaszRozmej]

So FastSync is used for syncing State (with SnapSync being a flavor of this sync), while FastBlocks is used for backward syncing of Blocks and Receipts. They are kind of disjointed. Both need a pivot (from CL or from config).

Not sure if it is worth or unifying them, maybe just flip them to default to true.
Or sophisticated - make them nullable by default, and if null - make them true if either pivot is set or merge is enabled.

[MarekM25]

@LukaszRozmej the problem is that our node will not work with FastSync = true, SnapSync = true and FastBlocks = false
There were users and (our) devs who configured node in this way and we spent time on debugging.
The configuration that you mention is redundant because we have also DownloadBodiesInFastSync and DownloadReceiptsInFastSync

I don't see FastBlocks configuration use cases. It's just the way to misconfiguration of the nodes without additional value

[LukaszRozmej]

Ok, lets just nuke it then (not what PR did)

[obasekiosa]

So ignore fast block in the config, and always set it to true if fast sync is true? (the case of fastsync being false is irrelevant)

Meaning keep the interfaces but switch off fastblock settings internally in code?

1. Switching off would mean, having it replaced with fastsync settings Internally within the class abstraction?

or

2. ignoring its value at point of use and simply performing sync based on fastsync only...

1 should imply 2, unless I'm missing something?

[MarekM25]

@obasekiosa in my view, just replace all FastBlocks usages to FastSync, but don't remove the FastBlocks config option, because some nodes might use this configuration. We will mark it as a deprecated and in one release we could remove many deprecated config options

[obasekiosa]

@MarekM25 @marcindsobczak @LukaszRozmej I was able make the changes
but i noticed one particular test that had fastbocks set to false even when fastsync was set to true.
Since we are removing fastBlocks entirely and making decisions based on fastsync. How would that affect this test?

1. MemoryHintManTest: db sizes are computed correctly test.

2. FastBlocksFeedTests: Throws_when_launched_and_disabled_in_config

3. SyncProgressResolverTests: Is_fast_block_finished_returns_true_when_no_fast_block_sync_is_used

4. ReceiptsSyncFeedTests: Should_throw_when_fast_blocks_not_enabled