Increase validation quorum to 80%

All listed validators are trusted and quorum is 80% of trusted
validators regardless of the number of:
* configured published lists
* listed or trusted validators
* recently seen validators

Exceptions:
* A listed validator whose master key has been revoked is not trusted
* Custom minimum quorum (specified with --quorum in the command line)
  is used if the normal quorum appears unreachable based on the number
  of recently received validators.

RIPD-1640

cfg/validators-example.txt

@@ -47,6 +47,12 @@
 
 # The default validator list publishers that the rippled instance
 # trusts.
+#
+# WARNING: Changing these values can cause your rippled instance to see a
+#          validated ledger that contradicts other rippled instances'
+#          validated ledgers (aka a ledger fork) if your validator list(s)
+#          do not sufficiently overlap with the list(s) used by others.
+#          See: https://arxiv.org/pdf/1802.07242.pdf
 
 [validator_list_sites]
 https://vl.ripple.com

src/ripple/app/misc/ValidatorList.h

@@ -148,17 +148,6 @@ class ValidatorList
     // Currently supported version of publisher list format
     static constexpr std::uint32_t requiredListVersion = 1;
 
-    // The minimum number of listed validators required to allow removing
-    // non-communicative validators from the trusted set. In other words, if the
-    // number of listed validators is less, then use all of them in the
-    // trusted set.
-    std::size_t const MINIMUM_RESIZEABLE_UNL {25};
-    // The maximum size of a trusted set for which greater than Byzantine fault
-    // tolerance isn't needed.
-    std::size_t const BYZANTINE_THRESHOLD {32};
-
-
-
 public:
     ValidatorList (
         ManifestCache& validatorManifests,
@@ -393,15 +382,15 @@ private:
     bool
     removePublisherList (PublicKey const& publisherKey);
 
-    /** Return safe minimum quorum for listed validator set
+    /** Return quorum for trusted validator set
 
-        @param nListedKeys Number of list validator keys
+        @param trusted Number of trusted validator keys
 
-        @param unListedLocal Whether the local node is an unlisted validator
-    */
-    static std::size_t
-    calculateMinimumQuorum (
-        std::size_t nListedKeys, bool unlistedLocal=false);
+        @param seen Number of trusted validators that have signed
+        recently received validations */
+    std::size_t
+    calculateQuorum (
+        std::size_t trusted, std::size_t seen);
 };
 } // ripple
 

src/ripple/app/misc/impl/ValidatorList.cpp

@@ -436,8 +436,7 @@ ValidatorList::removePublisherList (PublicKey const& publisherKey)
         return false;
 
     JLOG (j_.debug()) <<
-        "Removing validator list for revoked publisher " <<
-        toBase58(TokenType::NodePublic, publisherKey);
+        "Removing validator list for publisher " << strHex(publisherKey);
 
     for (auto const& val : iList->second.list)
     {
@@ -569,33 +568,62 @@ ValidatorList::for_each_listed (
 }
 
 std::size_t
-ValidatorList::calculateMinimumQuorum (
-    std::size_t nListedKeys, bool unlistedLocal)
+ValidatorList::calculateQuorum (
+    std::size_t trusted, std::size_t seen)
 {
-    // Only require 51% quorum for small number of validators to facilitate
-    // bootstrapping a network.
-    if (nListedKeys <= 6)
-        return nListedKeys/2 + 1;
+    // Do not use achievable quorum until lists from all configured
+    // publishers are available
+    for (auto const& list : publisherLists_)
+    {
+        if (! list.second.available)
+            return std::numeric_limits<std::size_t>::max();
+    }
 
-    // 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;
+    // Use an 80% quorum to balance fork safety, liveness, and required UNL
+    // overlap.
+    //
+    // Theorem 8 of the Analysis of the XRP Ledger Consensus Protocol
+    // (https://arxiv.org/abs/1802.07242) says:
+    //     XRP LCP guarantees fork safety if Oi,j > nj/2 + ni − qi + ti,j for
+    //     every pair of nodes Pi, Pj.
+    //
+    // ni: size of Pi's UNL
+    // nj: size of Pj's UNL
+    // Oi,j: number of validators in both UNLs
+    // qi: validation quorum for Pi's UNL
+    // ti, tj: maximum number of allowed Byzantine faults in Pi and Pj's UNLs
+    // ti,j: min{ti, tj, Oi,j}
+    //
+    // Assume ni < nj, meaning and ti,j = ti
+    //
+    // For qi = .8*ni, we make ti <= .2*ni
+    // (We could make ti lower and tolerate less UNL overlap. However in order
+    // to prioritize safety over liveness, we need ti >= ni - qi)
+    //
+    // An 80% quorum allows two UNLs to safely have < .2*ni unique validators
+    // between them:
+    //
+    // pi = ni - Oi,j
+    // pj = nj - Oi,j
+    //
+    // Oi,j > nj/2 + ni − qi + ti,j
+    // ni - pi > (ni - pi + pj)/2 + ni − .8*ni + .2*ni
+    // pi + pj < .2*ni
+    auto quorum = static_cast<std::size_t>(std::ceil(trusted * 0.8f));
 
-    // Guarantee safety with up to 1/3 listed validators being malicious.
-    // 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;
+    // Use lower quorum specified via command line if the normal quorum appears
+    // unreachable based on the number of recently received validations.
+    if (minimumQuorum_ && *minimumQuorum_ < quorum && seen < quorum)
+    {
+        quorum = *minimumQuorum_;
+
+        JLOG (j_.warn())
+            << "Using unsafe quorum of "
+            << quorum
+            << " as specified in the command line";
+    }
+
+    return quorum;
 }
 
 TrustChanges
@@ -603,120 +631,43 @@ ValidatorList::updateTrusted(hash_set<NodeID> const& seenValidators)
 {
     boost::unique_lock<boost::shared_mutex> lock{mutex_};
 
-    // Check that lists from all configured publishers are available
-    bool allListsAvailable = true;
-
+    // Remove any expired published lists
     for (auto const& list : publisherLists_)
     {
-        // Remove any expired published lists
-        if (TimeKeeper::time_point{} < list.second.expiration &&
+        if (list.second.available &&
             list.second.expiration <= timeKeeper_.now())
             removePublisherList(list.first);
-
-        if (! list.second.available)
-            allListsAvailable = false;
     }
 
-    std::multimap<std::size_t, PublicKey> rankedKeys;
-    bool localKeyListed = false;
+    TrustChanges trustChanges;
 
-    // "Iterate" the listed keys in random order so that the rank of multiple
-    // keys with the same number of listings is not deterministic
-    std::vector<std::size_t> indexes (keyListings_.size());
-    std::iota (indexes.begin(), indexes.end(), 0);
-    std::shuffle (indexes.begin(), indexes.end(), crypto_prng());
-
-    for (auto const& index : indexes)
+    auto it = trustedKeys_.cbegin();
+    while (it != trustedKeys_.cend())
     {
-        auto const& val = std::next (keyListings_.begin(), index);
-
-        if (validatorManifests_.revoked (val->first))
-            continue;
-
-        if (val->first == localPubKey_)
+        if (! keyListings_.count(*it) ||
+            validatorManifests_.revoked(*it))
         {
-            localKeyListed = val->second > 1;
-            rankedKeys.insert (
-                std::pair<std::size_t,PublicKey>(
-                    std::numeric_limits<std::size_t>::max(), localPubKey_));
-        }
-        // If the total number of validators is too small, or
-        // no validations are being received, use all validators.
-        // Otherwise, do not use validators whose validations aren't
-        // being received.
-        else if (
-            keyListings_.size() < MINIMUM_RESIZEABLE_UNL ||
-            seenValidators.empty() ||
-            seenValidators.find(calcNodeID(val->first)) != seenValidators.end())
-        {
-            rankedKeys.insert (
-                std::pair<std::size_t,PublicKey>(val->second, val->first));
-        }
-    }
-
-    // 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();
-
-    // Require 80% quorum if there are lots of validators.
-    if (rankedKeys.size() > BYZANTINE_THRESHOLD)
-    {
-        // Use all eligible keys if there is only one trusted list
-        if (publisherLists_.size() == 1 ||
-                keyListings_.size() < MINIMUM_RESIZEABLE_UNL)
-        {
-            // Try to raise the quorum to at least 80% of the trusted set
-            quorum = std::max(quorum, size - size / 5);
+            trustChanges.removed.insert(calcNodeID(*it));
+            it = trustedKeys_.erase(it);
         }
         else
         {
-            // Reduce the trusted set size so that the quorum represents
-            // at least 80%
-            size = quorum * 1.25;
+            ++it;
         }
     }
 
-    if (minimumQuorum_ && seenValidators.size() < quorum)
+    for (auto const& val : keyListings_)
     {
-        quorum = *minimumQuorum_;
-        JLOG (j_.warn())
-            << "Using unsafe quorum of "
-            << quorum_
-            << " as specified in the command line";
+        if (! validatorManifests_.revoked(val.first) &&
+                trustedKeys_.emplace(val.first).second)
+            trustChanges.added.insert(calcNodeID(val.first));
     }
 
-    // Do not use achievable quorum until lists from all configured
-    // publishers are available
-    else if (! allListsAvailable)
-        quorum = std::numeric_limits<std::size_t>::max();
-
-    TrustChanges trustChanges;
-    {
-        hash_set<PublicKey> newTrustedKeys;
-        for (auto const& val : boost::adaptors::reverse(rankedKeys))
-        {
-            if (size <= newTrustedKeys.size())
-                break;
-            newTrustedKeys.insert(val.second);
-
-            if (trustedKeys_.erase(val.second) == 0)
-                trustChanges.added.insert(calcNodeID(val.second));
-        }
-
-        for (auto const& k : trustedKeys_)
-            trustChanges.removed.insert(calcNodeID(k));
-        trustedKeys_ = std::move(newTrustedKeys);
-    }
-
-    quorum_ = quorum;
+    JLOG (j_.debug()) <<
+        trustedKeys_.size() << "  of " << keyListings_.size() <<
+        " listed validators eligible for inclusion in the trusted set";
 
+    quorum_ = calculateQuorum (trustedKeys_.size(), seenValidators.size());
 
     JLOG(j_.debug()) << "Using quorum of " << quorum_ << " for new set of "
                      << trustedKeys_.size() << " trusted validators ("

src/test/app/ValidatorList_test.cpp

@@ -531,14 +531,12 @@ private:
 
         std::vector<std::string> cfgPublishers;
         hash_set<NodeID> activeValidators;
-        hash_set<NodeID> secondAddedValidators;
 
-        // BFT: n >= 3f+1
         std::size_t const n = 40;
-        std::size_t const f = 13;
         {
             std::vector<std::string> cfgKeys;
             cfgKeys.reserve(n);
+            hash_set<NodeID> unseenValidators;
 
             while (cfgKeys.size () != n)
             {
@@ -547,52 +545,43 @@ private:
                     TokenType::NodePublic, valKey));
                 if (cfgKeys.size () <= n - 5)
                     activeValidators.emplace (calcNodeID(valKey));
+                else
+                    unseenValidators.emplace (calcNodeID(valKey));
             }
 
             BEAST_EXPECT(trustedKeys->load (
                 emptyLocalKey, cfgKeys, cfgPublishers));
 
-            // updateTrusted should make all available configured
-            // validators trusted
+            // updateTrusted should make all configured validators trusted
+            // even if they are not active/seen
             TrustChanges changes =
                 trustedKeys->updateTrusted(activeValidators);
+
+            for (auto const& val : unseenValidators)
+                activeValidators.emplace (val);
+
             BEAST_EXPECT(changes.added == activeValidators);
             BEAST_EXPECT(changes.removed.empty());
-            // Add 1 to n because I'm not on a published list.
-            BEAST_EXPECT(trustedKeys->quorum () == n + 1 - f);
-            std::size_t i = 0;
+            BEAST_EXPECT(trustedKeys->quorum () ==
+                std::ceil(cfgKeys.size() * 0.8f));
             for (auto const& val : cfgKeys)
             {
                 if (auto const valKey = parseBase58<PublicKey>(
                     TokenType::NodePublic, val))
                 {
                     BEAST_EXPECT(trustedKeys->listed (*valKey));
-                    if (i++ < activeValidators.size ())
-                        BEAST_EXPECT(trustedKeys->trusted (*valKey));
-                    else
-                        BEAST_EXPECT(!trustedKeys->trusted (*valKey));
+                    BEAST_EXPECT(trustedKeys->trusted (*valKey));
                 }
                 else
                     fail ();
             }
 
-            {
-                // Quorum should be 80% with all listed validators active
-                hash_set<NodeID> activeValidatorsNew{activeValidators};
-                for (auto const valKey : cfgKeys)
-                {
-                    auto const ins = activeValidatorsNew.emplace(
-                        calcNodeID(*parseBase58<PublicKey>(
-                            TokenType::NodePublic, valKey)));
-                    if(ins.second)
-                        secondAddedValidators.insert(*ins.first);
-                }
-                TrustChanges changes =
-                    trustedKeys->updateTrusted(activeValidatorsNew);
-                BEAST_EXPECT(changes.added == secondAddedValidators);
-                BEAST_EXPECT(changes.removed.empty());
-                BEAST_EXPECT(trustedKeys->quorum () == cfgKeys.size() * 4/5);
-            }
+            changes =
+                trustedKeys->updateTrusted(activeValidators);
+            BEAST_EXPECT(changes.added.empty());
+            BEAST_EXPECT(changes.removed.empty());
+            BEAST_EXPECT(trustedKeys->quorum () ==
+                std::ceil(cfgKeys.size() * 0.8f));
         }
         {
             // update with manifests
@@ -614,7 +603,8 @@ private:
             TrustChanges changes =
                 trustedKeys->updateTrusted(activeValidators);
             BEAST_EXPECT(changes.added == asNodeIDs({masterPublic}));
-            BEAST_EXPECT(changes.removed == secondAddedValidators);
+            BEAST_EXPECT(changes.removed.empty());
+            BEAST_EXPECT(trustedKeys->quorum () == std::ceil((n + 1) * 0.8f));
             BEAST_EXPECT(trustedKeys->listed (masterPublic));
             BEAST_EXPECT(trustedKeys->trusted (masterPublic));
             BEAST_EXPECT(!trustedKeys->listed (signingPublic1));
@@ -628,10 +618,6 @@ private:
             BEAST_EXPECT(
                 manifests.applyManifest(std::move (*m1)) ==
                     ManifestDisposition::accepted);
-            changes = trustedKeys->updateTrusted(activeValidators);
-            BEAST_EXPECT(changes.removed.empty());
-            BEAST_EXPECT(changes.added.empty());
-            BEAST_EXPECT(trustedKeys->quorum () == n + 2 - f);
             BEAST_EXPECT(trustedKeys->listed (masterPublic));
             BEAST_EXPECT(trustedKeys->trusted (masterPublic));
             BEAST_EXPECT(trustedKeys->listed (signingPublic1));
@@ -647,10 +633,6 @@ private:
             BEAST_EXPECT(
                 manifests.applyManifest(std::move (*m2)) ==
                     ManifestDisposition::accepted);
-            changes = trustedKeys->updateTrusted (activeValidators);
-            BEAST_EXPECT(changes.removed.empty());
-            BEAST_EXPECT(changes.added.empty());
-            BEAST_EXPECT(trustedKeys->quorum () == n + 2 - f);
             BEAST_EXPECT(trustedKeys->listed (masterPublic));
             BEAST_EXPECT(trustedKeys->trusted (masterPublic));
             BEAST_EXPECT(trustedKeys->listed (signingPublic2));
@@ -673,10 +655,15 @@ private:
                     ManifestDisposition::accepted);
             BEAST_EXPECT(manifests.getSigningKey (masterPublic) == masterPublic);
             BEAST_EXPECT(manifests.revoked (masterPublic));
+
+            // Revoked key remains trusted until list is updated
+            BEAST_EXPECT(trustedKeys->listed (masterPublic));
+            BEAST_EXPECT(trustedKeys->trusted (masterPublic));
+
             changes = trustedKeys->updateTrusted (activeValidators);
             BEAST_EXPECT(changes.removed == asNodeIDs({masterPublic}));
             BEAST_EXPECT(changes.added.empty());
-            BEAST_EXPECT(trustedKeys->quorum () == n + 1 - f);
+            BEAST_EXPECT(trustedKeys->quorum () == std::ceil(n * 0.8f));
             BEAST_EXPECT(trustedKeys->listed (masterPublic));
             BEAST_EXPECT(!trustedKeys->trusted (masterPublic));
             BEAST_EXPECT(!trustedKeys->listed (signingPublicMax));
@@ -708,29 +695,6 @@ private:
             BEAST_EXPECT(trustedKeys->quorum () ==
                 std::numeric_limits<std::size_t>::max());
         }
-        {
-            // Trust all listed validators if none are active
-            auto trustedKeys = std::make_unique <ValidatorList> (
-                manifests, manifests, env.timeKeeper(), beast::Journal ());
-
-            std::vector<PublicKey> keys ({ randomNode (), randomNode () });
-            hash_set<NodeID> activeValidators;
-            std::vector<std::string> cfgKeys ({
-                toBase58 (TokenType::NodePublic, keys[0]),
-                toBase58 (TokenType::NodePublic, keys[1])});
-
-            BEAST_EXPECT(trustedKeys->load (
-                emptyLocalKey, cfgKeys, cfgPublishers));
-
-            TrustChanges changes =
-                trustedKeys->updateTrusted(activeValidators);
-            BEAST_EXPECT(changes.removed.empty());
-            BEAST_EXPECT(changes.added == asNodeIDs({keys[0], keys[1]}));
-
-            BEAST_EXPECT(trustedKeys->quorum () == 2);
-            for (auto const& key : keys)
-                BEAST_EXPECT(trustedKeys->trusted (key));
-        }
         {
             // Should use custom minimum quorum
             std::size_t const minQuorum = 1;
@@ -738,10 +702,24 @@ private:
             auto trustedKeys = std::make_unique <ValidatorList> (
                 manifests, manifests, env.timeKeeper(), beast::Journal (), minQuorum);
 
-            auto const node = randomNode ();
-            std::vector<std::string> cfgKeys ({
-                toBase58 (TokenType::NodePublic, node)});
+            std::size_t n = 10;
+            std::vector<std::string> cfgKeys;
+            cfgKeys.reserve(n);
+            hash_set<NodeID> expectedTrusted;
             hash_set<NodeID> activeValidators;
+            NodeID toBeSeen;
+
+            while (cfgKeys.size () < n)
+            {
+                auto const valKey = randomNode();
+                cfgKeys.push_back (toBase58(
+                    TokenType::NodePublic, valKey));
+                expectedTrusted.emplace (calcNodeID(valKey));
+                if (cfgKeys.size () < std::ceil(n*0.8f))
+                    activeValidators.emplace (calcNodeID(valKey));
+                else if (cfgKeys.size () < std::ceil(n*0.8f))
+                    toBeSeen = calcNodeID(valKey);
+            }
 
             BEAST_EXPECT(trustedKeys->load (
                 emptyLocalKey, cfgKeys, cfgPublishers));
@@ -749,39 +727,15 @@ private:
             TrustChanges changes =
                 trustedKeys->updateTrusted(activeValidators);
             BEAST_EXPECT(changes.removed.empty());
-            BEAST_EXPECT(changes.added == asNodeIDs({node}));
+            BEAST_EXPECT(changes.added == expectedTrusted);
             BEAST_EXPECT(trustedKeys->quorum () == minQuorum);
 
-            activeValidators.emplace (calcNodeID(node));
+            // Use normal quorum when seen validators >= quorum
+            activeValidators.emplace (toBeSeen);
             changes = trustedKeys->updateTrusted(activeValidators);
             BEAST_EXPECT(changes.removed.empty());
             BEAST_EXPECT(changes.added.empty());
-            BEAST_EXPECT(trustedKeys->quorum () == 1);
-        }
-        {
-            // Increase quorum when running as an unlisted validator
-            auto trustedKeys = std::make_unique <ValidatorList> (
-                manifests, manifests, env.timeKeeper(), beast::Journal ());
-
-            std::vector<PublicKey> keys ({ randomNode (), randomNode () });
-            hash_set<NodeID> activeValidators (asNodeIDs({ keys[0] }));
-            std::vector<std::string> cfgKeys ({
-                toBase58 (TokenType::NodePublic, keys[0]),
-                toBase58 (TokenType::NodePublic, keys[1])});
-
-            auto const localKey = randomNode ();
-            BEAST_EXPECT(trustedKeys->load (
-                localKey, cfgKeys, cfgPublishers));
-
-            TrustChanges changes =
-                trustedKeys->updateTrusted(activeValidators);
-            BEAST_EXPECT(changes.removed.empty());
-            BEAST_EXPECT(
-                changes.added == asNodeIDs({keys[0], keys[1], localKey}));
-            BEAST_EXPECT(trustedKeys->quorum () == 2);
-
-            // local validator key is always trusted
-            BEAST_EXPECT(trustedKeys->trusted (localKey));
+            BEAST_EXPECT(trustedKeys->quorum () == std::ceil(n * 0.8f));
         }
         {
             // Remove expired published list
@@ -894,8 +848,7 @@ private:
                 BEAST_EXPECT(changes.removed.empty());
                 BEAST_EXPECT(changes.added == asNodeIDs({valKey}));
                 BEAST_EXPECT(trustedKeys->quorum () ==
-                    ((cfgKeys.size() <= 6) ? cfgKeys.size()/2 + 1 :
-                        cfgKeys.size() * 2/3 + 1));
+                    std::ceil(cfgKeys.size() * 0.8f));
                 for (auto const& key : activeKeys)
                     BEAST_EXPECT(trustedKeys->trusted (key));
             }
@@ -933,15 +886,14 @@ private:
                         changes.added == asNodeIDs({localKey, valKey}));
 
                 BEAST_EXPECT(trustedKeys->quorum () ==
-                    ((cfgKeys.size() <= 6) ? cfgKeys.size()/2 + 1 :
-                        (cfgKeys.size() + 1) * 2/3 + 1));
+                    std::ceil(cfgKeys.size() * 0.8f));
 
                 for (auto const& key : activeKeys)
                     BEAST_EXPECT(trustedKeys->trusted (key));
             }
         }
         {
-            // Trusted set should be trimmed with multiple validator lists
+            // Trusted set should include all validators from multiple lists
             ManifestCache manifests;
             auto trustedKeys = std::make_unique <ValidatorList> (
                 manifests, manifests, env.timeKeeper(), beast::Journal ());
@@ -994,25 +946,17 @@ private:
             TrustChanges changes =
                 trustedKeys->updateTrusted(activeValidators);
 
-            // Minimum quorum should be used
-            BEAST_EXPECT(trustedKeys->quorum () == (valKeys.size() * 2/3 + 1));
+            BEAST_EXPECT(trustedKeys->quorum () ==
+                std::ceil(valKeys.size() * 0.8f));
 
             hash_set<NodeID> added;
-            std::size_t nTrusted = 0;
             for (auto const& val : valKeys)
             {
-                if (trustedKeys->trusted (val.masterPublic))
-                {
-                    added.insert(calcNodeID(val.masterPublic));
-                    ++nTrusted;
-                }
+                BEAST_EXPECT(trustedKeys->trusted (val.masterPublic));
+                added.insert(calcNodeID(val.masterPublic));
             }
             BEAST_EXPECT(changes.added == added);
             BEAST_EXPECT(changes.removed.empty());
-
-            // The number of trusted keys should be 125% of the minimum quorum
-            BEAST_EXPECT(nTrusted ==
-                static_cast<std::size_t>(trustedKeys->quorum () * 5 / 4));
         }
     }