fixReducedOffersV2: prevent offers from blocking order books: (#5032)

Fixes issue #4937.

The fixReducedOffersV1 amendment fixed certain forms of offer
modification that could lead to blocked order books.  Reduced
offers can block order books if the effective quality of the
reduced offer is worse than the quality of the original offer
(from the perspective of the taker). It turns out that, for
small values, the quality of the reduced offer can be
significantly affected by the rounding mode used during
scaling computations.

Issue #4937 identified an additional code path that modified
offers in a way that could lead to blocked order books.  This
commit changes the rounding in that newly located code path so
the quality of the modified offer is never worse than the
quality of the offer as it was originally placed.

It is possible that additional ways of producing blocking
offers will come to light.  Therefore there may be a future
need for a V3 amendment.

src/ripple/app/paths/AMMOffer.h

@@ -103,7 +103,6 @@ public:
     limitOut(
         TAmounts<TIn, TOut> const& offrAmt,
         TOut const& limit,
-        bool fixReducedOffers,
         bool roundUp) const;
 
     /** Limit in of the provided offer. If one-path then swapIn
@@ -111,7 +110,8 @@ public:
      * current quality.
      */
     TAmounts<TIn, TOut>
-    limitIn(TAmounts<TIn, TOut> const& offrAmt, TIn const& limit) const;
+    limitIn(TAmounts<TIn, TOut> const& offrAmt, TIn const& limit, bool roundUp)
+        const;
 
     QualityFunction
     getQualityFunc() const;

src/ripple/app/paths/impl/AMMOffer.cpp

@@ -81,7 +81,6 @@ TAmounts<TIn, TOut>
 AMMOffer<TIn, TOut>::limitOut(
     TAmounts<TIn, TOut> const& offrAmt,
     TOut const& limit,
-    bool fixReducedOffers,
     bool roundUp) const
 {
     // Change the offer size proportionally to the original offer quality
@@ -92,7 +91,8 @@ AMMOffer<TIn, TOut>::limitOut(
     // poolPays * poolGets < (poolPays - assetOut) * (poolGets + assetIn)
     if (ammLiquidity_.multiPath())
     {
-        if (fixReducedOffers)
+        if (auto const& rules = getCurrentTransactionRules();
+            rules && rules->enabled(fixReducedOffersV1))
             // It turns out that the ceil_out implementation has some slop in
             // it.  ceil_out_strict removes that slop.  But removing that slop
             // affects transaction outcomes, so the change must be made using
@@ -110,11 +110,18 @@ template <typename TIn, typename TOut>
 TAmounts<TIn, TOut>
 AMMOffer<TIn, TOut>::limitIn(
     TAmounts<TIn, TOut> const& offrAmt,
-    TIn const& limit) const
+    TIn const& limit,
+    bool roundUp) const
 {
     // See the comments above in limitOut().
     if (ammLiquidity_.multiPath())
+    {
+        if (auto const& rules = getCurrentTransactionRules();
+            rules && rules->enabled(fixReducedOffersV2))
+            return quality().ceil_in_strict(offrAmt, limit, roundUp);
+
         return quality().ceil_in(offrAmt, limit);
+    }
     return {limit, swapAssetIn(balances_, limit, ammLiquidity_.tradingFee())};
 }
 

src/ripple/app/paths/impl/BookStep.cpp

@@ -668,7 +668,14 @@ limitStepIn(
         stpAmt.in = limit;
         auto const inLmt =
             mulRatio(stpAmt.in, QUALITY_ONE, transferRateIn, /*roundUp*/ false);
-        ofrAmt = offer.limitIn(ofrAmt, inLmt);
+        // It turns out we can prevent order book blocking by (strictly)
+        // rounding down the ceil_in() result.  By rounding down we guarantee
+        // that the quality of an offer left in the ledger is as good or
+        // better than the quality of the containing order book page.
+        //
+        // This adjustment changes transaction outcomes, so it must be made
+        // under an amendment.
+        ofrAmt = offer.limitIn(ofrAmt, inLmt, /* roundUp */ false);
         stpAmt.out = ofrAmt.out;
         ownerGives = mulRatio(
             ofrAmt.out, transferRateOut, QUALITY_ONE, /*roundUp*/ false);
@@ -685,8 +692,7 @@ limitStepOut(
     TOut& ownerGives,
     std::uint32_t transferRateIn,
     std::uint32_t transferRateOut,
-    TOut const& limit,
+    TOut const& limit)
-    Rules const& rules)
 {
     if (limit < stpAmt.out)
     {
@@ -696,7 +702,6 @@ limitStepOut(
         ofrAmt = offer.limitOut(
             ofrAmt,
             stpAmt.out,
-            rules.enabled(fixReducedOffersV1),
             /*roundUp*/ true);
         stpAmt.in =
             mulRatio(ofrAmt.in, transferRateIn, QUALITY_ONE, /*roundUp*/ true);
@@ -736,7 +741,6 @@ BookStep<TIn, TOut, TDerived>::forEachOffer(
         sb, afView, book_, sb.parentCloseTime(), counter, j_);
 
     bool const flowCross = afView.rules().enabled(featureFlowCross);
-    bool const fixReduced = afView.rules().enabled(fixReducedOffersV1);
     bool offerAttempted = false;
     std::optional<Quality> ofrQ;
     auto execOffer = [&](auto& offer) {
@@ -817,8 +821,7 @@ BookStep<TIn, TOut, TDerived>::forEachOffer(
             // It turns out we can prevent order book blocking by (strictly)
             // rounding down the ceil_out() result.  This adjustment changes
             // transaction outcomes, so it must be made under an amendment.
-            ofrAmt = offer.limitOut(
+            ofrAmt = offer.limitOut(ofrAmt, stpAmt.out, /*roundUp*/ false);
-                ofrAmt, stpAmt.out, fixReduced, /*roundUp*/ false);
 
             stpAmt.in =
                 mulRatio(ofrAmt.in, ofrInRate, QUALITY_ONE, /*roundUp*/ true);
@@ -1055,8 +1058,7 @@ BookStep<TIn, TOut, TDerived>::revImp(
                 ownerGivesAdj,
                 transferRateIn,
                 transferRateOut,
-                remainingOut,
+                remainingOut);
-                afView.rules());
             remainingOut = beast::zero;
             savedIns.insert(stpAdjAmt.in);
             savedOuts.insert(remainingOut);
@@ -1208,8 +1210,7 @@ BookStep<TIn, TOut, TDerived>::fwdImp(
                 ownerGivesAdjRev,
                 transferRateIn,
                 transferRateOut,
-                remainingOut,
+                remainingOut);
-                afView.rules());
 
             if (stpAdjAmtRev.in == remainingIn)
             {
@@ -1228,7 +1229,7 @@ BookStep<TIn, TOut, TDerived>::fwdImp(
             }
             else
             {
-                // This is (likely) a problem case, and wil be caught
+                // This is (likely) a problem case, and will be caught
                 // with later checks
                 savedOuts.insert(lastOutAmt);
             }

src/ripple/app/tx/impl/Offer.h

@@ -23,6 +23,7 @@
 #include <ripple/basics/contract.h>
 #include <ripple/ledger/View.h>
 #include <ripple/protocol/Quality.h>
+#include <ripple/protocol/Rules.h>
 #include <ripple/protocol/SField.h>
 #include <ripple/protocol/STLedgerEntry.h>
 #include <ostream>
@@ -140,11 +141,11 @@ public:
     limitOut(
         TAmounts<TIn, TOut> const& offrAmt,
         TOut const& limit,
-        bool fixReducedOffers,
         bool roundUp) const;
 
     TAmounts<TIn, TOut>
-    limitIn(TAmounts<TIn, TOut> const& offrAmt, TIn const& limit) const;
+    limitIn(TAmounts<TIn, TOut> const& offrAmt, TIn const& limit, bool roundUp)
+        const;
 
     template <typename... Args>
     static TER
@@ -219,10 +220,10 @@ TAmounts<TIn, TOut>
 TOffer<TIn, TOut>::limitOut(
     TAmounts<TIn, TOut> const& offrAmt,
     TOut const& limit,
-    bool fixReducedOffers,
     bool roundUp) const
 {
-    if (fixReducedOffers)
+    if (auto const& rules = getCurrentTransactionRules();
+        rules && rules->enabled(fixReducedOffersV1))
         // It turns out that the ceil_out implementation has some slop in
         // it.  ceil_out_strict removes that slop.  But removing that slop
         // affects transaction outcomes, so the change must be made using
@@ -233,9 +234,18 @@ TOffer<TIn, TOut>::limitOut(
 
 template <class TIn, class TOut>
 TAmounts<TIn, TOut>
-TOffer<TIn, TOut>::limitIn(TAmounts<TIn, TOut> const& offrAmt, TIn const& limit)
+TOffer<TIn, TOut>::limitIn(
-    const
+    TAmounts<TIn, TOut> const& offrAmt,
+    TIn const& limit,
+    bool roundUp) const
 {
+    if (auto const& rules = getCurrentTransactionRules();
+        rules && rules->enabled(fixReducedOffersV2))
+        // It turns out that the ceil_in implementation has some slop in
+        // it.  ceil_in_strict removes that slop.  But removing that slop
+        // affects transaction outcomes, so the change must be made using
+        // an amendment.
+        return quality().ceil_in_strict(offrAmt, limit, roundUp);
     return m_quality.ceil_in(offrAmt, limit);
 }
 

src/ripple/protocol/Feature.h

@@ -74,7 +74,7 @@ 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 = 73;
+static constexpr std::size_t numFeatures = 74;
 
 /** Amendments that this server supports and the default voting behavior.
    Whether they are enabled depends on the Rules defined in the validated
@@ -360,6 +360,7 @@ extern uint256 const fixEmptyDID;
 extern uint256 const fixXChainRewardRounding;
 extern uint256 const fixPreviousTxnID;
 extern uint256 const fixAMMv1_1;
+extern uint256 const fixReducedOffersV2;
 
 }  // namespace ripple
 

src/ripple/protocol/Quality.h

@@ -181,71 +181,71 @@ public:
         Math is avoided if the result is exact. The output is clamped
         to prevent money creation.
     */
-    Amounts
+    [[nodiscard]] Amounts
     ceil_in(Amounts const& amount, STAmount const& limit) const;
 
     template <class In, class Out>
-    TAmounts<In, Out>
+    [[nodiscard]] TAmounts<In, Out>
-    ceil_in(TAmounts<In, Out> const& amount, In const& limit) const
+    ceil_in(TAmounts<In, Out> const& amount, In const& limit) const;
-    {
-        if (amount.in <= limit)
-            return amount;
 
-        // Use the existing STAmount implementation for now, but consider
+    // Some of the underlying rounding functions called by ceil_in() ignored
-        // replacing with code specific to IOUAMount and XRPAmount
+    // low order bits that could influence rounding decisions.  This "strict"
-        Amounts stAmt(toSTAmount(amount.in), toSTAmount(amount.out));
+    // method uses underlying functions that pay attention to all the bits.
-        STAmount stLim(toSTAmount(limit));
+    [[nodiscard]] Amounts
-        auto const stRes = ceil_in(stAmt, stLim);
+    ceil_in_strict(Amounts const& amount, STAmount const& limit, bool roundUp)
-        return TAmounts<In, Out>(
+        const;
-            toAmount<In>(stRes.in), toAmount<Out>(stRes.out));
+
-    }
+    template <class In, class Out>
+    [[nodiscard]] TAmounts<In, Out>
+    ceil_in_strict(
+        TAmounts<In, Out> const& amount,
+        In const& limit,
+        bool roundUp) const;
 
     /** Returns the scaled amount with out capped.
         Math is avoided if the result is exact. The input is clamped
         to prevent money creation.
     */
-    Amounts
+    [[nodiscard]] Amounts
     ceil_out(Amounts const& amount, STAmount const& limit) const;
 
     template <class In, class Out>
-    TAmounts<In, Out>
+    [[nodiscard]] TAmounts<In, Out>
-    ceil_out(TAmounts<In, Out> const& amount, Out const& limit) const
+    ceil_out(TAmounts<In, Out> const& amount, Out const& limit) const;
-    {
-        if (amount.out <= limit)
-            return amount;
 
-        // Use the existing STAmount implementation for now, but consider
+    // Some of the underlying rounding functions called by ceil_out() ignored
-        // replacing with code specific to IOUAMount and XRPAmount
+    // low order bits that could influence rounding decisions.  This "strict"
-        Amounts stAmt(toSTAmount(amount.in), toSTAmount(amount.out));
+    // method uses underlying functions that pay attention to all the bits.
-        STAmount stLim(toSTAmount(limit));
+    [[nodiscard]] Amounts
-        auto const stRes = ceil_out(stAmt, stLim);
-        return TAmounts<In, Out>(
-            toAmount<In>(stRes.in), toAmount<Out>(stRes.out));
-    }
-
-    Amounts
     ceil_out_strict(Amounts const& amount, STAmount const& limit, bool roundUp)
         const;
 
     template <class In, class Out>
-    TAmounts<In, Out>
+    [[nodiscard]] TAmounts<In, Out>
     ceil_out_strict(
         TAmounts<In, Out> const& amount,
         Out const& limit,
-        bool roundUp) const
+        bool roundUp) const;
-    {
-        if (amount.out <= limit)
-            return amount;
 
-        // Use the existing STAmount implementation for now, but consider
+private:
-        // replacing with code specific to IOUAMount and XRPAmount
+    // The ceil_in and ceil_out methods that deal in TAmount all convert
-        Amounts stAmt(toSTAmount(amount.in), toSTAmount(amount.out));
+    // their arguments to STAoumout and convert the result back to TAmount.
-        STAmount stLim(toSTAmount(limit));
+    // This helper function takes care of all the conversion operations.
-        auto const stRes = ceil_out_strict(stAmt, stLim, roundUp);
+    template <
-        return TAmounts<In, Out>(
+        class In,
-            toAmount<In>(stRes.in), toAmount<Out>(stRes.out));
+        class Out,
-    }
+        class Lim,
+        typename FnPtr,
+        std::same_as<bool>... Round>
+    [[nodiscard]] TAmounts<In, Out>
+    ceil_TAmounts_helper(
+        TAmounts<In, Out> const& amount,
+        Lim const& limit,
+        Lim const& limit_cmp,
+        FnPtr ceil_function,
+        Round... round) const;
 
+public:
     /** Returns `true` if lhs is lower quality than `rhs`.
         Lower quality means the taker receives a worse deal.
         Higher quality is better for the taker.
@@ -327,6 +327,84 @@ public:
     }
 };
 
+template <
+    class In,
+    class Out,
+    class Lim,
+    typename FnPtr,
+    std::same_as<bool>... Round>
+TAmounts<In, Out>
+Quality::ceil_TAmounts_helper(
+    TAmounts<In, Out> const& amount,
+    Lim const& limit,
+    Lim const& limit_cmp,
+    FnPtr ceil_function,
+    Round... roundUp) const
+{
+    if (limit_cmp <= limit)
+        return amount;
+
+    // Use the existing STAmount implementation for now, but consider
+    // replacing with code specific to IOUAMount and XRPAmount
+    Amounts stAmt(toSTAmount(amount.in), toSTAmount(amount.out));
+    STAmount stLim(toSTAmount(limit));
+    Amounts const stRes = ((*this).*ceil_function)(stAmt, stLim, roundUp...);
+    return TAmounts<In, Out>(toAmount<In>(stRes.in), toAmount<Out>(stRes.out));
+}
+
+template <class In, class Out>
+TAmounts<In, Out>
+Quality::ceil_in(TAmounts<In, Out> const& amount, In const& limit) const
+{
+    // Construct a function pointer to the function we want to call.
+    static constexpr Amounts (Quality::*ceil_in_fn_ptr)(
+        Amounts const&, STAmount const&) const = &Quality::ceil_in;
+
+    return ceil_TAmounts_helper(amount, limit, amount.in, ceil_in_fn_ptr);
+}
+
+template <class In, class Out>
+TAmounts<In, Out>
+Quality::ceil_in_strict(
+    TAmounts<In, Out> const& amount,
+    In const& limit,
+    bool roundUp) const
+{
+    // Construct a function pointer to the function we want to call.
+    static constexpr Amounts (Quality::*ceil_in_fn_ptr)(
+        Amounts const&, STAmount const&, bool) const = &Quality::ceil_in_strict;
+
+    return ceil_TAmounts_helper(
+        amount, limit, amount.in, ceil_in_fn_ptr, roundUp);
+}
+
+template <class In, class Out>
+TAmounts<In, Out>
+Quality::ceil_out(TAmounts<In, Out> const& amount, Out const& limit) const
+{
+    // Construct a function pointer to the function we want to call.
+    static constexpr Amounts (Quality::*ceil_out_fn_ptr)(
+        Amounts const&, STAmount const&) const = &Quality::ceil_out;
+
+    return ceil_TAmounts_helper(amount, limit, amount.out, ceil_out_fn_ptr);
+}
+
+template <class In, class Out>
+TAmounts<In, Out>
+Quality::ceil_out_strict(
+    TAmounts<In, Out> const& amount,
+    Out const& limit,
+    bool roundUp) const
+{
+    // Construct a function pointer to the function we want to call.
+    static constexpr Amounts (Quality::*ceil_out_fn_ptr)(
+        Amounts const&, STAmount const&, bool) const =
+        &Quality::ceil_out_strict;
+
+    return ceil_TAmounts_helper(
+        amount, limit, amount.out, ceil_out_fn_ptr, roundUp);
+}
+
 /** Calculate the quality of a two-hop path given the two hops.
     @param lhs  The first leg of the path: input to intermediate.
     @param rhs  The second leg of the path: intermediate to output.

src/ripple/protocol/impl/Feature.cpp

@@ -467,6 +467,7 @@ REGISTER_FIX    (fixEmptyDID,                   Supported::yes, VoteBehavior::De
 REGISTER_FIX    (fixXChainRewardRounding,       Supported::yes, VoteBehavior::DefaultNo);
 REGISTER_FIX    (fixPreviousTxnID,              Supported::yes, VoteBehavior::DefaultNo);
 REGISTER_FIX    (fixAMMv1_1,                    Supported::yes, VoteBehavior::DefaultNo);
+REGISTER_FIX    (fixReducedOffersV2,            Supported::yes, VoteBehavior::DefaultNo);
 
 // The following amendments are obsolete, but must remain supported
 // because they could potentially get enabled.

src/ripple/protocol/impl/Quality.cpp

@@ -64,13 +64,20 @@ Quality::operator--(int)
     return prev;
 }
 
-Amounts
+template <STAmount (
-Quality::ceil_in(Amounts const& amount, STAmount const& limit) const
+    *DivRoundFunc)(STAmount const&, STAmount const&, Issue const&, bool)>
+static Amounts
+ceil_in_impl(
+    Amounts const& amount,
+    STAmount const& limit,
+    bool roundUp,
+    Quality const& quality)
 {
     if (amount.in > limit)
     {
         Amounts result(
-            limit, divRound(limit, rate(), amount.out.issue(), true));
+            limit,
+            DivRoundFunc(limit, quality.rate(), amount.out.issue(), roundUp));
         // Clamp out
         if (result.out > amount.out)
             result.out = amount.out;
@@ -81,6 +88,21 @@ Quality::ceil_in(Amounts const& amount, STAmount const& limit) const
     return amount;
 }
 
+Amounts
+Quality::ceil_in(Amounts const& amount, STAmount const& limit) const
+{
+    return ceil_in_impl<divRound>(amount, limit, /* roundUp */ true, *this);
+}
+
+Amounts
+Quality::ceil_in_strict(
+    Amounts const& amount,
+    STAmount const& limit,
+    bool roundUp) const
+{
+    return ceil_in_impl<divRoundStrict>(amount, limit, roundUp, *this);
+}
+
 template <STAmount (
     *MulRoundFunc)(STAmount const&, STAmount const&, Issue const&, bool)>
 static Amounts

src/test/app/AMM_test.cpp

@@ -3618,13 +3618,16 @@ private:
                     STAmount(USD, UINT64_C(9'970'097277662122), -12),
                     STAmount(EUR, UINT64_C(10'029'99250187452), -11),
                     ammUSD_EUR.tokens()));
-                BEAST_EXPECT(expectOffers(
+
-                    env,
+                // fixReducedOffersV2 changes the expected results slightly.
-                    alice,
+                Amounts const expectedAmounts =
-                    1,
+                    env.closed()->rules().enabled(fixReducedOffersV2)
-                    {{Amounts{
+                    ? Amounts{XRPAmount(30'201'749), STAmount(USD, UINT64_C(29'90272233787816), -14)}
+                    : Amounts{
                           XRPAmount(30'201'749),
-                        STAmount(USD, UINT64_C(29'90272233787818), -14)}}}));
+                          STAmount(USD, UINT64_C(29'90272233787818), -14)};
+
+                BEAST_EXPECT(expectOffers(env, alice, 1, {{expectedAmounts}}));
             }
             else
             {
@@ -3632,13 +3635,16 @@ private:
                     STAmount(USD, UINT64_C(9'970'097277662172), -12),
                     STAmount(EUR, UINT64_C(10'029'99250187452), -11),
                     ammUSD_EUR.tokens()));
-                BEAST_EXPECT(expectOffers(
+
-                    env,
+                // fixReducedOffersV2 changes the expected results slightly.
-                    alice,
+                Amounts const expectedAmounts =
-                    1,
+                    env.closed()->rules().enabled(fixReducedOffersV2)
-                    {{Amounts{
+                    ? Amounts{XRPAmount(30'201'749), STAmount(USD, UINT64_C(29'90272233782839), -14)}
+                    : Amounts{
                           XRPAmount(30'201'749),
-                        STAmount(USD, UINT64_C(29'9027223378284), -13)}}}));
+                          STAmount(USD, UINT64_C(29'90272233782840), -14)};
+
+                BEAST_EXPECT(expectOffers(env, alice, 1, {{expectedAmounts}}));
             }
             // Initial 30,000 + 100
             BEAST_EXPECT(expectLine(env, carol, STAmount{USD, 30'100}));
@@ -6874,6 +6880,8 @@ private:
         testInvalidAMMPayment();
         testBasicPaymentEngine(all);
         testBasicPaymentEngine(all - fixAMMv1_1);
+        testBasicPaymentEngine(all - fixReducedOffersV2);
+        testBasicPaymentEngine(all - fixAMMv1_1 - fixReducedOffersV2);
         testAMMTokens();
         testAmendment();
         testFlags();

src/test/app/Flow_test.cpp

@@ -506,7 +506,6 @@ struct Flow_test : public beast::unit_test::suite
             // Without limits, the 0.4 USD would produce 1000 EUR in the forward
             // pass. This test checks that the payment produces 1 EUR, as
             // expected.
-
             Env env(*this, features);
             env.fund(XRP(10000), alice, bob, carol, gw);
             env.trust(USD(1000), alice, bob, carol);
@@ -515,17 +514,52 @@ struct Flow_test : public beast::unit_test::suite
             env(pay(gw, alice, USD(1000)));
             env(pay(gw, bob, EUR(1000)));
 
+            Keylet const bobUsdOffer = keylet::offer(bob, env.seq(bob));
             env(offer(bob, USD(1), drops(2)), txflags(tfPassive));
             env(offer(bob, drops(1), EUR(1000)), txflags(tfPassive));
 
+            bool const reducedOffersV2 = features[fixReducedOffersV2];
+
+            // With reducedOffersV2, it is not allowed to accept less than
+            // USD(0.5) of bob's USD offer.  If we provide 1 drop for less
+            // than USD(0.5), then the remaining fractional offer would
+            // block the order book.
+            TER const expectedTER =
+                reducedOffersV2 ? TER(tecPATH_DRY) : TER(tesSUCCESS);
             env(pay(alice, carol, EUR(1)),
                 path(~XRP, ~EUR),
                 sendmax(USD(0.4)),
-                txflags(tfNoRippleDirect | tfPartialPayment));
+                txflags(tfNoRippleDirect | tfPartialPayment),
+                ter(expectedTER));
 
+            if (!reducedOffersV2)
+            {
                 env.require(balance(carol, EUR(1)));
                 env.require(balance(bob, USD(0.4)));
                 env.require(balance(bob, EUR(999)));
+
+                // Show that bob's USD offer is now a blocker.
+                std::shared_ptr<SLE const> const usdOffer = env.le(bobUsdOffer);
+                if (BEAST_EXPECT(usdOffer))
+                {
+                    std::uint64_t const bookRate = [&usdOffer]() {
+                        // Extract the least significant 64 bits from the
+                        // book page.  That's where the quality is stored.
+                        std::string bookDirStr =
+                            to_string(usdOffer->at(sfBookDirectory));
+                        bookDirStr.erase(0, 48);
+                        return std::stoull(bookDirStr, nullptr, 16);
+                    }();
+                    std::uint64_t const actualRate = getRate(
+                        usdOffer->at(sfTakerGets), usdOffer->at(sfTakerPays));
+
+                    // We expect the actual rate of the offer to be worse
+                    // (larger) than the rate of the book page holding the
+                    // offer.  This is a defect which is corrected by
+                    // fixReducedOffersV2.
+                    BEAST_EXPECT(actualRate > bookRate);
+                }
+            }
         }
     }
 
@@ -1375,9 +1409,11 @@ struct Flow_test : public beast::unit_test::suite
     {
         using namespace jtx;
         FeatureBitset const ownerPaysFee{featureOwnerPaysFee};
+        FeatureBitset const reducedOffersV2(fixReducedOffersV2);
 
         testLineQuality(features);
         testFalseDry(features);
+        testBookStep(features - reducedOffersV2);
         testDirectStep(features);
         testBookStep(features);
         testDirectStep(features | ownerPaysFee);

src/test/app/Offer_test.cpp

@@ -1387,7 +1387,12 @@ public:
 
         using namespace jtx;
 
-        Env env{*this, features};
+        // This is one of the few tests where fixReducedOffersV2 changes the
+        // results.  So test both with and without fixReducedOffersV2.
+        for (FeatureBitset localFeatures :
+             {features - fixReducedOffersV2, features | fixReducedOffersV2})
+        {
+            Env env{*this, localFeatures};
 
             auto const gw = Account{"gateway"};
             auto const alice = Account{"alice"};
@@ -1426,7 +1431,8 @@ public:
 
             // verify balances
             auto jrr = ledgerEntryState(env, alice, gw, "USD");
-        BEAST_EXPECT(jrr[jss::node][sfBalance.fieldName][jss::value] == "50");
+            BEAST_EXPECT(
+                jrr[jss::node][sfBalance.fieldName][jss::value] == "50");
 
             jrr = ledgerEntryState(env, bob, gw, "USD");
             BEAST_EXPECT(
@@ -1434,32 +1440,54 @@ public:
                 "-2710505431213761e-33");
 
             // create crossing offer
+            std::uint32_t const bobOfferSeq = env.seq(bob);
             env(offer(bob, XRP(2000), USD(1)));
 
+            if (localFeatures[featureFlowCross] &&
+                localFeatures[fixReducedOffersV2])
+            {
+                // With the rounding introduced by fixReducedOffersV2, bob's
+                // offer does not cross alice's offer and goes straight into
+                // the ledger.
+                jrr = ledgerEntryState(env, bob, gw, "USD");
+                BEAST_EXPECT(
+                    jrr[jss::node][sfBalance.fieldName][jss::value] ==
+                    "-2710505431213761e-33");
+
+                Json::Value const bobOffer =
+                    ledgerEntryOffer(env, bob, bobOfferSeq)[jss::node];
+                BEAST_EXPECT(bobOffer[sfTakerGets.jsonName][jss::value] == "1");
+                BEAST_EXPECT(bobOffer[sfTakerPays.jsonName] == "2000000000");
+                return;
+            }
+
             // verify balances again.
             //
-        // NOTE :
+            // NOTE:
-        // Here a difference in the rounding modes of our two offer crossing
+            // Here a difference in the rounding modes of our two offer
-        // algorithms becomes apparent.  The old offer crossing would consume
+            // crossing algorithms becomes apparent.  The old offer crossing
-        // small_amount and transfer no XRP.  The new offer crossing transfers
+            // would consume small_amount and transfer no XRP.  The new offer
-        // a single drop, rather than no drops.
+            // crossing transfers a single drop, rather than no drops.
             auto const crossingDelta =
-            features[featureFlowCross] ? drops(1) : drops(0);
+                localFeatures[featureFlowCross] ? drops(1) : drops(0);
 
             jrr = ledgerEntryState(env, alice, gw, "USD");
-        BEAST_EXPECT(jrr[jss::node][sfBalance.fieldName][jss::value] == "50");
+            BEAST_EXPECT(
+                jrr[jss::node][sfBalance.fieldName][jss::value] == "50");
             BEAST_EXPECT(
                 env.balance(alice, xrpIssue()) ==
                 alice_initial_balance - env.current()->fees().base * 3 -
                     crossingDelta);
 
             jrr = ledgerEntryState(env, bob, gw, "USD");
-        BEAST_EXPECT(jrr[jss::node][sfBalance.fieldName][jss::value] == "0");
+            BEAST_EXPECT(
+                jrr[jss::node][sfBalance.fieldName][jss::value] == "0");
             BEAST_EXPECT(
                 env.balance(bob, xrpIssue()) ==
                 bob_initial_balance - env.current()->fees().base * 2 +
                     crossingDelta);
         }
+    }
 
     void
     testOfferCrossWithXRP(bool reverse_order, FeatureBitset features)

src/test/app/ReducedOffer_test.cpp

@@ -22,6 +22,8 @@
 #include <ripple/protocol/jss.h>
 #include <test/jtx.h>
 
+#include <initializer_list>
+
 namespace ripple {
 namespace test {
 
@@ -56,13 +58,11 @@ class ReducedOffer_test : public beast::unit_test::suite
     static void
     cleanupOldOffers(
         jtx::Env& env,
-        jtx::Account const& acct1,
+        std::initializer_list<std::pair<jtx::Account const&, std::uint32_t>>
-        jtx::Account const& acct2,
+            list)
-        std::uint32_t acct1OfferSeq,
-        std::uint32_t acct2OfferSeq)
     {
-        env(offer_cancel(acct1, acct1OfferSeq));
+        for (auto [acct, offerSeq] : list)
-        env(offer_cancel(acct2, acct2OfferSeq));
+            env(offer_cancel(acct, offerSeq));
         env.close();
     }
 
@@ -180,7 +180,8 @@ public:
 
                 // In preparation for the next iteration make sure the two
                 // offers are gone from the ledger.
-                cleanupOldOffers(env, alice, bob, aliceOfferSeq, bobOfferSeq);
+                cleanupOldOffers(
+                    env, {{alice, aliceOfferSeq}, {bob, bobOfferSeq}});
                 return badRate;
             };
 
@@ -279,7 +280,7 @@ public:
                     // If the in-ledger offer was not consumed then further
                     // results are meaningless.
                     cleanupOldOffers(
-                        env, alice, bob, aliceOfferSeq, bobOfferSeq);
+                        env, {{alice, aliceOfferSeq}, {bob, bobOfferSeq}});
                     return 1;
                 }
                 // alice's offer should still be in the ledger, but reduced in
@@ -337,7 +338,8 @@ public:
 
                 // In preparation for the next iteration make sure the two
                 // offers are gone from the ledger.
-                cleanupOldOffers(env, alice, bob, aliceOfferSeq, bobOfferSeq);
+                cleanupOldOffers(
+                    env, {{alice, aliceOfferSeq}, {bob, bobOfferSeq}});
                 return badRate;
             };
 
@@ -452,7 +454,7 @@ public:
                     // In preparation for the next iteration clean up any
                     // leftover offers.
                     cleanupOldOffers(
-                        env, alice, bob, aliceOfferSeq, bobOfferSeq);
+                        env, {{alice, aliceOfferSeq}, {bob, bobOfferSeq}});
 
                     // Zero out alice's and bob's USD balances.
                     if (STAmount const aliceBalance = env.balance(alice, USD);
@@ -573,7 +575,8 @@ public:
 
                 // In preparation for the next iteration clean up any
                 // leftover offers.
-                cleanupOldOffers(env, alice, bob, aliceOfferSeq, bobOfferSeq);
+                cleanupOldOffers(
+                    env, {{alice, aliceOfferSeq}, {bob, bobOfferSeq}});
 
                 // Zero out alice's and bob's IOU balances.
                 auto zeroBalance = [&env, &gw](
@@ -606,6 +609,185 @@ public:
         }
     }
 
+    Amounts
+    jsonOfferToAmounts(Json::Value const& json)
+    {
+        STAmount const in =
+            amountFromJson(sfTakerPays, json[sfTakerPays.jsonName]);
+        STAmount const out =
+            amountFromJson(sfTakerGets, json[sfTakerGets.jsonName]);
+        return {in, out};
+    }
+
+    void
+    testSellPartialCrossOldXrpIouQChange()
+    {
+        // This test case was motivated by Issue #4937.  It recreates
+        // the specific failure identified in that issue and samples some other
+        // cases in the same vicinity to make sure that the new behavior makes
+        // sense.
+        testcase("exercise tfSell partial cross old XRP/IOU offer Q change");
+
+        using namespace jtx;
+
+        Account const gw("gateway");
+        Account const alice("alice");
+        Account const bob("bob");
+        Account const carol("carol");
+        auto const USD = gw["USD"];
+
+        // Make one test run without fixReducedOffersV2 and one with.
+        for (FeatureBitset features :
+             {supported_amendments() - fixReducedOffersV2,
+              supported_amendments() | fixReducedOffersV2})
+        {
+            // Make sure none of the offers we generate are under funded.
+            Env env{*this, features};
+            env.fund(XRP(10'000'000), gw, alice, bob, carol);
+            env.close();
+
+            env(trust(alice, USD(10'000'000)));
+            env(trust(bob, USD(10'000'000)));
+            env(trust(carol, USD(10'000'000)));
+            env.close();
+
+            env(pay(gw, alice, USD(10'000'000)));
+            env(pay(gw, bob, USD(10'000'000)));
+            env(pay(gw, carol, USD(10'000'000)));
+            env.close();
+
+            // Lambda that:
+            //  1. Exercises one offer trio,
+            //  2. Collects the results, and
+            //  3. Cleans up for the next offer trio.
+            auto exerciseOfferTrio =
+                [this, &env, &alice, &bob, &carol, &USD](
+                    Amounts const& carolOffer) -> unsigned int {
+                // alice submits an offer that may become a blocker.
+                std::uint32_t const aliceOfferSeq = env.seq(alice);
+                static Amounts const aliceInitialOffer(USD(2), drops(3382562));
+                env(offer(alice, aliceInitialOffer.in, aliceInitialOffer.out));
+                env.close();
+                STAmount const initialRate =
+                    Quality(jsonOfferToAmounts(ledgerEntryOffer(
+                                env, alice, aliceOfferSeq)[jss::node]))
+                        .rate();
+
+                // bob submits an offer that is more desirable than alice's
+                std::uint32_t const bobOfferSeq = env.seq(bob);
+                env(offer(bob, USD(0.97086565812384), drops(1642020)));
+                env.close();
+
+                // Now carol's offer consumes bob's and partially crosses
+                // alice's.  The tfSell flag is important.
+                std::uint32_t const carolOfferSeq = env.seq(carol);
+                env(offer(carol, carolOffer.in, carolOffer.out),
+                    txflags(tfSell));
+                env.close();
+
+                // carol's offer should not have made it into the ledger and
+                // bob's offer should be fully consumed.
+                if (!BEAST_EXPECT(
+                        !offerInLedger(env, carol, carolOfferSeq) &&
+                        !offerInLedger(env, bob, bobOfferSeq)))
+                {
+                    // If carol's or bob's offers are still in the ledger then
+                    // further results are meaningless.
+                    cleanupOldOffers(
+                        env,
+                        {{alice, aliceOfferSeq},
+                         {bob, bobOfferSeq},
+                         {carol, carolOfferSeq}});
+                    return 1;
+                }
+                // alice's offer should still be in the ledger, but reduced in
+                // size.
+                unsigned int badRate = 1;
+                {
+                    Json::Value aliceOffer =
+                        ledgerEntryOffer(env, alice, aliceOfferSeq);
+
+                    Amounts aliceReducedOffer =
+                        jsonOfferToAmounts(aliceOffer[jss::node]);
+
+                    BEAST_EXPECT(aliceReducedOffer.in < aliceInitialOffer.in);
+                    BEAST_EXPECT(aliceReducedOffer.out < aliceInitialOffer.out);
+                    STAmount const inLedgerRate =
+                        Quality(aliceReducedOffer).rate();
+                    badRate = inLedgerRate > initialRate ? 1 : 0;
+
+                    // If the inLedgerRate is less than initial rate, then
+                    // incrementing the mantissa of the reduced TakerGets
+                    // should result in a rate higher than initial.  Check
+                    // this to verify that the largest allowable TakerGets
+                    // was computed.
+                    if (badRate == 0)
+                    {
+                        STAmount const tweakedTakerGets(
+                            aliceReducedOffer.in.issue(),
+                            aliceReducedOffer.in.mantissa() + 1,
+                            aliceReducedOffer.in.exponent(),
+                            aliceReducedOffer.in.negative());
+                        STAmount const tweakedRate =
+                            Quality(
+                                Amounts{aliceReducedOffer.in, tweakedTakerGets})
+                                .rate();
+                        BEAST_EXPECT(tweakedRate > initialRate);
+                    }
+#if 0
+                    std::cout << "Placed rate: " << initialRate
+                              << "; in-ledger rate: " << inLedgerRate
+                              << "; TakerPays: " << aliceReducedOffer.in
+                              << "; TakerGets: " << aliceReducedOffer.out
+                              << std::endl;
+// #else
+                    std::string_view filler = badRate ? "**" : "  ";
+                    std::cout << "| " << aliceReducedOffer.in << "` | `"
+                              << aliceReducedOffer.out << "` | `" << initialRate
+                              << "` | " << filler << "`" << inLedgerRate << "`"
+                              << filler << std::endl;
+#endif
+                }
+
+                // In preparation for the next iteration make sure all three
+                // offers are gone from the ledger.
+                cleanupOldOffers(
+                    env,
+                    {{alice, aliceOfferSeq},
+                     {bob, bobOfferSeq},
+                     {carol, carolOfferSeq}});
+                return badRate;
+            };
+
+            constexpr int loopCount = 100;
+            unsigned int blockedCount = 0;
+            {
+                STAmount increaseGets = USD(0);
+                STAmount const step(increaseGets.issue(), 1, -8);
+                for (unsigned int i = 0; i < loopCount; ++i)
+                {
+                    blockedCount += exerciseOfferTrio(
+                        Amounts(drops(1642020), USD(1) + increaseGets));
+                    increaseGets += step;
+                }
+            }
+
+            // If fixReducedOffersV2 is enabled, then none of the test cases
+            // should produce a potentially blocking rate.
+            //
+            // Also verify that if fixReducedOffersV2 is not enabled then
+            // some of the test cases produced a potentially blocking rate.
+            if (features[fixReducedOffersV2])
+            {
+                BEAST_EXPECT(blockedCount == 0);
+            }
+            else
+            {
+                BEAST_EXPECT(blockedCount > 80);
+            }
+        }
+    }
+
     void
     run() override
     {
@@ -613,6 +795,7 @@ public:
         testPartialCrossOldXrpIouQChange();
         testUnderFundedXrpIouQChange();
         testUnderFundedIouIouQChange();
+        testSellPartialCrossOldXrpIouQChange();
     }
 };