diff --git a/src/ripple/app/paths/AMMOffer.h b/src/ripple/app/paths/AMMOffer.h index e3fb41e220b..e47a5613b09 100644 --- a/src/ripple/app/paths/AMMOffer.h +++ b/src/ripple/app/paths/AMMOffer.h @@ -103,7 +103,6 @@ class AMMOffer limitOut( TAmounts const& offrAmt, TOut const& limit, - bool fixReducedOffers, bool roundUp) const; /** Limit in of the provided offer. If one-path then swapIn @@ -111,7 +110,8 @@ class AMMOffer * current quality. */ TAmounts - limitIn(TAmounts const& offrAmt, TIn const& limit) const; + limitIn(TAmounts const& offrAmt, TIn const& limit, bool roundUp) + const; QualityFunction getQualityFunc() const; diff --git a/src/ripple/app/paths/impl/AMMOffer.cpp b/src/ripple/app/paths/impl/AMMOffer.cpp index 697bac9c790..bf85ead6194 100644 --- a/src/ripple/app/paths/impl/AMMOffer.cpp +++ b/src/ripple/app/paths/impl/AMMOffer.cpp @@ -81,7 +81,6 @@ TAmounts AMMOffer::limitOut( TAmounts const& offrAmt, TOut const& limit, - bool fixReducedOffers, bool roundUp) const { // Change the offer size proportionally to the original offer quality @@ -92,7 +91,8 @@ AMMOffer::limitOut( // poolPays * poolGets < (poolPays - assetOut) * (poolGets + assetIn) if (ammLiquidity_.multiPath()) { - if (fixReducedOffers) + if (auto const& rules = getCurrentTransactionRules(); + rules && 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 @@ -110,11 +110,18 @@ template TAmounts AMMOffer::limitIn( TAmounts const& offrAmt, - TIn const& limit) const + TIn const& limit, + bool roundUp) const { // See the comments above in limitOut(). if (ammLiquidity_.multiPath()) + { + if (auto const& rules = getCurrentTransactionRules(); + rules && rules->enabled(fixReducedOffersV2)) + return quality().ceil_in_strict(offrAmt, limit, roundUp); + return quality().ceil_in(offrAmt, limit); + } return {limit, swapAssetIn(balances_, limit, ammLiquidity_.tradingFee())}; } diff --git a/src/ripple/app/paths/impl/BookStep.cpp b/src/ripple/app/paths/impl/BookStep.cpp index 4a43d653e0c..af0c40b1925 100644 --- a/src/ripple/app/paths/impl/BookStep.cpp +++ b/src/ripple/app/paths/impl/BookStep.cpp @@ -668,7 +668,14 @@ limitStepIn( stpAmt.in = limit; auto const inLmt = mulRatio(stpAmt.in, QUALITY_ONE, transferRateIn, /*roundUp*/ false); - ofrAmt = offer.limitIn(ofrAmt, inLmt); + // It turns out we can prevent order book blocking by (strictly) + // rounding down the ceil_in() result. By rounding down we guarantee + // that the quality of an offer left in the ledger is as good or + // better than the quality of the containing order book page. + // + // This adjustment changes transaction outcomes, so it must be made + // under an amendment. + ofrAmt = offer.limitIn(ofrAmt, inLmt, /* roundUp */ false); stpAmt.out = ofrAmt.out; ownerGives = mulRatio( ofrAmt.out, transferRateOut, QUALITY_ONE, /*roundUp*/ false); @@ -685,8 +692,7 @@ limitStepOut( TOut& ownerGives, std::uint32_t transferRateIn, std::uint32_t transferRateOut, - TOut const& limit, - Rules const& rules) + TOut const& limit) { if (limit < stpAmt.out) { @@ -696,7 +702,6 @@ limitStepOut( ofrAmt = offer.limitOut( ofrAmt, stpAmt.out, - rules.enabled(fixReducedOffersV1), /*roundUp*/ true); stpAmt.in = mulRatio(ofrAmt.in, transferRateIn, QUALITY_ONE, /*roundUp*/ true); @@ -736,7 +741,6 @@ BookStep::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 ofrQ; auto execOffer = [&](auto& offer) { @@ -817,8 +821,7 @@ BookStep::forEachOffer( // 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. - ofrAmt = offer.limitOut( - ofrAmt, stpAmt.out, fixReduced, /*roundUp*/ false); + ofrAmt = offer.limitOut(ofrAmt, stpAmt.out, /*roundUp*/ false); stpAmt.in = mulRatio(ofrAmt.in, ofrInRate, QUALITY_ONE, /*roundUp*/ true); @@ -1055,8 +1058,7 @@ BookStep::revImp( ownerGivesAdj, transferRateIn, transferRateOut, - remainingOut, - afView.rules()); + remainingOut); remainingOut = beast::zero; savedIns.insert(stpAdjAmt.in); savedOuts.insert(remainingOut); @@ -1208,8 +1210,7 @@ BookStep::fwdImp( ownerGivesAdjRev, transferRateIn, transferRateOut, - remainingOut, - afView.rules()); + remainingOut); if (stpAdjAmtRev.in == remainingIn) { @@ -1228,7 +1229,7 @@ BookStep::fwdImp( } else { - // This is (likely) a problem case, and wil be caught + // This is (likely) a problem case, and will be caught // with later checks savedOuts.insert(lastOutAmt); } diff --git a/src/ripple/app/tx/impl/Offer.h b/src/ripple/app/tx/impl/Offer.h index bdae4d2b155..53253426c40 100644 --- a/src/ripple/app/tx/impl/Offer.h +++ b/src/ripple/app/tx/impl/Offer.h @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include @@ -140,11 +141,11 @@ class TOffer : private TOfferBase limitOut( TAmounts const& offrAmt, TOut const& limit, - bool fixReducedOffers, bool roundUp) const; TAmounts - limitIn(TAmounts const& offrAmt, TIn const& limit) const; + limitIn(TAmounts const& offrAmt, TIn const& limit, bool roundUp) + const; template static TER @@ -219,10 +220,10 @@ TAmounts TOffer::limitOut( TAmounts const& offrAmt, TOut const& limit, - bool fixReducedOffers, bool roundUp) const { - if (fixReducedOffers) + if (auto const& rules = getCurrentTransactionRules(); + rules && 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 @@ -233,9 +234,18 @@ TOffer::limitOut( template TAmounts -TOffer::limitIn(TAmounts const& offrAmt, TIn const& limit) - const +TOffer::limitIn( + TAmounts const& offrAmt, + TIn const& limit, + bool roundUp) const { + if (auto const& rules = getCurrentTransactionRules(); + rules && rules->enabled(fixReducedOffersV2)) + // It turns out that the ceil_in implementation has some slop in + // it. ceil_in_strict removes that slop. But removing that slop + // affects transaction outcomes, so the change must be made using + // an amendment. + return quality().ceil_in_strict(offrAmt, limit, roundUp); return m_quality.ceil_in(offrAmt, limit); } diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index 57cd9513eea..91cf4d97ca4 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 = 73; +static constexpr std::size_t numFeatures = 74; /** Amendments that this server supports and the default voting behavior. Whether they are enabled depends on the Rules defined in the validated @@ -360,6 +360,7 @@ extern uint256 const fixEmptyDID; extern uint256 const fixXChainRewardRounding; extern uint256 const fixPreviousTxnID; extern uint256 const fixAMMv1_1; +extern uint256 const fixReducedOffersV2; } // namespace ripple diff --git a/src/ripple/protocol/Quality.h b/src/ripple/protocol/Quality.h index 840d8d444e1..06b8254df5e 100644 --- a/src/ripple/protocol/Quality.h +++ b/src/ripple/protocol/Quality.h @@ -181,71 +181,71 @@ class Quality Math is avoided if the result is exact. The output is clamped to prevent money creation. */ - Amounts + [[nodiscard]] Amounts ceil_in(Amounts const& amount, STAmount const& limit) const; template - TAmounts - ceil_in(TAmounts const& amount, In const& limit) const - { - if (amount.in <= 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_in(stAmt, stLim); - return TAmounts( - toAmount(stRes.in), toAmount(stRes.out)); - } + [[nodiscard]] TAmounts + ceil_in(TAmounts const& amount, In const& limit) const; + + // Some of the underlying rounding functions called by ceil_in() ignored + // low order bits that could influence rounding decisions. This "strict" + // method uses underlying functions that pay attention to all the bits. + [[nodiscard]] Amounts + ceil_in_strict(Amounts const& amount, STAmount const& limit, bool roundUp) + const; + + template + [[nodiscard]] TAmounts + ceil_in_strict( + TAmounts const& amount, + In const& limit, + bool roundUp) const; /** Returns the scaled amount with out capped. Math is avoided if the result is exact. The input is clamped to prevent money creation. */ - Amounts + [[nodiscard]] Amounts ceil_out(Amounts const& amount, STAmount const& limit) const; template - TAmounts - ceil_out(TAmounts const& amount, Out const& limit) 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(stAmt, stLim); - return TAmounts( - toAmount(stRes.in), toAmount(stRes.out)); - } + [[nodiscard]] TAmounts + ceil_out(TAmounts const& amount, Out const& limit) const; - Amounts + // Some of the underlying rounding functions called by ceil_out() ignored + // low order bits that could influence rounding decisions. This "strict" + // method uses underlying functions that pay attention to all the bits. + [[nodiscard]] Amounts ceil_out_strict(Amounts const& amount, STAmount const& limit, bool roundUp) const; template - TAmounts + [[nodiscard]] TAmounts ceil_out_strict( TAmounts 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( - toAmount(stRes.in), toAmount(stRes.out)); - } + bool roundUp) const; +private: + // The ceil_in and ceil_out methods that deal in TAmount all convert + // their arguments to STAoumout and convert the result back to TAmount. + // This helper function takes care of all the conversion operations. + template < + class In, + class Out, + class Lim, + typename FnPtr, + std::same_as... Round> + [[nodiscard]] TAmounts + ceil_TAmounts_helper( + TAmounts const& amount, + Lim const& limit, + Lim const& limit_cmp, + FnPtr ceil_function, + Round... round) const; + +public: /** 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. @@ -327,6 +327,84 @@ class Quality } }; +template < + class In, + class Out, + class Lim, + typename FnPtr, + std::same_as... Round> +TAmounts +Quality::ceil_TAmounts_helper( + TAmounts const& amount, + Lim const& limit, + Lim const& limit_cmp, + FnPtr ceil_function, + Round... roundUp) const +{ + if (limit_cmp <= 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)); + Amounts const stRes = ((*this).*ceil_function)(stAmt, stLim, roundUp...); + return TAmounts(toAmount(stRes.in), toAmount(stRes.out)); +} + +template +TAmounts +Quality::ceil_in(TAmounts const& amount, In const& limit) const +{ + // Construct a function pointer to the function we want to call. + static constexpr Amounts (Quality::*ceil_in_fn_ptr)( + Amounts const&, STAmount const&) const = &Quality::ceil_in; + + return ceil_TAmounts_helper(amount, limit, amount.in, ceil_in_fn_ptr); +} + +template +TAmounts +Quality::ceil_in_strict( + TAmounts const& amount, + In const& limit, + bool roundUp) const +{ + // Construct a function pointer to the function we want to call. + static constexpr Amounts (Quality::*ceil_in_fn_ptr)( + Amounts const&, STAmount const&, bool) const = &Quality::ceil_in_strict; + + return ceil_TAmounts_helper( + amount, limit, amount.in, ceil_in_fn_ptr, roundUp); +} + +template +TAmounts +Quality::ceil_out(TAmounts const& amount, Out const& limit) const +{ + // Construct a function pointer to the function we want to call. + static constexpr Amounts (Quality::*ceil_out_fn_ptr)( + Amounts const&, STAmount const&) const = &Quality::ceil_out; + + return ceil_TAmounts_helper(amount, limit, amount.out, ceil_out_fn_ptr); +} + +template +TAmounts +Quality::ceil_out_strict( + TAmounts const& amount, + Out const& limit, + bool roundUp) const +{ + // Construct a function pointer to the function we want to call. + static constexpr Amounts (Quality::*ceil_out_fn_ptr)( + Amounts const&, STAmount const&, bool) const = + &Quality::ceil_out_strict; + + return ceil_TAmounts_helper( + amount, limit, amount.out, ceil_out_fn_ptr, roundUp); +} + /** Calculate the quality of a two-hop path given the two hops. @param lhs The first leg of the path: input to intermediate. @param rhs The second leg of the path: intermediate to output. diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index ae7a8291bb4..4c347375766 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -467,6 +467,7 @@ REGISTER_FIX (fixEmptyDID, Supported::yes, VoteBehavior::De REGISTER_FIX (fixXChainRewardRounding, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FIX (fixPreviousTxnID, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FIX (fixAMMv1_1, Supported::yes, VoteBehavior::DefaultNo); +REGISTER_FIX (fixReducedOffersV2, Supported::yes, VoteBehavior::DefaultNo); // The following amendments are obsolete, but must remain supported // because they could potentially get enabled. diff --git a/src/ripple/protocol/impl/Quality.cpp b/src/ripple/protocol/impl/Quality.cpp index f7b9d6b3c41..2cb60f738d8 100644 --- a/src/ripple/protocol/impl/Quality.cpp +++ b/src/ripple/protocol/impl/Quality.cpp @@ -64,13 +64,20 @@ Quality::operator--(int) return prev; } -Amounts -Quality::ceil_in(Amounts const& amount, STAmount const& limit) const +template +static Amounts +ceil_in_impl( + Amounts const& amount, + STAmount const& limit, + bool roundUp, + Quality const& quality) { if (amount.in > limit) { Amounts result( - limit, divRound(limit, rate(), amount.out.issue(), true)); + limit, + DivRoundFunc(limit, quality.rate(), amount.out.issue(), roundUp)); // Clamp out if (result.out > amount.out) result.out = amount.out; @@ -81,6 +88,21 @@ Quality::ceil_in(Amounts const& amount, STAmount const& limit) const return amount; } +Amounts +Quality::ceil_in(Amounts const& amount, STAmount const& limit) const +{ + return ceil_in_impl(amount, limit, /* roundUp */ true, *this); +} + +Amounts +Quality::ceil_in_strict( + Amounts const& amount, + STAmount const& limit, + bool roundUp) const +{ + return ceil_in_impl(amount, limit, roundUp, *this); +} + template static Amounts diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index b4abd385257..a3c4ae9800c 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -3618,13 +3618,16 @@ struct AMM_test : public jtx::AMMTest STAmount(USD, UINT64_C(9'970'097277662122), -12), STAmount(EUR, UINT64_C(10'029'99250187452), -11), ammUSD_EUR.tokens())); - BEAST_EXPECT(expectOffers( - env, - alice, - 1, - {{Amounts{ - XRPAmount(30'201'749), - STAmount(USD, UINT64_C(29'90272233787818), -14)}}})); + + // fixReducedOffersV2 changes the expected results slightly. + Amounts const expectedAmounts = + env.closed()->rules().enabled(fixReducedOffersV2) + ? Amounts{XRPAmount(30'201'749), STAmount(USD, UINT64_C(29'90272233787816), -14)} + : Amounts{ + XRPAmount(30'201'749), + STAmount(USD, UINT64_C(29'90272233787818), -14)}; + + BEAST_EXPECT(expectOffers(env, alice, 1, {{expectedAmounts}})); } else { @@ -3632,13 +3635,16 @@ struct AMM_test : public jtx::AMMTest STAmount(USD, UINT64_C(9'970'097277662172), -12), STAmount(EUR, UINT64_C(10'029'99250187452), -11), ammUSD_EUR.tokens())); - BEAST_EXPECT(expectOffers( - env, - alice, - 1, - {{Amounts{ - XRPAmount(30'201'749), - STAmount(USD, UINT64_C(29'9027223378284), -13)}}})); + + // fixReducedOffersV2 changes the expected results slightly. + Amounts const expectedAmounts = + env.closed()->rules().enabled(fixReducedOffersV2) + ? Amounts{XRPAmount(30'201'749), STAmount(USD, UINT64_C(29'90272233782839), -14)} + : Amounts{ + XRPAmount(30'201'749), + STAmount(USD, UINT64_C(29'90272233782840), -14)}; + + BEAST_EXPECT(expectOffers(env, alice, 1, {{expectedAmounts}})); } // Initial 30,000 + 100 BEAST_EXPECT(expectLine(env, carol, STAmount{USD, 30'100})); @@ -6874,6 +6880,8 @@ struct AMM_test : public jtx::AMMTest testInvalidAMMPayment(); testBasicPaymentEngine(all); testBasicPaymentEngine(all - fixAMMv1_1); + testBasicPaymentEngine(all - fixReducedOffersV2); + testBasicPaymentEngine(all - fixAMMv1_1 - fixReducedOffersV2); testAMMTokens(); testAmendment(); testFlags(); diff --git a/src/test/app/Flow_test.cpp b/src/test/app/Flow_test.cpp index 920f7a6e058..bb7b96f1072 100644 --- a/src/test/app/Flow_test.cpp +++ b/src/test/app/Flow_test.cpp @@ -506,7 +506,6 @@ struct Flow_test : public beast::unit_test::suite // Without limits, the 0.4 USD would produce 1000 EUR in the forward // pass. This test checks that the payment produces 1 EUR, as // expected. - Env env(*this, features); env.fund(XRP(10000), alice, bob, carol, gw); env.trust(USD(1000), alice, bob, carol); @@ -515,17 +514,52 @@ struct Flow_test : public beast::unit_test::suite env(pay(gw, alice, USD(1000))); env(pay(gw, bob, EUR(1000))); + Keylet const bobUsdOffer = keylet::offer(bob, env.seq(bob)); env(offer(bob, USD(1), drops(2)), txflags(tfPassive)); env(offer(bob, drops(1), EUR(1000)), txflags(tfPassive)); + bool const reducedOffersV2 = features[fixReducedOffersV2]; + + // With reducedOffersV2, it is not allowed to accept less than + // USD(0.5) of bob's USD offer. If we provide 1 drop for less + // than USD(0.5), then the remaining fractional offer would + // block the order book. + TER const expectedTER = + reducedOffersV2 ? TER(tecPATH_DRY) : TER(tesSUCCESS); env(pay(alice, carol, EUR(1)), path(~XRP, ~EUR), sendmax(USD(0.4)), - txflags(tfNoRippleDirect | tfPartialPayment)); + txflags(tfNoRippleDirect | tfPartialPayment), + ter(expectedTER)); + + if (!reducedOffersV2) + { + env.require(balance(carol, EUR(1))); + env.require(balance(bob, USD(0.4))); + env.require(balance(bob, EUR(999))); - env.require(balance(carol, EUR(1))); - env.require(balance(bob, USD(0.4))); - env.require(balance(bob, EUR(999))); + // Show that bob's USD offer is now a blocker. + std::shared_ptr const usdOffer = env.le(bobUsdOffer); + if (BEAST_EXPECT(usdOffer)) + { + std::uint64_t const bookRate = [&usdOffer]() { + // Extract the least significant 64 bits from the + // book page. That's where the quality is stored. + std::string bookDirStr = + to_string(usdOffer->at(sfBookDirectory)); + bookDirStr.erase(0, 48); + return std::stoull(bookDirStr, nullptr, 16); + }(); + std::uint64_t const actualRate = getRate( + usdOffer->at(sfTakerGets), usdOffer->at(sfTakerPays)); + + // We expect the actual rate of the offer to be worse + // (larger) than the rate of the book page holding the + // offer. This is a defect which is corrected by + // fixReducedOffersV2. + BEAST_EXPECT(actualRate > bookRate); + } + } } } @@ -1375,9 +1409,11 @@ struct Flow_test : public beast::unit_test::suite { using namespace jtx; FeatureBitset const ownerPaysFee{featureOwnerPaysFee}; + FeatureBitset const reducedOffersV2(fixReducedOffersV2); testLineQuality(features); testFalseDry(features); + testBookStep(features - reducedOffersV2); testDirectStep(features); testBookStep(features); testDirectStep(features | ownerPaysFee); diff --git a/src/test/app/Offer_test.cpp b/src/test/app/Offer_test.cpp index 95ffd9f3aee..add436d5c59 100644 --- a/src/test/app/Offer_test.cpp +++ b/src/test/app/Offer_test.cpp @@ -1387,78 +1387,106 @@ class OfferBaseUtil_test : public beast::unit_test::suite using namespace jtx; - Env env{*this, features}; + // This is one of the few tests where fixReducedOffersV2 changes the + // results. So test both with and without fixReducedOffersV2. + for (FeatureBitset localFeatures : + {features - fixReducedOffersV2, features | fixReducedOffersV2}) + { + Env env{*this, localFeatures}; - auto const gw = Account{"gateway"}; - auto const alice = Account{"alice"}; - auto const bob = Account{"bob"}; - auto const USD = gw["USD"]; - auto const BTC = gw["BTC"]; + auto const gw = Account{"gateway"}; + auto const alice = Account{"alice"}; + auto const bob = Account{"bob"}; + auto const USD = gw["USD"]; + auto const BTC = gw["BTC"]; - // these *interesting* amounts were taken - // from the original JS test that was ported here - auto const gw_initial_balance = drops(1149999730); - auto const alice_initial_balance = drops(499946999680); - auto const bob_initial_balance = drops(10199999920); - auto const small_amount = - STAmount{bob["USD"].issue(), UINT64_C(2710505431213761), -33}; + // these *interesting* amounts were taken + // from the original JS test that was ported here + auto const gw_initial_balance = drops(1149999730); + auto const alice_initial_balance = drops(499946999680); + auto const bob_initial_balance = drops(10199999920); + auto const small_amount = + STAmount{bob["USD"].issue(), UINT64_C(2710505431213761), -33}; - env.fund(gw_initial_balance, gw); - env.fund(alice_initial_balance, alice); - env.fund(bob_initial_balance, bob); + env.fund(gw_initial_balance, gw); + env.fund(alice_initial_balance, alice); + env.fund(bob_initial_balance, bob); - env(rate(gw, 1.005)); + env(rate(gw, 1.005)); - env(trust(alice, USD(500))); - env(trust(bob, USD(50))); - env(trust(gw, alice["USD"](100))); + env(trust(alice, USD(500))); + env(trust(bob, USD(50))); + env(trust(gw, alice["USD"](100))); - env(pay(gw, alice, alice["USD"](50))); - env(pay(gw, bob, small_amount)); + env(pay(gw, alice, alice["USD"](50))); + env(pay(gw, bob, small_amount)); - env(offer(alice, USD(50), XRP(150000))); + env(offer(alice, USD(50), XRP(150000))); - // unfund the offer - env(pay(alice, gw, USD(100))); + // unfund the offer + env(pay(alice, gw, USD(100))); - // drop the trust line (set to 0) - env(trust(gw, alice["USD"](0))); + // drop the trust line (set to 0) + env(trust(gw, alice["USD"](0))); - // verify balances - auto jrr = ledgerEntryState(env, alice, gw, "USD"); - BEAST_EXPECT(jrr[jss::node][sfBalance.fieldName][jss::value] == "50"); + // verify balances + auto jrr = ledgerEntryState(env, alice, gw, "USD"); + BEAST_EXPECT( + jrr[jss::node][sfBalance.fieldName][jss::value] == "50"); - jrr = ledgerEntryState(env, bob, gw, "USD"); - BEAST_EXPECT( - jrr[jss::node][sfBalance.fieldName][jss::value] == - "-2710505431213761e-33"); + jrr = ledgerEntryState(env, bob, gw, "USD"); + BEAST_EXPECT( + jrr[jss::node][sfBalance.fieldName][jss::value] == + "-2710505431213761e-33"); - // create crossing offer - env(offer(bob, XRP(2000), USD(1))); + // create crossing offer + std::uint32_t const bobOfferSeq = env.seq(bob); + env(offer(bob, XRP(2000), USD(1))); - // verify balances again. - // - // NOTE : - // Here a difference in the rounding modes of our two offer crossing - // algorithms becomes apparent. The old offer crossing would consume - // small_amount and transfer no XRP. The new offer crossing transfers - // a single drop, rather than no drops. - auto const crossingDelta = - features[featureFlowCross] ? drops(1) : drops(0); + if (localFeatures[featureFlowCross] && + localFeatures[fixReducedOffersV2]) + { + // With the rounding introduced by fixReducedOffersV2, bob's + // offer does not cross alice's offer and goes straight into + // the ledger. + jrr = ledgerEntryState(env, bob, gw, "USD"); + BEAST_EXPECT( + jrr[jss::node][sfBalance.fieldName][jss::value] == + "-2710505431213761e-33"); + + Json::Value const bobOffer = + ledgerEntryOffer(env, bob, bobOfferSeq)[jss::node]; + BEAST_EXPECT(bobOffer[sfTakerGets.jsonName][jss::value] == "1"); + BEAST_EXPECT(bobOffer[sfTakerPays.jsonName] == "2000000000"); + return; + } - jrr = ledgerEntryState(env, alice, gw, "USD"); - BEAST_EXPECT(jrr[jss::node][sfBalance.fieldName][jss::value] == "50"); - BEAST_EXPECT( - env.balance(alice, xrpIssue()) == - alice_initial_balance - env.current()->fees().base * 3 - - crossingDelta); + // verify balances again. + // + // NOTE: + // Here a difference in the rounding modes of our two offer + // crossing algorithms becomes apparent. The old offer crossing + // would consume small_amount and transfer no XRP. The new offer + // crossing transfers a single drop, rather than no drops. + auto const crossingDelta = + localFeatures[featureFlowCross] ? drops(1) : drops(0); + + jrr = ledgerEntryState(env, alice, gw, "USD"); + BEAST_EXPECT( + jrr[jss::node][sfBalance.fieldName][jss::value] == "50"); + BEAST_EXPECT( + env.balance(alice, xrpIssue()) == + alice_initial_balance - env.current()->fees().base * 3 - + crossingDelta); - jrr = ledgerEntryState(env, bob, gw, "USD"); - BEAST_EXPECT(jrr[jss::node][sfBalance.fieldName][jss::value] == "0"); - BEAST_EXPECT( - env.balance(bob, xrpIssue()) == - bob_initial_balance - env.current()->fees().base * 2 + - crossingDelta); + jrr = ledgerEntryState(env, bob, gw, "USD"); + BEAST_EXPECT( + jrr[jss::node][sfBalance.fieldName][jss::value] == "0"); + BEAST_EXPECT( + env.balance(bob, xrpIssue()) == + bob_initial_balance - env.current()->fees().base * 2 + + crossingDelta); + } } void diff --git a/src/test/app/ReducedOffer_test.cpp b/src/test/app/ReducedOffer_test.cpp index f82efcb7fc8..186162d5023 100644 --- a/src/test/app/ReducedOffer_test.cpp +++ b/src/test/app/ReducedOffer_test.cpp @@ -22,6 +22,8 @@ #include #include +#include + namespace ripple { namespace test { @@ -56,13 +58,11 @@ class ReducedOffer_test : public beast::unit_test::suite static void cleanupOldOffers( jtx::Env& env, - jtx::Account const& acct1, - jtx::Account const& acct2, - std::uint32_t acct1OfferSeq, - std::uint32_t acct2OfferSeq) + std::initializer_list> + list) { - env(offer_cancel(acct1, acct1OfferSeq)); - env(offer_cancel(acct2, acct2OfferSeq)); + for (auto [acct, offerSeq] : list) + env(offer_cancel(acct, offerSeq)); env.close(); } @@ -180,7 +180,8 @@ class ReducedOffer_test : public beast::unit_test::suite // In preparation for the next iteration make sure the two // offers are gone from the ledger. - cleanupOldOffers(env, alice, bob, aliceOfferSeq, bobOfferSeq); + cleanupOldOffers( + env, {{alice, aliceOfferSeq}, {bob, bobOfferSeq}}); return badRate; }; @@ -279,7 +280,7 @@ class ReducedOffer_test : public beast::unit_test::suite // If the in-ledger offer was not consumed then further // results are meaningless. cleanupOldOffers( - env, alice, bob, aliceOfferSeq, bobOfferSeq); + env, {{alice, aliceOfferSeq}, {bob, bobOfferSeq}}); return 1; } // alice's offer should still be in the ledger, but reduced in @@ -337,7 +338,8 @@ class ReducedOffer_test : public beast::unit_test::suite // In preparation for the next iteration make sure the two // offers are gone from the ledger. - cleanupOldOffers(env, alice, bob, aliceOfferSeq, bobOfferSeq); + cleanupOldOffers( + env, {{alice, aliceOfferSeq}, {bob, bobOfferSeq}}); return badRate; }; @@ -452,7 +454,7 @@ class ReducedOffer_test : public beast::unit_test::suite // In preparation for the next iteration clean up any // leftover offers. cleanupOldOffers( - env, alice, bob, aliceOfferSeq, bobOfferSeq); + env, {{alice, aliceOfferSeq}, {bob, bobOfferSeq}}); // Zero out alice's and bob's USD balances. if (STAmount const aliceBalance = env.balance(alice, USD); @@ -573,7 +575,8 @@ class ReducedOffer_test : public beast::unit_test::suite // In preparation for the next iteration clean up any // leftover offers. - cleanupOldOffers(env, alice, bob, aliceOfferSeq, bobOfferSeq); + cleanupOldOffers( + env, {{alice, aliceOfferSeq}, {bob, bobOfferSeq}}); // Zero out alice's and bob's IOU balances. auto zeroBalance = [&env, &gw]( @@ -606,6 +609,185 @@ class ReducedOffer_test : public beast::unit_test::suite } } + Amounts + jsonOfferToAmounts(Json::Value const& json) + { + STAmount const in = + amountFromJson(sfTakerPays, json[sfTakerPays.jsonName]); + STAmount const out = + amountFromJson(sfTakerGets, json[sfTakerGets.jsonName]); + return {in, out}; + } + + void + testSellPartialCrossOldXrpIouQChange() + { + // This test case was motivated by Issue #4937. It recreates + // the specific failure identified in that issue and samples some other + // cases in the same vicinity to make sure that the new behavior makes + // sense. + testcase("exercise tfSell partial cross old XRP/IOU offer Q change"); + + using namespace jtx; + + Account const gw("gateway"); + Account const alice("alice"); + Account const bob("bob"); + Account const carol("carol"); + auto const USD = gw["USD"]; + + // Make one test run without fixReducedOffersV2 and one with. + for (FeatureBitset features : + {supported_amendments() - fixReducedOffersV2, + supported_amendments() | fixReducedOffersV2}) + { + // Make sure none of the offers we generate are under funded. + Env env{*this, features}; + env.fund(XRP(10'000'000), gw, alice, bob, carol); + env.close(); + + env(trust(alice, USD(10'000'000))); + env(trust(bob, USD(10'000'000))); + env(trust(carol, USD(10'000'000))); + env.close(); + + env(pay(gw, alice, USD(10'000'000))); + env(pay(gw, bob, USD(10'000'000))); + env(pay(gw, carol, USD(10'000'000))); + env.close(); + + // Lambda that: + // 1. Exercises one offer trio, + // 2. Collects the results, and + // 3. Cleans up for the next offer trio. + auto exerciseOfferTrio = + [this, &env, &alice, &bob, &carol, &USD]( + Amounts const& carolOffer) -> unsigned int { + // alice submits an offer that may become a blocker. + std::uint32_t const aliceOfferSeq = env.seq(alice); + static Amounts const aliceInitialOffer(USD(2), drops(3382562)); + env(offer(alice, aliceInitialOffer.in, aliceInitialOffer.out)); + env.close(); + STAmount const initialRate = + Quality(jsonOfferToAmounts(ledgerEntryOffer( + env, alice, aliceOfferSeq)[jss::node])) + .rate(); + + // bob submits an offer that is more desirable than alice's + std::uint32_t const bobOfferSeq = env.seq(bob); + env(offer(bob, USD(0.97086565812384), drops(1642020))); + env.close(); + + // Now carol's offer consumes bob's and partially crosses + // alice's. The tfSell flag is important. + std::uint32_t const carolOfferSeq = env.seq(carol); + env(offer(carol, carolOffer.in, carolOffer.out), + txflags(tfSell)); + env.close(); + + // carol's offer should not have made it into the ledger and + // bob's offer should be fully consumed. + if (!BEAST_EXPECT( + !offerInLedger(env, carol, carolOfferSeq) && + !offerInLedger(env, bob, bobOfferSeq))) + { + // If carol's or bob's offers are still in the ledger then + // further results are meaningless. + cleanupOldOffers( + env, + {{alice, aliceOfferSeq}, + {bob, bobOfferSeq}, + {carol, carolOfferSeq}}); + return 1; + } + // alice's offer should still be in the ledger, but reduced in + // size. + unsigned int badRate = 1; + { + Json::Value aliceOffer = + ledgerEntryOffer(env, alice, aliceOfferSeq); + + Amounts aliceReducedOffer = + jsonOfferToAmounts(aliceOffer[jss::node]); + + BEAST_EXPECT(aliceReducedOffer.in < aliceInitialOffer.in); + BEAST_EXPECT(aliceReducedOffer.out < aliceInitialOffer.out); + STAmount const inLedgerRate = + Quality(aliceReducedOffer).rate(); + badRate = inLedgerRate > initialRate ? 1 : 0; + + // If the inLedgerRate is less than initial rate, then + // incrementing the mantissa of the reduced TakerGets + // should result in a rate higher than initial. Check + // this to verify that the largest allowable TakerGets + // was computed. + if (badRate == 0) + { + STAmount const tweakedTakerGets( + aliceReducedOffer.in.issue(), + aliceReducedOffer.in.mantissa() + 1, + aliceReducedOffer.in.exponent(), + aliceReducedOffer.in.negative()); + STAmount const tweakedRate = + Quality( + Amounts{aliceReducedOffer.in, tweakedTakerGets}) + .rate(); + BEAST_EXPECT(tweakedRate > initialRate); + } +#if 0 + std::cout << "Placed rate: " << initialRate + << "; in-ledger rate: " << inLedgerRate + << "; TakerPays: " << aliceReducedOffer.in + << "; TakerGets: " << aliceReducedOffer.out + << std::endl; +// #else + std::string_view filler = badRate ? "**" : " "; + std::cout << "| " << aliceReducedOffer.in << "` | `" + << aliceReducedOffer.out << "` | `" << initialRate + << "` | " << filler << "`" << inLedgerRate << "`" + << filler << std::endl; +#endif + } + + // In preparation for the next iteration make sure all three + // offers are gone from the ledger. + cleanupOldOffers( + env, + {{alice, aliceOfferSeq}, + {bob, bobOfferSeq}, + {carol, carolOfferSeq}}); + return badRate; + }; + + constexpr int loopCount = 100; + unsigned int blockedCount = 0; + { + STAmount increaseGets = USD(0); + STAmount const step(increaseGets.issue(), 1, -8); + for (unsigned int i = 0; i < loopCount; ++i) + { + blockedCount += exerciseOfferTrio( + Amounts(drops(1642020), USD(1) + increaseGets)); + increaseGets += step; + } + } + + // If fixReducedOffersV2 is enabled, then none of the test cases + // should produce a potentially blocking rate. + // + // Also verify that if fixReducedOffersV2 is not enabled then + // some of the test cases produced a potentially blocking rate. + if (features[fixReducedOffersV2]) + { + BEAST_EXPECT(blockedCount == 0); + } + else + { + BEAST_EXPECT(blockedCount > 80); + } + } + } + void run() override { @@ -613,6 +795,7 @@ class ReducedOffer_test : public beast::unit_test::suite testPartialCrossOldXrpIouQChange(); testUnderFundedXrpIouQChange(); testUnderFundedIouIouQChange(); + testSellPartialCrossOldXrpIouQChange(); } };