From 7aad6e51278a90fdb743b45d6b5df94a03d90d31 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Thu, 23 Mar 2023 22:28:53 -0700 Subject: [PATCH] feat: mark 4 amendments as obsolete: (#4291) Add the ability to mark amendments as obsolete. There are some known amendments that should not be voted for because they are broken (or similar reasons). This commit marks four amendments as obsolete: 1. `CryptoConditionsSuite` 2. `NonFungibleTokensV1` 3. `fixNFTokenDirV1` 4. `fixNFTokenNegOffer` When an amendment is `Obsolete`, voting for the amendment is prevented. A determined operator can still vote for the amendment by changing the source, and doing so does not break any protocol rules. The "feature" command now does not modify the vote for obsolete amendments. Before this change, there were two options for an amendment's `DefaultVote` behavior: yes and no. After this change, there are three options for an amendment's `VoteBehavior`: DefaultYes, DefaultNo, and Obsolete. To be clear, if an obsolete amendment were to (somehow) be activated by consensus, the server still has the code to process transactions according to that amendment, and would not be amendment blocked. It would function the same as if it had been voting "no" on the amendment. Resolves #4014. Incorporates review feedback from @scottschurr. --- src/ripple/app/misc/AmendmentTable.h | 4 +- src/ripple/app/misc/impl/AmendmentTable.cpp | 41 ++++- src/ripple/app/rdb/Wallet.h | 2 +- src/ripple/protocol/Feature.h | 14 +- src/ripple/protocol/impl/Feature.cpp | 133 +++++++------- src/test/app/AmendmentTable_test.cpp | 172 +++++++++++++----- src/test/rpc/Feature_test.cpp | 184 ++++++++++++++++---- 7 files changed, 386 insertions(+), 164 deletions(-) diff --git a/src/ripple/app/misc/AmendmentTable.h b/src/ripple/app/misc/AmendmentTable.h index 14558b7da5..0a5f6a011a 100644 --- a/src/ripple/app/misc/AmendmentTable.h +++ b/src/ripple/app/misc/AmendmentTable.h @@ -40,14 +40,14 @@ public: struct FeatureInfo { FeatureInfo() = delete; - FeatureInfo(std::string const& n, uint256 const& f, DefaultVote v) + FeatureInfo(std::string const& n, uint256 const& f, VoteBehavior v) : name(n), feature(f), vote(v) { } std::string const name; uint256 const feature; - DefaultVote const vote; + VoteBehavior const vote; }; virtual ~AmendmentTable() = default; diff --git a/src/ripple/app/misc/impl/AmendmentTable.cpp b/src/ripple/app/misc/impl/AmendmentTable.cpp index 93113af800..6f9ea86fa6 100644 --- a/src/ripple/app/misc/impl/AmendmentTable.cpp +++ b/src/ripple/app/misc/impl/AmendmentTable.cpp @@ -333,19 +333,31 @@ AmendmentTableImpl::AmendmentTableImpl( }(); // Parse supported amendments - for (auto const& [name, amendment, defaultVote] : supported) + for (auto const& [name, amendment, votebehavior] : supported) { AmendmentState& s = add(amendment, lock); s.name = name; s.supported = true; - s.vote = defaultVote == DefaultVote::yes ? AmendmentVote::up - : AmendmentVote::down; + switch (votebehavior) + { + case VoteBehavior::DefaultYes: + s.vote = AmendmentVote::up; + break; + + case VoteBehavior::DefaultNo: + s.vote = AmendmentVote::down; + break; + + case VoteBehavior::Obsolete: + s.vote = AmendmentVote::obsolete; + break; + } 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."; + << " voted by default if not enabled on the ledger."; } hash_set detect_conflict; @@ -420,7 +432,9 @@ AmendmentTableImpl::AmendmentTableImpl( << amend_hash << "} is downvoted."; if (!amendment_name->empty()) s->name = *amendment_name; - s->vote = *vote; + // An obsolete amendment's vote can never be changed + if (s->vote != AmendmentVote::obsolete) + s->vote = *vote; } } else // up-vote @@ -431,7 +445,9 @@ AmendmentTableImpl::AmendmentTableImpl( << amend_hash << "} is upvoted."; if (!amendment_name->empty()) s.name = *amendment_name; - s.vote = *vote; + // An obsolete amendment's vote can never be changed + if (s.vote != AmendmentVote::obsolete) + s.vote = *vote; } }); } @@ -489,6 +505,7 @@ AmendmentTableImpl::persistVote( std::string const& name, AmendmentVote vote) const { + assert(vote != AmendmentVote::obsolete); auto db = db_.checkoutDb(); voteAmendment(*db, amendment, name, vote); } @@ -499,7 +516,7 @@ AmendmentTableImpl::veto(uint256 const& amendment) std::lock_guard lock(mutex_); AmendmentState& s = add(amendment, lock); - if (s.vote == AmendmentVote::down) + if (s.vote != AmendmentVote::up) return false; s.vote = AmendmentVote::down; persistVote(amendment, s.name, s.vote); @@ -512,7 +529,7 @@ AmendmentTableImpl::unVeto(uint256 const& amendment) std::lock_guard lock(mutex_); AmendmentState* const s = get(amendment, lock); - if (!s || s->vote == AmendmentVote::up) + if (!s || s->vote != AmendmentVote::down) return false; s->vote = AmendmentVote::up; persistVote(amendment, s->name, s->vote); @@ -734,7 +751,13 @@ AmendmentTableImpl::injectJson( v[jss::name] = fs.name; v[jss::supported] = fs.supported; - v[jss::vetoed] = fs.vote == AmendmentVote::down; + if (!fs.enabled) + { + if (fs.vote == AmendmentVote::obsolete) + v[jss::vetoed] = "Obsolete"; + else + v[jss::vetoed] = fs.vote == AmendmentVote::down; + } v[jss::enabled] = fs.enabled; if (!fs.enabled && lastVote_) diff --git a/src/ripple/app/rdb/Wallet.h b/src/ripple/app/rdb/Wallet.h index 2769e459ac..e9846714ec 100644 --- a/src/ripple/app/rdb/Wallet.h +++ b/src/ripple/app/rdb/Wallet.h @@ -144,7 +144,7 @@ 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 }; +enum class AmendmentVote : int { obsolete = -1, up = 0, down = 1 }; /** * @brief readAmendments Reads all amendments from the FeatureVotes table. diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index 62dc327d98..e4b0e3d4ac 100644 --- a/src/ripple/protocol/Feature.h +++ b/src/ripple/protocol/Feature.h @@ -36,17 +36,17 @@ * 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 + * the feature's name, `Supported::no`, and `VoteBehavior::DefaultNo`. 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`. + * parameter to `VoteBehavior::DefaultYes`. * 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. + * a `VoteBehavior::DefaultNo` 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: @@ -55,7 +55,7 @@ * 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`. + * and `VoteBehavior::DefaultNo`. * 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 @@ -66,7 +66,7 @@ namespace ripple { -enum class DefaultVote : bool { no = false, yes }; +enum class VoteBehavior : int { Obsolete = -1, DefaultNo = 0, DefaultYes }; namespace detail { @@ -79,7 +79,7 @@ static constexpr std::size_t numFeatures = 58; /** 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& +std::map const& supportedAmendments(); /** Amendments that this server won't vote for by default. diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index d15c3fc60f..6b8e7719d0 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -67,9 +67,10 @@ enum class Supported : bool { no = false, yes }; // updated. // // Generally, amendments which introduce new features should be set as -// "DefaultVote::no" whereas in rare cases, amendments that fix critical -// bugs should be set as "DefaultVote::yes", if off-chain consensus is -// reached amongst reviewers, validator operators, and other participants. +// "VoteBehavior::DefaultNo" whereas in rare cases, amendments that fix +// critical bugs should be set as "VoteBehavior::DefaultYes", if off-chain +// consensus is reached amongst reviewers, validator operators, and other +// participants. class FeatureCollections { @@ -115,7 +116,7 @@ class FeatureCollections // name, index, and uint256 feature identifier boost::multi_index::multi_index_container features; - std::map supported; + std::map supported; std::size_t upVotes = 0; std::size_t downVotes = 0; mutable std::atomic readOnly = false; @@ -163,7 +164,7 @@ public: registerFeature( std::string const& name, Supported support, - DefaultVote vote); + VoteBehavior vote); /** Tell FeatureCollections when registration is complete. */ bool @@ -181,7 +182,7 @@ public: /** Amendments that this server supports. Whether they are enabled depends on the Rules defined in the validated ledger */ - std::map const& + std::map const& supportedAmendments() const { return supported; @@ -230,11 +231,11 @@ uint256 FeatureCollections::registerFeature( std::string const& name, Supported support, - DefaultVote vote) + VoteBehavior vote) { check(!readOnly, "Attempting to register a feature after startup."); check( - support == Supported::yes || vote == DefaultVote::no, + support == Supported::yes || vote == VoteBehavior::DefaultNo, "Invalid feature parameters. Must be supported to be up-voted."); Feature const* i = getByName(name); if (!i) @@ -254,7 +255,7 @@ FeatureCollections::registerFeature( { supported.emplace(name, vote); - if (vote == DefaultVote::yes) + if (vote == VoteBehavior::DefaultYes) ++upVotes; else ++downVotes; @@ -315,7 +316,7 @@ static FeatureCollections featureCollections; /** Amendments that this server supports. Whether they are enabled depends on the Rules defined in the validated ledger */ -std::map const& +std::map const& detail::supportedAmendments() { return featureCollections.supportedAmendments(); @@ -344,7 +345,7 @@ getRegisteredFeature(std::string const& name) } uint256 -registerFeature(std::string const& name, Supported support, DefaultVote vote) +registerFeature(std::string const& name, Supported support, VoteBehavior vote) { return featureCollections.registerFeature(name, support, vote); } @@ -354,7 +355,7 @@ registerFeature(std::string const& name, Supported support, DefaultVote vote) uint256 retireFeature(std::string const& name) { - return registerFeature(name, Supported::yes, DefaultVote::no); + return registerFeature(name, Supported::yes, VoteBehavior::Obsolete); } /** Tell FeatureCollections when registration is complete. */ @@ -390,9 +391,9 @@ 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) +#define REGISTER_FEATURE(fName, supported, votebehavior) \ + uint256 const feature##fName = \ + registerFeature(#fName, supported, votebehavior) #pragma push_macro("REGISTER_FIX") #undef REGISTER_FIX @@ -402,59 +403,71 @@ 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) +#define REGISTER_FIX(fName, supported, votebehavior) \ + uint256 const fName = registerFeature(#fName, supported, votebehavior) // clang-format off // 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); +REGISTER_FEATURE(OwnerPaysFee, Supported::no, VoteBehavior::DefaultNo); +REGISTER_FEATURE(Flow, Supported::yes, VoteBehavior::DefaultYes); +REGISTER_FEATURE(FlowCross, Supported::yes, VoteBehavior::DefaultYes); +REGISTER_FIX (fix1513, Supported::yes, VoteBehavior::DefaultYes); +REGISTER_FEATURE(DepositAuth, Supported::yes, VoteBehavior::DefaultYes); +REGISTER_FEATURE(Checks, Supported::yes, VoteBehavior::DefaultYes); +REGISTER_FIX (fix1571, Supported::yes, VoteBehavior::DefaultYes); +REGISTER_FIX (fix1543, Supported::yes, VoteBehavior::DefaultYes); +REGISTER_FIX (fix1623, Supported::yes, VoteBehavior::DefaultYes); +REGISTER_FEATURE(DepositPreauth, Supported::yes, VoteBehavior::DefaultYes); // 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); +REGISTER_FIX (fix1515, Supported::yes, VoteBehavior::DefaultYes); +REGISTER_FIX (fix1578, Supported::yes, VoteBehavior::DefaultYes); +REGISTER_FEATURE(MultiSignReserve, Supported::yes, VoteBehavior::DefaultYes); +REGISTER_FIX (fixTakerDryOfferRemoval, Supported::yes, VoteBehavior::DefaultYes); +REGISTER_FIX (fixMasterKeyAsRegularKey, Supported::yes, VoteBehavior::DefaultYes); +REGISTER_FIX (fixCheckThreading, Supported::yes, VoteBehavior::DefaultYes); +REGISTER_FIX (fixPayChanRecipientOwnerDir, Supported::yes, VoteBehavior::DefaultYes); +REGISTER_FEATURE(DeletableAccounts, Supported::yes, VoteBehavior::DefaultYes); // fixQualityUpperBound should be activated before FlowCross -REGISTER_FIX (fixQualityUpperBound, Supported::yes, DefaultVote::yes); -REGISTER_FEATURE(RequireFullyCanonicalSig, Supported::yes, DefaultVote::yes); +REGISTER_FIX (fixQualityUpperBound, Supported::yes, VoteBehavior::DefaultYes); +REGISTER_FEATURE(RequireFullyCanonicalSig, Supported::yes, VoteBehavior::DefaultYes); // 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::yes); -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); -REGISTER_FEATURE(NonFungibleTokensV1, Supported::yes, DefaultVote::no); -REGISTER_FEATURE(ExpandedSignerList, Supported::yes, DefaultVote::no); -REGISTER_FIX (fixNFTokenDirV1, Supported::yes, DefaultVote::no); -REGISTER_FIX (fixNFTokenNegOffer, Supported::yes, DefaultVote::no); -REGISTER_FEATURE(NonFungibleTokensV1_1, Supported::yes, DefaultVote::no); -REGISTER_FIX (fixTrustLinesToSelf, Supported::yes, DefaultVote::no); -REGISTER_FIX (fixRemoveNFTokenAutoTrustLine, Supported::yes, DefaultVote::yes); -REGISTER_FEATURE(ImmediateOfferKilled, Supported::yes, DefaultVote::no); -REGISTER_FEATURE(DisallowIncoming, Supported::yes, DefaultVote::no); -REGISTER_FEATURE(XRPFees, Supported::yes, DefaultVote::no); -REGISTER_FIX (fixUniversalNumber, Supported::yes, DefaultVote::no); -REGISTER_FIX (fixNonFungibleTokensV1_2, Supported::yes, DefaultVote::no); -REGISTER_FIX (fixNFTokenRemint, Supported::yes, DefaultVote::no); +REGISTER_FIX (fix1781, Supported::yes, VoteBehavior::DefaultYes); +REGISTER_FEATURE(HardenedValidations, Supported::yes, VoteBehavior::DefaultYes); +REGISTER_FIX (fixAmendmentMajorityCalc, Supported::yes, VoteBehavior::DefaultYes); +REGISTER_FEATURE(NegativeUNL, Supported::yes, VoteBehavior::DefaultYes); +REGISTER_FEATURE(TicketBatch, Supported::yes, VoteBehavior::DefaultYes); +REGISTER_FEATURE(FlowSortStrands, Supported::yes, VoteBehavior::DefaultYes); +REGISTER_FIX (fixSTAmountCanonicalize, Supported::yes, VoteBehavior::DefaultYes); +REGISTER_FIX (fixRmSmallIncreasedQOffers, Supported::yes, VoteBehavior::DefaultYes); +REGISTER_FEATURE(CheckCashMakesTrustLine, Supported::yes, VoteBehavior::DefaultNo); +REGISTER_FEATURE(ExpandedSignerList, Supported::yes, VoteBehavior::DefaultNo); +REGISTER_FEATURE(NonFungibleTokensV1_1, Supported::yes, VoteBehavior::DefaultNo); +REGISTER_FIX (fixTrustLinesToSelf, Supported::yes, VoteBehavior::DefaultNo); +REGISTER_FIX (fixRemoveNFTokenAutoTrustLine, Supported::yes, VoteBehavior::DefaultYes); +REGISTER_FEATURE(ImmediateOfferKilled, Supported::yes, VoteBehavior::DefaultNo); +REGISTER_FEATURE(DisallowIncoming, Supported::yes, VoteBehavior::DefaultNo); +REGISTER_FEATURE(XRPFees, Supported::yes, VoteBehavior::DefaultNo); +REGISTER_FIX (fixUniversalNumber, Supported::yes, VoteBehavior::DefaultNo); +REGISTER_FIX (fixNonFungibleTokensV1_2, Supported::yes, VoteBehavior::DefaultNo); +REGISTER_FIX (fixNFTokenRemint, Supported::yes, VoteBehavior::DefaultNo); + +// The following amendments are obsolete, but must remain supported +// because they could potentially get enabled. +// +// Obsolete features are (usually) not in the ledger, and may have code +// controlled by the feature. They need to be supported because at some +// time in the past, the feature was supported and votable, but never +// passed. So the feature needs to be supported in case it is ever +// enabled (added to the ledger). +// +// If a feature remains obsolete for long enough that no clients are able +// to vote for it, the feature can be removed (entirely?) from the code. +REGISTER_FEATURE(CryptoConditionsSuite, Supported::yes, VoteBehavior::Obsolete); +REGISTER_FEATURE(NonFungibleTokensV1, Supported::yes, VoteBehavior::Obsolete); +REGISTER_FIX (fixNFTokenDirV1, Supported::yes, VoteBehavior::Obsolete); +REGISTER_FIX (fixNFTokenNegOffer, Supported::yes, VoteBehavior::Obsolete); // 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 99922e863a..4284190a18 100644 --- a/src/test/app/AmendmentTable_test.cpp +++ b/src/test/app/AmendmentTable_test.cpp @@ -87,45 +87,105 @@ private: } static std::vector - makeDefaultYes(std::vector const& amendments) + makeFeatureInfo( + std::vector const& amendments, + VoteBehavior voteBehavior) { std::vector result; result.reserve(amendments.size()); for (auto const& a : amendments) { - result.emplace_back(a, amendmentId(a), DefaultVote::yes); + result.emplace_back(a, amendmentId(a), voteBehavior); } return result; } + static std::vector + makeDefaultYes(std::vector const& amendments) + { + return makeFeatureInfo(amendments, VoteBehavior::DefaultYes); + } + static std::vector makeDefaultYes(uint256 const amendment) { std::vector result{ - {to_string(amendment), amendment, DefaultVote::yes}}; + {to_string(amendment), amendment, VoteBehavior::DefaultYes}}; return result; } + static std::vector + makeDefaultNo(std::vector const& amendments) + { + return makeFeatureInfo(amendments, VoteBehavior::DefaultNo); + } + + static std::vector + makeObsolete(std::vector const& amendments) + { + return makeFeatureInfo(amendments, VoteBehavior::Obsolete); + } + + template + static size_t + totalsize(std::vector const& src, Args const&... args) + { + if constexpr (sizeof...(args) > 0) + return src.size() + totalsize(args...); + return src.size(); + } + + template + static void + combine_arg( + std::vector& dest, + std::vector const& src, + Args const&... args) + { + assert(dest.capacity() >= dest.size() + src.size()); + std::copy(src.begin(), src.end(), std::back_inserter(dest)); + if constexpr (sizeof...(args) > 0) + combine_arg(dest, args...); + } + + template + static std::vector + combine( + // Pass "left" by value. The values will need to be copied one way or + // another, so just reuse it. + std::vector left, + std::vector const& right, + Args const&... args) + { + left.reserve(totalsize(left, right, args...)); + + combine_arg(left, right, args...); + + return left; + } + // 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 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 + yes_{"g", "i", "k", "m", "o", "q", "r", "s", "t", "u"}; std::vector const enabled_{"b", "d", "f", "h", "j", "l", "n", "p"}; std::vector const vetoed_{"a", "c", "e"}; + std::vector const obsolete_{"0", "1", "2"}; + std::vector const allSupported_{ + combine(yes_, enabled_, vetoed_, obsolete_)}; std::vector const unsupported_{"v", "w", "x"}; std::vector const unsupportedMajority_{"y", "z"}; - Section const emptySection; - std::vector const emptyYes; + Section const emptySection_; + std::vector const emptyYes_; - test::SuiteJournal journal; + test::SuiteJournal journal_; public: - AmendmentTable_test() : journal("AmendmentTable_test", *this) + AmendmentTable_test() : journal_("AmendmentTable_test", *this) { } @@ -138,7 +198,7 @@ public: Section const& vetoed) { return make_AmendmentTable( - app, majorityTime, supported, enabled, vetoed, journal); + app, majorityTime, supported, enabled, vetoed, journal_); } std::unique_ptr @@ -155,10 +215,20 @@ public: std::unique_ptr makeTable(test::jtx::Env& env, std::chrono::seconds majorityTime) { + static std::vector const supported = + combine( + makeDefaultYes(yes_), + // Use non-intuitive default votes for "enabled_" and "vetoed_" + // so that when the tests later explicitly enable or veto them, + // we can be certain that they are not simply going by their + // default vote setting. + makeDefaultNo(enabled_), + makeDefaultYes(vetoed_), + makeObsolete(obsolete_)); return makeTable( env.app(), majorityTime, - makeDefaultYes(supportedYes_), + supported, makeSection(enabled_), makeSection(vetoed_)); } @@ -170,21 +240,26 @@ public: test::jtx::Env env{*this, makeConfig()}; auto table = makeTable(env, weeks(1)); - for (auto const& a : supportedYes_) - { + for (auto const& a : allSupported_) + BEAST_EXPECT(table->isSupported(amendmentId(a))); + + for (auto const& a : yes_) BEAST_EXPECT(table->isSupported(amendmentId(a))); - } for (auto const& a : enabled_) - { BEAST_EXPECT(table->isSupported(amendmentId(a))); - } for (auto const& a : vetoed_) { BEAST_EXPECT(table->isSupported(amendmentId(a))); BEAST_EXPECT(!table->isEnabled(amendmentId(a))); } + + for (auto const& a : obsolete_) + { + BEAST_EXPECT(table->isSupported(amendmentId(a))); + BEAST_EXPECT(!table->isEnabled(amendmentId(a))); + } } void @@ -195,13 +270,14 @@ public: test::jtx::Env env{*this, makeConfig()}; auto table = makeTable(env, weeks(1)); - for (auto const& a : supportedYes_) + for (auto const& a : yes_) BEAST_EXPECT(table->find(a) == amendmentId(a)); for (auto const& a : enabled_) BEAST_EXPECT(table->find(a) == amendmentId(a)); - for (auto const& a : vetoed_) BEAST_EXPECT(table->find(a) == amendmentId(a)); + for (auto const& a : obsolete_) + BEAST_EXPECT(table->find(a) == amendmentId(a)); for (auto const& a : unsupported_) BEAST_EXPECT(!table->find(a)); for (auto const& a : unsupportedMajority_) @@ -228,7 +304,7 @@ public: void testBadConfig() { - auto const yesVotes = makeDefaultYes(supportedYes_); + auto const yesVotes = makeDefaultYes(yes_); auto const section = makeSection(vetoed_); auto const id = to_string(amendmentId(enabled_[0])); @@ -241,7 +317,7 @@ public: try { test::jtx::Env env{*this, makeConfig()}; - if (makeTable(env, weeks(2), yesVotes, test, emptySection)) + if (makeTable(env, weeks(2), yesVotes, test, emptySection_)) fail("Accepted only amendment ID"); } catch (std::exception const& e) @@ -258,7 +334,7 @@ public: try { test::jtx::Env env{*this, makeConfig()}; - if (makeTable(env, weeks(2), yesVotes, test, emptySection)) + if (makeTable(env, weeks(2), yesVotes, test, emptySection_)) fail("Accepted extra arguments"); } catch (std::exception const& e) @@ -279,7 +355,7 @@ public: try { test::jtx::Env env{*this, makeConfig()}; - if (makeTable(env, weeks(2), yesVotes, test, emptySection)) + if (makeTable(env, weeks(2), yesVotes, test, emptySection_)) fail("Accepted short amendment ID"); } catch (std::exception const& e) @@ -299,7 +375,7 @@ public: try { test::jtx::Env env{*this, makeConfig()}; - if (makeTable(env, weeks(2), yesVotes, test, emptySection)) + if (makeTable(env, weeks(2), yesVotes, test, emptySection_)) fail("Accepted long amendment ID"); } catch (std::exception const& e) @@ -320,7 +396,7 @@ public: try { test::jtx::Env env{*this, makeConfig()}; - if (makeTable(env, weeks(2), yesVotes, test, emptySection)) + if (makeTable(env, weeks(2), yesVotes, test, emptySection_)) fail("Accepted non-hex amendment ID"); } catch (std::exception const& e) @@ -339,7 +415,7 @@ public: test::jtx::Env env{*this, makeConfig()}; std::unique_ptr table = makeTable(env, weeks(2)); - // Note which entries are enabled + // Note which entries are enabled (convert the amendment names to IDs) std::set allEnabled; for (auto const& a : enabled_) allEnabled.insert(amendmentId(a)); @@ -351,7 +427,7 @@ public: BEAST_EXPECT(!table->hasUnsupportedEnabled()); // Verify all enables are enabled and nothing else. - for (std::string const& a : supportedYes_) + for (std::string const& a : yes_) { uint256 const supportedID = amendmentId(a); bool const enabled = table->isEnabled(supportedID); @@ -375,7 +451,7 @@ public: // Unveto an amendment that is already not vetoed. Shouldn't // hurt anything, but the values returned by getDesired() // shouldn't change. - BEAST_EXPECT(!table->unVeto(amendmentId(supportedYes_[1]))); + BEAST_EXPECT(!table->unVeto(amendmentId(yes_[1]))); BEAST_EXPECT(desired == table->getDesired()); } @@ -391,7 +467,7 @@ public: } // Veto all supported amendments. Now desired should be empty. - for (std::string const& a : supportedYes_) + for (std::string const& a : allSupported_) { table->veto(amendmentId(a)); } @@ -533,7 +609,7 @@ public: test::jtx::Env env{*this}; auto table = - makeTable(env, weeks(2), emptyYes, emptySection, emptySection); + makeTable(env, weeks(2), emptyYes_, emptySection_, emptySection_); std::vector> votes; std::vector ourVotes; @@ -594,7 +670,11 @@ public: test::jtx::Env env{*this}; auto table = makeTable( - env, weeks(2), emptyYes, emptySection, makeSection(testAmendment)); + env, + weeks(2), + emptyYes_, + emptySection_, + makeSection(testAmendment)); auto const validators = makeValidators(10); @@ -653,11 +733,7 @@ public: test::jtx::Env env{*this}; auto table = makeTable( - env, - weeks(2), - makeDefaultYes(supportedYes_), - emptySection, - emptySection); + env, weeks(2), makeDefaultYes(yes_), emptySection_, emptySection_); auto const validators = makeValidators(10); std::vector> votes; @@ -675,13 +751,13 @@ public: ourVotes, enabled, majority); - BEAST_EXPECT(ourVotes.size() == supportedYes_.size()); + BEAST_EXPECT(ourVotes.size() == yes_.size()); BEAST_EXPECT(enabled.empty()); - for (auto const& i : supportedYes_) + for (auto const& i : yes_) BEAST_EXPECT(majority.find(amendmentId(i)) == majority.end()); // Now, everyone votes for this feature - for (auto const& i : supportedYes_) + for (auto const& i : yes_) votes.emplace_back(amendmentId(i), validators.size()); // Week 2: We should recognize a majority @@ -694,10 +770,10 @@ public: ourVotes, enabled, majority); - BEAST_EXPECT(ourVotes.size() == supportedYes_.size()); + BEAST_EXPECT(ourVotes.size() == yes_.size()); BEAST_EXPECT(enabled.empty()); - for (auto const& i : supportedYes_) + for (auto const& i : yes_) BEAST_EXPECT(majority[amendmentId(i)] == weekTime(weeks{2})); // Week 5: We should enable the amendment @@ -710,7 +786,7 @@ public: ourVotes, enabled, majority); - BEAST_EXPECT(enabled.size() == supportedYes_.size()); + BEAST_EXPECT(enabled.size() == yes_.size()); // Week 6: We should remove it from our votes and from having a majority doRound( @@ -722,9 +798,9 @@ public: ourVotes, enabled, majority); - BEAST_EXPECT(enabled.size() == supportedYes_.size()); + BEAST_EXPECT(enabled.size() == yes_.size()); BEAST_EXPECT(ourVotes.empty()); - for (auto const& i : supportedYes_) + for (auto const& i : yes_) BEAST_EXPECT(majority.find(amendmentId(i)) == majority.end()); } @@ -740,8 +816,8 @@ public: env, weeks(2), makeDefaultYes(testAmendment), - emptySection, - emptySection); + emptySection_, + emptySection_); auto const validators = makeValidators(16); @@ -811,8 +887,8 @@ public: env, weeks(8), makeDefaultYes(testAmendment), - emptySection, - emptySection); + emptySection_, + emptySection_); std::set enabled; majorityAmendments_t majority; diff --git a/src/test/rpc/Feature_test.cpp b/src/test/rpc/Feature_test.cpp index 1ba4d865d4..dcd95c8a96 100644 --- a/src/test/rpc/Feature_test.cpp +++ b/src/test/rpc/Feature_test.cpp @@ -31,25 +31,33 @@ class Feature_test : public beast::unit_test::suite { testcase("internals"); - std::map const& supported = + 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 : + std::size_t up = 0, down = 0, obsolete = 0; + for (std::pair const& amendment : supported) { - if (amendment.second == DefaultVote::no) - ++down; - else + switch (amendment.second) { - if (BEAST_EXPECT(amendment.second == DefaultVote::yes)) + case VoteBehavior::DefaultYes: ++up; + break; + case VoteBehavior::DefaultNo: + ++down; + break; + case VoteBehavior::Obsolete: + ++obsolete; + break; + default: + fail("Unknown VoteBehavior", __FILE__, __LINE__); } } - BEAST_EXPECT(down == ripple::detail::numDownVotedAmendments()); + BEAST_EXPECT( + down + obsolete == ripple::detail::numDownVotedAmendments()); BEAST_EXPECT(up == ripple::detail::numUpVotedAmendments()); } @@ -105,7 +113,7 @@ class Feature_test : public beast::unit_test::suite using namespace test::jtx; Env env{*this}; - std::map const& votes = + std::map const& votes = ripple::detail::supportedAmendments(); auto jrr = env.rpc("feature")[jss::result]; @@ -118,15 +126,26 @@ class Feature_test : public beast::unit_test::suite // default config - so all should be disabled, and // supported. Some may be vetoed. bool expectVeto = - !(votes.at(feature[jss::name].asString()) == DefaultVote::yes); + (votes.at(feature[jss::name].asString()) == + VoteBehavior::DefaultNo); + bool expectObsolete = + (votes.at(feature[jss::name].asString()) == + VoteBehavior::Obsolete); BEAST_EXPECTS( - !feature[jss::enabled].asBool(), + feature.isMember(jss::enabled) && + !feature[jss::enabled].asBool(), feature[jss::name].asString() + " enabled"); BEAST_EXPECTS( - feature[jss::vetoed].asBool() == expectVeto, + feature.isMember(jss::vetoed) && + feature[jss::vetoed].isBool() == !expectObsolete && + (!feature[jss::vetoed].isBool() || + feature[jss::vetoed].asBool() == expectVeto) && + (feature[jss::vetoed].isBool() || + feature[jss::vetoed].asString() == "Obsolete"), feature[jss::name].asString() + " vetoed"); BEAST_EXPECTS( - feature[jss::supported].asBool(), + feature.isMember(jss::supported) && + feature[jss::supported].asBool(), feature[jss::name].asString() + " supported"); } } @@ -150,7 +169,9 @@ class Feature_test : public beast::unit_test::suite BEAST_EXPECTS(feature[jss::name] == "MultiSignReserve", "name"); BEAST_EXPECTS(!feature[jss::enabled].asBool(), "enabled"); - BEAST_EXPECTS(!feature[jss::vetoed].asBool(), "vetoed"); + BEAST_EXPECTS( + feature[jss::vetoed].isBool() && !feature[jss::vetoed].asBool(), + "vetoed"); BEAST_EXPECTS(feature[jss::supported].asBool(), "supported"); // feature names are case-sensitive - expect error here @@ -200,7 +221,7 @@ class Feature_test : public beast::unit_test::suite Env env{ *this, FeatureBitset(featureDepositAuth, featureDepositPreauth)}; - std::map const& votes = + std::map const& votes = ripple::detail::supportedAmendments(); auto jrr = env.rpc("feature")[jss::result]; @@ -218,15 +239,31 @@ class Feature_test : public beast::unit_test::suite bool expectSupported = env.app().getAmendmentTable().isSupported(id); bool expectVeto = - !(votes.at((*it)[jss::name].asString()) == DefaultVote::yes); + (votes.at((*it)[jss::name].asString()) == + VoteBehavior::DefaultNo); + bool expectObsolete = + (votes.at((*it)[jss::name].asString()) == + VoteBehavior::Obsolete); BEAST_EXPECTS( - (*it)[jss::enabled].asBool() == expectEnabled, + (*it).isMember(jss::enabled) && + (*it)[jss::enabled].asBool() == expectEnabled, (*it)[jss::name].asString() + " enabled"); + if (expectEnabled) + BEAST_EXPECTS( + !(*it).isMember(jss::vetoed), + (*it)[jss::name].asString() + " vetoed"); + else + BEAST_EXPECTS( + (*it).isMember(jss::vetoed) && + (*it)[jss::vetoed].isBool() == !expectObsolete && + (!(*it)[jss::vetoed].isBool() || + (*it)[jss::vetoed].asBool() == expectVeto) && + ((*it)[jss::vetoed].isBool() || + (*it)[jss::vetoed].asString() == "Obsolete"), + (*it)[jss::name].asString() + " vetoed"); BEAST_EXPECTS( - (*it)[jss::vetoed].asBool() == expectVeto, - (*it)[jss::name].asString() + " vetoed"); - BEAST_EXPECTS( - (*it)[jss::supported].asBool() == expectSupported, + (*it).isMember(jss::supported) && + (*it)[jss::supported].asBool() == expectSupported, (*it)[jss::name].asString() + " supported"); } } @@ -282,7 +319,7 @@ 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 = + std::map const& votes = ripple::detail::supportedAmendments(); jrr = env.rpc("feature")[jss::result]; @@ -293,13 +330,22 @@ 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); + (votes.at(feature[jss::name].asString()) == + VoteBehavior::DefaultNo); + bool expectObsolete = + (votes.at(feature[jss::name].asString()) == + VoteBehavior::Obsolete); BEAST_EXPECTS( - expectVeto ^ feature.isMember(jss::majority), + (expectVeto || expectObsolete) ^ + feature.isMember(jss::majority), feature[jss::name].asString() + " majority"); BEAST_EXPECTS( feature.isMember(jss::vetoed) && - feature[jss::vetoed].asBool() == expectVeto, + feature[jss::vetoed].isBool() == !expectObsolete && + (!feature[jss::vetoed].isBool() || + feature[jss::vetoed].asBool() == expectVeto) && + (feature[jss::vetoed].isBool() || + feature[jss::vetoed].asString() == "Obsolete"), feature[jss::name].asString() + " vetoed"); BEAST_EXPECTS( feature.isMember(jss::count), @@ -310,11 +356,13 @@ class Feature_test : public beast::unit_test::suite BEAST_EXPECTS( feature.isMember(jss::validations), feature[jss::name].asString() + " validations"); - BEAST_EXPECT(feature[jss::count] == (expectVeto ? 0 : 1)); + BEAST_EXPECT( + feature[jss::count] == + ((expectVeto || expectObsolete) ? 0 : 1)); BEAST_EXPECT(feature[jss::threshold] == 1); BEAST_EXPECT(feature[jss::validations] == 1); BEAST_EXPECTS( - expectVeto || feature[jss::majority] == 2540, + expectVeto || expectObsolete || feature[jss::majority] == 2540, "Majority: " + feature[jss::majority].asString()); } } @@ -326,39 +374,100 @@ class Feature_test : public beast::unit_test::suite using namespace test::jtx; Env env{*this, FeatureBitset(featureMultiSignReserve)}; + constexpr const char* featureName = "MultiSignReserve"; - auto jrr = env.rpc("feature", "MultiSignReserve")[jss::result]; + auto jrr = env.rpc("feature", featureName)[jss::result]; if (!BEAST_EXPECTS(jrr[jss::status] == jss::success, "status")) return; jrr.removeMember(jss::status); if (!BEAST_EXPECT(jrr.size() == 1)) return; auto feature = *(jrr.begin()); - BEAST_EXPECTS(feature[jss::name] == "MultiSignReserve", "name"); - BEAST_EXPECTS(!feature[jss::vetoed].asBool(), "vetoed"); + BEAST_EXPECTS(feature[jss::name] == featureName, "name"); + BEAST_EXPECTS( + feature[jss::vetoed].isBool() && !feature[jss::vetoed].asBool(), + "vetoed"); - jrr = env.rpc("feature", "MultiSignReserve", "reject")[jss::result]; + jrr = env.rpc("feature", featureName, "reject")[jss::result]; if (!BEAST_EXPECTS(jrr[jss::status] == jss::success, "status")) return; jrr.removeMember(jss::status); if (!BEAST_EXPECT(jrr.size() == 1)) return; feature = *(jrr.begin()); - BEAST_EXPECTS(feature[jss::name] == "MultiSignReserve", "name"); - BEAST_EXPECTS(feature[jss::vetoed].asBool(), "vetoed"); + BEAST_EXPECTS(feature[jss::name] == featureName, "name"); + BEAST_EXPECTS( + feature[jss::vetoed].isBool() && feature[jss::vetoed].asBool(), + "vetoed"); - jrr = env.rpc("feature", "MultiSignReserve", "accept")[jss::result]; + jrr = env.rpc("feature", featureName, "accept")[jss::result]; if (!BEAST_EXPECTS(jrr[jss::status] == jss::success, "status")) return; jrr.removeMember(jss::status); if (!BEAST_EXPECT(jrr.size() == 1)) return; feature = *(jrr.begin()); - BEAST_EXPECTS(feature[jss::name] == "MultiSignReserve", "name"); - BEAST_EXPECTS(!feature[jss::vetoed].asBool(), "vetoed"); + BEAST_EXPECTS(feature[jss::name] == featureName, "name"); + BEAST_EXPECTS( + feature[jss::vetoed].isBool() && !feature[jss::vetoed].asBool(), + "vetoed"); // anything other than accept or reject is an error - jrr = env.rpc("feature", "MultiSignReserve", "maybe"); + jrr = env.rpc("feature", featureName, "maybe"); + BEAST_EXPECT(jrr[jss::error] == "invalidParams"); + BEAST_EXPECT(jrr[jss::error_message] == "Invalid parameters."); + } + + void + testObsolete() + { + testcase("Obsolete"); + + using namespace test::jtx; + Env env{*this}; + constexpr const char* featureName = "NonFungibleTokensV1"; + + auto jrr = env.rpc("feature", featureName)[jss::result]; + if (!BEAST_EXPECTS(jrr[jss::status] == jss::success, "status")) + return; + jrr.removeMember(jss::status); + if (!BEAST_EXPECT(jrr.size() == 1)) + return; + auto feature = *(jrr.begin()); + BEAST_EXPECTS(feature[jss::name] == featureName, "name"); + BEAST_EXPECTS( + feature[jss::vetoed].isString() && + feature[jss::vetoed].asString() == "Obsolete", + "vetoed"); + + jrr = env.rpc("feature", featureName, "reject")[jss::result]; + if (!BEAST_EXPECTS(jrr[jss::status] == jss::success, "status")) + return; + jrr.removeMember(jss::status); + if (!BEAST_EXPECT(jrr.size() == 1)) + return; + feature = *(jrr.begin()); + BEAST_EXPECTS(feature[jss::name] == featureName, "name"); + BEAST_EXPECTS( + feature[jss::vetoed].isString() && + feature[jss::vetoed].asString() == "Obsolete", + "vetoed"); + + jrr = env.rpc("feature", featureName, "accept")[jss::result]; + if (!BEAST_EXPECTS(jrr[jss::status] == jss::success, "status")) + return; + jrr.removeMember(jss::status); + if (!BEAST_EXPECT(jrr.size() == 1)) + return; + feature = *(jrr.begin()); + BEAST_EXPECTS(feature[jss::name] == featureName, "name"); + BEAST_EXPECTS( + feature[jss::vetoed].isString() && + feature[jss::vetoed].asString() == "Obsolete", + "vetoed"); + + // anything other than accept or reject is an error + jrr = env.rpc("feature", featureName, "maybe"); BEAST_EXPECT(jrr[jss::error] == "invalidParams"); BEAST_EXPECT(jrr[jss::error_message] == "Invalid parameters."); } @@ -376,6 +485,7 @@ public: testSomeEnabled(); testWithMajorities(); testVeto(); + testObsolete(); } };