EN-5829: node start in epoch #1238

bogdan-rosianu · 2020-03-17T16:36:52Z

No description provided.

…-start-mb # Conflicts: # cmd/node/factory/structs.go # cmd/node/main.go # dataRetriever/factory/resolverscontainer/shardResolversContainerFactory.go # facade/elrondNodeFacade.go # facade/elrondNodeFacade_test.go

…-start-mb # Conflicts: # integrationTests/testInitializer.go

…-start-in-epoch-to-bootstrap-from-storage

…on-test' into EN-5829-link-start-in-epoch-to-bootstrap-from-storage # Conflicts: # epochStart/bootstrap/epochStartDataProvider.go # epochStart/bootstrap/errors.go

…into EN-5829-link-start-in-epoch-to-bootstrap-from-storage # Conflicts: # cmd/node/main.go # epochStart/bootstrap/epochStartDataProvider.go # epochStart/bootstrap/factory/epochStartDataProviderFactory.go

epochStart/bootstrap/nodesCoordinator.go

iulianpascalau · 2020-03-26T16:43:55Z

epochStart/bootstrap/process.go

@@ -44,6 +44,13 @@ var log = logger.GetOrCreate("epochStart/bootstrap")

 const timeToWait = 5 * time.Second

+// BootstrapParameters
+type Parameters struct {


Maybe BootstrapParameters ?

it is not recommended to have the same starting string as the package name.

EpochCoordinates :)

Anyway, fix comment.

…o-bootstrap-from-storage # Conflicts: # cmd/node/factory/structs.go # cmd/node/main.go # process/peer/process.go # sharding/indexHashedNodesCoordinator.go # sharding/indexHashedNodesCoordinatorRegistry.go

andreibancioiu · 2020-03-30T17:00:39Z

data/trie/branchNode.go

 		if err != nil {
-			return err
+			missingChildren = append(missingChildren, bn.EncodedChildren[i])


Should we Trace() the error perhaps, at least, or not necessary?

not necessary - this is called by syncing mechanism so it is normal to have a few missing - than request it.

andreibancioiu · 2020-03-30T18:28:09Z

update/sync/syncHeadersByHash.go

-			return err
+		if requestedHdrs == 0 {
+			m.mutMissingHdrs.Lock()
+			m.stopSyncing = true


If syncedAll and stopSyncing were atomic.Flag, there would be less mutex handling inside select expressions - improved readability, less complexity.

I would leave it as it is for now.

andreibancioiu · 2020-03-30T18:32:20Z

update/sync/syncHeadersByHash.go

 }

 // receivedHeader is a callback function when a new header was received
 // it will further ask for missing transactions
-func (m *missingHeadersByHash) receivedHeader(hdrHandler data.HeaderHandler, hdrHash []byte) {
+func (m *syncHeadersByHash) receivedHeader(hdrHandler data.HeaderHandler, hdrHash []byte) {


Why not using Unlock with defer here? Could cause troubles if function is modified without full attention.

not possible as you have the h.chReceivedAll <- true which should be called outside of mutex.

andreibancioiu · 2020-03-30T18:35:41Z

update/sync/syncMiniBlocks.go

-	return nil
+		select {
+		case <-p.chReceivedAll:
+			p.mutPendingMb.Lock()


As before, perhaps atomic flags (core/atomic/flag.go) instead of using so much mutex handling in channel-related expressions (can improve code readability).

I would leave it as it is for now.

andreibancioiu · 2020-03-30T18:52:27Z

data/syncer/userAccountsSyncer.go

@@ -52,6 +54,10 @@ func (u *userAccountsSyncer) SyncAccounts(rootHash []byte) error {
 	u.mutex.Lock()
 	defer u.mutex.Unlock()

+	ctx, cancel := context.WithTimeout(context.Background(), u.waitTime)
+	defer cancel()
+	u.ctx = ctx


According to the docs,

Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it.

This is the pattern, but not mandatory:

golang/go#22602

agree, with little injecting work we can drop u.ctx = ctx

andreibancioiu · 2020-03-30T19:28:35Z

sharding/indexHashedNodesCoordinatorRegistry.go

+	validatorsMap := make(map[uint32][]Validator)
+
+	for shId, nodeInfoList := range nodesInfo {
+		validators := make([]Validator, 0)


Can set capacity optionally.

andreibancioiu · 2020-03-30T19:31:10Z

epochStart/bootstrap/nodesCoordinator.go

+	for _, validatorInfo := range validatorInfos {
+		validator, err := sharding.NewValidator(validatorInfo.PublicKey, validatorInfo.RewardAddress)
+		if err != nil {
+			return nil, err


Break-break or skip validator info?

break. as error

andreibancioiu · 2020-03-30T19:33:29Z

epochStart/bootstrap/nodesCoordinator.go

+
+func (n *nodesCoordinator) computeLeaving(allValidators []*state.ValidatorInfo) ([]sharding.Validator, error) {
+	leavingValidators := make([]sharding.Validator, 0)
+	minChances := n.chance.GetChance(0)


minChanceOfNotLeaving?

I would keep it simple.

andreibancioiu · 2020-03-30T19:40:50Z

epochStart/bootstrap/fromLocalStorage.go

+		return Parameters{}, err
+	}
+
+	unitsToOpen := make([]string, 0)


May use simple initialization instead of append.

andreibancioiu · 2020-03-30T19:41:47Z

epochStart/bootstrap/fromLocalStorage.go

+	}
+
+	if e.shardCoordinator.SelfId() == core.MetachainShardId {
+		err = e.requestAndProcessForShard()


If meta, requestAndProcessForShard / from shard? Just double check if this is correct.

…ting from epoch 0.

iulianpascalau · 2020-03-31T09:06:41Z

data/syncer/userAccountsSyncer.go

@@ -52,6 +54,10 @@ func (u *userAccountsSyncer) SyncAccounts(rootHash []byte) error {
 	u.mutex.Lock()
 	defer u.mutex.Unlock()

+	ctx, cancel := context.WithTimeout(context.Background(), u.waitTime)
+	defer cancel()
+	u.ctx = ctx


agree, with little injecting work we can drop u.ctx = ctx

data/trie/sync.go

iulianpascalau · 2020-03-31T09:20:41Z

data/trie/sync.go

 	}
-	return false
+
+	ts.receivedNodesMutex.Lock()


might extract this

3 lines of code used in a single space

iulianpascalau · 2020-03-31T09:21:14Z

data/trie/sync.go

-	if hashInSlice(hash, ts.requestedHashes) {
-		ts.chRcvTrieNodes <- true
-		ts.removeRequestedHash(hash)
+	ts.nodeHashesMutex.Lock()


might extract this

3 lines of code used in a single space

iulianpascalau · 2020-03-31T09:21:57Z

dataRetriever/requestHandlers/requestHandler.go

@@ -47,6 +49,9 @@ func NewResolverRequestHandler(
 	if check.IfNil(whiteList) {
 		return nil, dataRetriever.ErrNilWhiteListHandler
 	}
+	if requestInterval < time.Millisecond {
+		return nil, dataRetriever.ErrRequestIntervalTooSmall


might wrap this

it is of common sense to put more than a millisecond. done.

Agree, but wrapping the error is all about finding the problem in no time (most of the start-up problems are due to bad configs). Showing a simple, inexpressive "nil cacher" message should be avoided.

epochStart/bootstrap/process.go

iulianpascalau · 2020-03-31T09:43:42Z

epochStart/bootstrap/simpleEpochStartMetaBlockInterceptor.go

+}
+
+// ProcessReceivedMessage will receive the metablocks and will add them to the maps
+func (s *simpleEpochStartMetaBlockInterceptor) ProcessReceivedMessage(message p2p.MessageP2P, _ p2p.PeerID) error {


epochStart/metachain/trigger.go

sharding/indexHashedNodesCoordinatorRegistry.go

iulianpascalau · 2020-03-31T10:32:19Z

update/sync/syncMiniBlocks.go

 	p.mutPendingMb.Unlock()

-	var err error
-	defer func() {
+	for {


duplicate code with syncHeadersByHash.go. Add a TODO to extract the common functionality

namings are different. mutexes are different. not really duplicated code - yes, same principle. the code is kept as clean as possible. the thing is underlying structures have different elements - one headers other miniblocks. go does not support generics.

Will think of a solution to this.

bogdan-rosianu and others added 20 commits February 27, 2020 15:51

EN-5829: epoch start data provider first implementation

3277e6b

EN-5829: removed a func

8a9acd0

EN-5829: some changes

1bcf1e9

EN-5829: added requests for previous epoch start

335bdcb

EN-5829: added unit tests and some changes

8dec7d7

EN-5829: fixed bug when requesting eoch start metablock from epoch 0

346daa5

EN-5736: fix after review

321ec2f

further work (WIP)

79e1985

some refactoring + added more interceptors

4147784

removed unused const

39f89f5

fix after merge

ca5384a

Merge branch 'feat/start-in-epoch' into EN-5829-request-receive-epoch…

a2de97f

…-start-mb # Conflicts: # integrationTests/testInitializer.go

fix after second merge + cleanup

38c5f2a

EN-5829: integration test for start in epoch

f6b80e1

EN-5829: fix after review

b189822

EN-5829: link with bootstrap from storage (WIP)

8f49fea

Merge branch 'dev-hardfork-to-start-in-epoch-mar18' into EN-5829-link…

24671c9

…-start-in-epoch-to-bootstrap-from-storage

merges and fixes.

690f885

merges and fixes.

c330312

iulianpascalau assigned bogdan-rosianu Mar 18, 2020

iulianpascalau added the type:feature New feature or request label Mar 18, 2020

sasurobert and others added 8 commits March 19, 2020 10:46

Merge remote-tracking branch 'origin/EN-5829-start-in-epoch-integrati…

5280209

…on-test' into EN-5829-link-start-in-epoch-to-bootstrap-from-storage # Conflicts: # epochStart/bootstrap/epochStartDataProvider.go # epochStart/bootstrap/errors.go

merges and fixes.

1f5cc4b

fixes, moving implementation to bootstrapper.

f22b91d

EN-5829 created interceptors container (WIP)

f48bc1f

fixes, moving implementation to bootstrapper.

47e9410

EN-5829 added sync headers by hash + integration (WIP)

607e5c2

fixes, moving implementation to bootstrapper.

fdd2c5c

Merge remote-tracking branch 'origin/EN-5829-refactor-data-provider' …

899f9f6

…into EN-5829-link-start-in-epoch-to-bootstrap-from-storage # Conflicts: # cmd/node/main.go # epochStart/bootstrap/epochStartDataProvider.go # epochStart/bootstrap/factory/epochStartDataProviderFactory.go

sasurobert added 2 commits March 25, 2020 20:23

finished the remaining implementation

85d8f4a

fixes after review

3db82f5

iulianpascalau reviewed Mar 26, 2020

View reviewed changes

epochStart/bootstrap/nodesCoordinator.go Show resolved Hide resolved

epochStart/bootstrap/nodesCoordinator.go Outdated Show resolved Hide resolved

sasurobert and others added 6 commits March 26, 2020 14:25

fixes after review

2f4e039

EN-6013: updated integration test + fixes

3002baf

fixes after review

d96a683

fixes after review

982dbcc

fix edge case

38d5c2d

EN-6013: updated integration test + fixes 2

c0d7f5f

iulianpascalau reviewed Mar 26, 2020

View reviewed changes

bogdan-rosianu and others added 9 commits March 26, 2020 19:22

EN-6013: fixed 2 hardcoded fields

ab3a026

logs + configs

f79f4f1

Merge branch 'feat/start-in-epoch' into EN-5829-link-start-in-epoch-t…

5b6e489

…o-bootstrap-from-storage # Conflicts: # cmd/node/factory/structs.go # cmd/node/main.go # process/peer/process.go # sharding/indexHashedNodesCoordinator.go # sharding/indexHashedNodesCoordinatorRegistry.go

merge from feat.

dcc1718

fixed network sharding integration tests

e4b0487

logs + configs

b456da3

new trie syncer with Context.

250aada

added more implementation.

73db391

added more implementation.

3bf3409

andreibancioiu reviewed Mar 30, 2020

View reviewed changes

sasurobert and others added 3 commits March 31, 2020 00:51

no need for updateListIndex, fixed shard coordinator return when star…

22b8222

…ting from epoch 0.

no need for updateListIndex, fixed shard coordinator return when star…

23d6891

…ting from epoch 0.

EN-6030: fix trie storers (temporary)

232ba2e

iulianpascalau reviewed Mar 31, 2020

View reviewed changes

sasurobert added 2 commits March 31, 2020 14:30

processing fixes.

9ec5def

fix after review.

dfbbfcf

andreibancioiu approved these changes Mar 31, 2020

View reviewed changes

iulianpascalau approved these changes Mar 31, 2020

View reviewed changes

iulianpascalau merged commit 5ad08ed into feat/start-in-epoch Mar 31, 2020

iulianpascalau deleted the EN-5829-link-start-in-epoch-to-bootstrap-from-storage branch March 31, 2020 14:21

EN-5829: node start in epoch #1238

EN-5829: node start in epoch #1238

Conversation

bogdan-rosianu commented Mar 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreibancioiu Mar 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreibancioiu Mar 30, 2020 •

edited

Loading