Fix CheckCash hard caps MPT DeliverMin at half of maxMPTokenAmount

Signed-off-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com>

.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

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/tx/transactors/check/CheckCash.cpp

@@ -16,8 +16,9 @@
 #include <xrpl/protocol/Issue.h>
 #include <xrpl/protocol/Keylet.h>
 #include <xrpl/protocol/LedgerFormats.h>
+#include <xrpl/protocol/MPTAmount.h>
 #include <xrpl/protocol/MPTIssue.h>
-#include <xrpl/protocol/Protocol.h>
+#include <xrpl/protocol/Quality.h>
 #include <xrpl/protocol/SField.h>
 #include <xrpl/protocol/STAmount.h>
 #include <xrpl/protocol/STLedgerEntry.h>
@@ -368,18 +369,29 @@ CheckCash::doApply()
         else
         {
             // Note that for DeliverMin we don't know exactly how much
-            // currency we want flow to deliver.  We can't ask for the
-            // maximum possible currency because there might be a gateway
-            // transfer rate to account for.  Since the transfer rate cannot
-            // exceed 200%, we use 1/2 maxValue as our limit.
+            // currency we want flow to deliver.  For IOUs, use a value
+            // higher than any real delivery as the request. MPTs are
+            // bounded integral amounts, so use the maximum output the check
+            // can actually deliver without exceeding SendMax.
             auto const maxDeliverMin = [&]() {
                 return optDeliverMin->asset().visit(
                     [&](Issue const&) {
                         return STAmount(
                             optDeliverMin->asset(), STAmount::kMaxValue / 2, STAmount::kMaxOffset);
                     },
-                    [&](MPTIssue const&) {
-                        return STAmount(optDeliverMin->asset(), kMaxMpTokenAmount / 2);
+                    [&](MPTIssue const& issue) {
+                        MPTAmount maxDeliver = sendMax.mpt();
+                        auto const& issuer = issue.getIssuer();
+                        if (srcId != issuer && accountID_ != issuer)
+                        {
+                            auto const rate = transferRate(psb, issue.getMptID());
+                            // Request at most floor(SendMax / rate). The endpoint reverse pass
+                            // will quote ceil(output * rate), so this keeps the input
+                            // representable and within SendMax.
+                            maxDeliver =
+                                mulRatio(maxDeliver, QUALITY_ONE, rate.value, /*roundUp*/ false);
+                        }
+                        return STAmount(maxDeliver, issue);
                     });
             };
             STAmount const flowDeliver{

src/test/app/CheckMPT_test.cpp

@@ -31,6 +31,7 @@
 #include <xrpl/protocol/Issue.h>
 #include <xrpl/protocol/KeyType.h>
 #include <xrpl/protocol/LedgerFormats.h>
+#include <xrpl/protocol/Protocol.h>
 #include <xrpl/protocol/SField.h>
 #include <xrpl/protocol/STAmount.h>
 #include <xrpl/protocol/TER.h>
@@ -653,6 +654,32 @@ class CheckMPT_test : public beast::unit_test::Suite
             BEAST_EXPECT(ownerCount(env, alice) == 1);
             BEAST_EXPECT(ownerCount(env, bob) == 1);
         }
+
+        {
+            Env env{*this, features};
+
+            env.fund(XRP(1'000), gw, alice, bob);
+
+            // MPT DeliverMin should not be capped at half of the legal range.
+            std::uint64_t constexpr deliverMin = (kMaxMpTokenAmount / 2) + 1;
+            MPT const usd = MPTTester(
+                {.env = env, .issuer = gw, .holders = {alice, bob}, .maxAmt = kMaxMpTokenAmount});
+
+            env(pay(gw, alice, usd(deliverMin)));
+            env.close();
+
+            uint256 const chkId{getCheckIndex(alice, env.seq(alice))};
+            env(check::create(alice, bob, usd(deliverMin)));
+            env.close();
+
+            env(check::cash(bob, chkId, check::DeliverMin(usd(deliverMin))));
+            verifyDeliveredAmount(env, usd(deliverMin));
+            env.require(Balance(alice, usd(0)));
+            env.require(Balance(bob, usd(deliverMin)));
+            BEAST_EXPECT(checksOnAccount(env, alice).empty());
+            BEAST_EXPECT(checksOnAccount(env, bob).empty());
+        }
+
         {
             // Examine the effects of the asfRequireAuth flag.
             Env env(*this, features);
@@ -831,6 +858,32 @@ class CheckMPT_test : public beast::unit_test::Suite
         BEAST_EXPECT(checksOnAccount(env, alice).size() == 0);
         BEAST_EXPECT(checksOnAccount(env, bob).size() == 0);
 #endif
+
+        // With the maximum transfer fee, this is the largest output whose
+        // fee-adjusted debit is still within SendMax.
+        std::uint64_t constexpr maxDeliver = (kMaxMpTokenAmount / 3) * 2;
+        MPT const eur = MPTTester(
+            {.env = env,
+             .issuer = gw,
+             .holders = {alice, bob},
+             .transferFee = kMaxTransferFee,
+             .maxAmt = kMaxMpTokenAmount});
+
+        env(pay(gw, alice, eur(kMaxMpTokenAmount)));
+        env.close();
+
+        uint256 const chkIdMax{getCheckIndex(alice, env.seq(alice))};
+        env(check::create(alice, bob, eur(kMaxMpTokenAmount)));
+        env.close();
+
+        // The DeliverMin cap must divide SendMax by the rate before flow()
+        // computes the fee-adjusted input.
+        env(check::cash(bob, chkIdMax, check::DeliverMin(eur(maxDeliver))));
+        verifyDeliveredAmount(env, eur(maxDeliver));
+        env.require(Balance(alice, eur(1)));
+        env.require(Balance(bob, eur(maxDeliver)));
+        BEAST_EXPECT(checksOnAccount(env, alice).empty());
+        BEAST_EXPECT(checksOnAccount(env, bob).empty());
     }
 
     void

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();
     }
 };