[BFT] Reporting basic consensus protocol violations #4174
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…d structuring of consensus related consumers. Fixed tests and dependant code
…ages. Integrated BaseProtocolViolationConsumer in compliance engine of consensus follower
…thub.com/onflow/flow-go into yurii/4125-reporting-protocol-violations
Codecov Report
@@ Coverage Diff @@
## master #4174 +/- ##
==========================================
- Coverage 53.87% 53.84% -0.03%
==========================================
Files 869 869
Lines 80465 80453 -12
==========================================
- Hits 43349 43321 -28
- Misses 33704 33723 +19
+ Partials 3412 3409 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
durkmurder
requested review from
peterargue,
ramtinms and
yhassanzadeh13
as code owners
jordanschalm
approved these changes
Apr 10, 2023
Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>
…/4125-reporting-protocol-violations
… specific events. Updated relevant code
…w-go into alex/4174-suggestions
Suggestions for PR 4174
…g of InvalidProposalError
…/4125-reporting-protocol-violations
| state protocol.FollowerState, | ||
| follower module.HotStuffFollower, | ||
| validator hotstuff.Validator, | ||
| sync module.BlockRequester, | ||
| tracer module.Tracer, | ||
| ) (*ComplianceCore, error) { | ||
| onEquivocation := func(block, otherBlock *flow.Block) { | ||
| finalizationConsumer.OnDoubleProposeDetected(model.BlockFromFlow(block.Header), model.BlockFromFlow(otherBlock.Header)) | ||
| followerConsumer.OnDoubleProposeDetected(model.BlockFromFlow(block.Header), model.BlockFromFlow(otherBlock.Header)) |
There was a problem hiding this comment.
Thank you, I would appreciate the revision 🙇
The cache here is already very specific to the follower. We can always generalize it should want to use the cache somewhere else ... honestly though, I think the chances of us using the cache elsewhere is reasonably low in my opinion. Therefore, I think specificity to improve the code's clarity is in my opinion the way to go.
AlexHentschel
approved these changes
May 12, 2023
There was a problem hiding this comment.
This is an amazing refactoring. It improves modularity and code clarity in sooo many ways. Thank you for your hard work with multiple iterations.
I think it woudl be great to include the two suggestions above still in this PR, as they are tiny changes:
- rename
NoopPartialConsumeras proposed in my comment above - extend the declaration of interfaces implemented by the
SlashingViolationsConsumeras proposed in my comment above
Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co>
…/4125-reporting-protocol-violations
|
bors merge |
bors bot
added a commit
that referenced
this pull request
Jun 19, 2023
4350: [BFT] Reporting compliance engine protocol violations r=durkmurder a=durkmurder ### Context This is a follow-up PR which addresses outstanding comments in [previous](#4174) PR. In compliance engine I have removed usage of `engine.InvalidInputError` since I wanted to consolidate slashing evidence reporting in one place(which works with `model.InvalidProposalError`) and `engine.InvalidInputError` was just adding another level of wrapping without real benefit. I've decided to replace it with `model.InvalidProposalError` which simplified error handling. Reference comments: [1](#4174 (comment)), [2](#4174 (comment)), [3](#4174 (comment)) Co-authored-by: Yurii Oleksyshyn <yuraolex@gmail.com> Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co>
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
In order to address reporting of protocol violations this PR refactors existing code and introduces new interface for reporting basic consensus protocol violations that can and have to be detected by all node types.
As part of this PR:
FollowerConsumerwhich combines separate interfaces usable by consensus follower(FinalizationConsumerandBaseProtocolViolationConsumer).