From 5e11ad543f99c72792347da39055c3fff5d9b381 Mon Sep 17 00:00:00 2001 From: Edward Hennis Date: Sat, 13 Jun 2020 22:56:36 -0400 Subject: [PATCH] Refactor Feature name management and creation: * Only require adding the new feature names in one place. (Also need to increment a counter, but a check on startup will catch that.) * Allows rippled to have the code to support a given amendment, but not vote for it by default. This allows the amendment to be enabled in a future version without necessarily amendment blocking these older versions. * The default vote is carried with the amendment name in the list of supported amendments. * The amendment table is constructed with the amendment and default vote. --- src/ripple/app/main/Application.cpp | 34 +- src/ripple/app/misc/AmendmentTable.h | 16 +- src/ripple/app/misc/impl/AmendmentTable.cpp | 171 +++--- .../app/rdb/RelationalDBInterface_global.h | 12 +- .../rdb/impl/RelationalDBInterface_global.cpp | 14 +- src/ripple/protocol/Feature.h | 164 ++---- src/ripple/protocol/impl/Feature.cpp | 532 +++++++++++++----- src/ripple/rpc/handlers/Feature1.cpp | 2 + src/test/app/AmendmentTable_test.cpp | 134 +++-- src/test/app/TxQ_test.cpp | 122 ++-- src/test/jtx/Env.h | 3 +- src/test/jtx/Env_test.cpp | 1 + src/test/rpc/Feature_test.cpp | 110 +++- 13 files changed, 831 insertions(+), 484 deletions(-) diff --git a/src/ripple/app/main/Application.cpp b/src/ripple/app/main/Application.cpp index 0dab388225a..b9b60767bed 100644 --- a/src/ripple/app/main/Application.cpp +++ b/src/ripple/app/main/Application.cpp @@ -1250,27 +1250,29 @@ ApplicationImp::setup() // Configure the amendments the server supports { - auto const& sa = detail::supportedAmendments(); - std::vector saHashes; - saHashes.reserve(sa.size()); - for (auto const& name : sa) - { - auto const f = getRegisteredFeature(name); - BOOST_ASSERT(f); - if (f) - saHashes.push_back(to_string(*f) + " " + name); - } - Section supportedAmendments("Supported Amendments"); - supportedAmendments.append(saHashes); + auto const supported = []() { + auto const& amendments = detail::supportedAmendments(); + std::vector supported; + supported.reserve(amendments.size()); + for (auto const& [a, vote] : amendments) + { + auto const f = ripple::getRegisteredFeature(a); + assert(f); + if (f) + supported.emplace_back(a, *f, vote); + } + return supported; + }(); + Section const& downVoted = config_->section(SECTION_VETO_AMENDMENTS); - Section enabledAmendments = config_->section(SECTION_AMENDMENTS); + Section const& upVoted = config_->section(SECTION_AMENDMENTS); m_amendmentTable = make_AmendmentTable( *this, config().AMENDMENT_MAJORITY_TIME, - supportedAmendments, - enabledAmendments, - config_->section(SECTION_VETO_AMENDMENTS), + supported, + upVoted, + downVoted, logs_->journal("Amendments")); } diff --git a/src/ripple/app/misc/AmendmentTable.h b/src/ripple/app/misc/AmendmentTable.h index 654efaf6559..14558b7da51 100644 --- a/src/ripple/app/misc/AmendmentTable.h +++ b/src/ripple/app/misc/AmendmentTable.h @@ -22,6 +22,7 @@ #include #include +#include #include #include @@ -36,6 +37,19 @@ namespace ripple { class AmendmentTable { public: + struct FeatureInfo + { + FeatureInfo() = delete; + FeatureInfo(std::string const& n, uint256 const& f, DefaultVote v) + : name(n), feature(f), vote(v) + { + } + + std::string const name; + uint256 const feature; + DefaultVote const vote; + }; + virtual ~AmendmentTable() = default; virtual uint256 @@ -168,7 +182,7 @@ std::unique_ptr make_AmendmentTable( Application& app, std::chrono::seconds majorityTime, - Section const& supported, + std::vector const& supported, Section const& enabled, Section const& vetoed, beast::Journal journal); diff --git a/src/ripple/app/misc/impl/AmendmentTable.cpp b/src/ripple/app/misc/impl/AmendmentTable.cpp index 8cedabc8369..be59320be4b 100644 --- a/src/ripple/app/misc/impl/AmendmentTable.cpp +++ b/src/ripple/app/misc/impl/AmendmentTable.cpp @@ -73,8 +73,8 @@ parseSection(Section const& section) */ struct AmendmentState { - /** If an amendment is vetoed, a server will not support it */ - bool vetoed = false; + /** If an amendment is down-voted, a server will not vote to enable it */ + AmendmentVote vote = AmendmentVote::down; /** Indicates that the amendment has been enabled. This is a one-way switch: once an amendment is enabled @@ -224,15 +224,16 @@ class AmendmentTableImpl final : public AmendmentTable DatabaseCon& db_; // Finds or creates state. Must be called with mutex_ locked. - AmendmentState* - add(uint256 const& amendment, std::lock_guard const& sl); + AmendmentState& + add(uint256 const& amendment, std::lock_guard const& lock); // Finds existing state. Must be called with mutex_ locked. AmendmentState* - get(uint256 const& amendment, std::lock_guard const& sl); + get(uint256 const& amendment, std::lock_guard const& lock); AmendmentState const* - get(uint256 const& amendment, std::lock_guard const& sl) const; + get(uint256 const& amendment, + std::lock_guard const& lock) const; // Injects amendment json into v. Must be called with mutex_ locked. void @@ -240,19 +241,19 @@ class AmendmentTableImpl final : public AmendmentTable Json::Value& v, uint256 const& amendment, AmendmentState const& state, - std::lock_guard const& sl) const; + std::lock_guard const& lock) const; void persistVote( uint256 const& amendment, std::string const& name, - bool vote_to_veto) const; + AmendmentVote vote) const; public: AmendmentTableImpl( Application& app, std::chrono::seconds majorityTime, - Section const& supported, + std::vector const& supported, Section const& enabled, Section const& vetoed, beast::Journal journal); @@ -313,7 +314,7 @@ class AmendmentTableImpl final : public AmendmentTable AmendmentTableImpl::AmendmentTableImpl( Application& app, std::chrono::seconds majorityTime, - Section const& supported, + std::vector const& supported, Section const& enabled, Section const& vetoed, beast::Journal journal) @@ -323,7 +324,7 @@ AmendmentTableImpl::AmendmentTableImpl( , j_(journal) , db_(app.getWalletDB()) { - std::lock_guard sl(mutex_); + std::lock_guard lock(mutex_); // Find out if the FeatureVotes table exists in WalletDB bool const featureVotesExist = [this]() { @@ -332,22 +333,24 @@ AmendmentTableImpl::AmendmentTableImpl( }(); // Parse supported amendments - for (auto const& a : parseSection(supported)) + for (auto const& [name, amendment, defaultVote] : supported) { - if (auto s = add(a.first, sl)) - { - JLOG(j_.debug()) << "Amendment " << a.first << " is supported."; + AmendmentState& s = add(amendment, lock); - if (!a.second.empty()) - s->name = a.second; + s.name = name; + s.supported = true; + s.vote = defaultVote == DefaultVote::yes ? AmendmentVote::up + : AmendmentVote::down; - s->supported = true; - } + JLOG(j_.debug()) << "Amendment " << amendment << " (" << s.name + << ") is supported and will be " + << (s.vote == AmendmentVote::up ? "up" : "down") + << " voted if not enabled on the ledger."; } hash_set detect_conflict; // Parse enabled amendments from config - for (auto const& a : parseSection(enabled)) + for (std::pair const& a : parseSection(enabled)) { if (featureVotesExist) { // If the table existed, warn about duplicate config info @@ -358,7 +361,7 @@ AmendmentTableImpl::AmendmentTableImpl( else { // Otherwise transfer config data into the table detect_conflict.insert(a.first); - persistVote(a.first, a.second, false); // un-veto + persistVote(a.first, a.second, AmendmentVote::up); } } @@ -376,7 +379,7 @@ AmendmentTableImpl::AmendmentTableImpl( { // Otherwise transfer config data into the table if (detect_conflict.count(a.first) == 0) { - persistVote(a.first, a.second, true); // veto + persistVote(a.first, a.second, AmendmentVote::down); } else { @@ -394,57 +397,62 @@ AmendmentTableImpl::AmendmentTableImpl( *db, [&](boost::optional amendment_hash, boost::optional amendment_name, - boost::optional vote_to_veto) { + boost::optional vote) { uint256 amend_hash; + if (!amendment_hash || !amendment_name || !vote) + { + // These fields should never have nulls, but check + Throw( + "Invalid FeatureVotes row in wallet.db"); + } if (!amend_hash.parseHex(*amendment_hash)) { Throw( "Invalid amendment ID '" + *amendment_hash + " in wallet.db"); } - if (*vote_to_veto) + if (*vote == AmendmentVote::down) { // Unknown amendments are effectively vetoed already - if (auto s = get(amend_hash, sl)) + if (auto s = get(amend_hash, lock)) { JLOG(j_.info()) << "Amendment {" << *amendment_name << ", " - << amend_hash << "} is vetoed."; + << amend_hash << "} is downvoted."; if (!amendment_name->empty()) s->name = *amendment_name; - s->vetoed = true; + s->vote = *vote; } } - else // un-veto + else // up-vote { - if (auto s = add(amend_hash, sl)) - { - JLOG(j_.debug()) << "Amendment {" << *amendment_name << ", " - << amend_hash << "} is un-vetoed."; - if (!amendment_name->empty()) - s->name = *amendment_name; - s->vetoed = false; - } + auto s = add(amend_hash, lock); + + JLOG(j_.debug()) << "Amendment {" << *amendment_name << ", " + << amend_hash << "} is upvoted."; + if (!amendment_name->empty()) + s.name = *amendment_name; + s.vote = *vote; } }); } -AmendmentState* +AmendmentState& AmendmentTableImpl::add( uint256 const& amendmentHash, std::lock_guard const&) { // call with the mutex held - return &amendmentMap_[amendmentHash]; + return amendmentMap_[amendmentHash]; } AmendmentState* AmendmentTableImpl::get( uint256 const& amendmentHash, - std::lock_guard const& sl) + std::lock_guard const& lock) { // Forward to the const version of get. return const_cast( - std::as_const(*this).get(amendmentHash, sl)); + std::as_const(*this).get(amendmentHash, lock)); } AmendmentState const* @@ -464,7 +472,7 @@ AmendmentTableImpl::get( uint256 AmendmentTableImpl::find(std::string const& name) const { - std::lock_guard sl(mutex_); + std::lock_guard lock(mutex_); for (auto const& e : amendmentMap_) { @@ -479,50 +487,50 @@ void AmendmentTableImpl::persistVote( uint256 const& amendment, std::string const& name, - bool vote_to_veto) const + AmendmentVote vote) const { auto db = db_.checkoutDb(); - voteAmendment(*db, amendment, name, vote_to_veto); + voteAmendment(*db, amendment, name, vote); } bool AmendmentTableImpl::veto(uint256 const& amendment) { - std::lock_guard sl(mutex_); - auto s = add(amendment, sl); + std::lock_guard lock(mutex_); + AmendmentState& s = add(amendment, lock); - if (s->vetoed) + if (s.vote == AmendmentVote::down) return false; - s->vetoed = true; - persistVote(amendment, s->name, s->vetoed); + s.vote = AmendmentVote::down; + persistVote(amendment, s.name, s.vote); return true; } bool AmendmentTableImpl::unVeto(uint256 const& amendment) { - std::lock_guard sl(mutex_); - auto s = get(amendment, sl); + std::lock_guard lock(mutex_); + AmendmentState* const s = get(amendment, lock); - if (!s || !s->vetoed) + if (!s || s->vote == AmendmentVote::up) return false; - s->vetoed = false; - persistVote(amendment, s->name, s->vetoed); + s->vote = AmendmentVote::up; + persistVote(amendment, s->name, s->vote); return true; } bool AmendmentTableImpl::enable(uint256 const& amendment) { - std::lock_guard sl(mutex_); - auto s = add(amendment, sl); + std::lock_guard lock(mutex_); + AmendmentState& s = add(amendment, lock); - if (s->enabled) + if (s.enabled) return false; - s->enabled = true; + s.enabled = true; - if (!s->supported) + if (!s.supported) { JLOG(j_.error()) << "Unsupported amendment " << amendment << " activated."; @@ -535,30 +543,30 @@ AmendmentTableImpl::enable(uint256 const& amendment) bool AmendmentTableImpl::isEnabled(uint256 const& amendment) const { - std::lock_guard sl(mutex_); - auto s = get(amendment, sl); + std::lock_guard lock(mutex_); + AmendmentState const* s = get(amendment, lock); return s && s->enabled; } bool AmendmentTableImpl::isSupported(uint256 const& amendment) const { - std::lock_guard sl(mutex_); - auto s = get(amendment, sl); + std::lock_guard lock(mutex_); + AmendmentState const* s = get(amendment, lock); return s && s->supported; } bool AmendmentTableImpl::hasUnsupportedEnabled() const { - std::lock_guard sl(mutex_); + std::lock_guard lock(mutex_); return unsupportedEnabled_; } std::optional AmendmentTableImpl::firstUnsupportedExpected() const { - std::lock_guard sl(mutex_); + std::lock_guard lock(mutex_); return firstUnsupportedExpected_; } @@ -570,14 +578,15 @@ AmendmentTableImpl::doValidation(std::set const& enabled) const std::vector amendments; { - std::lock_guard sl(mutex_); + std::lock_guard lock(mutex_); amendments.reserve(amendmentMap_.size()); for (auto const& e : amendmentMap_) { - if (e.second.supported && !e.second.vetoed && + if (e.second.supported && e.second.vote == AmendmentVote::up && (enabled.count(e.first) == 0)) { amendments.push_back(e.first); + JLOG(j_.info()) << "Voting for amendment " << e.second.name; } } } @@ -617,7 +626,7 @@ AmendmentTableImpl::doVoting( // the value of the flags in the pseudo-transaction std::map actions; - std::lock_guard sl(mutex_); + std::lock_guard lock(mutex_); // process all amendments we know of for (auto const& entry : amendmentMap_) @@ -638,7 +647,7 @@ AmendmentTableImpl::doVoting( } else if ( hasValMajority && (majorityTime == NetClock::time_point{}) && - !entry.second.vetoed) + entry.second.vote == AmendmentVote::up) { // Ledger says no majority, validators say yes JLOG(j_.debug()) << entry.first << ": amendment got majority"; @@ -653,7 +662,7 @@ AmendmentTableImpl::doVoting( else if ( (majorityTime != NetClock::time_point{}) && ((majorityTime + majorityTime_) <= closeTime) && - !entry.second.vetoed) + entry.second.vote == AmendmentVote::up) { // Ledger says majority held JLOG(j_.debug()) << entry.first << ": amendment majority held"; @@ -669,7 +678,7 @@ AmendmentTableImpl::doVoting( bool AmendmentTableImpl::needValidatedLedger(LedgerIndex ledgerSeq) const { - std::lock_guard sl(mutex_); + std::lock_guard lock(mutex_); // Is there a ledger in which an amendment could have been enabled // between these two ledger sequences? @@ -686,7 +695,7 @@ AmendmentTableImpl::doValidatedLedger( for (auto& e : enabled) enable(e); - std::lock_guard sl(mutex_); + std::lock_guard lock(mutex_); // Remember the ledger sequence of this update. lastUpdateSeq_ = ledgerSeq; @@ -697,12 +706,12 @@ AmendmentTableImpl::doValidatedLedger( firstUnsupportedExpected_.reset(); for (auto const& [hash, time] : majority) { - auto s = add(hash, sl); + AmendmentState& s = add(hash, lock); - if (s->enabled) + if (s.enabled) continue; - if (!s->supported) + if (!s.supported) { JLOG(j_.info()) << "Unsupported amendment " << hash << " reached majority at " << to_string(time); @@ -725,7 +734,7 @@ AmendmentTableImpl::injectJson( v[jss::name] = fs.name; v[jss::supported] = fs.supported; - v[jss::vetoed] = fs.vetoed; + v[jss::vetoed] = fs.vote == AmendmentVote::down; v[jss::enabled] = fs.enabled; if (!fs.enabled && lastVote_) @@ -747,14 +756,14 @@ AmendmentTableImpl::getJson() const { Json::Value ret(Json::objectValue); { - std::lock_guard sl(mutex_); + std::lock_guard lock(mutex_); for (auto const& e : amendmentMap_) { injectJson( ret[to_string(e.first)] = Json::objectValue, e.first, e.second, - sl); + lock); } } return ret; @@ -767,10 +776,10 @@ AmendmentTableImpl::getJson(uint256 const& amendmentID) const Json::Value& jAmendment = (ret[to_string(amendmentID)] = Json::objectValue); { - std::lock_guard sl(mutex_); - auto a = get(amendmentID, sl); + std::lock_guard lock(mutex_); + AmendmentState const* a = get(amendmentID, lock); if (a) - injectJson(jAmendment, amendmentID, *a, sl); + injectJson(jAmendment, amendmentID, *a, lock); } return ret; @@ -780,7 +789,7 @@ std::unique_ptr make_AmendmentTable( Application& app, std::chrono::seconds majorityTime, - Section const& supported, + std::vector const& supported, Section const& enabled, Section const& vetoed, beast::Journal journal) diff --git a/src/ripple/app/rdb/RelationalDBInterface_global.h b/src/ripple/app/rdb/RelationalDBInterface_global.h index aa50b2e97d1..3eb0469ee8e 100644 --- a/src/ripple/app/rdb/RelationalDBInterface_global.h +++ b/src/ripple/app/rdb/RelationalDBInterface_global.h @@ -132,11 +132,15 @@ deletePeerReservation(soci::session& session, PublicKey const& nodeId); bool createFeatureVotes(soci::session& session); +// For historical reasons the up-vote and down-vote integer representations +// are unintuitive. +enum class AmendmentVote : int { up = 0, down = 1 }; + /** * @brief readAmendments Read all amendments from FeatureVotes table. * @param session Session with walletDB database. * @param callback Callback called for each amendment passing its hash, name - * and teh flag if it should be vetoed as callback parameters + * and the flag if it should be vetoed as callback parameters */ void readAmendments( @@ -144,21 +148,21 @@ readAmendments( std::function amendment_hash, boost::optional amendment_name, - boost::optional vote_to_veto)> const& callback); + boost::optional vote)> const& callback); /** * @brief voteAmendment Set veto value for particular amendment. * @param session Session with walletDB database. * @param amendment Hash of amendment. * @param name Name of amendment. - * @param vote_to_veto Trus if this amendment should be vetoed. + * @param vote Whether to vote in favor of this amendment. */ void voteAmendment( soci::session& session, uint256 const& amendment, std::string const& name, - bool vote_to_veto); + AmendmentVote vote); /* State DB */ diff --git a/src/ripple/app/rdb/impl/RelationalDBInterface_global.cpp b/src/ripple/app/rdb/impl/RelationalDBInterface_global.cpp index 5c000fc9ace..17b86f0cabc 100644 --- a/src/ripple/app/rdb/impl/RelationalDBInterface_global.cpp +++ b/src/ripple/app/rdb/impl/RelationalDBInterface_global.cpp @@ -258,8 +258,14 @@ readAmendments( std::function amendment_hash, boost::optional amendment_name, - boost::optional vote_to_veto)> const& callback) + boost::optional vote)> const& callback) { + // lambda that converts the internally stored int to an AmendmentVote. + auto intToVote = [](boost::optional const& dbVote) + -> boost::optional { + return safe_cast(dbVote.value_or(1)); + }; + soci::transaction tr(session); std::string sql = "SELECT AmendmentHash, AmendmentName, Veto FROM FeatureVotes"; @@ -275,7 +281,7 @@ readAmendments( st.execute(); while (st.fetch()) { - callback(amendment_hash, amendment_name, vote_to_veto); + callback(amendment_hash, amendment_name, intToVote(vote_to_veto)); } } @@ -284,7 +290,7 @@ voteAmendment( soci::session& session, uint256 const& amendment, std::string const& name, - bool vote_to_veto) + AmendmentVote vote) { soci::transaction tr(session); std::string sql = @@ -292,7 +298,7 @@ voteAmendment( "('"; sql += to_string(amendment); sql += "', '" + name; - sql += "', '" + std::to_string(int{vote_to_veto}) + "');"; + sql += "', '" + std::to_string(safe_cast(vote)) + "');"; session << sql; tr.commit(); } diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index 3fb67fa0811..d65e8f8f074 100644 --- a/src/ripple/protocol/Feature.h +++ b/src/ripple/protocol/Feature.h @@ -32,120 +32,69 @@ * * Steps required to add new features to the code: * - * 1) add the new feature name to the featureNames array below - * 2) add a uint256 declaration for the feature to the bottom of this file - * 3) add a uint256 definition for the feature to the corresponding source - * file (Feature.cpp) - * 4) if the feature is going to be supported in the near future, add its - * sha512half value and name (matching exactly the featureName here) to - * the supportedAmendments in Feature.cpp. + * 1) In this file, increment `numFeatures` and add a uint256 declaration + * for the feature at the bottom + * 2) Add a uint256 definition for the feature to the corresponding source + * file (Feature.cpp). Use `registerFeature` to create the feature with + * the feature's name, `Supported::no`, and `DefaultVote::no`. This + * should be the only place the feature's name appears in code as a string. + * 3) Use the uint256 as the parameter to `view.rules.enabled()` to + * control flow into new code that this feature limits. + * 4) If the feature development is COMPLETE, and the feature is ready to be + * SUPPORTED, change the `registerFeature` parameter to Supported::yes. + * 5) When the feature is ready to be ENABLED, change the `registerFeature` + * parameter to `DefaultVote::yes`. + * In general, any newly supported amendments (`Supported::yes`) should have + * a `DefaultVote::no` for at least one full release cycle. High priority + * bug fixes can be an exception to this rule of thumb. + * + * When a feature has been enabled for several years, the conditional code + * may be removed, and the feature "retired". To retire a feature: + * 1) Remove the uint256 declaration from this file. + * 2) MOVE the uint256 definition in Feature.cpp to the "retired features" + * section at the end of the file. + * 3) CHANGE the name of the variable to start with "retired". + * 4) CHANGE the parameters of the `registerFeature` call to `Supported::yes` + * and `DefaultVote::no`. + * The feature must remain registered and supported indefinitely because it + * still exists in the ledger, but there is no need to vote for it because + * there's nothing to vote for. If it is removed completely from the code, any + * instances running that code will get amendment blocked. Removing the + * feature from the ledger is beyond the scope of these instructions. * */ namespace ripple { +enum class DefaultVote : bool { no = false, yes }; + namespace detail { -// *NOTE* -// -// Features, or Amendments as they are called elsewhere, are enabled on the -// network at some specific time based on Validator voting. Features are -// enabled using run-time conditionals based on the state of the amendment. -// There is value in retaining that conditional code for some time after -// the amendment is enabled to make it simple to replay old transactions. -// However, once an Amendment has been enabled for, say, more than two years -// then retaining that conditional code has less value since it is -// uncommon to replay such old transactions. -// -// Starting in January of 2020 Amendment conditionals from before January -// 2018 are being removed. So replaying any ledger from before January -// 2018 needs to happen on an older version of the server code. There's -// a log message in Application.cpp that warns about replaying old ledgers. -// -// At some point in the future someone may wish to remove Amendment -// conditional code for Amendments that were enabled after January 2018. -// When that happens then the log message in Application.cpp should be -// updated. - -class FeatureCollections -{ - static constexpr char const* const featureNames[] = { - "MultiSign", // Unconditionally supported. - "TrustSetAuth", // Unconditionally supported. - "FeeEscalation", // Unconditionally supported. - "OwnerPaysFee", - "PayChan", - "Flow", // Unconditionally supported. - "CompareTakerFlowCross", - "FlowCross", - "CryptoConditions", - "TickSize", - "fix1368", - "Escrow", - "CryptoConditionsSuite", - "fix1373", - "EnforceInvariants", - "SortedDirectories", - "fix1201", - "fix1512", - "fix1513", - "fix1523", - "fix1528", - "DepositAuth", - "Checks", - "fix1571", - "fix1543", - "fix1623", - "DepositPreauth", - "fix1515", - "fix1578", - "MultiSignReserve", - "fixTakerDryOfferRemoval", - "fixMasterKeyAsRegularKey", - "fixCheckThreading", - "fixPayChanRecipientOwnerDir", - "DeletableAccounts", - // fixQualityUpperBound should be activated before FlowCross - "fixQualityUpperBound", - "RequireFullyCanonicalSig", - "fix1781", // XRPEndpointSteps should be included in the circular - // payment check - "HardenedValidations", - "fixAmendmentMajorityCalc", // Fix Amendment majority calculation - "NegativeUNL", - "TicketBatch", - "FlowSortStrands", - "fixSTAmountCanonicalize", - "fixRmSmallIncreasedQOffers", - "CheckCashMakesTrustLine", - }; - - std::vector features; - boost::container::flat_map featureToIndex; - boost::container::flat_map nameToFeature; +// This value SHOULD be equal to the number of amendments registered in +// Feature.cpp. Because it's only used to reserve storage, and determine how +// large to make the FeatureBitset, it MAY be larger. It MUST NOT be less than +// the actual number of amendments. A LogicError on startup will verify this. +static constexpr std::size_t numFeatures = 46; -public: - FeatureCollections(); - - static constexpr std::size_t - numFeatures() - { - return sizeof(featureNames) / sizeof(featureNames[0]); - } +/** Amendments that this server supports and the default voting behavior. + Whether they are enabled depends on the Rules defined in the validated + ledger */ +std::map const& +supportedAmendments(); - std::optional - getRegisteredFeature(std::string const& name) const; +/** Amendments that this server won't vote for by default. - std::size_t - featureToBitsetIndex(uint256 const& f) const; + This function is only used in unit tests. +*/ +std::size_t +numDownVotedAmendments(); - uint256 const& - bitsetIndexToFeature(size_t i) const; -}; +/** Amendments that this server will vote for by default. -/** Amendments that this server supports, but doesn't enable by default */ -std::vector const& -supportedAmendments(); + This function is only used in unit tests. +*/ +std::size_t +numUpVotedAmendments(); } // namespace detail @@ -158,10 +107,12 @@ featureToBitsetIndex(uint256 const& f); uint256 bitsetIndexToFeature(size_t i); -class FeatureBitset - : private std::bitset +std::string +featureToName(uint256 const& f); + +class FeatureBitset : private std::bitset { - using base = std::bitset; + using base = std::bitset; template void @@ -195,12 +146,14 @@ class FeatureBitset explicit FeatureBitset(base const& b) : base(b) { + assert(b.count() == count()); } template explicit FeatureBitset(uint256 const& f, Fs&&... fs) { initFromFeatures(f, std::forward(fs)...); + assert(count() == (sizeof...(fs) + 1)); } template @@ -208,6 +161,7 @@ class FeatureBitset { for (auto const& f : fs) set(featureToBitsetIndex(f)); + assert(fs.size() == count()); } auto diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index 5c7910fe95d..e1d82cb1b21 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -17,128 +17,317 @@ */ //============================================================================== -#include #include -#include +#include +#include +#include +#include +#include +#include +#include +#include #include namespace ripple { -//------------------------------------------------------------------------------ +inline std::size_t +hash_value(ripple::uint256 const& feature) +{ + std::size_t seed = 0; + using namespace boost; + for (auto const& n : feature) + hash_combine(seed, n); + return seed; +} + +namespace { + +enum class Supported : bool { no = false, yes }; -constexpr char const* const detail::FeatureCollections::featureNames[]; +// *NOTE* +// +// Features, or Amendments as they are called elsewhere, are enabled on the +// network at some specific time based on Validator voting. Features are +// enabled using run-time conditionals based on the state of the amendment. +// There is value in retaining that conditional code for some time after +// the amendment is enabled to make it simple to replay old transactions. +// However, once an Amendment has been enabled for, say, more than two years +// then retaining that conditional code has less value since it is +// uncommon to replay such old transactions. +// +// Starting in January of 2020 Amendment conditionals from before January +// 2018 are being removed. So replaying any ledger from before January +// 2018 needs to happen on an older version of the server code. There's +// a log message in Application.cpp that warns about replaying old ledgers. +// +// At some point in the future someone may wish to remove Amendment +// conditional code for Amendments that were enabled after January 2018. +// When that happens then the log message in Application.cpp should be +// updated. -detail::FeatureCollections::FeatureCollections() +class FeatureCollections { - features.reserve(numFeatures()); - featureToIndex.reserve(numFeatures()); - nameToFeature.reserve(numFeatures()); + struct Feature + { + std::string name; + uint256 feature; + + Feature() = delete; + explicit Feature(std::string const& name_, uint256 const& feature_) + : name(name_), feature(feature_) + { + } + + // These structs are used by the `features` multi_index_container to + // provide access to the features collection by size_t index, string + // name, and uint256 feature identifier + struct byIndex + { + }; + struct byName + { + }; + struct byFeature + { + }; + }; + + // Intermediate types to help with readability + template + using feature_hashed_unique = boost::multi_index::hashed_unique< + boost::multi_index::tag, + boost::multi_index::member>; + + // Intermediate types to help with readability + using feature_indexing = boost::multi_index::indexed_by< + boost::multi_index::random_access< + boost::multi_index::tag>, + feature_hashed_unique, + feature_hashed_unique>; + + // This multi_index_container provides access to the features collection by + // name, index, and uint256 feature identifier + boost::multi_index::multi_index_container + features; + std::map supported; + std::size_t upVotes = 0; + std::size_t downVotes = 0; + mutable std::atomic readOnly = false; + + // These helper functions provide access to the features collection by name, + // index, and uint256 feature identifier, so the details of + // multi_index_container can be hidden + Feature const& + getByIndex(size_t i) const + { + if (i >= features.size()) + LogicError("Invalid FeatureBitset index"); + const auto& sequence = features.get(); + return sequence[i]; + } + size_t + getIndex(Feature const& feature) const + { + const auto& sequence = features.get(); + auto const it_to = sequence.iterator_to(feature); + return it_to - sequence.begin(); + } + Feature const* + getByFeature(uint256 const& feature) const + { + const auto& feature_index = features.get(); + auto const feature_it = feature_index.find(feature); + return feature_it == feature_index.end() ? nullptr : &*feature_it; + } + Feature const* + getByName(std::string const& name) const + { + const auto& name_index = features.get(); + auto const name_it = name_index.find(name); + return name_it == name_index.end() ? nullptr : &*name_it; + } + +public: + FeatureCollections(); + + std::optional + getRegisteredFeature(std::string const& name) const; + + uint256 + registerFeature( + std::string const& name, + Supported support, + DefaultVote vote); + + /** Tell FeatureCollections when registration is complete. */ + bool + registrationIsDone(); + + std::size_t + featureToBitsetIndex(uint256 const& f) const; + + uint256 const& + bitsetIndexToFeature(size_t i) const; + + std::string + featureToName(uint256 const& f) const; + + /** Amendments that this server supports. + Whether they are enabled depends on the Rules defined in the validated + ledger */ + std::map const& + supportedAmendments() const + { + return supported; + } + + /** Amendments that this server WON'T vote for by default. */ + std::size_t + numDownVotedAmendments() const + { + return downVotes; + } - for (std::size_t i = 0; i < numFeatures(); ++i) + /** Amendments that this server WILL vote for by default. */ + std::size_t + numUpVotedAmendments() const { - auto const name = featureNames[i]; - sha512_half_hasher h; - h(name, std::strlen(name)); - auto const f = static_cast(h); - - features.push_back(f); - featureToIndex[f] = i; - nameToFeature[name] = f; + return upVotes; } +}; + +//------------------------------------------------------------------------------ + +FeatureCollections::FeatureCollections() +{ + features.reserve(ripple::detail::numFeatures); } std::optional -detail::FeatureCollections::getRegisteredFeature(std::string const& name) const +FeatureCollections::getRegisteredFeature(std::string const& name) const { - auto const i = nameToFeature.find(name); - if (i == nameToFeature.end()) - return std::nullopt; - return i->second; + assert(readOnly); + Feature const* feature = getByName(name); + if (feature) + return feature->feature; + return std::nullopt; +} + +void +check(bool condition, const char* logicErrorMessage) +{ + if (!condition) + LogicError(logicErrorMessage); +} + +uint256 +FeatureCollections::registerFeature( + std::string const& name, + Supported support, + DefaultVote vote) +{ + check(!readOnly, "Attempting to register a feature after startup."); + check( + support == Supported::yes || vote == DefaultVote::no, + "Invalid feature parameters. Must be supported to be up-voted."); + Feature const* i = getByName(name); + if (!i) + { + // If this check fails, and you just added a feature, increase the + // numFeatures value in Feature.h + check( + features.size() < detail::numFeatures, + "More features defined than allocated. Adjust numFeatures in " + "Feature.h."); + + auto const f = sha512Half(Slice(name.data(), name.size())); + + features.emplace_back(name, f); + + if (support == Supported::yes) + { + supported.emplace(name, vote); + + if (vote == DefaultVote::yes) + ++upVotes; + else + ++downVotes; + } + check( + upVotes + downVotes == supported.size(), + "Feature counting logic broke"); + check( + supported.size() <= features.size(), + "More supported features than defined features"); + return f; + } + else + // Each feature should only be registered once + LogicError("Duplicate feature registration"); +} + +/** Tell FeatureCollections when registration is complete. */ +bool +FeatureCollections::registrationIsDone() +{ + readOnly = true; + return true; } size_t -detail::FeatureCollections::featureToBitsetIndex(uint256 const& f) const +FeatureCollections::featureToBitsetIndex(uint256 const& f) const { - auto const i = featureToIndex.find(f); - if (i == featureToIndex.end()) + assert(readOnly); + + Feature const* feature = getByFeature(f); + if (!feature) LogicError("Invalid Feature ID"); - return i->second; + + return getIndex(*feature); } uint256 const& -detail::FeatureCollections::bitsetIndexToFeature(size_t i) const +FeatureCollections::bitsetIndexToFeature(size_t i) const { - if (i >= features.size()) - LogicError("Invalid FeatureBitset index"); - return features[i]; + assert(readOnly); + Feature const& feature = getByIndex(i); + return feature.feature; } -static detail::FeatureCollections const featureCollections; +std::string +FeatureCollections::featureToName(uint256 const& f) const +{ + assert(readOnly); + Feature const* feature = getByFeature(f); + return feature ? feature->name : to_string(f); +} + +static FeatureCollections featureCollections; -/** Amendments that this server supports, but doesn't enable by default */ -std::vector const& +} // namespace + +/** Amendments that this server supports. + Whether they are enabled depends on the Rules defined in the validated + ledger */ +std::map const& detail::supportedAmendments() { - // Commented out amendments will be supported in a future release (and - // uncommented at that time). - // - // There are also unconditionally supported amendments in the list. - // Those are amendments that were enabled some time ago and the - // amendment conditional code has been removed. - // - // ** WARNING ** - // Unconditionally supported amendments need to remain in the list. - // Removing them will cause servers to become amendment blocked. - static std::vector const supported{ - "MultiSign", // Unconditionally supported. - "TrustSetAuth", // Unconditionally supported. - "FeeEscalation", // Unconditionally supported. - // "OwnerPaysFee", - "PayChan", - "Flow", - "CryptoConditions", - "TickSize", - "fix1368", - "Escrow", - "CryptoConditionsSuite", - "fix1373", - "EnforceInvariants", - "FlowCross", - "SortedDirectories", - "fix1201", - "fix1512", - "fix1513", - "fix1523", - "fix1528", - "DepositAuth", - "Checks", - "fix1571", - "fix1543", - "fix1623", - "DepositPreauth", - // Use liquidity from strands that consume max offers, but mark as dry - "fix1515", - "fix1578", - "MultiSignReserve", - "fixTakerDryOfferRemoval", - "fixMasterKeyAsRegularKey", - "fixCheckThreading", - "fixPayChanRecipientOwnerDir", - "DeletableAccounts", - "fixQualityUpperBound", - "RequireFullyCanonicalSig", - "fix1781", - "HardenedValidations", - "fixAmendmentMajorityCalc", - "NegativeUNL", - "TicketBatch", - "FlowSortStrands", - "fixSTAmountCanonicalize", - "fixRmSmallIncreasedQOffers", - "CheckCashMakesTrustLine", - }; - return supported; + return featureCollections.supportedAmendments(); +} + +/** Amendments that this server won't vote for by default. */ +std::size_t +detail::numDownVotedAmendments() +{ + return featureCollections.numDownVotedAmendments(); +} + +/** Amendments that this server will vote for by default. */ +std::size_t +detail::numUpVotedAmendments() +{ + return featureCollections.numUpVotedAmendments(); } //------------------------------------------------------------------------------ @@ -149,6 +338,27 @@ getRegisteredFeature(std::string const& name) return featureCollections.getRegisteredFeature(name); } +uint256 +registerFeature(std::string const& name, Supported support, DefaultVote vote) +{ + return featureCollections.registerFeature(name, support, vote); +} + +// Retired features are in the ledger and have no code controlled by the +// feature. They need to be supported, but do not need to be voted on. +uint256 +retireFeature(std::string const& name) +{ + return registerFeature(name, Supported::yes, DefaultVote::no); +} + +/** Tell FeatureCollections when registration is complete. */ +bool +registrationIsDone() +{ + return featureCollections.registrationIsDone(); +} + size_t featureToBitsetIndex(uint256 const& f) { @@ -161,57 +371,109 @@ bitsetIndexToFeature(size_t i) return featureCollections.bitsetIndexToFeature(i); } +std::string +featureToName(uint256 const& f) +{ + return featureCollections.featureToName(f); +} + +#pragma push_macro("REGISTER_FEATURE") +#undef REGISTER_FEATURE + +/** +Takes the name of a feature, whether it's supported, and the default vote. Will +register the feature, and create a variable whose name is "feature" plus the +feature name. +*/ +#define REGISTER_FEATURE(fName, supported, defaultvote) \ + uint256 const feature##fName = \ + registerFeature(#fName, supported, defaultvote) + +#pragma push_macro("REGISTER_FIX") +#undef REGISTER_FIX + +/** +Takes the name of a feature, whether it's supported, and the default vote. Will +register the feature, and create a variable whose name is the unmodified feature +name. +*/ +#define REGISTER_FIX(fName, supported, defaultvote) \ + uint256 const fName = registerFeature(#fName, supported, defaultvote) + // clang-format off -uint256 const - featureOwnerPaysFee = *getRegisteredFeature("OwnerPaysFee"), - featureFlow = *getRegisteredFeature("Flow"), - featureFlowCross = *getRegisteredFeature("FlowCross"), - featureCryptoConditionsSuite = *getRegisteredFeature("CryptoConditionsSuite"), - fix1513 = *getRegisteredFeature("fix1513"), - featureDepositAuth = *getRegisteredFeature("DepositAuth"), - featureChecks = *getRegisteredFeature("Checks"), - fix1571 = *getRegisteredFeature("fix1571"), - fix1543 = *getRegisteredFeature("fix1543"), - fix1623 = *getRegisteredFeature("fix1623"), - featureDepositPreauth = *getRegisteredFeature("DepositPreauth"), - fix1515 = *getRegisteredFeature("fix1515"), - fix1578 = *getRegisteredFeature("fix1578"), - featureMultiSignReserve = *getRegisteredFeature("MultiSignReserve"), - fixTakerDryOfferRemoval = *getRegisteredFeature("fixTakerDryOfferRemoval"), - fixMasterKeyAsRegularKey = *getRegisteredFeature("fixMasterKeyAsRegularKey"), - fixCheckThreading = *getRegisteredFeature("fixCheckThreading"), - fixPayChanRecipientOwnerDir = *getRegisteredFeature("fixPayChanRecipientOwnerDir"), - featureDeletableAccounts = *getRegisteredFeature("DeletableAccounts"), - fixQualityUpperBound = *getRegisteredFeature("fixQualityUpperBound"), - featureRequireFullyCanonicalSig = *getRegisteredFeature("RequireFullyCanonicalSig"), - fix1781 = *getRegisteredFeature("fix1781"), - featureHardenedValidations = *getRegisteredFeature("HardenedValidations"), - fixAmendmentMajorityCalc = *getRegisteredFeature("fixAmendmentMajorityCalc"), - featureNegativeUNL = *getRegisteredFeature("NegativeUNL"), - featureTicketBatch = *getRegisteredFeature("TicketBatch"), - featureFlowSortStrands = *getRegisteredFeature("FlowSortStrands"), - fixSTAmountCanonicalize = *getRegisteredFeature("fixSTAmountCanonicalize"), - fixRmSmallIncreasedQOffers = *getRegisteredFeature("fixRmSmallIncreasedQOffers"), - featureCheckCashMakesTrustLine = *getRegisteredFeature("CheckCashMakesTrustLine"); +// All known amendments must be registered either here or below with the +// "retired" amendments +REGISTER_FEATURE(OwnerPaysFee, Supported::no, DefaultVote::no); +REGISTER_FEATURE(Flow, Supported::yes, DefaultVote::yes); +REGISTER_FEATURE(FlowCross, Supported::yes, DefaultVote::yes); +REGISTER_FEATURE(CryptoConditionsSuite, Supported::yes, DefaultVote::no); +REGISTER_FIX (fix1513, Supported::yes, DefaultVote::yes); +REGISTER_FEATURE(DepositAuth, Supported::yes, DefaultVote::yes); +REGISTER_FEATURE(Checks, Supported::yes, DefaultVote::yes); +REGISTER_FIX (fix1571, Supported::yes, DefaultVote::yes); +REGISTER_FIX (fix1543, Supported::yes, DefaultVote::yes); +REGISTER_FIX (fix1623, Supported::yes, DefaultVote::yes); +REGISTER_FEATURE(DepositPreauth, Supported::yes, DefaultVote::yes); +// Use liquidity from strands that consume max offers, but mark as dry +REGISTER_FIX (fix1515, Supported::yes, DefaultVote::yes); +REGISTER_FIX (fix1578, Supported::yes, DefaultVote::yes); +REGISTER_FEATURE(MultiSignReserve, Supported::yes, DefaultVote::yes); +REGISTER_FIX (fixTakerDryOfferRemoval, Supported::yes, DefaultVote::yes); +REGISTER_FIX (fixMasterKeyAsRegularKey, Supported::yes, DefaultVote::yes); +REGISTER_FIX (fixCheckThreading, Supported::yes, DefaultVote::yes); +REGISTER_FIX (fixPayChanRecipientOwnerDir, Supported::yes, DefaultVote::yes); +REGISTER_FEATURE(DeletableAccounts, Supported::yes, DefaultVote::yes); +// fixQualityUpperBound should be activated before FlowCross +REGISTER_FIX (fixQualityUpperBound, Supported::yes, DefaultVote::yes); +REGISTER_FEATURE(RequireFullyCanonicalSig, Supported::yes, DefaultVote::yes); +// fix1781: XRPEndpointSteps should be included in the circular payment check +REGISTER_FIX (fix1781, Supported::yes, DefaultVote::yes); +REGISTER_FEATURE(HardenedValidations, Supported::yes, DefaultVote::yes); +REGISTER_FIX (fixAmendmentMajorityCalc, Supported::yes, DefaultVote::yes); +REGISTER_FEATURE(NegativeUNL, Supported::yes, DefaultVote::no); +REGISTER_FEATURE(TicketBatch, Supported::yes, DefaultVote::yes); +REGISTER_FEATURE(FlowSortStrands, Supported::yes, DefaultVote::yes); +REGISTER_FIX (fixSTAmountCanonicalize, Supported::yes, DefaultVote::yes); +REGISTER_FIX (fixRmSmallIncreasedQOffers, Supported::yes, DefaultVote::yes); +REGISTER_FEATURE(CheckCashMakesTrustLine, Supported::yes, DefaultVote::no); // The following amendments have been active for at least two years. Their // pre-amendment code has been removed and the identifiers are deprecated. +// All known amendments and amendments that may appear in a validated +// ledger must be registered either here or above with the "active" amendments [[deprecated("The referenced amendment has been retired"), maybe_unused]] uint256 const - retiredPayChan = *getRegisteredFeature("PayChan"), - retiredCryptoConditions = *getRegisteredFeature("CryptoConditions"), - retiredTickSize = *getRegisteredFeature("TickSize"), - retiredFix1368 = *getRegisteredFeature("fix1368"), - retiredEscrow = *getRegisteredFeature("Escrow"), - retiredFix1373 = *getRegisteredFeature("fix1373"), - retiredEnforceInvariants = *getRegisteredFeature("EnforceInvariants"), - retiredSortedDirectories = *getRegisteredFeature("SortedDirectories"), - retiredFix1201 = *getRegisteredFeature("fix1201"), - retiredFix1512 = *getRegisteredFeature("fix1512"), - retiredFix1523 = *getRegisteredFeature("fix1523"), - retiredFix1528 = *getRegisteredFeature("fix1528"); + retiredMultiSign = retireFeature("MultiSign"), + retiredTrustSetAuth = retireFeature("TrustSetAuth"), + retiredFeeEscalation = retireFeature("FeeEscalation"), + retiredPayChan = retireFeature("PayChan"), + retiredCryptoConditions = retireFeature("CryptoConditions"), + retiredTickSize = retireFeature("TickSize"), + retiredFix1368 = retireFeature("fix1368"), + retiredEscrow = retireFeature("Escrow"), + retiredFix1373 = retireFeature("fix1373"), + retiredEnforceInvariants = retireFeature("EnforceInvariants"), + retiredSortedDirectories = retireFeature("SortedDirectories"), + retiredFix1201 = retireFeature("fix1201"), + retiredFix1512 = retireFeature("fix1512"), + retiredFix1523 = retireFeature("fix1523"), + retiredFix1528 = retireFeature("fix1528"); // clang-format on +#undef REGISTER_FIX +#pragma pop_macro("REGISTER_FIX") + +#undef REGISTER_FEATURE +#pragma pop_macro("REGISTER_FEATURE") + +// All of the features should now be registered, since variables in a cpp file +// are initialized from top to bottom. +// +// Use initialization of one final static variable to set +// featureCollections::readOnly. +[[maybe_unused]] static const bool readOnlySet = + featureCollections.registrationIsDone(); + } // namespace ripple diff --git a/src/ripple/rpc/handlers/Feature1.cpp b/src/ripple/rpc/handlers/Feature1.cpp index b01281a347c..1858a062e53 100644 --- a/src/ripple/rpc/handlers/Feature1.cpp +++ b/src/ripple/rpc/handlers/Feature1.cpp @@ -62,6 +62,8 @@ doFeature(RPC::JsonContext& context) auto feature = table.find(context.params[jss::feature].asString()); + // If the feature is not found by name, try to parse the `feature` param as + // a feature ID. If that fails, return an error. if (!feature && !feature.parseHex(context.params[jss::feature].asString())) return rpcError(rpcBAD_FEATURE); diff --git a/src/test/app/AmendmentTable_test.cpp b/src/test/app/AmendmentTable_test.cpp index 1f626c0a30e..99922e863a4 100644 --- a/src/test/app/AmendmentTable_test.cpp +++ b/src/test/app/AmendmentTable_test.cpp @@ -86,11 +86,31 @@ class AmendmentTable_test final : public beast::unit_test::suite return cfg; } + static std::vector + makeDefaultYes(std::vector const& amendments) + { + std::vector result; + result.reserve(amendments.size()); + for (auto const& a : amendments) + { + result.emplace_back(a, amendmentId(a), DefaultVote::yes); + } + return result; + } + + static std::vector + makeDefaultYes(uint256 const amendment) + { + std::vector result{ + {to_string(amendment), amendment, DefaultVote::yes}}; + return result; + } + // All useful amendments are supported amendments. // Enabled amendments are typically a subset of supported amendments. // Vetoed amendments should be supported but not enabled. // Unsupported amendments may be added to the AmendmentTable. - std::vector const supported_{ + std::vector const supportedYes_{ "a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l", "m", "n", "o", "p", "q", "r", "s", "t", "u"}; std::vector const @@ -100,6 +120,7 @@ class AmendmentTable_test final : public beast::unit_test::suite std::vector const unsupportedMajority_{"y", "z"}; Section const emptySection; + std::vector const emptyYes; test::SuiteJournal journal; @@ -112,7 +133,7 @@ class AmendmentTable_test final : public beast::unit_test::suite makeTable( Application& app, std::chrono::seconds majorityTime, - Section const& supported, + std::vector const& supported, Section const& enabled, Section const& vetoed) { @@ -124,7 +145,7 @@ class AmendmentTable_test final : public beast::unit_test::suite makeTable( test::jtx::Env& env, std::chrono::seconds majorityTime, - Section const& supported, + std::vector const& supported, Section const& enabled, Section const& vetoed) { @@ -137,7 +158,7 @@ class AmendmentTable_test final : public beast::unit_test::suite return makeTable( env.app(), majorityTime, - makeSection(supported_), + makeDefaultYes(supportedYes_), makeSection(enabled_), makeSection(vetoed_)); } @@ -149,7 +170,7 @@ class AmendmentTable_test final : public beast::unit_test::suite test::jtx::Env env{*this, makeConfig()}; auto table = makeTable(env, weeks(1)); - for (auto const& a : supported_) + for (auto const& a : supportedYes_) { BEAST_EXPECT(table->isSupported(amendmentId(a))); } @@ -174,7 +195,7 @@ class AmendmentTable_test final : public beast::unit_test::suite test::jtx::Env env{*this, makeConfig()}; auto table = makeTable(env, weeks(1)); - for (auto const& a : supported_) + for (auto const& a : supportedYes_) BEAST_EXPECT(table->find(a) == amendmentId(a)); for (auto const& a : enabled_) BEAST_EXPECT(table->find(a) == amendmentId(a)); @@ -207,7 +228,8 @@ class AmendmentTable_test final : public beast::unit_test::suite void testBadConfig() { - auto const section = makeSection(supported_); + auto const yesVotes = makeDefaultYes(supportedYes_); + auto const section = makeSection(vetoed_); auto const id = to_string(amendmentId(enabled_[0])); testcase("Bad Config"); @@ -219,12 +241,13 @@ class AmendmentTable_test final : public beast::unit_test::suite try { test::jtx::Env env{*this, makeConfig()}; - if (makeTable(env, weeks(2), test, emptySection, emptySection)) + if (makeTable(env, weeks(2), yesVotes, test, emptySection)) fail("Accepted only amendment ID"); } - catch (...) + catch (std::exception const& e) { - pass(); + BEAST_EXPECT( + e.what() == "Invalid entry '" + id + "' in [Test]"); } } @@ -235,12 +258,14 @@ class AmendmentTable_test final : public beast::unit_test::suite try { test::jtx::Env env{*this, makeConfig()}; - if (makeTable(env, weeks(2), test, emptySection, emptySection)) + if (makeTable(env, weeks(2), yesVotes, test, emptySection)) fail("Accepted extra arguments"); } - catch (...) + catch (std::exception const& e) { - pass(); + BEAST_EXPECT( + e.what() == + "Invalid entry '" + id + " Test Name' in [Test]"); } } @@ -254,12 +279,13 @@ class AmendmentTable_test final : public beast::unit_test::suite try { test::jtx::Env env{*this, makeConfig()}; - if (makeTable(env, weeks(2), test, emptySection, emptySection)) + if (makeTable(env, weeks(2), yesVotes, test, emptySection)) fail("Accepted short amendment ID"); } - catch (...) + catch (std::exception const& e) { - pass(); + BEAST_EXPECT( + e.what() == "Invalid entry '" + sid + " Name' in [Test]"); } } @@ -273,12 +299,13 @@ class AmendmentTable_test final : public beast::unit_test::suite try { test::jtx::Env env{*this, makeConfig()}; - if (makeTable(env, weeks(2), test, emptySection, emptySection)) + if (makeTable(env, weeks(2), yesVotes, test, emptySection)) fail("Accepted long amendment ID"); } - catch (...) + catch (std::exception const& e) { - pass(); + BEAST_EXPECT( + e.what() == "Invalid entry '" + sid + " Name' in [Test]"); } } @@ -293,12 +320,13 @@ class AmendmentTable_test final : public beast::unit_test::suite try { test::jtx::Env env{*this, makeConfig()}; - if (makeTable(env, weeks(2), test, emptySection, emptySection)) + if (makeTable(env, weeks(2), yesVotes, test, emptySection)) fail("Accepted non-hex amendment ID"); } - catch (...) + catch (std::exception const& e) { - pass(); + BEAST_EXPECT( + e.what() == "Invalid entry '" + sid + " Name' in [Test]"); } } } @@ -311,27 +339,27 @@ class AmendmentTable_test final : public beast::unit_test::suite test::jtx::Env env{*this, makeConfig()}; std::unique_ptr table = makeTable(env, weeks(2)); - // Note which entries are enabled. + // Note which entries are enabled std::set allEnabled; - - // Subset of amendments to enable - allEnabled.insert(amendmentId(supported_[0])); - allEnabled.insert(amendmentId(enabled_[0])); - allEnabled.insert(amendmentId(vetoed_[0])); + for (auto const& a : enabled_) + allEnabled.insert(amendmentId(a)); for (uint256 const& a : allEnabled) - table->enable(a); + BEAST_EXPECT(table->enable(a)); // So far all enabled amendments are supported. BEAST_EXPECT(!table->hasUnsupportedEnabled()); // Verify all enables are enabled and nothing else. - for (std::string const& a : supported_) + for (std::string const& a : supportedYes_) { uint256 const supportedID = amendmentId(a); - BEAST_EXPECT( - table->isEnabled(supportedID) == - (allEnabled.find(supportedID) != allEnabled.end())); + bool const enabled = table->isEnabled(supportedID); + bool const found = allEnabled.find(supportedID) != allEnabled.end(); + BEAST_EXPECTS( + enabled == found, + a + (enabled ? " enabled " : " disabled ") + + (found ? " found" : " not found")); } // All supported and unVetoed amendments should be returned as desired. @@ -347,14 +375,14 @@ class AmendmentTable_test final : public beast::unit_test::suite // Unveto an amendment that is already not vetoed. Shouldn't // hurt anything, but the values returned by getDesired() // shouldn't change. - table->unVeto(amendmentId(supported_[1])); + BEAST_EXPECT(!table->unVeto(amendmentId(supportedYes_[1]))); BEAST_EXPECT(desired == table->getDesired()); } // UnVeto one of the vetoed amendments. It should now be desired. { uint256 const unvetoedID = amendmentId(vetoed_[0]); - table->unVeto(unvetoedID); + BEAST_EXPECT(table->unVeto(unvetoedID)); std::vector const desired = table->getDesired(); BEAST_EXPECT( @@ -363,7 +391,7 @@ class AmendmentTable_test final : public beast::unit_test::suite } // Veto all supported amendments. Now desired should be empty. - for (std::string const& a : supported_) + for (std::string const& a : supportedYes_) { table->veto(amendmentId(a)); } @@ -505,7 +533,7 @@ class AmendmentTable_test final : public beast::unit_test::suite test::jtx::Env env{*this}; auto table = - makeTable(env, weeks(2), emptySection, emptySection, emptySection); + makeTable(env, weeks(2), emptyYes, emptySection, emptySection); std::vector> votes; std::vector ourVotes; @@ -566,11 +594,7 @@ class AmendmentTable_test final : public beast::unit_test::suite test::jtx::Env env{*this}; auto table = makeTable( - env, - weeks(2), - emptySection, - emptySection, - makeSection(testAmendment)); + env, weeks(2), emptyYes, emptySection, makeSection(testAmendment)); auto const validators = makeValidators(10); @@ -629,7 +653,11 @@ class AmendmentTable_test final : public beast::unit_test::suite test::jtx::Env env{*this}; auto table = makeTable( - env, weeks(2), makeSection(supported_), emptySection, emptySection); + env, + weeks(2), + makeDefaultYes(supportedYes_), + emptySection, + emptySection); auto const validators = makeValidators(10); std::vector> votes; @@ -647,13 +675,13 @@ class AmendmentTable_test final : public beast::unit_test::suite ourVotes, enabled, majority); - BEAST_EXPECT(ourVotes.size() == supported_.size()); + BEAST_EXPECT(ourVotes.size() == supportedYes_.size()); BEAST_EXPECT(enabled.empty()); - for (auto const& i : supported_) + for (auto const& i : supportedYes_) BEAST_EXPECT(majority.find(amendmentId(i)) == majority.end()); // Now, everyone votes for this feature - for (auto const& i : supported_) + for (auto const& i : supportedYes_) votes.emplace_back(amendmentId(i), validators.size()); // Week 2: We should recognize a majority @@ -666,10 +694,10 @@ class AmendmentTable_test final : public beast::unit_test::suite ourVotes, enabled, majority); - BEAST_EXPECT(ourVotes.size() == supported_.size()); + BEAST_EXPECT(ourVotes.size() == supportedYes_.size()); BEAST_EXPECT(enabled.empty()); - for (auto const& i : supported_) + for (auto const& i : supportedYes_) BEAST_EXPECT(majority[amendmentId(i)] == weekTime(weeks{2})); // Week 5: We should enable the amendment @@ -682,7 +710,7 @@ class AmendmentTable_test final : public beast::unit_test::suite ourVotes, enabled, majority); - BEAST_EXPECT(enabled.size() == supported_.size()); + BEAST_EXPECT(enabled.size() == supportedYes_.size()); // Week 6: We should remove it from our votes and from having a majority doRound( @@ -694,9 +722,9 @@ class AmendmentTable_test final : public beast::unit_test::suite ourVotes, enabled, majority); - BEAST_EXPECT(enabled.size() == supported_.size()); + BEAST_EXPECT(enabled.size() == supportedYes_.size()); BEAST_EXPECT(ourVotes.empty()); - for (auto const& i : supported_) + for (auto const& i : supportedYes_) BEAST_EXPECT(majority.find(amendmentId(i)) == majority.end()); } @@ -711,7 +739,7 @@ class AmendmentTable_test final : public beast::unit_test::suite auto table = makeTable( env, weeks(2), - makeSection(testAmendment), + makeDefaultYes(testAmendment), emptySection, emptySection); @@ -782,7 +810,7 @@ class AmendmentTable_test final : public beast::unit_test::suite auto table = makeTable( env, weeks(8), - makeSection(testAmendment), + makeDefaultYes(testAmendment), emptySection, emptySection); diff --git a/src/test/app/TxQ_test.cpp b/src/test/app/TxQ_test.cpp index ccc35d784d8..3fdf00a50f0 100644 --- a/src/test/app/TxQ_test.cpp +++ b/src/test/app/TxQ_test.cpp @@ -136,11 +136,10 @@ class TxQ1_test : public beast::unit_test::suite for (auto i = env.current()->seq(); i <= 257; ++i) env.close(); // The ledger after the flag ledger creates all the - // fee (1) and amendment (supportedAmendments().size()) + // fee (1) and amendment (numUpVotedAmendments()) // pseudotransactions. The queue treats the fees on these // transactions as though they are ordinary transactions. - auto const flagPerLedger = - 1 + ripple::detail::supportedAmendments().size(); + auto const flagPerLedger = 1 + ripple::detail::numUpVotedAmendments(); auto const flagMaxQueue = ledgersInQueue * flagPerLedger; checkMetrics(env, 0, flagMaxQueue, 0, flagPerLedger, 256); @@ -4157,8 +4156,7 @@ class TxQ1_test : public beast::unit_test::suite if (!getMajorityAmendments(*env.closed()).empty()) break; } - auto expectedPerLedger = - ripple::detail::supportedAmendments().size() + 1; + auto expectedPerLedger = ripple::detail::numUpVotedAmendments() + 1; checkMetrics(env, 0, 5 * expectedPerLedger, 0, expectedPerLedger, 256); // Now wait 2 weeks modulo 256 ledgers for the amendments to be @@ -4213,79 +4211,49 @@ class TxQ1_test : public beast::unit_test::suite // These particular amendments don't impact any of the queued // transactions, so we won't see any change in the transaction // outcomes. But code coverage is affected. - env.close(closeDuration); - expectedInQueue -= expectedPerLedger + 2; - ++expectedPerLedger; - checkMetrics( - env, - expectedInQueue, - 5 * expectedPerLedger, - expectedPerLedger + 1, - expectedPerLedger, - 256); + do { - auto const expectedPerAccount = expectedInQueue / 6; - auto const expectedRemainder = expectedInQueue % 6; - BEAST_EXPECT(env.seq(alice) == seqAlice - expectedPerAccount); - BEAST_EXPECT( - env.seq(bob) == - seqBob - expectedPerAccount - (expectedRemainder > 4 ? 1 : 0)); - BEAST_EXPECT( - env.seq(carol) == - seqCarol - expectedPerAccount - - (expectedRemainder > 3 ? 1 : 0)); - BEAST_EXPECT( - env.seq(daria) == - seqDaria - expectedPerAccount - - (expectedRemainder > 2 ? 1 : 0)); - BEAST_EXPECT( - env.seq(ellie) == - seqEllie - expectedPerAccount - - (expectedRemainder > 1 ? 1 : 0)); - BEAST_EXPECT( - env.seq(fiona) == - seqFiona - expectedPerAccount - - (expectedRemainder > 0 ? 1 : 0)); - } - - env.close(closeDuration); - auto expectedInLedger = expectedInQueue; - expectedInQueue = - (expectedInQueue > expectedPerLedger + 2 - ? expectedInQueue - (expectedPerLedger + 2) - : 0); - ++expectedPerLedger; - checkMetrics( - env, - 0, - 5 * expectedPerLedger, - expectedInLedger, - expectedPerLedger, - 256); - { - auto const expectedPerAccount = expectedInQueue / 6; - auto const expectedRemainder = expectedInQueue % 6; - BEAST_EXPECT(env.seq(alice) == seqAlice - expectedPerAccount); - BEAST_EXPECT( - env.seq(bob) == - seqBob - expectedPerAccount - (expectedRemainder > 4 ? 1 : 0)); - BEAST_EXPECT( - env.seq(carol) == - seqCarol - expectedPerAccount - - (expectedRemainder > 3 ? 1 : 0)); - BEAST_EXPECT( - env.seq(daria) == - seqDaria - expectedPerAccount - - (expectedRemainder > 2 ? 1 : 0)); - BEAST_EXPECT( - env.seq(ellie) == - seqEllie - expectedPerAccount - - (expectedRemainder > 1 ? 1 : 0)); - BEAST_EXPECT( - env.seq(fiona) == - seqFiona - expectedPerAccount - - (expectedRemainder > 0 ? 1 : 0)); - } + env.close(closeDuration); + auto expectedInLedger = expectedInQueue; + expectedInQueue = + (expectedInQueue > expectedPerLedger + 2 + ? expectedInQueue - (expectedPerLedger + 2) + : 0); + expectedInLedger -= expectedInQueue; + ++expectedPerLedger; + checkMetrics( + env, + expectedInQueue, + 5 * expectedPerLedger, + expectedInLedger, + expectedPerLedger, + 256); + { + auto const expectedPerAccount = expectedInQueue / 6; + auto const expectedRemainder = expectedInQueue % 6; + BEAST_EXPECT(env.seq(alice) == seqAlice - expectedPerAccount); + BEAST_EXPECT( + env.seq(bob) == + seqBob - expectedPerAccount - + (expectedRemainder > 4 ? 1 : 0)); + BEAST_EXPECT( + env.seq(carol) == + seqCarol - expectedPerAccount - + (expectedRemainder > 3 ? 1 : 0)); + BEAST_EXPECT( + env.seq(daria) == + seqDaria - expectedPerAccount - + (expectedRemainder > 2 ? 1 : 0)); + BEAST_EXPECT( + env.seq(ellie) == + seqEllie - expectedPerAccount - + (expectedRemainder > 1 ? 1 : 0)); + BEAST_EXPECT( + env.seq(fiona) == + seqFiona - expectedPerAccount - + (expectedRemainder > 0 ? 1 : 0)); + } + } while (expectedInQueue > 0); } void diff --git a/src/test/jtx/Env.h b/src/test/jtx/Env.h index 23d6dca4e5c..8841d2d2f58 100644 --- a/src/test/jtx/Env.h +++ b/src/test/jtx/Env.h @@ -73,8 +73,9 @@ supported_amendments() auto const& sa = ripple::detail::supportedAmendments(); std::vector feats; feats.reserve(sa.size()); - for (auto const& s : sa) + for (auto const& [s, vote] : sa) { + (void)vote; if (auto const f = getRegisteredFeature(s)) feats.push_back(*f); else diff --git a/src/test/jtx/Env_test.cpp b/src/test/jtx/Env_test.cpp index d8ee49cdb4d..b1a1a81253c 100644 --- a/src/test/jtx/Env_test.cpp +++ b/src/test/jtx/Env_test.cpp @@ -812,6 +812,7 @@ class Env_test : public beast::unit_test::suite auto const missingSomeFeatures = supported_amendments() - featureMultiSignReserve - featureFlow; + BEAST_EXPECT(missingSomeFeatures.count() == (supported.count() - 2)); { // a Env supported_features_except is missing *only* those features Env env{*this, missingSomeFeatures}; diff --git a/src/test/rpc/Feature_test.cpp b/src/test/rpc/Feature_test.cpp index 4a653956239..1ba4d865d44 100644 --- a/src/test/rpc/Feature_test.cpp +++ b/src/test/rpc/Feature_test.cpp @@ -26,6 +26,77 @@ namespace ripple { class Feature_test : public beast::unit_test::suite { + void + testInternals() + { + testcase("internals"); + + std::map const& supported = + ripple::detail::supportedAmendments(); + BEAST_EXPECT( + supported.size() == + ripple::detail::numDownVotedAmendments() + + ripple::detail::numUpVotedAmendments()); + std::size_t up = 0, down = 0; + for (std::pair const& amendment : + supported) + { + if (amendment.second == DefaultVote::no) + ++down; + else + { + if (BEAST_EXPECT(amendment.second == DefaultVote::yes)) + ++up; + } + } + BEAST_EXPECT(down == ripple::detail::numDownVotedAmendments()); + BEAST_EXPECT(up == ripple::detail::numUpVotedAmendments()); + } + + void + testFeatureLookups() + { + testcase("featureToName"); + + // Test all the supported features. In a perfect world, this would test + // FeatureCollections::featureNames, but that's private. Leave it that + // way. + auto const supported = ripple::detail::supportedAmendments(); + + for (auto const& [feature, vote] : supported) + { + (void)vote; + auto const registered = getRegisteredFeature(feature); + if (BEAST_EXPECT(registered)) + { + BEAST_EXPECT(featureToName(*registered) == feature); + BEAST_EXPECT( + bitsetIndexToFeature(featureToBitsetIndex(*registered)) == + *registered); + } + } + + // Test an arbitrary unknown feature + uint256 zero{0}; + BEAST_EXPECT(featureToName(zero) == to_string(zero)); + BEAST_EXPECT( + featureToName(zero) == + "0000000000000000000000000000000000000000000000000000000000000000"); + + // Test looking up an unknown feature + BEAST_EXPECT(!getRegisteredFeature("unknown")); + + // Test a random sampling of the variables. If any of these get retired + // or removed, swap out for any other feature. + BEAST_EXPECT(featureToName(featureOwnerPaysFee) == "OwnerPaysFee"); + BEAST_EXPECT(featureToName(featureFlow) == "Flow"); + BEAST_EXPECT(featureToName(featureNegativeUNL) == "NegativeUNL"); + BEAST_EXPECT(featureToName(fix1578) == "fix1578"); + BEAST_EXPECT( + featureToName(fixTakerDryOfferRemoval) == + "fixTakerDryOfferRemoval"); + } + void testNoParams() { @@ -34,6 +105,9 @@ class Feature_test : public beast::unit_test::suite using namespace test::jtx; Env env{*this}; + std::map const& votes = + ripple::detail::supportedAmendments(); + auto jrr = env.rpc("feature")[jss::result]; if (!BEAST_EXPECT(jrr.isMember(jss::features))) return; @@ -41,13 +115,15 @@ class Feature_test : public beast::unit_test::suite { if (!BEAST_EXPECT(feature.isMember(jss::name))) return; - // default config - so all should be disabled, not vetoed, and - // supported + // default config - so all should be disabled, and + // supported. Some may be vetoed. + bool expectVeto = + !(votes.at(feature[jss::name].asString()) == DefaultVote::yes); BEAST_EXPECTS( !feature[jss::enabled].asBool(), feature[jss::name].asString() + " enabled"); BEAST_EXPECTS( - !feature[jss::vetoed].asBool(), + feature[jss::vetoed].asBool() == expectVeto, feature[jss::name].asString() + " vetoed"); BEAST_EXPECTS( feature[jss::supported].asBool(), @@ -67,6 +143,9 @@ class Feature_test : public beast::unit_test::suite BEAST_EXPECTS(jrr[jss::status] == jss::success, "status"); jrr.removeMember(jss::status); BEAST_EXPECT(jrr.size() == 1); + BEAST_EXPECT( + jrr.isMember("586480873651E106F1D6339B0C4A8945BA705A777F3F4524626FF" + "1FC07EFE41D")); auto feature = *(jrr.begin()); BEAST_EXPECTS(feature[jss::name] == "MultiSignReserve", "name"); @@ -121,6 +200,9 @@ class Feature_test : public beast::unit_test::suite Env env{ *this, FeatureBitset(featureDepositAuth, featureDepositPreauth)}; + std::map const& votes = + ripple::detail::supportedAmendments(); + auto jrr = env.rpc("feature")[jss::result]; if (!BEAST_EXPECT(jrr.isMember(jss::features))) return; @@ -135,11 +217,13 @@ class Feature_test : public beast::unit_test::suite bool expectEnabled = env.app().getAmendmentTable().isEnabled(id); bool expectSupported = env.app().getAmendmentTable().isSupported(id); + bool expectVeto = + !(votes.at((*it)[jss::name].asString()) == DefaultVote::yes); BEAST_EXPECTS( (*it)[jss::enabled].asBool() == expectEnabled, (*it)[jss::name].asString() + " enabled"); BEAST_EXPECTS( - !(*it)[jss::vetoed].asBool(), + (*it)[jss::vetoed].asBool() == expectVeto, (*it)[jss::name].asString() + " vetoed"); BEAST_EXPECTS( (*it)[jss::supported].asBool() == expectSupported, @@ -198,6 +282,8 @@ class Feature_test : public beast::unit_test::suite // There should be at least 5 amendments. Don't do exact comparison // to avoid maintenance as more amendments are added in the future. BEAST_EXPECT(majorities.size() >= 5); + std::map const& votes = + ripple::detail::supportedAmendments(); jrr = env.rpc("feature")[jss::result]; if (!BEAST_EXPECT(jrr.isMember(jss::features))) @@ -206,9 +292,15 @@ class Feature_test : public beast::unit_test::suite { if (!BEAST_EXPECT(feature.isMember(jss::name))) return; + bool expectVeto = + !(votes.at(feature[jss::name].asString()) == DefaultVote::yes); BEAST_EXPECTS( - feature.isMember(jss::majority), + expectVeto ^ feature.isMember(jss::majority), feature[jss::name].asString() + " majority"); + BEAST_EXPECTS( + feature.isMember(jss::vetoed) && + feature[jss::vetoed].asBool() == expectVeto, + feature[jss::name].asString() + " vetoed"); BEAST_EXPECTS( feature.isMember(jss::count), feature[jss::name].asString() + " count"); @@ -218,10 +310,12 @@ class Feature_test : public beast::unit_test::suite BEAST_EXPECTS( feature.isMember(jss::validations), feature[jss::name].asString() + " validations"); - BEAST_EXPECT(feature[jss::count] == 1); + BEAST_EXPECT(feature[jss::count] == (expectVeto ? 0 : 1)); BEAST_EXPECT(feature[jss::threshold] == 1); BEAST_EXPECT(feature[jss::validations] == 1); - BEAST_EXPECT(feature[jss::majority] == 2540); + BEAST_EXPECTS( + expectVeto || feature[jss::majority] == 2540, + "Majority: " + feature[jss::majority].asString()); } } @@ -273,6 +367,8 @@ class Feature_test : public beast::unit_test::suite void run() override { + testInternals(); + testFeatureLookups(); testNoParams(); testSingleFeature(); testInvalidFeature();