fixReducedOffersV1: prevent offers from blocking order books: (XRPL…

…F#4512)

Curtail the occurrence of order books that are blocked by reduced offers
with the implementation of the fixReducedOffersV1 amendment.

This commit identifies three ways in which offers can be reduced:

1. A new offer can be partially crossed by existing offers, so the new
   offer is reduced when placed in the ledger.

2. An in-ledger offer can be partially crossed by a new offer in a
   transaction. So the in-ledger offer is reduced by the new offer.

3. An in-ledger offer may be under-funded. In this case the in-ledger
   offer is scaled down to match the available funds.

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.

This commit adjusts some rounding modes so that the quality of a reduced
offer is always at least as good (from the taker's perspective) as the
original offer.

The amendment is titled fixReducedOffersV1 because additional ways of
producing reduced offers may come to light. Therefore, there may be a
future need for a V2 amendment.

Builds/CMake/RippledCore.cmake

@@ -721,6 +721,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)
 {