-
Notifications
You must be signed in to change notification settings - Fork 461
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
Deprecate FastBlock Flag #6792
Conversation
There was a problem hiding this 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.
fe2c722
to
52c0804
Compare
ignore this, I wanted to see all tests that would fail in the case if fastsync was forced. |
52c0804
to
aa77629
Compare
…and mark obsolete.
aa77629
to
da7812e
Compare
src/Nethermind/Nethermind.Synchronization.Test/FastBlocks/FastBlocksFeedTests.cs
Outdated
Show resolved
Hide resolved
src/Nethermind/Nethermind.Synchronization/ParallelSync/SyncProgressResolver.cs
Show resolved
Hide resolved
I see - so the test needed to change |
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 }; |
There was a problem hiding this comment.
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
There was a problem hiding this 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! :)
I don't know how could you walk me through the overview, left a dm. |
Don't know if it is connected, but with 1.26 on MainNet I'm getting
And if I check the mainnet.cfg of the 1.26 I have
|
Fast block was deprecated, but it being present in the config shouldn't raise an error. |
Fixes: #6295
Changes
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
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
Requires explanation in Release Notes
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