From ab45a8a737680f1fb39eb336732a7fbedd431254 Mon Sep 17 00:00:00 2001 From: Jingchen Date: Fri, 31 Oct 2025 20:25:05 +0000 Subject: [PATCH] refactor: Retire fixReducedOffersV1 amendment (#5972) Amendments activated for more than 2 years can be retired. This change retires the fixReducedOffersV1 amendment. --- include/xrpl/protocol/detail/features.macro | 2 +- src/test/app/ReducedOffer_test.cpp | 94 ++++----------------- src/xrpld/app/paths/detail/AMMOffer.cpp | 11 +-- src/xrpld/app/tx/detail/CreateOffer.cpp | 18 +--- src/xrpld/app/tx/detail/Offer.h | 11 +-- src/xrpld/app/tx/detail/OfferStream.cpp | 16 +--- 6 files changed, 29 insertions(+), 123 deletions(-) diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index b6bdcd5c06..4594a4f856 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -77,7 +77,6 @@ XRPL_FIX (DisallowIncomingV1, Supported::yes, VoteBehavior::DefaultNo XRPL_FEATURE(XChainBridge, Supported::yes, VoteBehavior::DefaultNo) XRPL_FEATURE(AMM, Supported::yes, VoteBehavior::DefaultNo) XRPL_FEATURE(Clawback, Supported::yes, VoteBehavior::DefaultNo) -XRPL_FIX (ReducedOffersV1, Supported::yes, VoteBehavior::DefaultNo) XRPL_FIX (NFTokenRemint, Supported::yes, VoteBehavior::DefaultNo) XRPL_FIX (NonFungibleTokensV1_2, Supported::yes, VoteBehavior::DefaultNo) XRPL_FIX (UniversalNumber, Supported::yes, VoteBehavior::DefaultNo) @@ -141,6 +140,7 @@ XRPL_RETIRE(fix1781) XRPL_RETIRE(fixAmendmentMajorityCalc) XRPL_RETIRE(fixCheckThreading) XRPL_RETIRE(fixQualityUpperBound) +XRPL_RETIRE(fixReducedOffersV1) XRPL_RETIRE(fixRmSmallIncreasedQOffers) XRPL_RETIRE(fixSTAmountCanonicalize) XRPL_RETIRE(fixTakerDryOfferRemoval) diff --git a/src/test/app/ReducedOffer_test.cpp b/src/test/app/ReducedOffer_test.cpp index fa2be451fa..c991daef85 100644 --- a/src/test/app/ReducedOffer_test.cpp +++ b/src/test/app/ReducedOffer_test.cpp @@ -80,12 +80,8 @@ public: auto const bob = Account{"bob"}; auto const USD = gw["USD"]; - // Make one test run without fixReducedOffersV1 and one with. - for (FeatureBitset features : - {testable_amendments() - fixReducedOffersV1, - testable_amendments() | fixReducedOffersV1}) { - Env env{*this, features}; + Env env{*this, testable_amendments()}; // Make sure none of the offers we generate are under funded. env.fund(XRP(10'000'000), gw, alice, bob); @@ -208,19 +204,9 @@ public: 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); - } + // None of the test cases should produce a potentially blocking + // rate. + BEAST_EXPECT(blockedCount == 0); } } @@ -236,13 +222,9 @@ public: auto const bob = Account{"bob"}; auto const USD = gw["USD"]; - // Make one test run without fixReducedOffersV1 and one with. - for (FeatureBitset features : - {testable_amendments() - fixReducedOffersV1, - testable_amendments() | fixReducedOffersV1}) { // Make sure none of the offers we generate are under funded. - Env env{*this, features}; + Env env{*this, testable_amendments()}; env.fund(XRP(10'000'000), gw, alice, bob); env.close(); @@ -367,19 +349,9 @@ public: 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); - } + // None of the test cases should produce a potentially blocking + // rate. + BEAST_EXPECT(blockedCount == 0); } } @@ -389,9 +361,6 @@ public: 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"}; @@ -399,12 +368,8 @@ public: auto const gw = Account{"gw"}; auto const USD = gw["USD"]; - // Make one test run without fixReducedOffersV1 and one with. - for (FeatureBitset features : - {testable_amendments() - fixReducedOffersV1, - testable_amendments() | fixReducedOffersV1}) { - Env env{*this, features}; + Env env{*this, testable_amendments()}; env.fund(XRP(10000), alice, bob, gw); env.close(); @@ -470,19 +435,9 @@ public: } } - // 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); - } + // None of the test cases should produce a potentially blocking + // rate. + BEAST_EXPECT(blockedOrderBookCount == 0); } } @@ -492,9 +447,6 @@ public: 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; @@ -507,12 +459,8 @@ public: STAmount const tinyUSD(USD.issue(), /*mantissa*/ 1, /*exponent*/ -81); - // Make one test run without fixReducedOffersV1 and one with. - for (FeatureBitset features : - {testable_amendments() - fixReducedOffersV1, - testable_amendments() | fixReducedOffersV1}) { - Env env{*this, features}; + Env env{*this, testable_amendments()}; env.fund(XRP(10000), alice, bob, gw); env.close(); @@ -594,19 +542,9 @@ public: 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); - } + // None of the test cases should produce a potentially blocking + // rate. + BEAST_EXPECT(blockedOrderBookCount == 0); } } diff --git a/src/xrpld/app/paths/detail/AMMOffer.cpp b/src/xrpld/app/paths/detail/AMMOffer.cpp index c719c0a92d..aa55a9759e 100644 --- a/src/xrpld/app/paths/detail/AMMOffer.cpp +++ b/src/xrpld/app/paths/detail/AMMOffer.cpp @@ -92,14 +92,9 @@ AMMOffer::limitOut( // poolPays * poolGets < (poolPays - assetOut) * (poolGets + assetIn) if (ammLiquidity_.multiPath()) { - if (auto const& rules = getCurrentTransactionRules(); - rules && 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. - return quality().ceil_out_strict(offrAmt, limit, roundUp); - return quality().ceil_out(offrAmt, limit); + // It turns out that the ceil_out implementation has some slop in + // it, which ceil_out_strict removes. + return quality().ceil_out_strict(offrAmt, limit, roundUp); } // Change the offer size according to the conservation function. The offer // quality is increased in this case, but it doesn't matter since there is diff --git a/src/xrpld/app/tx/detail/CreateOffer.cpp b/src/xrpld/app/tx/detail/CreateOffer.cpp index 6303918ef2..a7884a825c 100644 --- a/src/xrpld/app/tx/detail/CreateOffer.cpp +++ b/src/xrpld/app/tx/detail/CreateOffer.cpp @@ -477,22 +477,8 @@ CreateOffer::flowCross( // what is a good threshold to check? afterCross.in.clear(); - 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); - }(); + afterCross.out = divRoundStrict( + afterCross.in, rate, takerAmount.out.issue(), false); } else { diff --git a/src/xrpld/app/tx/detail/Offer.h b/src/xrpld/app/tx/detail/Offer.h index 58bd65ce46..8c2b14af6c 100644 --- a/src/xrpld/app/tx/detail/Offer.h +++ b/src/xrpld/app/tx/detail/Offer.h @@ -241,14 +241,9 @@ TOffer::limitOut( TOut const& limit, bool roundUp) const { - if (auto const& rules = getCurrentTransactionRules(); - rules && 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. - return quality().ceil_out_strict(offrAmt, limit, roundUp); - return m_quality.ceil_out(offrAmt, limit); + // It turns out that the ceil_out implementation has some slop in + // it, which ceil_out_strict removes. + return quality().ceil_out_strict(offrAmt, limit, roundUp); } template diff --git a/src/xrpld/app/tx/detail/OfferStream.cpp b/src/xrpld/app/tx/detail/OfferStream.cpp index 8f19c20cf5..9b638d322c 100644 --- a/src/xrpld/app/tx/detail/OfferStream.cpp +++ b/src/xrpld/app/tx/detail/OfferStream.cpp @@ -184,7 +184,6 @@ TOfferStreamBase::shouldRmSmallIncreasedQOffer() const } TTakerGets const ownerFunds = toAmount(*ownerFunds_); - bool const fixReduced = view_.rules().enabled(fixReducedOffersV1); auto const effectiveAmounts = [&] { if (offer_.owner() != offer_.issueOut().account && @@ -193,22 +192,15 @@ TOfferStreamBase::shouldRmSmallIncreasedQOffer() const // 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); + // the ceil_out() result. + return offer_.quality().ceil_out_strict( + ofrAmts, ownerFunds, /* roundUp */ false); } 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)) + if (effectiveAmounts.in.signum() <= 0 || effectiveAmounts.out.signum() <= 0) return true; if (effectiveAmounts.in > TTakerPays::minPositiveAmount())