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

Deprecate FastBlock Flag #6792

Merged
merged 6 commits into from
Mar 12, 2024
Merged

Deprecate FastBlock Flag #6792

merged 6 commits into from
Mar 12, 2024

Conversation

obasekiosa
Copy link
Contributor

@obasekiosa obasekiosa commented Feb 28, 2024

Fixes: #6295

Changes

  • Replace FastBlocks with FastSync
  • Marks FastBlocks as deprecated on SyncConfig Interface
  • Removes FastBlocks from config files
  • Updates ParallelSync tests to accommodate for FastHeaders being synced before testing.
  • Removes commented out tests

Types of changes

What types of changes does your code introduce?

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

Notes on testing

Some tests where updated to remove FastBlocks, and some parallel sync tests where updated to assume FastHeaders have already been synced before testing.

Documentation

Enabling fast sync now automatically enables fast blocks. Since fast blocks depends on fast sync it is no longer needed as a configuration option.

Requires documentation update

  • Yes
  • No

Requires explanation in Release Notes

  • Yes
  • No

Documentation and release notes should indicate FastBlocks has been deprecated. Although it would be ignored if added, currently adding it to config would not break setups.

Remarks

During a set of tests by @marcindsobczak it was noticed that some Syncing tests faild with the error message Unable to find beacon header at height 1. This is unexpected, forcing a new beacon sync.
The reason for these errors were FastBlocks being false when FastSync was set to true. Given that that case is hardly ever if ever at all used. It was decided its better to remove FastBlocks completed, deprecating it and replacing its use in logic to the value of FastSync.

This fix is the first phase of fully deprecating FastBlocks

Copy link
Contributor

@MarekM25 MarekM25 left a comment

Choose a reason for hiding this comment

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

This wasn't the objective of this issue. Let's settle the internal discussion with @LukaszRozmej, and then we can schedule a call to provide you with a better understanding.

@obasekiosa obasekiosa force-pushed the fix/6295_deprecate-fastblocks branch from fe2c722 to 52c0804 Compare March 1, 2024 10:54
@obasekiosa
Copy link
Contributor Author

ignore this, I wanted to see all tests that would fail in the case if fastsync was forced.
Can't do that locally due to system dependent flaky tests.

@obasekiosa obasekiosa force-pushed the fix/6295_deprecate-fastblocks branch from 52c0804 to aa77629 Compare March 1, 2024 11:28
@obasekiosa obasekiosa force-pushed the fix/6295_deprecate-fastblocks branch from aa77629 to da7812e Compare March 1, 2024 12:27
@LukaszRozmej LukaszRozmej marked this pull request as ready for review March 1, 2024 17:38
@LukaszRozmej LukaszRozmej requested a review from rubo as a code owner March 1, 2024 17:38
@LukaszRozmej LukaszRozmej marked this pull request as draft March 1, 2024 17:58
@obasekiosa
Copy link
Contributor Author

obasekiosa commented Mar 1, 2024

I see - so the test needed to change

@LukaszRozmej LukaszRozmej marked this pull request as ready for review March 1, 2024 18:51
public static ISyncConfig WithFastSync { get; } = new SyncConfig { FastSync = true };
public static ISyncConfig WithFastBlocks { get; } = new SyncConfig { FastSync = true, FastBlocks = true };
public static ISyncConfig WithEth2Merge { get; } = new SyncConfig { FastSync = false, FastBlocks = false, BlockGossipEnabled = false };
public static ISyncConfig WithFastBlocks { get; } = new SyncConfig { FastSync = true };
Copy link
Contributor

Choose a reason for hiding this comment

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

Just delete this whole line

Copy link
Contributor

@MarekM25 MarekM25 left a comment

Choose a reason for hiding this comment

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

@obasekiosa Have you synced one node with one-click to confirm that code works correctly? If you don't know how - let me know! :)

@obasekiosa
Copy link
Contributor Author

@obasekiosa Have you synced one node with one-click to confirm that code works correctly? If you don't know how - let me know! :)

I don't know how could you walk me through the overview, left a dm.

@LukaszRozmej LukaszRozmej merged commit f5ad049 into master Mar 12, 2024
70 checks passed
@LukaszRozmej LukaszRozmej deleted the fix/6295_deprecate-fastblocks branch March 12, 2024 14:07
@xanatos
Copy link

xanatos commented May 8, 2024

Don't know if it is connected, but with 1.26 on MainNet I'm getting

Incorrect config settings found:
ConfigType:JsonConfigFile|Category:SyncConfig|Name:FastBlocks

And if I check the mainnet.cfg of the 1.26 I have

"FastBlocks": true,

@obasekiosa
Copy link
Contributor Author

Don't know if it is connected, but with 1.26 on MainNet I'm getting

Incorrect config settings found:
ConfigType:JsonConfigFile|Category:SyncConfig|Name:FastBlocks

And if I check the mainnet.cfg of the 1.26 I have

"FastBlocks": true,

Fast block was deprecated, but it being present in the config shouldn't raise an error.
A warning yes but not an error.
it's left there to allow for smooth transition, but it's not being used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate FastBlocks flag
6 participants