From 16d1e3a3093b2e61a6c557b1fa2eb1ac10378bbd Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Fri, 31 Jan 2025 13:25:31 -0500 Subject: [PATCH 1/5] Calculate numFeatures automatically - Update documentation --- include/xrpl/protocol/Feature.h | 69 ++++++++++++++------- include/xrpl/protocol/detail/features.macro | 22 +++++++ src/libxrpl/protocol/Feature.cpp | 38 +++--------- 3 files changed, 77 insertions(+), 52 deletions(-) diff --git a/include/xrpl/protocol/Feature.h b/include/xrpl/protocol/Feature.h index 1c476df617f..bdec238651c 100644 --- a/include/xrpl/protocol/Feature.h +++ b/include/xrpl/protocol/Feature.h @@ -33,35 +33,35 @@ * * 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`. + * 1) Add a 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) When the feature is ready to be ENABLED, change the macro parameter in + * features.macro 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. * * 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. + * there's nothing to vote for. If the 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. * */ @@ -80,7 +80,27 @@ namespace detail { // 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 = 88; +static constexpr std::size_t numFeatures = 0 +#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 + +#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 @@ -320,12 +340,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 7b120c0b8d2..f54fb9e9742 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. @@ -121,3 +124,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 05164489ec7..52e1d7217cf 100644 --- a/src/libxrpl/protocol/Feature.cpp +++ b/src/libxrpl/protocol/Feature.cpp @@ -250,12 +250,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())); @@ -424,45 +421,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. // From a1d5340c16d5e31c16170b65820f2478064a067f Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Thu, 27 Feb 2025 19:33:16 -0500 Subject: [PATCH 2/5] Review feedback from @Bronek: move the macros outside of the computation --- include/xrpl/protocol/Feature.h | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/include/xrpl/protocol/Feature.h b/include/xrpl/protocol/Feature.h index bdec238651c..d61aaaf904a 100644 --- a/include/xrpl/protocol/Feature.h +++ b/include/xrpl/protocol/Feature.h @@ -76,11 +76,6 @@ allAmendments(); namespace detail { -// 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 = 0 #pragma push_macro("XRPL_FEATURE") #undef XRPL_FEATURE #pragma push_macro("XRPL_FIX") @@ -92,7 +87,14 @@ static constexpr std::size_t numFeatures = 0 #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 = + (0 + #include + ); #undef XRPL_RETIRE #pragma pop_macro("XRPL_RETIRE") @@ -100,7 +102,6 @@ static constexpr std::size_t numFeatures = 0 #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 From 0fc28f10785e3ee25b5ec8a8a1d27cc02edbd06e Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Fri, 28 Feb 2025 15:49:57 -0500 Subject: [PATCH 3/5] Review feedback from @Bronek: - Reword some of the instructions to better reflect current behaviors. --- include/xrpl/protocol/Feature.h | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/include/xrpl/protocol/Feature.h b/include/xrpl/protocol/Feature.h index d61aaaf904a..3a3d9d4bf21 100644 --- a/include/xrpl/protocol/Feature.h +++ b/include/xrpl/protocol/Feature.h @@ -44,12 +44,12 @@ * 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) When the feature is ready to be ENABLED, change the macro parameter in - * features.macro parameter to `VoteBehavior::DefaultYes`. + * 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, change the macro parameter in + * features.macro 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. * * When a feature has been enabled for several years, the conditional code * may be removed, and the feature "retired". To retire a feature: @@ -58,10 +58,11 @@ * 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 the 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. + * 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. * */ From 9429bf473d6d41973cff93e0209046ffef727a37 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Fri, 28 Feb 2025 18:28:16 -0500 Subject: [PATCH 4/5] Update include/xrpl/protocol/Feature.h Fix typo found by @mvadari Co-authored-by: Mayukha Vadari --- include/xrpl/protocol/Feature.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/xrpl/protocol/Feature.h b/include/xrpl/protocol/Feature.h index 3a3d9d4bf21..0b2afbb2000 100644 --- a/include/xrpl/protocol/Feature.h +++ b/include/xrpl/protocol/Feature.h @@ -33,7 +33,7 @@ * * Steps required to add new features to the code: * - * 1) Add a the appropriate XRPL_FEATURE or XRPL_FIX macro definition for the + * 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`. * From 77add335249407ac878d82d727496b3008dcc8ef Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Tue, 11 Mar 2025 11:30:57 -0400 Subject: [PATCH 5/5] Review feedback from @Bronek: Clarify documentation --- include/xrpl/protocol/Feature.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/include/xrpl/protocol/Feature.h b/include/xrpl/protocol/Feature.h index 0b2afbb2000..617ae47090a 100644 --- a/include/xrpl/protocol/Feature.h +++ b/include/xrpl/protocol/Feature.h @@ -47,8 +47,11 @@ * 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, change the macro parameter in - * features.macro to `VoteBehavior::DefaultYes`. + * 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