Amendment table clean up (Improve amendment processing and activation logic)

src/ripple/app/main/Application.cpp

@@ -77,9 +77,6 @@
 
 namespace ripple {
 
-// 204/256 about 80%
-static int const MAJORITY_FRACTION(204);
-
 //------------------------------------------------------------------------------
 
 namespace detail {
@@ -1533,8 +1530,7 @@ ApplicationImp::setup()
         Section enabledAmendments = config_->section(SECTION_AMENDMENTS);
 
         m_amendmentTable = make_AmendmentTable(
-            weeks{2},
-            MAJORITY_FRACTION,
+            config().AMENDMENT_MAJORITY_TIME,
             supportedAmendments,
             enabledAmendments,
             config_->section(SECTION_VETO_AMENDMENTS),

src/ripple/app/misc/AmendmentTable.h

@@ -99,6 +99,7 @@ class AmendmentTable
     // inject pseudo-transactions
     virtual std::map<uint256, std::uint32_t>
     doVoting(
+        Rules const& rules,
         NetClock::time_point closeTime,
         std::set<uint256> const& enabledAmendments,
         majorityAmendments_t const& majorityAmendments,
@@ -130,6 +131,7 @@ class AmendmentTable
     {
         // Ask implementation what to do
         auto actions = doVoting(
+            lastClosedLedger->rules(),
             lastClosedLedger->parentCloseTime(),
             getEnabledAmendments(*lastClosedLedger),
             getMajorityAmendments(*lastClosedLedger),
@@ -164,7 +166,6 @@ class AmendmentTable
 std::unique_ptr<AmendmentTable>
 make_AmendmentTable(
     std::chrono::seconds majorityTime,
-    int majorityFraction,
     Section const& supported,
     Section const& enabled,
     Section const& vetoed,

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

@@ -21,6 +21,7 @@
 #include <ripple/app/misc/AmendmentTable.h>
 #include <ripple/core/ConfigSections.h>
 #include <ripple/core/DatabaseCon.h>
+#include <ripple/protocol/Feature.h>
 #include <ripple/protocol/STValidation.h>
 #include <ripple/protocol/TxFlags.h>
 #include <ripple/protocol/jss.h>
@@ -92,28 +93,74 @@ struct AmendmentState
 };
 
 /** The status of all amendments requested in a given window. */
-struct AmendmentSet
+class AmendmentSet
 {
 private:
     // How many yes votes each amendment received
     hash_map<uint256, int> votes_;
-
-public:
+    Rules const& rules_;
     // number of trusted validations
-    int mTrustedValidations = 0;
-
+    int trustedValidations_ = 0;
     // number of votes needed
-    int mThreshold = 0;
+    int threshold_ = 0;
 
-    AmendmentSet() = default;
+public:
+    AmendmentSet(
+        Rules const& rules,
+        std::vector<std::shared_ptr<STValidation>> const& valSet)
+        : rules_(rules)
+    {
+        // process validations for ledger before flag ledger
+        for (auto const& val : valSet)
+        {
+            if (val->isTrusted())
+            {
+                if (val->isFieldPresent(sfAmendments))
+                {
+                    auto const choices = val->getFieldV256(sfAmendments);
+                    std::for_each(
+                        choices.begin(),
+                        choices.end(),
+                        [&](auto const& amendment) { ++votes_[amendment]; });
+                }
+
+                ++trustedValidations_;
+            }
+        }
 
-    void
-    tally(std::set<uint256> const& amendments)
+        threshold_ = !rules_.enabled(fixAmendmentMajorityCalc)
+            ? std::max(
+                  1L,
+                  static_cast<long>(
+                      (trustedValidations_ *
+                       preFixAmendmentMajorityCalcThreshold.num) /
+                      preFixAmendmentMajorityCalcThreshold.den))
+            : std::max(
+                  1L,
+                  static_cast<long>(
+                      (trustedValidations_ *
+                       postFixAmendmentMajorityCalcThreshold.num) /
+                      postFixAmendmentMajorityCalcThreshold.den));
+    }
+
+    bool
+    passes(uint256 const& amendment) const
     {
-        ++mTrustedValidations;
+        auto const& it = votes_.find(amendment);
 
-        for (auto const& amendment : amendments)
-            ++votes_[amendment];
+        if (it == votes_.end())
+            return false;
+
+        // Before this fix, it was possible for an amendment to activate with a
+        // percentage slightly less than 80% because we compared for "greater
+        // than or equal to" instead of strictly "greater than".
+        // One validator is an exception, otherwise it is not possible
+        // to gain majority.
+        if (!rules_.enabled(fixAmendmentMajorityCalc) ||
+            trustedValidations_ == 1)
+            return it->second >= threshold_;
+
+        return it->second > threshold_;
     }
 
     int
@@ -126,6 +173,18 @@ struct AmendmentSet
 
         return it->second;
     }
+
+    int
+    trustedValidations() const
+    {
+        return trustedValidations_;
+    }
+
+    int
+    threshold() const
+    {
+        return threshold_;
+    }
 };
 
 //------------------------------------------------------------------------------
@@ -138,7 +197,7 @@ struct AmendmentSet
 */
 class AmendmentTableImpl final : public AmendmentTable
 {
-protected:
+private:
     mutable std::mutex mutex_;
 
     hash_map<uint256, AmendmentState> amendmentMap_;
@@ -147,10 +206,6 @@ class AmendmentTableImpl final : public AmendmentTable
     // Time that an amendment must hold a majority for
     std::chrono::seconds const majorityTime_;
 
-    // The amount of support that an amendment must receive
-    // 0 = 0% and 256 = 100%
-    int const majorityFraction_;
-
     // The results of the last voting round - may be empty if
     // we haven't participated in one yet.
     std::unique_ptr<AmendmentSet> lastVote_;
@@ -187,7 +242,6 @@ class AmendmentTableImpl final : public AmendmentTable
 public:
     AmendmentTableImpl(
         std::chrono::seconds majorityTime,
-        int majorityFraction,
         Section const& supported,
         Section const& enabled,
         Section const& vetoed,
@@ -237,6 +291,7 @@ class AmendmentTableImpl final : public AmendmentTable
 
     std::map<uint256, std::uint32_t>
     doVoting(
+        Rules const& rules,
         NetClock::time_point closeTime,
         std::set<uint256> const& enabledAmendments,
         majorityAmendments_t const& majorityAmendments,
@@ -247,19 +302,15 @@ class AmendmentTableImpl final : public AmendmentTable
 
 AmendmentTableImpl::AmendmentTableImpl(
     std::chrono::seconds majorityTime,
-    int majorityFraction,
     Section const& supported,
     Section const& enabled,
     Section const& vetoed,
     beast::Journal journal)
     : lastUpdateSeq_(0)
     , majorityTime_(majorityTime)
-    , majorityFraction_(majorityFraction)
     , unsupportedEnabled_(false)
     , j_(journal)
 {
-    assert(majorityFraction_ != 0);
-
     std::lock_guard sl(mutex_);
 
     for (auto const& a : parseSection(supported))
@@ -461,6 +512,7 @@ AmendmentTableImpl::getDesired() const
 
 std::map<uint256, std::uint32_t>
 AmendmentTableImpl::doVoting(
+    Rules const& rules,
     NetClock::time_point closeTime,
     std::set<uint256> const& enabledAmendments,
     majorityAmendments_t const& majorityAmendments,
@@ -470,31 +522,11 @@ AmendmentTableImpl::doVoting(
                      << ": " << enabledAmendments.size() << ", "
                      << majorityAmendments.size() << ", " << valSet.size();
 
-    auto vote = std::make_unique<AmendmentSet>();
-
-    // process validations for ledger before flag ledger
-    for (auto const& val : valSet)
-    {
-        if (val->isTrusted())
-        {
-            std::set<uint256> ballot;
+    auto vote = std::make_unique<AmendmentSet>(rules, valSet);
 
-            if (val->isFieldPresent(sfAmendments))
-            {
-                auto const choices = val->getFieldV256(sfAmendments);
-                ballot.insert(choices.begin(), choices.end());
-            }
-
-            vote->tally(ballot);
-        }
-    }
-
-    vote->mThreshold =
-        std::max(1, (vote->mTrustedValidations * majorityFraction_) / 256);
-
-    JLOG(j_.debug()) << "Received " << vote->mTrustedValidations
+    JLOG(j_.debug()) << "Received " << vote->trustedValidations()
                      << " trusted validations, threshold is: "
-                     << vote->mThreshold;
+                     << vote->threshold();
 
     // Map of amendments to the action to be taken for each one. The action is
     // the value of the flags in the pseudo-transaction
@@ -507,8 +539,7 @@ AmendmentTableImpl::doVoting(
     {
         NetClock::time_point majorityTime = {};
 
-        bool const hasValMajority =
-            (vote->votes(entry.first) >= vote->mThreshold);
+        bool const hasValMajority = vote->passes(entry.first);
 
         {
             auto const it = majorityAmendments.find(entry.first);
@@ -614,18 +645,15 @@ AmendmentTableImpl::injectJson(
 
     if (!fs.enabled && lastVote_)
     {
-        auto const votesTotal = lastVote_->mTrustedValidations;
-        auto const votesNeeded = lastVote_->mThreshold;
+        auto const votesTotal = lastVote_->trustedValidations();
+        auto const votesNeeded = lastVote_->threshold();
         auto const votesFor = lastVote_->votes(id);
 
         v[jss::count] = votesFor;
         v[jss::validations] = votesTotal;
 
         if (votesNeeded)
-        {
-            v[jss::vote] = votesFor * 256 / votesNeeded;
             v[jss::threshold] = votesNeeded;
-        }
     }
 }
 
@@ -666,14 +694,13 @@ AmendmentTableImpl::getJson(uint256 const& amendmentID) const
 std::unique_ptr<AmendmentTable>
 make_AmendmentTable(
     std::chrono::seconds majorityTime,
-    int majorityFraction,
     Section const& supported,
     Section const& enabled,
     Section const& vetoed,
     beast::Journal journal)
 {
     return std::make_unique<AmendmentTableImpl>(
-        majorityTime, majorityFraction, supported, enabled, vetoed, journal);
+        majorityTime, supported, enabled, vetoed, journal);
 }
 
 }  // namespace ripple

src/ripple/core/Config.h

@@ -31,6 +31,7 @@
 #include <boost/lexical_cast.hpp>
 #include <boost/optional.hpp>
 #include <algorithm>
+#include <chrono>
 #include <cstdint>
 #include <map>
 #include <string>
@@ -172,6 +173,9 @@ class Config : public BasicConfig
     // Compression
     bool COMPRESSION = false;
 
+    // Amendment majority time
+    std::chrono::seconds AMENDMENT_MAJORITY_TIME = defaultAmendmentMajorityTime;
+
     // Thread pool configuration
     std::size_t WORKERS = 0;
 

src/ripple/core/ConfigSections.h

@@ -60,6 +60,7 @@ struct ConfigSection
 #define SECTION_INSIGHT "insight"
 #define SECTION_IPS "ips"
 #define SECTION_IPS_FIXED "ips_fixed"
+#define SECTION_AMENDMENT_MAJORITY_TIME "amendment_majority_time"
 #define SECTION_NETWORK_QUORUM "network_quorum"
 #define SECTION_NODE_SEED "node_seed"
 #define SECTION_NODE_SIZE "node_size"

src/ripple/core/impl/Config.cpp

@@ -480,6 +480,37 @@ Config::loadFromString(std::string const& fileContents)
     if (getSingleSection(secConfig, SECTION_COMPRESSION, strTemp, j_))
         COMPRESSION = beast::lexicalCastThrow<bool>(strTemp);
 
+    if (getSingleSection(
+            secConfig, SECTION_AMENDMENT_MAJORITY_TIME, strTemp, j_))
+    {
+        using namespace std::chrono;
+        boost::regex const re(
+            "^\\s*(\\d+)\\s*(minutes|hours|days|weeks)\\s*(\\s+.*)?$");
+        boost::smatch match;
+        if (!boost::regex_match(strTemp, match, re))
+            Throw<std::runtime_error>(
+                "Invalid " SECTION_AMENDMENT_MAJORITY_TIME
+                ", must be: [0-9]+ [minutes|hours|days|weeks]");
+
+        std::uint32_t duration =
+            beast::lexicalCastThrow<std::uint32_t>(match[1].str());
+
+        if (boost::iequals(match[2], "minutes"))
+            AMENDMENT_MAJORITY_TIME = minutes(duration);
+        else if (boost::iequals(match[2], "hours"))
+            AMENDMENT_MAJORITY_TIME = hours(duration);
+        else if (boost::iequals(match[2], "days"))
+            AMENDMENT_MAJORITY_TIME = days(duration);
+        else if (boost::iequals(match[2], "weeks"))
+            AMENDMENT_MAJORITY_TIME = weeks(duration);
+
+        if (AMENDMENT_MAJORITY_TIME < minutes(15))
+            Throw<std::runtime_error>(
+                "Invalid " SECTION_AMENDMENT_MAJORITY_TIME
+                ", the minimum amount of time an amendment must hold a "
+                "majority is 15 minutes");
+    }
+
     // Do not load trusted validator configuration for standalone mode
     if (!RUN_STANDALONE)
     {

src/ripple/protocol/Feature.h

@@ -111,7 +111,8 @@ class FeatureCollections
         "RequireFullyCanonicalSig",
         "fix1781",  // XRPEndpointSteps should be included in the circular
                     // payment check
-        "HardenedValidations"};
+        "HardenedValidations",
+        "fixAmendmentMajorityCalc"};  // Fix Amendment majority calculation
 
     std::vector<uint256> features;
     boost::container::flat_map<uint256, std::size_t> featureToIndex;
@@ -367,6 +368,7 @@ extern uint256 const fixQualityUpperBound;
 extern uint256 const featureRequireFullyCanonicalSig;
 extern uint256 const fix1781;
 extern uint256 const featureHardenedValidations;
+extern uint256 const fixAmendmentMajorityCalc;
 
 }  // namespace ripple