From 2359d93c64bf2dfb1f87be8f0d40adf53601c976 Mon Sep 17 00:00:00 2001 From: Scott Schurr Date: Wed, 3 Apr 2024 15:39:10 -0700 Subject: [PATCH] Amendment fixReducedOffersV2 fixes issue #4937 --- src/ripple/app/paths/AMMOffer.h | 9 +- src/ripple/app/paths/impl/AMMOffer.cpp | 13 +- src/ripple/app/paths/impl/BookStep.cpp | 18 +- src/ripple/app/tx/impl/Offer.h | 26 +- src/ripple/protocol/Feature.h | 3 +- src/ripple/protocol/Quality.h | 23 ++ src/ripple/protocol/impl/Feature.cpp | 1 + src/ripple/protocol/impl/Quality.cpp | 28 +- src/test/app/AMM_test.cpp | 18 +- src/test/app/Flow_test.cpp | 24 +- src/test/app/Offer_test.cpp | 142 ++++++---- src/test/app/ReducedOffer_test.cpp | 360 ++++++++++++++++++++++++- 12 files changed, 563 insertions(+), 102 deletions(-) diff --git a/src/ripple/app/paths/AMMOffer.h b/src/ripple/app/paths/AMMOffer.h index 426ba96a772..7e7ef33af46 100644 --- a/src/ripple/app/paths/AMMOffer.h +++ b/src/ripple/app/paths/AMMOffer.h @@ -23,6 +23,7 @@ #include #include #include +#include #include namespace ripple { @@ -106,7 +107,7 @@ class AMMOffer limitOut( TAmounts const& offrAmt, TOut const& limit, - bool fixReducedOffers, + Rules const& rules, bool roundUp) const; /** Limit in of the provided offer. If one-path then swapIn @@ -114,7 +115,11 @@ class AMMOffer * current quality. */ TAmounts - limitIn(TAmounts const& offrAmt, TIn const& limit) const; + limitIn( + TAmounts const& offrAmt, + TIn const& limit, + Rules const& rules, + 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 759851b7afe..9063266f678 100644 --- a/src/ripple/app/paths/impl/AMMOffer.cpp +++ b/src/ripple/app/paths/impl/AMMOffer.cpp @@ -88,7 +88,7 @@ TAmounts AMMOffer::limitOut( TAmounts const& offrAmt, TOut const& limit, - bool fixReducedOffers, + Rules const& rules, bool roundUp) const { // Change the offer size proportionally to the original offer quality @@ -99,7 +99,7 @@ AMMOffer::limitOut( // poolPays * poolGets < (poolPays - assetOut) * (poolGets + assetIn) if (ammLiquidity_.multiPath()) { - if (fixReducedOffers) + if (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 @@ -117,11 +117,18 @@ template TAmounts AMMOffer::limitIn( TAmounts const& offrAmt, - TIn const& limit) const + TIn const& limit, + Rules const& rules, + bool roundUp) const { // See the comments above in limitOut(). if (ammLiquidity_.multiPath()) + { + if (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 358dac4c796..20016c4fe28 100644 --- a/src/ripple/app/paths/impl/BookStep.cpp +++ b/src/ripple/app/paths/impl/BookStep.cpp @@ -589,14 +589,18 @@ limitStepIn( TOut& ownerGives, std::uint32_t transferRateIn, std::uint32_t transferRateOut, - TIn const& limit) + TIn const& limit, + Rules const& rules) { if (limit < stpAmt.in) { 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. This adjustment changes + // transaction outcomes, so it must be made under an amendment. + ofrAmt = offer.limitIn(ofrAmt, inLmt, rules, /* roundUp */ false); stpAmt.out = ofrAmt.out; ownerGives = mulRatio( ofrAmt.out, transferRateOut, QUALITY_ONE, /*roundUp*/ false); @@ -624,7 +628,7 @@ limitStepOut( ofrAmt = offer.limitOut( ofrAmt, stpAmt.out, - rules.enabled(fixReducedOffersV1), + rules, /*roundUp*/ true); stpAmt.in = mulRatio(ofrAmt.in, transferRateIn, QUALITY_ONE, /*roundUp*/ true); @@ -664,7 +668,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) { @@ -746,7 +749,7 @@ BookStep::forEachOffer( // 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, stpAmt.out, afView.rules(), /*roundUp*/ false); stpAmt.in = mulRatio(ofrAmt.in, ofrInRate, QUALITY_ONE, /*roundUp*/ true); @@ -1077,7 +1080,8 @@ BookStep::fwdImp( ownerGivesAdj, transferRateIn, transferRateOut, - remainingIn); + remainingIn, + afView.rules()); savedIns.insert(remainingIn); lastOut = savedOuts.insert(stpAdjAmt.out); result.out = sum(savedOuts); @@ -1128,7 +1132,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..69e6b102da2 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,15 @@ class TOffer : private TOfferBase limitOut( TAmounts const& offrAmt, TOut const& limit, - bool fixReducedOffers, + Rules const& rules, bool roundUp) const; TAmounts - limitIn(TAmounts const& offrAmt, TIn const& limit) const; + limitIn( + TAmounts const& offrAmt, + TIn const& limit, + Rules const& rules, + bool roundUp) const; template static TER @@ -219,10 +224,10 @@ TAmounts TOffer::limitOut( TAmounts const& offrAmt, TOut const& limit, - bool fixReducedOffers, + Rules const& rules, bool roundUp) const { - if (fixReducedOffers) + if (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 +238,18 @@ TOffer::limitOut( template TAmounts -TOffer::limitIn(TAmounts const& offrAmt, TIn const& limit) - const +TOffer::limitIn( + TAmounts const& offrAmt, + TIn const& limit, + Rules const& rules, + bool roundUp) const { + if (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 d00dd1555f2..d368ce1c4a0 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 = 71; +static constexpr std::size_t numFeatures = 72; /** Amendments that this server supports and the default voting behavior. Whether they are enabled depends on the Rules defined in the validated @@ -358,6 +358,7 @@ extern uint256 const fixAMMOverflowOffer; extern uint256 const featurePriceOracle; extern uint256 const fixEmptyDID; extern uint256 const fixXChainRewardRounding; +extern uint256 const fixReducedOffersV2; } // namespace ripple diff --git a/src/ripple/protocol/Quality.h b/src/ripple/protocol/Quality.h index 840d8d444e1..2057e385de8 100644 --- a/src/ripple/protocol/Quality.h +++ b/src/ripple/protocol/Quality.h @@ -200,6 +200,29 @@ class Quality toAmount(stRes.in), toAmount(stRes.out)); } + Amounts + ceil_in_strict(Amounts const& amount, STAmount const& limit, bool roundUp) + const; + + template + TAmounts + ceil_in_strict( + TAmounts const& amount, + In const& limit, + bool roundUp) 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_strict(stAmt, stLim, roundUp); + return TAmounts( + toAmount(stRes.in), toAmount(stRes.out)); + } + /** Returns the scaled amount with out capped. Math is avoided if the result is exact. The input is clamped to prevent money creation. diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index 8bd4b7aea27..e128ea13387 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -465,6 +465,7 @@ REGISTER_FIX (fixAMMOverflowOffer, Supported::yes, VoteBehavior::De REGISTER_FEATURE(PriceOracle, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FIX (fixEmptyDID, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FIX (fixXChainRewardRounding, 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 b6828fab773..c90129eae39 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -3151,13 +3151,17 @@ 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}})); + } // Initial 30,000 + 100 BEAST_EXPECT(expectLine(env, carol, STAmount{USD, 30'100})); // Initial 1,000 - 30082730(AMM pool) - 70798251(offer) - 10(tx fee) diff --git a/src/test/app/Flow_test.cpp b/src/test/app/Flow_test.cpp index 920f7a6e058..a4adecfa4cf 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); @@ -518,14 +517,26 @@ struct Flow_test : public beast::unit_test::suite 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)); - env.require(balance(carol, EUR(1))); - env.require(balance(bob, USD(0.4))); - env.require(balance(bob, EUR(999))); + if (!reducedOffersV2) + { + env.require(balance(carol, EUR(1))); + env.require(balance(bob, USD(0.4))); + env.require(balance(bob, EUR(999))); + } } } @@ -1375,9 +1386,12 @@ struct Flow_test : public beast::unit_test::suite { using namespace jtx; FeatureBitset const ownerPaysFee{featureOwnerPaysFee}; + FeatureBitset const reducedOffersV2(fixReducedOffersV2); testLineQuality(features); testFalseDry(features); + testDirectStep(features - reducedOffersV2); + 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..d995dce9aae 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,339 @@ 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 + testReproduceIssue4937() // !!!! DEBUG !!!! Remove test before checkin. + { + testcase("Reproduce issue 4937"); + + // The following transaction... + // 8F4A907920B7574C5FF1A12F8E30AD425903ECFE331A8E72025DCC980F340BC1 + // was found to produce a blocking offer. Reproduce that failure. + // + // clang-format off + // tx: { + // "Account": "", + // "Fee": "12", + // "Flags": 655360, + // "TakerGets": { + // "currency": "USD", + // "issuer": "", + // "value": "1" + // }, + // "TakerPays": "1642020", + // "TransactionType": "OfferCreate", + // } + // meta: { + // "AffectedNodes": [ + // ... + // { + // "DeletedNode": { + // "FinalFields": { + // "Account": "", + // "BookDirectory"" "DFA3B6DDAB58C7E8E5D944E736DA4B7046C30E4F460FD9DE4E1501816E11E800", + // "BookNode": "0", + // "Expiration": 762392704, + // "Flags": 0, + // "OwnerNode": "0", + // "PreviousTxnLgrSeq": 86261068, + // "Sequence": 127164370, + // "TakerGets": "0", + // "TakerPays": { + // "currency": "USD", + // "issuer": "", + // "value": "0" + // } + // }, + // "LedgerEntryType": "Offer", + // "PreviousFields": { + // "TakerGets": "1642020", + // "TakerPays": { + // "currency": "USD", + // "issuer": "", + // "value": "0.97086565812384" + // } + // } + // } + // }, + // ... + // { + // "ModifiedNode": { + // "FinalFields": { + // "Account": "", + // "BookDirectory": "DFA3B6DDAB58C7E8E5D944E736DA4B7046C30E4F460FD9DE4E15018CA140B134", + // "BookNode": "0", + // "Flags": 0, + // "OwnerNode": "0", + // "Sequence": 81661880, + // "TakerGets": "3335820", + // "TakerPays": { + // "currency": "USD", + // "issuer": "", + // "value": "1.972363411493785" + // } + // }, + // "LedgerEntryType": "Offer", + // "LedgerIndex": "EAFA5CA3183C0824955BED48436BB3E6FE15139605EF887901281172505F865F", + // "PreviousFields": { + // "TakerGets": "3382562", + // "TakerPays": { + // "currency": "USD", + // "issuer": "", + // "value": "2" + // } + // }, + // "PreviousTxnID": "2E271C034BCB31F98868444E24B8E83DC33394EAB7D220C68D72285B6E58408B", + // "PreviousTxnLgrSeq": 86257823 + // } + // } + // ], + // "TransactionIndex": 40, + // "TransactionResult": "tesSUCCESS" + // } + // clang-format on + + // Get the necessary initial offers into the order book. + using namespace jtx; + using namespace std::chrono_literals; + + auto const alice = Account("alice"); + auto const bob = Account("bob"); + auto const carol = Account("carol"); + auto const gw = Account("gw"); + + auto const USD = gw["USD"]; + + Env env{*this, supported_amendments()}; + + env.fund(XRP(10000), alice, bob, carol, gw); + env.close(); + env.trust(USD(1000), alice, bob, carol); + env.close(); + env(pay(gw, alice, USD(500))); + env(pay(gw, bob, USD(500))); + env(pay(gw, carol, USD(500))); + env.close(); + + // alice submits an offer that will become a blocker. + std::uint32_t const aliceOfferSeq = env.seq(alice); + env(offer(alice, USD(2), drops(3382562))); + 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(); + + // carol submits a tfSell offer that consumes all of bob's offer + // and part of alice's offer. + // + // The tfSell flag is important. Without that flag alice's offer + // remains untouched. + std::uint32_t const carolOfferSeq = env.seq(carol); + env(offer(carol, drops(1642020), USD(1)), txflags(tfSell)); + env.close(); + + // carol's offer did not leave any remnant in the ledger. + BEAST_EXPECT(!offerInLedger(env, carol, carolOfferSeq)); + + // bob's offer is fully consumed. + BEAST_EXPECT(!offerInLedger(env, bob, bobOfferSeq)); + + // alice's offer is partially consumed and has a worse (larger) + // rate than the book page indicates. That makes the partially + // consumed offer a blocker (the offer has a worse rate than the + // rate of the offer book page containing the offer). + Json::Value const blockingOffer = + ledgerEntryOffer(env, alice, aliceOfferSeq)[jss::node]; + STAmount const inLedgerRate = + Quality(jsonOfferToAmounts(blockingOffer)).rate(); + + // This check verifies that the offer left behind is a blocker! + BEAST_EXPECT(inLedgerRate > initialRate); + } + + 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 cross alice's. + 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 +949,8 @@ class ReducedOffer_test : public beast::unit_test::suite testPartialCrossOldXrpIouQChange(); testUnderFundedXrpIouQChange(); testUnderFundedIouIouQChange(); + testSellPartialCrossOldXrpIouQChange(); + // testReproduceIssue4937(); !!!! DEBUG !!!! Remove test before checkin. } };