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

* XRPLF/develop: (48 commits)
  test: Add null check unit test for `Oracle::aggregatePrice` (7306)
  ci: Patch conan recipe for Nix to be able to use on macOS (7532)
  ci: Run sanitizers on release builds too (7527)
  fix: Correct hybrid offer deletion on credential expiry (6843)
  ci: Make sanitizer flags lists in the profile, not a string (7449)
  ci: Make configurations launch on certain event types (7447)
  fix: Add [[maybe_unused]] to fix320Enabled for assert=OFF builds (7446)
  ci: Add `gh` and `file` to nix packages (7444)
  fix: Disable transaction invariants (7409)
  perf: Dispatch "hasInvalidAmount()" on type tag instead of dynamic_cast (7402)
  refactor: Retire fixUniversalNumber amendment (5962)
  test: Do not create data directory for memory databases (7323)
  ci: Launch upload-conan-deps on profile change (7442)
  fix: Fix Number comparison operator (7406)
  feat: Use C++ 23 standard (7431)
  refactor: Introduce XRPL_ASSERT_IF for amendment-gated assertions (7378)
  refactor: Change config section and key string literals into constants (7095)
  refactor: Use `std::move` and `std::string_view` where possible (7424)
  refactor: Use const function arguments where possible (7423)
  ci: Use XRPLF/actions build-multiarch-image workflow (7428)
  ...

.github/scripts/strategy-matrix/linux.json

@@ -10,7 +10,7 @@
 
       {
         "compiler": ["gcc", "clang"],
-        "build_type": ["Debug"],
+        "build_type": ["Debug", "Release"],
         "arch": ["amd64"],
         "sanitizers": ["address", "undefinedbehavior"]
       },

cspell.config.yaml

@@ -233,8 +233,10 @@ words:
   - pyenv
   - pyparsing
   - qalloc
+  - qbsprofile
   - queuable
   - Raphson
+  - rcflags
   - replayer
   - rerere
   - retriable

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_FIX    (Cleanup3_3_0,                Supported::Yes, VoteBehavior::DefaultNo)
 XRPL_FIX    (Cleanup3_2_0,                Supported::Yes, VoteBehavior::DefaultNo)
 XRPL_FEATURE(MPTokensV2,                  Supported::No,  VoteBehavior::DefaultNo)

nix/devshell.nix

@@ -1,6 +1,26 @@
 { pkgs, ... }:
 let
-  inherit (import ./packages.nix { inherit pkgs; }) commonPackages;
+  # conan is in the binary cache for Linux but not for Darwin, so on Darwin
+  # it is always built from source — and its bundled test suite is unreliable
+  # in the sandbox: `test_qbsprofile_rcflags` needs gcc (absent on Darwin, see
+  # https://github.com/NixOS/nixpkgs/pull/528995) and the patch tests are
+  # flaky from source. We only use conan as a build tool, so skip its tests on
+  # Darwin. Scoped to the dev shell (not the CI env, which builds conan on
+  # Linux from the cache). Drop once the fix reaches nixos-unstable and the
+  # lock is bumped.
+  pkgs_patched =
+    if pkgs.stdenv.isDarwin then
+      pkgs.extend (
+        final: prev: {
+          conan = prev.conan.overridePythonAttrs (_: {
+            doCheck = false;
+          });
+        }
+      )
+    else
+      pkgs;
+
+  inherit (import ./packages.nix { pkgs = pkgs_patched; }) commonPackages;
 
   # Supported compiler versions
   gccVersion = pkgs.lib.range 13 15;

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/test/rpc/GetAggregatePrice_test.cpp

@@ -3,12 +3,21 @@
 #include <test/jtx/Oracle.h>
 #include <test/jtx/amount.h>
 
+#include <xrpld/app/ledger/OpenLedger.h>
+
 #include <xrpl/basics/Number.h>
+#include <xrpl/basics/base_uint.h>
 #include <xrpl/beast/unit_test/suite.h>
+#include <xrpl/beast/utility/Journal.h>
+#include <xrpl/core/ServiceRegistry.h>
+#include <xrpl/ledger/OpenView.h>
 #include <xrpl/protocol/Feature.h>
+#include <xrpl/protocol/Indexes.h>
+#include <xrpl/protocol/SField.h>
 #include <xrpl/protocol/jss.h>
 
 #include <cstdlib>
+#include <memory>
 #include <optional>
 #include <string>
 #include <vector>
@@ -312,11 +321,91 @@ public:
         }
     }
 
+    void
+    testNullTxReadMeta()
+    {
+        testcase("Null txRead metadata");
+        using namespace jtx;
+
+        // Verify that iteratePriceData handles a null txRead result
+        // gracefully (returns early) rather than crashing with a
+        // nullptr dereference. This simulates local data corruption
+        // where a transaction referenced by sfPreviousTxnID is missing
+        // from the ledger's transaction map.
+        Env env(*this);
+        auto const baseFee = static_cast<int>(env.current()->fees().base.drops());
+
+        Account const owner{"owner"};
+        env.fund(XRP(1'000), owner);
+
+        // Create oracle with XRP/USD and XRP/EUR
+        Oracle oracle(
+            env,
+            {.owner = owner,
+             .series = {{"XRP", "USD", 740, 1}, {"XRP", "EUR", 840, 1}},
+             .fee = baseFee});
+
+        // Update oracle to only have XRP/EUR, pushing XRP/USD into
+        // history. iteratePriceData will need to read historical tx
+        // metadata to find the XRP/USD price.
+        oracle.set(UpdateArg{.series = {{"XRP", "EUR", 850, 1}}, .fee = baseFee});
+
+        OraclesData const oracles{{owner, oracle.documentID()}};
+
+        // Precondition: with an uncorrupted oracle, the historical
+        // traversal must succeed and produce a price for XRP/USD.
+        // This proves the test reaches iteratePriceData's history
+        // path; without it, a future change that breaks the setup
+        // could turn the post-corruption assertion into a vacuous
+        // pass (objectNotFound is reachable from many unrelated
+        // code paths).
+        {
+            auto const ret = Oracle::aggregatePrice(env, "XRP", "USD", oracles);
+            BEAST_EXPECT(!ret.isMember(jss::error));
+            BEAST_EXPECT(ret.isMember(jss::median));
+        }
+
+        // Simulate data corruption: modify the oracle SLE in the open
+        // ledger to have a bogus sfPreviousTxnID that doesn't exist in
+        // any ledger. sfPreviousTxnLgrSeq still points to a valid closed
+        // ledger, so getLedgerBySeq succeeds but txRead returns null.
+        auto const oracleKeylet = keylet::oracle(owner, oracle.documentID());
+        uint256 const bogusTxnID{0xABCABCAB};
+        bool const modified = env.app().getOpenLedger().modify(
+            [&oracleKeylet, &bogusTxnID](OpenView& view, beast::Journal) -> bool {
+                auto const sle = view.read(oracleKeylet);
+                if (!sle)
+                    return false;
+                auto replacement = std::make_shared<SLE>(*sle, sle->key());
+                replacement->setFieldH256(sfPreviousTxnID, bogusTxnID);
+                view.rawReplace(replacement);
+                return true;
+            });
+
+        // Confirm the injection actually took effect: modify must
+        // report success, and re-reading the SLE must show the
+        // bogus hash. Otherwise the failure-mode assertion below
+        // would not be exercising the null-txRead path at all.
+        BEAST_EXPECT(modified);
+        if (auto const sle = env.current()->read(oracleKeylet); BEAST_EXPECT(sle))
+            BEAST_EXPECT(sle->getFieldH256(sfPreviousTxnID) == bogusTxnID);
+
+        // Query for XRP/USD using the "current" (open) ledger.
+        // The oracle SLE now has a bogus sfPreviousTxnID. The current
+        // oracle only has EUR, so iteratePriceData will try to read
+        // history. txRead returns null for the bogus hash, and the
+        // null check should cause a graceful early return instead of
+        // a nullptr dereference.
+        auto const ret = Oracle::aggregatePrice(env, "XRP", "USD", oracles);
+        BEAST_EXPECT(ret[jss::error].asString() == "objectNotFound");
+    }
+
     void
     run() override
     {
         testErrors();
         testRpc();
+        testNullTxReadMeta();
     }
 };