-
Notifications
You must be signed in to change notification settings - Fork 401
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
[Config] Change default barriers of mainnet to 0 #6225
[Config] Change default barriers of mainnet to 0 #6225
Conversation
|| WithinOldBarrierDefault; | ||
|
||
// This property was introduced when we switched defaults of barriers from 11052984 to 0 to not disturb existing node operators | ||
private bool WithinOldBarrierDefault => _blockTree.LowestInsertedBodyNumber <= DepositContractBarrier |
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.
are you sure it will work correctly in every case?
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.
We are covering 99 percent of cases. the only case that is not covered is if someone is syncing, and suddenly stopped the client at exactly around the old default, then unfortunately the sync will stop.
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.
Do other network have barrier too?
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.
Yeah, they do. The assumption is we only want to do that for mainnet
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.
By default we only had barriers on mainnet
@@ -231,6 +231,20 @@ public async Task When_configured_to_skip_receipts_then_finishes_immediately() | |||
_measuredProgressQueue.HasEnded.Should().BeTrue(); | |||
} | |||
|
|||
[Test] | |||
public async Task When_finished_sync_with_old_default_barrier_then_finishes_imedietely() |
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.
tests for ReceiptsSyncFeed, but no tests for bodies
no tests for every possible case
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.
There are no tests for bodies in the first place. which was surprising tbh
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.
Added more cases for receipts
@@ -16,8 +16,6 @@ | |||
"PivotHash": "0x24290b7c113efe59095ff1d3afc15efec735655e6d4065d24704759fa5c9a308", | |||
"PivotTotalDifficulty": "58750003716598352816469", | |||
"FastBlocks": true, | |||
"AncientBodiesBarrier": 11052984, | |||
"AncientReceiptsBarrier": 11052984, |
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.
Do we want to remove these values completely? If user decide to enable them he can set value close to the head and CL will not be able to sync deposit contract. Maybe we can introduce something like Sync.EnableBarrier
?
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.
I dont think the user will set the value from his mind. If the user wants to enable them, we can have something in the docs to guide?
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.
Fair, but I think it's better option to have default value for this. User may decide to set it to current head
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.
If that is desired, we can just make another config file, like mainet-barriers
, for example
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.
I'd better go with mainnet-light
or something but not sure.
You'll need to change |
455cbd1
to
a86d436
Compare
Following tests were perfomed
The current implementation covers the following case:
|
189f2b0
to
44ab04c
Compare
7bf3b7d
to
e25b7d2
Compare
@smartprogrammer93 Let's make sure it is well tested since may break some setups. @shashankshampi Important topic - please prepare a lot of scenarios including nodes synced on all version (before and after autopivot). Please also put a summary of testing here under PR before it would be merged - let's don't rush this one - it would be included in next version so we have a lot of time to make sure we are well prepared. |
The test is done and looks good to go. |
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.
Tested this branch with couple of scenarios and looks stable to process ahead
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.
It looks much better now. Added two comments to consider, but PR looks good
else if (_specProvider.ChainId == BlockchainIds.Mainnet) | ||
{ | ||
// Assume the receipts barrier was the previous defualt (deposit contract barrier) only for mainnet | ||
_barrierWhenStarted = DepositContractBarrier; |
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.
Edge case (not sure if worth handling): if the mainnet user with config barriers different than default deposits will upgrade Nethermind during receipts download it will stop receipts sync.
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.
I would ignore case when someone upgrades during sync
@@ -72,8 +92,27 @@ public override void InitializeFeed() | |||
_barrier = _syncConfig.AncientBodiesBarrierCalc; | |||
if (_logger.IsInfo) _logger.Info($"Changed pivot in bodies sync. Now using pivot {_pivotNumber} and barrier {_barrier}"); | |||
ResetSyncStatusList(); | |||
if (_blockTree.LowestInsertedBodyNumber is null || _blockTree.LowestInsertedBodyNumber >= _syncConfig.PivotNumberParsed) |
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.
Think about refactoring BodiesSyncFeed and ReceiptSyncFeed to reuse the same (similar) code
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.
Looks good to me. Make sure that it was retested after all changes.
Retested and got green light from kamil. Merging |
Changes
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Documentation
Requires documentation update
Requires explanation in Release Notes
default to barriers on mainnet configuration changed to 0 to support the network. If you wish to use the old default barrier you need to set it manually.