chain parameters notifier #4927

bogdan-rosianu · 2023-01-30T14:59:49Z

Reasoning behind the pull request

some components used static values for consensus group size

Proposed changes

created a chain parameters change notifier and integrate it where needed

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?

iulianpascalau · 2023-02-02T09:59:37Z

common/chainparametersnotifier/chainParametersNotifier.go

+}
+
+// New creates a new instance of a chainParametersNotifier component
+func New() *chainParametersNotifier {


NewChainParametersNotifier to be consistent with the rest of the project

iulianpascalau · 2023-02-02T10:09:12Z

common/chainparametersnotifier/chainParametersNotifier.go

+	cpn.currentChainParameters = params
+	cpn.mutData.Unlock()
+
+	cpn.mutHandler.RLock()


L32-L46 ensures that the handlers are called only once for each epoch change regardless of the number of UpdateCurrentChainParameters calls. However, if the calls for different epochs are called in extremely fast manner in parallel, some calls on the handlers can be made in a different order. I do not think this will be an issue because the processing is done in sync.

iulianpascalau · 2023-02-02T10:15:44Z

factory/interface.go

 	SetPeerValidatorMapper(validatorMapper process.PeerValidatorMapper) error
 	SetTopicsForAll(topics ...string)
-	ApplyConsensusSize(size int)
+	SetConsensusSizeNotifier(chainParametersNotifier process.ChainParametersSubscriber, shardID uint32)


let's inject this into the constructor. No need for a setter.

Since the shard ID is unknown when creating the antiflood components, I cannot make this change

iulianpascalau · 2023-02-02T10:18:00Z

process/throttle/antiflood/p2pAntiflood.go

+		Value:    shardID,
+	}
+
+	chainParametersNotifier.RegisterNotifyHandler(af)


this can be called on the constructor

Since the shard ID is unknown when creating the antiflood components, I cannot make this change

iulianpascalau · 2023-02-02T10:20:11Z

process/throttle/antiflood/p2pAntiflood.go

+
+// ChainParametersChanged will be called when new chain parameters are confirmed on the network
+func (af *p2pAntiflood) ChainParametersChanged(chainParameters config.ChainParametersByEpochConfig) {
+	size := chainParameters.ShardConsensusGroupSize


concurrency issues on the usage of the af.shardID ?
Also why do we need the set on L76?

added a mutex. the setter is needed for computing the consensus size in case of shard vs meta

iulianpascalau · 2023-02-03T06:43:35Z

common/chainparametersnotifier/chainParametersNotifier_test.go

 	"github.com/stretchr/testify/require"
 )

 func TestNew(t *testing.T) {


TestNewChainParametersNotifier ?

iulianpascalau · 2023-02-03T06:44:42Z

process/throttle/antiflood/p2pAntiflood.go


 // Close will call the close function on all sub components
 func (af *p2pAntiflood) Close() error {
+	af.mutDebugger.Lock()


…fier

codecov-commenter · 2023-02-17T09:11:03Z

Codecov Report

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

❗ Current head 80d86a8 differs from pull request most recent head 961bb62. Consider uploading reports for the commit 961bb62 to get more accurate results

Additional details and impacted files

@@                      Coverage Diff                       @@
##             feat/consensus-size-changes    #4927   +/-   ##
==============================================================
  Coverage                               ?   70.91%           
==============================================================
  Files                                  ?      677           
  Lines                                  ?    87950           
  Branches                               ?        0           
==============================================================
  Hits                                   ?    62370           
  Misses                                 ?    20897           
  Partials                               ?     4683

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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

…fier # Conflicts: # common/interface.go # factory/consensus/consensusComponents.go # integrationTests/testConsensusNode.go # process/throttle/antiflood/p2pAntiflood_test.go

iulianpascalau · 2023-04-03T07:43:56Z

factory/consensus/consensusComponents.go

 	}
 	cc := &consensusComponents{}

-	consensusGroupSize, err := getConsensusGroupSize(ccf.coreComponents.GenesisNodesSetup(), ccf.processComponents.ShardCoordinator())


iulianpascalau · 2023-04-03T07:44:24Z

process/throttle/antiflood/p2pAntiflood.go

+		Value:    shardID,
+	}
+
+	chainParametersNotifier.RegisterNotifyHandler(af)


iulianpascalau · 2023-04-03T07:47:55Z

node/node.go

 type Node struct {
 	initialNodesPubkeys map[uint32][]string
 	roundDuration       uint64
-	consensusGroupSize  int


minor conflicts here when we will bring the rc/v1.5.0 in rc/v1.6.0 because I've added unit test for the getter & setter of the consensus group size

I'll happily delete those unit tests when merging 😄

iulianpascalau · 2023-04-03T07:49:27Z

sharding/nodesCoordinator/indexHashedNodesCoordinator_test.go

+				}, nil
+			}
+
+			if epoch < 9 {


👍 nice approach to avoid calls like if (epoch < 9) && (epoch >= 6) {...

iulianpascalau · 2023-04-03T07:51:20Z

sharding/nodesCoordinator/indexHashedNodesCoordinator_test.go

+		EpochStart:   block.EpochStart{LastFinalizedHeaders: []block.EpochStartShardData{{}}},
+	}
+
+	// change to epoch 1 - should have 3 eligible per shard, 3 per meta


I would also add tests for epochs 2, 3, 4, 6, 8, 9, 10, and 11 to test the correct change conditions

refactored the unit test and now it checks from epoch 0 to epoch 100

gabi-vuls

@@ Log scanner @@

chain-params-notifier-test-mar30

================================================================================

Known Warnings 11
New Warnings 5
Known Errors 0
New Errors 0
Panics 0
================================================================================
block hash does not match 10501
wrong nonce in block 3632
miniblocks does not match 0
num miniblocks does not match 0
miniblock hash does not match 0
block bodies does not match 0
receipts hash missmatch 0
================================================================================
No jailed nodes on the thestnet
================================================================================

AdoAdoAdo · 2023-04-11T08:00:38Z

process/throttle/antiflood/p2pAntiflood_test.go


-	afm.ApplyConsensusSize(expectedSize)
+	chainParamsSubscriber := chainparametersnotifier.NewChainParametersNotifier()
+	afm.SetConsensusSizeNotifier(chainParamsSubscriber, 5)


can you use a variable/constant instead of literal 5?

AdoAdoAdo · 2023-04-11T08:05:53Z

process/throttle/antiflood/p2pAntiflood.go


 // Close will call the close function on all sub components
 func (af *p2pAntiflood) Close() error {
+	af.mutDebugger.Lock()


gabi-vuls

System test passed

chain parameters notifier

8238e5b

bogdan-rosianu added area:processing complexity:medium labels Jan 30, 2023

bogdan-rosianu self-assigned this Jan 30, 2023

bogdan-rosianu added 5 commits January 30, 2023 17:13

fix linter + stubs

be4806f

fix stub

e071567

added test for the notification of the new params

6b6e478

remove unused interface

053fe06

fix subscriber instance

617da1e

bogdan-rosianu marked this pull request as ready for review February 2, 2023 09:35

iulianpascalau self-requested a review February 2, 2023 09:36

iulianpascalau reviewed Feb 2, 2023

View reviewed changes

fixes after review

abc1890

iulianpascalau reviewed Feb 3, 2023

View reviewed changes

bogdan-rosianu and others added 3 commits February 17, 2023 10:51

Merge branch 'feat/consensus-size-changes' into chain-parameters-noti…

1b49b5a

…fier

test renaming

a885724

fix compilation issue

b210638

iulianpascalau previously approved these changes Feb 17, 2023

View reviewed changes

add changes from the consensus-group-size-further-updates branch

56bcfb7

bogdan-rosianu dismissed iulianpascalau’s stale review via 56bcfb7 February 21, 2023 14:34

bogdan-rosianu added 3 commits February 21, 2023 17:39

remove option

19b2db2

Merge branch 'feat/consensus-size-changes' into chain-parameters-noti…

fbe6f81

…fier # Conflicts: # common/interface.go # factory/consensus/consensusComponents.go # integrationTests/testConsensusNode.go # process/throttle/antiflood/p2pAntiflood_test.go

fix init in consensus components

55b9939

iulianpascalau reviewed Apr 3, 2023

View reviewed changes

extended unit test

5601282

iulianpascalau previously approved these changes Apr 3, 2023

View reviewed changes

gabi-vuls previously approved these changes Apr 4, 2023

View reviewed changes

AdoAdoAdo reviewed Apr 11, 2023

View reviewed changes

fixes after review

961bb62

bogdan-rosianu dismissed stale reviews from gabi-vuls and iulianpascalau via 961bb62 April 11, 2023 11:39

AdoAdoAdo approved these changes Apr 11, 2023

View reviewed changes

iulianpascalau approved these changes Apr 11, 2023

View reviewed changes

gabi-vuls approved these changes Apr 11, 2023

View reviewed changes

bogdan-rosianu merged commit ab675e9 into feat/consensus-size-changes Apr 11, 2023

bogdan-rosianu deleted the chain-parameters-notifier branch April 11, 2023 13:40

chain parameters notifier #4927

chain parameters notifier #4927

Uh oh!

Conversation

bogdan-rosianu commented Jan 30, 2023

Reasoning behind the pull request

Proposed changes

Testing procedure

Pre-requisites

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Feb 17, 2023 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabi-vuls left a comment

Choose a reason for hiding this comment

chain-params-notifier-test-mar30

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabi-vuls left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-commenter commented Feb 17, 2023 •

edited by codecov bot

Loading