-
Notifications
You must be signed in to change notification settings - Fork 961
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
Bug 2267 quorum intersection pick any scc with a quorum #2279
Bug 2267 quorum intersection pick any scc with a quorum #2279
Conversation
@fixxxedpoint is this more along the correct line of behaviour? |
@@ -674,18 +674,19 @@ void | |||
QuorumIntersectionCheckerImpl::buildSCCs() | |||
{ | |||
mTSC.calculateSCCs(); | |||
mMaxSCC.clear(); | |||
mScanSCC.clear(); | |||
for (auto const& scc : mTSC.mSCCs) |
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.
I would just remove that loop together with mScanSCC class variable and "merge" it with that main loop in networkEnjoysQuorumIntersection(). Name suggests it is only responsible for building SCCs and the whole thing will be also more easier to understand (as we talked before, you don't need to search for that greatest element). It is not a big deal but current version also iterates all SCCs twice.
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.
@fixxxedpoint Well, mScanSCC is used in a couple other helper methods throughout the implementation, I can't easily eliminate it entirely without turning it into a parameter I have to pass everywhere; but I can merge the setup loops as you suggest. Will do so. Otherwise I just wanted to double check: style issues aside, this at least looks correct to you?
Thanks again for all the input and hand-holding; I just want to make sure I'm faithfully transplanting your work here :)
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.
(Update: actually making the variable local / sinking it into a member into the helper class was relatively low-impact; patch updated with the results!)
2a8810e
to
c2d9b9e
Compare
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.
This looks correct and good to me! 👍
r+ c2d9b9e |
…ck-any-scc-with-a-quorum Bug 2267 quorum intersection pick any scc with a quorum Reviewed-by: MonsieurNicolas
Description
Resolves #2267 in a different way than #2278. Simpler: just pick the first quorum-containing SCC we find, and fail if there's more than one (as we were already doing). Avoids artificially picking an SCC with no quorums that happens to just be "big".
Checklist
clang-format
v5.0.0 (viamake format
or the Visual Studio extension)