added checks on proofs parameters like epoch, nonce and shard #6698
Conversation
sstanculeanu
added
the
ignore-for-release-notes
process/block/baseProcess.go
Outdated
| return fmt.Errorf("%w while getting header for proof hash %s", err, hex.EncodeToString(prevProof.GetHeaderHash())) | ||
| } | ||
|
|
||
| if prevProof.GetHeaderNonce() != prevHeader.GetNonce() { |
There was a problem hiding this comment.
we have now also round in header proof, so we can also add a check for round
| return fmt.Errorf("%w while getting header for proof hash %s", err, hex.EncodeToString(iep.proof.GetHeaderHash())) | ||
| } | ||
|
|
||
| if iep.proof.GetHeaderNonce() != header.GetNonce() { |
process/block/baseProcess.go
Outdated
| return bp.checkPrevProofValidity(headerHandler) | ||
| } | ||
|
|
||
| func (bp *baseProcessor) checkPrevProofValidity(headerHandler data.HeaderHandler) error { |
There was a problem hiding this comment.
I think the checks can be done against the currentBlockHeader fields which is already loaded, like in the checkBlockValidity method.
currentBlockHeader can be passed as prevHeader
There was a problem hiding this comment.
also, do we have this kind of check as well for the proofs in the shard data included in a metablock?
e.g the prevProof from a shardData needs to be verified that it has the correct data (epoch, round, nonce) compared to the prevBlock of that shardData
There was a problem hiding this comment.
extended the checks over the prevProof as suggested
process/block/baseProcess.go
Outdated
| } | ||
|
|
||
| func (bp *baseProcessor) checkPrevProofValidity(headerHandler data.HeaderHandler) error { | ||
| if !bp.enableEpochsHandler.IsFlagEnabledInEpoch(common.EquivalentMessagesFlag, headerHandler.GetEpoch()) { |
There was a problem hiding this comment.
can you check with this instead:
if !common.ShouldBlockHavePrevProof(headerHandler, enableEpochsHandler, common.EquivalentMessagesFlag) {
return nil
}the above check takes into consideration also the first block which should not have a previous proof.
process/block/metablock.go
Outdated
| if !mp.proofsPool.HasProof(shardData.ShardID, shardData.HeaderHash) { | ||
| return fmt.Errorf("%w for header hash %s", process.ErrMissingHeaderProof, hex.EncodeToString(shardData.HeaderHash)) | ||
| } | ||
|
|
||
| prevProof := shardData.GetPreviousProof() | ||
| headersPool := mp.dataPool.Headers() | ||
| prevHeader, err := headersPool.GetHeaderByHash(prevProof.GetHeaderHash()) |
There was a problem hiding this comment.
here we check if there is shardData.HeaderHash proof in pool, and then we check proof fields on previousProof, is this ok?
There was a problem hiding this comment.
yes, the proof from pool should have already been validated
| @@ -430,6 +430,18 @@ func (mp *metaProcessor) checkProofsForShardData(header *block.MetaBlock) error | |||
| if !mp.proofsPool.HasProof(shardData.ShardID, shardData.HeaderHash) { | |||
| return fmt.Errorf("%w for header hash %s", process.ErrMissingHeaderProof, hex.EncodeToString(shardData.HeaderHash)) | |||
| } | |||
|
|
|||
| prevProof := shardData.GetPreviousProof() | |||
| headersPool := mp.dataPool.Headers() | |||
There was a problem hiding this comment.
maybe here we need to take the header from a different place (storer cache), if the header was already committed I think it should have been removed from the headers pool.
There was a problem hiding this comment.
added new method
| @@ -24,3 +27,25 @@ func IsFlagEnabledAfterEpochsStartBlock(header data.HeaderHandler, enableEpochsH | |||
| func ShouldBlockHavePrevProof(header data.HeaderHandler, enableEpochsHandler EnableEpochsHandler, flag core.EnableEpochFlag) bool { | |||
| return IsFlagEnabledAfterEpochsStartBlock(header, enableEpochsHandler, flag) && header.GetNonce() > 1 | |||
| } | |||
|
|
|||
| // VerifyProofAgainstHeader verifies the fields on the proof match the ones on the header | |||
| func VerifyProofAgainstHeader(proof data.HeaderProofHandler, header data.HeaderHandler) error { | |||
| } | ||
|
|
||
| return nil | ||
| return common.VerifyProofAgainstHeader(iep.proof, header) |
There was a problem hiding this comment.
I think we need to add a method (it there is none already) to get the header by hash from pool and if not found from storer (cache and if not found from storage)
sstanculeanu
merged commit 5495467
into
equivalent-proofs-feat-stabilization
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:
featbranch created?featbranch merging, do all satellite projects have a proper tag insidego.mod?