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.

Sign up for GitHub

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

Jump to bottom

Bug 2267 quorum intersection max scc topological order #2278

Closed
wants to merge 5 commits into from
Closed

Bug 2267 quorum intersection max scc topological order #2278

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2addb66
herder: move no-quorum SCC check earlier in intersection checker.
graydon Sep 18, 2019
f0e476a
bitset: add explicit operator !=
graydon Sep 18, 2019
55a2a6b
herder: move quorum intersection checker impl to non-empty namespace …
graydon Sep 18, 2019
dd8f123
herder: fix "SCP" DEBUG logging in tests
graydon Sep 18, 2019
2fa5b4d
bug 2267: order SCCs by reachability in quorum intersection checker
graydon Sep 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
132 changes: 95 additions & 37 deletions src/herder/QuorumIntersectionCheckerImpl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,17 @@
#include "util/Logging.h"
#include "util/Math.h"

namespace
namespace stellar
{
std::shared_ptr<QuorumIntersectionChecker>
QuorumIntersectionChecker::create(QuorumTracker::QuorumMap const& qmap,
Config const& cfg, bool quiet)
{
using namespace quorum_intersection;
return std::make_shared<QuorumIntersectionCheckerImpl>(qmap, cfg, quiet);
}

namespace quorum_intersection
{

////////////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -670,14 +680,69 @@ QuorumIntersectionCheckerImpl::buildGraph(QuorumTracker::QuorumMap const& qmap)
mStats.mTotalNodes = mPubKeyBitNums.size();
}

SCCReachabilityCalculator::SCCReachabilityCalculator(QGraph const& graph)
: mNodeGraph(graph)
{
}

void
SCCReachabilityCalculator::calculateReachability()
{
mReachableNodes.clear();
mReachableNodes.resize(mNodeGraph.size());
bool changed;
do
{
changed = false;
for (size_t i = 0; i < mNodeGraph.size(); ++i)
{
auto& r = mReachableNodes.at(i);
auto const& q = mNodeGraph.at(i);
if (r.empty())
{
r = q.mAllSuccessors;
changed = true;
}
else
{
BitSet b = r;
for (size_t j = 0; r.nextSet(j); ++j)
{
b |= mReachableNodes.at(j);
}
if (r != b)
{
changed = true;
r = b;
}
}
}
} while (changed);
}

bool
SCCReachabilityCalculator::canReach(BitSet const& a, BitSet const& b) const
{
for (size_t i = 0; a.nextSet(i); ++i)
{
if (mReachableNodes.at(i).intersectionCount(b) != 0)
{
return true;
}
}
return false;
}

void
QuorumIntersectionCheckerImpl::buildSCCs()
{
SCCReachabilityCalculator reach(mGraph);
reach.calculateReachability();
mTSC.calculateSCCs();
mMaxSCC.clear();
for (auto const& scc : mTSC.mSCCs)
{
if (scc.count() > mMaxSCC.count())
if (mMaxSCC.empty() || reach.canReach(mMaxSCC, scc))
Copy link

Choose a reason for hiding this comment

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

Quick note: doesn't Tarjan give you the SCCs in the right (topological) order? So you could simply set maxSCC to the last component found (or actually first, I do not remember the order) instead of iterating here and computing the reachability?

{
mMaxSCC = scc;
}
Expand All @@ -697,15 +762,38 @@ QuorumIntersectionCheckerImpl::nodeName(size_t node) const
bool
QuorumIntersectionCheckerImpl::networkEnjoysQuorumIntersection() const
{
// First stage: check the graph-level SCCs for disjoint quorums,
// and filter out nodes that aren't in the main SCC.
bool foundDisjoint = false;
size_t nNodes = mPubKeyBitNums.size();
if (!mQuiet)
{
CLOG(INFO, "SCP") << "Calculating " << nNodes
<< "-node network quorum intersection";
}

for (size_t i = 0; mMaxSCC.nextSet(i); ++i)
{
CLOG(DEBUG, "SCP") << "Main SCC node: " << nodeName(i);
}

if (auto q = contractToMaximalQuorum(mMaxSCC))
{
CLOG(DEBUG, "SCP") << "Maximal main SCC quorum: " << q;
}
else
{
// We vacuously "enjoy quorum intersection" if there are no quorums,
// though this is probably enough of a potential problem itself that
// it's worth warning about.
if (!mQuiet)
{
CLOG(WARNING, "SCP") << "No quorum found in main SCC "
"(possible network halt)";
}
return true;
}

// First stage: check the graph-level SCCs for disjoint quorums,
// and filter out nodes that aren't in the main SCC.
bool foundDisjoint = false;
for (auto const& scc : mTSC.mSCCs)
{
if (scc == mMaxSCC)
Expand All @@ -730,28 +818,6 @@ QuorumIntersectionCheckerImpl::networkEnjoysQuorumIntersection() const
}
}
}
for (size_t i = 0; mMaxSCC.nextSet(i); ++i)
{
CLOG(DEBUG, "SCP") << "Main SCC node: " << nodeName(i);
}

auto q = contractToMaximalQuorum(mMaxSCC);
if (q)
{
CLOG(DEBUG, "SCP") << "Maximal main SCC quorum: " << q;
}
else
{
// We vacuously "enjoy quorum intersection" if there are no quorums,
// though this is probably enough of a potential problem itself that
// it's worth warning about.
if (!mQuiet)
{
CLOG(WARNING, "SCP") << "No quorum found in transitive closure "
"(possible network halt)";
}
return true;
}

// Second stage: scan the main SCC powerset, potentially expensive.
if (!foundDisjoint)
Expand Down Expand Up @@ -833,16 +899,7 @@ groupString(Config const& cfg, std::set<PublicKey> const& group)
out << ']';
return out.str();
}
}

namespace stellar
{
std::shared_ptr<QuorumIntersectionChecker>
QuorumIntersectionChecker::create(QuorumTracker::QuorumMap const& qmap,
Config const& cfg, bool quiet)
{
return std::make_shared<QuorumIntersectionCheckerImpl>(qmap, cfg, quiet);
}
} // end namespace quorum_intersection

std::set<std::set<PublicKey>>
QuorumIntersectionChecker::getIntersectionCriticalGroups(
Expand Down Expand Up @@ -873,6 +930,7 @@ QuorumIntersectionChecker::getIntersectionCriticalGroups(
// that asks, but still counts as an intersecting member of any two quorums
// it's a member of.

using namespace quorum_intersection;
std::set<std::set<PublicKey>> candidates;
std::set<std::set<PublicKey>> critical;
QuorumTracker::QuorumMap test_qmap(qmap);
Expand Down
29 changes: 27 additions & 2 deletions src/herder/QuorumIntersectionCheckerImpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,9 @@
#include "xdr/Stellar-SCP.h"
#include "xdr/Stellar-types.h"

namespace
namespace stellar
{
namespace quorum_intersection
{

struct QBitSet;
Expand All @@ -374,7 +376,8 @@ struct QBitSet
const QGraph mInnerSets;

// Union of mNodes and i.mAllSuccessors for i in mInnerSets: summarizes
// every node that this QBitSet directly depends on.
// every node that this QBitSet directly depends on. Eagerly calculated.
// on construction.
const BitSet mAllSuccessors;

QBitSet(uint32_t threshold, BitSet const& nodes, QGraph const& innerSets);
Expand Down Expand Up @@ -411,6 +414,16 @@ struct TarjanSCCCalculator
void scc(size_t i);
};

struct SCCReachabilityCalculator
{
std::vector<BitSet> mReachableNodes;
QGraph const& mNodeGraph;

SCCReachabilityCalculator(QGraph const& graph);
void calculateReachability();
bool canReach(BitSet const& a, BitSet const& b) const;
};

// A MinQuorumEnumerator is responsible to scanning the powerset of the SCC
// we're considering, in a recursive bottom-up order, with a lot of early exits
// described above. Each instance of MinQuorumEnumerator represents one call in
Expand Down Expand Up @@ -529,8 +542,20 @@ class QuorumIntersectionCheckerImpl : public stellar::QuorumIntersectionChecker
bool quiet = false);
bool networkEnjoysQuorumIntersection() const override;

QGraph const&
getGraph() const
{
return mGraph;
}
size_t
getPubkeyBitNum(stellar::PublicKey const& b)
{
return mPubKeyBitNums.at(b);
}

std::pair<std::vector<stellar::PublicKey>, std::vector<stellar::PublicKey>>
getPotentialSplit() const override;
size_t getMaxQuorumsFound() const override;
};
}
}
102 changes: 99 additions & 3 deletions src/herder/test/QuorumIntersectionTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include "crypto/Hex.h"
#include "crypto/SHA.h"
#include "herder/QuorumIntersectionChecker.h"
#include "herder/QuorumIntersectionCheckerImpl.h"
#include "lib/catch.hpp"
#include "main/Config.h"
#include "scp/LocalNode.h"
Expand Down Expand Up @@ -291,7 +292,7 @@ interconnectOrgs(xdr::xvector<xdr::xvector<PublicKey>> const& orgs,
}
if (shouldDepend(i, j))
{
CLOG(DEBUG, "SCP") << "dep: org#" << i << " => org#" << j;
CLOG(DEBUG, "Herder") << "dep: org#" << i << " => org#" << j;
auto& otherOrg = orgs.at(j);
auto thresh = roundUpPct(otherOrg.size(), innerThreshPct);
depOrgs.emplace_back(thresh, otherOrg, emptySet);
Expand Down Expand Up @@ -764,12 +765,12 @@ debugQmap(Config const& cfg, QuorumTracker::QuorumMap const& qm)
LocalNode::toJson(*pair.second, [&cfg](PublicKey const& k) {
return cfg.toShortString(k);
});
CLOG(DEBUG, "SCP")
CLOG(DEBUG, "Herder")
<< "qmap[" << cfg.toShortString(pair.first) << "] = " << str;
}
else
{
CLOG(DEBUG, "SCP")
CLOG(DEBUG, "Herder")
<< "qmap[" << cfg.toShortString(pair.first) << "] = nullptr";
}
}
Expand Down Expand Up @@ -840,3 +841,98 @@ TEST_CASE("quorum intersection criticality",
REQUIRE(groups.size() == 1);
REQUIRE(groups == std::set<std::set<PublicKey>>{{orgs[3][0]}});
}

TEST_CASE("quorum intersection topological order of SCCs",
"[herder][quorumintersectiontopo]")
{
using namespace stellar::quorum_intersection;

// This test checks that the SCCs examined by the quorum intersection
// checker are ordered _topologically_ rather than by size, and that the
// checker examines a maximum element of the reachability relation between
// SCCs.
//
// We test this by manufacturing a large SCC A that can reach a smaller SCC
// B, and then making SCC A contain no quorums, and checking that we
// actually found some quorums (meaning: we scanned SCC B). Previously this
// would cause the checker to focus on A (which we're configuring with _no_
// quorums), when it was sorting by size; but we want it to focus on B.
//
//
// SCC A
//
// org0 <--+
// | |
// v |
// org1 |
// | | SCC B
// v |
// org2 | org7 <-+
// | | ^ |
// | | | |
// v | v |
// org3 | org6 |
// | | ^ |
// | | | |
// v | v |
// org4 ---+---> org5 <-+
//

auto orgs = generateOrgs(8, {1});
auto qm = interconnectOrgsUnidir(orgs,
{
{0, 1},
{1, 2},
{2, 3},
{3, 4},
{4, 0},
{4, 5},
{5, 6},
{6, 5},
{6, 7},
{7, 6},
{7, 5},
{5, 7},
},
/* ownThreshPct=*/100);
Config cfg(getTestConfig());
cfg = configureShortNames(cfg, orgs);
debugQmap(cfg, qm);
QuorumIntersectionCheckerImpl qic(qm, cfg, /*quiet=*/false);
REQUIRE(qic.networkEnjoysQuorumIntersection());
REQUIRE(qic.getMaxQuorumsFound() != 0);
REQUIRE(qic.getMaxQuorumsFound() != 0);

// Now check the reachability calculator calculated the correct relation.
std::vector<BitSet> bs;
bs.resize(8);
for (size_t i = 0; i < 8; ++i)
{
bs.at(i).set(qic.getPubkeyBitNum(orgs[i][0]));
}

SCCReachabilityCalculator r(qic.getGraph());
r.calculateReachability();

for (size_t i = 0; i < r.mReachableNodes.size(); ++i)
{
CLOG(DEBUG, "Herder")
<< "reachable from " << i << ": " << r.mReachableNodes.at(i);
}

for (size_t i = 0; i < 5; ++i)
{
for (size_t j = 0; j < 8; ++j)
{
REQUIRE(r.canReach(bs.at(i), bs.at(j)));
}
}

for (size_t i = 5; i < 8; ++i)
{
for (size_t j = 5; j < 8; ++j)
{
REQUIRE(r.canReach(bs.at(i), bs.at(j)));
}
}
}
6 changes: 6 additions & 0 deletions src/util/BitSet.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,12 @@ class BitSet
BitSet(BitSet&& other) = default;
BitSet& operator=(BitSet&& other) = default;

bool
operator!=(BitSet const& other) const
{
return !((*this) == other);
}

bool
operator==(BitSet const& other) const
{
Expand Down