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

Integrate proof check on sync #6501

Merged
merged 36 commits into from
Dec 18, 2024

Conversation

ssd04
Copy link
Contributor

@ssd04 ssd04 commented Sep 25, 2024

Reasoning behind the pull request

  • Wait for confirmation block in epochstart bootstrap
  • integrate equivalent proofs into process sync
  • use real proofs pool component in interceptors

Testing procedure

  • Will be tested with the feat branch

Pre-requisites

Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:

  • was the PR targeted to the correct branch?
  • if this is a larger feature that probably needs more than one PR, is there a feat branch created?
  • if this is a feat branch merging, do all satellite projects have a proper tag inside go.mod?

@ssd04 ssd04 self-assigned this Sep 25, 2024
@ssd04 ssd04 marked this pull request as ready for review October 7, 2024 06:34
@sstanculeanu sstanculeanu self-requested a review November 12, 2024 09:54
@AdoAdoAdo AdoAdoAdo self-requested a review December 16, 2024 08:48
@@ -90,6 +91,10 @@ func createShardDataPools() dataRetriever.PoolsHolder {
pools.CurrBlockTxsCalled = func() dataRetriever.TransactionCacher {
return &mock.TxForCurrentBlockStub{}
}
pools.ProofsCalled = func() dataRetriever.ProofsPool {
return &dataRetrieverMock.ProofsPoolMock{}
Copy link
Collaborator

Choose a reason for hiding this comment

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

the mocked component should return by default this mocked proofs pool, so this would be avoided

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"called" fields are not set in this mocked component, so we have to set them here

@@ -68,6 +74,12 @@ func (hip *HdrInterceptorProcessor) Save(data process.InterceptedData, _ core.Pe

hip.headers.AddHeader(interceptedHdr.Hash(), interceptedHdr.HeaderHandler())

// TODO: check for equivalent flag
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added flag check


log.Trace("baseBootstrap.handleEquivalentProof: did not have proof for header, will try again", "headerHash", headerHash)

// TODO: evaluate adding a wait here, or request for next header if missing
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure this would work as expected, to be seen in tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed TODO for now, requesting for next header is already implemented

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 integration test is working in this case, will see with more testing if there are other edge cases here

sstanculeanu
sstanculeanu previously approved these changes Dec 17, 2024
dataRetriever/dataPool/proofsCache/proofsPool.go Outdated Show resolved Hide resolved
@@ -293,7 +294,7 @@ func (hsv *HeaderSigVerifier) VerifyPreviousBlockProof(header data.HeaderHandler
return fmt.Errorf("%w, received header without leader signature after flag activation", process.ErrInvalidHeader)
}

return nil
return hsv.VerifyHeaderProof(previousProof)
Copy link
Contributor

Choose a reason for hiding this comment

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

I did some refactoring in this area, we should check after merge.

process/sync/metablock_test.go Outdated Show resolved Hide resolved
process/sync/metablock_test.go Outdated Show resolved Hide resolved
process/sync/metablock_test.go Outdated Show resolved Hide resolved
process/sync/baseSync.go Show resolved Hide resolved
epochStart/bootstrap/epochStartMetaBlockProcessor.go Outdated Show resolved Hide resolved
epochStart/bootstrap/epochStartMetaBlockProcessor.go Outdated Show resolved Hide resolved
epochStart/bootstrap/epochStartMetaBlockProcessor.go Outdated Show resolved Hide resolved
epochStart/bootstrap/epochStartMetaBlockProcessor.go Outdated Show resolved Hide resolved
Copy link

codecov bot commented Dec 17, 2024

Codecov Report

Attention: Patch coverage is 75.39267% with 47 lines in your changes missing coverage. Please review.

Project coverage is 78.70%. Comparing base (d663d3c) to head (555ea3d).
Report is 9 commits behind head on feat/equivalent-messages.

Files with missing lines Patch % Lines
process/sync/baseSync.go 53.48% 15 Missing and 5 partials ⚠️
...ochStart/bootstrap/epochStartMetaBlockProcessor.go 74.32% 13 Missing and 6 partials ⚠️
epochStart/bootstrap/common.go 0.00% 1 Missing and 1 partial ⚠️
.../interceptors/processor/hdrInterceptorProcessor.go 84.61% 1 Missing and 1 partial ⚠️
process/sync/metablock.go 80.00% 2 Missing ⚠️
process/sync/shardblock.go 80.00% 2 Missing ⚠️
Additional details and impacted files
@@                     Coverage Diff                      @@
##           feat/equivalent-messages    #6501      +/-   ##
============================================================
- Coverage                     78.74%   78.70%   -0.04%     
============================================================
  Files                           769      769              
  Lines                        101810   102005     +195     
============================================================
+ Hits                          80167    80284     +117     
- Misses                        16215    16271      +56     
- Partials                       5428     5450      +22     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sstanculeanu
sstanculeanu previously approved these changes Dec 17, 2024
AdoAdoAdo
AdoAdoAdo previously approved these changes Dec 17, 2024
@sstanculeanu sstanculeanu merged commit 7b02128 into feat/equivalent-messages Dec 18, 2024
5 checks passed
@sstanculeanu sstanculeanu deleted the integrate-proof-check-on-sync branch December 18, 2024 09:54
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.

3 participants