[BFT] Extend CombinedVoteProcessorV3 #7918
Conversation
…/6127-vote-double-counting
…or. Wrote a detailed explanation and reasoning behind this logic
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
| err := s.collector.ProcessBlock(proposal) | ||
| require.NoError(s.T(), err) | ||
|
|
||
| time.Sleep(100 * time.Millisecond) |
There was a problem hiding this comment.
Can we stop the worker pool here instead of sleeping?
s.workerPool.StopWait()
| onQCCreated := func(qc *flow.QuorumCertificate) { | ||
| require.Fail(t, "qc is not expected to be created in this test scenario") | ||
| } |
There was a problem hiding this comment.
Adding 2f-1 valid votes to the vote processor, then adding the 2 duplicate votes, would make this a stronger check. Currently we're adding 2 votes (only one of which is valid), but the threshold for QC creation is 3, so we wouldn't expect a QC even if the duplicate vote was accepted.
If we added 1 valid vote from some other node, then 2 duplicate votes from the leader, then it is more likely that a faulty QC creation process would incorrectly create an invalid QC.
…/6127-vote-double-counting
…otes equivocation
…/6127-vote-double-counting
Co-authored-by: Alexander Hentschel <alex.hentschel@flowfoundation.org> Co-authored-by: Jordan Schalm <jordan.schalm@flowfoundation.org>
…onflow/flow-go into yurii/6127-vote-double-counting
…onflow/flow-go into alex/6127-vote-double-counting_-_suggestions
…_suggestions Suggested amendments for PR 7918
| // It gets more interesting when any of the operations fail after we add the vote to the cache. The way this logic is structured | ||
| // it results in the following rule: _only the very first vote that was added to the votesCache from the same signer will be processed by the component_. | ||
| // It means that if the vote was invalid by any reason from the point of view of the aggregator the second vote won't be processed at all | ||
| // even if it might be correct from the pointer of view of the aggregator. |
There was a problem hiding this comment.
| // even if it might be correct from the pointer of view of the aggregator. | |
| // even if it might be correct from the point of view of the aggregator. |
AlexHentschel
left a comment
AlexHentschel
left a comment
There was a problem hiding this comment.
Very nice. This is so much of an improvement in clarity -- really think that is worth the time investment in the long run. Only a small amount of non-critical suggestions.
Co-authored-by: Jordan Schalm <jordan.schalm@flowfoundation.org>
AlexHentschel
left a comment
AlexHentschel
left a comment
There was a problem hiding this comment.
Thanks for removing the mutex. I think CAS must be used in caching2Verifying for correctness (unless we are relying on guarantees outside of that method, which I did not check). For consistency and clarity, I would suggest to also implement the CAS Loop pattern in terminateVoteProcessing.
Suggested documented, drop-in code code replacements in my comments.
Co-authored-by: Alexander Hentschel <alex.hentschel@flowfoundation.org>
Co-authored-by: Alexander Hentschel <alex.hentschel@flowfoundation.org>
4 participants
https://github.com/onflow/flow-go-internal/issues/6127
Context
This PR introduces a vote cache for
CombinedVoteProcessorV3. PreviouslyCombinedVoteProcessorV3couldn't guarantee correctness of the QC it provides under some scenarios when leader is equivocating. By using a separate vote cache for the processor we can guarantee that each vote will be included at most once per signer which inherently guarantees that leader will be able to construct a valid QC once he collects enough votes to form a super-majority.The runtime overhead of introducing votesCache is marginal: For votesCache to add 500 votes (concurrently with 20 threads) takes about 0.25ms. This runtime overhead is negligible and a good trade-off for the gain in maintainability and code clarity.