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.

Author: scottschurr

src/ripple/app/paths/AMMOffer.h

@@ -103,15 +103,15 @@ class AMMOffer
     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
      * 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, 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,
-    Rules const& rules)
+    TOut const& limit)
 {
     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, stpAmt.out, fixReduced, /*roundUp*/ false);
+            ofrAmt = offer.limitOut(ofrAmt, stpAmt.out, /*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,
-                afView.rules());
+                remainingOut);
             remainingOut = beast::zero;
             savedIns.insert(stpAdjAmt.in);
             savedOuts.insert(remainingOut);
@@ -1208,8 +1210,7 @@ BookStep<TIn, TOut, TDerived>::fwdImp(
                 ownerGivesAdjRev,
                 transferRateIn,
                 transferRateOut,
-                remainingOut,
-                afView.rules());
+                remainingOut);
 
             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 @@ class TOffer : private TOfferBase<TIn, TOut>
     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)
-    const
+TOffer<TIn, TOut>::limitIn(
+    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