diff --git a/src/ripple/app/tx/impl/OfferStream.cpp b/src/ripple/app/tx/impl/OfferStream.cpp index 370ea46c530..58fd209ca0b 100644 --- a/src/ripple/app/tx/impl/OfferStream.cpp +++ b/src/ripple/app/tx/impl/OfferStream.cpp @@ -19,6 +19,7 @@ #include #include +#include namespace ripple { @@ -132,6 +133,73 @@ accountFundsHelper( view, id, issue.currency, issue.account, freezeHandling, j)); } +template +template +bool +TOfferStreamBase::shouldRmSmallIncreasedQOffer() const +{ + static_assert( + std::is_same_v || + std::is_same_v, + "STAmount is not supported"); + + static_assert( + std::is_same_v || + std::is_same_v, + "STAmount is not supported"); + + static_assert( + !std::is_same_v || + !std::is_same_v, + "Cannot have XRP/XRP offers"); + + if (!view_.rules().enabled(fixRmSmallIncreasedQOffers)) + return false; + + // Consider removing the offer if: + // o `TakerPays` is XRP (because of XRP drops granularity) or + // o `TakerPays` and `TakerGets` are both IOU and `TakerPays`<`TakerGets` + constexpr bool const inIsXRP = std::is_same_v; + constexpr bool const outIsXRP = std::is_same_v; + + if constexpr (outIsXRP) + { + // If `TakerGets` is XRP, the worst this offer's quality can change is + // to about 10^-81 `TakerPays` and 1 drop `TakerGets`. This will be + // remarkably good quality for any realistic asset, so these offers + // don't need this extra check. + return false; + } + + TAmounts const ofrAmts{ + toAmount(offer_.amount().in), + toAmount(offer_.amount().out)}; + + if constexpr (!inIsXRP && !outIsXRP) + { + if (ofrAmts.in >= ofrAmts.out) + return false; + } + + TTakerGets const ownerFunds = toAmount(*ownerFunds_); + + auto const effectiveAmounts = [&] { + if (offer_.owner() != offer_.issueOut().account && + ownerFunds < ofrAmts.out) + { + // adjust the amounts by owner funds + return offer_.quality().ceil_out(ofrAmts, ownerFunds); + } + return ofrAmts; + }(); + + if (effectiveAmounts.in > TTakerPays::minPositiveAmount()) + return false; + + Quality const effectiveQuality{effectiveAmounts}; + return effectiveQuality < offer_.quality(); +} + template bool TOfferStreamBase::step() @@ -222,6 +290,68 @@ TOfferStreamBase::step() << "Removing became unfunded offer " << entry->key(); } offer_ = TOffer{}; + // See comment at top of loop for how the offer is removed + continue; + } + + bool const rmSmallIncreasedQOffer = [&] { + bool const inIsXRP = isXRP(offer_.issueIn()); + bool const outIsXRP = isXRP(offer_.issueOut()); + if (inIsXRP && !outIsXRP) + { + // Without the `if constexpr`, the + // `shouldRmSmallIncreasedQOffer` template will be instantiated + // even if it is never used. This can cause compiler errors in + // some cases, hence the `if constexpr` guard. + // Note that TIn can be XRPAmount or STAmount, and TOut can be + // IOUAmount or STAmount. + if constexpr (!(std::is_same_v || + std::is_same_v)) + return shouldRmSmallIncreasedQOffer(); + } + if (!inIsXRP && outIsXRP) + { + // See comment above for `if constexpr` rationale + if constexpr (!(std::is_same_v || + std::is_same_v)) + return shouldRmSmallIncreasedQOffer(); + } + if (!inIsXRP && !outIsXRP) + { + // See comment above for `if constexpr` rationale + if constexpr (!(std::is_same_v || + std::is_same_v)) + return shouldRmSmallIncreasedQOffer(); + } + assert(0); // xrp/xrp offer!?! should never happen + return false; + }(); + + if (rmSmallIncreasedQOffer) + { + auto const original_funds = accountFundsHelper( + cancelView_, + offer_.owner(), + amount.out, + offer_.issueOut(), + fhZERO_IF_FROZEN, + j_); + + if (original_funds == *ownerFunds_) + { + permRmOffer(entry->key()); + JLOG(j_.trace()) + << "Removing tiny offer due to reduced quality " + << entry->key(); + } + else + { + JLOG(j_.trace()) << "Removing tiny offer that became tiny due " + "to reduced quality " + << entry->key(); + } + offer_ = TOffer{}; + // See comment at top of loop for how the offer is removed continue; } diff --git a/src/ripple/app/tx/impl/OfferStream.h b/src/ripple/app/tx/impl/OfferStream.h index 69946790cc3..9472bc4e74d 100644 --- a/src/ripple/app/tx/impl/OfferStream.h +++ b/src/ripple/app/tx/impl/OfferStream.h @@ -85,6 +85,10 @@ class TOfferStreamBase virtual void permRmOffer(uint256 const& offerIndex) = 0; + template + bool + shouldRmSmallIncreasedQOffer() const; + public: TOfferStreamBase( ApplyView& view, diff --git a/src/ripple/basics/IOUAmount.h b/src/ripple/basics/IOUAmount.h index 46a37d23a1a..7e9e50d7ee9 100644 --- a/src/ripple/basics/IOUAmount.h +++ b/src/ripple/basics/IOUAmount.h @@ -129,6 +129,9 @@ class IOUAmount : private boost::totally_ordered, { return mantissa_; } + + static IOUAmount + minPositiveAmount(); }; std::string diff --git a/src/ripple/basics/XRPAmount.h b/src/ripple/basics/XRPAmount.h index c9f0d08eab7..08f82b1752e 100644 --- a/src/ripple/basics/XRPAmount.h +++ b/src/ripple/basics/XRPAmount.h @@ -238,6 +238,12 @@ class XRPAmount : private boost::totally_ordered, s >> val.drops_; return s; } + + static XRPAmount + minPositiveAmount() + { + return XRPAmount{1}; + } }; /** Number of drops per 1 XRP */ diff --git a/src/ripple/basics/impl/IOUAmount.cpp b/src/ripple/basics/impl/IOUAmount.cpp index bb3c39ef315..27254662347 100644 --- a/src/ripple/basics/impl/IOUAmount.cpp +++ b/src/ripple/basics/impl/IOUAmount.cpp @@ -34,6 +34,12 @@ static std::int64_t const maxMantissa = 9999999999999999ull; static int const minExponent = -96; static int const maxExponent = 80; +IOUAmount +IOUAmount::minPositiveAmount() +{ + return IOUAmount(minMantissa, minExponent); +} + void IOUAmount::normalize() { @@ -353,7 +359,7 @@ mulRatio( { if (!result) { - return IOUAmount(minMantissa, minExponent); + return IOUAmount::minPositiveAmount(); } // This addition cannot overflow because the mantissa is already // normalized diff --git a/src/ripple/protocol/AmountConversions.h b/src/ripple/protocol/AmountConversions.h index 899d313fbcf..f60b976d57c 100644 --- a/src/ripple/protocol/AmountConversions.h +++ b/src/ripple/protocol/AmountConversions.h @@ -63,11 +63,7 @@ toSTAmount(XRPAmount const& xrp, Issue const& iss) template T -toAmount(STAmount const& amt) -{ - static_assert(sizeof(T) == -1, "Must use specialized function"); - return T(0); -} +toAmount(STAmount const& amt) = delete; template <> inline STAmount @@ -102,6 +98,28 @@ toAmount(STAmount const& amt) return XRPAmount(sMant); } +template +T +toAmount(IOUAmount const& amt) = delete; + +template <> +inline IOUAmount +toAmount(IOUAmount const& amt) +{ + return amt; +} + +template +T +toAmount(XRPAmount const& amt) = delete; + +template <> +inline XRPAmount +toAmount(XRPAmount const& amt) +{ + return amt; +} + } // namespace ripple #endif diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index 6864ff49ab8..2fbcc9789df 100644 --- a/src/ripple/protocol/Feature.h +++ b/src/ripple/protocol/Feature.h @@ -116,6 +116,7 @@ class FeatureCollections "TicketBatch", "FlowSortStrands", "fixSTAmountCanonicalize", + "fixRmSmallIncreasedQOffers", }; std::vector features; @@ -376,6 +377,7 @@ extern uint256 const featureNegativeUNL; extern uint256 const featureTicketBatch; extern uint256 const featureFlowSortStrands; extern uint256 const fixSTAmountCanonicalize; +extern uint256 const fixRmSmallIncreasedQOffers; } // namespace ripple diff --git a/src/ripple/protocol/Quality.h b/src/ripple/protocol/Quality.h index 27026b6740d..6324a8836a7 100644 --- a/src/ripple/protocol/Quality.h +++ b/src/ripple/protocol/Quality.h @@ -132,6 +132,13 @@ class Quality /** Create a quality from the ratio of two amounts. */ explicit Quality(Amounts const& amount); + /** Create a quality from the ratio of two amounts. */ + template + Quality(TAmounts const& amount) + : Quality(Amounts(toSTAmount(amount.in), toSTAmount(amount.out))) + { + } + /** Create a quality from the ratio of two amounts. */ template Quality(Out const& out, In const& in) diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index 87e739e2e6d..9f29dd1ba80 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -135,6 +135,7 @@ detail::supportedAmendments() "TicketBatch", "FlowSortStrands", "fixSTAmountCanonicalize", + "fixRmSmallIncreasedQOffers", }; return supported; } @@ -190,7 +191,8 @@ uint256 const featureNegativeUNL = *getRegisteredFeature("NegativeUNL"), featureTicketBatch = *getRegisteredFeature("TicketBatch"), featureFlowSortStrands = *getRegisteredFeature("FlowSortStrands"), - fixSTAmountCanonicalize = *getRegisteredFeature("fixSTAmountCanonicalize"); + fixSTAmountCanonicalize = *getRegisteredFeature("fixSTAmountCanonicalize"), + fixRmSmallIncreasedQOffers = *getRegisteredFeature("fixRmSmallIncreasedQOffers"); // The following amendments have been active for at least two years. Their // pre-amendment code has been removed and the identifiers are deprecated. diff --git a/src/test/app/Offer_test.cpp b/src/test/app/Offer_test.cpp index f30fc19e161..e0d2de4e34f 100644 --- a/src/test/app/Offer_test.cpp +++ b/src/test/app/Offer_test.cpp @@ -320,17 +320,20 @@ class Offer_test : public beast::unit_test::suite Env env{*this, features}; env.fund(XRP(10000), alice, bob, carol, dan, erin, gw); + env.close(); env.trust(USD(1000), alice, bob, carol, dan, erin); + env.close(); env(pay(gw, carol, USD(0.99999))); env(pay(gw, dan, USD(1))); env(pay(gw, erin, USD(1))); + env.close(); // Carol doesn't quite have enough funds for this offer // The amount left after this offer is taken will cause // STAmount to incorrectly round to zero when the next offer - // (at a good quality) is considered. (when the - // stAmountCalcSwitchover2 patch is inactive) - env(offer(carol, drops(1), USD(1))); + // (at a good quality) is considered. (when the now removed + // stAmountCalcSwitchover2 patch was inactive) + env(offer(carol, drops(1), USD(0.99999))); // Offer at a quality poor enough so when the input xrp is // calculated in the reverse pass, the amount is not zero. env(offer(dan, XRP(100), USD(1))); @@ -340,9 +343,9 @@ class Offer_test : public beast::unit_test::suite // It is considered after the offer from carol, which leaves a // tiny amount left to pay. When calculating the amount of xrp // needed for this offer, it will incorrectly compute zero in both - // the forward and reverse passes (when the stAmountCalcSwitchover2 - // is inactive.) - env(offer(erin, drops(2), USD(2))); + // the forward and reverse passes (when the now removed + // stAmountCalcSwitchover2 was inactive.) + env(offer(erin, drops(2), USD(1))); env(pay(alice, bob, USD(1)), path(~USD), @@ -356,6 +359,317 @@ class Offer_test : public beast::unit_test::suite env.require(balance(erin, USD(0.99999)), offers(erin, 1)); } + void + testRmSmallIncreasedQOffersXRP(FeatureBitset features) + { + testcase("Rm small increased q offers XRP"); + + // Carol places an offer, but cannot fully fund the offer. When her + // funding is taken into account, the offer's quality drops below its + // initial quality and has an input amount of 1 drop. This is removed as + // an offer that may block offer books. + + 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"]; + + // Test offer crossing + for (auto crossBothOffers : {false, true}) + { + Env env{*this, features}; + + env.fund(XRP(10000), alice, bob, carol, gw); + env.close(); + env.trust(USD(1000), alice, bob, carol); + // underfund carol's offer + auto initialCarolUSD = USD(0.499); + env(pay(gw, carol, initialCarolUSD)); + env(pay(gw, bob, USD(100))); + env.close(); + // This offer is underfunded + env(offer(carol, drops(1), USD(1))); + env.close(); + // offer at a lower quality + env(offer(bob, drops(2), USD(1), tfPassive)); + env.close(); + env.require(offers(bob, 1), offers(carol, 1)); + + // alice places an offer that crosses carol's; depending on + // "crossBothOffers" it may cross bob's as well + auto aliceTakerGets = crossBothOffers ? drops(2) : drops(1); + env(offer(alice, USD(1), aliceTakerGets)); + env.close(); + + if (features[fixRmSmallIncreasedQOffers]) + { + env.require( + offers(carol, 0), + balance( + carol, + initialCarolUSD)); // offer is removed but not taken + if (crossBothOffers) + { + env.require( + offers(alice, 0), + balance(alice, USD(1))); // alice's offer is crossed + } + else + { + env.require( + offers(alice, 1), + balance( + alice, USD(0))); // alice's offer is not crossed + } + } + else + { + env.require( + offers(alice, 1), + offers(bob, 1), + offers(carol, 1), + balance(alice, USD(0)), + balance( + carol, + initialCarolUSD)); // offer is not crossed at all + } + } + + // Test payments + for (auto partialPayment : {false, true}) + { + Env env{*this, features}; + + env.fund(XRP(10000), alice, bob, carol, gw); + env.close(); + env.trust(USD(1000), alice, bob, carol); + env.close(); + auto const initialCarolUSD = USD(0.999); + env(pay(gw, carol, initialCarolUSD)); + env.close(); + env(pay(gw, bob, USD(100))); + env.close(); + env(offer(carol, drops(1), USD(1))); + env.close(); + env(offer(bob, drops(2), USD(2), tfPassive)); + env.close(); + env.require(offers(bob, 1), offers(carol, 1)); + + std::uint32_t const flags = partialPayment + ? (tfNoRippleDirect | tfPartialPayment) + : tfNoRippleDirect; + + TER const expectedTer = + partialPayment ? TER{tesSUCCESS} : TER{tecPATH_PARTIAL}; + + env(pay(alice, bob, USD(5)), + path(~USD), + sendmax(XRP(1)), + txflags(flags), + ter(expectedTer)); + env.close(); + + if (features[fixRmSmallIncreasedQOffers]) + { + if (expectedTer == tesSUCCESS) + { + env.require(offers(carol, 0)); + env.require(balance( + carol, + initialCarolUSD)); // offer is removed but not taken + } + else + { + // TODO: Offers are not removed when payments fail + // If that is addressed, the test should show that carol's + // offer is removed but not taken, as in the other branch of + // this if statement + } + } + else + { + if (partialPayment) + { + env.require(offers(carol, 0)); + env.require( + balance(carol, USD(0))); // offer is removed and taken + } + else + { + // offer is not removed or taken + BEAST_EXPECT(isOffer(env, carol, drops(1), USD(1))); + } + } + } + } + + void + testRmSmallIncreasedQOffersIOU(FeatureBitset features) + { + testcase("Rm small increased q offers IOU"); + + // Carol places an offer, but cannot fully fund the offer. When her + // funding is taken into account, the offer's quality drops below its + // initial quality and has an input amount of 1 drop. This is removed as + // an offer that may block offer books. + + 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"]; + auto const EUR = gw["EUR"]; + + auto tinyAmount = [&](IOU const& iou) -> PrettyAmount { + STAmount amt( + iou.issue(), + /*mantissa*/ 1, + /*exponent*/ -81); + return PrettyAmount(amt, iou.account.name()); + }; + + // Test offer crossing + for (auto crossBothOffers : {false, true}) + { + Env env{*this, features}; + + env.fund(XRP(10000), alice, bob, carol, gw); + env.close(); + env.trust(USD(1000), alice, bob, carol); + env.trust(EUR(1000), alice, bob, carol); + // underfund carol's offer + auto initialCarolUSD = tinyAmount(USD); + env(pay(gw, carol, initialCarolUSD)); + env(pay(gw, bob, USD(100))); + env(pay(gw, alice, EUR(100))); + env.close(); + // This offer is underfunded + env(offer(carol, EUR(1), USD(10))); + env.close(); + // offer at a lower quality + env(offer(bob, EUR(1), USD(5), tfPassive)); + env.close(); + env.require(offers(bob, 1), offers(carol, 1)); + + // alice places an offer that crosses carol's; depending on + // "crossBothOffers" it may cross bob's as well + // Whatever + auto aliceTakerGets = crossBothOffers ? EUR(0.2) : EUR(0.1); + env(offer(alice, USD(1), aliceTakerGets)); + env.close(); + + if (features[fixRmSmallIncreasedQOffers]) + { + env.require( + offers(carol, 0), + balance( + carol, + initialCarolUSD)); // offer is removed but not taken + if (crossBothOffers) + { + env.require( + offers(alice, 0), + balance(alice, USD(1))); // alice's offer is crossed + } + else + { + env.require( + offers(alice, 1), + balance( + alice, USD(0))); // alice's offer is not crossed + } + } + else + { + env.require( + offers(alice, 1), + offers(bob, 1), + offers(carol, 1), + balance(alice, USD(0)), + balance( + carol, + initialCarolUSD)); // offer is not crossed at all + } + } + + // Test payments + for (auto partialPayment : {false, true}) + { + Env env{*this, features}; + + env.fund(XRP(10000), alice, bob, carol, gw); + env.close(); + env.trust(USD(1000), alice, bob, carol); + env.trust(EUR(1000), alice, bob, carol); + env.close(); + // underfund carol's offer + auto const initialCarolUSD = tinyAmount(USD); + env(pay(gw, carol, initialCarolUSD)); + env(pay(gw, bob, USD(100))); + env(pay(gw, alice, EUR(100))); + env.close(); + // This offer is underfunded + env(offer(carol, EUR(1), USD(2))); + env.close(); + env(offer(bob, EUR(2), USD(4), tfPassive)); + env.close(); + env.require(offers(bob, 1), offers(carol, 1)); + + std::uint32_t const flags = partialPayment + ? (tfNoRippleDirect | tfPartialPayment) + : tfNoRippleDirect; + + TER const expectedTer = + partialPayment ? TER{tesSUCCESS} : TER{tecPATH_PARTIAL}; + + env(pay(alice, bob, USD(5)), + path(~USD), + sendmax(EUR(10)), + txflags(flags), + ter(expectedTer)); + env.close(); + + if (features[fixRmSmallIncreasedQOffers]) + { + if (expectedTer == tesSUCCESS) + { + env.require(offers(carol, 0)); + env.require(balance( + carol, + initialCarolUSD)); // offer is removed but not taken + } + else + { + // TODO: Offers are not removed when payments fail + // If that is addressed, the test should show that carol's + // offer is removed but not taken, as in the other branch of + // this if statement + } + } + else + { + if (partialPayment) + { + env.require(offers(carol, 0)); + env.require( + balance(carol, USD(0))); // offer is removed and taken + } + else + { + // offer is not removed or taken + BEAST_EXPECT(isOffer(env, carol, EUR(1), USD(2))); + } + } + } + } + void testEnforceNoRipple(FeatureBitset features) { @@ -5387,7 +5701,7 @@ class Offer_test : public beast::unit_test::suite { // An assert was falsely triggering when computing rates for offers. // This unit test would trigger that assert (which has been removed). - testcase("false assert"); + testcase("incorrect assert fixed"); using namespace jtx; Env env{*this}; @@ -5459,6 +5773,8 @@ class Offer_test : public beast::unit_test::suite testTickSize(features); testTicketOffer(features); testTicketCancelOffer(features); + testRmSmallIncreasedQOffersXRP(features); + testRmSmallIncreasedQOffersIOU(features); } void @@ -5468,10 +5784,12 @@ class Offer_test : public beast::unit_test::suite FeatureBitset const all{supported_amendments()}; FeatureBitset const flowCross{featureFlowCross}; FeatureBitset const takerDryOffer{fixTakerDryOfferRemoval}; + FeatureBitset const rmSmallIncreasedQOffers{fixRmSmallIncreasedQOffers}; testAll(all - takerDryOffer); testAll(all - flowCross - takerDryOffer); testAll(all - flowCross); + testAll(all - rmSmallIncreasedQOffers); testAll(all); testFalseAssert(); }