diff --git a/Builds/CMake/RippledCore.cmake b/Builds/CMake/RippledCore.cmake index db7757f9c2f..a853a6cff53 100644 --- a/Builds/CMake/RippledCore.cmake +++ b/Builds/CMake/RippledCore.cmake @@ -721,6 +721,7 @@ if (tests) src/test/app/PseudoTx_test.cpp src/test/app/RCLCensorshipDetector_test.cpp src/test/app/RCLValidations_test.cpp + src/test/app/ReducedOffer_test.cpp src/test/app/Regression_test.cpp src/test/app/SHAMapStore_test.cpp src/test/app/SetAuth_test.cpp diff --git a/src/ripple/app/paths/impl/BookStep.cpp b/src/ripple/app/paths/impl/BookStep.cpp index a6b2c59611e..555d90fac8c 100644 --- a/src/ripple/app/paths/impl/BookStep.cpp +++ b/src/ripple/app/paths/impl/BookStep.cpp @@ -531,14 +531,22 @@ limitStepOut( TOut& ownerGives, std::uint32_t transferRateIn, std::uint32_t transferRateOut, - TOut const& limit) + TOut const& limit, + Rules const& rules) { if (limit < stpAmt.out) { stpAmt.out = limit; ownerGives = mulRatio( stpAmt.out, transferRateOut, QUALITY_ONE, /*roundUp*/ false); - ofrAmt = ofrQ.ceil_out(ofrAmt, stpAmt.out); + 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 + // an amendment. + ofrAmt = ofrQ.ceil_out_strict(ofrAmt, stpAmt.out, /*roundUp*/ true); + else + ofrAmt = ofrQ.ceil_out(ofrAmt, stpAmt.out); stpAmt.in = mulRatio(ofrAmt.in, transferRateIn, QUALITY_ONE, /*roundUp*/ true); } @@ -577,6 +585,7 @@ 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; while (offers.step()) @@ -654,7 +663,16 @@ BookStep::forEachOffer( ownerGives = funds; stpAmt.out = mulRatio( ownerGives, QUALITY_ONE, ofrOutRate, /*roundUp*/ false); - ofrAmt = ofrQ->ceil_out(ofrAmt, stpAmt.out); + + // 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. + if (fixReduced) + ofrAmt = ofrQ->ceil_out_strict( + ofrAmt, stpAmt.out, /* roundUp */ false); + else + ofrAmt = ofrQ->ceil_out(ofrAmt, stpAmt.out); + stpAmt.in = mulRatio(ofrAmt.in, ofrInRate, QUALITY_ONE, /*roundUp*/ true); } @@ -770,7 +788,8 @@ BookStep::revImp( ownerGivesAdj, transferRateIn, transferRateOut, - remainingOut); + remainingOut, + afView.rules()); remainingOut = beast::zero; savedIns.insert(stpAdjAmt.in); savedOuts.insert(remainingOut); @@ -922,7 +941,8 @@ BookStep::fwdImp( ownerGivesAdjRev, transferRateIn, transferRateOut, - remainingOut); + remainingOut, + afView.rules()); if (stpAdjAmtRev.in == remainingIn) { diff --git a/src/ripple/app/tx/impl/CreateOffer.cpp b/src/ripple/app/tx/impl/CreateOffer.cpp index 4f1d9108bca..dd01a64b5f2 100644 --- a/src/ripple/app/tx/impl/CreateOffer.cpp +++ b/src/ripple/app/tx/impl/CreateOffer.cpp @@ -824,8 +824,22 @@ CreateOffer::flowCross( // what is a good threshold to check? afterCross.in.clear(); - afterCross.out = divRound( - afterCross.in, rate, takerAmount.out.issue(), true); + afterCross.out = [&]() { + // Careful analysis showed that rounding up this + // divRound result could lead to placing a reduced + // offer in the ledger that blocks order books. So + // the fixReducedOffersV1 amendment changes the + // behavior to round down instead. + if (psb.rules().enabled(fixReducedOffersV1)) + return divRoundStrict( + afterCross.in, + rate, + takerAmount.out.issue(), + false); + + return divRound( + afterCross.in, rate, takerAmount.out.issue(), true); + }(); } else { diff --git a/src/ripple/app/tx/impl/OfferStream.cpp b/src/ripple/app/tx/impl/OfferStream.cpp index 58fd209ca0b..5933d9c3838 100644 --- a/src/ripple/app/tx/impl/OfferStream.cpp +++ b/src/ripple/app/tx/impl/OfferStream.cpp @@ -182,17 +182,33 @@ TOfferStreamBase::shouldRmSmallIncreasedQOffer() const } TTakerGets const ownerFunds = toAmount(*ownerFunds_); + bool const fixReduced = view_.rules().enabled(fixReducedOffersV1); auto const effectiveAmounts = [&] { if (offer_.owner() != offer_.issueOut().account && ownerFunds < ofrAmts.out) { - // adjust the amounts by owner funds + // adjust the amounts by owner funds. + // + // It turns out we can prevent order book blocking by rounding down + // the ceil_out() result. This adjustment changes transaction + // results, so it must be made under an amendment. + if (fixReduced) + return offer_.quality().ceil_out_strict( + ofrAmts, ownerFunds, /* roundUp */ false); + return offer_.quality().ceil_out(ofrAmts, ownerFunds); } return ofrAmts; }(); + // If either the effective in or out are zero then remove the offer. + // This can happen with fixReducedOffersV1 since it rounds down. + if (fixReduced && + (effectiveAmounts.in.signum() <= 0 || + effectiveAmounts.out.signum() <= 0)) + return true; + if (effectiveAmounts.in > TTakerPays::minPositiveAmount()) return false; diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index e4b0e3d4acd..48198e38315 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 = 58; +static constexpr std::size_t numFeatures = 59; /** Amendments that this server supports and the default voting behavior. Whether they are enabled depends on the Rules defined in the validated @@ -345,6 +345,7 @@ extern uint256 const featureXRPFees; extern uint256 const fixUniversalNumber; extern uint256 const fixNonFungibleTokensV1_2; extern uint256 const fixNFTokenRemint; +extern uint256 const fixReducedOffersV1; } // namespace ripple diff --git a/src/ripple/protocol/Quality.h b/src/ripple/protocol/Quality.h index 9de137d8770..840d8d444e1 100644 --- a/src/ripple/protocol/Quality.h +++ b/src/ripple/protocol/Quality.h @@ -223,6 +223,29 @@ class Quality toAmount(stRes.in), toAmount(stRes.out)); } + Amounts + ceil_out_strict(Amounts const& amount, STAmount const& limit, bool roundUp) + const; + + template + 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)); + } + /** 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. diff --git a/src/ripple/protocol/STAmount.h b/src/ripple/protocol/STAmount.h index f04b6bb0e50..63f97bb48fe 100644 --- a/src/ripple/protocol/STAmount.h +++ b/src/ripple/protocol/STAmount.h @@ -503,7 +503,7 @@ divide(STAmount const& v1, STAmount const& v2, Issue const& issue); STAmount multiply(STAmount const& v1, STAmount const& v2, Issue const& issue); -// multiply, or divide rounding result in specified direction +// multiply rounding result in specified direction STAmount mulRound( STAmount const& v1, @@ -511,6 +511,15 @@ mulRound( Issue const& issue, bool roundUp); +// multiply following the rounding directions more precisely. +STAmount +mulRoundStrict( + STAmount const& v1, + STAmount const& v2, + Issue const& issue, + bool roundUp); + +// divide rounding result in specified direction STAmount divRound( STAmount const& v1, @@ -518,6 +527,14 @@ divRound( Issue const& issue, bool roundUp); +// divide following the rounding directions more precisely. +STAmount +divRoundStrict( + STAmount const& v1, + STAmount const& v2, + Issue const& issue, + bool roundUp); + // Someone is offering X for Y, what is the rate? // Rate: smaller is better, the taker wants the most out: in/out // VFALCO TODO Return a Quality object diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index 6b8e7719d00..09923109671 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -452,6 +452,7 @@ REGISTER_FEATURE(XRPFees, Supported::yes, VoteBehavior::De REGISTER_FIX (fixUniversalNumber, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FIX (fixNonFungibleTokensV1_2, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FIX (fixNFTokenRemint, Supported::yes, VoteBehavior::DefaultNo); +REGISTER_FIX (fixReducedOffersV1, 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 97e1b8a9fd7..f7b9d6b3c41 100644 --- a/src/ripple/protocol/impl/Quality.cpp +++ b/src/ripple/protocol/impl/Quality.cpp @@ -81,12 +81,20 @@ Quality::ceil_in(Amounts const& amount, STAmount const& limit) const return amount; } -Amounts -Quality::ceil_out(Amounts const& amount, STAmount const& limit) const +template +static Amounts +ceil_out_impl( + Amounts const& amount, + STAmount const& limit, + bool roundUp, + Quality const& quality) { if (amount.out > limit) { - Amounts result(mulRound(limit, rate(), amount.in.issue(), true), limit); + Amounts result( + MulRoundFunc(limit, quality.rate(), amount.in.issue(), roundUp), + limit); // Clamp in if (result.in > amount.in) result.in = amount.in; @@ -97,6 +105,21 @@ Quality::ceil_out(Amounts const& amount, STAmount const& limit) const return amount; } +Amounts +Quality::ceil_out(Amounts const& amount, STAmount const& limit) const +{ + return ceil_out_impl(amount, limit, /* roundUp */ true, *this); +} + +Amounts +Quality::ceil_out_strict( + Amounts const& amount, + STAmount const& limit, + bool roundUp) const +{ + return ceil_out_impl(amount, limit, roundUp, *this); +} + Quality composed_quality(Quality const& lhs, Quality const& rhs) { diff --git a/src/ripple/protocol/impl/STAmount.cpp b/src/ripple/protocol/impl/STAmount.cpp index 02e3345944e..90a646787ae 100644 --- a/src/ripple/protocol/impl/STAmount.cpp +++ b/src/ripple/protocol/impl/STAmount.cpp @@ -1266,8 +1266,28 @@ multiply(STAmount const& v1, STAmount const& v2, Issue const& issue) v1.negative() != v2.negative()); } +// This is the legacy version of canonicalizeRound. It's been in use +// for years, so it is deeply embedded in the behavior of cross-currency +// transactions. +// +// However in 2022 it was noticed that the rounding characteristics were +// surprising. When the code converts from IOU-like to XRP-like there may +// be a fraction of the IOU-like representation that is too small to be +// represented in drops. `canonicalizeRound()` currently does some unusual +// rounding. +// +// 1. If the fractional part is greater than or equal to 0.1, then the +// number of drops is rounded up. +// +// 2. However, if the fractional part is less than 0.1 (for example, +// 0.099999), then the number of drops is rounded down. +// +// The XRP Ledger has this rounding behavior baked in. But there are +// situations where this rounding behavior led to undesirable outcomes. +// So an alternative rounding approach was introduced. You'll see that +// alternative below. static void -canonicalizeRound(bool native, std::uint64_t& value, int& offset) +canonicalizeRound(bool native, std::uint64_t& value, int& offset, bool) { if (native) { @@ -1301,8 +1321,100 @@ canonicalizeRound(bool native, std::uint64_t& value, int& offset) } } -STAmount -mulRound( +// The original canonicalizeRound did not allow the rounding direction to +// be specified. It also ignored some of the bits that could contribute to +// rounding decisions. canonicalizeRoundStrict() tracks all of the bits in +// the value being rounded. +static void +canonicalizeRoundStrict( + bool native, + std::uint64_t& value, + int& offset, + bool roundUp) +{ + if (native) + { + if (offset < 0) + { + bool hadRemainder = false; + + while (offset < -1) + { + // It would be better to use std::lldiv than to separately + // compute the remainder. But std::lldiv does not support + // unsigned arguments. + std::uint64_t const newValue = value / 10; + hadRemainder |= (value != (newValue * 10)); + value = newValue; + ++offset; + } + value += + (hadRemainder && roundUp) ? 10 : 9; // Add before last divide + value /= 10; + ++offset; + } + } + else if (value > STAmount::cMaxValue) + { + while (value > (10 * STAmount::cMaxValue)) + { + value /= 10; + ++offset; + } + value += 9; // add before last divide + value /= 10; + ++offset; + } +} + +namespace { + +// saveNumberRoundMode doesn't do quite enough for us. What we want is a +// Number::RoundModeGuard that sets the new mode and restores the old mode +// when it leaves scope. Since Number doesn't have that facility, we'll +// build it here. +class NumberRoundModeGuard +{ + saveNumberRoundMode saved_; + +public: + explicit NumberRoundModeGuard(Number::rounding_mode mode) noexcept + : saved_{Number::setround(mode)} + { + } + + NumberRoundModeGuard(NumberRoundModeGuard const&) = delete; + + NumberRoundModeGuard& + operator=(NumberRoundModeGuard const&) = delete; +}; + +// We need a class that has an interface similar to NumberRoundModeGuard +// but does nothing. +class DontAffectNumberRoundMode +{ +public: + explicit DontAffectNumberRoundMode(Number::rounding_mode mode) noexcept + { + } + + DontAffectNumberRoundMode(DontAffectNumberRoundMode const&) = delete; + + DontAffectNumberRoundMode& + operator=(DontAffectNumberRoundMode const&) = delete; +}; + +} // anonymous namespace + +// Pass the canonicalizeRound function pointer as a template parameter. +// +// We might need to use NumberRoundModeGuard. Allow the caller +// to pass either that or a replacement as a template parameter. +template < + void (*CanonicalizeFunc)(bool, std::uint64_t&, int&, bool), + typename MightSaveRound> +static STAmount +mulRoundImpl( STAmount const& v1, STAmount const& v2, Issue const& issue, @@ -1365,8 +1477,15 @@ mulRound( int offset = offset1 + offset2 + 14; if (resultNegative != roundUp) - canonicalizeRound(xrp, amount, offset); - STAmount result(issue, amount, offset, resultNegative); + { + CanonicalizeFunc(xrp, amount, offset, roundUp); + } + STAmount result = [&]() { + // If appropriate, tell Number to round down. This gives the desired + // result from STAmount::canonicalize. + MightSaveRound const savedRound(Number::towards_zero); + return STAmount(issue, amount, offset, resultNegative); + }(); if (roundUp && !resultNegative && !result) { @@ -1388,7 +1507,32 @@ mulRound( } STAmount -divRound( +mulRound( + STAmount const& v1, + STAmount const& v2, + Issue const& issue, + bool roundUp) +{ + return mulRoundImpl( + v1, v2, issue, roundUp); +} + +STAmount +mulRoundStrict( + STAmount const& v1, + STAmount const& v2, + Issue const& issue, + bool roundUp) +{ + return mulRoundImpl( + v1, v2, issue, roundUp); +} + +// We might need to use NumberRoundModeGuard. Allow the caller +// to pass either that or a replacement as a template parameter. +template +static STAmount +divRoundImpl( STAmount const& num, STAmount const& den, Issue const& issue, @@ -1437,9 +1581,18 @@ divRound( int offset = numOffset - denOffset - 17; if (resultNegative != roundUp) - canonicalizeRound(isXRP(issue), amount, offset); + canonicalizeRound(isXRP(issue), amount, offset, roundUp); + + STAmount result = [&]() { + // If appropriate, tell Number the rounding mode we are using. + // Note that "roundUp == true" actually means "round away from zero". + // Otherwise round toward zero. + using enum Number::rounding_mode; + MightSaveRound const savedRound( + roundUp ^ resultNegative ? upward : downward); + return STAmount(issue, amount, offset, resultNegative); + }(); - STAmount result(issue, amount, offset, resultNegative); if (roundUp && !resultNegative && !result) { if (isXRP(issue)) @@ -1459,4 +1612,24 @@ divRound( return result; } +STAmount +divRound( + STAmount const& num, + STAmount const& den, + Issue const& issue, + bool roundUp) +{ + return divRoundImpl(num, den, issue, roundUp); +} + +STAmount +divRoundStrict( + STAmount const& num, + STAmount const& den, + Issue const& issue, + bool roundUp) +{ + return divRoundImpl(num, den, issue, roundUp); +} + } // namespace ripple diff --git a/src/test/app/Offer_test.cpp b/src/test/app/Offer_test.cpp index 0d1a4326440..9d97fb692a6 100644 --- a/src/test/app/Offer_test.cpp +++ b/src/test/app/Offer_test.cpp @@ -2126,18 +2126,17 @@ class Offer_test : public beast::unit_test::suite BEAST_EXPECT( jrr[jss::node][sfBalance.fieldName][jss::value] == "49.96666666666667"); + jrr = ledgerEntryState(env, bob, gw, "USD"); - if (NumberSwitchOver) + Json::Value const bobsUSD = + jrr[jss::node][sfBalance.fieldName][jss::value]; + if (!NumberSwitchOver) { - BEAST_EXPECT( - jrr[jss::node][sfBalance.fieldName][jss::value] == - "-0.9665000000333333"); + BEAST_EXPECT(bobsUSD == "-0.966500000033334"); } else { - BEAST_EXPECT( - jrr[jss::node][sfBalance.fieldName][jss::value] == - "-0.966500000033334"); + BEAST_EXPECT(bobsUSD == "-0.9665000000333333"); } } } diff --git a/src/test/app/ReducedOffer_test.cpp b/src/test/app/ReducedOffer_test.cpp new file mode 100644 index 00000000000..f82efcb7fc8 --- /dev/null +++ b/src/test/app/ReducedOffer_test.cpp @@ -0,0 +1,622 @@ +//------------------------------------------------------------------------------ +/* + This file is part of rippled: https://github.com/ripple/rippled + Copyright (c) 2022 Ripple Labs Inc. + + Permission to use, copy, modify, and/or distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#include +#include +#include +#include + +namespace ripple { +namespace test { + +class ReducedOffer_test : public beast::unit_test::suite +{ + static auto + ledgerEntryOffer( + jtx::Env& env, + jtx::Account const& acct, + std::uint32_t offer_seq) + { + Json::Value jvParams; + jvParams[jss::offer][jss::account] = acct.human(); + jvParams[jss::offer][jss::seq] = offer_seq; + return env.rpc( + "json", "ledger_entry", to_string(jvParams))[jss::result]; + } + + static bool + offerInLedger( + jtx::Env& env, + jtx::Account const& acct, + std::uint32_t offerSeq) + { + Json::Value ledgerOffer = ledgerEntryOffer(env, acct, offerSeq); + return !( + ledgerOffer.isMember(jss::error) && + ledgerOffer[jss::error].asString() == "entryNotFound"); + } + + // Common code to clean up unneeded offers. + static void + cleanupOldOffers( + jtx::Env& env, + jtx::Account const& acct1, + jtx::Account const& acct2, + std::uint32_t acct1OfferSeq, + std::uint32_t acct2OfferSeq) + { + env(offer_cancel(acct1, acct1OfferSeq)); + env(offer_cancel(acct2, acct2OfferSeq)); + env.close(); + } + +public: + void + testPartialCrossNewXrpIouQChange() + { + testcase("exercise partial cross new XRP/IOU offer Q change"); + + using namespace jtx; + + auto const gw = Account{"gateway"}; + auto const alice = Account{"alice"}; + auto const bob = Account{"bob"}; + auto const USD = gw["USD"]; + + // Make one test run without fixReducedOffersV1 and one with. + for (FeatureBitset features : + {supported_amendments() - fixReducedOffersV1, + supported_amendments() | fixReducedOffersV1}) + { + Env env{*this, features}; + + // Make sure none of the offers we generate are under funded. + env.fund(XRP(10'000'000), gw, alice, bob); + env.close(); + + env(trust(alice, USD(10'000'000))); + env(trust(bob, USD(10'000'000))); + env.close(); + + env(pay(gw, bob, USD(10'000'000))); + env.close(); + + // Lambda that: + // 1. Exercises one offer pair, + // 2. Collects the results, and + // 3. Cleans up for the next offer pair. + // Returns 1 if the crossed offer has a bad rate for the book. + auto exerciseOfferPair = + [this, &env, &alice, &bob]( + Amounts const& inLedger, + Amounts const& newOffer) -> unsigned int { + // Put inLedger offer in the ledger so newOffer can cross it. + std::uint32_t const aliceOfferSeq = env.seq(alice); + env(offer(alice, inLedger.in, inLedger.out)); + env.close(); + + // Now alice's offer will partially cross bob's offer. + STAmount const initialRate = Quality(newOffer).rate(); + std::uint32_t const bobOfferSeq = env.seq(bob); + STAmount const bobInitialBalance = env.balance(bob); + STAmount const bobsFee = drops(10); + env(offer(bob, newOffer.in, newOffer.out, tfSell), + fee(bobsFee)); + env.close(); + STAmount const bobFinalBalance = env.balance(bob); + + // alice's offer should be fully crossed and so gone from + // the ledger. + if (!BEAST_EXPECT(!offerInLedger(env, alice, aliceOfferSeq))) + // If the in-ledger offer was not consumed then further + // results are meaningless. + return 1; + + // bob's offer should be in the ledger, but reduced in size. + unsigned int badRate = 1; + { + Json::Value bobOffer = + ledgerEntryOffer(env, bob, bobOfferSeq); + + STAmount const reducedTakerGets = amountFromJson( + sfTakerGets, bobOffer[jss::node][sfTakerGets.jsonName]); + STAmount const reducedTakerPays = amountFromJson( + sfTakerPays, bobOffer[jss::node][sfTakerPays.jsonName]); + STAmount const bobGot = + env.balance(bob) + bobsFee - bobInitialBalance; + BEAST_EXPECT(reducedTakerPays < newOffer.in); + BEAST_EXPECT(reducedTakerGets < newOffer.out); + STAmount const inLedgerRate = + Quality(Amounts{reducedTakerPays, reducedTakerGets}) + .rate(); + + badRate = inLedgerRate > initialRate ? 1 : 0; + + // If the inLedgerRate is less than initial rate, then + // incrementing the mantissa of the reduced taker pays + // should result in a rate higher than initial. Check + // this to verify that the largest allowable TakerPays + // was computed. + if (badRate == 0) + { + STAmount const tweakedTakerPays = + reducedTakerPays + drops(1); + STAmount const tweakedRate = + Quality(Amounts{tweakedTakerPays, reducedTakerGets}) + .rate(); + BEAST_EXPECT(tweakedRate > initialRate); + } +#if 0 + std::cout << "Placed rate: " << initialRate + << "; in-ledger rate: " << inLedgerRate + << "; TakerPays: " << reducedTakerPays + << "; TakerGets: " << reducedTakerGets + << "; bob already got: " << bobGot << std::endl; +// #else + std::string_view filler = + inLedgerRate > initialRate ? "**" : " "; + std::cout << "| `" << reducedTakerGets << "` | `" + << reducedTakerPays << "` | `" << initialRate + << "` | " << filler << "`" << inLedgerRate << "`" + << filler << " |`" << std::endl; +#endif + } + + // In preparation for the next iteration make sure the two + // offers are gone from the ledger. + cleanupOldOffers(env, alice, bob, aliceOfferSeq, bobOfferSeq); + return badRate; + }; + + // bob's offer (the new offer) is the same every time: + Amounts const bobsOffer{ + STAmount(XRP(1)), STAmount(USD.issue(), 1, 0)}; + + // alice's offer has a slightly smaller TakerPays with each + // iteration. This should mean that the size of the offer bob + // places in the ledger should increase with each iteration. + unsigned int blockedCount = 0; + for (std::uint64_t mantissaReduce = 1'000'000'000ull; + mantissaReduce <= 5'000'000'000ull; + mantissaReduce += 20'000'000ull) + { + STAmount aliceUSD{ + bobsOffer.out.issue(), + bobsOffer.out.mantissa() - mantissaReduce, + bobsOffer.out.exponent()}; + STAmount aliceXRP{ + bobsOffer.in.issue(), bobsOffer.in.mantissa() - 1}; + Amounts alicesOffer{aliceUSD, aliceXRP}; + blockedCount += exerciseOfferPair(alicesOffer, bobsOffer); + } + + // If fixReducedOffersV1 is enabled, then none of the test cases + // should produce a potentially blocking rate. + // + // Also verify that if fixReducedOffersV1 is not enabled then + // some of the test cases produced a potentially blocking rate. + if (features[fixReducedOffersV1]) + { + BEAST_EXPECT(blockedCount == 0); + } + else + { + BEAST_EXPECT(blockedCount >= 170); + } + } + } + + void + testPartialCrossOldXrpIouQChange() + { + testcase("exercise partial cross old XRP/IOU offer Q change"); + + using namespace jtx; + + auto const gw = Account{"gateway"}; + auto const alice = Account{"alice"}; + auto const bob = Account{"bob"}; + auto const USD = gw["USD"]; + + // Make one test run without fixReducedOffersV1 and one with. + for (FeatureBitset features : + {supported_amendments() - fixReducedOffersV1, + supported_amendments() | fixReducedOffersV1}) + { + // Make sure none of the offers we generate are under funded. + Env env{*this, features}; + env.fund(XRP(10'000'000), gw, alice, bob); + env.close(); + + env(trust(alice, USD(10'000'000))); + env(trust(bob, USD(10'000'000))); + env.close(); + + env(pay(gw, alice, USD(10'000'000))); + env.close(); + + // Lambda that: + // 1. Exercises one offer pair, + // 2. Collects the results, and + // 3. Cleans up for the next offer pair. + auto exerciseOfferPair = + [this, &env, &alice, &bob]( + Amounts const& inLedger, + Amounts const& newOffer) -> unsigned int { + // Get the inLedger offer into the ledger so newOffer can cross + // it. + STAmount const initialRate = Quality(inLedger).rate(); + std::uint32_t const aliceOfferSeq = env.seq(alice); + env(offer(alice, inLedger.in, inLedger.out)); + env.close(); + + // Now bob's offer will partially cross alice's offer. + std::uint32_t const bobOfferSeq = env.seq(bob); + STAmount const aliceInitialBalance = env.balance(alice); + env(offer(bob, newOffer.in, newOffer.out)); + env.close(); + STAmount const aliceFinalBalance = env.balance(alice); + + // bob's offer should not have made it into the ledger. + if (!BEAST_EXPECT(!offerInLedger(env, bob, bobOfferSeq))) + { + // If the in-ledger offer was not consumed then further + // results are meaningless. + cleanupOldOffers( + env, alice, bob, aliceOfferSeq, bobOfferSeq); + 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); + + STAmount const reducedTakerGets = amountFromJson( + sfTakerGets, + aliceOffer[jss::node][sfTakerGets.jsonName]); + STAmount const reducedTakerPays = amountFromJson( + sfTakerPays, + aliceOffer[jss::node][sfTakerPays.jsonName]); + STAmount const aliceGot = + env.balance(alice) - aliceInitialBalance; + BEAST_EXPECT(reducedTakerPays < inLedger.in); + BEAST_EXPECT(reducedTakerGets < inLedger.out); + STAmount const inLedgerRate = + Quality(Amounts{reducedTakerPays, reducedTakerGets}) + .rate(); + badRate = inLedgerRate > initialRate ? 1 : 0; + + // If the inLedgerRate is less than initial rate, then + // incrementing the mantissa of the reduced taker pays + // should result in a rate higher than initial. Check + // this to verify that the largest allowable TakerPays + // was computed. + if (badRate == 0) + { + STAmount const tweakedTakerPays = + reducedTakerPays + drops(1); + STAmount const tweakedRate = + Quality(Amounts{tweakedTakerPays, reducedTakerGets}) + .rate(); + BEAST_EXPECT(tweakedRate > initialRate); + } +#if 0 + std::cout << "Placed rate: " << initialRate + << "; in-ledger rate: " << inLedgerRate + << "; TakerPays: " << reducedTakerPays + << "; TakerGets: " << reducedTakerGets + << "; alice already got: " << aliceGot + << std::endl; +// #else + std::string_view filler = badRate ? "**" : " "; + std::cout << "| `" << reducedTakerGets << "` | `" + << reducedTakerPays << "` | `" << initialRate + << "` | " << filler << "`" << inLedgerRate << "`" + << filler << " | `" << aliceGot << "` |" + << std::endl; +#endif + } + + // In preparation for the next iteration make sure the two + // offers are gone from the ledger. + cleanupOldOffers(env, alice, bob, aliceOfferSeq, bobOfferSeq); + return badRate; + }; + + // alice's offer (the old offer) is the same every time: + Amounts const aliceOffer{ + STAmount(XRP(1)), STAmount(USD.issue(), 1, 0)}; + + // bob's offer has a slightly smaller TakerPays with each iteration. + // This should mean that the size of the offer alice leaves in the + // ledger should increase with each iteration. + unsigned int blockedCount = 0; + for (std::uint64_t mantissaReduce = 1'000'000'000ull; + mantissaReduce <= 4'000'000'000ull; + mantissaReduce += 20'000'000ull) + { + STAmount bobUSD{ + aliceOffer.out.issue(), + aliceOffer.out.mantissa() - mantissaReduce, + aliceOffer.out.exponent()}; + STAmount bobXRP{ + aliceOffer.in.issue(), aliceOffer.in.mantissa() - 1}; + Amounts bobsOffer{bobUSD, bobXRP}; + + blockedCount += exerciseOfferPair(aliceOffer, bobsOffer); + } + + // If fixReducedOffersV1 is enabled, then none of the test cases + // should produce a potentially blocking rate. + // + // Also verify that if fixReducedOffersV1 is not enabled then + // some of the test cases produced a potentially blocking rate. + if (features[fixReducedOffersV1]) + { + BEAST_EXPECT(blockedCount == 0); + } + else + { + BEAST_EXPECT(blockedCount > 10); + } + } + } + + void + testUnderFundedXrpIouQChange() + { + testcase("exercise underfunded XRP/IOU offer Q change"); + + // Bob places an offer that is not fully funded. + // + // This unit test compares the behavior of this situation before and + // after applying the fixReducedOffersV1 amendment. + + using namespace jtx; + auto const alice = Account{"alice"}; + auto const bob = Account{"bob"}; + auto const gw = Account{"gw"}; + auto const USD = gw["USD"]; + + // Make one test run without fixReducedOffersV1 and one with. + for (FeatureBitset features : + {supported_amendments() - fixReducedOffersV1, + supported_amendments() | fixReducedOffersV1}) + { + Env env{*this, features}; + + env.fund(XRP(10000), alice, bob, gw); + env.close(); + env.trust(USD(1000), alice, bob); + + int blockedOrderBookCount = 0; + for (STAmount initialBobUSD = USD(0.45); initialBobUSD <= USD(1); + initialBobUSD += USD(0.025)) + { + // underfund bob's offer + env(pay(gw, bob, initialBobUSD)); + env.close(); + + std::uint32_t const bobOfferSeq = env.seq(bob); + env(offer(bob, drops(2), USD(1))); + env.close(); + + // alice places an offer that would cross bob's if bob's were + // well funded. + std::uint32_t const aliceOfferSeq = env.seq(alice); + env(offer(alice, USD(1), drops(2))); + env.close(); + + // We want to detect order book blocking. If: + // 1. bob's offer is still in the ledger and + // 2. alice received no USD + // then we use that as evidence that bob's offer blocked the + // order book. + { + bool const bobsOfferGone = + !offerInLedger(env, bob, bobOfferSeq); + STAmount const aliceBalanceUSD = env.balance(alice, USD); + + // Sanity check the ledger if alice got USD. + if (aliceBalanceUSD.signum() > 0) + { + BEAST_EXPECT(aliceBalanceUSD == initialBobUSD); + BEAST_EXPECT(env.balance(bob, USD) == USD(0)); + BEAST_EXPECT(bobsOfferGone); + } + + // Track occurrences of order book blocking. + if (!bobsOfferGone && aliceBalanceUSD.signum() == 0) + { + ++blockedOrderBookCount; + } + + // In preparation for the next iteration clean up any + // leftover offers. + cleanupOldOffers( + env, alice, bob, aliceOfferSeq, bobOfferSeq); + + // Zero out alice's and bob's USD balances. + if (STAmount const aliceBalance = env.balance(alice, USD); + aliceBalance.signum() > 0) + env(pay(alice, gw, aliceBalance)); + + if (STAmount const bobBalance = env.balance(bob, USD); + bobBalance.signum() > 0) + env(pay(bob, gw, bobBalance)); + + env.close(); + } + } + + // If fixReducedOffersV1 is enabled, then none of the test cases + // should produce a potentially blocking rate. + // + // Also verify that if fixReducedOffersV1 is not enabled then + // some of the test cases produced a potentially blocking rate. + if (features[fixReducedOffersV1]) + { + BEAST_EXPECT(blockedOrderBookCount == 0); + } + else + { + BEAST_EXPECT(blockedOrderBookCount > 15); + } + } + } + + void + testUnderFundedIouIouQChange() + { + testcase("exercise underfunded IOU/IOU offer Q change"); + + // Bob places an IOU/IOU offer that is not fully funded. + // + // This unit test compares the behavior of this situation before and + // after applying the fixReducedOffersV1 amendment. + + using namespace jtx; + using namespace std::chrono_literals; + auto const alice = Account{"alice"}; + auto const bob = Account{"bob"}; + auto const gw = Account{"gw"}; + + auto const USD = gw["USD"]; + auto const EUR = gw["EUR"]; + + STAmount const tinyUSD(USD.issue(), /*mantissa*/ 1, /*exponent*/ -81); + + // Make one test run without fixReducedOffersV1 and one with. + for (FeatureBitset features : + {supported_amendments() - fixReducedOffersV1, + supported_amendments() | fixReducedOffersV1}) + { + Env env{*this, features}; + + env.fund(XRP(10000), alice, bob, gw); + env.close(); + env.trust(USD(1000), alice, bob); + env.trust(EUR(1000), alice, bob); + + STAmount const eurOffer( + EUR.issue(), /*mantissa*/ 2957, /*exponent*/ -76); + STAmount const usdOffer( + USD.issue(), /*mantissa*/ 7109, /*exponent*/ -76); + + STAmount const endLoop( + USD.issue(), /*mantissa*/ 50, /*exponent*/ -81); + + int blockedOrderBookCount = 0; + for (STAmount initialBobUSD = tinyUSD; initialBobUSD <= endLoop; + initialBobUSD += tinyUSD) + { + // underfund bob's offer + env(pay(gw, bob, initialBobUSD)); + env(pay(gw, alice, EUR(100))); + env.close(); + + // This offer is underfunded + std::uint32_t bobOfferSeq = env.seq(bob); + env(offer(bob, eurOffer, usdOffer)); + env.close(); + env.require(offers(bob, 1)); + + // alice places an offer that crosses bob's. + std::uint32_t aliceOfferSeq = env.seq(alice); + env(offer(alice, usdOffer, eurOffer)); + env.close(); + + // Examine the aftermath of alice's offer. + { + bool const bobsOfferGone = + !offerInLedger(env, bob, bobOfferSeq); + STAmount aliceBalanceUSD = env.balance(alice, USD); +#if 0 + std::cout + << "bobs initial: " << initialBobUSD + << "; alice final: " << aliceBalanceUSD + << "; bobs offer: " << bobsOfferJson.toStyledString() + << std::endl; +#endif + // Sanity check the ledger if alice got USD. + if (aliceBalanceUSD.signum() > 0) + { + BEAST_EXPECT(aliceBalanceUSD == initialBobUSD); + BEAST_EXPECT(env.balance(bob, USD) == USD(0)); + BEAST_EXPECT(bobsOfferGone); + } + + // Track occurrences of order book blocking. + if (!bobsOfferGone && aliceBalanceUSD.signum() == 0) + { + ++blockedOrderBookCount; + } + } + + // In preparation for the next iteration clean up any + // leftover offers. + cleanupOldOffers(env, alice, bob, aliceOfferSeq, bobOfferSeq); + + // Zero out alice's and bob's IOU balances. + auto zeroBalance = [&env, &gw]( + Account const& acct, IOU const& iou) { + if (STAmount const balance = env.balance(acct, iou); + balance.signum() > 0) + env(pay(acct, gw, balance)); + }; + + zeroBalance(alice, EUR); + zeroBalance(alice, USD); + zeroBalance(bob, EUR); + zeroBalance(bob, USD); + env.close(); + } + + // If fixReducedOffersV1 is enabled, then none of the test cases + // should produce a potentially blocking rate. + // + // Also verify that if fixReducedOffersV1 is not enabled then + // some of the test cases produced a potentially blocking rate. + if (features[fixReducedOffersV1]) + { + BEAST_EXPECT(blockedOrderBookCount == 0); + } + else + { + BEAST_EXPECT(blockedOrderBookCount > 20); + } + } + } + + void + run() override + { + testPartialCrossNewXrpIouQChange(); + testPartialCrossOldXrpIouQChange(); + testUnderFundedXrpIouQChange(); + testUnderFundedIouIouQChange(); + } +}; + +BEAST_DEFINE_TESTSUITE_PRIO(ReducedOffer, tx, ripple, 2); + +} // namespace test +} // namespace ripple