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
Alex/5857 full fix #1325
Alex/5857 full fix #1325
Conversation
…not add necessary gap
…posed to top-level test
code still needs consolidation
Codecov Report
@@ Coverage Diff @@
## master #1325 +/- ##
=======================================
Coverage 56.23% 56.24%
=======================================
Files 500 500
Lines 31072 31079 +7
=======================================
+ Hits 17474 17479 +5
- Misses 11232 11234 +2
Partials 2366 2366
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
One thing that's confusing to me is that this indicates "final", mentions it only implements step 1 of https://github.com/dapperlabs/flow-go/issues/5857, yet:
- seems to tick all boxes for step 2 of 5857,
- contains a "TEMPORARY" all-caps comment.
I feel there's a way to make this PR more kind to a future eng exploring the status of the remediation we're putting in, here.
Otherwise, the code looks good!
This implements step 1 of https://github.com/dapperlabs/flow-go/issues/5857:
simple fix for the business logic
consensus.Builder
to always include a gapsealValidator
to enforce the gapContext
This PR is an extension of #1307. In this PR, we also update the seal-validation logic to reject seals that do not have a gap.