Handle concurrency issues occurred with building hyperblock coordinates (using the fast-replay method) #2263
Conversation
andreibancioiu
requested review from
AdoAdoAdo,
SebastianMarian and
LucianMincu
cmd/node/config/config.toml
Outdated
| @@ -645,7 +645,7 @@ | |||
| PollingIntervalInMinutes = 65 | |||
|
|
|||
| [DbLookupExtensions] | |||
| Enabled = false | |||
| Enabled = true # TODO: Disable after tests | |||
There was a problem hiding this comment.
remember to disable after tests.
|
|
||
| hr.notarizedAtSourceNotifications.Remove(string(miniblockHash)) | ||
| hr.pendingNotarizedAtBothNotifications.Set(string(miniblockHash), ¬arizedNotification{ | ||
| metaNonce: blockHeader.GetNonce(), |
There was a problem hiding this comment.
blockHeader is always a meta block?
There was a problem hiding this comment.
Fixed as discussed (removed "simulated notification").
…lock tracker. This notification works either from shard nodes or meta nodes.
| } | ||
| } | ||
|
|
||
| func (hr *historyRepository) onNotarizedInMetaBlock(metaBlockNonce uint64, metaBlockHash []byte, shardData *block.ShardData) { | ||
| for _, miniblockHeader := range shardData.GetShardMiniBlockHeaders() { | ||
| hr.onNotarizedMiniblock(metaBlockNonce, metaBlockHash, shardData.GetShardID(), miniblockHeader) | ||
| if metaBlockNonce <= 1 { |
There was a problem hiding this comment.
< 1 instead <= 1. If meta misses some rounds it can include in nonce 1 a lot of shardData if the shards worked good
| iDontCare := miniblockHeader.SenderShardID != hr.selfShardID && miniblockHeader.ReceiverShardID != hr.selfShardID | ||
| isNotarizedAtSource := miniblockHeader.SenderShardID == shardOfContainingBlock | ||
| isNotarizedAtDestination := miniblockHeader.ReceiverShardID == shardOfContainingBlock | ||
| isNotarizedAtBoth := isIntra || isToMeta || (isFromMeta && isNotarizedAtDestination) |
There was a problem hiding this comment.
I suggested the last condition, but thinking a bit about it I don't think it is entirely correct.
isNotarizedAtBoth should cover the miniblocks that are notarized in a single metablock for both source and destination.
- for isIntra this is true, as bot sender and destination solved
- for isToMeta this is again true as when included in metablock it means that also destination (meta) was executed in adition to source notarization
- for isFromMeta && notarized at destination, this is not true, as the sender notarizationi (metachain) is done in a different metablock, where metachain is the source. For the destination should follow the isNotarizedAtDestination logic.
Was there a problem maybe that the "from metachain" miniblocks were not reported on source?
Checking the function onNotarizedInMetablock only the shardData.ShardMiniblockHeaders (for every shard) are reported.
To cover also the from metachain miniblocks at source, also the metablock.MiniBlockHeaders should be reported.
There was a problem hiding this comment.
Makes total sense, fixed & tested.
By exploring the fastly-rebuilt database lookup extensions, we can confirm that some info is missing from the miniblocks metadata.
(data in 3 shards)
Under the assumption of concurrent commits and "notarized" notifications, I've redesigned the way notifications are processed. All notifications are now added to a pending state, and consumed as soon as possible (both at commit and at notify).
Furthermore,
historyRepositoryregisters to a brand new, custom handler of the block tracker - by @SebastianMarian, in order to always receive "notarized" notifications, along with themetaBlock.