-
Notifications
You must be signed in to change notification settings - Fork 23
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
Make isQuorumSetSane() more informative #549
Conversation
Probably need to use |
That would upset downstream. :> |
{ | ||
if (reason != nullptr) | ||
{ | ||
const char* msg = "Number of validator nodes exceeds the limit [1..1000]"; |
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.
Technically it's out of bounds, as it can be 0
.
You could also split it up into "Quorum set has no validator" and "Quorum set has over 1000 validators".
: mExtraChecks{extraChecks} | ||
{ | ||
mIsSane = checkSanity(qSet, 0) && mCount >= 1 && mCount <= 1000; | ||
mIsSane = checkSanity(qSet, 0, reason); |
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.
You can simplify the code by making the reason
optional on the API, but required in the functions (IOW provides a dummy variable if reason
is null so you avoid bounds check).
Can we already use |
I think we can, as long as we don't copy it and just read it. |
b74a4cf
to
fe64a8b
Compare
Codecov Report
@@ Coverage Diff @@
## v0.x.x #549 +/- ##
==========================================
+ Coverage 89.14% 89.21% +0.07%
==========================================
Files 59 59
Lines 4285 4276 -9
==========================================
- Hits 3820 3815 -5
+ Misses 465 461 -4
Continue to review full report at Codecov.
|
Fixed to avoid bounds / null checking. I'm using
|
mIsSane = checkSanity(qSet, 0, reason); | ||
if (mCount < 1) | ||
{ | ||
const char* msg = "Number of validator nodes is zero"; |
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.
Am I missing something w.r.t. the const char* msg = ...; *reason = msg
pattern ? Wouldn't *reason = ...
be more straightforward ?
Also assert that reason
and mIsSane
are consistent at the end of this constructor.
Sad! |
@@ -247,10 +247,17 @@ bool | |||
BallotProtocol::isStatementSane(SCPStatement const& st, bool self) | |||
{ | |||
auto qSet = mSlot.getQuorumSetFromStatement(st); | |||
bool res = qSet != nullptr && isQuorumSetSane(*qSet, false); | |||
const char* reason; |
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.
bad bad bad! no T.init
in C++.
fe64a8b
to
523834a
Compare
This enables better debugging / UX when a quorum set is misconfigured. Part of bosagora#515
523834a
to
10620c3
Compare
Updated. |
} | ||
|
||
// only one of the two may be true | ||
assert(mIsSane ^ (*reason != nullptr)); |
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.
Attaboy
If this looks good to you, I will also submit a PR upstream.
However in upstream they already use a different set of rules for
isQuorumSetSane
, so we would have to update our bindings to the latest version first. And that's not trivial as of now.Part of #515