-
Notifications
You must be signed in to change notification settings - Fork 205
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
Metachain notarizez based on equivalent proofs #6513
Conversation
process/block/baseProcess.go
Outdated
@@ -639,7 +642,7 @@ func (bp *baseProcessor) sortHeaderHashesForCurrentBlockByNonce(usedInBlock bool | |||
|
|||
bp.hdrsForCurrBlock.mutHdrsForBlock.RLock() | |||
for metaBlockHash, headerInfo := range bp.hdrsForCurrBlock.hdrHashAndInfo { | |||
if headerInfo.usedInBlock != usedInBlock { | |||
if headerInfo.usedInBlock != usedInBlock || !headerInfo.hasProof { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that if a header is used in block but has no valid proof, then this is an error case
Could you add the condition separately and log an error for that?
in case we end up in this case we need to find out how it happened and fix.
When picking up a header to notarize it, and adding it to usedInBlock then it should have already been checked that there is a valid proof for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
usedInBlock
field is set to true also when shard headers are requested.. it is basically set in advance to true, even though the header is missing.. so maybe the proof would be received later.. not sure if this is possible though
process/block/metablock.go
Outdated
@@ -1838,6 +1867,20 @@ func (mp *metaProcessor) checkShardHeadersFinality( | |||
continue | |||
} | |||
|
|||
if mp.enableEpochsHandler.IsFlagEnabledInEpoch(common.EquivalentMessagesFlag, lastVerifiedHdr.GetEpoch()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this loop is correct, as it does not get to iterate over all shard headers and verify.
it returns either a nil or an error in the first iteration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true, moved this if into the next for loop and switched the return nil
into a break
process/block/metablock.go
Outdated
hasProof := mp.proofsPool.HasProof(shardHeader.GetShardID(), shardHeaderHash) | ||
hdrInfoForHash.hasProof = hasProof | ||
if hasProof { | ||
mp.hdrsForCurrBlock.missingHdrsProofs-- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missingHdrsProofs should be decreased if that proof was missing previously and still missing at this step only
e.g you receive a shard hdr which is not part of the metablock (an older block maybe), so here you would end up decreasing the missing headers proofs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated the condition, it was not working as expected
process/block/metablock.go
Outdated
mp.enableEpochsHandler.IsFlagEnabledInEpoch(common.EquivalentMessagesFlag, headerInfo.hdr.GetEpoch()) | ||
hasMissingProof := isBlockAfterEquivalentMessagesFlag && !headerInfo.hasProof | ||
if hasMissingProof { | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is an error if a header was used in block but has no proof, should not continue but return an error.
If we get such errors, then we need to fix in the place where the headers are notarized, as there should be the check that the notarized headers have the proper proof.
Please check that the shardInfo (createShardInfo) contains the proof for the shard header itself, along with the proof for the previous block.
This means that the shardInfo would change as well to include 2 proofs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated to return error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated also to use both proofs
process/block/shardblock.go
Outdated
core.MetachainShardId, | ||
sp.metaBlockFinality, | ||
) | ||
if !sp.shouldConsiderProofsForNotarization(metaBlock) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on the else, you should at least check that there is a valid proof for this metablock, otherwise add it to the missing proofs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
process/block/metablock.go
Outdated
if mp.hdrsForCurrBlock.missingHdrs == 0 { | ||
mp.hdrsForCurrBlock.missingFinalityAttestingHdrs = mp.requestMissingFinalityAttestingShardHeaders() | ||
if !mp.shouldConsiderProofsForNotarization(shardHeader) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on the else, you should check that there is a valid proof for this header, otherwise add it to the missing proofs.
process/block/shardblock.go
Outdated
hasProofForShardHdr := sp.proofsPool.HasProof(core.MetachainShardId, metaBlockHashes[i]) | ||
sp.hdrsForCurrBlock.hdrHashAndInfo[string(metaBlockHashes[i])].hasProof = hasProofForShardHdr | ||
if !hasProofForShardHdr { | ||
sp.hdrsForCurrBlock.missingHdrsProofs++ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add the header hash in a map for missing proofs instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved the logic to be based on hasProof field, which is part of a map too
process/block/hdrForBlock.go
Outdated
@@ -9,6 +9,7 @@ import ( | |||
type hdrForBlock struct { | |||
missingHdrs uint32 | |||
missingFinalityAttestingHdrs uint32 | |||
missingHdrsProofs uint32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i might help to rename here to indicate that it's num missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed it
process/block/metablock.go
Outdated
if len(currShardHdr.GetMiniBlockHeadersWithDst(mp.shardCoordinator.SelfId())) == 0 { | ||
mp.hdrsForCurrBlock.hdrHashAndInfo[string(orderedHdrsHashes[i])] = &hdrInfo{hdr: currShardHdr, usedInBlock: true} | ||
mp.hdrsForCurrBlock.hdrHashAndInfo[string(orderedHdrsHashes[i])] = &hdrInfo{hdr: currShardHdr, usedInBlock: true, hasProof: true} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
split on multiple lines for better visibility? same below
process/block/metablock.go
Outdated
@@ -1081,8 +1089,18 @@ func (mp *metaProcessor) createAndProcessCrossMiniBlocksDstMe( | |||
continue | |||
} | |||
|
|||
shouldCheckProof := mp.enableEpochsHandler.IsFlagEnabledInEpoch(common.EquivalentMessagesFlag, currShardHdr.GetEpoch()) | |||
hasProofForHdr := mp.proofsPool.HasProof(currShardHdr.GetShardID(), orderedHdrsHashes[i]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think here we should check if HasProof
only if shouldCheckProof
is true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct, optimization.. implemented it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adapt also sortHeadersForCurrentBlockByNonce
to have hasProof check?
process/block/baseProcess.go
Outdated
@@ -639,7 +642,7 @@ func (bp *baseProcessor) sortHeaderHashesForCurrentBlockByNonce(usedInBlock bool | |||
|
|||
bp.hdrsForCurrBlock.mutHdrsForBlock.RLock() | |||
for metaBlockHash, headerInfo := range bp.hdrsForCurrBlock.hdrHashAndInfo { | |||
if headerInfo.usedInBlock != usedInBlock { | |||
if headerInfo.usedInBlock != usedInBlock || !headerInfo.hasProof { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check for enabled flag here?
process/block/metablock.go
Outdated
@@ -1838,6 +1867,20 @@ func (mp *metaProcessor) checkShardHeadersFinality( | |||
continue | |||
} | |||
|
|||
if mp.enableEpochsHandler.IsFlagEnabledInEpoch(common.EquivalentMessagesFlag, lastVerifiedHdr.GetEpoch()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it fits better to check proof in checkShardHeadersValidity
which is called just before instead of here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should stay here, as here is the place where we check for finality
process/block/shardblock.go
Outdated
|
||
headerHash := sp.hasher.Compute(string(marshalledHeader)) | ||
if !sp.proofsPool.HasProof(header.GetShardID(), headerHash) { | ||
return process.ErrHeaderNotFinal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return a composed error here to indicate also that the proof is missing?
process/block/shardblock.go
Outdated
createAndProcessInfo.currMetaHdrHash = orderedMetaBlocksHashes[i] | ||
if len(createAndProcessInfo.currMetaHdr.GetMiniBlockHeadersWithDst(sp.shardCoordinator.SelfId())) == 0 { | ||
sp.hdrsForCurrBlock.hdrHashAndInfo[string(createAndProcessInfo.currMetaHdrHash)] = &hdrInfo{hdr: createAndProcessInfo.currMetaHdr, usedInBlock: true} | ||
sp.hdrsForCurrBlock.hdrHashAndInfo[string(createAndProcessInfo.currMetaHdrHash)] = &hdrInfo{hdr: createAndProcessInfo.currMetaHdr, usedInBlock: true, hasProof: true} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion to expand on multiple lines
…rsx/mx-chain-go into adapt_notarization # Conflicts: # process/block/baseProcess.go
…rsx/mx-chain-go into adapt_notarization # Conflicts: # integrationTests/multiShard/endOfEpoch/startInEpoch/startInEpoch_test.go # integrationTests/multiShard/hardFork/hardFork_test.go # process/block/shardblock.go
…more with the new flow
process/block/metablock.go
Outdated
// check proofs for shard data | ||
for _, shardData := range header.ShardInfo { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think here we should do this only if equivalent messages flag is activated
process/block/metablock.go
Outdated
// if there is an entry for the missing proof, it means that proofsPool did not have it while scanning shardData | ||
// thus header epoch was not available at that time | ||
incompleteProof, hasMissingProof := mp.hdrsForCurrBlock.missingProofs[string(shardHeaderHash)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think here also
} | ||
|
||
if mp.hdrsForCurrBlock.missingHdrs == 0 { | ||
shouldRequestMissingFinalityAttestingShardHeaders := notarizedShardHdrsBasedOnProofs != len(metaBlock.ShardInfo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check this only if equivalent flag enabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not needed, here notarizedShardHdrsBasedOnProofs would be 0, thus the second condition will always be true
process/block/shardblock.go
Outdated
// check proofs for shard data | ||
for _, metaBlockHash := range header.GetMetaBlockHashes() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check if proofs activated?
} | ||
|
||
if sp.hdrsForCurrBlock.missingHdrs == 0 { | ||
shouldRequestMissingFinalityAttestingMetaHeaders := notarizedMetaHdrsBasedOnProofs != len(metaBlockHashes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add this only if flag activated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be ok too
@@ -361,44 +359,29 @@ func (sr *subroundBlock) createHeader() (data.HeaderHandler, error) { | |||
return hdr, nil | |||
} | |||
|
|||
func (sr *subroundBlock) isEpochChangeBlockForEquivalentMessagesActivation(header data.HeaderHandler) bool { | |||
isEquivalentMessagesFlagEnabledForHeader := sr.EnableEpochsHandler().IsFlagEnabledInEpoch(common.EquivalentMessagesFlag, header.GetEpoch()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we still need this if we have condition at line 365?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, deleted
process/block/baseProcess.go
Outdated
} | ||
|
||
func (bp *baseProcessor) isEpochChangeBlockForEquivalentMessagesActivation(header data.HeaderHandler) bool { | ||
isEquivalentMessagesFlagEnabledForHeader := bp.enableEpochsHandler.IsFlagEnabledInEpoch(common.EquivalentMessagesFlag, header.GetEpoch()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this condition seems to be redundant.
also can we extract this as a separate function and call it from consensus and base process as well?
it can get the enableEpochsHandler as argument then it doesn't need to be a method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about this.. applied the suggestion, extended the method to work for any given flag
process/block/baseProcess.go
Outdated
for _, headerInfo := range bp.hdrsForCurrBlock.hdrHashAndInfo { | ||
if headerInfo.usedInBlock != usedInBlock { | ||
for hdrHash, headerInfo := range bp.hdrsForCurrBlock.hdrHashAndInfo { | ||
isFlagEnabledForHeader := bp.enableEpochsHandler.IsFlagEnabledInEpoch(common.EquivalentMessagesFlag, headerInfo.hdr.GetEpoch()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can extract the code from L623-625 in a function and call it here and at line 646
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
applied the suggestion
process/track/blockProcessor.go
Outdated
@@ -300,6 +315,19 @@ func (bp *blockProcessor) checkHeaderFinality( | |||
return process.ErrNilBlockHeader | |||
} | |||
|
|||
if bp.enableEpochsHandler.IsFlagEnabledInEpoch(common.EquivalentMessagesFlag, header.GetEpoch()) { | |||
headerHash, err := core.CalculateHash(bp.marshaller, bp.hasher, header) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the hash can be passed, as it is available in ComputeLongestChain, so that we won't have to recompute here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also here we need to be careful about the first header in the epoch, as it is only final if it has a confirmation (for it the finality works by the old rules).
IsEpochChangeBlockForEquivalentMessagesActivation can be used for that, so the condition above could be changed to something like
equivalentMessagesActive := bp.enableEpochsHandler.IsFlagEnabledInEpoch(common.EquivalentMessagesFlag, header.GetEpoch())
if equivalentMessagesActive && !IsEpochChangeBlockForEquivalentMessagesActivation() {
....
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
applied both suggestions, indeed the hashes were available, sorted
process/track/blockProcessor.go
Outdated
@@ -234,7 +245,11 @@ func (bp *blockProcessor) ComputeLongestChain(shardID uint32, header data.Header | |||
go bp.requestHeadersIfNeeded(header, sortedHeaders, headers) | |||
}() | |||
|
|||
sortedHeaders, sortedHeadersHashes = bp.blockTracker.SortHeadersFromNonce(shardID, header.GetNonce()+1) | |||
startingNonce := header.GetNonce() + 1 | |||
if bp.enableEpochsHandler.IsFlagEnabledInEpoch(common.EquivalentMessagesFlag, header.GetEpoch()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need this condition here, as the header received as argument should be the last notarized header, not the header that needs to be processed next.
In this case both with previous and with the next version of consensus the starting nonce should be the same (next nonce after the nonce of the last notarized header)
Could you confirm this (I may remember this wrongly)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, the header is the last notarized header.. updated as suggested
process/block/metablock.go
Outdated
PubKeysBitmap: shardData.CurrentPubKeysBitmap, | ||
AggregatedSignature: shardData.CurrentSignature, | ||
HeaderHash: shardData.HeaderHash, | ||
HeaderEpoch: hdr.GetEpoch(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shardData.Epoch?
process/block/metablock.go
Outdated
} | ||
shardData.CurrentPubKeysBitmap = currentProof.GetPubKeysBitmap() | ||
shardData.CurrentSignature = currentProof.GetAggregatedSignature() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also set the epoch.
process/block/shardblock.go
Outdated
@@ -291,6 +293,15 @@ func (sp *shardProcessor) ProcessBlock( | |||
return process.ErrAccountStateDirty | |||
} | |||
|
|||
if sp.enableEpochsHandler.IsFlagEnabledInEpoch(common.EquivalentMessagesFlag, header.GetEpoch()) { | |||
// check proofs for shard data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is for referenced metablock proofs
process/block/shardblock.go
Outdated
log.Trace("no proof for meta header", | ||
"hash", logger.DisplayByteSlice(orderedMetaBlocksHashes[i]), | ||
) | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be break, continuing will iterate on the next metablock, which will break at line 1944 (the metablocks are ordered by nonce)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missed that.. updated
header.SetPreviousProof(proof) | ||
return true | ||
} | ||
log.Debug("addProofOnHeader: no proof found") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log also header hash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i did not realized that we need to calculate hash here, it might not help to add more processing only for logging
process/block/baseProcess.go
Outdated
@@ -2175,3 +2183,17 @@ func (bp *baseProcessor) checkSentSignaturesAtCommitTime(header data.HeaderHandl | |||
|
|||
return nil | |||
} | |||
|
|||
func (bp *baseProcessor) isFirstBlock(header data.HeaderHandler) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more specific naming? like isFirstBlockInEpoch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed it
process/block/metablock.go
Outdated
prevProof := shardHdr.GetPreviousProof() | ||
if prevProof != nil { | ||
shardData.PubKeysBitmap = prevProof.GetPubKeysBitmap() | ||
shardData.Signature = prevProof.GetAggregatedSignature() | ||
err := shardData.SetPreviousProof(prevProof) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we set here pubkeysbitmap and signature from previous proof?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because now the bitmap + signature from block are part of the proof of the previous block
hasProofForMetablock := false | ||
// attesting something | ||
if sp.hdrsForCurrBlock.missingHdrs == 0 { | ||
sp.hdrsForCurrBlock.missingFinalityAttestingHdrs = sp.requestMissingFinalityAttestingHeaders( | ||
core.MetachainShardId, | ||
sp.metaBlockFinality, | ||
) | ||
hasProofForMetablock = sp.hasProofForMetablock(metaBlockHash, metaBlock) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is might be cleaner to keep old code here and set hasProofForMetablock
to true initially, and then set it based on HasProof
if flag is enabled
@@ -364,41 +363,23 @@ func (sr *subroundBlock) createHeader() (data.HeaderHandler, error) { | |||
func (sr *subroundBlock) addProofOnHeader(header data.HeaderHandler) bool { | |||
prevBlockProof, err := sr.EquivalentProofsPool().GetProof(sr.ShardCoordinator().SelfId(), sr.GetData()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is correct.
sr.GetData would return the current block header hash, so this is not prevBlockProof but it is instead current block proof.
the previousBlockProof should be taken with key header.PrevHash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed.. found out during testing, sr.GetData() would be anyway empty here, as it is reset before, thus it was always returning that the proof does not exists
applied the suggestion
@@ -617,8 +619,8 @@ func (bp *baseProcessor) sortHeadersForCurrentBlockByNonce(usedInBlock bool) map | |||
hdrsForCurrentBlock := make(map[uint32][]data.HeaderHandler) | |||
|
|||
bp.hdrsForCurrBlock.mutHdrsForBlock.RLock() | |||
for _, headerInfo := range bp.hdrsForCurrBlock.hdrHashAndInfo { | |||
if headerInfo.usedInBlock != usedInBlock { | |||
for hdrHash, headerInfo := range bp.hdrsForCurrBlock.hdrHashAndInfo { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method sortHeadersForCurrentBlockByNonce is currently used in two contexts:
- when called with true (usedInBlock) it returns all the headers used in block, ordered by nonce - will be used to check the validity of all included headers in a block - checkShardHeadersValidity, checkMetaHeadersValidityAndFinality
- when called with false (usedInBlock) it returns all the headers not used explicitly in the block, but implicitly to check for finality in the context of confirmation blocks. - will be used to check the finality of all the included headers in a block - checkShardHeadersFinality, checkMetaHdrFinality
The function shouldSkipHeader mixes the fact that a header is required for the operation, with the existence of the proof.
I would suggest to keep the condition as before, and treat the lack of a proof separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated to keep the old check and return error
process/block/baseProcess.go
Outdated
@@ -666,6 +668,12 @@ func (bp *baseProcessor) sortHeaderHashesForCurrentBlockByNonce(usedInBlock bool | |||
return hdrsHashesForCurrentBlock | |||
} | |||
|
|||
func (bp *baseProcessor) shouldSkipHeader(headerInfo *hdrInfo, usedInBlock bool, hdrHash string) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check my previous comment, I would treat the missing proof separately.
The effect needs to be that if there is a missing proof for a header that is either notarized or used to decide the finality of a notarized header (only applies to the first and second block in the activation epoch), then the block needs to not be signed in consensus, or if this is the proposer perspective, the notarized header (that either is notarized but doesn't have a proof, or is given finality through confirmation by a block that doesn't have a proof) needs to be dropped.
process/block/metablock.go
Outdated
@@ -340,6 +341,18 @@ func (mp *metaProcessor) ProcessBlock( | |||
} | |||
} | |||
|
|||
if mp.enableEpochsHandler.IsFlagEnabledInEpoch(common.EquivalentMessagesFlag, header.Epoch) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you extract this in a method and call it here?
process/block/metablock.go
Outdated
log.Trace("could not add the constructed proof after header received", "hash", hex.EncodeToString(incompleteProof.HeaderHash)) | ||
} | ||
|
||
delete(mp.hdrsForCurrBlock.missingProofs, string(shardHeaderHash)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is correct, as the missing proof here is deleted even if it is not correct (errAddProof != nil). We still have a missing proof (we don't yet have a correct one)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed it
process/block/metablock.go
Outdated
@@ -1982,6 +2076,9 @@ func (mp *metaProcessor) computeExistingAndRequestMissingShardHeaders(metaBlock | |||
hdr: nil, | |||
usedInBlock: true, | |||
} | |||
|
|||
mp.hdrsForCurrBlock.missingProofs[string(shardData.HeaderHash)] = shardData.CurrentShardHeaderProof |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct?
maybe we can discuss it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed the concept of missing proofs
process/block/metablock.go
Outdated
hasProofForShardHdr := mp.proofsPool.HasProof(shardData.ShardID, shardData.HeaderHash) | ||
if !hasProofForShardHdr { | ||
// TODO: consider verifying the proof before adding it into the proofsPool | ||
errAddProof := mp.proofsPool.AddProof(shardData.CurrentShardHeaderProof) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the addProof doing also the proof verification?
Let's discuss this as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, it doesn't, thus the TODO.. indeed, let's discuss it and perhaps fix in a new PR
process/block/metablock.go
Outdated
shardData.PubKeysBitmap = proof.GetPubKeysBitmap() | ||
prevProof := shardHdr.GetPreviousProof() | ||
if prevProof != nil { | ||
shardData.PubKeysBitmap = prevProof.GetPubKeysBitmap() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I understand this part.
We have now separate fields for current proof and previous proof, we wouldn't need to set the shardData.PubKeysBitmap and shardData.Signature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed, updated
process/block/metablock.go
Outdated
@@ -1832,6 +1850,11 @@ func (mp *metaProcessor) checkShardHeadersValidity(metaHdr *block.MetaBlock) (ma | |||
return nil, process.ErrDeveloperFeesDoNotMatch | |||
} | |||
|
|||
err = mp.verifyShardDataProofs(shardData) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will be expensive verify here, we need to see if we can move it to the interceptor instead, so that if we accept the metablock, these checks are already done.
I think the proof checks could also be done on go routines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved
return fmt.Errorf("%w for previous block", err) | ||
} | ||
|
||
err = checkProof(shardData.GetCurrentProof()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to remove the current proof from the sharddata
the proof is anyway checked when making sure the headers that generated the shard data are available and have proofs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
assert.Nil(t, err) | ||
} | ||
|
||
func TestCheckMetaShardInfo_OkValsShouldWorkMultipleShardData(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the name is not good as it tests for errors
process/block/metablock.go
Outdated
@@ -1800,6 +1865,24 @@ func (mp *metaProcessor) checkShardHeadersValidity(metaHdr *block.MetaBlock) (ma | |||
return highestNonceHdrs, nil | |||
} | |||
|
|||
func (mp *metaProcessor) verifyProof(proof data.HeaderProofHandler) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be a function, no need for the receiver
Reasoning behind the pull request
Proposed changes
Testing procedure
Pre-requisites
Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:
feat
branch created?feat
branch merging, do all satellite projects have a proper tag insidego.mod
?