From 29b74b2d90e250f82b8933b2d7f6caffd67ec8c4 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Mon, 18 May 2020 17:40:53 -0400 Subject: [PATCH 01/12] Use base 10 for majority vote calculation Add majority timer configuration FIXES: #3396 --- src/ripple/app/main/Application.cpp | 8 ++- src/ripple/app/misc/AmendmentTable.h | 10 +++- src/ripple/app/misc/impl/AmendmentTable.cpp | 26 +++++---- src/ripple/core/Config.h | 4 ++ src/ripple/core/ConfigSections.h | 1 + src/ripple/core/impl/Config.cpp | 25 ++++++++ src/ripple/protocol/Feature.h | 4 +- src/ripple/protocol/impl/Feature.cpp | 6 +- src/test/app/AmendmentTable_test.cpp | 63 +++++++++++++-------- src/test/core/Config_test.cpp | 63 +++++++++++++++++++++ 10 files changed, 170 insertions(+), 40 deletions(-) diff --git a/src/ripple/app/main/Application.cpp b/src/ripple/app/main/Application.cpp index 3b37c736963..441f673cce1 100644 --- a/src/ripple/app/main/Application.cpp +++ b/src/ripple/app/main/Application.cpp @@ -78,7 +78,9 @@ namespace ripple { // 204/256 about 80% -static int const MAJORITY_FRACTION(204); +static int const MAJORITY_FRACTION_OLD(204); +// 8/10 = 80% +static int const MAJORITY_FRACTION(8); //------------------------------------------------------------------------------ @@ -1533,8 +1535,8 @@ ApplicationImp::setup() Section enabledAmendments = config_->section(SECTION_AMENDMENTS); m_amendmentTable = make_AmendmentTable( - weeks{2}, - MAJORITY_FRACTION, + config().AMENDMENT_MAJORITY_TIME, + {MAJORITY_FRACTION_OLD, MAJORITY_FRACTION}, supportedAmendments, enabledAmendments, config_->section(SECTION_VETO_AMENDMENTS), diff --git a/src/ripple/app/misc/AmendmentTable.h b/src/ripple/app/misc/AmendmentTable.h index 0ac55858074..cab519c86d7 100644 --- a/src/ripple/app/misc/AmendmentTable.h +++ b/src/ripple/app/misc/AmendmentTable.h @@ -27,6 +27,12 @@ namespace ripple { +struct MajorityFraction +{ + int old_ = 0; + int new_ = 0; +}; + /** The amendment table stores the list of enabled and potential amendments. Individuals amendments are voted on by validators during the consensus process. @@ -99,6 +105,7 @@ class AmendmentTable // inject pseudo-transactions virtual std::map doVoting( + Rules const& rules, NetClock::time_point closeTime, std::set const& enabledAmendments, majorityAmendments_t const& majorityAmendments, @@ -130,6 +137,7 @@ class AmendmentTable { // Ask implementation what to do auto actions = doVoting( + lastClosedLedger->rules(), lastClosedLedger->parentCloseTime(), getEnabledAmendments(*lastClosedLedger), getMajorityAmendments(*lastClosedLedger), @@ -164,7 +172,7 @@ class AmendmentTable std::unique_ptr make_AmendmentTable( std::chrono::seconds majorityTime, - int majorityFraction, + MajorityFraction const& majorityFraction, Section const& supported, Section const& enabled, Section const& vetoed, diff --git a/src/ripple/app/misc/impl/AmendmentTable.cpp b/src/ripple/app/misc/impl/AmendmentTable.cpp index 4d499ea9172..dcf64509d0a 100644 --- a/src/ripple/app/misc/impl/AmendmentTable.cpp +++ b/src/ripple/app/misc/impl/AmendmentTable.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -148,8 +149,8 @@ class AmendmentTableImpl final : public AmendmentTable std::chrono::seconds const majorityTime_; // The amount of support that an amendment must receive - // 0 = 0% and 256 = 100% - int const majorityFraction_; + // 0 = 0% and 256 = 100%(old) 8 = 100%(new) + MajorityFraction const majorityFraction_; // The results of the last voting round - may be empty if // we haven't participated in one yet. @@ -187,7 +188,7 @@ class AmendmentTableImpl final : public AmendmentTable public: AmendmentTableImpl( std::chrono::seconds majorityTime, - int majorityFraction, + MajorityFraction const& majorityFraction, Section const& supported, Section const& enabled, Section const& vetoed, @@ -237,6 +238,7 @@ class AmendmentTableImpl final : public AmendmentTable std::map doVoting( + Rules const& rules, NetClock::time_point closeTime, std::set const& enabledAmendments, majorityAmendments_t const& majorityAmendments, @@ -247,7 +249,7 @@ class AmendmentTableImpl final : public AmendmentTable AmendmentTableImpl::AmendmentTableImpl( std::chrono::seconds majorityTime, - int majorityFraction, + MajorityFraction const& majorityFraction, Section const& supported, Section const& enabled, Section const& vetoed, @@ -258,7 +260,7 @@ AmendmentTableImpl::AmendmentTableImpl( , unsupportedEnabled_(false) , j_(journal) { - assert(majorityFraction_ != 0); + assert(majorityFraction_.old_ != 0 && majorityFraction_.new_ != 0); std::lock_guard sl(mutex_); @@ -461,6 +463,7 @@ AmendmentTableImpl::getDesired() const std::map AmendmentTableImpl::doVoting( + Rules const& rules, NetClock::time_point closeTime, std::set const& enabledAmendments, majorityAmendments_t const& majorityAmendments, @@ -489,8 +492,10 @@ AmendmentTableImpl::doVoting( } } - vote->mThreshold = - std::max(1, (vote->mTrustedValidations * majorityFraction_) / 256); + vote->mThreshold = rules.enabled(ripple::fix3396) + ? std::max(1, (vote->mTrustedValidations * majorityFraction_.new_) / 10) + : std::max( + 1, (vote->mTrustedValidations * majorityFraction_.old_) / 256); JLOG(j_.debug()) << "Received " << vote->mTrustedValidations << " trusted validations, threshold is: " @@ -507,8 +512,9 @@ AmendmentTableImpl::doVoting( { NetClock::time_point majorityTime = {}; - bool const hasValMajority = - (vote->votes(entry.first) >= vote->mThreshold); + bool const hasValMajority = rules.enabled(ripple::fix3396) + ? (vote->votes(entry.first) > vote->mThreshold) + : (vote->votes(entry.first) >= vote->mThreshold); { auto const it = majorityAmendments.find(entry.first); @@ -666,7 +672,7 @@ AmendmentTableImpl::getJson(uint256 const& amendmentID) const std::unique_ptr make_AmendmentTable( std::chrono::seconds majorityTime, - int majorityFraction, + MajorityFraction const& majorityFraction, Section const& supported, Section const& enabled, Section const& vetoed, diff --git a/src/ripple/core/Config.h b/src/ripple/core/Config.h index 7943906fdae..6fede8ae0a3 100644 --- a/src/ripple/core/Config.h +++ b/src/ripple/core/Config.h @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include // VFALCO Breaks levelization @@ -172,6 +173,9 @@ class Config : public BasicConfig // Compression bool COMPRESSION = false; + // Amendment majority time + std::chrono::seconds AMENDMENT_MAJORITY_TIME = weeks{2}; + // Thread pool configuration std::size_t WORKERS = 0; diff --git a/src/ripple/core/ConfigSections.h b/src/ripple/core/ConfigSections.h index c9b61c2cb2b..3aae9774d10 100644 --- a/src/ripple/core/ConfigSections.h +++ b/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" diff --git a/src/ripple/core/impl/Config.cpp b/src/ripple/core/impl/Config.cpp index f12ba7dbcee..5fb0e75084e 100644 --- a/src/ripple/core/impl/Config.cpp +++ b/src/ripple/core/impl/Config.cpp @@ -480,6 +480,31 @@ Config::loadFromString(std::string const& fileContents) if (getSingleSection(secConfig, SECTION_COMPRESSION, strTemp, j_)) COMPRESSION = beast::lexicalCastThrow(strTemp); + if (getSingleSection( + secConfig, SECTION_AMENDMENT_MAJORITY_TIME, strTemp, j_)) + { + using namespace std::chrono; + boost::regex const re( + "^\\s*(\\d+)\\s*(seconds|minutes|hours|days|weeks)\\s*(\\s+.*)?$"); + boost::smatch match; + if (!boost::regex_match(strTemp, match, re)) + Throw( + "Invalid " SECTION_AMENDMENT_MAJORITY_TIME + ", must be: [0-9]+ [seconds|minutes|hours|days|weeks]"); + std::uint32_t duration = + beast::lexicalCastThrow(match[1].str()); + if (boost::iequals(match[2], "seconds")) + AMENDMENT_MAJORITY_TIME = seconds(duration); + else 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); + } + // Do not load trusted validator configuration for standalone mode if (!RUN_STANDALONE) { diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index e47778a03a4..47230f0ab0f 100644 --- a/src/ripple/protocol/Feature.h +++ b/src/ripple/protocol/Feature.h @@ -111,7 +111,8 @@ class FeatureCollections "RequireFullyCanonicalSig", "fix1781", // XRPEndpointSteps should be included in the circular // payment check - "HardenedValidations"}; + "HardenedValidations", + "fix3396"}; // Fix Amendment majority calculation std::vector features; boost::container::flat_map featureToIndex; @@ -367,6 +368,7 @@ extern uint256 const fixQualityUpperBound; extern uint256 const featureRequireFullyCanonicalSig; extern uint256 const fix1781; extern uint256 const featureHardenedValidations; +extern uint256 const fix3396; } // namespace ripple diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index b8cac9b8d68..d990f7c5d21 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -130,7 +130,8 @@ detail::supportedAmendments() "fixQualityUpperBound", "RequireFullyCanonicalSig", "fix1781", - "HardenedValidations"}; + "HardenedValidations", + "fix3396"}; return supported; } @@ -181,7 +182,8 @@ uint256 const fixQualityUpperBound = *getRegisteredFeature("fixQualityUpperBound"), featureRequireFullyCanonicalSig = *getRegisteredFeature("RequireFullyCanonicalSig"), fix1781 = *getRegisteredFeature("fix1781"), - featureHardenedValidations = *getRegisteredFeature("HardenedValidations"); + featureHardenedValidations = *getRegisteredFeature("HardenedValidations"), + fix3396 = *getRegisteredFeature("fix3396"); // The following amendments have been active for at least two years. Their // pre-amendment code has been removed and the identifiers are deprecated. diff --git a/src/test/app/AmendmentTable_test.cpp b/src/test/app/AmendmentTable_test.cpp index ec9293281b0..8439b713441 100644 --- a/src/test/app/AmendmentTable_test.cpp +++ b/src/test/app/AmendmentTable_test.cpp @@ -39,7 +39,21 @@ class AmendmentTable_test final : public beast::unit_test::suite { private: // 204/256 about 80% (we round down because the implementation rounds up) - static int const majorityFraction{204}; + // 8/10 is 80% + static MajorityFraction constexpr majorityFraction{204, 8}; + uint256 fix3396_ = {}; + + bool + enabled3396() + { + return fix3396_ == fix3396; + } + + void + enable3396(bool enable) + { + fix3396_ = enable ? fix3396 : uint256{}; + } static uint256 amendmentId(std::string in) @@ -399,25 +413,25 @@ class AmendmentTable_test final : public beast::unit_test::suite validations.reserve(validators.size()); int i = 0; - for (auto const& val : validators) + for (auto const& [pub, sec] : validators) { ++i; std::vector field; - for (auto const& amendment : votes) + for (auto const& [hash, nVotes] : votes) { - if ((256 * i) < (validators.size() * amendment.second)) + if (auto yes = enabled3396() ? nVotes >= i : nVotes > i; yes) { // We vote yes on this amendment - field.push_back(amendment.first); + field.push_back(hash); } } auto v = std::make_shared( ripple::NetClock::time_point{}, - val.first, - val.second, - calcNodeID(val.first), + pub, + sec, + calcNodeID(pub), [&field](STValidation& v) { if (!field.empty()) v.setFieldV256( @@ -430,14 +444,13 @@ class AmendmentTable_test final : public beast::unit_test::suite ourVotes = table.doValidation(enabled); - auto actions = - table.doVoting(roundTime, enabled, majority, validations); - for (auto const& action : actions) + auto actions = table.doVoting( + Rules({fix3396_}), roundTime, enabled, majority, validations); + for (auto const& [hash, action] : actions) { // This code assumes other validators do as we do - auto const& hash = action.first; - switch (action.second) + switch (action) { case 0: // amendment goes from majority to enabled @@ -492,7 +505,7 @@ class AmendmentTable_test final : public beast::unit_test::suite BEAST_EXPECT(enabled.empty()); BEAST_EXPECT(majority.empty()); - votes.emplace_back(testAmendment, 256); + votes.emplace_back(testAmendment, validators.size()); doRound( *table, weeks{2}, validators, votes, ourVotes, enabled, majority); @@ -533,7 +546,7 @@ class AmendmentTable_test final : public beast::unit_test::suite BEAST_EXPECT(enabled.empty()); BEAST_EXPECT(majority.empty()); - votes.emplace_back(testAmendment, 256); + votes.emplace_back(testAmendment, validators.size()); doRound( *table, weeks{2}, validators, votes, ourVotes, enabled, majority); @@ -573,7 +586,7 @@ class AmendmentTable_test final : public beast::unit_test::suite // Now, everyone votes for this feature for (auto const& i : supported_) - votes.emplace_back(amendmentId(i), 256); + votes.emplace_back(amendmentId(i), validators.size()); // Week 2: We should recognize a majority doRound( @@ -619,7 +632,7 @@ class AmendmentTable_test final : public beast::unit_test::suite std::vector ourVotes; if ((i > 0) && (i < 17)) - votes.emplace_back(testAmendment, i * 16); + votes.emplace_back(testAmendment, i); doRound( *table, @@ -630,7 +643,7 @@ class AmendmentTable_test final : public beast::unit_test::suite enabled, majority); - if (i < 13) + if (i < 13) // 13 => 13/16 = 0.8125 => > 80% { // We are voting yes, not enabled, no majority BEAST_EXPECT(!ourVotes.empty()); @@ -681,7 +694,7 @@ class AmendmentTable_test final : public beast::unit_test::suite std::vector> votes; std::vector ourVotes; - votes.emplace_back(testAmendment, 250); + votes.emplace_back(testAmendment, validators.size()); doRound( *table, @@ -696,13 +709,13 @@ class AmendmentTable_test final : public beast::unit_test::suite BEAST_EXPECT(!majority.empty()); } - for (int i = 1; i < 16; ++i) + for (int i = 1; i < 8; ++i) { std::vector> votes; std::vector ourVotes; // Gradually reduce support - votes.emplace_back(testAmendment, 256 - i * 8); + votes.emplace_back(testAmendment, validators.size() - i); doRound( *table, @@ -713,8 +726,8 @@ class AmendmentTable_test final : public beast::unit_test::suite enabled, majority); - if (i < 8) - { + if (i < 4) // 16 - 3 = 13 => 13/16 = 0.8125 => > 80% + { // 16 - 4 = 12 => 12/16 = 0.75 => < 80% // We are voting yes, not enabled, majority BEAST_EXPECT(!ourVotes.empty()); BEAST_EXPECT(enabled.empty()); @@ -778,6 +791,7 @@ class AmendmentTable_test final : public beast::unit_test::suite void run() override { + enable3396(false); testConstruct(); testGet(); testBadConfig(); @@ -788,6 +802,9 @@ class AmendmentTable_test final : public beast::unit_test::suite testDetectMajority(); testLostMajority(); testHasUnsupported(); + enable3396(true); + testDetectMajority(); + testLostMajority(); } }; diff --git a/src/test/core/Config_test.cpp b/src/test/core/Config_test.cpp index 0e829642af8..d1caa6e3d71 100644 --- a/src/test/core/Config_test.cpp +++ b/src/test/core/Config_test.cpp @@ -1014,6 +1014,68 @@ r.ripple.com 51235 } } + void + testAmendment() + { + testcase("amendment"); + + std::vector> units = { + {"seconds", 1}, + {"minutes", 60}, + {"hours", 3600}, + {"days", 86400}, + {"weeks", 604800}}; + + std::string space = ""; + for (auto& [unit, m] : units) + { + Config c; + std::string toLoad(R"rippleConfig( +[amendment_majority_time] +10)rippleConfig"); + toLoad += space + unit; + space = space == "" ? " " : ""; + + c.loadFromString(toLoad); + + BEAST_EXPECT(c.AMENDMENT_MAJORITY_TIME.count() == 10 * m); + } + { + Config c; + std::string toLoad(R"rippleConfig( +[amendment_majority_time] +10years +)rippleConfig"); + + try + { + c.loadFromString(toLoad); + fail(); + } + catch (std::runtime_error&) + { + pass(); + } + } + { + Config c; + std::string toLoad(R"rippleConfig( +[amendment_majority_time] +seconds10 +)rippleConfig"); + + try + { + c.loadFromString(toLoad); + fail(); + } + catch (std::runtime_error&) + { + pass(); + } + } + } + void run() override { @@ -1027,6 +1089,7 @@ r.ripple.com 51235 testWhitespace(); testComments(); testGetters(); + testAmendment(); } }; From 7d90194ad3bf75944fa6237e6b3d4698031c0c6e Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Tue, 2 Jun 2020 14:53:05 -0400 Subject: [PATCH 02/12] Remove seconds option from amendment majority time configuration Make 15 minutes the min amendment majority time Move amendment related const to SystemParameters.h --- src/ripple/app/main/Application.cpp | 7 +------ src/ripple/core/Config.h | 2 +- src/ripple/core/impl/Config.cpp | 15 ++++++++++----- src/ripple/protocol/SystemParameters.h | 13 +++++++++++++ 4 files changed, 25 insertions(+), 12 deletions(-) diff --git a/src/ripple/app/main/Application.cpp b/src/ripple/app/main/Application.cpp index 441f673cce1..65b80d0480c 100644 --- a/src/ripple/app/main/Application.cpp +++ b/src/ripple/app/main/Application.cpp @@ -77,11 +77,6 @@ namespace ripple { -// 204/256 about 80% -static int const MAJORITY_FRACTION_OLD(204); -// 8/10 = 80% -static int const MAJORITY_FRACTION(8); - //------------------------------------------------------------------------------ namespace detail { @@ -1536,7 +1531,7 @@ ApplicationImp::setup() m_amendmentTable = make_AmendmentTable( config().AMENDMENT_MAJORITY_TIME, - {MAJORITY_FRACTION_OLD, MAJORITY_FRACTION}, + {amendmentMajorityFractionOld, amendmentMajorityFraction}, supportedAmendments, enabledAmendments, config_->section(SECTION_VETO_AMENDMENTS), diff --git a/src/ripple/core/Config.h b/src/ripple/core/Config.h index 6fede8ae0a3..54cd0a1fbd7 100644 --- a/src/ripple/core/Config.h +++ b/src/ripple/core/Config.h @@ -174,7 +174,7 @@ class Config : public BasicConfig bool COMPRESSION = false; // Amendment majority time - std::chrono::seconds AMENDMENT_MAJORITY_TIME = weeks{2}; + std::chrono::seconds AMENDMENT_MAJORITY_TIME = defaultAmendmentMajorityTime; // Thread pool configuration std::size_t WORKERS = 0; diff --git a/src/ripple/core/impl/Config.cpp b/src/ripple/core/impl/Config.cpp index 5fb0e75084e..70b96764274 100644 --- a/src/ripple/core/impl/Config.cpp +++ b/src/ripple/core/impl/Config.cpp @@ -485,17 +485,17 @@ Config::loadFromString(std::string const& fileContents) { using namespace std::chrono; boost::regex const re( - "^\\s*(\\d+)\\s*(seconds|minutes|hours|days|weeks)\\s*(\\s+.*)?$"); + "^\\s*(\\d+)\\s*(minutes|hours|days|weeks)\\s*(\\s+.*)?$"); boost::smatch match; if (!boost::regex_match(strTemp, match, re)) Throw( "Invalid " SECTION_AMENDMENT_MAJORITY_TIME - ", must be: [0-9]+ [seconds|minutes|hours|days|weeks]"); + ", must be: [0-9]+ [minutes|hours|days|weeks]"); + std::uint32_t duration = beast::lexicalCastThrow(match[1].str()); - if (boost::iequals(match[2], "seconds")) - AMENDMENT_MAJORITY_TIME = seconds(duration); - else if (boost::iequals(match[2], "minutes")) + + if (boost::iequals(match[2], "minutes")) AMENDMENT_MAJORITY_TIME = minutes(duration); else if (boost::iequals(match[2], "hours")) AMENDMENT_MAJORITY_TIME = hours(duration); @@ -503,6 +503,11 @@ Config::loadFromString(std::string const& fileContents) AMENDMENT_MAJORITY_TIME = days(duration); else if (boost::iequals(match[2], "weeks")) AMENDMENT_MAJORITY_TIME = weeks(duration); + + if (AMENDMENT_MAJORITY_TIME < defaultAmendmentMajorityTime) + Throw( + "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 diff --git a/src/ripple/protocol/SystemParameters.h b/src/ripple/protocol/SystemParameters.h index dcadaab247c..a48ed46096f 100644 --- a/src/ripple/protocol/SystemParameters.h +++ b/src/ripple/protocol/SystemParameters.h @@ -21,6 +21,7 @@ #define RIPPLE_PROTOCOL_SYSTEMPARAMETERS_H_INCLUDED #include +#include #include #include @@ -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 fix3396 amendment activates. +*/ + constexpr int const amendmentMajorityFractionOld = 204; + + constexpr int const amendmentMajorityFraction = 8; + +/** The minimum amount of time an amendment must hold a majority */ + constexpr std::chrono::seconds const defaultAmendmentMajorityTime = weeks{2}; + } // namespace ripple #endif From 492272c8c827673922f6a1510b87c063d65d2cdc Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Tue, 2 Jun 2020 14:56:53 -0400 Subject: [PATCH 03/12] Fix clang --- src/ripple/core/impl/Config.cpp | 5 +++-- src/ripple/protocol/SystemParameters.h | 6 +++--- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/ripple/core/impl/Config.cpp b/src/ripple/core/impl/Config.cpp index 70b96764274..43f94eeb123 100644 --- a/src/ripple/core/impl/Config.cpp +++ b/src/ripple/core/impl/Config.cpp @@ -506,8 +506,9 @@ Config::loadFromString(std::string const& fileContents) if (AMENDMENT_MAJORITY_TIME < defaultAmendmentMajorityTime) Throw( - "Invalid " SECTION_AMENDMENT_MAJORITY_TIME - ", the minimum amount of time an amendment must hold a majority is 15 minutes"); + "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 diff --git a/src/ripple/protocol/SystemParameters.h b/src/ripple/protocol/SystemParameters.h index a48ed46096f..de339c0ce00 100644 --- a/src/ripple/protocol/SystemParameters.h +++ b/src/ripple/protocol/SystemParameters.h @@ -65,12 +65,12 @@ static std::uint32_t constexpr XRP_LEDGER_EARLIEST_SEQ{32570}; @note This value is used by legacy code and will become obsolete once the fix3396 amendment activates. */ - constexpr int const amendmentMajorityFractionOld = 204; +constexpr int const amendmentMajorityFractionOld = 204; - constexpr int const amendmentMajorityFraction = 8; +constexpr int const amendmentMajorityFraction = 8; /** The minimum amount of time an amendment must hold a majority */ - constexpr std::chrono::seconds const defaultAmendmentMajorityTime = weeks{2}; +constexpr std::chrono::seconds const defaultAmendmentMajorityTime = weeks{2}; } // namespace ripple From 7482d749f6f98374368f99e6dbfa1cca36781d33 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Tue, 2 Jun 2020 15:08:19 -0400 Subject: [PATCH 04/12] Fix min majority time config --- src/ripple/core/impl/Config.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ripple/core/impl/Config.cpp b/src/ripple/core/impl/Config.cpp index 43f94eeb123..cd272fd885c 100644 --- a/src/ripple/core/impl/Config.cpp +++ b/src/ripple/core/impl/Config.cpp @@ -504,7 +504,7 @@ Config::loadFromString(std::string const& fileContents) else if (boost::iequals(match[2], "weeks")) AMENDMENT_MAJORITY_TIME = weeks(duration); - if (AMENDMENT_MAJORITY_TIME < defaultAmendmentMajorityTime) + if (AMENDMENT_MAJORITY_TIME < minutes(15)) Throw( "Invalid " SECTION_AMENDMENT_MAJORITY_TIME ", the minimum amount of time an amendment must hold a " From 5e989eceb727d4c54b0a9aa4904d73f2208da45d Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Thu, 11 Jun 2020 09:54:51 -0400 Subject: [PATCH 05/12] Move majority threshold calc into AmendmentSet Replace majority fraction with std::ratio Reference super majority threshold constant directly in AmendmentTable instead of passing majority fraction into constructor Rename fix3396 feature to fixAmendmentMajorityCalc Add test config test cases Refactor tests with/without amendment feature --- src/ripple/app/main/Application.cpp | 1 - src/ripple/app/misc/AmendmentTable.h | 7 - src/ripple/app/misc/impl/AmendmentTable.cpp | 107 +++++++------- src/ripple/protocol/Feature.h | 4 +- src/ripple/protocol/SystemParameters.h | 6 +- src/ripple/protocol/impl/Feature.cpp | 4 +- src/test/app/AmendmentTable_test.cpp | 153 +++++++++++++------- src/test/core/Config_test.cpp | 68 ++++----- 8 files changed, 198 insertions(+), 152 deletions(-) diff --git a/src/ripple/app/main/Application.cpp b/src/ripple/app/main/Application.cpp index 65b80d0480c..5d4b7c88573 100644 --- a/src/ripple/app/main/Application.cpp +++ b/src/ripple/app/main/Application.cpp @@ -1531,7 +1531,6 @@ ApplicationImp::setup() m_amendmentTable = make_AmendmentTable( config().AMENDMENT_MAJORITY_TIME, - {amendmentMajorityFractionOld, amendmentMajorityFraction}, supportedAmendments, enabledAmendments, config_->section(SECTION_VETO_AMENDMENTS), diff --git a/src/ripple/app/misc/AmendmentTable.h b/src/ripple/app/misc/AmendmentTable.h index cab519c86d7..bcd21f763b5 100644 --- a/src/ripple/app/misc/AmendmentTable.h +++ b/src/ripple/app/misc/AmendmentTable.h @@ -27,12 +27,6 @@ namespace ripple { -struct MajorityFraction -{ - int old_ = 0; - int new_ = 0; -}; - /** The amendment table stores the list of enabled and potential amendments. Individuals amendments are voted on by validators during the consensus process. @@ -172,7 +166,6 @@ class AmendmentTable std::unique_ptr make_AmendmentTable( std::chrono::seconds majorityTime, - MajorityFraction const& majorityFraction, Section const& supported, Section const& enabled, Section const& vetoed, diff --git a/src/ripple/app/misc/impl/AmendmentTable.cpp b/src/ripple/app/misc/impl/AmendmentTable.cpp index dcf64509d0a..801ee7e2c49 100644 --- a/src/ripple/app/misc/impl/AmendmentTable.cpp +++ b/src/ripple/app/misc/impl/AmendmentTable.cpp @@ -98,25 +98,68 @@ struct AmendmentSet private: // How many yes votes each amendment received hash_map 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> 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 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 { @@ -139,7 +182,7 @@ struct AmendmentSet */ class AmendmentTableImpl final : public AmendmentTable { -protected: +private: mutable std::mutex mutex_; hash_map amendmentMap_; @@ -148,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%(old) 8 = 100%(new) - MajorityFraction const majorityFraction_; - // The results of the last voting round - may be empty if // we haven't participated in one yet. std::unique_ptr lastVote_; @@ -188,7 +227,6 @@ class AmendmentTableImpl final : public AmendmentTable public: AmendmentTableImpl( std::chrono::seconds majorityTime, - MajorityFraction const& majorityFraction, Section const& supported, Section const& enabled, Section const& vetoed, @@ -249,19 +287,15 @@ class AmendmentTableImpl final : public AmendmentTable AmendmentTableImpl::AmendmentTableImpl( std::chrono::seconds majorityTime, - MajorityFraction const& 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_.old_ != 0 && majorityFraction_.new_ != 0); - std::lock_guard sl(mutex_); for (auto const& a : parseSection(supported)) @@ -473,29 +507,7 @@ AmendmentTableImpl::doVoting( << ": " << enabledAmendments.size() << ", " << majorityAmendments.size() << ", " << valSet.size(); - auto vote = std::make_unique(); - - // process validations for ledger before flag ledger - for (auto const& val : valSet) - { - if (val->isTrusted()) - { - std::set ballot; - - if (val->isFieldPresent(sfAmendments)) - { - auto const choices = val->getFieldV256(sfAmendments); - ballot.insert(choices.begin(), choices.end()); - } - - vote->tally(ballot); - } - } - - vote->mThreshold = rules.enabled(ripple::fix3396) - ? std::max(1, (vote->mTrustedValidations * majorityFraction_.new_) / 10) - : std::max( - 1, (vote->mTrustedValidations * majorityFraction_.old_) / 256); + auto vote = std::make_unique(rules, valSet); JLOG(j_.debug()) << "Received " << vote->mTrustedValidations << " trusted validations, threshold is: " @@ -512,9 +524,7 @@ AmendmentTableImpl::doVoting( { NetClock::time_point majorityTime = {}; - bool const hasValMajority = rules.enabled(ripple::fix3396) - ? (vote->votes(entry.first) > vote->mThreshold) - : (vote->votes(entry.first) >= vote->mThreshold); + bool const hasValMajority = vote->passes(entry.first); { auto const it = majorityAmendments.find(entry.first); @@ -672,14 +682,13 @@ AmendmentTableImpl::getJson(uint256 const& amendmentID) const std::unique_ptr make_AmendmentTable( std::chrono::seconds majorityTime, - MajorityFraction const& majorityFraction, Section const& supported, Section const& enabled, Section const& vetoed, beast::Journal journal) { return std::make_unique( - majorityTime, majorityFraction, supported, enabled, vetoed, journal); + majorityTime, supported, enabled, vetoed, journal); } } // namespace ripple diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index 47230f0ab0f..ae05c8c1d11 100644 --- a/src/ripple/protocol/Feature.h +++ b/src/ripple/protocol/Feature.h @@ -112,7 +112,7 @@ class FeatureCollections "fix1781", // XRPEndpointSteps should be included in the circular // payment check "HardenedValidations", - "fix3396"}; // Fix Amendment majority calculation + "fixAmendmentMajorityCalc"}; // Fix Amendment majority calculation std::vector features; boost::container::flat_map featureToIndex; @@ -368,7 +368,7 @@ extern uint256 const fixQualityUpperBound; extern uint256 const featureRequireFullyCanonicalSig; extern uint256 const fix1781; extern uint256 const featureHardenedValidations; -extern uint256 const fix3396; +extern uint256 const fixAmendmentMajorityCalc; } // namespace ripple diff --git a/src/ripple/protocol/SystemParameters.h b/src/ripple/protocol/SystemParameters.h index de339c0ce00..35f4e7cc7a6 100644 --- a/src/ripple/protocol/SystemParameters.h +++ b/src/ripple/protocol/SystemParameters.h @@ -63,11 +63,11 @@ 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 fix3396 amendment activates. + once the fixAmendmentMajorityCalc amendment activates. */ -constexpr int const amendmentMajorityFractionOld = 204; +constexpr std::ratio<205, 256> amendmentSuperMajorityThresholdPre3396; -constexpr int const amendmentMajorityFraction = 8; +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}; diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index d990f7c5d21..9d88c9d211a 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -131,7 +131,7 @@ detail::supportedAmendments() "RequireFullyCanonicalSig", "fix1781", "HardenedValidations", - "fix3396"}; + "fixAmendmentMajorityCalc"}; return supported; } @@ -183,7 +183,7 @@ uint256 const featureRequireFullyCanonicalSig = *getRegisteredFeature("RequireFullyCanonicalSig"), fix1781 = *getRegisteredFeature("fix1781"), featureHardenedValidations = *getRegisteredFeature("HardenedValidations"), - fix3396 = *getRegisteredFeature("fix3396"); + 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. diff --git a/src/test/app/AmendmentTable_test.cpp b/src/test/app/AmendmentTable_test.cpp index 8439b713441..3b4ec47d143 100644 --- a/src/test/app/AmendmentTable_test.cpp +++ b/src/test/app/AmendmentTable_test.cpp @@ -38,23 +38,6 @@ namespace ripple { class AmendmentTable_test final : public beast::unit_test::suite { private: - // 204/256 about 80% (we round down because the implementation rounds up) - // 8/10 is 80% - static MajorityFraction constexpr majorityFraction{204, 8}; - uint256 fix3396_ = {}; - - bool - enabled3396() - { - return fix3396_ == fix3396; - } - - void - enable3396(bool enable) - { - fix3396_ = enable ? fix3396 : uint256{}; - } - static uint256 amendmentId(std::string in) { @@ -114,12 +97,7 @@ class AmendmentTable_test final : public beast::unit_test::suite Section const vetoed) { return make_AmendmentTable( - majorityTime, - majorityFraction, - supported, - enabled, - vetoed, - journal); + majorityTime, supported, enabled, vetoed, journal); } std::unique_ptr @@ -387,6 +365,7 @@ class AmendmentTable_test final : public beast::unit_test::suite // Execute a pretend consensus round for a flag ledger void doRound( + uint256 const& feat, AmendmentTable& table, weeks week, std::vector> const& validators, @@ -420,7 +399,7 @@ class AmendmentTable_test final : public beast::unit_test::suite for (auto const& [hash, nVotes] : votes) { - if (auto yes = enabled3396() ? nVotes >= i : nVotes > i; yes) + if (feat == fixAmendmentMajorityCalc ? nVotes >= i : nVotes > i) { // We vote yes on this amendment field.push_back(hash); @@ -445,7 +424,7 @@ class AmendmentTable_test final : public beast::unit_test::suite ourVotes = table.doValidation(enabled); auto actions = table.doVoting( - Rules({fix3396_}), roundTime, enabled, majority, validations); + Rules({feat}), roundTime, enabled, majority, validations); for (auto const& [hash, action] : actions) { // This code assumes other validators do as we do @@ -484,7 +463,7 @@ class AmendmentTable_test final : public beast::unit_test::suite // No vote on unknown amendment void - testNoOnUnknown() + testNoOnUnknown(uint256 const& feat) { testcase("Vote NO on unknown"); @@ -500,7 +479,14 @@ class AmendmentTable_test final : public beast::unit_test::suite majorityAmendments_t majority; doRound( - *table, weeks{1}, validators, votes, ourVotes, enabled, majority); + feat, + *table, + weeks{1}, + validators, + votes, + ourVotes, + enabled, + majority); BEAST_EXPECT(ourVotes.empty()); BEAST_EXPECT(enabled.empty()); BEAST_EXPECT(majority.empty()); @@ -508,7 +494,14 @@ class AmendmentTable_test final : public beast::unit_test::suite votes.emplace_back(testAmendment, validators.size()); doRound( - *table, weeks{2}, validators, votes, ourVotes, enabled, majority); + feat, + *table, + weeks{2}, + validators, + votes, + ourVotes, + enabled, + majority); BEAST_EXPECT(ourVotes.empty()); BEAST_EXPECT(enabled.empty()); @@ -517,14 +510,21 @@ class AmendmentTable_test final : public beast::unit_test::suite // Note that the simulation code assumes others behave as we do, // so the amendment won't get enabled doRound( - *table, weeks{5}, validators, votes, ourVotes, enabled, majority); + feat, + *table, + weeks{5}, + validators, + votes, + ourVotes, + enabled, + majority); BEAST_EXPECT(ourVotes.empty()); BEAST_EXPECT(enabled.empty()); } // No vote on vetoed amendment void - testNoOnVetoed() + testNoOnVetoed(uint256 const& feat) { testcase("Vote NO on vetoed"); @@ -541,7 +541,14 @@ class AmendmentTable_test final : public beast::unit_test::suite majorityAmendments_t majority; doRound( - *table, weeks{1}, validators, votes, ourVotes, enabled, majority); + feat, + *table, + weeks{1}, + validators, + votes, + ourVotes, + enabled, + majority); BEAST_EXPECT(ourVotes.empty()); BEAST_EXPECT(enabled.empty()); BEAST_EXPECT(majority.empty()); @@ -549,21 +556,35 @@ class AmendmentTable_test final : public beast::unit_test::suite votes.emplace_back(testAmendment, validators.size()); doRound( - *table, weeks{2}, validators, votes, ourVotes, enabled, majority); + feat, + *table, + weeks{2}, + validators, + votes, + ourVotes, + enabled, + majority); BEAST_EXPECT(ourVotes.empty()); BEAST_EXPECT(enabled.empty()); majority[testAmendment] = weekTime(weeks{1}); doRound( - *table, weeks{5}, validators, votes, ourVotes, enabled, majority); + feat, + *table, + weeks{5}, + validators, + votes, + ourVotes, + enabled, + majority); BEAST_EXPECT(ourVotes.empty()); BEAST_EXPECT(enabled.empty()); } // Vote on and enable known, not-enabled amendment void - testVoteEnable() + testVoteEnable(uint256 const& feat) { testcase("voteEnable"); @@ -578,7 +599,14 @@ class AmendmentTable_test final : public beast::unit_test::suite // Week 1: We should vote for all known amendments not enabled doRound( - *table, weeks{1}, validators, votes, ourVotes, enabled, majority); + feat, + *table, + weeks{1}, + validators, + votes, + ourVotes, + enabled, + majority); BEAST_EXPECT(ourVotes.size() == supported_.size()); BEAST_EXPECT(enabled.empty()); for (auto const& i : supported_) @@ -590,7 +618,14 @@ class AmendmentTable_test final : public beast::unit_test::suite // Week 2: We should recognize a majority doRound( - *table, weeks{2}, validators, votes, ourVotes, enabled, majority); + feat, + *table, + weeks{2}, + validators, + votes, + ourVotes, + enabled, + majority); BEAST_EXPECT(ourVotes.size() == supported_.size()); BEAST_EXPECT(enabled.empty()); @@ -599,12 +634,26 @@ class AmendmentTable_test final : public beast::unit_test::suite // Week 5: We should enable the amendment doRound( - *table, weeks{5}, validators, votes, ourVotes, enabled, majority); + feat, + *table, + weeks{5}, + validators, + votes, + ourVotes, + enabled, + majority); BEAST_EXPECT(enabled.size() == supported_.size()); // Week 6: We should remove it from our votes and from having a majority doRound( - *table, weeks{6}, validators, votes, ourVotes, enabled, majority); + feat, + *table, + weeks{6}, + validators, + votes, + ourVotes, + enabled, + majority); BEAST_EXPECT(enabled.size() == supported_.size()); BEAST_EXPECT(ourVotes.empty()); for (auto const& i : supported_) @@ -613,7 +662,7 @@ class AmendmentTable_test final : public beast::unit_test::suite // Detect majority at 80%, enable later void - testDetectMajority() + testDetectMajority(uint256 const& feat) { testcase("detectMajority"); @@ -635,6 +684,7 @@ class AmendmentTable_test final : public beast::unit_test::suite votes.emplace_back(testAmendment, i); doRound( + feat, *table, weeks{i}, validators, @@ -676,7 +726,7 @@ class AmendmentTable_test final : public beast::unit_test::suite // Detect loss of majority void - testLostMajority() + testLostMajority(uint256 const& feat) { testcase("lostMajority"); @@ -697,6 +747,7 @@ class AmendmentTable_test final : public beast::unit_test::suite votes.emplace_back(testAmendment, validators.size()); doRound( + feat, *table, weeks{1}, validators, @@ -718,6 +769,7 @@ class AmendmentTable_test final : public beast::unit_test::suite votes.emplace_back(testAmendment, validators.size() - i); doRound( + feat, *table, weeks{i + 1}, validators, @@ -788,23 +840,26 @@ class AmendmentTable_test final : public beast::unit_test::suite BEAST_EXPECT(table->needValidatedLedger(257)); } + void + testFeature(uint256 const& feat) + { + testNoOnUnknown(feat); + testNoOnVetoed(feat); + testVoteEnable(feat); + testDetectMajority(feat); + testLostMajority(feat); + } + void run() override { - enable3396(false); testConstruct(); testGet(); testBadConfig(); testEnableVeto(); - testNoOnUnknown(); - testNoOnVetoed(); - testVoteEnable(); - testDetectMajority(); - testLostMajority(); testHasUnsupported(); - enable3396(true); - testDetectMajority(); - testLostMajority(); + testFeature({}); + testFeature(fixAmendmentMajorityCalc); } }; diff --git a/src/test/core/Config_test.cpp b/src/test/core/Config_test.cpp index d1caa6e3d71..8e536eecd1c 100644 --- a/src/test/core/Config_test.cpp +++ b/src/test/core/Config_test.cpp @@ -1018,60 +1018,50 @@ r.ripple.com 51235 testAmendment() { testcase("amendment"); - - std::vector> units = { - {"seconds", 1}, - {"minutes", 60}, - {"hours", 3600}, - {"days", 86400}, - {"weeks", 604800}}; - - std::string space = ""; - for (auto& [unit, m] : units) + struct ConfigUnit { - Config c; - std::string toLoad(R"rippleConfig( -[amendment_majority_time] -10)rippleConfig"); - toLoad += space + unit; - space = space == "" ? " " : ""; - - c.loadFromString(toLoad); + std::string unit; + std::uint32_t numSeconds; + std::uint32_t configVal; + bool shouldPass; + }; - BEAST_EXPECT(c.AMENDMENT_MAJORITY_TIME.count() == 10 * m); - } - { - Config c; - std::string toLoad(R"rippleConfig( -[amendment_majority_time] -10years -)rippleConfig"); + std::vector units = { + {"seconds", 1, 15 * 60 - 1, false}, + {"seconds", 1, 15 * 60, true}, + {"minutes", 60, 14, false}, + {"minutes", 60, 15, true}, + {"hours", 3600, 10, true}, + {"days", 86400, 10, true}, + {"weeks", 604800, 2, true}, + {"months", 2592000, 1, false}, + {"years", 31536000, 1, false}}; - try - { - c.loadFromString(toLoad); - fail(); - } - catch (std::runtime_error&) - { - pass(); - } - } + std::string space = ""; + for (auto& [unit, sec, val, shouldPass] : units) { Config c; std::string toLoad(R"rippleConfig( [amendment_majority_time] -seconds10 )rippleConfig"); + toLoad += std::to_string(val) + space + unit; + space = space == "" ? " " : ""; try { c.loadFromString(toLoad); - fail(); + if (shouldPass) + BEAST_EXPECT( + c.AMENDMENT_MAJORITY_TIME.count() == val * sec); + else + fail(); } catch (std::runtime_error&) { - pass(); + if (!shouldPass) + pass(); + else + fail(); } } } From 63370d12808cbe2d3e71fc394134d4491ca7c1e5 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Sat, 13 Jun 2020 09:23:50 -0400 Subject: [PATCH 06/12] Include in Config.h Fix super majority threshold typo 205 -> 204 --- src/ripple/core/Config.h | 2 +- src/ripple/protocol/SystemParameters.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ripple/core/Config.h b/src/ripple/core/Config.h index 54cd0a1fbd7..7eec3bc0764 100644 --- a/src/ripple/core/Config.h +++ b/src/ripple/core/Config.h @@ -23,7 +23,6 @@ #include #include #include -#include #include #include #include // VFALCO Breaks levelization @@ -32,6 +31,7 @@ #include #include #include +#include #include #include #include diff --git a/src/ripple/protocol/SystemParameters.h b/src/ripple/protocol/SystemParameters.h index 35f4e7cc7a6..30f841497ad 100644 --- a/src/ripple/protocol/SystemParameters.h +++ b/src/ripple/protocol/SystemParameters.h @@ -65,7 +65,7 @@ static std::uint32_t constexpr XRP_LEDGER_EARLIEST_SEQ{32570}; @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<204, 256> amendmentSuperMajorityThresholdPre3396; constexpr std::ratio<80, 100> amendmentSuperMajorityThresholdPost3396; From e0f516f1d5615d9573d8c8b57db3ee14bac31d50 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Sat, 13 Jun 2020 20:25:20 -0400 Subject: [PATCH 07/12] Fix amendment threshold calculation --- src/ripple/app/misc/impl/AmendmentTable.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ripple/app/misc/impl/AmendmentTable.cpp b/src/ripple/app/misc/impl/AmendmentTable.cpp index 801ee7e2c49..0c8028221a1 100644 --- a/src/ripple/app/misc/impl/AmendmentTable.cpp +++ b/src/ripple/app/misc/impl/AmendmentTable.cpp @@ -124,7 +124,7 @@ struct AmendmentSet } } - mThreshold = rules_.enabled(fixAmendmentMajorityCalc) + mThreshold = !rules_.enabled(fixAmendmentMajorityCalc) ? std::max( 1L, (mTrustedValidations * From 9f6aefc28163e9d29f1a8f00e6bffd7dd06a52e5 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Mon, 15 Jun 2020 07:33:43 -0400 Subject: [PATCH 08/12] Fix Amendment calculation with one trusted validator --- src/ripple/app/misc/impl/AmendmentTable.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/ripple/app/misc/impl/AmendmentTable.cpp b/src/ripple/app/misc/impl/AmendmentTable.cpp index 0c8028221a1..74ef2549c18 100644 --- a/src/ripple/app/misc/impl/AmendmentTable.cpp +++ b/src/ripple/app/misc/impl/AmendmentTable.cpp @@ -148,7 +148,10 @@ struct AmendmentSet // 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)) + // One validator is an exception, otherwise it is not possible + // to gain majority. + if (!rules_.enabled(fixAmendmentMajorityCalc) || + mTrustedValidations == 1) return it->second >= mThreshold; return it->second > mThreshold; From f3c752c9e9818274ea99cd4ef09834d4b1be5e97 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Sat, 20 Jun 2020 08:51:46 -0400 Subject: [PATCH 09/12] Fix amendment majority time Config unit test Fix Windows compile Remove per-256 response in Feature rpc test --- src/ripple/app/misc/impl/AmendmentTable.cpp | 17 ++++++++--------- src/test/core/Config_test.cpp | 3 +-- src/test/rpc/Feature_test.cpp | 7 +++---- 3 files changed, 12 insertions(+), 15 deletions(-) diff --git a/src/ripple/app/misc/impl/AmendmentTable.cpp b/src/ripple/app/misc/impl/AmendmentTable.cpp index 74ef2549c18..6a7c7d4aca2 100644 --- a/src/ripple/app/misc/impl/AmendmentTable.cpp +++ b/src/ripple/app/misc/impl/AmendmentTable.cpp @@ -127,14 +127,16 @@ struct AmendmentSet mThreshold = !rules_.enabled(fixAmendmentMajorityCalc) ? std::max( 1L, - (mTrustedValidations * - amendmentSuperMajorityThresholdPre3396.num) / - amendmentSuperMajorityThresholdPre3396.den) + static_cast( + (mTrustedValidations * + amendmentSuperMajorityThresholdPre3396.num) / + amendmentSuperMajorityThresholdPre3396.den)) : std::max( 1L, - (mTrustedValidations * - amendmentSuperMajorityThresholdPost3396.num) / - amendmentSuperMajorityThresholdPost3396.den); + static_cast( + (mTrustedValidations * + amendmentSuperMajorityThresholdPost3396.num) / + amendmentSuperMajorityThresholdPost3396.den)); } bool @@ -641,10 +643,7 @@ AmendmentTableImpl::injectJson( v[jss::validations] = votesTotal; if (votesNeeded) - { - v[jss::vote] = votesFor * 256 / votesNeeded; v[jss::threshold] = votesNeeded; - } } } diff --git a/src/test/core/Config_test.cpp b/src/test/core/Config_test.cpp index 8e536eecd1c..03282fd59de 100644 --- a/src/test/core/Config_test.cpp +++ b/src/test/core/Config_test.cpp @@ -1027,8 +1027,7 @@ r.ripple.com 51235 }; std::vector units = { - {"seconds", 1, 15 * 60 - 1, false}, - {"seconds", 1, 15 * 60, true}, + {"seconds", 1, 15 * 60, false}, {"minutes", 60, 14, false}, {"minutes", 60, 15, true}, {"hours", 3600, 10, true}, diff --git a/src/test/rpc/Feature_test.cpp b/src/test/rpc/Feature_test.cpp index 731e3560dfc..5773f756667 100644 --- a/src/test/rpc/Feature_test.cpp +++ b/src/test/rpc/Feature_test.cpp @@ -218,10 +218,9 @@ class Feature_test : public beast::unit_test::suite BEAST_EXPECTS( feature.isMember(jss::validations), feature[jss::name].asString() + " validations"); - BEAST_EXPECTS( - feature.isMember(jss::vote), - feature[jss::name].asString() + " vote"); - BEAST_EXPECT(feature[jss::vote] == 256); + BEAST_EXPECT(feature[jss::count] == 1); + BEAST_EXPECT(feature[jss::threshold] == 1); + BEAST_EXPECT(feature[jss::validations] == 1); BEAST_EXPECT(feature[jss::majority] == 2740); } } From 2c429ddf947cf98856feb16dde7d1a5b8af6e71f Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Tue, 23 Jun 2020 12:07:50 -0400 Subject: [PATCH 10/12] Refactor: - make AmendmentSet a class - change amendmentSuperMajorityThreshold[Pre,Post]3396 to [pre,post]FixAmendmentMajorityCaltThreshold --- src/ripple/app/misc/impl/AmendmentTable.cpp | 54 ++++++++++++--------- src/ripple/protocol/SystemParameters.h | 4 +- 2 files changed, 34 insertions(+), 24 deletions(-) diff --git a/src/ripple/app/misc/impl/AmendmentTable.cpp b/src/ripple/app/misc/impl/AmendmentTable.cpp index 6a7c7d4aca2..2201491196f 100644 --- a/src/ripple/app/misc/impl/AmendmentTable.cpp +++ b/src/ripple/app/misc/impl/AmendmentTable.cpp @@ -93,12 +93,16 @@ 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 votes_; Rules const& rules_; + // number of trusted validations + int trustedValidations_ = 0; + // number of votes needed + int threshold_ = 0; public: AmendmentSet( @@ -120,23 +124,23 @@ struct AmendmentSet [&](auto const& amendment) { ++votes_[amendment]; }); } - ++mTrustedValidations; + ++trustedValidations_; } } - mThreshold = !rules_.enabled(fixAmendmentMajorityCalc) + threshold_ = !rules_.enabled(fixAmendmentMajorityCalc) ? std::max( 1L, static_cast( - (mTrustedValidations * - amendmentSuperMajorityThresholdPre3396.num) / - amendmentSuperMajorityThresholdPre3396.den)) + (trustedValidations_ * + preFixAmendmentMajorityCalcThreshold.num) / + preFixAmendmentMajorityCalcThreshold.den)) : std::max( 1L, static_cast( - (mTrustedValidations * - amendmentSuperMajorityThresholdPost3396.num) / - amendmentSuperMajorityThresholdPost3396.den)); + (trustedValidations_ * + postFixAmendmentMajorityCalcThreshold.num) / + postFixAmendmentMajorityCalcThreshold.den)); } bool @@ -153,18 +157,12 @@ struct AmendmentSet // One validator is an exception, otherwise it is not possible // to gain majority. if (!rules_.enabled(fixAmendmentMajorityCalc) || - mTrustedValidations == 1) - return it->second >= mThreshold; + trustedValidations_ == 1) + return it->second >= threshold_; - return it->second > mThreshold; + return it->second > threshold_; } - // number of trusted validations - int mTrustedValidations = 0; - - // number of votes needed - int mThreshold = 0; - int votes(uint256 const& amendment) const { @@ -175,6 +173,18 @@ struct AmendmentSet return it->second; } + + int + trustedValidations() const + { + return trustedValidations_; + } + + int + threshold() const + { + return threshold_; + } }; //------------------------------------------------------------------------------ @@ -514,9 +524,9 @@ AmendmentTableImpl::doVoting( auto vote = std::make_unique(rules, valSet); - 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 @@ -635,8 +645,8 @@ 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; diff --git a/src/ripple/protocol/SystemParameters.h b/src/ripple/protocol/SystemParameters.h index 30f841497ad..129c8acb3bc 100644 --- a/src/ripple/protocol/SystemParameters.h +++ b/src/ripple/protocol/SystemParameters.h @@ -65,9 +65,9 @@ static std::uint32_t constexpr XRP_LEDGER_EARLIEST_SEQ{32570}; @note This value is used by legacy code and will become obsolete once the fixAmendmentMajorityCalc amendment activates. */ -constexpr std::ratio<204, 256> amendmentSuperMajorityThresholdPre3396; +constexpr std::ratio<204, 256> preFixAmendmentMajorityCalcThreshold; -constexpr std::ratio<80, 100> amendmentSuperMajorityThresholdPost3396; +constexpr std::ratio<80, 100> postFixAmendmentMajorityCalcThreshold; /** The minimum amount of time an amendment must hold a majority */ constexpr std::chrono::seconds const defaultAmendmentMajorityTime = weeks{2}; From 69ea2d468ddec9d9df3c08965117a0522a34aa5b Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Tue, 23 Jun 2020 12:12:05 -0400 Subject: [PATCH 11/12] Clang fix --- src/ripple/app/misc/impl/AmendmentTable.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/ripple/app/misc/impl/AmendmentTable.cpp b/src/ripple/app/misc/impl/AmendmentTable.cpp index 2201491196f..9b831599655 100644 --- a/src/ripple/app/misc/impl/AmendmentTable.cpp +++ b/src/ripple/app/misc/impl/AmendmentTable.cpp @@ -132,15 +132,15 @@ class AmendmentSet ? std::max( 1L, static_cast( - (trustedValidations_ * - preFixAmendmentMajorityCalcThreshold.num) / - preFixAmendmentMajorityCalcThreshold.den)) + (trustedValidations_ * + preFixAmendmentMajorityCalcThreshold.num) / + preFixAmendmentMajorityCalcThreshold.den)) : std::max( 1L, static_cast( - (trustedValidations_ * - postFixAmendmentMajorityCalcThreshold.num) / - postFixAmendmentMajorityCalcThreshold.den)); + (trustedValidations_ * + postFixAmendmentMajorityCalcThreshold.num) / + postFixAmendmentMajorityCalcThreshold.den)); } bool From d7cb90cdee6907e69787f60d8fbdf1a70d7b158f Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Tue, 23 Jun 2020 12:14:57 -0400 Subject: [PATCH 12/12] Clang fix --- src/ripple/app/misc/impl/AmendmentTable.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ripple/app/misc/impl/AmendmentTable.cpp b/src/ripple/app/misc/impl/AmendmentTable.cpp index 9b831599655..2f29a2f5788 100644 --- a/src/ripple/app/misc/impl/AmendmentTable.cpp +++ b/src/ripple/app/misc/impl/AmendmentTable.cpp @@ -133,13 +133,13 @@ class AmendmentSet 1L, static_cast( (trustedValidations_ * - preFixAmendmentMajorityCalcThreshold.num) / + preFixAmendmentMajorityCalcThreshold.num) / preFixAmendmentMajorityCalcThreshold.den)) : std::max( 1L, static_cast( (trustedValidations_ * - postFixAmendmentMajorityCalcThreshold.num) / + postFixAmendmentMajorityCalcThreshold.num) / postFixAmendmentMajorityCalcThreshold.den)); }