fix: improper handling of large synthetic AMM offers:

A large synthetic offer was not handled correctly in the payment engine.
This patch fixes that issue and introduces a new invariant check while
processing synthetic offers.

Author: gregtatcam

src/ripple/app/misc/AMMHelpers.h

@@ -134,7 +134,7 @@ withinRelativeDistance(
 template <typename Amt>
     requires(
         std::is_same_v<Amt, STAmount> || std::is_same_v<Amt, IOUAmount> ||
-        std::is_same_v<Amt, XRPAmount>)
+        std::is_same_v<Amt, XRPAmount> || std::is_same_v<Amt, Number>)
 bool
 withinRelativeDistance(Amt const& calc, Amt const& req, Number const& dist)
 {

src/ripple/app/paths/AMMLiquidity.h

@@ -137,10 +137,17 @@ class AMMLiquidity
     TAmounts<TIn, TOut>
     generateFibSeqOffer(TAmounts<TIn, TOut> const& balances) const;
 
-    /** Generate max offer
+    /** Generate max offer.
+     * If `fixAMMOverflowOffer` is active, the offer is generated as:
+     * takerGets = 99% * balances.out takerPays = swapOut(takerGets).
+     * Return nullopt if takerGets is 0 or takerGets == balances.out.
+     *
+     * If `fixAMMOverflowOffer` is not active, the offer is generated as:
+     * takerPays = max input amount;
+     * takerGets = swapIn(takerPays).
      */
-    AMMOffer<TIn, TOut>
-    maxOffer(TAmounts<TIn, TOut> const& balances) const;
+    std::optional<AMMOffer<TIn, TOut>>
+    maxOffer(TAmounts<TIn, TOut> const& balances, Rules const& rules) const;
 };
 
 }  // namespace ripple

src/ripple/app/paths/AMMOffer.h

@@ -50,9 +50,8 @@ class AMMOffer
     // the swap out of the entire side of the pool, in which case
     // the swap in amount is infinite.
     TAmounts<TIn, TOut> const amounts_;
-    // If seated then current pool balances. Used in one-path limiting steps
-    // to swap in/out.
-    std::optional<TAmounts<TIn, TOut>> const balances_;
+    // Current pool balances.
+    TAmounts<TIn, TOut> const balances_;
     // The Spot Price quality if balances != amounts
     // else the amounts quality
     Quality const quality_;
@@ -63,7 +62,7 @@ class AMMOffer
     AMMOffer(
         AMMLiquidity<TIn, TOut> const& ammLiquidity,
         TAmounts<TIn, TOut> const& amounts,
-        std::optional<TAmounts<TIn, TOut>> const& balances,
+        TAmounts<TIn, TOut> const& balances,
         Quality const& quality);
 
     Quality
@@ -142,6 +141,12 @@ class AMMOffer
         // AMM doesn't pay transfer fee on Payment tx
         return {ofrInRate, QUALITY_ONE};
     }
+
+    /** Check the new pool product is greater or equal to the old pool
+     * product or if decreases then within some threshold.
+     */
+    bool
+    checkInvariant(TAmounts<TIn, TOut> const& consumed, beast::Journal j) const;
 };
 
 }  // namespace ripple

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

@@ -93,6 +93,7 @@ AMMLiquidity<TIn, TOut>::generateFibSeqOffer(
     return cur;
 }
 
+namespace {
 template <typename T>
 constexpr T
 maxAmount()
@@ -105,16 +106,41 @@ maxAmount()
         return STAmount(STAmount::cMaxValue / 2, STAmount::cMaxOffset);
 }
 
+template <typename T>
+T
+maxOut(T const& out, Issue const& iss)
+{
+    Number const res = out * Number{99, -2};
+    return toAmount<T>(iss, res, Number::rounding_mode::downward);
+}
+}  // namespace
+
 template <typename TIn, typename TOut>
-AMMOffer<TIn, TOut>
-AMMLiquidity<TIn, TOut>::maxOffer(TAmounts<TIn, TOut> const& balances) const
+std::optional<AMMOffer<TIn, TOut>>
+AMMLiquidity<TIn, TOut>::maxOffer(
+    TAmounts<TIn, TOut> const& balances,
+    Rules const& rules) const
 {
-    return AMMOffer<TIn, TOut>(
-        *this,
-        {maxAmount<TIn>(),
-         swapAssetIn(balances, maxAmount<TIn>(), tradingFee_)},
-        balances,
-        Quality{balances});
+    if (!rules.enabled(fixAMMOverflowOffer))
+    {
+        return AMMOffer<TIn, TOut>(
+            *this,
+            {maxAmount<TIn>(),
+             swapAssetIn(balances, maxAmount<TIn>(), tradingFee_)},
+            balances,
+            Quality{balances});
+    }
+    else
+    {
+        auto const out = maxOut<TOut>(balances.out, issueOut());
+        if (out <= TOut{0} || out >= balances.out)
+            return std::nullopt;
+        return AMMOffer<TIn, TOut>(
+            *this,
+            {swapAssetOut(balances, out, tradingFee_), out},
+            balances,
+            Quality{balances});
+    }
 }
 
 template <typename TIn, typename TOut>
@@ -167,15 +193,16 @@ AMMLiquidity<TIn, TOut>::getOffer(
                 if (clobQuality && Quality{amounts} < clobQuality)
                     return std::nullopt;
                 return AMMOffer<TIn, TOut>(
-                    *this, amounts, std::nullopt, Quality{amounts});
+                    *this, amounts, balances, Quality{amounts});
             }
             else if (!clobQuality)
             {
                 // If there is no CLOB to compare against, return the largest
                 // amount, which doesn't overflow. The size is going to be
                 // changed in BookStep per either deliver amount limit, or
-                // sendmax, or available output or input funds.
-                return maxOffer(balances);
+                // sendmax, or available output or input funds. Might return
+                // nullopt if the pool is small.
+                return maxOffer(balances, view.rules());
             }
             else if (
                 auto const amounts =
@@ -188,7 +215,10 @@ AMMLiquidity<TIn, TOut>::getOffer(
         catch (std::overflow_error const& e)
         {
             JLOG(j_.error()) << "AMMLiquidity::getOffer overflow " << e.what();
-            return maxOffer(balances);
+            if (!view.rules().enabled(fixAMMOverflowOffer))
+                return maxOffer(balances, view.rules());
+            else
+                return std::nullopt;
         }
         catch (std::exception const& e)
         {

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

@@ -27,7 +27,7 @@ template <typename TIn, typename TOut>
 AMMOffer<TIn, TOut>::AMMOffer(
     AMMLiquidity<TIn, TOut> const& ammLiquidity,
     TAmounts<TIn, TOut> const& amounts,
-    std::optional<TAmounts<TIn, TOut>> const& balances,
+    TAmounts<TIn, TOut> const& balances,
     Quality const& quality)
     : ammLiquidity_(ammLiquidity)
     , amounts_(amounts)
@@ -110,7 +110,7 @@ AMMOffer<TIn, TOut>::limitOut(
     // Change the offer size according to the conservation function. The offer
     // quality is increased in this case, but it doesn't matter since there is
     // only one path.
-    return {swapAssetOut(*balances_, limit, ammLiquidity_.tradingFee()), limit};
+    return {swapAssetOut(balances_, limit, ammLiquidity_.tradingFee()), limit};
 }
 
 template <typename TIn, typename TOut>
@@ -122,7 +122,7 @@ AMMOffer<TIn, TOut>::limitIn(
     // See the comments above in limitOut().
     if (ammLiquidity_.multiPath())
         return quality().ceil_in(offrAmt, limit);
-    return {limit, swapAssetIn(*balances_, limit, ammLiquidity_.tradingFee())};
+    return {limit, swapAssetIn(balances_, limit, ammLiquidity_.tradingFee())};
 }
 
 template <typename TIn, typename TOut>
@@ -132,7 +132,45 @@ AMMOffer<TIn, TOut>::getQualityFunc() const
     if (ammLiquidity_.multiPath())
         return QualityFunction{quality(), QualityFunction::CLOBLikeTag{}};
     return QualityFunction{
-        *balances_, ammLiquidity_.tradingFee(), QualityFunction::AMMTag{}};
+        balances_, ammLiquidity_.tradingFee(), QualityFunction::AMMTag{}};
+}
+
+template <typename TIn, typename TOut>
+bool
+AMMOffer<TIn, TOut>::checkInvariant(
+    TAmounts<TIn, TOut> const& consumed,
+    beast::Journal j) const
+{
+    if (consumed.in > amounts_.in || consumed.out > amounts_.out)
+    {
+        JLOG(j.error()) << "AMMOffer::checkInvariant failed: consumed "
+                        << to_string(consumed.in) << " "
+                        << to_string(consumed.out) << " amounts "
+                        << to_string(amounts_.in) << " "
+                        << to_string(amounts_.out);
+
+        return false;
+    }
+
+    Number const product = balances_.in * balances_.out;
+    auto const newBalances = TAmounts<TIn, TOut>{
+        balances_.in + consumed.in, balances_.out - consumed.out};
+    Number const newProduct = newBalances.in * newBalances.out;
+
+    if (newProduct >= product ||
+        withinRelativeDistance(product, newProduct, Number{1, -7}))
+        return true;
+
+    JLOG(j.error()) << "AMMOffer::checkInvariant failed: balances "
+                    << to_string(balances_.in) << " "
+                    << to_string(balances_.out) << " new balances "
+                    << to_string(newBalances.in) << " "
+                    << to_string(newBalances.out) << " product/newProduct "
+                    << product << " " << newProduct << " diff "
+                    << (product != Number{0}
+                            ? to_string((product - newProduct) / product)
+                            : "undefined");
+    return false;
 }
 
 template class AMMOffer<STAmount, STAmount>;

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

@@ -793,6 +793,17 @@ BookStep<TIn, TOut, TDerived>::consumeOffer(
     TAmounts<TIn, TOut> const& stepAmt,
     TOut const& ownerGives) const
 {
+    if (!offer.checkInvariant(ofrAmt, j_))
+    {
+        // purposely written as separate if statements so we get logging even
+        // when the amendment isn't active.
+        if (sb.rules().enabled(fixAMMOverflowOffer))
+        {
+            Throw<FlowException>(
+                tecINVARIANT_FAILED, "AMM pool product invariant failed.");
+        }
+    }
+
     // The offer owner gets the ofrAmt. The difference between ofrAmt and
     // stepAmt is a transfer fee that goes to book_.in.account
     {

src/ripple/app/tx/impl/Offer.h

@@ -163,6 +163,15 @@ class TOffer : private TOfferBase<TIn, TOut>
         // CLOB offer pays the transfer fee
         return {ofrInRate, ofrOutRate};
     }
+
+    /** Check any required invariant. Limit order book offer
+     * always returns true.
+     */
+    bool
+    checkInvariant(TAmounts<TIn, TOut> const&, beast::Journal j) const
+    {
+        return true;
+    }
 };
 
 using Offer = TOffer<>;

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 = 67;
+static constexpr std::size_t numFeatures = 68;
 
 /** Amendments that this server supports and the default voting behavior.
    Whether they are enabled depends on the Rules defined in the validated
@@ -354,6 +354,7 @@ extern uint256 const featureDID;
 extern uint256 const fixFillOrKill;
 extern uint256 const fixNFTokenReserve;
 extern uint256 const fixInnerObjTemplate;
+extern uint256 const fixAMMOverflowOffer;
 
 }  // namespace ripple
 

src/ripple/protocol/impl/Feature.cpp

@@ -460,7 +460,8 @@ REGISTER_FIX    (fixDisallowIncomingV1,         Supported::yes, VoteBehavior::De
 REGISTER_FEATURE(DID,                           Supported::yes, VoteBehavior::DefaultNo);
 REGISTER_FIX(fixFillOrKill,                     Supported::yes, VoteBehavior::DefaultNo);
 REGISTER_FIX    (fixNFTokenReserve,             Supported::yes, VoteBehavior::DefaultNo);
-REGISTER_FIX(fixInnerObjTemplate,               Supported::yes, VoteBehavior::DefaultNo);
+REGISTER_FIX    (fixInnerObjTemplate,           Supported::yes, VoteBehavior::DefaultNo);
+REGISTER_FIX    (fixAMMOverflowOffer,           Supported::yes, VoteBehavior::DefaultYes);
 
 // The following amendments are obsolete, but must remain supported
 // because they could potentially get enabled.