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 | 170 +++++++++++++----- src/test/rpc/Feature_test.cpp | 184 ++++++++++++++++---- 7 files changed, 385 insertions(+), 163 deletions(-) diff --git a/src/ripple/app/misc/AmendmentTable.h b/src/ripple/app/misc/AmendmentTable.h index 14558b7da51..0a5f6a011ad 100644 --- a/src/ripple/app/misc/AmendmentTable.h +++ b/src/ripple/app/misc/AmendmentTable.h @@ -40,14 +40,14 @@ class AmendmentTable 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 93113af800e..6f9ea86fa6c 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 2769e459acc..e9846714ece 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 62dc327d98d..e4b0e3d4acd 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 d15c3fc60ff..6b8e7719d00 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 @@ class FeatureCollections registerFeature( std::string const& name, Supported support, - DefaultVote vote); + VoteBehavior vote); /** Tell FeatureCollections when registration is complete. */ bool @@ -181,7 +182,7 @@ class 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& 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 99922e863a4..4284190a18a 100644 --- a/src/test/app/AmendmentTable_test.cpp +++ b/src/test/app/AmendmentTable_test.cpp @@ -87,45 +87,105 @@ class AmendmentTable_test final : public beast::unit_test::suite } 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 @@ class AmendmentTable_test final : public beast::unit_test::suite 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 @@ class AmendmentTable_test final : public beast::unit_test::suite 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,17 +240,22 @@ 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 : 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 : vetoed_) + for (auto const& a : obsolete_) { BEAST_EXPECT(table->isSupported(amendmentId(a))); BEAST_EXPECT(!table->isEnabled(amendmentId(a))); @@ -195,13 +270,14 @@ 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 : 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 @@ class AmendmentTable_test final : public beast::unit_test::suite 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 @@ class AmendmentTable_test final : public beast::unit_test::suite 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 @@ class AmendmentTable_test final : public beast::unit_test::suite 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 @@ class AmendmentTable_test final : public beast::unit_test::suite 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 @@ class AmendmentTable_test final : public beast::unit_test::suite 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 @@ class AmendmentTable_test final : public beast::unit_test::suite 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 @@ 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 (convert the amendment names to IDs) std::set allEnabled; for (auto const& a : enabled_) allEnabled.insert(amendmentId(a)); @@ -351,7 +427,7 @@ class AmendmentTable_test final : public beast::unit_test::suite 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 @@ 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. - BEAST_EXPECT(!table->unVeto(amendmentId(supportedYes_[1]))); + BEAST_EXPECT(!table->unVeto(amendmentId(yes_[1]))); BEAST_EXPECT(desired == table->getDesired()); } @@ -391,7 +467,7 @@ class AmendmentTable_test final : public beast::unit_test::suite } // 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 @@ class AmendmentTable_test final : public beast::unit_test::suite 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 @@ class AmendmentTable_test final : public beast::unit_test::suite 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 @@ class AmendmentTable_test final : public beast::unit_test::suite 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 @@ class AmendmentTable_test final : public beast::unit_test::suite 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 @@ class AmendmentTable_test final : public beast::unit_test::suite 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 @@ class AmendmentTable_test final : public beast::unit_test::suite 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 @@ class AmendmentTable_test final : public beast::unit_test::suite 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 @@ class AmendmentTable_test final : public beast::unit_test::suite env, weeks(2), makeDefaultYes(testAmendment), - emptySection, - emptySection); + emptySection_, + emptySection_); auto const validators = makeValidators(16); @@ -811,8 +887,8 @@ class AmendmentTable_test final : public beast::unit_test::suite 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 1ba4d865d44..dcd95c8a968 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", 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", "MultiSignReserve", "maybe"); + jrr = env.rpc("feature", featureName, "maybe"); BEAST_EXPECT(jrr[jss::error] == "invalidParams"); BEAST_EXPECT(jrr[jss::error_message] == "Invalid parameters."); } @@ -376,6 +485,7 @@ class Feature_test : public beast::unit_test::suite testSomeEnabled(); testWithMajorities(); testVeto(); + testObsolete(); } };