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 {
@@ -103,15 +104,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

@@ -81,7 +81,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
@@ -92,7 +92,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
@@ -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,
+    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

@@ -661,14 +661,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);
@@ -696,7 +700,7 @@ limitStepOut(
         ofrAmt = offer.limitOut(
             ofrAmt,
             stpAmt.out,
-            rules.enabled(fixReducedOffersV1),
+            rules,
             /*roundUp*/ true);
         stpAmt.in =
             mulRatio(ofrAmt.in, transferRateIn, QUALITY_ONE, /*roundUp*/ true);
@@ -736,7 +740,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) {
@@ -818,7 +821,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);
@@ -1177,7 +1180,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);
@@ -1228,7 +1232,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 = 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

@@ -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

@@ -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
-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

@@ -3618,27 +3618,37 @@ 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}}));
             }
             else
             {
                 BEAST_EXPECT(ammUSD_EUR.expectBalances(
                     STAmount(USD, UINT64_C(9'970'097277662172), -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'9027223378284), -13)}}}));
+
+                // fixReducedOffersV2 changes the expected results slightly.
+                Amounts const expectedAmounts =
+                    env.closed()->rules().enabled(fixReducedOffersV2)
+                    ? Amounts{
+                          XRPAmount(30'201'749),
+                          STAmount(USD, UINT64_C(29'90272233782839), -14)}
+                    : Amounts{
+                          XRPAmount(30'201'749),
+                          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 +6884,8 @@ struct AMM_test : public jtx::AMMTest
         testInvalidAMMPayment();
         testBasicPaymentEngine(all);
         testBasicPaymentEngine(all - fixAMMv1_1);
+        testBasicPaymentEngine(all - fixReducedOffersV2);
+        testBasicPaymentEngine(all - fixAMMv1_1 - fixReducedOffersV2);
         testAMMTokens();
         testAmendment();
         testFlags();