diff --git a/src/ripple/app/misc/ValidatorList.h b/src/ripple/app/misc/ValidatorList.h index 4b6012f2ebb..661e0e4ed1e 100644 --- a/src/ripple/app/misc/ValidatorList.h +++ b/src/ripple/app/misc/ValidatorList.h @@ -309,7 +309,8 @@ class ValidatorList std::function func) const; static std::size_t - calculateQuorum (std::size_t nTrustedKeys); + calculateMinimumQuorum ( + std::size_t nListedKeys, bool unlistedLocal=false); private: /** Check response for trusted valid published list @@ -399,15 +400,11 @@ 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 of 2f + 1 guarantees safe overlap with the trusted + // sets of other nodes using the same set of published lists. + // Safety is guaranteed with up to 1/3 listed validators being malicious. + std::size_t quorum = calculateMinimumQuorum (keyListings_.size(), + localPubKey_.size() && !localKeyListed); JLOG (j_.debug()) << rankedKeys.size() << " of " << keyListings_.size() << @@ -415,20 +412,23 @@ ValidatorList::onConsensusStart ( 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 (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 + std::size_t const targetQuorum = size - size / 5; + if (targetQuorum > quorum) + quorum = targetQuorum; + } + else + { + // Reduce the trusted set size so that the quorum represents + // at least 80% + size = quorum * 1.25; + } } if (minimumQuorum_ && (seenValidators.empty() || diff --git a/src/ripple/app/misc/impl/ValidatorList.cpp b/src/ripple/app/misc/impl/ValidatorList.cpp index 68182148751..df8b07ba8cc 100644 --- a/src/ripple/app/misc/impl/ValidatorList.cpp +++ b/src/ripple/app/misc/impl/ValidatorList.cpp @@ -409,15 +409,23 @@ 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 quorum{{ 0, 1, 2, 2, 3, 3, 4, 5, 6, 7 }}; + // Use 2f + 1 for large values of n, but have special cases for small numbers. + constexpr std::array quorum{{ 0, 1, 2, 2, 3, 3 }}; - if (nTrustedKeys < quorum.size()) - return quorum[nTrustedKeys]; - return nTrustedKeys - nTrustedKeys / 5; + // 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 >= quorum.size()) + ++nListedKeys; + + if (nListedKeys < quorum.size()) + return quorum[nListedKeys]; + + return nListedKeys * 2/3 + 1; } } // ripple diff --git a/src/test/app/ValidatorList_test.cpp b/src/test/app/ValidatorList_test.cpp index 8fa257c6566..428bc8fd6d0 100644 --- a/src/test/app/ValidatorList_test.cpp +++ b/src/test/app/ValidatorList_test.cpp @@ -125,17 +125,26 @@ class ValidatorList_test : public beast::unit_test::suite } void - testCalculateQuorum () + testCalculateMinimumQuorum () { - testcase ("Calculate Quorum"); + testcase ("Calculate Minimum Quorum"); for(std::size_t i = 1; i < 20; ++i) { - auto const quorum = ValidatorList::calculateQuorum(i); - if (i < 10) + auto const quorum = ValidatorList::calculateMinimumQuorum(i); + if (i <= 5) + { BEAST_EXPECT(quorum >= (i/2 + 1)); + BEAST_EXPECT(ValidatorList::calculateMinimumQuorum( + i, true /* untrusted local validator */) == quorum); + } else - BEAST_EXPECT(quorum == std::ceil (i * 0.8)); + { + BEAST_EXPECT(quorum == std::floor (i * 2/3) + 1); + BEAST_EXPECT(ValidatorList::calculateMinimumQuorum( + i, true /* untrusted local validator */) == + ValidatorList::calculateMinimumQuorum(i + 1)); + } } } @@ -522,7 +531,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) { @@ -571,7 +580,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)); @@ -584,12 +593,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)); @@ -613,7 +621,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)); @@ -700,7 +708,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)); @@ -770,7 +778,7 @@ class ValidatorList_test : public beast::unit_test::suite emptyLocalKey, cfgKeys, cfgPublishers)); trustedKeys->onConsensusStart (activeValidators); BEAST_EXPECT(trustedKeys->quorum () == - ValidatorList::calculateQuorum(cfgKeys.size())); + ValidatorList::calculateMinimumQuorum(cfgKeys.size())); for (auto const& key : activeValidators) BEAST_EXPECT(trustedKeys->trusted (key)); } @@ -799,11 +807,9 @@ 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; + auto expectedQuorum = ValidatorList::calculateMinimumQuorum( + cfgKeys.size(), true /* unlisted local validator */); + BEAST_EXPECT(trustedKeys->quorum () == expectedQuorum); for (auto const& key : activeValidators) BEAST_EXPECT(trustedKeys->trusted (key)); @@ -816,7 +822,7 @@ class ValidatorList_test : public beast::unit_test::suite run() override { testGenesisQuorum (); - testCalculateQuorum (); + testCalculateMinimumQuorum (); testConfigLoad (); testApplyList (); testUpdate ();