nethermind icon indicating copy to clipboard operation
nethermind copied to clipboard

Deprecate FastBlock Flag

Open obasekiosa opened this issue 11 months ago • 2 comments

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?

  • [x] 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
  • [x] Refactoring
  • [x] Documentation update
  • [ ] Build-related changes
  • [ ] Other: Description

Testing

Requires testing

  • [x] Yes
  • [ ] No

If yes, did you write tests?

  • [x] 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

  • [x] Yes
  • [ ] No

Requires explanation in Release Notes

  • [x] 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

obasekiosa avatar Feb 28 '24 15:02 obasekiosa

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 avatar Mar 01 '24 10:03 obasekiosa

I see - so the test needed to change

obasekiosa avatar Mar 01 '24 18:03 obasekiosa

@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.

obasekiosa avatar Mar 08 '24 11:03 obasekiosa

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,

xanatos avatar May 08 '24 11:05 xanatos

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.

obasekiosa avatar May 08 '24 11:05 obasekiosa