Amendment fixReducedOffersV2 fixes issue XRPLF#4937

src/ripple/app/paths/AMMOffer.h

@@ -23,6 +23,7 @@
 #include <ripple/ledger/ApplyView.h>
 #include <ripple/ledger/View.h>
 #include <ripple/protocol/Quality.h>
+#include <ripple/protocol/Rules.h>
 #include <ripple/protocol/TER.h>
 
 namespace ripple {
@@ -106,15 +107,19 @@ class AMMOffer
     limitOut(
         TAmounts<TIn, TOut> const& offrAmt,
         TOut const& limit,
-        bool fixReducedOffers,
+        Rules const& rules,
         bool roundUp) const;
 
     /** Limit in of the provided offer. If one-path then swapIn
      * using current balances. If multi-path then ceil_in using
      * current quality.
      */
     TAmounts<TIn, TOut>
-    limitIn(TAmounts<TIn, TOut> const& offrAmt, TIn const& limit) const;
+    limitIn(
+        TAmounts<TIn, TOut> const& offrAmt,
+        TIn const& limit,
+        Rules const& rules,
+        bool roundUp) const;
 
     QualityFunction
     getQualityFunc() const;

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

@@ -88,7 +88,7 @@ TAmounts<TIn, TOut>
 AMMOffer<TIn, TOut>::limitOut(
     TAmounts<TIn, TOut> const& offrAmt,
     TOut const& limit,
-    bool fixReducedOffers,
+    Rules const& rules,
     bool roundUp) const
 {
     // Change the offer size proportionally to the original offer quality
@@ -99,7 +99,7 @@ AMMOffer<TIn, TOut>::limitOut(
     // poolPays * poolGets < (poolPays - assetOut) * (poolGets + assetIn)
     if (ammLiquidity_.multiPath())
     {
-        if (fixReducedOffers)
+        if (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
@@ -117,11 +117,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,
+    Rules const& rules,
+    bool roundUp) const
 {
     // See the comments above in limitOut().
     if (ammLiquidity_.multiPath())
+    {
+        if (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

@@ -589,14 +589,18 @@ limitStepIn(
     TOut& ownerGives,
     std::uint32_t transferRateIn,
     std::uint32_t transferRateOut,
-    TIn const& limit)
+    TIn const& limit,
+    Rules const& rules)
 {
     if (limit < stpAmt.in)
     {
         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.  This adjustment changes
+        // transaction outcomes, so it must be made under an amendment.
+        ofrAmt = offer.limitIn(ofrAmt, inLmt, rules, /* roundUp */ false);
         stpAmt.out = ofrAmt.out;
         ownerGives = mulRatio(
             ofrAmt.out, transferRateOut, QUALITY_ONE, /*roundUp*/ false);
@@ -624,7 +628,7 @@ limitStepOut(
         ofrAmt = offer.limitOut(
             ofrAmt,
             stpAmt.out,
-            rules.enabled(fixReducedOffersV1),
+            rules,
             /*roundUp*/ true);
         stpAmt.in =
             mulRatio(ofrAmt.in, transferRateIn, QUALITY_ONE, /*roundUp*/ true);
@@ -664,7 +668,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) {
@@ -746,7 +749,7 @@ BookStep<TIn, TOut, TDerived>::forEachOffer(
             // rounding down the ceil_out() result.  This adjustment changes
             // transaction outcomes, so it must be made under an amendment.
             ofrAmt = offer.limitOut(
-                ofrAmt, stpAmt.out, fixReduced, /*roundUp*/ false);
+                ofrAmt, stpAmt.out, afView.rules(), /*roundUp*/ false);
 
             stpAmt.in =
                 mulRatio(ofrAmt.in, ofrInRate, QUALITY_ONE, /*roundUp*/ true);
@@ -1077,7 +1080,8 @@ BookStep<TIn, TOut, TDerived>::fwdImp(
                 ownerGivesAdj,
                 transferRateIn,
                 transferRateOut,
-                remainingIn);
+                remainingIn,
+                afView.rules());
             savedIns.insert(remainingIn);
             lastOut = savedOuts.insert(stpAdjAmt.out);
             result.out = sum(savedOuts);
@@ -1128,7 +1132,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,15 @@ class TOffer : private TOfferBase<TIn, TOut>
     limitOut(
         TAmounts<TIn, TOut> const& offrAmt,
         TOut const& limit,
-        bool fixReducedOffers,
+        Rules const& rules,
         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,
+        Rules const& rules,
+        bool roundUp) const;
 
     template <typename... Args>
     static TER
@@ -219,10 +224,10 @@ TAmounts<TIn, TOut>
 TOffer<TIn, TOut>::limitOut(
     TAmounts<TIn, TOut> const& offrAmt,
     TOut const& limit,
-    bool fixReducedOffers,
+    Rules const& rules,
     bool roundUp) const
 {
-    if (fixReducedOffers)
+    if (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 +238,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)
-    const
+TOffer<TIn, TOut>::limitIn(
+    TAmounts<TIn, TOut> const& offrAmt,
+    TIn const& limit,
+    Rules const& rules,
+    bool roundUp) const
 {
+    if (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 = 71;
+static constexpr std::size_t numFeatures = 72;
 
 /** Amendments that this server supports and the default voting behavior.
    Whether they are enabled depends on the Rules defined in the validated
@@ -358,6 +358,7 @@ extern uint256 const fixAMMOverflowOffer;
 extern uint256 const featurePriceOracle;
 extern uint256 const fixEmptyDID;
 extern uint256 const fixXChainRewardRounding;
+extern uint256 const fixReducedOffersV2;
 
 }  // namespace ripple
 

src/ripple/protocol/Quality.h

@@ -200,6 +200,29 @@ class Quality
             toAmount<In>(stRes.in), toAmount<Out>(stRes.out));
     }
 
+    Amounts
+    ceil_in_strict(Amounts const& amount, STAmount const& limit, bool roundUp)
+        const;
+
+    template <class In, class Out>
+    TAmounts<In, Out>
+    ceil_in_strict(
+        TAmounts<In, Out> const& amount,
+        In const& limit,
+        bool roundUp) const
+    {
+        if (amount.in <= 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));
+        auto const stRes = ceil_in_strict(stAmt, stLim, roundUp);
+        return TAmounts<In, Out>(
+            toAmount<In>(stRes.in), toAmount<Out>(stRes.out));
+    }
+
     /** Returns the scaled amount with out capped.
         Math is avoided if the result is exact. The input is clamped
         to prevent money creation.

src/ripple/protocol/impl/Feature.cpp

@@ -465,6 +465,7 @@ REGISTER_FIX    (fixAMMOverflowOffer,           Supported::yes, VoteBehavior::De
 REGISTER_FEATURE(PriceOracle,                   Supported::yes, VoteBehavior::DefaultNo);
 REGISTER_FIX    (fixEmptyDID,                   Supported::yes, VoteBehavior::DefaultNo);
 REGISTER_FIX    (fixXChainRewardRounding,       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
-Quality::ceil_in(Amounts const& amount, STAmount const& limit) const
+template <STAmount (
+    *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

@@ -3151,13 +3151,17 @@ struct AMM_test : public jtx::AMMTest
                 STAmount(USD, UINT64_C(9'970'097277662122), -12),
                 STAmount(EUR, UINT64_C(10'029'99250187452), -11),
                 ammUSD_EUR.tokens()));
-            BEAST_EXPECT(expectOffers(
-                env,
-                alice,
-                1,
-                {{Amounts{
-                    XRPAmount(30'201'749),
-                    STAmount(USD, UINT64_C(29'90272233787818), -14)}}}));
+            {
+                // fixReducedOffersV2 changes the expected results slightly.
+                Amounts const expectedAmounts =
+                    env.closed()->rules().enabled(fixReducedOffersV2)
+                    ? Amounts{XRPAmount(30'201'749), STAmount(USD, UINT64_C(29'90272233787816), -14)}
+                    : Amounts{
+                        XRPAmount(30'201'749),
+                        STAmount(USD, UINT64_C(29'90272233787818), -14)};
+
+                BEAST_EXPECT(expectOffers(env, alice, 1, {{expectedAmounts}}));
+            }
             // Initial 30,000 + 100
             BEAST_EXPECT(expectLine(env, carol, STAmount{USD, 30'100}));
             // Initial 1,000 - 30082730(AMM pool) - 70798251(offer) - 10(tx fee)

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);
@@ -518,14 +517,26 @@ struct Flow_test : public beast::unit_test::suite
             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));
 
-            env.require(balance(carol, EUR(1)));
-            env.require(balance(bob, USD(0.4)));
-            env.require(balance(bob, EUR(999)));
+            if (!reducedOffersV2)
+            {
+                env.require(balance(carol, EUR(1)));
+                env.require(balance(bob, USD(0.4)));
+                env.require(balance(bob, EUR(999)));
+            }
         }
     }
 
@@ -1375,9 +1386,12 @@ struct Flow_test : public beast::unit_test::suite
     {
         using namespace jtx;
         FeatureBitset const ownerPaysFee{featureOwnerPaysFee};
+        FeatureBitset const reducedOffersV2(fixReducedOffersV2);
 
         testLineQuality(features);
         testFalseDry(features);
+        testDirectStep(features - reducedOffersV2);
+        testBookStep(features - reducedOffersV2);
         testDirectStep(features);
         testBookStep(features);
         testDirectStep(features | ownerPaysFee);