Skip to content
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

Make sure broadcast happens #1871

Merged
merged 26 commits into from Jun 21, 2020
Merged

Conversation

AdoAdoAdo
Copy link
Contributor

@AdoAdoAdo AdoAdoAdo commented Jun 5, 2020

This implementation adds a fallback for broadcasting cross shard data.
In case leader fails to broadcast the cross shard miniblocks and transactions then participants in consensus will schedule their slots for doing the broadcast instead of consensus leader in the order they were assigned in consensus.
The broadcast slots are canceled when the cross shard data is observed on the cross shard topics.

@AdoAdoAdo AdoAdoAdo self-assigned this Jun 5, 2020
# Conflicts:
#	epochStart/bootstrap/epochStartMetaBlockProcessor.go
#	epochStart/bootstrap/epochStartMetaBlockProcessor_test.go
#	epochStart/mock/metaBlockInterceptorProcessorStub.go
#	process/interceptors/processor/hdrInterceptorProcessor.go
#	process/interceptors/processor/miniblockInterceptorProcessor.go
#	process/interceptors/processor/trieNodeInterceptorProcessor.go
#	process/interceptors/processor/txInterceptorProcessor.go
#	process/interface.go
#	process/mock/interceptorProcessorStub.go
@iulianpascalau iulianpascalau self-requested a review June 16, 2020 07:51
@AdoAdoAdo AdoAdoAdo marked this pull request as ready for review June 16, 2020 08:27
consensus/broadcast/delayedBroadcast.go Outdated Show resolved Hide resolved
consensus/broadcast/delayedBroadcast.go Outdated Show resolved Hide resolved
consensus/broadcast/delayedBroadcast.go Show resolved Hide resolved
core/alarm/alarm.go Outdated Show resolved Hide resolved
core/alarm/alarm.go Show resolved Hide resolved
process/interceptors/processor/txInterceptorProcessor.go Outdated Show resolved Hide resolved
@SebastianMarian SebastianMarian self-requested a review June 16, 2020 18:02
if check.IfNil(args.HeadersSubscriber) {
return spos.ErrNilHeadersSubscriber
}
if args.MaxDelayCacheSize == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

MaxValidatorDelayCacheSize should not be checked for 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, done

return spos.ErrNilParameter
}

dbb.mutDataForBroadcast.RLock()
Copy link
Contributor

Choose a reason for hiding this comment

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

Lock/Unlock

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


log.Debug("delayedBroadcast.headerReceived", "nbHeaderHashes", len(headerHashes))
for i := range headerHashes {
log.Debug("delayedBroadcast.headerReceived", "headerHash", headerHashes[i])
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be set to Trace after we are sure that everything will be ok

log.Debug("delayedBroadcast.scheduleValidatorBroadcast - header data for validator")

for i := range dataForValidators {
log.Debug("delayedBroadcast.scheduleValidatorBroadcast",
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be set to Trace after we are sure that everything will be ok


log.Debug("delayedBroadcast.scheduleValidatorBroadcast - registered data for broadcast")
for i := range dbb.valBroadcastData {
log.Debug("delayedBroadcast.scheduleValidatorBroadcast",
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be set to Trace after we are sure that everything will be ok

dbb.cacheHeaders.Put(headerHash, struct{}{}, 0)
dbb.mutHeadersCache.Unlock()

dbb.mutDataForBroadcast.Lock()
Copy link
Contributor

Choose a reason for hiding this comment

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

RLock/RUnlock

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

log.Debug("delayedBroadcast.interceptedHeader canceling alarm for broadcasting header",
"headerHash", headerHash,
"alarmID-header", alarmID,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

You add break here, if there could not be more than one header witth the same round and prevRandSeed in dbb.valHeaderBroadcastData

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

"headerHash", broadcastData.headerHash,
"alarmID-delay", alarmID,
)
return
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to iterate all dbb.valBroadcastData? There could not be more records with the same topic and hash in dbb.valBroadcastData ?

Copy link
Contributor Author

@AdoAdoAdo AdoAdoAdo Jun 18, 2020

Choose a reason for hiding this comment

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

should only be one, if the doEndRoundJob is called only once per consensus round, unless different leaders include the same miniblock hash "fromMe".

To cover also this case, removed the return.

case <-ctx.Done():
return
case evt := <-as.event:
elapsedTime := time.Now().Sub(startTime)
Copy link
Contributor

Choose a reason for hiding this comment

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

time.Since(startTime)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -78,6 +83,8 @@ func (mip *MiniblockInterceptorProcessor) Save(data process.InterceptedData, _ c
return err
}

go mip.notify(miniblock, interceptedMiniblock.Hash(), topic)
Copy link
Contributor

Choose a reason for hiding this comment

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

This will notify even the cross miniblocks dest me and even if they are not whitelisted. It is ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The miniblocks "destination me" can be ignored, on subscriber (this is done on delayedBlockBroadcaster, as it waits for specific miniblocks on specific topics).

The whitelisting is done on the "cross shard destination me" miniblocks, whereas for delayed broadcaster the "cross shard source me" are relevant.

I agree that if the miniblock is received before the metablock notarizing the parent header then it could be ignored on destination, so waiting for suggestions here.

# Conflicts:
#	integrationTests/consensus/testInitializer.go
#	node/node_test.go
Copy link
Contributor

@LucianMincu LucianMincu left a comment

Choose a reason for hiding this comment

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

System tests passed.

@LucianMincu LucianMincu merged commit ba8995b into development Jun 21, 2020
@LucianMincu LucianMincu deleted the make-sure-broadcast-happens branch June 21, 2020 14:05
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.

None yet

4 participants