Merge remote-tracking branch 'XRPLF/develop' into ximinez/directory

* XRPLF/develop:
  chore: Enable most cppcoreguidelines checks (7660)
  test: Migrate basics Beast tests to GTest (7136)
  test: Migrate resource, shamap Beast tests to GTest (7133)
  chore: Enable most misc checks (7663)
  chore: Fix unity build (7730)
  chore: Enable most modernize checks (7664)
  chore: Delete dead code (7718)
  chore: Enable modernize-use-constraints (7715)
  chore: Enable modernize-avoid-bind (7711)
  chore: Enable modernize-use-auto (7707)
  build: Add protobuf dependencies to Nix (7706)
  feat: Enable ConfidentialTransfer and BatchV1_1 (7698)
  chore: Enable modernize-unary-static-assert (7705)
  chore: Make clang-tidy happy on macOS (7701)
  chore: Improve pre-commit hooks (7702)
  refactor: Retire DisallowIncomingV1 fix (7364)
  build: Add verify-headers target to cleanup headers (7670)

.clang-tidy

@@ -50,7 +50,13 @@ Checks: "-*,
   performance-*,
   -performance-avoid-endl,
   -performance-enum-size,
+  -performance-inefficient-algorithm,
+  -performance-inefficient-string-concatenation,
+  -performance-no-int-to-ptr,
+  -performance-noexcept-destructor,
   -performance-noexcept-move-constructor,
+  -performance-noexcept-swap,
+  -performance-type-promotion-in-math-fn,
   -performance-unnecessary-copy-initialization,
   -performance-unnecessary-value-param,
 

include/xrpl/basics/IntrusivePointer.ipp

@@ -641,9 +641,6 @@ template <class T>
 T*
 SharedWeakUnion<T>::unsafeGetRawPtr() const
 {
-    // tp_ packs a raw pointer together with a strength bit; recovering the
-    // pointer inherently requires an integer-to-pointer cast.
-    // NOLINTNEXTLINE(performance-no-int-to-ptr)
     return reinterpret_cast<T*>(tp_ & kPtrMask);
 }
 

include/xrpl/protocol/detail/features.macro

@@ -15,6 +15,7 @@
 // Add new amendments to the top of this list.
 // Keep it sorted in reverse chronological order.
 
+XRPL_FEATURE(DefragDirectories,           Supported::No,  VoteBehavior::DefaultNo)
 XRPL_FEATURE(BatchV1_1,                   Supported::Yes, VoteBehavior::DefaultNo)
 XRPL_FEATURE(LendingProtocolV1_1,         Supported::No,  VoteBehavior::DefaultNo)
 XRPL_FEATURE(ConfidentialTransfer,        Supported::Yes, VoteBehavior::DefaultNo)

include/xrpl/shamap/detail/TaggedPointer.ipp

@@ -507,9 +507,6 @@ TaggedPointer::operator=(TaggedPointer&& other)
 [[nodiscard]] inline std::pair<std::uint8_t, void*>
 TaggedPointer::decode() const
 {
-    // tp_ packs a raw pointer together with the tag bits; recovering the
-    // pointer inherently requires an integer-to-pointer cast.
-    // NOLINTNEXTLINE(performance-no-int-to-ptr)
     return {tp_ & kTagMask, reinterpret_cast<void*>(tp_ & kPtrMask)};
 }
 
@@ -538,9 +535,6 @@ TaggedPointer::getHashesAndChildren() const
 [[nodiscard]] inline SHAMapHash*
 TaggedPointer::getHashes() const
 {
-    // tp_ packs a raw pointer together with the tag bits; recovering the
-    // pointer inherently requires an integer-to-pointer cast.
-    // NOLINTNEXTLINE(performance-no-int-to-ptr)
     return reinterpret_cast<SHAMapHash*>(tp_ & kPtrMask);
 };
 

src/libxrpl/json/json_reader.cpp

@@ -5,13 +5,12 @@
 
 #include <algorithm>
 #include <cctype>
-#include <charconv>
 #include <cstdint>
 #include <cstdio>
+#include <cstring>
 #include <istream>
 #include <stdexcept>
 #include <string>
-#include <system_error>
 
 namespace json {
 // Implementation of class Reader
@@ -606,17 +605,36 @@ bool
 Reader::decodeDouble(Token& token)
 {
     double value = 0;
-    auto const [ptr, ec] = std::from_chars(token.start, token.end, value);
-
-    // Reject anything from_chars could not turn into a finite double:
-    //   - ec != std::errc{}: no valid conversion, or an out-of-range magnitude
-    //     (e.g. 1e400).
-    //   - ptr != token.end: readNumber() is permissive about which characters
-    //     it collects into a token (it will, for example, keep a '+' mid-token),
-    //     but from_chars() will stop at the first character it cannot parse.
-    if (ec != std::errc{} || ptr != token.end)
+    int const bufferSize = 32;
+    int count = 0;
+    int const length = int(token.end - token.start);
+    // Sanity check to avoid buffer overflow exploits.
+    if (length < 0)
+    {
+        return addError("Unable to parse token length", token);
+    }
+    // Avoid using a string constant for the format control string given to
+    // sscanf, as this can cause hard to debug crashes on OS X. See here for
+    // more info:
+    //
+    // http://developer.apple.com/library/mac/#DOCUMENTATION/DeveloperTools/gcc-4.0.1/gcc/Incompatibilities.html
+    char format[] = "%lf";
+    if (length <= bufferSize)
+    {
+        Char buffer[bufferSize + 1];
+        memcpy(buffer, token.start, length);
+        buffer[length] = 0;
+        // NOLINTNEXTLINE(cppcoreguidelines-pro-type-vararg)
+        count = sscanf(buffer, format, &value);
+    }
+    else
+    {
+        std::string const buffer(token.start, token.end);
+        // NOLINTNEXTLINE(cppcoreguidelines-pro-type-vararg)
+        count = sscanf(buffer.c_str(), format, &value);
+    }
+    if (count != 1)
         return addError("'" + std::string(token.start, token.end) + "' is not a number.", token);
-
     currentValue() = value;
     return true;
 }

src/libxrpl/json/json_writer.cpp

@@ -4,14 +4,13 @@
 #include <xrpl/json/json_forwards.h>
 #include <xrpl/json/json_value.h>
 
-#include <charconv>
+#include <cstdio>
 #include <cstring>
 #include <iomanip>
 #include <ios>
 #include <ostream>
 #include <sstream>
 #include <string>
-#include <system_error>
 #include <utility>
 
 namespace json {
@@ -77,18 +76,20 @@ valueToString(UInt value)
 std::string
 valueToString(double value)
 {
-    // Format with 16 significant digits.
-    // We need not request the alternative representation that always has a
-    // decimal point because JSON doesn't distinguish the concepts of reals and integers.
-    // A double never needs more than 32 characters in this form,
-    // so to_chars cannot actually run out of room here.
+    // Allocate a buffer that is more than large enough to store the 16 digits
+    // of precision requested below.
     char buffer[32];
-    auto const [ptr, ec] =
-        std::to_chars(buffer, buffer + sizeof(buffer), value, std::chars_format::general, 16);
-    XRPL_ASSERT(ec == std::errc{}, "json::valueToString(double) : conversion fits buffer");
-    if (ec != std::errc{})
-        return {};
-    return std::string(buffer, ptr);
+    // Print into the buffer. We need not request the alternative representation
+    // that always has a decimal point because JSON doesn't distinguish the
+    // concepts of reals and integers.
+#if defined(_MSC_VER) && defined(__STDC_SECURE_LIB__)  // Use secure version with visual studio 2005
+                                                       // to avoid warning.
+    sprintf_s(buffer, sizeof(buffer), "%.16g", value);
+#else
+    // NOLINTNEXTLINE(cppcoreguidelines-pro-type-vararg)
+    snprintf(buffer, sizeof(buffer), "%.16g", value);
+#endif
+    return buffer;
 }
 
 std::string

src/libxrpl/ledger/ApplyView.cpp

@@ -26,6 +26,14 @@ namespace xrpl {
 
 namespace directory {
 
+struct Gap
+{
+    uint64_t const page;
+    SLE::pointer node;
+    uint64_t const nextPage;
+    SLE::pointer next;
+};
+
 std::uint64_t
 createRoot(
     ApplyView& view,
@@ -126,7 +134,9 @@ insertPage(
     if (page == 0)
         return std::nullopt;
     if (!view.rules().enabled(fixDirectoryLimit) && page >= kDirNodeMaxPages)  // Old pages limit
+    {
         return std::nullopt;
+    }
 
     // We are about to create a new node; we'll link it to
     // the chain first:
@@ -147,12 +157,8 @@ insertPage(
     // Save some space by not specifying the value 0 since it's the default.
     if (page != 1)
         node->setFieldU64(sfIndexPrevious, page - 1);
-    XRPL_ASSERT_PARTS(!nextPage, "xrpl::directory::insertPage", "nextPage has default value");
-    /* Reserved for future use when directory pages may be inserted in
-     * between two other pages instead of only at the end of the chain.
     if (nextPage)
         node->setFieldU64(sfIndexNext, nextPage);
-    */
     describe(node);
     view.insert(node);
 
@@ -168,7 +174,7 @@ ApplyView::dirAdd(
     uint256 const& key,
     std::function<void(SLE::ref)> const& describe)
 {
-    auto root = peek(directory);
+    auto const root = peek(directory);
 
     if (!root)
     {
@@ -178,6 +184,43 @@ ApplyView::dirAdd(
 
     auto [page, node, indexes] = directory::findPreviousPage(*this, directory, root);
 
+    if (rules().enabled(featureDefragDirectories))
+    {
+        // If there are more nodes than just the root, and there's no space in
+        // the last one, walk backwards to find one with space, or to find one
+        // missing.
+        std::optional<directory::Gap> gapPages;
+        while (page && indexes.size() >= kDIR_NODE_MAX_PAGES)
+        {
+            // Find a page with space, or a gap in pages.
+            auto [prevPage, prevNode, prevIndexes] =
+                directory::findPreviousPage(*this, directory, node);
+            if (!gapPages && prevPage != page - 1)
+                gapPages.emplace(prevPage, prevNode, page, node);
+            page = prevPage;
+            node = prevNode;
+            indexes = prevIndexes;
+        }
+        // We looped through all the pages back to the root.
+        if (!page)
+        {
+            // If we found a gap, use it.
+            if (gapPages)
+            {
+                return directory::insertPage(
+                    *this,
+                    gapPages->page,
+                    gapPages->node,
+                    gapPages->nextPage,
+                    gapPages->next,
+                    key,
+                    directory,
+                    describe);
+            }
+            std::tie(page, node, indexes) = directory::findPreviousPage(*this, directory, root);
+        }
+    }
+
     // If there's space, we use it:
     if (indexes.size() < kDirNodeMaxEntries)
     {

src/libxrpl/protocol/STParsedJSON.cpp

@@ -69,17 +69,6 @@ toUnsigned(U2 value)
     return static_cast<U1>(value);
 }
 
-static std::string
-joinName(std::string const& jsonName, std::string const& fieldName)
-{
-    std::string result;
-    result.reserve(jsonName.size() + 1 + fieldName.size());
-    result += jsonName;
-    result += '.';
-    result += fieldName;
-    return result;
-}
-
 // LCOV_EXCL_START
 static inline std::string
 makeName(std::string const& object, std::string const& field)
@@ -87,7 +76,7 @@ makeName(std::string const& object, std::string const& field)
     if (field.empty())
         return object;
 
-    return joinName(object, field);
+    return object + "." + field;
 }
 
 static inline json::Value
@@ -1047,8 +1036,8 @@ parseObject(
 
                     try
                     {
-                        auto ret = parseObject(
-                            joinName(jsonName, fieldName), value, field, depth + 1, error);
+                        auto ret =
+                            parseObject(jsonName + "." + fieldName, value, field, depth + 1, error);
                         if (!ret)
                             return std::nullopt;
                         data.emplaceBack(std::move(*ret));
@@ -1065,8 +1054,8 @@ parseObject(
                 case STI_ARRAY:
                     try
                     {
-                        auto array = parseArray(
-                            joinName(jsonName, fieldName), value, field, depth + 1, error);
+                        auto array =
+                            parseArray(jsonName + "." + fieldName, value, field, depth + 1, error);
                         if (!array.has_value())
                             return std::nullopt;
                         data.emplaceBack(std::move(*array));

src/libxrpl/rdb/SociDB.cpp

@@ -213,12 +213,6 @@ public:
         if (auto [conn, keepAlive] = getConnection(); conn)
         {
             (void)keepAlive;
-            // The checkpointer is identified to the C callback by an integer id
-            // (resolved via checkpointerFromId) rather than a raw `this`, so it
-            // cannot dangle if the checkpointer is destroyed. Passing the id
-            // through sqlite's void* user-data requires an integer-to-pointer
-            // cast.
-            // NOLINTNEXTLINE(performance-no-int-to-ptr)
             sqlite_api::sqlite3_wal_hook(conn, &sqliteWALHook, reinterpret_cast<void*>(id_));
         }
     }

src/test/app/Batch_test.cpp

@@ -498,7 +498,7 @@ class Batch_test : public beast::unit_test::Suite
             auto const batchFee = batch::calcBatchFee(env, 0, 2);
             auto tx1 = batch::Inner(pay(alice, bob, XRP(1)), seq + 1);
             tx1[jss::Fee] = "1.5";
-            auto const g = env.getParseFailureGuard(true);
+            env.setParseFailureExpected(true);
             try
             {
                 env(batch::outer(alice, seq, batchFee, tfAllOrNothing),
@@ -510,6 +510,7 @@ class Batch_test : public beast::unit_test::Suite
             {
                 BEAST_EXPECT(true);
             }
+            env.setParseFailureExpected(false);
         }
 
         // temSEQ_AND_TICKET: Batch: inner txn cannot have both Sequence

src/test/app/Vault_test.cpp

@@ -5451,9 +5451,9 @@ class Vault_test : public beast::unit_test::Suite
             env.close();
 
             // 2. Mantissa larger than uint64 max
+            env.setParseFailureExpected(true);
             try
             {
-                auto const g = env.getParseFailureGuard(true);
                 tx[sfAssetsMaximum] = "18446744073709551617e5";  // uint64 max + 1
                 env(tx);
                 BEAST_EXPECTS(false, "Expected parse_error for mantissa larger than uint64 max");
@@ -5464,6 +5464,7 @@ class Vault_test : public beast::unit_test::Suite
                 BEAST_EXPECT(
                     e.what() == "invalidParamsField 'tx_json.AssetsMaximum' has invalid data."s);
             }
+            env.setParseFailureExpected(false);
         }
     }
 

src/test/core/Config_test.cpp

@@ -1475,7 +1475,6 @@ r.ripple.com:51235
             std::string toLoad(R"xrpldConfig(
 [amendment_majority_time]
 )xrpldConfig");
-            // NOLINTNEXTLINE(performance-inefficient-string-concatenation)
             toLoad += std::to_string(val) + space + unit;
             space = space.empty() ? " " : "";
 

src/test/jtx/Env.h

@@ -495,46 +495,6 @@ public:
         parseFailureExpected_ = b;
     }
 
-    /** RAII class to set and restore the parse failure flag (setParseFailureExpected).
-     *
-     * Can be created directly, or through the `getParseFailureGuard(bool)` function.
-     */
-    class ParseFailureGuard final
-    {
-        Env& self_;
-        bool oldExpected_ = self_.parseFailureExpected_;
-
-    public:
-        ParseFailureGuard(Env& self, bool b) : self_(self)
-        {
-            self_.setParseFailureExpected(b);
-        }
-
-        ~ParseFailureGuard()
-        {
-            self_.setParseFailureExpected(oldExpected_);
-        }
-
-        // No copy, no move
-        ParseFailureGuard(ParseFailureGuard const&) = delete;
-        ParseFailureGuard&
-        operator=(ParseFailureGuard const&) = delete;
-        ParseFailureGuard(ParseFailureGuard&& other) = delete;
-        ParseFailureGuard&
-        operator=(ParseFailureGuard&&) = delete;
-    };
-
-    /** Gets an RAII guard to set and restore the parse failure flag
-     *
-     * Usage:
-     * auto const guard = env.getParseFailureGuard(true/false);
-     */
-    [[nodiscard]] ParseFailureGuard
-    getParseFailureGuard(bool b)
-    {
-        return ParseFailureGuard{*this, b};
-    }
-
     /** Turn off signature checks. */
     void
     disableSigs()

src/test/jtx/impl/Env.cpp

@@ -602,7 +602,7 @@ Env::autofill(JTx& jt)
     catch (ParseError const&)
     {
         if (!parseFailureExpected_)
-            test.log << "parse failure:\n" << pretty(jv) << std::endl;
+            test.log << "parse failed:\n" << pretty(jv) << std::endl;
         rethrow();
     }
 }

src/tests/libxrpl/json/Value.cpp

@@ -13,7 +13,6 @@
 #include <exception>
 #include <limits>
 #include <numbers>
-#include <optional>
 #include <regex>
 #include <sstream>
 #include <string>
@@ -593,57 +592,6 @@ TEST(json_value, bad_json)
     EXPECT_TRUE(r.parse(s, j));
 }
 
-namespace {
-
-std::optional<json::Value>
-parseValue(std::string const& doc)
-{
-    json::Value j;
-    json::Reader r;
-    if (!r.parse("{\"v\":" + doc + "}", j))
-        return std::nullopt;
-    return j["v"];
-}
-
-}  // namespace
-
-TEST(json_value, parse_double_valid)
-{
-    // 1e300 is large but still representable, so it parses (unlike the out-of-range cases below).
-    for (auto const& [text, expected] :
-         {std::pair{"2.5", 2.5},
-          std::pair{"-3.25e2", -325.0},
-          std::pair{"0.0", 0.0},
-          std::pair{"1E3", 1000.0},
-          std::pair{"-0.5e-1", -0.05},
-          std::pair{"1e300", 1e300}})
-    {
-        auto const v = parseValue(text);
-        ASSERT_TRUE(v.has_value()) << text;
-        // NOLINTBEGIN(bugprone-unchecked-optional-access)
-        EXPECT_TRUE(v->isDouble()) << text;
-        EXPECT_EQ(v->asDouble(), expected) << text;
-        // NOLINTEND(bugprone-unchecked-optional-access)
-    }
-}
-
-TEST(json_value, parse_double_out_of_range)
-{
-    // Magnitudes with no finite double representation are rejected.
-    for (char const* oor : {"1e400", "-1e400", "0.001e500", "1e-400", "-1e-400", "123e-500"})
-        EXPECT_FALSE(parseValue(oor).has_value()) << oor;
-}
-
-TEST(json_value, parse_double_malformed)
-{
-    // readNumber() collects any run of digits and '.eE+-' into a single Double
-    // token, so these malformed tokens reach decodeDouble. Each has a valid
-    // leading prefix that from_chars would accept on its own; requiring the
-    // entire token be consumed rejects them instead of silently truncating.
-    for (char const* bad : {"1+2", "1-2", "1.2.3", "1e5e6", "1..2", "++5", "1e", "1e+", ".", "-"})
-        EXPECT_FALSE(parseValue(bad).has_value()) << bad;
-}
-
 TEST(json_value, edge_cases)
 {
     std::uint32_t const maxUInt = std::numeric_limits<std::uint32_t>::max();