Merge pull request #4968 from XRPLF/master

Merging changes for 2.1.1 from master into develop
XRPLF · Mar 28, 2024 · bcbf6c1 · bcbf6c1
2 parents 4bcbf70 + 2d1854f
commit bcbf6c1
Show file tree

Hide file tree

Showing 10 changed files with 148 additions and 25 deletions.
diff --git a/RELEASENOTES.md b/RELEASENOTES.md
@@ -6,6 +6,27 @@ This document contains the release notes for `rippled`, the reference server imp
 
 Have new ideas? Need help with setting up your node? [Please open an issue here](https://github.com/xrplf/rippled/issues/new/choose).
 
+## Version 2.1.1
+
+The `rippled` 2.1.1 release fixes a critical bug in the integration of AMMs with the payment engine.
+
+[Sign Up for Future Release Announcements](https://groups.google.com/g/ripple-server)
+
+<!-- BREAK -->
+
+
+## Action Required
+
+One new amendment is now open for voting according to the XRP Ledger's [amendment process](https://xrpl.org/amendments.html), which enables protocol changes following two weeks of >80% support from trusted validators.
+
+If you operate an XRP Ledger server, upgrade to version 2.1.1 by April 8, 2024 to ensure service continuity. The exact time that protocol changes take effect depends on the voting decisions of the decentralized network.
+
+## Changelog
+
+### Amendments
+
+- **fixAMMOverflowOffer**: Fix improper handling of large synthetic AMM offers in the payment engine. Due to the importance of this fix, the default vote in the source code has been set to YES. For information on how to configure your validator's amendment voting, see [Configure Amendment Voting](https://xrpl.org/docs/infrastructure/configuration/configure-amendment-voting).
+
 # Introducing XRP Ledger version 2.1.0
 
 Version 2.1.0 of `rippled`, the reference server implementation of the XRP Ledger protocol, is now available. This release adds a bug fix, build improvements, and introduces the `fixNFTokenReserve` and `fixInnerObjTemplate` amendments.

diff --git a/src/ripple/app/misc/AMMHelpers.h b/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)
 {

diff --git a/src/ripple/app/paths/AMMLiquidity.h b/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

diff --git a/src/ripple/app/paths/AMMOffer.h b/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

diff --git a/src/ripple/app/paths/impl/AMMLiquidity.cpp b/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)
         {

diff --git a/src/ripple/app/paths/impl/AMMOffer.cpp b/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>;

diff --git a/src/ripple/app/paths/impl/BookStep.cpp b/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
     {

diff --git a/src/ripple/app/tx/impl/Offer.h b/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<>;

diff --git a/src/ripple/protocol/Feature.h b/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 = 70;
+static constexpr std::size_t numFeatures = 71;
 
 /** 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;
 extern uint256 const featurePriceOracle;
 extern uint256 const fixEmptyDID;
 extern uint256 const fixXChainRewardRounding;

diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp
@@ -461,6 +461,7 @@ REGISTER_FEATURE(DID,                           Supported::yes, VoteBehavior::De
 REGISTER_FIX    (fixFillOrKill,                 Supported::yes, VoteBehavior::DefaultNo);
 REGISTER_FIX    (fixNFTokenReserve,             Supported::yes, VoteBehavior::DefaultNo);
 REGISTER_FIX    (fixInnerObjTemplate,           Supported::yes, VoteBehavior::DefaultNo);
+REGISTER_FIX    (fixAMMOverflowOffer,           Supported::yes, VoteBehavior::DefaultYes);
 REGISTER_FEATURE(PriceOracle,                   Supported::yes, VoteBehavior::DefaultNo);
 REGISTER_FIX    (fixEmptyDID,                   Supported::yes, VoteBehavior::DefaultNo);
 REGISTER_FIX    (fixXChainRewardRounding,       Supported::yes, VoteBehavior::DefaultNo);