Rm some offers where the quality is reduced:

Substantial reductions in an offer's effective quality from its
initial quality may clog offer books.

src/ripple/app/tx/impl/OfferStream.cpp

@@ -19,6 +19,7 @@
 
 #include <ripple/app/tx/impl/OfferStream.h>
 #include <ripple/basics/Log.h>
+#include <ripple/protocol/Feature.h>
 
 namespace ripple {
 
@@ -132,6 +133,73 @@ accountFundsHelper(
         view, id, issue.currency, issue.account, freezeHandling, j));
 }
 
+template <class TIn, class TOut>
+template <class TTakerPays, class TTakerGets>
+bool
+TOfferStreamBase<TIn, TOut>::shouldRmSmallIncreasedQOffer() const
+{
+    static_assert(
+        std::is_same_v<TTakerPays, IOUAmount> ||
+            std::is_same_v<TTakerPays, XRPAmount>,
+        "STAmount is not supported");
+
+    static_assert(
+        std::is_same_v<TTakerGets, IOUAmount> ||
+            std::is_same_v<TTakerGets, XRPAmount>,
+        "STAmount is not supported");
+
+    static_assert(
+        !std::is_same_v<TTakerPays, XRPAmount> ||
+            !std::is_same_v<TTakerGets, XRPAmount>,
+        "Cannot have XRP/XRP offers");
+
+    if (!view_.rules().enabled(fixRmSmallIncreasedQOffers))
+        return false;
+
+    // Consider removing the offer if:
+    //  o `TakerPays` is XRP (because of XRP drops granularity) or
+    //  o `TakerPays` and `TakerGets` are both IOU and `TakerPays`<`TakerGets`
+    constexpr bool const inIsXRP = std::is_same_v<TTakerPays, XRPAmount>;
+    constexpr bool const outIsXRP = std::is_same_v<TTakerGets, XRPAmount>;
+
+    if constexpr (outIsXRP)
+    {
+        // If `TakerGets` is XRP, the worst this offer's quality can change is
+        // to about 10^-81 `TakerPays` and 1 drop `TakerGets`. This will be
+        // remarkably good quality for any realistic asset, so these offers
+        // don't need this extra check.
+        return false;
+    }
+
+    TAmounts<TTakerPays, TTakerGets> const ofrAmts{
+        toAmount<TTakerPays>(offer_.amount().in),
+        toAmount<TTakerGets>(offer_.amount().out)};
+
+    if constexpr (!inIsXRP && !outIsXRP)
+    {
+        if (ofrAmts.in >= ofrAmts.out)
+            return false;
+    }
+
+    TTakerGets const ownerFunds = toAmount<TTakerGets>(*ownerFunds_);
+
+    auto const effectiveAmounts = [&] {
+        if (offer_.owner() != offer_.issueOut().account &&
+            ownerFunds < ofrAmts.out)
+        {
+            // adjust the amounts by owner funds
+            return offer_.quality().ceil_out(ofrAmts, ownerFunds);
+        }
+        return ofrAmts;
+    }();
+
+    if (effectiveAmounts.in > TTakerPays::minPositiveAmount())
+        return false;
+
+    Quality const effectiveQuality{effectiveAmounts};
+    return effectiveQuality < offer_.quality();
+}
+
 template <class TIn, class TOut>
 bool
 TOfferStreamBase<TIn, TOut>::step()
@@ -222,6 +290,68 @@ TOfferStreamBase<TIn, TOut>::step()
                     << "Removing became unfunded offer " << entry->key();
             }
             offer_ = TOffer<TIn, TOut>{};
+            // See comment at top of loop for how the offer is removed
+            continue;
+        }
+
+        bool const rmSmallIncreasedQOffer = [&] {
+            bool const inIsXRP = isXRP(offer_.issueIn());
+            bool const outIsXRP = isXRP(offer_.issueOut());
+            if (inIsXRP && !outIsXRP)
+            {
+                // Without the `if constexpr`, the
+                // `shouldRmSmallIncreasedQOffer` template will be instantiated
+                // even if it is never used. This can cause compiler errors in
+                // some cases, hence the `if constexpr` guard.
+                // Note that TIn can be XRPAmount or STAmount, and TOut can be
+                // IOUAmount or STAmount.
+                if constexpr (!(std::is_same_v<TIn, IOUAmount> ||
+                                std::is_same_v<TOut, XRPAmount>))
+                    return shouldRmSmallIncreasedQOffer<XRPAmount, IOUAmount>();
+            }
+            if (!inIsXRP && outIsXRP)
+            {
+                // See comment above for `if constexpr` rationale
+                if constexpr (!(std::is_same_v<TIn, XRPAmount> ||
+                                std::is_same_v<TOut, IOUAmount>))
+                    return shouldRmSmallIncreasedQOffer<IOUAmount, XRPAmount>();
+            }
+            if (!inIsXRP && !outIsXRP)
+            {
+                // See comment above for `if constexpr` rationale
+                if constexpr (!(std::is_same_v<TIn, XRPAmount> ||
+                                std::is_same_v<TOut, XRPAmount>))
+                    return shouldRmSmallIncreasedQOffer<IOUAmount, IOUAmount>();
+            }
+            assert(0);  // xrp/xrp offer!?! should never happen
+            return false;
+        }();
+
+        if (rmSmallIncreasedQOffer)
+        {
+            auto const original_funds = accountFundsHelper(
+                cancelView_,
+                offer_.owner(),
+                amount.out,
+                offer_.issueOut(),
+                fhZERO_IF_FROZEN,
+                j_);
+
+            if (original_funds == *ownerFunds_)
+            {
+                permRmOffer(entry->key());
+                JLOG(j_.trace())
+                    << "Removing tiny offer due to reduced quality "
+                    << entry->key();
+            }
+            else
+            {
+                JLOG(j_.trace()) << "Removing tiny offer that became tiny due "
+                                    "to reduced quality "
+                                 << entry->key();
+            }
+            offer_ = TOffer<TIn, TOut>{};
+            // See comment at top of loop for how the offer is removed
             continue;
         }
 

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

@@ -85,6 +85,10 @@ protected:
     virtual void
     permRmOffer(uint256 const& offerIndex) = 0;
 
+    template <class TTakerPays, class TTakerGets>
+    bool
+    shouldRmSmallIncreasedQOffer() const;
+
 public:
     TOfferStreamBase(
         ApplyView& view,

src/ripple/basics/IOUAmount.h

@@ -129,6 +129,9 @@ public:
     {
         return mantissa_;
     }
+
+    static IOUAmount
+    minPositiveAmount();
 };
 
 std::string

src/ripple/basics/XRPAmount.h

@@ -238,6 +238,12 @@ public:
         s >> val.drops_;
         return s;
     }
+
+    static XRPAmount
+    minPositiveAmount()
+    {
+        return XRPAmount{1};
+    }
 };
 
 /** Number of drops per 1 XRP */

src/ripple/basics/impl/IOUAmount.cpp

@@ -34,6 +34,12 @@ static std::int64_t const maxMantissa = 9999999999999999ull;
 static int const minExponent = -96;
 static int const maxExponent = 80;
 
+IOUAmount
+IOUAmount::minPositiveAmount()
+{
+    return IOUAmount(minMantissa, minExponent);
+}
+
 void
 IOUAmount::normalize()
 {
@@ -353,7 +359,7 @@ mulRatio(
         {
             if (!result)
             {
-                return IOUAmount(minMantissa, minExponent);
+                return IOUAmount::minPositiveAmount();
             }
             // This addition cannot overflow because the mantissa is already
             // normalized

src/ripple/protocol/AmountConversions.h

@@ -63,11 +63,7 @@ toSTAmount(XRPAmount const& xrp, Issue const& iss)
 
 template <class T>
 T
-toAmount(STAmount const& amt)
-{
-    static_assert(sizeof(T) == -1, "Must use specialized function");
-    return T(0);
-}
+toAmount(STAmount const& amt) = delete;
 
 template <>
 inline STAmount
@@ -102,6 +98,28 @@ toAmount<XRPAmount>(STAmount const& amt)
     return XRPAmount(sMant);
 }
 
+template <class T>
+T
+toAmount(IOUAmount const& amt) = delete;
+
+template <>
+inline IOUAmount
+toAmount<IOUAmount>(IOUAmount const& amt)
+{
+    return amt;
+}
+
+template <class T>
+T
+toAmount(XRPAmount const& amt) = delete;
+
+template <>
+inline XRPAmount
+toAmount<XRPAmount>(XRPAmount const& amt)
+{
+    return amt;
+}
+
 }  // namespace ripple
 
 #endif

src/ripple/protocol/Feature.h

@@ -116,6 +116,7 @@ class FeatureCollections
         "TicketBatch",
         "FlowSortStrands",
         "fixSTAmountCanonicalize",
+        "fixRmSmallIncreasedQOffers",
     };
 
     std::vector<uint256> features;
@@ -376,6 +377,7 @@ extern uint256 const featureNegativeUNL;
 extern uint256 const featureTicketBatch;
 extern uint256 const featureFlowSortStrands;
 extern uint256 const fixSTAmountCanonicalize;
+extern uint256 const fixRmSmallIncreasedQOffers;
 
 }  // namespace ripple
 

src/ripple/protocol/Quality.h

@@ -132,6 +132,13 @@ public:
     /** Create a quality from the ratio of two amounts. */
     explicit Quality(Amounts const& amount);
 
+    /** Create a quality from the ratio of two amounts. */
+    template <class In, class Out>
+    Quality(TAmounts<In, Out> const& amount)
+        : Quality(Amounts(toSTAmount(amount.in), toSTAmount(amount.out)))
+    {
+    }
+
     /** Create a quality from the ratio of two amounts. */
     template <class In, class Out>
     Quality(Out const& out, In const& in)

src/ripple/protocol/impl/Feature.cpp

@@ -135,6 +135,7 @@ detail::supportedAmendments()
         "TicketBatch",
         "FlowSortStrands",
         "fixSTAmountCanonicalize",
+        "fixRmSmallIncreasedQOffers",
     };
     return supported;
 }
@@ -190,7 +191,8 @@ uint256 const
     featureNegativeUNL              = *getRegisteredFeature("NegativeUNL"),
     featureTicketBatch              = *getRegisteredFeature("TicketBatch"),
     featureFlowSortStrands          = *getRegisteredFeature("FlowSortStrands"),
-    fixSTAmountCanonicalize         = *getRegisteredFeature("fixSTAmountCanonicalize");
+    fixSTAmountCanonicalize         = *getRegisteredFeature("fixSTAmountCanonicalize"),
+    fixRmSmallIncreasedQOffers      = *getRegisteredFeature("fixRmSmallIncreasedQOffers");
 
 // The following amendments have been active for at least two years. Their
 // pre-amendment code has been removed and the identifiers are deprecated.

src/test/app/Offer_test.cpp

@@ -320,17 +320,20 @@ public:
         Env env{*this, features};
 
         env.fund(XRP(10000), alice, bob, carol, dan, erin, gw);
+        env.close();
         env.trust(USD(1000), alice, bob, carol, dan, erin);
+        env.close();
         env(pay(gw, carol, USD(0.99999)));
         env(pay(gw, dan, USD(1)));
         env(pay(gw, erin, USD(1)));
+        env.close();
 
         // Carol doesn't quite have enough funds for this offer
         // The amount left after this offer is taken will cause
         // STAmount to incorrectly round to zero when the next offer
-        // (at a good quality) is considered. (when the
-        // stAmountCalcSwitchover2 patch is inactive)
-        env(offer(carol, drops(1), USD(1)));
+        // (at a good quality) is considered. (when the now removed
+        // stAmountCalcSwitchover2 patch was inactive)
+        env(offer(carol, drops(1), USD(0.99999)));
         // Offer at a quality poor enough so when the input xrp is
         // calculated  in the reverse pass, the amount is not zero.
         env(offer(dan, XRP(100), USD(1)));
@@ -340,9 +343,9 @@ public:
         // It is considered after the offer from carol, which leaves a
         // tiny amount left to pay. When calculating the amount of xrp
         // needed for this offer, it will incorrectly compute zero in both
-        // the forward and reverse passes (when the stAmountCalcSwitchover2
-        // is inactive.)
-        env(offer(erin, drops(2), USD(2)));
+        // the forward and reverse passes (when the now removed
+        // stAmountCalcSwitchover2 was inactive.)
+        env(offer(erin, drops(2), USD(1)));
 
         env(pay(alice, bob, USD(1)),
             path(~USD),
@@ -356,6 +359,317 @@ public:
         env.require(balance(erin, USD(0.99999)), offers(erin, 1));
     }
 
+    void
+    testRmSmallIncreasedQOffersXRP(FeatureBitset features)
+    {
+        testcase("Rm small increased q offers XRP");
+
+        // Carol places an offer, but cannot fully fund the offer. When her
+        // funding is taken into account, the offer's quality drops below its
+        // initial quality and has an input amount of 1 drop. This is removed as
+        // an offer that may block offer books.
+
+        using namespace jtx;
+        using namespace std::chrono_literals;
+        auto const alice = Account{"alice"};
+        auto const bob = Account{"bob"};
+        auto const carol = Account{"carol"};
+        auto const gw = Account{"gw"};
+
+        auto const USD = gw["USD"];
+
+        // Test offer crossing
+        for (auto crossBothOffers : {false, true})
+        {
+            Env env{*this, features};
+
+            env.fund(XRP(10000), alice, bob, carol, gw);
+            env.close();
+            env.trust(USD(1000), alice, bob, carol);
+            // underfund carol's offer
+            auto initialCarolUSD = USD(0.499);
+            env(pay(gw, carol, initialCarolUSD));
+            env(pay(gw, bob, USD(100)));
+            env.close();
+            // This offer is underfunded
+            env(offer(carol, drops(1), USD(1)));
+            env.close();
+            // offer at a lower quality
+            env(offer(bob, drops(2), USD(1), tfPassive));
+            env.close();
+            env.require(offers(bob, 1), offers(carol, 1));
+
+            // alice places an offer that crosses carol's; depending on
+            // "crossBothOffers" it may cross bob's as well
+            auto aliceTakerGets = crossBothOffers ? drops(2) : drops(1);
+            env(offer(alice, USD(1), aliceTakerGets));
+            env.close();
+
+            if (features[fixRmSmallIncreasedQOffers])
+            {
+                env.require(
+                    offers(carol, 0),
+                    balance(
+                        carol,
+                        initialCarolUSD));  // offer is removed but not taken
+                if (crossBothOffers)
+                {
+                    env.require(
+                        offers(alice, 0),
+                        balance(alice, USD(1)));  // alice's offer is crossed
+                }
+                else
+                {
+                    env.require(
+                        offers(alice, 1),
+                        balance(
+                            alice, USD(0)));  // alice's offer is not crossed
+                }
+            }
+            else
+            {
+                env.require(
+                    offers(alice, 1),
+                    offers(bob, 1),
+                    offers(carol, 1),
+                    balance(alice, USD(0)),
+                    balance(
+                        carol,
+                        initialCarolUSD));  // offer is not crossed at all
+            }
+        }
+
+        // Test payments
+        for (auto partialPayment : {false, true})
+        {
+            Env env{*this, features};
+
+            env.fund(XRP(10000), alice, bob, carol, gw);
+            env.close();
+            env.trust(USD(1000), alice, bob, carol);
+            env.close();
+            auto const initialCarolUSD = USD(0.999);
+            env(pay(gw, carol, initialCarolUSD));
+            env.close();
+            env(pay(gw, bob, USD(100)));
+            env.close();
+            env(offer(carol, drops(1), USD(1)));
+            env.close();
+            env(offer(bob, drops(2), USD(2), tfPassive));
+            env.close();
+            env.require(offers(bob, 1), offers(carol, 1));
+
+            std::uint32_t const flags = partialPayment
+                ? (tfNoRippleDirect | tfPartialPayment)
+                : tfNoRippleDirect;
+
+            TER const expectedTer =
+                partialPayment ? TER{tesSUCCESS} : TER{tecPATH_PARTIAL};
+
+            env(pay(alice, bob, USD(5)),
+                path(~USD),
+                sendmax(XRP(1)),
+                txflags(flags),
+                ter(expectedTer));
+            env.close();
+
+            if (features[fixRmSmallIncreasedQOffers])
+            {
+                if (expectedTer == tesSUCCESS)
+                {
+                    env.require(offers(carol, 0));
+                    env.require(balance(
+                        carol,
+                        initialCarolUSD));  // offer is removed but not taken
+                }
+                else
+                {
+                    // TODO: Offers are not removed when payments fail
+                    // If that is addressed, the test should show that carol's
+                    // offer is removed but not taken, as in the other branch of
+                    // this if statement
+                }
+            }
+            else
+            {
+                if (partialPayment)
+                {
+                    env.require(offers(carol, 0));
+                    env.require(
+                        balance(carol, USD(0)));  // offer is removed and taken
+                }
+                else
+                {
+                    // offer is not removed or taken
+                    BEAST_EXPECT(isOffer(env, carol, drops(1), USD(1)));
+                }
+            }
+        }
+    }
+
+    void
+    testRmSmallIncreasedQOffersIOU(FeatureBitset features)
+    {
+        testcase("Rm small increased q offers IOU");
+
+        // Carol places an offer, but cannot fully fund the offer. When her
+        // funding is taken into account, the offer's quality drops below its
+        // initial quality and has an input amount of 1 drop. This is removed as
+        // an offer that may block offer books.
+
+        using namespace jtx;
+        using namespace std::chrono_literals;
+        auto const alice = Account{"alice"};
+        auto const bob = Account{"bob"};
+        auto const carol = Account{"carol"};
+        auto const gw = Account{"gw"};
+
+        auto const USD = gw["USD"];
+        auto const EUR = gw["EUR"];
+
+        auto tinyAmount = [&](IOU const& iou) -> PrettyAmount {
+            STAmount amt(
+                iou.issue(),
+                /*mantissa*/ 1,
+                /*exponent*/ -81);
+            return PrettyAmount(amt, iou.account.name());
+        };
+
+        // Test offer crossing
+        for (auto crossBothOffers : {false, true})
+        {
+            Env env{*this, features};
+
+            env.fund(XRP(10000), alice, bob, carol, gw);
+            env.close();
+            env.trust(USD(1000), alice, bob, carol);
+            env.trust(EUR(1000), alice, bob, carol);
+            // underfund carol's offer
+            auto initialCarolUSD = tinyAmount(USD);
+            env(pay(gw, carol, initialCarolUSD));
+            env(pay(gw, bob, USD(100)));
+            env(pay(gw, alice, EUR(100)));
+            env.close();
+            // This offer is underfunded
+            env(offer(carol, EUR(1), USD(10)));
+            env.close();
+            // offer at a lower quality
+            env(offer(bob, EUR(1), USD(5), tfPassive));
+            env.close();
+            env.require(offers(bob, 1), offers(carol, 1));
+
+            // alice places an offer that crosses carol's; depending on
+            // "crossBothOffers" it may cross bob's as well
+            // Whatever
+            auto aliceTakerGets = crossBothOffers ? EUR(0.2) : EUR(0.1);
+            env(offer(alice, USD(1), aliceTakerGets));
+            env.close();
+
+            if (features[fixRmSmallIncreasedQOffers])
+            {
+                env.require(
+                    offers(carol, 0),
+                    balance(
+                        carol,
+                        initialCarolUSD));  // offer is removed but not taken
+                if (crossBothOffers)
+                {
+                    env.require(
+                        offers(alice, 0),
+                        balance(alice, USD(1)));  // alice's offer is crossed
+                }
+                else
+                {
+                    env.require(
+                        offers(alice, 1),
+                        balance(
+                            alice, USD(0)));  // alice's offer is not crossed
+                }
+            }
+            else
+            {
+                env.require(
+                    offers(alice, 1),
+                    offers(bob, 1),
+                    offers(carol, 1),
+                    balance(alice, USD(0)),
+                    balance(
+                        carol,
+                        initialCarolUSD));  // offer is not crossed at all
+            }
+        }
+
+        // Test payments
+        for (auto partialPayment : {false, true})
+        {
+            Env env{*this, features};
+
+            env.fund(XRP(10000), alice, bob, carol, gw);
+            env.close();
+            env.trust(USD(1000), alice, bob, carol);
+            env.trust(EUR(1000), alice, bob, carol);
+            env.close();
+            // underfund carol's offer
+            auto const initialCarolUSD = tinyAmount(USD);
+            env(pay(gw, carol, initialCarolUSD));
+            env(pay(gw, bob, USD(100)));
+            env(pay(gw, alice, EUR(100)));
+            env.close();
+            // This offer is underfunded
+            env(offer(carol, EUR(1), USD(2)));
+            env.close();
+            env(offer(bob, EUR(2), USD(4), tfPassive));
+            env.close();
+            env.require(offers(bob, 1), offers(carol, 1));
+
+            std::uint32_t const flags = partialPayment
+                ? (tfNoRippleDirect | tfPartialPayment)
+                : tfNoRippleDirect;
+
+            TER const expectedTer =
+                partialPayment ? TER{tesSUCCESS} : TER{tecPATH_PARTIAL};
+
+            env(pay(alice, bob, USD(5)),
+                path(~USD),
+                sendmax(EUR(10)),
+                txflags(flags),
+                ter(expectedTer));
+            env.close();
+
+            if (features[fixRmSmallIncreasedQOffers])
+            {
+                if (expectedTer == tesSUCCESS)
+                {
+                    env.require(offers(carol, 0));
+                    env.require(balance(
+                        carol,
+                        initialCarolUSD));  // offer is removed but not taken
+                }
+                else
+                {
+                    // TODO: Offers are not removed when payments fail
+                    // If that is addressed, the test should show that carol's
+                    // offer is removed but not taken, as in the other branch of
+                    // this if statement
+                }
+            }
+            else
+            {
+                if (partialPayment)
+                {
+                    env.require(offers(carol, 0));
+                    env.require(
+                        balance(carol, USD(0)));  // offer is removed and taken
+                }
+                else
+                {
+                    // offer is not removed or taken
+                    BEAST_EXPECT(isOffer(env, carol, EUR(1), USD(2)));
+                }
+            }
+        }
+    }
+
     void
     testEnforceNoRipple(FeatureBitset features)
     {
@@ -5387,7 +5701,7 @@ public:
     {
         // An assert was falsely triggering when computing rates for offers.
         // This unit test would trigger that assert (which has been removed).
-        testcase("false assert");
+        testcase("incorrect assert fixed");
         using namespace jtx;
 
         Env env{*this};
@@ -5459,6 +5773,8 @@ public:
         testTickSize(features);
         testTicketOffer(features);
         testTicketCancelOffer(features);
+        testRmSmallIncreasedQOffersXRP(features);
+        testRmSmallIncreasedQOffersIOU(features);
     }
 
     void
@@ -5468,10 +5784,12 @@ public:
         FeatureBitset const all{supported_amendments()};
         FeatureBitset const flowCross{featureFlowCross};
         FeatureBitset const takerDryOffer{fixTakerDryOfferRemoval};
+        FeatureBitset const rmSmallIncreasedQOffers{fixRmSmallIncreasedQOffers};
 
         testAll(all - takerDryOffer);
         testAll(all - flowCross - takerDryOffer);
         testAll(all - flowCross);
+        testAll(all - rmSmallIncreasedQOffers);
         testAll(all);
         testFalseAssert();
     }