fix: Remove a newline from logging statement in changeSpotPrice calculation (#6547)

This change removes an unnecessary newline in a logging statement. Namely, `std::endl` is unneeded in `JLOG`, since it automatically places a newline at the end of the string.

include/xrpl/protocol/Feature.h

@@ -64,6 +64,49 @@
 
 namespace xrpl {
 
+// Feature names must not exceed this length (in characters, excluding the null terminator).
+static constexpr std::size_t maxFeatureNameSize = 63;
+// Reserve this exact feature-name length (in characters/bytes, excluding the null terminator)
+// so that a 32-byte uint256 (for example, in WASM or other interop contexts) can be used
+// as a compact, fixed-size feature selector without conflicting with human-readable names.
+static constexpr std::size_t reservedFeatureNameSize = 32;
+
+// Both validFeatureNameSize and validFeatureName are consteval functions that can be used in
+// static_asserts to validate feature names at compile time. They are only used inside
+// enforceValidFeatureName in Feature.cpp, but are exposed here for testing. The expected
+// parameter `auto fn` is a constexpr lambda which returns a const char*, making it available
+// for compile-time evaluation. Read more in https://accu.org/journals/overload/30/172/wu/
+consteval auto
+validFeatureNameSize(auto fn) -> bool
+{
+    constexpr char const* n = fn();
+    // Note, std::strlen is not constexpr, we need to implement our own here.
+    constexpr std::size_t N = [](auto n) {
+        std::size_t ret = 0;
+        for (auto ptr = n; *ptr != '\0'; ret++, ++ptr)
+            ;
+        return ret;
+    }(n);
+    return N != reservedFeatureNameSize &&  //
+        N <= maxFeatureNameSize;
+}
+
+consteval auto
+validFeatureName(auto fn) -> bool
+{
+    constexpr char const* n = fn();
+    // Prevent the use of visually confusable characters and enforce that feature names
+    // are always valid ASCII. This is needed because C++ allows Unicode identifiers.
+    // Characters below 0x20 are nonprintable control characters, and characters with the 0x80 bit
+    // set are non-ASCII (e.g. UTF-8 encoding of Unicode), so both are disallowed.
+    for (auto ptr = n; *ptr != '\0'; ++ptr)
+    {
+        if (*ptr & 0x80 || *ptr < 0x20)
+            return false;
+    }
+    return true;
+}
+
 enum class VoteBehavior : int { Obsolete = -1, DefaultNo = 0, DefaultYes };
 enum class AmendmentSupport : int { Retired = -1, Supported = 0, Unsupported };
 

include/xrpl/tx/invariants/InvariantCheck.h

@@ -27,6 +27,33 @@ namespace xrpl {
  * communicate the interface required of any invariant checker. Any invariant
  * check implementation should implement the public methods documented here.
  *
+ * ## Rules for implementing `finalize`
+ *
+ * ### Invariants must run regardless of transaction result
+ *
+ * An invariant's `finalize` method MUST perform meaningful checks even when
+ * the transaction has failed (i.e., `!isTesSuccess(tec)`). The following
+ * pattern is almost certainly wrong and must never be used:
+ *
+ * @code
+ * // WRONG: skipping all checks on failure defeats the purpose of invariants
+ * if (!isTesSuccess(tec))
+ *     return true;
+ * @endcode
+ *
+ * The entire purpose of invariants is to detect and prevent the impossible.
+ * A bug or exploit could cause a failed transaction to mutate ledger state in
+ * unexpected ways. Invariants are the last line of defense against such
+ * scenarios.
+ *
+ * In general: an invariant that expects a domain-specific state change to
+ * occur (e.g., a new object being created) should only expect that change
+ * when the transaction succeeded. A failed VaultCreate must not have created
+ * a Vault. A failed LoanSet must not have created a Loan.
+ *
+ * Also be aware that failed transactions, regardless of type, carry no
+ * Privileges. Any privilege-gated checks must therefore also be applied to
+ * failed transactions.
  */
 class InvariantChecker_PROTOTYPE
 {
@@ -48,7 +75,11 @@ public:
 
     /**
      * @brief called after all ledger entries have been visited to determine
-     * the final status of the check
+     * the final status of the check.
+     *
+     * This method MUST perform meaningful checks even when `tec` indicates a
+     * failed transaction. See the class-level documentation for the rules
+     * governing how failed transactions must be handled.
      *
      * @param tx the transaction being applied
      * @param tec the current TER result of the transaction

include/xrpl/tx/transactors/dex/AMMHelpers.h

@@ -370,7 +370,7 @@ changeSpotPriceQuality(
     if (!amounts)
     {
         JLOG(j.trace()) << "changeSpotPrice calc failed: " << to_string(pool.in) << " "
-                        << to_string(pool.out) << " " << quality << " " << tfee << std::endl;
+                        << to_string(pool.out) << " " << quality << " " << tfee;
         return std::nullopt;
     }
 

src/libxrpl/protocol/Feature.cpp

@@ -395,10 +395,20 @@ featureToName(uint256 const& f)
 #pragma push_macro("XRPL_RETIRE_FIX")
 #undef XRPL_RETIRE_FIX
 
+consteval auto
+enforceValidFeatureName(auto fn) -> char const*
+{
+    static_assert(validFeatureName(fn), "Invalid feature name");
+    static_assert(validFeatureNameSize(fn), "Invalid feature name size");
+    return fn();
+}
+
 #define XRPL_FEATURE(name, supported, vote) \
-    uint256 const feature##name = registerFeature(#name, supported, vote);
+    uint256 const feature##name =           \
+        registerFeature(enforceValidFeatureName([] { return #name; }), supported, vote);
 #define XRPL_FIX(name, supported, vote) \
-    uint256 const fix##name = registerFeature("fix" #name, supported, vote);
+    uint256 const fix##name =           \
+        registerFeature(enforceValidFeatureName([] { return "fix" #name; }), supported, vote);
 
 // clang-format off
 #define XRPL_RETIRE_FEATURE(name)                                       \

src/libxrpl/tx/transactors/credentials/CredentialCreate.cpp

@@ -107,7 +107,7 @@ CredentialCreate::doApply()
             return tecEXPIRED;
         }
 
-        sleCred->setFieldU32(sfExpiration, ctx_.tx.getFieldU32(sfExpiration));
+        sleCred->setFieldU32(sfExpiration, *optExp);
     }
 
     auto const sleIssuer = view().peek(keylet::account(account_));

src/libxrpl/tx/transactors/dex/AMMDeposit.cpp

@@ -17,7 +17,6 @@ AMMDeposit::checkExtraFeatures(PreflightContext const& ctx)
 
 std::uint32_t
 AMMDeposit::getFlagsMask(PreflightContext const& ctx)
-
 {
     return tfAMMDepositMask;
 }

src/libxrpl/tx/transactors/dex/AMMWithdraw.cpp

@@ -676,7 +676,7 @@ AMMWithdraw::equalWithdrawTokens(
     STAmount const& lpTokens,
     STAmount const& lpTokensWithdraw,
     std::uint16_t tfee,
-    FreezeHandling freezeHanding,
+    FreezeHandling freezeHandling,
     WithdrawAll withdrawAll,
     XRPAmount const& priorBalance,
     beast::Journal const& journal)
@@ -697,7 +697,7 @@ AMMWithdraw::equalWithdrawTokens(
                 lptAMMBalance,
                 lpTokensWithdraw,
                 tfee,
-                freezeHanding,
+                freezeHandling,
                 WithdrawAll::Yes,
                 priorBalance,
                 journal);
@@ -731,7 +731,7 @@ AMMWithdraw::equalWithdrawTokens(
             lptAMMBalance,
             tokensAdj,
             tfee,
-            freezeHanding,
+            freezeHandling,
             withdrawAll,
             priorBalance,
             journal);

src/libxrpl/tx/transactors/dex/CreateOffer.cpp

@@ -214,8 +214,10 @@ CreateOffer::checkAcceptAsset(
         return (flags & tapRETRY) ? TER{terNO_ACCOUNT} : TER{tecNO_ISSUER};
     }
 
+    // An account cannot create a trustline to itself, so no line can exist
+    // to be frozen. Additionally, an issuer can always accept its own
+    // issuance.
     if (issue.account == id)
-        // An account can always accept its own issuance.
         return tesSUCCESS;
 
     if ((*issuerAccount)[sfFlags] & lsfRequireAuth)
@@ -242,14 +244,6 @@ CreateOffer::checkAcceptAsset(
         }
     }
 
-    // An account can not create a trustline to itself, so no line can exist
-    // to be frozen. Additionally, an issuer can always accept its own
-    // issuance.
-    if (issue.account == id)
-    {
-        return tesSUCCESS;
-    }
-
     auto const trustLine = view.read(keylet::line(id, issue.account, issue.currency));
 
     if (!trustLine)

src/test/rpc/Feature_test.cpp

@@ -2,6 +2,7 @@
 
 #include <xrpl/ledger/AmendmentTable.h>
 #include <xrpl/protocol/Feature.h>
+#include <xrpl/protocol/digest.h>
 #include <xrpl/protocol/jss.h>
 
 namespace xrpl {
@@ -168,16 +169,18 @@ class Feature_test : public beast::unit_test::suite
         using namespace test::jtx;
         Env env{*this};
 
-        auto jrr = env.rpc("feature", "fixAMMOverflowOffer")[jss::result];
+        std::string const name = "fixAMMOverflowOffer";
+        auto jrr = env.rpc("feature", name)[jss::result];
         BEAST_EXPECTS(jrr[jss::status] == jss::success, "status");
         jrr.removeMember(jss::status);
         BEAST_EXPECT(jrr.size() == 1);
-        BEAST_EXPECT(jrr.isMember(
+        auto const expected = to_string(sha512Half(Slice(name.data(), name.size())));
-            "12523DF04B553A0B1AD74F42DDB741DE8DC06A03FC089A0EF197E"
+        char const sha[] = "12523DF04B553A0B1AD74F42DDB741DE8DC06A03FC089A0EF197E2A87F1D8107";
-            "2A87F1D8107"));
+        BEAST_EXPECT(expected == sha);
+        BEAST_EXPECT(jrr.isMember(expected));
         auto feature = *(jrr.begin());
 
-        BEAST_EXPECTS(feature[jss::name] == "fixAMMOverflowOffer", "name");
+        BEAST_EXPECTS(feature[jss::name] == name, "name");
         BEAST_EXPECTS(!feature[jss::enabled].asBool(), "enabled");
         BEAST_EXPECTS(feature[jss::vetoed].isBool() && !feature[jss::vetoed].asBool(), "vetoed");
         BEAST_EXPECTS(feature[jss::supported].asBool(), "supported");
@@ -186,6 +189,37 @@ class Feature_test : public beast::unit_test::suite
         jrr = env.rpc("feature", "fMM")[jss::result];
         BEAST_EXPECT(jrr[jss::error] == "badFeature");
         BEAST_EXPECT(jrr[jss::error_message] == "Feature unknown or invalid.");
+
+        // Test feature name size checks
+        constexpr auto ok63Name = [] {
+            return "123456789012345678901234567890123456789012345678901234567890123";
+        };
+        static_assert(validFeatureNameSize(ok63Name));
+
+        constexpr auto bad64Name = [] {
+            return "1234567890123456789012345678901234567890123456789012345678901234";
+        };
+        static_assert(!validFeatureNameSize(bad64Name));
+
+        constexpr auto ok31Name = [] { return "1234567890123456789012345678901"; };
+        static_assert(validFeatureNameSize(ok31Name));
+
+        constexpr auto bad32Name = [] { return "12345678901234567890123456789012"; };
+        static_assert(!validFeatureNameSize(bad32Name));
+
+        constexpr auto ok33Name = [] { return "123456789012345678901234567890123"; };
+        static_assert(validFeatureNameSize(ok33Name));
+
+        // Test feature character set checks
+        constexpr auto okName = [] { return "AMM_123"; };
+        static_assert(validFeatureName(okName));
+
+        // First character is Greek Capital Alpha, visually confusable with ASCII 'A'
+        constexpr auto badName = [] { return "ΑMM_123"; };
+        static_assert(!validFeatureName(badName));
+
+        constexpr auto badEmoji = [] { return "🔥"; };
+        static_assert(!validFeatureName(badEmoji));
     }
 
     void