From 744bc9d2175fc0b66ea496f9c0971609597b287c Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Wed, 12 Mar 2025 17:34:06 -0400 Subject: [PATCH] refactor: Calculate numFeatures automatically (#5324) Requiring manual updates of numFeatures is an annoying manual process that is easily forgotten, and leads to frequent merge conflicts. This change takes advantage of the `XRPL_FEATURE` and `XRPL_FIX` macros, and adds a new `XRPL_RETIRE` macro to automatically set `numFeatures`. --- include/xrpl/protocol/Feature.h | 82 ++++++++++++++------- include/xrpl/protocol/detail/features.macro | 22 ++++++ src/libxrpl/protocol/Feature.cpp | 38 ++-------- 3 files changed, 86 insertions(+), 56 deletions(-) diff --git a/include/xrpl/protocol/Feature.h b/include/xrpl/protocol/Feature.h index 29efdc574..a2eb7d8e5 100644 --- a/include/xrpl/protocol/Feature.h +++ b/include/xrpl/protocol/Feature.h @@ -35,35 +35,39 @@ * * Steps required to add new features to the code: * - * 1) In this file, increment `numFeatures` and add a uint256 declaration - * for the feature at the bottom - * 2) Add a uint256 definition for the feature to the corresponding source - * file (Feature.cpp). Use `registerFeature` to create the feature with - * the feature's name, `Supported::no`, and `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 `VoteBehavior::DefaultYes`. - * In general, any newly supported amendments (`Supported::yes`) should have - * a `VoteBehavior::DefaultNo` for at least one full release cycle. High - * priority bug fixes can be an exception to this rule of thumb. + * 1) Add the appropriate XRPL_FEATURE or XRPL_FIX macro definition for the + * feature to features.macro with the feature's name, `Supported::no`, and + * `VoteBehavior::DefaultNo`. + * + * 2) Use the generated variable name as the parameter to `view.rules.enabled()` + * to control flow into new code that this feature limits. (featureName or + * fixName) + * + * 3) If the feature development is COMPLETE, and the feature is ready to be + * SUPPORTED, change the macro parameter in features.macro to Supported::yes. + * + * 4) In general, any newly supported amendments (`Supported::yes`) should have + * a `VoteBehavior::DefaultNo` indefinitely so that external governance can + * make the decision on when to activate it. High priority bug fixes can be + * an exception to this rule. In such cases, ensure the fix has been + * clearly communicated to the community using appropriate channels, + * then change the macro parameter in features.macro to + * `VoteBehavior::DefaultYes`. The communication process is beyond + * the scope of these instructions. + * * * When a feature has been enabled for several years, the conditional code * may be removed, and the feature "retired". To retire a feature: - * 1) Remove the uint256 declaration from this file. - * 2) MOVE the uint256 definition in Feature.cpp to the "retired features" - * section at the end of the file. - * 3) CHANGE the name of the variable to start with "retired". - * 4) CHANGE the parameters of the `registerFeature` call to `Supported::yes` - * and `VoteBehavior::DefaultNo`. + * + * 1) MOVE the macro definition in features.macro to the "retired features" + * section at the end of the file, and change the macro to XRPL_RETIRE. + * * The feature must remain registered and supported indefinitely because it - * still exists in the ledger, but there is no need to vote for it because - * there's nothing to vote for. If it is removed completely from the code, any - * instances running that code will get amendment blocked. Removing the - * feature from the ledger is beyond the scope of these instructions. + * may exist in the Amendments object on ledger. There is no need to vote + * for it because there's nothing to vote for. If the feature definition is + * removed completely from the code, any instances running that code will get + * amendment blocked. Removing the feature from the ledger is beyond the scope + * of these instructions. * */ @@ -78,11 +82,32 @@ allAmendments(); namespace detail { +#pragma push_macro("XRPL_FEATURE") +#undef XRPL_FEATURE +#pragma push_macro("XRPL_FIX") +#undef XRPL_FIX +#pragma push_macro("XRPL_RETIRE") +#undef XRPL_RETIRE + +#define XRPL_FEATURE(name, supported, vote) +1 +#define XRPL_FIX(name, supported, vote) +1 +#define XRPL_RETIRE(name) +1 + // This value SHOULD be equal to the number of amendments registered in // Feature.cpp. Because it's only used to reserve storage, and determine how // large to make the FeatureBitset, it MAY be larger. It MUST NOT be less than // the actual number of amendments. A LogicError on startup will verify this. -static constexpr std::size_t numFeatures = 110; +static constexpr std::size_t numFeatures = + (0 + +#include + ); + +#undef XRPL_RETIRE +#pragma pop_macro("XRPL_RETIRE") +#undef XRPL_FIX +#pragma pop_macro("XRPL_FIX") +#undef XRPL_FEATURE +#pragma pop_macro("XRPL_FEATURE") /** Amendments that this server supports and the default voting behavior. Whether they are enabled depends on the Rules defined in the validated @@ -322,12 +347,17 @@ foreachFeature(FeatureBitset bs, F&& f) #undef XRPL_FEATURE #pragma push_macro("XRPL_FIX") #undef XRPL_FIX +#pragma push_macro("XRPL_RETIRE") +#undef XRPL_RETIRE #define XRPL_FEATURE(name, supported, vote) extern uint256 const feature##name; #define XRPL_FIX(name, supported, vote) extern uint256 const fix##name; +#define XRPL_RETIRE(name) #include +#undef XRPL_RETIRE +#pragma pop_macro("XRPL_RETIRE") #undef XRPL_FIX #pragma pop_macro("XRPL_FIX") #undef XRPL_FEATURE diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index 1dfe26779..a56bc815e 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -23,6 +23,9 @@ #if !defined(XRPL_FIX) #error "undefined macro: XRPL_FIX" #endif +#if !defined(XRPL_RETIRE) +#error "undefined macro: XRPL_RETIRE" +#endif // Add new amendments to the top of this list. // Keep it sorted in reverse chronological order. @@ -142,3 +145,22 @@ XRPL_FIX (NFTokenDirV1, Supported::yes, VoteBehavior::Obsolete) XRPL_FEATURE(NonFungibleTokensV1, Supported::yes, VoteBehavior::Obsolete) XRPL_FEATURE(CryptoConditionsSuite, 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. +// All known amendments and amendments that may appear in a validated +// ledger must be registered either here or above with the "active" amendments +XRPL_RETIRE(MultiSign) +XRPL_RETIRE(TrustSetAuth) +XRPL_RETIRE(FeeEscalation) +XRPL_RETIRE(PayChan) +XRPL_RETIRE(CryptoConditions) +XRPL_RETIRE(TickSize) +XRPL_RETIRE(fix1368) +XRPL_RETIRE(Escrow) +XRPL_RETIRE(fix1373) +XRPL_RETIRE(EnforceInvariants) +XRPL_RETIRE(SortedDirectories) +XRPL_RETIRE(fix1201) +XRPL_RETIRE(fix1512) +XRPL_RETIRE(fix1523) +XRPL_RETIRE(fix1528) diff --git a/src/libxrpl/protocol/Feature.cpp b/src/libxrpl/protocol/Feature.cpp index 682f44456..3065bdf41 100644 --- a/src/libxrpl/protocol/Feature.cpp +++ b/src/libxrpl/protocol/Feature.cpp @@ -259,12 +259,9 @@ FeatureCollections::registerFeature( Feature const* i = getByName(name); if (!i) { - // If this check fails, and you just added a feature, increase the - // numFeatures value in Feature.h check( features.size() < detail::numFeatures, - "More features defined than allocated. Adjust numFeatures in " - "Feature.h."); + "More features defined than allocated."); auto const f = sha512Half(Slice(name.data(), name.size())); @@ -433,45 +430,26 @@ featureToName(uint256 const& f) #undef XRPL_FEATURE #pragma push_macro("XRPL_FIX") #undef XRPL_FIX +#pragma push_macro("XRPL_RETIRE") +#undef XRPL_RETIRE #define XRPL_FEATURE(name, supported, vote) \ uint256 const feature##name = registerFeature(#name, supported, vote); #define XRPL_FIX(name, supported, vote) \ uint256 const fix##name = registerFeature("fix" #name, supported, vote); +#define XRPL_RETIRE(name) \ + [[deprecated("The referenced amendment has been retired"), maybe_unused]] \ + uint256 const retired##name = retireFeature(#name); #include +#undef XRPL_RETIRE +#pragma pop_macro("XRPL_RETIRE") #undef XRPL_FIX #pragma pop_macro("XRPL_FIX") #undef XRPL_FEATURE #pragma pop_macro("XRPL_FEATURE") -// clang-format off - -// The following amendments have been active for at least two years. Their -// pre-amendment code has been removed and the identifiers are deprecated. -// All known amendments and amendments that may appear in a validated -// ledger must be registered either here or above with the "active" amendments -[[deprecated("The referenced amendment has been retired"), maybe_unused]] -uint256 const - retiredMultiSign = retireFeature("MultiSign"), - retiredTrustSetAuth = retireFeature("TrustSetAuth"), - retiredFeeEscalation = retireFeature("FeeEscalation"), - retiredPayChan = retireFeature("PayChan"), - retiredCryptoConditions = retireFeature("CryptoConditions"), - retiredTickSize = retireFeature("TickSize"), - retiredFix1368 = retireFeature("fix1368"), - retiredEscrow = retireFeature("Escrow"), - retiredFix1373 = retireFeature("fix1373"), - retiredEnforceInvariants = retireFeature("EnforceInvariants"), - retiredSortedDirectories = retireFeature("SortedDirectories"), - retiredFix1201 = retireFeature("fix1201"), - retiredFix1512 = retireFeature("fix1512"), - retiredFix1523 = retireFeature("fix1523"), - retiredFix1528 = retireFeature("fix1528"); - -// clang-format on - // All of the features should now be registered, since variables in a cpp file // are initialized from top to bottom. //