Make minimum quorum Byzantine fault tolerant (RIPD-1461)

src/ripple/app/misc/ValidatorList.h

@@ -308,9 +308,6 @@ public:
     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
 
@@ -341,6 +338,15 @@ private:
     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);
 };
 
 //------------------------------------------------------------------------------
@@ -399,15 +405,10 @@ ValidatorList::onConsensusStart (
         }
     }
 
-    // This quorum guarantees sufficient overlap with the trusted sets of other
+    // This minimum quorum guarantees safe overlap with the trusted sets of
-    // nodes using the same set of published lists.
+    // other nodes using the same set of published lists.
-    std::size_t quorum = keyListings_.size()/2 + 1;
+    std::size_t quorum = calculateMinimumQuorum (keyListings_.size(),
-
+        localPubKey_.size() && !localKeyListed);
-    // 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;
 
     JLOG (j_.debug()) <<
         rankedKeys.size() << "  of " << keyListings_.size() <<
@@ -415,25 +416,31 @@ ValidatorList::onConsensusStart (
 
     auto size = rankedKeys.size();
 
-    // Use all eligible keys if there is less than 10 listed validators or
+    // Do not require 80% quorum for less than 10 trusted validators
-    // only one trusted list
+    if (rankedKeys.size() >= 10)
-    if (size < 10 || publisherLists_.size() == 1)
     {
-        // Try to raise the quorum toward or above 80% of the trusted set
+        // Use all eligible keys if there is only one trusted list
-        std::size_t const targetQuorum = ValidatorList::calculateQuorum (size);
+        if (publisherLists_.size() == 1)
-        if (targetQuorum > quorum)
+        {
-            quorum = targetQuorum;
+            // Try to raise the quorum to at least 80% of the trusted set
-    }
+            quorum = std::max(quorum, size - size / 5);
-    else
+        }
-    {
+        else
-        // reduce the trusted set size so that the quorum represents
+        {
-        // at least 80%
+            // Reduce the trusted set size so that the quorum represents
-        size = quorum * 1.25;
+            // at least 80%
+            size = quorum * 1.25;
+        }
     }
 
     if (minimumQuorum_ && (seenValidators.empty() ||
             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

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

@@ -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.
+    // Only require 51% quorum for small number of validators to facilitate
-    constexpr std::array<std::size_t, 10> quorum{{ 0, 1, 2, 2, 3, 3, 4, 5, 6, 7 }};
+    // bootstrapping a network.
+    if (nListedKeys <= 5)
+        return nListedKeys/2 + 1;
 
-    if (nTrustedKeys < quorum.size())
+    // The number of listed validators is increased to preserve the safety
-        return quorum[nTrustedKeys];
+    // guarantee for two unlisted validators using the same set of listed
+    // validators.
+    if (unlistedLocal)
+        ++nListedKeys;
 
-    return nTrustedKeys - nTrustedKeys / 5;
+    // 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;
 }
 
 } // ripple

src/test/app/ValidatorList_test.cpp

@@ -124,21 +124,6 @@ private:
         }
     }
 
-    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 ()
     {
@@ -522,7 +507,7 @@ private:
             // 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)
             {
@@ -538,6 +523,16 @@ private:
                 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
@@ -571,7 +566,7 @@ private:
                 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 +579,11 @@ private:
             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 +607,7 @@ private:
             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 +694,7 @@ private:
                 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 +764,8 @@ private:
                     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));
             }
@@ -799,16 +794,80 @@ private:
                     localKey, cfgKeys, cfgPublishers));
                 trustedKeys->onConsensusStart (activeValidators);
 
-                // When running as an unlisted validator,
+                BEAST_EXPECT(trustedKeys->quorum () ==
-                // the quorum is incremented by 1 for 3 or 5 trusted validators.
+                    (cfgKeys.size() <= 5) ? cfgKeys.size()/2 + 1 :
-                auto expectedQuorum = ValidatorList::calculateQuorum(cfgKeys.size());
+                        (cfgKeys.size() + 1) * 2/3 + 1);
-                if (cfgKeys.size() == 3 || cfgKeys.size() == 5)
+
-                    ++expectedQuorum;
-                BEAST_EXPECT(trustedKeys->quorum () == expectedQuorum);
                 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:
@@ -816,7 +875,6 @@ public:
     run() override
     {
         testGenesisQuorum ();
-        testCalculateQuorum ();
         testConfigLoad ();
         testApplyList ();
         testUpdate ();