Skip to content
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

Added Critical Quality level #2277

Merged
merged 2 commits into from
Oct 2, 2019

Conversation

sisuresh
Copy link
Contributor

@sisuresh sisuresh commented Sep 17, 2019

Description

Resolves #2247

Added a Critical Quality level that will require 100% of the groups at that level to come to consensus.

Checklist

  • Reviewed the contributing document
  • Rebased on top of master (no merge commits)
  • Ran clang-format v5.0.0 (via make format or the Visual Studio extension)
  • Compiles
  • Ran all tests
  • If change impacts performance, include supporting evidence per the performance document

Copy link
Contributor

@MonsieurNicolas MonsieurNicolas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few updates needed, mostly because you forgot to make some changes (would have found them by updating the test validator config file)

@@ -40,38 +40,6 @@ static const std::unordered_set<std::string> TESTING_ONLY_OPTIONS = {
static const std::unordered_set<std::string> TESTING_SUGGESTED_OPTIONS = {
"ALLOW_LOCALHOST_FOR_TESTING"};

namespace
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

avoid moving code if you don't have to

@@ -172,25 +172,30 @@ TEST_CASE("sane quorum set", "[scp][quorumset]")
check(qSet, true, qSelfSet);
}

SECTION("{ t: 1, v0, { t: 1, v1, { t: 1, v2, { t: 1, v3 , { t: 1, v4} } } "
SECTION("{ t: 1, v0, { t: 1, v1, { t: 1, v2, { t: 1, v3 , { t: 1, v4, { t: "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

forgot to update config test "load validators config"

@@ -32,8 +32,17 @@ class Config : public std::enable_shared_from_this<Config>
{
VALIDATOR_LOW_QUALITY = 0,
VALIDATOR_MED_QUALITY = 1,
VALIDATOR_HIGH_QUALITY = 2
VALIDATOR_HIGH_QUALITY = 2,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

forgot to update documentation in docs/stellar-core_example.cfg (where QUALITY is mentioned)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

forgot to update kQualities (so CRITICAL cannot be parsed at all)

@@ -1411,7 +1384,9 @@ Config::generateQuorumSetHelper(
"High quality validator {} must have redundancy of at least 3",
it->mName));
}
innerSet.threshold = computeDefaultThreshold(innerSet, true);
innerSet.threshold = computeDefaultThreshold(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CRITICAL is better than HIGH, so we should require at least 3 validators at that quality level

src/scp/QuorumSetUtils.cpp Show resolved Hide resolved
Copy link
Contributor

@MonsieurNicolas MonsieurNicolas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor update needed, otherwise looks good

"} } -> too deep")
{
auto qSet = makeSingleton(keys[0]);
auto qSet1 = makeSingleton(keys[1]);
auto qSet2 = makeSingleton(keys[2]);
auto qSet3 = makeSingleton(keys[3]);
auto qSet4 = makeSingleton(keys[4]);
auto qSet5 = makeSingleton(keys[5]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test update belongs to the other commit

@MonsieurNicolas
Copy link
Contributor

r+ 24b0bab

latobarita added a commit that referenced this pull request Oct 2, 2019
Added Critical Quality level

Reviewed-by: MonsieurNicolas
@latobarita latobarita merged commit 24b0bab into stellar:master Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for "CRITICAL" quality validators
3 participants