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

@@ -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;

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<uint256> 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_)

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.

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<std::string, DefaultVote> const&
+std::map<std::string, VoteBehavior> const&
 supportedAmendments();
 
 /** Amendments that this server won't vote for by default.

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<Feature, feature_indexing>
  features;
- std::map<std::string, DefaultVote> supported;
+ std::map<std::string, VoteBehavior> supported;
  std::size_t upVotes = 0;
  std::size_t downVotes = 0;
  mutable std::atomic<bool> 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<std::string, DefaultVote> const&
+ std::map<std::string, VoteBehavior> 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<std::string, DefaultVote> const&
+std::map<std::string, VoteBehavior> 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.