diff --git a/include/xrpl/ledger/helpers/AMMHelpers.h b/include/xrpl/ledger/helpers/AMMHelpers.h index d21e50e7cb..de8bb9d3f7 100644 --- a/include/xrpl/ledger/helpers/AMMHelpers.h +++ b/include/xrpl/ledger/helpers/AMMHelpers.h @@ -37,6 +37,8 @@ reduceOffer(auto const& amount) enum class IsDeposit : bool { No = false, Yes = true }; +inline Number const kAMMInvariantRelativeTolerance{1, -11}; + /** Calculate LP Tokens given AMM pool reserves. * @param asset1 AMM one side of the pool reserve * @param asset2 AMM another side of the pool reserve @@ -738,6 +740,30 @@ ammPoolHolds( AuthHandling authHandling, beast::Journal const j); +/** Check AMM pool product invariant after an AMM operation that changes LP tokens + * (deposit/withdraw/clawback) from an already calculated pool product mean. + * Returns tecPRECISION_LOSS if poolProductMean < newLPTokenBalance beyond the + * invariant tolerance, + * tesSUCCESS otherwise. Skips check when newLPTokenBalance is zero (last withdrawal). + */ +TER +checkAMMPrecisionLoss(Number const& poolProductMean, STAmount const& newLPTokenBalance); + +/** Check AMM pool product invariant after an AMM operation that changes LP tokens + * (deposit/withdraw/clawback). + * Returns tecPRECISION_LOSS if sqrt(asset1 * asset2) < newLPTokenBalance beyond + * the invariant tolerance, + * tesSUCCESS otherwise. Skips check when newLPTokenBalance is zero (last withdrawal). + */ +TER +checkAMMPrecisionLoss( + ReadView const& view, + AccountID const& ammAccountID, + Asset const& asset1, + Asset const& asset2, + STAmount const& newLPTokenBalance, + beast::Journal const j); + /** Get AMM pool and LP token balances. If both optIssue are * provided then they are used as the AMM token pair issues. * Otherwise the missing issues are fetched from ammSle. diff --git a/src/libxrpl/ledger/helpers/AMMHelpers.cpp b/src/libxrpl/ledger/helpers/AMMHelpers.cpp index a59b8e4436..cacbfc9d58 100644 --- a/src/libxrpl/ledger/helpers/AMMHelpers.cpp +++ b/src/libxrpl/ledger/helpers/AMMHelpers.cpp @@ -433,6 +433,43 @@ ammPoolHolds( return std::make_pair(assetInBalance, assetOutBalance); } +TER +checkAMMPrecisionLoss(Number const& poolProductMean, STAmount const& newLPTokenBalance) +{ + if (newLPTokenBalance <= beast::kZero) + return tesSUCCESS; + if (poolProductMean >= newLPTokenBalance) + return tesSUCCESS; + // Strong check failed. Allow the same relative tolerance as the invariant + // checker's weak check. Only return tecPRECISION_LOSS when both fail. + if (withinRelativeDistance( + poolProductMean, Number{newLPTokenBalance}, kAMMInvariantRelativeTolerance)) + return tesSUCCESS; + return tecPRECISION_LOSS; +} + +TER +checkAMMPrecisionLoss( + ReadView const& view, + AccountID const& ammAccountID, + Asset const& asset1, + Asset const& asset2, + STAmount const& newLPTokenBalance, + beast::Journal const j) +{ + if (newLPTokenBalance <= beast::kZero) + return tesSUCCESS; + auto const [amount, amount2] = ammPoolHolds( + view, + ammAccountID, + asset1, + asset2, + FreezeHandling::IgnoreFreeze, + AuthHandling::IgnoreAuth, + j); + return checkAMMPrecisionLoss(root2(amount * amount2), newLPTokenBalance); +} + std::expected, TER> ammHolds( ReadView const& view, diff --git a/src/libxrpl/tx/invariants/AMMInvariant.cpp b/src/libxrpl/tx/invariants/AMMInvariant.cpp index 356c26d6b0..cca0ce149c 100644 --- a/src/libxrpl/tx/invariants/AMMInvariant.cpp +++ b/src/libxrpl/tx/invariants/AMMInvariant.cpp @@ -270,13 +270,8 @@ ValidAMM::generalInvariant( auto const poolProductMean = root2(amount * amount2); bool const nonNegativeBalances = validBalances(amount, amount2, *lptAMMBalanceAfter_, zeroAllowed); - bool const strongInvariantCheck = poolProductMean >= *lptAMMBalanceAfter_; - // Allow for a small relative error if strongInvariantCheck fails - auto weakInvariantCheck = [&]() { - return *lptAMMBalanceAfter_ != beast::kZero && - withinRelativeDistance(poolProductMean, Number{*lptAMMBalanceAfter_}, Number{1, -11}); - }; - if (!nonNegativeBalances || (!strongInvariantCheck && !weakInvariantCheck())) + auto const precisionLoss = checkAMMPrecisionLoss(poolProductMean, *lptAMMBalanceAfter_); + if (!nonNegativeBalances || !isTesSuccess(precisionLoss)) { JLOG(j.error()) << "Invariant failed: AMM " << tx.getTxnType() << " " << tx.getHash(HashPrefix::TransactionId) << " " << ammPoolChanged_ << " " diff --git a/src/libxrpl/tx/transactors/dex/AMMClawback.cpp b/src/libxrpl/tx/transactors/dex/AMMClawback.cpp index b94e97e931..0cc2be381f 100644 --- a/src/libxrpl/tx/transactors/dex/AMMClawback.cpp +++ b/src/libxrpl/tx/transactors/dex/AMMClawback.cpp @@ -258,6 +258,16 @@ AMMClawback::applyGuts(Sandbox& sb) if (!isTesSuccess(result)) return result; // LCOV_EXCL_LINE + if (sb.rules().enabled(fixCleanup3_3_0) && sb.rules().enabled(fixAMMv1_3)) + { + if (auto const ter = + checkAMMPrecisionLoss(sb, ammAccount, asset, asset2, newLPTokenBalance, j_); + !isTesSuccess(ter)) + { + return ter; + } + } + auto const res = AMMWithdraw::deleteAMMAccountIfEmpty(sb, ammSle, newLPTokenBalance, asset, asset2, j_); if (!res.second) diff --git a/src/libxrpl/tx/transactors/dex/AMMDeposit.cpp b/src/libxrpl/tx/transactors/dex/AMMDeposit.cpp index 91858e3cd7..653e8c6961 100644 --- a/src/libxrpl/tx/transactors/dex/AMMDeposit.cpp +++ b/src/libxrpl/tx/transactors/dex/AMMDeposit.cpp @@ -470,6 +470,19 @@ AMMDeposit::applyGuts(Sandbox& sb) XRPL_ASSERT( newLPTokenBalance > beast::kZero, "xrpl::AMMDeposit::applyGuts : valid new LP token balance"); + // Defensive check: deposit formulas with fixAMMv1_3 round LP tokens + // down and asset amounts up, so sqrt(pool1*pool2) >= newLPTokenBalance + // is guaranteed to hold. A precision loss failure is not expected. + if (sb.rules().enabled(fixCleanup3_3_0) && sb.rules().enabled(fixAMMv1_3)) + { + if (auto const ter = checkAMMPrecisionLoss( + sb, ammAccountID, ctx_.tx[sfAsset], ctx_.tx[sfAsset2], newLPTokenBalance, j_); + !isTesSuccess(ter)) + { + UNREACHABLE("xrpl::AMMDeposit::applyGuts : AMM precision loss"); + return {ter, false}; // LCOV_EXCL_LINE + } + } ammSle->setFieldAmount(sfLPTokenBalance, newLPTokenBalance); // LP depositing into AMM empty state gets the auction slot // and the voting diff --git a/src/libxrpl/tx/transactors/dex/AMMWithdraw.cpp b/src/libxrpl/tx/transactors/dex/AMMWithdraw.cpp index d3a6c9c74c..17ce1a6b83 100644 --- a/src/libxrpl/tx/transactors/dex/AMMWithdraw.cpp +++ b/src/libxrpl/tx/transactors/dex/AMMWithdraw.cpp @@ -406,6 +406,16 @@ AMMWithdraw::applyGuts(Sandbox& sb) if (!isTesSuccess(result)) return {result, false}; + if (sb.rules().enabled(fixCleanup3_3_0) && sb.rules().enabled(fixAMMv1_3)) + { + if (auto const ter = checkAMMPrecisionLoss( + sb, ammAccountID, ctx_.tx[sfAsset], ctx_.tx[sfAsset2], newLPTokenBalance, j_); + !isTesSuccess(ter)) + { + return {ter, false}; + } + } + auto const res = deleteAMMAccountIfEmpty( sb, ammSle, newLPTokenBalance, ctx_.tx[sfAsset], ctx_.tx[sfAsset2], j_); // LCOV_EXCL_START diff --git a/src/test/app/AMMClawback_test.cpp b/src/test/app/AMMClawback_test.cpp index 9683e8ac17..ba416d8192 100644 --- a/src/test/app/AMMClawback_test.cpp +++ b/src/test/app/AMMClawback_test.cpp @@ -2486,8 +2486,17 @@ class AMMClawback_test : public beast::unit_test::Suite else if (!features[fixAMMClawbackRounding]) { // sqrt(amount * amount2) >= LPTokens and exceeds the allowed - // tolerance - env(amm::ammClawback(gw, alice, usd, eur, usd(1)), Ter(tecINVARIANT_FAILED)); + // tolerance. + // With fixCleanup3_3_0 this is caught in the transaction layer; + // without it the invariant checker fires instead. + if (features[fixCleanup3_3_0]) + { + env(amm::ammClawback(gw, alice, usd, eur, usd(1)), Ter(tecPRECISION_LOSS)); + } + else + { + env(amm::ammClawback(gw, alice, usd, eur, usd(1)), Ter(tecINVARIANT_FAILED)); + } BEAST_EXPECT(amm.ammExists()); } else if (features[fixAMMv1_3] && features[fixAMMClawbackRounding]) @@ -2514,6 +2523,11 @@ class AMMClawback_test : public beast::unit_test::Suite testFeatureDisabled(all - featureAMMClawback); for (auto const& features : {all - fixAMMv1_3 - fixAMMClawbackRounding - featureMPTokensV2, + // fixAMMv1_3 on, fixAMMClawbackRounding off, fixCleanup3_3_0 off: + // precision loss caught by invariant checker -> tecINVARIANT_FAILED + all - fixAMMClawbackRounding - fixCleanup3_3_0 - featureMPTokensV2, + // fixAMMv1_3 on, fixAMMClawbackRounding off, fixCleanup3_3_0 on: + // precision loss caught in transaction layer -> tecPRECISION_LOSS all - fixAMMClawbackRounding - featureMPTokensV2, all - featureMPTokensV2, all}) diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 1b54c2aab9..b01d58ddff 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -1842,8 +1842,18 @@ private: // are rounded to all LP tokens. testAMM( [&](AMM& ammAlice, Env& env) { - auto const err = - env.enabled(fixAMMv1_3) ? Ter(tecINVARIANT_FAILED) : Ter(tecAMM_BALANCE); + // Without fixAMMv1_3: sub-method returns tecAMM_BALANCE early. + // With fixAMMv1_3 but without fixCleanup3_3_0: sub-method succeeds + // but invariant check catches the precision violation. + // With fixCleanup3_3_0: caught in the transaction layer before + // the invariant checker runs. + auto const err = [&] { + if (!env.enabled(fixAMMv1_3)) + return Ter(tecAMM_BALANCE); + if (env.enabled(fixCleanup3_3_0)) + return Ter(tecPRECISION_LOSS); + return Ter(tecINVARIANT_FAILED); + }(); ammAlice.withdraw( alice_, STAmount{USD, UINT64_C(9'999'999999999999), -12}, @@ -1851,7 +1861,7 @@ private: std::nullopt, err); }, - {.features = {all, all - fixAMMv1_3}, .noLog = true}); + {.features = {all, all - fixAMMv1_3, all - fixCleanup3_3_0}, .noLog = true}); // Tiny withdraw testAMM([&](AMM& ammAlice, Env&) {