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

Make minimum quorum Byzantine fault tolerant (RIPD-1461) #2093

Closed
wants to merge 1 commit into from
Closed

Make minimum quorum Byzantine fault tolerant (RIPD-1461) #2093

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
57 changes: 32 additions & 25 deletions src/ripple/app/misc/ValidatorList.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,9 +308,6 @@ class ValidatorList
for_each_listed (
std::function<void(PublicKey const&, bool)> func) const;

static std::size_t
calculateQuorum (std::size_t nTrustedKeys);

private:
/** Check response for trusted valid published list

Expand Down Expand Up @@ -341,6 +338,15 @@ class ValidatorList
bool
removePublisherList (PublicKey const& publisherKey);

/** Return safe minimum quorum for listed validator set

@param nListedKeys Number of list validator keys

@param unListedLocal Whether the local node is an unlisted validator
*/
static std::size_t
calculateMinimumQuorum (
std::size_t nListedKeys, bool unlistedLocal=false);
};

//------------------------------------------------------------------------------
Expand Down Expand Up @@ -399,41 +405,42 @@ ValidatorList::onConsensusStart (
}
}

// This quorum guarantees sufficient overlap with the trusted sets of other
// nodes using the same set of published lists.
std::size_t quorum = keyListings_.size()/2 + 1;

// Increment the quorum to prevent two unlisted validators using the same
// even number of listed validators from forking.
if (localPubKey_.size() && ! localKeyListed &&
rankedKeys.size () > 1 && keyListings_.size () % 2 != 0)
++quorum;
// This minimum quorum guarantees safe overlap with the trusted sets of
// other nodes using the same set of published lists.
std::size_t quorum = calculateMinimumQuorum (keyListings_.size(),
localPubKey_.size() && !localKeyListed);

JLOG (j_.debug()) <<
rankedKeys.size() << " of " << keyListings_.size() <<
" listed validators eligible for inclusion in the trusted set";

auto size = rankedKeys.size();

// Use all eligible keys if there is less than 10 listed validators or
// only one trusted list
if (size < 10 || publisherLists_.size() == 1)
{
// Try to raise the quorum toward or above 80% of the trusted set
std::size_t const targetQuorum = ValidatorList::calculateQuorum (size);
if (targetQuorum > quorum)
quorum = targetQuorum;
}
else
// Do not require 80% quorum for less than 10 trusted validators
if (rankedKeys.size() >= 10)
{
// reduce the trusted set size so that the quorum represents
// at least 80%
size = quorum * 1.25;
// Use all eligible keys if there is only one trusted list
if (publisherLists_.size() == 1)
{
// Try to raise the quorum to at least 80% of the trusted set
quorum = std::max(quorum, size - size / 5);
}
else
{
// Reduce the trusted set size so that the quorum represents
// at least 80%
size = quorum * 1.25;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is new with these changes, but can't size end up greater than rankedKeys_.size()?

For example, suppose there are

  • More than 1 publisher lists
  • 20 listed keys
  • 10 ranked keys
  • This node has an unlisted key

Then isn't the minimum quorum 14, but the calculated size = 14 * 1.25 = 17? If so, I think that would make for an infinite loop down on line 450.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

size can be greater than rankedKeys_.size()
That loop should be fine in that case since it is iterating rankedKeys. The result is just that we end up with fewer trusted keys than size.

Copy link
Collaborator

Choose a reason for hiding this comment

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

D'oh, misread that loop. Thanks!

}
}

if (minimumQuorum_ && (seenValidators.empty() ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar question to the above, but is setting quorum = *minimumQuorum_ dangerous for the extreme revoked key case?

Suppose

  • More than 1 publisher list
  • 10 list keys
  • 7 ranked keys
  • This node has unlisted key
    Calculated minimum quorum is 8, so rankedKeys.size() < 8 would set quorum to minimumQuorum_, which might be 3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Setting quorum = *minimumQuorum_ removes the safety guarantees from calculateMinimumQuorum. The quorum will not be Byzantine fault tolerant and may simply be forkable (<51%).
The quorum command line option used to set minimumQuorum_ is advertised as such
https://github.com/ripple/rippled/blob/develop/src/ripple/app/main/Main.cpp#L231

Copy link
Contributor

Choose a reason for hiding this comment

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

We should consider logging at warning if quorum > minimumQuorum_. Two tangential questions: first, under what circumstances would we use --quorum and second, should the description of the command include a marker (e.g. "(EXPERT USE ONLY)")?

Copy link
Contributor Author

@wilsonianb wilsonianb Apr 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Now that we're not bumping the quorum as an unlisted validator if there are less than 5, we might not need --quorum.
https://github.com/ripple/rippled/pull/2093/files#diff-7f8c7a926e17debbef064bc5c2f572dbR420
Previously, it was impossible to start up a new set of validators (reset the altnet) without specifying --quorum.
Note that I think --quorum acted the same pre-dynamic unl. Like [validation_quorum], it was the absolute minimum quorum you were willing to allow, which could possibly be unsafe.

rankedKeys.size() < quorum))
{
quorum = *minimumQuorum_;
JLOG (j_.warn()) <<
"Using unsafe quorum of " << quorum_ <<
" as specified in the command line";
}

// Do not use achievable quorum until lists from all configured
// publishers are available
Expand Down
34 changes: 26 additions & 8 deletions src/ripple/app/misc/impl/ValidatorList.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,15 +409,33 @@ ValidatorList::for_each_listed (
}

std::size_t
ValidatorList::calculateQuorum (std::size_t nTrustedKeys)
ValidatorList::calculateMinimumQuorum (
std::size_t nListedKeys, bool unlistedLocal)
{
// Use 80% for large values of n, but have special cases for small numbers.
constexpr std::array<std::size_t, 10> quorum{{ 0, 1, 2, 2, 3, 3, 4, 5, 6, 7 }};

if (nTrustedKeys < quorum.size())
return quorum[nTrustedKeys];

return nTrustedKeys - nTrustedKeys / 5;
// Only require 51% quorum for small number of validators to facilitate
// bootstrapping a network.
if (nListedKeys <= 5)
return nListedKeys/2 + 1;

// The number of listed validators is increased to preserve the safety
// guarantee for two unlisted validators using the same set of listed
// validators.
if (unlistedLocal)
++nListedKeys;

// Guarantee safety with up to 1/3 listed validators being malicious.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it worth documenting the reason for 1/3?

// This prioritizes safety (Byzantine fault tolerance) over liveness.
// It takes at least as many malicious nodes to split/fork the network as
// to stall the network.
// At 67%, the overlap of two quorums is 34%
// 67 + 67 - 100 = 34
// So under certain conditions, 34% of validators could vote for two
// different ledgers and split the network.
// Similarly 34% could prevent quorum from being met (by not voting) and
// stall the network.
// If/when the quorum is subsequently raised to/towards 80%, it becomes
// harder to split the network (more safe) and easier to stall it (less live).
return nListedKeys * 2/3 + 1;
}

} // ripple
116 changes: 87 additions & 29 deletions src/test/app/ValidatorList_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,21 +124,6 @@ class ValidatorList_test : public beast::unit_test::suite
}
}

void
testCalculateQuorum ()
{
testcase ("Calculate Quorum");

for(std::size_t i = 1; i < 20; ++i)
{
auto const quorum = ValidatorList::calculateQuorum(i);
if (i < 10)
BEAST_EXPECT(quorum >= (i/2 + 1));
else
BEAST_EXPECT(quorum == std::ceil (i * 0.8));
}
}

void
testConfigLoad ()
{
Expand Down Expand Up @@ -522,7 +507,7 @@ class ValidatorList_test : public beast::unit_test::suite
// onConsensusStart should make all available configured
// validators trusted
trustedKeys->onConsensusStart (activeValidators);
BEAST_EXPECT(trustedKeys->quorum () == 12);
BEAST_EXPECT(trustedKeys->quorum () == 14);
std::size_t i = 0;
for (auto const& val : cfgKeys)
{
Expand All @@ -538,6 +523,16 @@ class ValidatorList_test : public beast::unit_test::suite
else
fail ();
}

{
// Quorum should be 80% with all listed validators active
hash_set<PublicKey> activeValidators;
for (auto const valKey : cfgKeys)
activeValidators.emplace (*parseBase58<PublicKey>(
TokenType::TOKEN_NODE_PUBLIC, valKey));
trustedKeys->onConsensusStart (activeValidators);
BEAST_EXPECT(trustedKeys->quorum () == cfgKeys.size() * 4/5);
}
}
{
// update with manifests
Expand Down Expand Up @@ -571,7 +566,7 @@ class ValidatorList_test : public beast::unit_test::suite
manifests.applyManifest(std::move (*m1)) ==
ManifestDisposition::accepted);
trustedKeys->onConsensusStart (activeValidators);
BEAST_EXPECT(trustedKeys->quorum () == 13);
BEAST_EXPECT(trustedKeys->quorum () == 15);
BEAST_EXPECT(trustedKeys->listed (masterPublic));
BEAST_EXPECT(trustedKeys->trusted (masterPublic));
BEAST_EXPECT(trustedKeys->listed (signingPublic1));
Expand All @@ -584,12 +579,11 @@ class ValidatorList_test : public beast::unit_test::suite
auto m2 = Manifest::make_Manifest (makeManifestString (
masterPublic, masterPrivate,
signingPublic2, signingKeys2.second, 2));

BEAST_EXPECT(
manifests.applyManifest(std::move (*m2)) ==
ManifestDisposition::accepted);
trustedKeys->onConsensusStart (activeValidators);
BEAST_EXPECT(trustedKeys->quorum () == 13);
BEAST_EXPECT(trustedKeys->quorum () == 15);
BEAST_EXPECT(trustedKeys->listed (masterPublic));
BEAST_EXPECT(trustedKeys->trusted (masterPublic));
BEAST_EXPECT(trustedKeys->listed (signingPublic2));
Expand All @@ -613,7 +607,7 @@ class ValidatorList_test : public beast::unit_test::suite
BEAST_EXPECT(manifests.getSigningKey (masterPublic) == masterPublic);
BEAST_EXPECT(manifests.revoked (masterPublic));
trustedKeys->onConsensusStart (activeValidators);
BEAST_EXPECT(trustedKeys->quorum () == 12);
BEAST_EXPECT(trustedKeys->quorum () == 15);
BEAST_EXPECT(trustedKeys->listed (masterPublic));
BEAST_EXPECT(!trustedKeys->trusted (masterPublic));
BEAST_EXPECT(!trustedKeys->listed (signingPublicMax));
Expand Down Expand Up @@ -700,7 +694,7 @@ class ValidatorList_test : public beast::unit_test::suite
localKey, cfgKeys, cfgPublishers));

trustedKeys->onConsensusStart (activeValidators);
BEAST_EXPECT(trustedKeys->quorum () == 3);
BEAST_EXPECT(trustedKeys->quorum () == 2);

// local validator key is always trusted
BEAST_EXPECT(trustedKeys->trusted (localKey));
Expand Down Expand Up @@ -770,7 +764,8 @@ class ValidatorList_test : public beast::unit_test::suite
emptyLocalKey, cfgKeys, cfgPublishers));
trustedKeys->onConsensusStart (activeValidators);
BEAST_EXPECT(trustedKeys->quorum () ==
ValidatorList::calculateQuorum(cfgKeys.size()));
(cfgKeys.size() <= 5) ? cfgKeys.size()/2 + 1 :
cfgKeys.size() * 2/3 + 1);
for (auto const& key : activeValidators)
BEAST_EXPECT(trustedKeys->trusted (key));
}
Expand Down Expand Up @@ -799,24 +794,87 @@ class ValidatorList_test : public beast::unit_test::suite
localKey, cfgKeys, cfgPublishers));
trustedKeys->onConsensusStart (activeValidators);

// When running as an unlisted validator,
// the quorum is incremented by 1 for 3 or 5 trusted validators.
auto expectedQuorum = ValidatorList::calculateQuorum(cfgKeys.size());
if (cfgKeys.size() == 3 || cfgKeys.size() == 5)
++expectedQuorum;
BEAST_EXPECT(trustedKeys->quorum () == expectedQuorum);
BEAST_EXPECT(trustedKeys->quorum () ==
(cfgKeys.size() <= 5) ? cfgKeys.size()/2 + 1 :
(cfgKeys.size() + 1) * 2/3 + 1);

for (auto const& key : activeValidators)
BEAST_EXPECT(trustedKeys->trusted (key));
}
}
{
// Trusted set should be trimmed with multiple validator lists
ManifestCache manifests;
auto trustedKeys = std::make_unique <ValidatorList> (
manifests, manifests, env.timeKeeper(), beast::Journal ());

hash_set<PublicKey> activeValidators;

std::vector<PublicKey> valKeys;
valKeys.reserve(20);

while (valKeys.size () != 20)
{
valKeys.push_back (randomNode());
activeValidators.emplace (valKeys.back());
}

auto addPublishedList = [this, &env, &trustedKeys, &valKeys]()
{
auto const publisherSecret = randomSecretKey();
auto const publisherPublic =
derivePublicKey(KeyType::ed25519, publisherSecret);
auto const pubSigningKeys = randomKeyPair(KeyType::secp256k1);
auto const manifest = beast::detail::base64_encode(makeManifestString (
publisherPublic, publisherSecret,
pubSigningKeys.first, pubSigningKeys.second, 1));

std::vector<std::string> cfgPublishers({
strHex(publisherPublic)});
PublicKey emptyLocalKey;
std::vector<std::string> emptyCfgKeys;

BEAST_EXPECT(trustedKeys->load (
emptyLocalKey, emptyCfgKeys, cfgPublishers));

auto const version = 1;
auto const sequence = 1;
NetClock::time_point const expiration =
env.timeKeeper().now() + 3600s;
auto const blob = makeList (
valKeys, sequence, expiration.time_since_epoch().count());
auto const sig = signList (blob, pubSigningKeys);

BEAST_EXPECT(ListDisposition::accepted == trustedKeys->applyList (
manifest, blob, sig, version));
};

// Apply multiple published lists
for (auto i = 0; i < 3; ++i)
addPublishedList();

trustedKeys->onConsensusStart (activeValidators);

// Minimum quorum should be used
BEAST_EXPECT(trustedKeys->quorum () == (valKeys.size() * 2/3 + 1));

std::size_t nTrusted = 0;
for (auto const& key : activeValidators)
{
if (trustedKeys->trusted (key))
++nTrusted;
}

// The number of trusted keys should be 125% of the minimum quorum
BEAST_EXPECT(nTrusted == trustedKeys->quorum () * 5 / 4);
}
}

public:
void
run() override
{
testGenesisQuorum ();
testCalculateQuorum ();
testConfigLoad ();
testApplyList ();
testUpdate ();
Expand Down