From e1bf2c956ab0ea2cd04f1a582408c194d50fb944 Mon Sep 17 00:00:00 2001 From: seelabs Date: Mon, 19 Apr 2021 21:49:15 -0400 Subject: [PATCH 1/5] Rm some offers where the quality is reduced: Substantial reductions in an offer's effective quality from its initial quality may clog offer books. --- src/ripple/app/tx/impl/OfferStream.cpp | 117 +++++++++++ src/ripple/app/tx/impl/OfferStream.h | 4 + src/ripple/basics/IOUAmount.h | 3 + src/ripple/basics/XRPAmount.h | 6 + src/ripple/basics/impl/IOUAmount.cpp | 8 +- src/ripple/protocol/AmountConversions.h | 30 +++ src/ripple/protocol/Feature.h | 2 + src/ripple/protocol/Quality.h | 7 + src/ripple/protocol/impl/Feature.cpp | 4 +- src/test/app/Offer_test.cpp | 252 +++++++++++++++++++++++- 10 files changed, 429 insertions(+), 4 deletions(-) diff --git a/src/ripple/app/tx/impl/OfferStream.cpp b/src/ripple/app/tx/impl/OfferStream.cpp index 370ea46c530..73ce7d10512 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,68 @@ 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, + ""); + + static_assert( + std::is_same_v || + std::is_same_v, + ""); + + 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 `TakerPays` is XRP or `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 (outIsXRP) + 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; + }(); + + TTakerPays const thresh = TTakerPays::minPositiveAmount(); + + if (effectiveAmounts.in > thresh) + return false; + + Quality const effectiveQuality{effectiveAmounts}; + return effectiveQuality < offer_.quality(); +} + template bool TOfferStreamBase::step() @@ -222,6 +285,60 @@ TOfferStreamBase::step() << "Removing became unfunded offer " << entry->key(); } offer_ = TOffer{}; + // See comment at top of loop for why the offer is removed + continue; + } + + bool const rmSmallIncreasedQOffer = [&] { + bool const inIsXRP = isXRP(offer_.issueIn()); + bool const outIsXRP = isXRP(offer_.issueOut()); + if (inIsXRP && !outIsXRP) + { + if constexpr (!(std::is_same_v || + std::is_same_v)) + return shouldRmSmallIncreasedQOffer(); + } + if (!inIsXRP && outIsXRP) + { + if constexpr (!(std::is_same_v || + std::is_same_v)) + return shouldRmSmallIncreasedQOffer(); + } + if (!inIsXRP && !outIsXRP) + { + 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 became tiny offer due to " + "reduced quality " + << entry->key(); + } + offer_ = TOffer{}; + // See comment at top of loop for why 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..60b57ee9b1a 100644 --- a/src/ripple/protocol/AmountConversions.h +++ b/src/ripple/protocol/AmountConversions.h @@ -102,6 +102,36 @@ toAmount(STAmount const& amt) return XRPAmount(sMant); } +template +T +toAmount(IOUAmount const& amt) +{ + static_assert(sizeof(T) == -1, "Must use specialized function"); + return T(0); +} + +template <> +inline IOUAmount +toAmount(IOUAmount const& amt) +{ + return amt; +} + +template +T +toAmount(XRPAmount const& amt) +{ + static_assert(sizeof(T) == -1, "Must use specialized function"); + return T(0); +} + +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..f4dd2bf2d03 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))); + 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))); @@ -342,7 +345,7 @@ class Offer_test : public beast::unit_test::suite // 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))); + env(offer(erin, drops(2), USD(1))); env(pay(alice, bob, USD(1)), path(~USD), @@ -356,6 +359,247 @@ 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.1); + 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.1); + 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 + } + } + } + void testEnforceNoRipple(FeatureBitset features) { @@ -5459,6 +5703,8 @@ class Offer_test : public beast::unit_test::suite testTickSize(features); testTicketOffer(features); testTicketCancelOffer(features); + testRmSmallIncreasedQOffersXRP(features); + testRmSmallIncreasedQOffersIOU(features); } void @@ -5468,10 +5714,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(); } From 8ad2fd5cad6c37f257e73798013fda55d90896d4 Mon Sep 17 00:00:00 2001 From: seelabs Date: Thu, 22 Apr 2021 10:25:06 -0400 Subject: [PATCH 2/5] [fold] Add IOU/IOU payment test & some comments --- src/ripple/app/tx/impl/OfferStream.cpp | 12 +++++ src/test/app/Offer_test.cpp | 70 ++++++++++++++++++++++++++ 2 files changed, 82 insertions(+) diff --git a/src/ripple/app/tx/impl/OfferStream.cpp b/src/ripple/app/tx/impl/OfferStream.cpp index 73ce7d10512..e33dcec5147 100644 --- a/src/ripple/app/tx/impl/OfferStream.cpp +++ b/src/ripple/app/tx/impl/OfferStream.cpp @@ -162,7 +162,13 @@ TOfferStreamBase::shouldRmSmallIncreasedQOffer() const constexpr bool const outIsXRP = std::is_same_v; if (outIsXRP) + { + // If `TakerGets` is XRP, worse 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), @@ -294,18 +300,24 @@ TOfferStreamBase::step() 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, hense the `if constexpr` guard. if constexpr (!(std::is_same_v || std::is_same_v)) return shouldRmSmallIncreasedQOffer(); } if (!inIsXRP && outIsXRP) { + // See comment above for `if constexpr` rational if constexpr (!(std::is_same_v || std::is_same_v)) return shouldRmSmallIncreasedQOffer(); } if (!inIsXRP && !outIsXRP) { + // See comment above for `if constexpr` rational if constexpr (!(std::is_same_v || std::is_same_v)) return shouldRmSmallIncreasedQOffer(); diff --git a/src/test/app/Offer_test.cpp b/src/test/app/Offer_test.cpp index f4dd2bf2d03..3f7ac66ee50 100644 --- a/src/test/app/Offer_test.cpp +++ b/src/test/app/Offer_test.cpp @@ -598,6 +598,76 @@ class Offer_test : public beast::unit_test::suite 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 From 9b8c050cb348b28c6f4da43b25516ab66e316e0f Mon Sep 17 00:00:00 2001 From: seelabs Date: Tue, 27 Apr 2021 16:02:20 -0400 Subject: [PATCH 3/5] [fold] Minor cleanups --- src/ripple/app/tx/impl/OfferStream.cpp | 21 +++++++++++---------- src/test/app/Offer_test.cpp | 2 +- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/src/ripple/app/tx/impl/OfferStream.cpp b/src/ripple/app/tx/impl/OfferStream.cpp index e33dcec5147..435cf6ccc66 100644 --- a/src/ripple/app/tx/impl/OfferStream.cpp +++ b/src/ripple/app/tx/impl/OfferStream.cpp @@ -141,12 +141,12 @@ 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 || @@ -156,15 +156,16 @@ TOfferStreamBase::shouldRmSmallIncreasedQOffer() const if (!view_.rules().enabled(fixRmSmallIncreasedQOffers)) return false; - // Consider removing the offer if `TakerPays` is XRP or `TakerPays` and - // `TakerGets` are both IOU and `TakerPays` < `TakerGets` + // 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 (outIsXRP) + if constexpr (outIsXRP) { - // If `TakerGets` is XRP, worse this offer's quality can change is to - // about 10^-81 `TakerPays` and 1 drop `TakerGets`. This will be + // 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; @@ -291,7 +292,7 @@ TOfferStreamBase::step() << "Removing became unfunded offer " << entry->key(); } offer_ = TOffer{}; - // See comment at top of loop for why the offer is removed + // See comment at top of loop for how the offer is removed continue; } @@ -303,7 +304,7 @@ TOfferStreamBase::step() // Without the `if constexpr`, the // `shouldRmSmallIncreasedQOffer` template will be instantiated // even if it is never used. This can cause compiler errors in - // some cases, hense the `if constexpr` guard. + // some cases, hence the `if constexpr` guard. if constexpr (!(std::is_same_v || std::is_same_v)) return shouldRmSmallIncreasedQOffer(); @@ -350,7 +351,7 @@ TOfferStreamBase::step() << entry->key(); } offer_ = TOffer{}; - // See comment at top of loop for why the offer is removed + // See comment at top of loop for how the offer is removed continue; } diff --git a/src/test/app/Offer_test.cpp b/src/test/app/Offer_test.cpp index 3f7ac66ee50..f5584ea7adb 100644 --- a/src/test/app/Offer_test.cpp +++ b/src/test/app/Offer_test.cpp @@ -5701,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}; From d77ad06020a6ccb8d98f9354a576ddd548747fe0 Mon Sep 17 00:00:00 2001 From: seelabs Date: Tue, 27 Apr 2021 16:09:32 -0400 Subject: [PATCH 4/5] [fold] Change test params so they're closer to behavior change thresholds --- src/test/app/Offer_test.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/app/Offer_test.cpp b/src/test/app/Offer_test.cpp index f5584ea7adb..01c33effd7d 100644 --- a/src/test/app/Offer_test.cpp +++ b/src/test/app/Offer_test.cpp @@ -387,7 +387,7 @@ class Offer_test : public beast::unit_test::suite env.close(); env.trust(USD(1000), alice, bob, carol); // underfund carol's offer - auto initialCarolUSD = USD(0.1); + auto initialCarolUSD = USD(0.499); env(pay(gw, carol, initialCarolUSD)); env(pay(gw, bob, USD(100))); env.close(); @@ -448,7 +448,7 @@ class Offer_test : public beast::unit_test::suite env.close(); env.trust(USD(1000), alice, bob, carol); env.close(); - auto const initialCarolUSD = USD(0.1); + auto const initialCarolUSD = USD(0.999); env(pay(gw, carol, initialCarolUSD)); env.close(); env(pay(gw, bob, USD(100))); From a06ca4c9518b7de67ffbbfc329648f985ada32c1 Mon Sep 17 00:00:00 2001 From: seelabs Date: Thu, 6 May 2021 13:30:54 -0400 Subject: [PATCH 5/5] Misc cleanups (Nik's review): * Inline minPositiveAmount * Add some clarifying comments * Fix mispelling * Use deleted function instead of static_assert --- src/ripple/app/tx/impl/OfferStream.cpp | 14 +++++++------- src/ripple/protocol/AmountConversions.h | 18 +++--------------- src/test/app/Offer_test.cpp | 8 ++++---- 3 files changed, 14 insertions(+), 26 deletions(-) diff --git a/src/ripple/app/tx/impl/OfferStream.cpp b/src/ripple/app/tx/impl/OfferStream.cpp index 435cf6ccc66..58fd209ca0b 100644 --- a/src/ripple/app/tx/impl/OfferStream.cpp +++ b/src/ripple/app/tx/impl/OfferStream.cpp @@ -193,9 +193,7 @@ TOfferStreamBase::shouldRmSmallIncreasedQOffer() const return ofrAmts; }(); - TTakerPays const thresh = TTakerPays::minPositiveAmount(); - - if (effectiveAmounts.in > thresh) + if (effectiveAmounts.in > TTakerPays::minPositiveAmount()) return false; Quality const effectiveQuality{effectiveAmounts}; @@ -305,20 +303,22 @@ TOfferStreamBase::step() // `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` rational + // 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` rational + // See comment above for `if constexpr` rationale if constexpr (!(std::is_same_v || std::is_same_v)) return shouldRmSmallIncreasedQOffer(); @@ -346,8 +346,8 @@ TOfferStreamBase::step() } else { - JLOG(j_.trace()) << "Removing became tiny offer due to " - "reduced quality " + JLOG(j_.trace()) << "Removing tiny offer that became tiny due " + "to reduced quality " << entry->key(); } offer_ = TOffer{}; diff --git a/src/ripple/protocol/AmountConversions.h b/src/ripple/protocol/AmountConversions.h index 60b57ee9b1a..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 @@ -104,11 +100,7 @@ toAmount(STAmount const& amt) template T -toAmount(IOUAmount const& amt) -{ - static_assert(sizeof(T) == -1, "Must use specialized function"); - return T(0); -} +toAmount(IOUAmount const& amt) = delete; template <> inline IOUAmount @@ -119,11 +111,7 @@ toAmount(IOUAmount const& amt) template T -toAmount(XRPAmount const& amt) -{ - static_assert(sizeof(T) == -1, "Must use specialized function"); - return T(0); -} +toAmount(XRPAmount const& amt) = delete; template <> inline XRPAmount diff --git a/src/test/app/Offer_test.cpp b/src/test/app/Offer_test.cpp index 01c33effd7d..e0d2de4e34f 100644 --- a/src/test/app/Offer_test.cpp +++ b/src/test/app/Offer_test.cpp @@ -331,8 +331,8 @@ class Offer_test : public beast::unit_test::suite // 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) + // (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. @@ -343,8 +343,8 @@ 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.) + // 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)),