Add the ability to mark amendments as obsolete

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.