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>
@@ -97,25 +98,68 @@ struct AmendmentSet
 private:
     // How many yes votes each amendment received
     hash_map<uint256, int> votes_;
+    Rules const& rules_;
 
 public:
-    // number of trusted validations
-    int mTrustedValidations = 0;
-
-    // number of votes needed
-    int mThreshold = 0;
+    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]; });
+                }
+
+                ++mTrustedValidations;
+            }
+        }
 
-    AmendmentSet() = default;
+        mThreshold = rules_.enabled(fixAmendmentMajorityCalc)
+            ? std::max(
+                  1L,
+                  (mTrustedValidations *
+                   amendmentSuperMajorityThresholdPre3396.num) /
+                      amendmentSuperMajorityThresholdPre3396.den)
+            : std::max(
+                  1L,
+                  (mTrustedValidations *
+                   amendmentSuperMajorityThresholdPost3396.num) /
+                      amendmentSuperMajorityThresholdPost3396.den);
+    }
 
-    void
-    tally(std::set<uint256> const& amendments)
+    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".
+        if (!rules_.enabled(fixAmendmentMajorityCalc))
+            return it->second >= mThreshold;
+
+        return it->second > mThreshold;
     }
 
+    // number of trusted validations
+    int mTrustedValidations = 0;
+
+    // number of votes needed
+    int mThreshold = 0;
+
     int
     votes(uint256 const& amendment) const
     {
@@ -138,7 +182,7 @@ struct AmendmentSet
 */
 class AmendmentTableImpl final : public AmendmentTable
 {
-protected:
+private:
     mutable std::mutex mutex_;
 
     hash_map<uint256, AmendmentState> amendmentMap_;
@@ -147,10 +191,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 +227,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 +276,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 +287,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 +497,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,27 +507,7 @@ 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;
-
-            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);
+    auto vote = std::make_unique<AmendmentSet>(rules, valSet);
 
     JLOG(j_.debug()) << "Received " << vote->mTrustedValidations
                      << " trusted validations, threshold is: "
@@ -507,8 +524,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);
@@ -666,14 +682,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

@@ -23,6 +23,7 @@
 #include <ripple/basics/BasicConfig.h>
 #include <ripple/basics/FeeUnits.h>
 #include <ripple/basics/base_uint.h>
+#include <ripple/basics/chrono.h>
 #include <ripple/beast/net/IPEndpoint.h>
 #include <ripple/beast/utility/Journal.h>
 #include <ripple/protocol/SystemParameters.h>  // VFALCO Breaks levelization
@@ -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
 

src/ripple/protocol/SystemParameters.h

@@ -21,6 +21,7 @@
 #define RIPPLE_PROTOCOL_SYSTEMPARAMETERS_H_INCLUDED
 
 #include <ripple/basics/XRPAmount.h>
+#include <ripple/basics/chrono.h>
 #include <cstdint>
 #include <string>
 
@@ -59,6 +60,18 @@ systemCurrencyCode()
 /** The XRP ledger network's earliest allowed sequence */
 static std::uint32_t constexpr XRP_LEDGER_EARLIEST_SEQ{32570};
 
+/** The minimum amount of support an amendment should have.
+
+    @note This value is used by legacy code and will become obsolete
+          once the fixAmendmentMajorityCalc amendment activates.
+*/
+constexpr std::ratio<205, 256> amendmentSuperMajorityThresholdPre3396;
+
+constexpr std::ratio<80, 100> amendmentSuperMajorityThresholdPost3396;
+
+/** The minimum amount of time an amendment must hold a majority */
+constexpr std::chrono::seconds const defaultAmendmentMajorityTime = weeks{2};
+
 }  // namespace ripple
 
 #endif

src/ripple/protocol/impl/Feature.cpp

@@ -130,7 +130,8 @@ detail::supportedAmendments()
         "fixQualityUpperBound",
         "RequireFullyCanonicalSig",
         "fix1781",
-        "HardenedValidations"};
+        "HardenedValidations",
+        "fixAmendmentMajorityCalc"};
     return supported;
 }
 
@@ -181,7 +182,8 @@ uint256 const
     fixQualityUpperBound            = *getRegisteredFeature("fixQualityUpperBound"),
     featureRequireFullyCanonicalSig = *getRegisteredFeature("RequireFullyCanonicalSig"),
     fix1781                         = *getRegisteredFeature("fix1781"),
-    featureHardenedValidations      = *getRegisteredFeature("HardenedValidations");
+    featureHardenedValidations      = *getRegisteredFeature("HardenedValidations"),
+    fixAmendmentMajorityCalc        = *getRegisteredFeature("fixAmendmentMajorityCalc");
 
 // The following amendments have been active for at least two years. Their
 // pre-amendment code has been removed and the identifiers are deprecated.