From a7c4a47723c21e2fc100ddb00294659eb59f588e Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Sun, 24 Mar 2024 13:00:43 -0400 Subject: [PATCH 1/2] 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. --- src/ripple/app/misc/AMMHelpers.h | 2 +- src/ripple/app/paths/AMMLiquidity.h | 13 +++-- src/ripple/app/paths/AMMOffer.h | 13 +++-- src/ripple/app/paths/impl/AMMLiquidity.cpp | 56 +++++++++++++++++----- src/ripple/app/paths/impl/AMMOffer.cpp | 46 ++++++++++++++++-- src/ripple/app/paths/impl/BookStep.cpp | 11 +++++ src/ripple/app/tx/impl/Offer.h | 9 ++++ src/ripple/protocol/Feature.h | 3 +- src/ripple/protocol/impl/Feature.cpp | 3 +- 9 files changed, 129 insertions(+), 27 deletions(-) diff --git a/src/ripple/app/misc/AMMHelpers.h b/src/ripple/app/misc/AMMHelpers.h index 24c2592280..e84c6c535e 100644 --- a/src/ripple/app/misc/AMMHelpers.h +++ b/src/ripple/app/misc/AMMHelpers.h @@ -134,7 +134,7 @@ withinRelativeDistance( template requires( std::is_same_v || std::is_same_v || - std::is_same_v) + std::is_same_v || std::is_same_v) 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 index 7757bd4684..f1be112b2d 100644 --- a/src/ripple/app/paths/AMMLiquidity.h +++ b/src/ripple/app/paths/AMMLiquidity.h @@ -137,10 +137,17 @@ private: TAmounts generateFibSeqOffer(TAmounts 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 - maxOffer(TAmounts const& balances) const; + std::optional> + maxOffer(TAmounts const& balances, Rules const& rules) const; }; } // namespace ripple diff --git a/src/ripple/app/paths/AMMOffer.h b/src/ripple/app/paths/AMMOffer.h index 10e6017dd9..426ba96a77 100644 --- a/src/ripple/app/paths/AMMOffer.h +++ b/src/ripple/app/paths/AMMOffer.h @@ -50,9 +50,8 @@ private: // the swap out of the entire side of the pool, in which case // the swap in amount is infinite. TAmounts const amounts_; - // If seated then current pool balances. Used in one-path limiting steps - // to swap in/out. - std::optional> const balances_; + // Current pool balances. + TAmounts const balances_; // The Spot Price quality if balances != amounts // else the amounts quality Quality const quality_; @@ -63,7 +62,7 @@ public: AMMOffer( AMMLiquidity const& ammLiquidity, TAmounts const& amounts, - std::optional> const& balances, + TAmounts const& balances, Quality const& quality); Quality @@ -142,6 +141,12 @@ public: // 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 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 index 3f22ebacec..bcc086e23d 100644 --- a/src/ripple/app/paths/impl/AMMLiquidity.cpp +++ b/src/ripple/app/paths/impl/AMMLiquidity.cpp @@ -93,6 +93,7 @@ AMMLiquidity::generateFibSeqOffer( return cur; } +namespace { template constexpr T maxAmount() @@ -105,16 +106,41 @@ maxAmount() return STAmount(STAmount::cMaxValue / 2, STAmount::cMaxOffset); } -template -AMMOffer -AMMLiquidity::maxOffer(TAmounts const& balances) const +template +T +maxOut(T const& out, Issue const& iss) { - return AMMOffer( - *this, - {maxAmount(), - swapAssetIn(balances, maxAmount(), tradingFee_)}, - balances, - Quality{balances}); + Number const res = out * Number{99, -2}; + return toAmount(iss, res, Number::rounding_mode::downward); +} +} // namespace + +template +std::optional> +AMMLiquidity::maxOffer( + TAmounts const& balances, + Rules const& rules) const +{ + if (!rules.enabled(fixAMMOverflowOffer)) + { + return AMMOffer( + *this, + {maxAmount(), + swapAssetIn(balances, maxAmount(), tradingFee_)}, + balances, + Quality{balances}); + } + else + { + auto const out = maxOut(balances.out, issueOut()); + if (out <= TOut{0} || out >= balances.out) + return std::nullopt; + return AMMOffer( + *this, + {swapAssetOut(balances, out, tradingFee_), out}, + balances, + Quality{balances}); + } } template @@ -167,15 +193,16 @@ AMMLiquidity::getOffer( if (clobQuality && Quality{amounts} < clobQuality) return std::nullopt; return AMMOffer( - *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::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 index 10b75b7856..759851b7af 100644 --- a/src/ripple/app/paths/impl/AMMOffer.cpp +++ b/src/ripple/app/paths/impl/AMMOffer.cpp @@ -27,7 +27,7 @@ template AMMOffer::AMMOffer( AMMLiquidity const& ammLiquidity, TAmounts const& amounts, - std::optional> const& balances, + TAmounts const& balances, Quality const& quality) : ammLiquidity_(ammLiquidity) , amounts_(amounts) @@ -110,7 +110,7 @@ AMMOffer::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 @@ -122,7 +122,7 @@ AMMOffer::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 @@ -132,7 +132,45 @@ AMMOffer::getQualityFunc() const if (ammLiquidity_.multiPath()) return QualityFunction{quality(), QualityFunction::CLOBLikeTag{}}; return QualityFunction{ - *balances_, ammLiquidity_.tradingFee(), QualityFunction::AMMTag{}}; + balances_, ammLiquidity_.tradingFee(), QualityFunction::AMMTag{}}; +} + +template +bool +AMMOffer::checkInvariant( + TAmounts 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{ + 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; diff --git a/src/ripple/app/paths/impl/BookStep.cpp b/src/ripple/app/paths/impl/BookStep.cpp index dd6abe577f..358dac4c79 100644 --- a/src/ripple/app/paths/impl/BookStep.cpp +++ b/src/ripple/app/paths/impl/BookStep.cpp @@ -793,6 +793,17 @@ BookStep::consumeOffer( TAmounts 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( + 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 index 027030ec8d..bdae4d2b15 100644 --- a/src/ripple/app/tx/impl/Offer.h +++ b/src/ripple/app/tx/impl/Offer.h @@ -163,6 +163,15 @@ public: // CLOB offer pays the transfer fee return {ofrInRate, ofrOutRate}; } + + /** Check any required invariant. Limit order book offer + * always returns true. + */ + bool + checkInvariant(TAmounts const&, beast::Journal j) const + { + return true; + } }; using Offer = TOffer<>; diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index 8e6483b1db..a1e34952ea 100644 --- 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 = 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 diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index ab36983edd..bf6706b118 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/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. From 2d1854f354ff8bb2b5671fd51252c5acd837c433 Mon Sep 17 00:00:00 2001 From: seelabs Date: Tue, 26 Mar 2024 18:13:57 -0400 Subject: [PATCH 2/2] Set version to 2.1.1 --- RELEASENOTES.md | 21 +++++++++++++++++++++ src/ripple/protocol/impl/BuildInfo.cpp | 2 +- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 5288e12aed..7c924209ef 100644 --- 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) + + + + +## 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/protocol/impl/BuildInfo.cpp b/src/ripple/protocol/impl/BuildInfo.cpp index c6d61864d5..b7803b7a44 100644 --- a/src/ripple/protocol/impl/BuildInfo.cpp +++ b/src/ripple/protocol/impl/BuildInfo.cpp @@ -33,7 +33,7 @@ namespace BuildInfo { // and follow the format described at http://semver.org/ //------------------------------------------------------------------------------ // clang-format off -char const* const versionString = "2.1.0" +char const* const versionString = "2.1.1" // clang-format on #if defined(DEBUG) || defined(SANITIZER)