Skip to content

Conversation

@bogdan-rosianu
Copy link
Contributor

Reasoning behind the pull request

  • nodes shuffler used static values for min nodes per shard / min nodes in meta / hysteresis / adaptivity

Proposed changes

  • integrate chainParametersHandler into it

Testing procedure

Pre-requisites

Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:

  • was the PR targeted to the correct branch?
  • if this is a larger feature that probably needs more than one PR, is there a feat branch created?
  • if this is a feat branch merging, do all satellite projects have a proper tag inside go.mod?

@bogdan-rosianu bogdan-rosianu self-assigned this Dec 13, 2022
Base automatically changed from EN-13432-consensus-group-size-changes-part-2 to feat/consensus-size-changes December 14, 2022 08:54
@iulianpascalau iulianpascalau self-requested a review December 14, 2022 08:55
@bogdan-rosianu bogdan-rosianu marked this pull request as ready for review December 14, 2022 09:54
@codecov-commenter
Copy link

codecov-commenter commented Dec 14, 2022

Codecov Report

❗ No coverage uploaded for pull request base (feat/consensus-size-changes@38d96a4). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files
@@                      Coverage Diff                       @@
##             feat/consensus-size-changes    #4799   +/-   ##
==============================================================
  Coverage                               ?   70.82%           
==============================================================
  Files                                  ?      645           
  Lines                                  ?    85016           
  Branches                               ?        0           
==============================================================
  Hits                                   ?    60214           
  Misses                                 ?    20288           
  Partials                               ?     4514           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@bogdan-rosianu bogdan-rosianu changed the title [WIP] EN-13432: Nodes shuffler refactoring EN-13432: Nodes shuffler refactoring Dec 14, 2022
shardHysteresis uint32
metaHysteresis uint32
currentChainParameters config.ChainParametersByEpochConfig
//shardHysteresis uint32
Copy link
Contributor

Choose a reason for hiding this comment

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

remove commented out code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

ShuffleBetweenShards bool
MaxNodesEnableConfig []config.MaxNodesChangeConfig
EnableEpochsHandler common.EnableEpochsHandler
ChainParametersHandler ChainParametersHandler
Copy link
Contributor

Choose a reason for hiding this comment

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

check also if the notify order for chain parameters handler.
it needs to at least be before nodes coordinator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

refactored - so that it uses the epoch notifier with ordering

iulianpascalau
iulianpascalau previously approved these changes Dec 16, 2022

paramsHolder := &chainParametersHolder{
currentChainParameters: currentParams,
currentChainParameters: earliestChainParams, // will be updated on the epoch notifier handlers
Copy link
Contributor

Choose a reason for hiding this comment

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

yup, that is the behavior right now. I do not see a good reason why we should change this in the future.

@bogdan-rosianu bogdan-rosianu merged commit e559437 into feat/consensus-size-changes Dec 21, 2022
@bogdan-rosianu bogdan-rosianu deleted the EN-13432-shuffler-refactoring branch December 21, 2022 15:16
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.

5 participants