From b124c9f7e3383d6a64a4d7623f5ea089dbef61d2 Mon Sep 17 00:00:00 2001 From: Jingchen Date: Tue, 25 Nov 2025 14:21:17 +0000 Subject: [PATCH] refactor: Retire Flow and FlowSortStrands amendments (#6054) Amendments activated for more than 2 years can be retired. This change retires the Flow and FlowSortStrands amendments. --- include/xrpl/protocol/detail/features.macro | 4 +- src/test/app/CrossingLimits_test.cpp | 39 ++++------------- src/test/app/TxQ_test.cpp | 4 +- src/test/jtx/Env_test.cpp | 20 ++++++--- src/test/ledger/View_test.cpp | 4 +- src/test/rpc/Feature_test.cpp | 20 ++++----- src/xrpld/app/paths/RippleCalc.cpp | 9 ---- src/xrpld/app/paths/detail/StrandFlow.h | 48 ++++----------------- 8 files changed, 47 insertions(+), 101 deletions(-) diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index 054b9d0a03..5f1eca7afe 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -64,8 +64,6 @@ XRPL_FEATURE(Clawback, Supported::yes, VoteBehavior::DefaultNo XRPL_FIX (UniversalNumber, Supported::yes, VoteBehavior::DefaultNo) XRPL_FEATURE(XRPFees, Supported::yes, VoteBehavior::DefaultNo) XRPL_FIX (RemoveNFTokenAutoTrustLine, Supported::yes, VoteBehavior::DefaultYes) -XRPL_FEATURE(FlowSortStrands, Supported::yes, VoteBehavior::DefaultYes) -XRPL_FEATURE(Flow, Supported::yes, VoteBehavior::DefaultYes) // The following amendments are obsolete, but must remain supported // because they could potentially get enabled. @@ -124,7 +122,9 @@ XRPL_RETIRE_FEATURE(Escrow) XRPL_RETIRE_FEATURE(EnforceInvariants) XRPL_RETIRE_FEATURE(ExpandedSignerList) XRPL_RETIRE_FEATURE(FeeEscalation) +XRPL_RETIRE_FEATURE(Flow) XRPL_RETIRE_FEATURE(FlowCross) +XRPL_RETIRE_FEATURE(FlowSortStrands) XRPL_RETIRE_FEATURE(HardenedValidations) XRPL_RETIRE_FEATURE(ImmediateOfferKilled) XRPL_RETIRE_FEATURE(MultiSign) diff --git a/src/test/app/CrossingLimits_test.cpp b/src/test/app/CrossingLimits_test.cpp index 8f0483b590..13d7aef505 100644 --- a/src/test/app/CrossingLimits_test.cpp +++ b/src/test/app/CrossingLimits_test.cpp @@ -267,9 +267,8 @@ public: // strand dry until the liquidity is actually used) // The implementation allows any single step to consume at most 1000 - // offers. With the `FlowSortStrands` feature enabled, if the total - // number of offers consumed by all the steps combined exceeds 1500, the - // payment stops. + // offers.If the total number of offers consumed by all the steps + // combined exceeds 1500, the payment stops. { Env env(*this, features); @@ -457,16 +456,12 @@ public: // below the limit. However, if all the offers are consumed it would // create a tecOVERSIZE error. - // The featureFlowSortStrands introduces a way of tracking the total - // number of consumed offers; with this feature the transaction no - // longer fails with a tecOVERSIZE error. // The implementation allows any single step to consume at most 1000 - // offers. With the `FlowSortStrands` feature enabled, if the total - // number of offers consumed by all the steps combined exceeds 1500, the - // payment stops. Since the first set of offers consumes 998 offers, the - // second set will consume 998, which is not over the limit and the - // payment stops. So 2*998, or 1996 is the expected value when - // `FlowSortStrands` is enabled. + // offers. If the total number of offers consumed by all the steps + // combined exceeds 1500, the payment stops. Since the first set of + // offers consumes 998 offers, the second set will consume 998, which is + // not over the limit and the payment stops. So 2*998, or 1996 is the + // expected value. n_offers(env, 998, alice, XRP(1.00), USD(1)); n_offers(env, 998, alice, XRP(0.99), USD(1)); n_offers(env, 998, alice, XRP(0.98), USD(1)); @@ -474,24 +469,10 @@ public: n_offers(env, 998, alice, XRP(0.96), USD(1)); n_offers(env, 998, alice, XRP(0.95), USD(1)); - bool const withSortStrands = features[featureFlowSortStrands]; - - auto const expectedTER = [&]() -> TER { - if (!withSortStrands) - return TER{tecOVERSIZE}; - return tesSUCCESS; - }(); - - env(offer(bob, USD(8000), XRP(8000)), ter(expectedTER)); + env(offer(bob, USD(8000), XRP(8000)), ter(tesSUCCESS)); env.close(); - auto const expectedUSD = [&] { - if (!withSortStrands) - return USD(0); - return USD(1996); - }(); - - env.require(balance(bob, expectedUSD)); + env.require(balance(bob, USD(1996))); } void @@ -507,9 +488,7 @@ public: using namespace jtx; auto const sa = testable_amendments(); testAll(sa); - testAll(sa - featureFlowSortStrands); testAll(sa - featurePermissionedDEX); - testAll(sa - featureFlowSortStrands - featurePermissionedDEX); } }; diff --git a/src/test/app/TxQ_test.cpp b/src/test/app/TxQ_test.cpp index 8e78969397..e59cf7deff 100644 --- a/src/test/app/TxQ_test.cpp +++ b/src/test/app/TxQ_test.cpp @@ -1054,7 +1054,7 @@ public: // Charlie - queue a transaction, with a higher fee // than default env(noop(charlie), fee(15), queued); - checkMetrics(*this, env, 6, initQueueMax, 4, 3); + checkMetrics(*this, env, 6, initQueueMax, 4, 3, 257); BEAST_EXPECT(env.seq(alice) == aliceSeq); BEAST_EXPECT(env.seq(bob) == bobSeq); @@ -4330,7 +4330,7 @@ public: Account const ellie("ellie"); Account const fiona("fiona"); - constexpr int ledgersInQueue = 20; + constexpr int ledgersInQueue = 30; auto cfg = makeConfig( {{"minimum_txn_in_ledger_standalone", "1"}, {"ledgers_in_queue", std::to_string(ledgersInQueue)}, diff --git a/src/test/jtx/Env_test.cpp b/src/test/jtx/Env_test.cpp index 5e4f534b6a..fbc4bd37a2 100644 --- a/src/test/jtx/Env_test.cpp +++ b/src/test/jtx/Env_test.cpp @@ -798,16 +798,18 @@ public: { // a Env FeatureBitset has *only* those features - Env env{*this, FeatureBitset{featureDynamicMPT | featureFlow}}; + Env env{ + *this, FeatureBitset{featureDynamicMPT | featureTokenEscrow}}; BEAST_EXPECT(env.app().config().features.size() == 2); foreachFeature(supported, [&](uint256 const& f) { - bool const has = (f == featureDynamicMPT || f == featureFlow); + bool const has = + (f == featureDynamicMPT || f == featureTokenEscrow); this->BEAST_EXPECT(has == hasFeature(env, f)); }); } auto const missingSomeFeatures = - testable_amendments() - featureDynamicMPT - featureFlow; + testable_amendments() - featureDynamicMPT - featureTokenEscrow; BEAST_EXPECT(missingSomeFeatures.count() == (supported.count() - 2)); { // a Env supported_features_except is missing *only* those features @@ -815,7 +817,8 @@ public: BEAST_EXPECT( env.app().config().features.size() == (supported.count() - 2)); foreachFeature(supported, [&](uint256 const& f) { - bool hasnot = (f == featureDynamicMPT || f == featureFlow); + bool hasnot = + (f == featureDynamicMPT || f == featureTokenEscrow); this->BEAST_EXPECT(hasnot != hasFeature(env, f)); }); } @@ -828,7 +831,9 @@ public: Env env{ *this, FeatureBitset{ - featureDynamicMPT, featureFlow, *neverSupportedFeat}}; + featureDynamicMPT, + featureTokenEscrow, + *neverSupportedFeat}}; // this app will have just 2 supported amendments and // one additional never supported feature flag @@ -836,7 +841,7 @@ public: BEAST_EXPECT(hasFeature(env, *neverSupportedFeat)); foreachFeature(supported, [&](uint256 const& f) { - bool has = (f == featureDynamicMPT || f == featureFlow); + bool has = (f == featureDynamicMPT || f == featureTokenEscrow); this->BEAST_EXPECT(has == hasFeature(env, f)); }); } @@ -856,7 +861,8 @@ public: (supported.count() - 2 + 1)); BEAST_EXPECT(hasFeature(env, *neverSupportedFeat)); foreachFeature(supported, [&](uint256 const& f) { - bool hasnot = (f == featureDynamicMPT || f == featureFlow); + bool hasnot = + (f == featureDynamicMPT || f == featureTokenEscrow); this->BEAST_EXPECT(hasnot != hasFeature(env, f)); }); } diff --git a/src/test/ledger/View_test.cpp b/src/test/ledger/View_test.cpp index ab978fda59..9c7a5a286e 100644 --- a/src/test/ledger/View_test.cpp +++ b/src/test/ledger/View_test.cpp @@ -1117,7 +1117,7 @@ class GetAmendments_test : public beast::unit_test::suite // There should be at least 3 amendments. Don't do exact comparison // to avoid maintenance as more amendments are added in the future. BEAST_EXPECT(i == 254); - BEAST_EXPECT(majorities.size() >= 3); + BEAST_EXPECT(majorities.size() >= 2); // None of the amendments should be enabled yet. auto enableds = getEnabledAmendments(*env.closed()); @@ -1135,7 +1135,7 @@ class GetAmendments_test : public beast::unit_test::suite break; } BEAST_EXPECT(i == 255); - BEAST_EXPECT(enableds.size() >= 3); + BEAST_EXPECT(enableds.size() >= 2); } void diff --git a/src/test/rpc/Feature_test.cpp b/src/test/rpc/Feature_test.cpp index e01f7566ac..058b4a9572 100644 --- a/src/test/rpc/Feature_test.cpp +++ b/src/test/rpc/Feature_test.cpp @@ -123,7 +123,7 @@ class Feature_test : public beast::unit_test::suite BEAST_EXPECT( featureToName(fixRemoveNFTokenAutoTrustLine) == "fixRemoveNFTokenAutoTrustLine"); - BEAST_EXPECT(featureToName(featureFlow) == "Flow"); + BEAST_EXPECT(featureToName(featureBatch) == "Batch"); BEAST_EXPECT(featureToName(featureDID) == "DID"); BEAST_EXPECT( featureToName(fixIncludeKeyletFields) == "fixIncludeKeyletFields"); @@ -183,16 +183,16 @@ class Feature_test : public beast::unit_test::suite using namespace test::jtx; Env env{*this}; - auto jrr = env.rpc("feature", "Flow")[jss::result]; + auto jrr = env.rpc("feature", "fixAMMOverflowOffer")[jss::result]; BEAST_EXPECTS(jrr[jss::status] == jss::success, "status"); jrr.removeMember(jss::status); BEAST_EXPECT(jrr.size() == 1); BEAST_EXPECT( - jrr.isMember("740352F2412A9909880C23A559FCECEDA3BE2126FED62FC7660D6" - "28A06927F11")); + jrr.isMember("12523DF04B553A0B1AD74F42DDB741DE8DC06A03FC089A0EF197E" + "2A87F1D8107")); auto feature = *(jrr.begin()); - BEAST_EXPECTS(feature[jss::name] == "Flow", "name"); + BEAST_EXPECTS(feature[jss::name] == "fixAMMOverflowOffer", "name"); BEAST_EXPECTS(!feature[jss::enabled].asBool(), "enabled"); BEAST_EXPECTS( feature[jss::vetoed].isBool() && !feature[jss::vetoed].asBool(), @@ -200,7 +200,7 @@ class Feature_test : public beast::unit_test::suite BEAST_EXPECTS(feature[jss::supported].asBool(), "supported"); // feature names are case-sensitive - expect error here - jrr = env.rpc("feature", "flow")[jss::result]; + jrr = env.rpc("feature", "fMM")[jss::result]; BEAST_EXPECT(jrr[jss::error] == "badFeature"); BEAST_EXPECT(jrr[jss::error_message] == "Feature unknown or invalid."); } @@ -419,9 +419,9 @@ class Feature_test : public beast::unit_test::suite break; } - // There should be at least 3 amendments. Don't do exact comparison + // There should be at least 2 amendments. Don't do exact comparison // to avoid maintenance as more amendments are added in the future. - BEAST_EXPECT(majorities.size() >= 3); + BEAST_EXPECT(majorities.size() >= 2); std::map const& votes = ripple::detail::supportedAmendments(); @@ -476,8 +476,8 @@ class Feature_test : public beast::unit_test::suite testcase("Veto"); using namespace test::jtx; - Env env{*this, FeatureBitset{featureFlow}}; - constexpr char const* featureName = "Flow"; + Env env{*this, FeatureBitset{featurePriceOracle}}; + constexpr char const* featureName = "fixAMMOverflowOffer"; auto jrr = env.rpc("feature", featureName)[jss::result]; if (!BEAST_EXPECTS(jrr[jss::status] == jss::success, "status")) diff --git a/src/xrpld/app/paths/RippleCalc.cpp b/src/xrpld/app/paths/RippleCalc.cpp index ac30932f11..a6751169d7 100644 --- a/src/xrpld/app/paths/RippleCalc.cpp +++ b/src/xrpld/app/paths/RippleCalc.cpp @@ -43,15 +43,6 @@ RippleCalc::rippleCalculate( PaymentSandbox flowSB(&view); auto j = l.journal("Flow"); - if (!view.rules().enabled(featureFlow)) - { - // The new payment engine was enabled several years ago. New transaction - // should never use the old rules. Assume this is a replay - j.fatal() - << "Old payment rules are required for this transaction. Assuming " - "this is a replay and running with the new rules."; - } - { bool const defaultPaths = !pInputs ? true : pInputs->defaultPathsAllowed; diff --git a/src/xrpld/app/paths/detail/StrandFlow.h b/src/xrpld/app/paths/detail/StrandFlow.h index c9a4619717..fb93f3fdb7 100644 --- a/src/xrpld/app/paths/detail/StrandFlow.h +++ b/src/xrpld/app/paths/detail/StrandFlow.h @@ -433,7 +433,7 @@ public: // add the strands in `next_` to `cur_`, sorted by theoretical quality. // Best quality first. cur_.clear(); - if (v.rules().enabled(featureFlowSortStrands) && !next_.empty()) + if (!next_.empty()) { std::vector> strandQuals; strandQuals.reserve(next_.size()); @@ -719,46 +719,16 @@ flow( continue; } - if (baseView.rules().enabled(featureFlowSortStrands)) - { - XRPL_ASSERT(!best, "ripple::flow : best is unset"); - if (!f.inactive) - activeStrands.push(strand); - best.emplace(f.in, f.out, std::move(*f.sandbox), *strand, q); - activeStrands.pushRemainingCurToNext(strandIndex + 1); - break; - } - - activeStrands.push(strand); - - if (!best || best->quality < q || - (best->quality == q && best->out < f.out)) - { - // If this strand is inactive (because it consumed too many - // offers) and ends up having the best quality, remove it - // from the activeStrands. If it doesn't end up having the - // best quality, keep it active. - - if (f.inactive) - { - // This should be `nextSize`, not `size`. This issue is - // fixed in featureFlowSortStrands. - markInactiveOnUse = activeStrands.size() - 1; - } - else - { - markInactiveOnUse.reset(); - } - - best.emplace(f.in, f.out, std::move(*f.sandbox), *strand, q); - } + XRPL_ASSERT(!best, "ripple::flow : best is unset"); + if (!f.inactive) + activeStrands.push(strand); + best.emplace(f.in, f.out, std::move(*f.sandbox), *strand, q); + activeStrands.pushRemainingCurToNext(strandIndex + 1); + break; } - bool const shouldBreak = [&] { - if (baseView.rules().enabled(featureFlowSortStrands)) - return !best || offersConsidered >= maxOffersToConsider; - return !best; - }(); + bool const shouldBreak = + !best || offersConsidered >= maxOffersToConsider; if (best) {