fixReducedOffersV1: curtail the occurrence of order books that are blocked by reduced offers

Builds/CMake/RippledCore.cmake

@@ -718,6 +718,7 @@ if (tests)
     src/test/app/PseudoTx_test.cpp
     src/test/app/RCLCensorshipDetector_test.cpp
     src/test/app/RCLValidations_test.cpp
+    src/test/app/ReducedOffer_test.cpp
     src/test/app/Regression_test.cpp
     src/test/app/SHAMapStore_test.cpp
     src/test/app/SetAuth_test.cpp

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

@@ -531,14 +531,22 @@ limitStepOut(
     TOut& ownerGives,
     std::uint32_t transferRateIn,
     std::uint32_t transferRateOut,
-    TOut const& limit)
+    TOut const& limit,
+    Rules const& rules)
 {
     if (limit < stpAmt.out)
     {
         stpAmt.out = limit;
         ownerGives = mulRatio(
             stpAmt.out, transferRateOut, QUALITY_ONE, /*roundUp*/ false);
-        ofrAmt = ofrQ.ceil_out(ofrAmt, stpAmt.out);
+        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
+            // an amendment.
+            ofrAmt = ofrQ.ceil_out_strict(ofrAmt, stpAmt.out, /*roundUp*/ true);
+        else
+            ofrAmt = ofrQ.ceil_out(ofrAmt, stpAmt.out);
         stpAmt.in =
             mulRatio(ofrAmt.in, transferRateIn, QUALITY_ONE, /*roundUp*/ true);
     }
@@ -577,6 +585,7 @@ 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;
     while (offers.step())
@@ -654,7 +663,16 @@ BookStep<TIn, TOut, TDerived>::forEachOffer(
             ownerGives = funds;
             stpAmt.out = mulRatio(
                 ownerGives, QUALITY_ONE, ofrOutRate, /*roundUp*/ false);
-            ofrAmt = ofrQ->ceil_out(ofrAmt, stpAmt.out);
+
+            // 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.
+            if (fixReduced)
+                ofrAmt = ofrQ->ceil_out_strict(
+                    ofrAmt, stpAmt.out, /* roundUp */ false);
+            else
+                ofrAmt = ofrQ->ceil_out(ofrAmt, stpAmt.out);
+
             stpAmt.in =
                 mulRatio(ofrAmt.in, ofrInRate, QUALITY_ONE, /*roundUp*/ true);
         }
@@ -770,7 +788,8 @@ BookStep<TIn, TOut, TDerived>::revImp(
                 ownerGivesAdj,
                 transferRateIn,
                 transferRateOut,
-                remainingOut);
+                remainingOut,
+                afView.rules());
             remainingOut = beast::zero;
             savedIns.insert(stpAdjAmt.in);
             savedOuts.insert(remainingOut);
@@ -922,7 +941,8 @@ BookStep<TIn, TOut, TDerived>::fwdImp(
                 ownerGivesAdjRev,
                 transferRateIn,
                 transferRateOut,
-                remainingOut);
+                remainingOut,
+                afView.rules());
 
             if (stpAdjAmtRev.in == remainingIn)
             {

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

@@ -824,8 +824,22 @@ CreateOffer::flowCross(
                         // what is a good threshold to check?
                         afterCross.in.clear();
 
-                    afterCross.out = divRound(
-                        afterCross.in, rate, takerAmount.out.issue(), true);
+                    afterCross.out = [&]() {
+                        // Careful analysis showed that rounding up this
+                        // divRound result could lead to placing a reduced
+                        // offer in the ledger that blocks order books.  So
+                        // the fixReducedOffersV1 amendment changes the
+                        // behavior to round down instead.
+                        if (psb.rules().enabled(fixReducedOffersV1))
+                            return divRoundStrict(
+                                afterCross.in,
+                                rate,
+                                takerAmount.out.issue(),
+                                false);
+
+                        return divRound(
+                            afterCross.in, rate, takerAmount.out.issue(), true);
+                    }();
                 }
                 else
                 {

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

@@ -182,17 +182,33 @@ TOfferStreamBase<TIn, TOut>::shouldRmSmallIncreasedQOffer() const
     }
 
     TTakerGets const ownerFunds = toAmount<TTakerGets>(*ownerFunds_);
+    bool const fixReduced = view_.rules().enabled(fixReducedOffersV1);
 
     auto const effectiveAmounts = [&] {
         if (offer_.owner() != offer_.issueOut().account &&
             ownerFunds < ofrAmts.out)
         {
-            // adjust the amounts by owner funds
+            // adjust the amounts by owner funds.
+            //
+            // It turns out we can prevent order book blocking by rounding down
+            // the ceil_out() result.  This adjustment changes transaction
+            // results, so it must be made under an amendment.
+            if (fixReduced)
+                return offer_.quality().ceil_out_strict(
+                    ofrAmts, ownerFunds, /* roundUp */ false);
+
             return offer_.quality().ceil_out(ofrAmts, ownerFunds);
         }
         return ofrAmts;
     }();
 
+    // If either the effective in or out are zero then remove the offer.
+    // This can happen with fixReducedOffersV1 since it rounds down.
+    if (fixReduced &&
+        (effectiveAmounts.in.signum() <= 0 ||
+         effectiveAmounts.out.signum() <= 0))
+        return true;
+
     if (effectiveAmounts.in > TTakerPays::minPositiveAmount())
         return false;
 

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 = 58;
+static constexpr std::size_t numFeatures = 59;
 
 /** Amendments that this server supports and the default voting behavior.
    Whether they are enabled depends on the Rules defined in the validated
@@ -345,6 +345,7 @@ extern uint256 const featureXRPFees;
 extern uint256 const fixUniversalNumber;
 extern uint256 const fixNonFungibleTokensV1_2;
 extern uint256 const fixNFTokenRemint;
+extern uint256 const fixReducedOffersV1;
 
 }  // namespace ripple
 

src/ripple/protocol/Quality.h

@@ -223,6 +223,29 @@ class Quality
             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>
+    ceil_out_strict(
+        TAmounts<In, Out> const& amount,
+        Out const& limit,
+        bool roundUp) const
+    {
+        if (amount.out <= 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_out_strict(stAmt, stLim, roundUp);
+        return TAmounts<In, Out>(
+            toAmount<In>(stRes.in), toAmount<Out>(stRes.out));
+    }
+
     /** 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.

src/ripple/protocol/STAmount.h

@@ -503,21 +503,38 @@ divide(STAmount const& v1, STAmount const& v2, Issue const& issue);
 STAmount
 multiply(STAmount const& v1, STAmount const& v2, Issue const& issue);
 
-// multiply, or divide rounding result in specified direction
+// multiply rounding result in specified direction
 STAmount
 mulRound(
     STAmount const& v1,
     STAmount const& v2,
     Issue const& issue,
     bool roundUp);
 
+// multiply following the rounding directions more precisely.
+STAmount
+mulRoundStrict(
+    STAmount const& v1,
+    STAmount const& v2,
+    Issue const& issue,
+    bool roundUp);
+
+// divide rounding result in specified direction
 STAmount
 divRound(
     STAmount const& v1,
     STAmount const& v2,
     Issue const& issue,
     bool roundUp);
 
+// divide following the rounding directions more precisely.
+STAmount
+divRoundStrict(
+    STAmount const& v1,
+    STAmount const& v2,
+    Issue const& issue,
+    bool roundUp);
+
 // Someone is offering X for Y, what is the rate?
 // Rate: smaller is better, the taker wants the most out: in/out
 // VFALCO TODO Return a Quality object

src/ripple/protocol/impl/Feature.cpp

@@ -452,6 +452,7 @@ REGISTER_FEATURE(XRPFees,                       Supported::yes, VoteBehavior::De
 REGISTER_FIX    (fixUniversalNumber,            Supported::yes, VoteBehavior::DefaultNo);
 REGISTER_FIX    (fixNonFungibleTokensV1_2,      Supported::yes, VoteBehavior::DefaultNo);
 REGISTER_FIX    (fixNFTokenRemint,              Supported::yes, VoteBehavior::DefaultNo);
+REGISTER_FIX    (fixReducedOffersV1,            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

@@ -81,12 +81,20 @@ Quality::ceil_in(Amounts const& amount, STAmount const& limit) const
     return amount;
 }
 
-Amounts
-Quality::ceil_out(Amounts const& amount, STAmount const& limit) const
+template <STAmount (
+    *MulRoundFunc)(STAmount const&, STAmount const&, Issue const&, bool)>
+static Amounts
+ceil_out_impl(
+    Amounts const& amount,
+    STAmount const& limit,
+    bool roundUp,
+    Quality const& quality)
 {
     if (amount.out > limit)
     {
-        Amounts result(mulRound(limit, rate(), amount.in.issue(), true), limit);
+        Amounts result(
+            MulRoundFunc(limit, quality.rate(), amount.in.issue(), roundUp),
+            limit);
         // Clamp in
         if (result.in > amount.in)
             result.in = amount.in;
@@ -97,6 +105,21 @@ Quality::ceil_out(Amounts const& amount, STAmount const& limit) const
     return amount;
 }
 
+Amounts
+Quality::ceil_out(Amounts const& amount, STAmount const& limit) const
+{
+    return ceil_out_impl<mulRound>(amount, limit, /* roundUp */ true, *this);
+}
+
+Amounts
+Quality::ceil_out_strict(
+    Amounts const& amount,
+    STAmount const& limit,
+    bool roundUp) const
+{
+    return ceil_out_impl<mulRoundStrict>(amount, limit, roundUp, *this);
+}
+
 Quality
 composed_quality(Quality const& lhs, Quality const& rhs)
 {