From fc0a0827007573a9bba28e2f0e9ecc4203ba0ee6 Mon Sep 17 00:00:00 2001 From: Scott Schurr Date: Mon, 9 Dec 2019 12:33:54 -0800 Subject: [PATCH] Remove STAmountSO::soTime and soTime2: STAmount::soTime and soTime2 were time based "amendment like" switches to control small changes in behavior for STAmount. soTime2, which was the most recent, was dated Feb 27, 2016. That's over 3 years ago. The main reason to retain these soTimes would be to replay old transactions. The likelihood of needing to replay a transaction from over three years ago is pretty low. So it makes sense to remove these soTime values. In Flow_test the testZeroOutputStep() test is removed. That test started to fail when the STAmount::soTimes were removed. I checked with the original author of the test. He said that the code being tested by that unit test has been removed, so it makes sense to remove the test. That test is removed. --- src/ripple/app/misc/impl/TxQ.cpp | 16 +-- .../app/paths/cursor/DeliverNodeReverse.cpp | 8 -- src/ripple/app/tx/impl/apply.cpp | 1 - src/ripple/protocol/STAmount.h | 35 ------ src/ripple/protocol/impl/STAmount.cpp | 21 +--- src/test/app/Flow_test.cpp | 40 ------ src/test/app/Offer_test.cpp | 116 ++++++------------ 7 files changed, 40 insertions(+), 197 deletions(-) diff --git a/src/ripple/app/misc/impl/TxQ.cpp b/src/ripple/app/misc/impl/TxQ.cpp index 59b604b43a..c9421dec0c 100644 --- a/src/ripple/app/misc/impl/TxQ.cpp +++ b/src/ripple/app/misc/impl/TxQ.cpp @@ -278,9 +278,6 @@ TxQ::MaybeTx::MaybeTx( std::pair TxQ::MaybeTx::apply(Application& app, OpenView& view, beast::Journal j) { - boost::optional saved; - if (view.rules().enabled(fix1513)) - saved.emplace(view.info().parentCloseTime); // If the rules or flags change, preflight again assert(pfresult); if (pfresult->rules != view.rules() || @@ -539,13 +536,7 @@ TxQ::tryClearAccountQueue(Application& app, OpenView& view, } // Apply the current tx. Because the state of the view has been changed // by the queued txs, we also need to preclaim again. - auto txResult = [&]{ - boost::optional saved; - if (view.rules().enabled(fix1513)) - saved.emplace(view.info().parentCloseTime); - auto const pcresult = preclaim(pfresult, app, view); - return doApply(pcresult, app, view); - }(); + auto const txResult = doApply (preclaim (pfresult, app, view), app, view); if (txResult.second) { @@ -630,11 +621,6 @@ TxQ::apply(Application& app, OpenView& view, auto const account = (*tx)[sfAccount]; auto const transactionID = tx->getTransactionID(); auto const tSeq = tx->getSequence(); - - boost::optional saved; - if (view.rules().enabled(fix1513)) - saved.emplace(view.info().parentCloseTime); - // See if the transaction is valid, properly formed, // etc. before doing potentially expensive queue // replace and multi-transaction operations. diff --git a/src/ripple/app/paths/cursor/DeliverNodeReverse.cpp b/src/ripple/app/paths/cursor/DeliverNodeReverse.cpp index 25bcea1636..ac8e7a5373 100644 --- a/src/ripple/app/paths/cursor/DeliverNodeReverse.cpp +++ b/src/ripple/app/paths/cursor/DeliverNodeReverse.cpp @@ -185,14 +185,6 @@ TER PathCursor::deliverNodeReverseImpl ( // Compute portion of input needed to cover actual output. auto outputFee = mulRound ( saOutPassAct, node().saOfrRate, node().saTakerPays.issue (), true); - if (*stAmountCalcSwitchover == false && ! outputFee) - { - JLOG (j_.fatal()) - << "underflow computing outputFee " - << "saOutPassAct: " << saOutPassAct - << " saOfrRate: " << node ().saOfrRate; - return telFAILED_PROCESSING; - } STAmount saInPassReq = std::min (node().saTakerPays, outputFee); STAmount saInPassAct; diff --git a/src/ripple/app/tx/impl/apply.cpp b/src/ripple/app/tx/impl/apply.cpp index 5f0d6c6a88..169adc03f8 100644 --- a/src/ripple/app/tx/impl/apply.cpp +++ b/src/ripple/app/tx/impl/apply.cpp @@ -104,7 +104,6 @@ apply (Application& app, OpenView& view, STTx const& tx, ApplyFlags flags, beast::Journal j) { - STAmountSO saved(view.info().parentCloseTime); auto pfresult = preflight(app, view.rules(), tx, flags, j); auto pcresult = preclaim(pfresult, app, view); return doApply(pcresult, app, view); diff --git a/src/ripple/protocol/STAmount.h b/src/ripple/protocol/STAmount.h index 95ee41bd7d..841961bb4a 100644 --- a/src/ripple/protocol/STAmount.h +++ b/src/ripple/protocol/STAmount.h @@ -20,15 +20,12 @@ #ifndef RIPPLE_PROTOCOL_STAMOUNT_H_INCLUDED #define RIPPLE_PROTOCOL_STAMOUNT_H_INCLUDED -#include #include -#include #include #include #include #include #include -#include namespace ripple { @@ -406,38 +403,6 @@ inline bool isXRP(STAmount const& amount) return isXRP (amount.issue().currency); } -extern LocalValue stAmountCalcSwitchover; -extern LocalValue stAmountCalcSwitchover2; - -/** RAII class to set and restore the STAmount calc switchover.*/ -class STAmountSO -{ -public: - explicit STAmountSO(NetClock::time_point const closeTime) - : saved_(*stAmountCalcSwitchover) - , saved2_(*stAmountCalcSwitchover2) - { - *stAmountCalcSwitchover = closeTime > soTime; - *stAmountCalcSwitchover2 = closeTime > soTime2; - } - - ~STAmountSO() - { - *stAmountCalcSwitchover = saved_; - *stAmountCalcSwitchover2 = saved2_; - } - - // Mon Dec 28, 2015 18:00:00 UTC - static NetClock::time_point const soTime; - - // Sat Feb 27, 2016 05:00:00 UTC - static NetClock::time_point const soTime2; - -private: - bool saved_; - bool saved2_; -}; - } // ripple #endif diff --git a/src/ripple/protocol/impl/STAmount.cpp b/src/ripple/protocol/impl/STAmount.cpp index 9b653ff009..ee804fac9a 100644 --- a/src/ripple/protocol/impl/STAmount.cpp +++ b/src/ripple/protocol/impl/STAmount.cpp @@ -35,17 +35,6 @@ namespace ripple { -LocalValue stAmountCalcSwitchover(true); -LocalValue stAmountCalcSwitchover2(true); - -using namespace std::chrono_literals; - -// Mon Dec 28, 2015 18:00:00 UTC -const NetClock::time_point STAmountSO::soTime{504640800s}; - -// Sat Feb 27, 2016 05:00:00 UTC -const NetClock::time_point STAmountSO::soTime2{509864400s}; - static const std::uint64_t tenTo14 = 100000000000000ull; static const std::uint64_t tenTo14m1 = tenTo14 - 1; static const std::uint64_t tenTo17 = tenTo14 * 1000; @@ -1246,10 +1235,9 @@ mulRound (STAmount const& v1, STAmount const& v2, Issue const& issue, canonicalizeRound (xrp, amount, offset); STAmount result (issue, amount, offset, resultNegative); - // Control when bugfixes that require switchover dates are enabled - if (roundUp && !resultNegative && !result && *stAmountCalcSwitchover) + if (roundUp && !resultNegative && !result) { - if (xrp && *stAmountCalcSwitchover2) + if (xrp) { // return the smallest value above zero amount = 1; @@ -1318,10 +1306,9 @@ divRound (STAmount const& num, STAmount const& den, canonicalizeRound (isXRP (issue), amount, offset); STAmount result (issue, amount, offset, resultNegative); - // Control when bugfixes that require switchover dates are enabled - if (roundUp && !resultNegative && !result && *stAmountCalcSwitchover) + if (roundUp && !resultNegative && !result) { - if (isXRP(issue) && *stAmountCalcSwitchover2) + if (isXRP(issue)) { // return the smallest value above zero amount = 1; diff --git a/src/test/app/Flow_test.cpp b/src/test/app/Flow_test.cpp index 656f5a56cd..67176363e1 100644 --- a/src/test/app/Flow_test.cpp +++ b/src/test/app/Flow_test.cpp @@ -492,11 +492,6 @@ struct Flow_test : public beast::unit_test::suite // pass. This test checks that the payment produces 1 EUR, as expected. Env env (*this, features); - - auto const closeTime = STAmountSO::soTime2 + - 100 * env.closed ()->info ().closeTimeResolution; - env.close (closeTime); - env.fund (XRP (10000), alice, bob, carol, gw); env.trust (USD (1000), alice, bob, carol); env.trust (EUR (1000), alice, bob, carol); @@ -1249,40 +1244,6 @@ struct Flow_test : public beast::unit_test::suite ter(temBAD_PATH)); } - void - testZeroOutputStep() - { - testcase("Zero Output Step"); - - using namespace jtx; - 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 const features = supported_amendments(); - Env env(*this, features); - env.fund(XRP(10000), alice, bob, carol, gw); - env.trust(USD(1000), alice, bob, carol); - env.trust(EUR(1000), alice, bob, carol); - env(pay(gw, alice, USD(100))); - env(pay(gw, bob, USD(100))); - env(pay(gw, bob, EUR(100))); - env.close(); - - env(offer(bob, USD(100), EUR(100))); - env(offer(bob, EUR(100), XRP(0.000001))); - env.close(); - - env(pay(alice, carol, XRP(1)), - path(~EUR, ~XRP), - sendmax(USD(1)), - txflags(tfPartialPayment), - ter(tecPATH_DRY)); - } - void testWithFeats(FeatureBitset features) { using namespace jtx; @@ -1311,7 +1272,6 @@ struct Flow_test : public beast::unit_test::suite void run() override { testLimitQuality(); - testZeroOutputStep(); testRIPD1443(true); testRIPD1443(false); testRIPD1449(true); diff --git a/src/test/app/Offer_test.cpp b/src/test/app/Offer_test.cpp index 4435aed1a8..2a219b9edc 100644 --- a/src/test/app/Offer_test.cpp +++ b/src/test/app/Offer_test.cpp @@ -274,25 +274,7 @@ public: for (int i=0;i<101;++i) env (offer (carol, USD (1), EUR (2))); - auto hasFeature = [](Env& e, uint256 const& f) - { - return (e.app().config().features.find(f) != - e.app().config().features.end()); - }; - - for (auto d : {-1, 1}) - { - auto const closeTime = STAmountSO::soTime + - d * env.closed()->info().closeTimeResolution; - env.close (closeTime); - *stAmountCalcSwitchover = closeTime > STAmountSO::soTime || - !hasFeature(env, fix1513); - // Will fail without the underflow fix - TER const expectedResult = *stAmountCalcSwitchover ? - TER {tesSUCCESS} : TER {tecPATH_PARTIAL}; - env (pay (alice, bob, EUR (epsilon)), path (~EUR), - sendmax (USD (100)), ter (expectedResult)); - } + env (pay (alice, bob, EUR (epsilon)), path (~EUR), sendmax (USD (100))); } void testXRPTinyPayment (FeatureBitset features) @@ -324,74 +306,46 @@ public: auto const gw = Account {"gw"}; auto const USD = gw["USD"]; + Env env {*this, features}; - for (auto withFix : {false, true}) - { - if (!withFix) - continue; + env.fund (XRP (10000), alice, bob, carol, dan, erin, gw); + env.trust (USD (1000), alice, bob, carol, dan, erin); + env (pay (gw, carol, USD (0.99999))); + env (pay (gw, dan, USD (1))); + env (pay (gw, erin, USD (1))); - Env env {*this, features}; + // 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))); + // 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))); - auto closeTime = [&] - { - auto const delta = - 100 * env.closed ()->info ().closeTimeResolution; - if (withFix) - return STAmountSO::soTime2 + delta; - else - return STAmountSO::soTime2 - delta; - }(); + env.close (); + // This is the funded offer that will be incorrectly removed. + // 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 (1), USD (1))); - env.fund (XRP (10000), alice, bob, carol, dan, erin, gw); - env.trust (USD (1000), alice, bob, carol, dan, erin); - env (pay (gw, carol, USD (0.99999))); - env (pay (gw, dan, USD (1))); - env (pay (gw, erin, USD (1))); + env (pay (alice, bob, USD (1)), path (~USD), + sendmax (XRP (102)), + txflags (tfNoRippleDirect | tfPartialPayment)); - // 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))); - // 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))); + env.require ( + offers (carol, 0), + offers (dan, 1)); - env.close (closeTime); - // This is the funded offer that will be incorrectly removed. - // 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 (1), USD (1))); - - { - env (pay (alice, bob, USD (1)), path (~USD), - sendmax (XRP (102)), - txflags (tfNoRippleDirect | tfPartialPayment)); - - env.require ( - offers (carol, 0), - offers (dan, 1)); - if (!withFix) - { - // funded offer was removed - env.require ( - balance (erin, USD (1)), - offers (erin, 0)); - } - else - { - // offer was correctly consumed. There is still some - // liquidity left on that offer. - env.require ( - balance (erin, USD (0.99999)), - offers (erin, 1)); - } - } - } + // offer was correctly consumed. There is still some + // liquidity left on that offer. + env.require ( + balance (erin, USD (0.99999)), + offers (erin, 1)); } void testEnforceNoRipple (FeatureBitset features)