From ea482217dc1d39d66a2c64634369e24d84119bf3 Mon Sep 17 00:00:00 2001 From: Scott Schurr Date: Fri, 23 Jun 2023 08:20:25 +0300 Subject: [PATCH] `fixReducedOffersV1`: prevent offers from blocking order books: (#4512) Curtail the occurrence of order books that are blocked by reduced offers with the implementation of the fixReducedOffersV1 amendment. This commit identifies three ways in which offers can be reduced: 1. A new offer can be partially crossed by existing offers, so the new offer is reduced when placed in the ledger. 2. An in-ledger offer can be partially crossed by a new offer in a transaction. So the in-ledger offer is reduced by the new offer. 3. An in-ledger offer may be under-funded. In this case the in-ledger offer is scaled down to match the available funds. Reduced offers can block order books if the effective quality of the reduced offer is worse than the quality of the original offer (from the perspective of the taker). It turns out that, for small values, the quality of the reduced offer can be significantly affected by the rounding mode used during scaling computations. This commit adjusts some rounding modes so that the quality of a reduced offer is always at least as good (from the taker's perspective) as the original offer. The amendment is titled fixReducedOffersV1 because additional ways of producing reduced offers may come to light. Therefore, there may be a future need for a V2 amendment. --- Builds/CMake/RippledCore.cmake | 1 + src/ripple/app/paths/impl/BookStep.cpp | 30 +- src/ripple/app/tx/impl/CreateOffer.cpp | 18 +- src/ripple/app/tx/impl/OfferStream.cpp | 18 +- src/ripple/protocol/Feature.h | 3 +- src/ripple/protocol/Quality.h | 23 + src/ripple/protocol/STAmount.h | 19 +- src/ripple/protocol/impl/Feature.cpp | 1 + src/ripple/protocol/impl/Quality.cpp | 29 +- src/ripple/protocol/impl/STAmount.cpp | 189 +++++++- src/test/app/Offer_test.cpp | 13 +- src/test/app/ReducedOffer_test.cpp | 622 +++++++++++++++++++++++++ 12 files changed, 938 insertions(+), 28 deletions(-) create mode 100644 src/test/app/ReducedOffer_test.cpp 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