From ae9930b87dd9ca3c3ca7ee73d46b261f67803bb3 Mon Sep 17 00:00:00 2001 From: Edward Hennis Date: Tue, 7 Dec 2021 17:14:55 -0500 Subject: [PATCH] Consensus transaction recovery/deferral completely ignores the TxQ --- src/ripple/app/ledger/OpenLedger.h | 15 +------ src/ripple/app/ledger/impl/OpenLedger.cpp | 33 ++-------------- src/ripple/app/main/Application.cpp | 3 +- src/ripple/app/misc/HashRouter.cpp | 10 ----- src/ripple/app/misc/HashRouter.h | 41 +------------------ src/ripple/app/misc/impl/TxQ.cpp | 14 +------ src/ripple/ledger/ApplyView.h | 13 ++---- src/test/app/HashRouter_test.cpp | 48 +++-------------------- 8 files changed, 20 insertions(+), 157 deletions(-) diff --git a/src/ripple/app/ledger/OpenLedger.h b/src/ripple/app/ledger/OpenLedger.h index 42120cf63..e3cb2b100 100644 --- a/src/ripple/app/ledger/OpenLedger.h +++ b/src/ripple/app/ledger/OpenLedger.h @@ -185,7 +185,6 @@ private: FwdRange const& txs, OrderedTxs& retries, ApplyFlags flags, - std::map& shouldRecover, beast::Journal j); enum Result { success, failure, retry }; @@ -200,7 +199,6 @@ private: std::shared_ptr const& tx, bool retry, ApplyFlags flags, - bool shouldRecover, beast::Journal j); }; @@ -215,7 +213,6 @@ OpenLedger::apply( FwdRange const& txs, OrderedTxs& retries, ApplyFlags flags, - std::map& shouldRecover, beast::Journal j) { for (auto iter = txs.begin(); iter != txs.end(); ++iter) @@ -227,8 +224,7 @@ OpenLedger::apply( auto const txId = tx->getTransactionID(); if (check.txExists(txId)) continue; - auto const result = - apply_one(app, view, tx, true, flags, shouldRecover[txId], j); + auto const result = apply_one(app, view, tx, true, flags, j); if (result == Result::retry) retries.insert(tx); } @@ -245,14 +241,7 @@ OpenLedger::apply( auto iter = retries.begin(); while (iter != retries.end()) { - switch (apply_one( - app, - view, - iter->second, - retry, - flags, - shouldRecover[iter->second->getTransactionID()], - j)) + switch (apply_one(app, view, iter->second, retry, flags, j)) { case Result::success: ++changes; diff --git a/src/ripple/app/ledger/impl/OpenLedger.cpp b/src/ripple/app/ledger/impl/OpenLedger.cpp index 113c46951..5bb6e53ac 100644 --- a/src/ripple/app/ledger/impl/OpenLedger.cpp +++ b/src/ripple/app/ledger/impl/OpenLedger.cpp @@ -81,17 +81,11 @@ OpenLedger::accept( { JLOG(j_.trace()) << "accept ledger " << ledger->seq() << " " << suffix; auto next = create(rules, ledger); - std::map shouldRecover; if (retriesFirst) { - for (auto const& tx : retries) - { - auto const txID = tx.second->getTransactionID(); - shouldRecover[txID] = app.getHashRouter().shouldRecover(txID); - } // Handle disputed tx, outside lock using empty = std::vector>; - apply(app, *next, *ledger, empty{}, retries, flags, shouldRecover, j_); + apply(app, *next, *ledger, empty{}, retries, flags, j_); } // Block calls to modify, otherwise // new tx going into the open ledger @@ -100,17 +94,6 @@ OpenLedger::accept( // Apply tx from the current open view if (!current_->txs.empty()) { - for (auto const& tx : current_->txs) - { - auto const txID = tx.first->getTransactionID(); - auto iter = shouldRecover.lower_bound(txID); - if (iter != shouldRecover.end() && iter->first == txID) - // already had a chance via disputes - iter->second = false; - else - shouldRecover.emplace_hint( - iter, txID, app.getHashRouter().shouldRecover(txID)); - } apply( app, *next, @@ -124,7 +107,6 @@ OpenLedger::accept( }), retries, flags, - shouldRecover, j_); } // Call the modifier @@ -179,21 +161,12 @@ OpenLedger::apply_one( std::shared_ptr const& tx, bool retry, ApplyFlags flags, - bool shouldRecover, beast::Journal j) -> Result { if (retry) flags = flags | tapRETRY; - auto const result = [&] { - auto const queueResult = - app.getTxQ().apply(app, view, tx, flags | tapPREFER_QUEUE, j); - // If the transaction can't get into the queue for intrinsic - // reasons, and it can still be recovered, try to put it - // directly into the open ledger, else drop it. - if (queueResult.first == telCAN_NOT_QUEUE && shouldRecover) - return ripple::apply(app, view, *tx, flags, j); - return queueResult; - }(); + // If it's in anybody's proposed set, try to keep it in the ledger + auto const result = ripple::apply(app, view, *tx, flags, j); if (result.second || result.first == terQUEUED) return Result::success; if (isTefFailure(result.first) || isTemMalformed(result.first) || diff --git a/src/ripple/app/main/Application.cpp b/src/ripple/app/main/Application.cpp index 5095aafb5..60381b158 100644 --- a/src/ripple/app/main/Application.cpp +++ b/src/ripple/app/main/Application.cpp @@ -448,8 +448,7 @@ public: , hashRouter_(std::make_unique( stopwatch(), - HashRouter::getDefaultHoldTime(), - HashRouter::getDefaultRecoverLimit())) + HashRouter::getDefaultHoldTime())) , mValidations( ValidationParms(), diff --git a/src/ripple/app/misc/HashRouter.cpp b/src/ripple/app/misc/HashRouter.cpp index 8a8170f48..8085d6892 100644 --- a/src/ripple/app/misc/HashRouter.cpp +++ b/src/ripple/app/misc/HashRouter.cpp @@ -128,14 +128,4 @@ HashRouter::shouldRelay(uint256 const& key) return s.releasePeerSet(); } -bool -HashRouter::shouldRecover(uint256 const& key) -{ - std::lock_guard lock(mutex_); - - auto& s = emplace(key).first; - - return s.shouldRecover(recoverLimit_); -} - } // namespace ripple diff --git a/src/ripple/app/misc/HashRouter.h b/src/ripple/app/misc/HashRouter.h index a43bc1278..8c546b2c5 100644 --- a/src/ripple/app/misc/HashRouter.h +++ b/src/ripple/app/misc/HashRouter.h @@ -116,20 +116,6 @@ private: return true; } - /** Determines if this item should be recovered from the open ledger. - - Counts the number of times the item has been recovered. - Every `limit` times the function is called, return false. - Else return true. - - @note The limit must be > 0 - */ - bool - shouldRecover(std::uint32_t limit) - { - return ++recoveries_ % limit != 0; - } - bool shouldProcess(Stopwatch::time_point now, std::chrono::seconds interval) { @@ -146,7 +132,6 @@ private: // than one flag needs to expire independently. std::optional relayed_; std::optional processed_; - std::uint32_t recoveries_ = 0; }; public: @@ -158,19 +143,8 @@ public: return 300s; } - static inline std::uint32_t - getDefaultRecoverLimit() - { - return 1; - } - - HashRouter( - Stopwatch& clock, - std::chrono::seconds entryHoldTimeInSeconds, - std::uint32_t recoverLimit) - : suppressionMap_(clock) - , holdTime_(entryHoldTimeInSeconds) - , recoverLimit_(recoverLimit + 1u) + HashRouter(Stopwatch& clock, std::chrono::seconds entryHoldTimeInSeconds) + : suppressionMap_(clock), holdTime_(entryHoldTimeInSeconds) { } @@ -231,15 +205,6 @@ public: std::optional> shouldRelay(uint256 const& key); - /** Determines whether the hashed item should be recovered - from the open ledger into the next open ledger or the transaction - queue. - - @return `bool` indicates whether the item should be recovered - */ - bool - shouldRecover(uint256 const& key); - private: // pair.second indicates whether the entry was created std::pair @@ -256,8 +221,6 @@ private: suppressionMap_; std::chrono::seconds const holdTime_; - - std::uint32_t const recoverLimit_; }; } // namespace ripple diff --git a/src/ripple/app/misc/impl/TxQ.cpp b/src/ripple/app/misc/impl/TxQ.cpp index 40d6aced3..1315bb6e2 100644 --- a/src/ripple/app/misc/impl/TxQ.cpp +++ b/src/ripple/app/misc/impl/TxQ.cpp @@ -1176,8 +1176,7 @@ TxQ::apply( for some other reason. Tx is allowed to queue in case conditions change, but don't waste the effort to clear). */ - if (!(flags & tapPREFER_QUEUE) && txSeqProx.isSeq() && txIter && - multiTxn.has_value() && + if (txSeqProx.isSeq() && txIter && multiTxn.has_value() && txIter->first->second.retriesRemaining == MaybeTx::retriesAllowed && feeLevelPaid > requiredFeeLevel && requiredFeeLevel > baseLevel) { @@ -1307,9 +1306,6 @@ TxQ::apply( // Don't allow soft failures, which can lead to retries flags &= ~tapRETRY; - // Don't queue because we're already in the queue - flags &= ~tapPREFER_QUEUE; - auto& candidate = accountIter->second.add( {tx, transactionID, feeLevelPaid, flags, pfresult}); @@ -1612,13 +1608,7 @@ TxQ::getRequiredFeeLevel( FeeMetrics::Snapshot const& metricsSnapshot, std::lock_guard const& lock) const { - FeeLevel64 const feeLevel = - FeeMetrics::scaleFeeLevel(metricsSnapshot, view); - - if ((flags & tapPREFER_QUEUE) && !byFee_.empty()) - return std::max(feeLevel, byFee_.begin()->feeLevel); - - return feeLevel; + return FeeMetrics::scaleFeeLevel(metricsSnapshot, view); } std::optional> diff --git a/src/ripple/ledger/ApplyView.h b/src/ripple/ledger/ApplyView.h index b99a22ae1..5394acc78 100644 --- a/src/ripple/ledger/ApplyView.h +++ b/src/ripple/ledger/ApplyView.h @@ -37,11 +37,6 @@ enum ApplyFlags : std::uint32_t { // Transaction can be retried, soft failures allowed tapRETRY = 0x20, - // Transaction must pay more than both the open ledger - // fee and all transactions in the queue to get into the - // open ledger - tapPREFER_QUEUE = 0x40, - // Transaction came from a privileged source tapUNLIMITED = 0x400, }; @@ -55,10 +50,10 @@ operator|(ApplyFlags const& lhs, ApplyFlags const& rhs) } static_assert( - (tapPREFER_QUEUE | tapRETRY) == safe_cast(0x60u), + (tapFAIL_HARD | tapRETRY) == safe_cast(0x30u), "ApplyFlags operator |"); static_assert( - (tapRETRY | tapPREFER_QUEUE) == safe_cast(0x60u), + (tapRETRY | tapFAIL_HARD) == safe_cast(0x30u), "ApplyFlags operator |"); constexpr ApplyFlags @@ -69,8 +64,8 @@ operator&(ApplyFlags const& lhs, ApplyFlags const& rhs) safe_cast>(rhs)); } -static_assert((tapPREFER_QUEUE & tapRETRY) == tapNONE, "ApplyFlags operator &"); -static_assert((tapRETRY & tapPREFER_QUEUE) == tapNONE, "ApplyFlags operator &"); +static_assert((tapFAIL_HARD & tapRETRY) == tapNONE, "ApplyFlags operator &"); +static_assert((tapRETRY & tapFAIL_HARD) == tapNONE, "ApplyFlags operator &"); constexpr ApplyFlags operator~(ApplyFlags const& flags) diff --git a/src/test/app/HashRouter_test.cpp b/src/test/app/HashRouter_test.cpp index 9162fb9b1..96d14e824 100644 --- a/src/test/app/HashRouter_test.cpp +++ b/src/test/app/HashRouter_test.cpp @@ -31,7 +31,7 @@ class HashRouter_test : public beast::unit_test::suite { using namespace std::chrono_literals; TestStopwatch stopwatch; - HashRouter router(stopwatch, 2s, 2); + HashRouter router(stopwatch, 2s); uint256 const key1(1); uint256 const key2(2); @@ -68,7 +68,7 @@ class HashRouter_test : public beast::unit_test::suite { using namespace std::chrono_literals; TestStopwatch stopwatch; - HashRouter router(stopwatch, 2s, 2); + HashRouter router(stopwatch, 2s); uint256 const key1(1); uint256 const key2(2); @@ -146,7 +146,7 @@ class HashRouter_test : public beast::unit_test::suite // Normal HashRouter using namespace std::chrono_literals; TestStopwatch stopwatch; - HashRouter router(stopwatch, 2s, 2); + HashRouter router(stopwatch, 2s); uint256 const key1(1); uint256 const key2(2); @@ -174,7 +174,7 @@ class HashRouter_test : public beast::unit_test::suite { using namespace std::chrono_literals; TestStopwatch stopwatch; - HashRouter router(stopwatch, 2s, 2); + HashRouter router(stopwatch, 2s); uint256 const key1(1); BEAST_EXPECT(router.setFlags(key1, 10)); @@ -187,7 +187,7 @@ class HashRouter_test : public beast::unit_test::suite { using namespace std::chrono_literals; TestStopwatch stopwatch; - HashRouter router(stopwatch, 1s, 2); + HashRouter router(stopwatch, 1s); uint256 const key1(1); @@ -225,47 +225,12 @@ class HashRouter_test : public beast::unit_test::suite BEAST_EXPECT(peers && peers->size() == 0); } - void - testRecover() - { - using namespace std::chrono_literals; - TestStopwatch stopwatch; - HashRouter router(stopwatch, 1s, 5); - - uint256 const key1(1); - - BEAST_EXPECT(router.shouldRecover(key1)); - BEAST_EXPECT(router.shouldRecover(key1)); - BEAST_EXPECT(router.shouldRecover(key1)); - BEAST_EXPECT(router.shouldRecover(key1)); - BEAST_EXPECT(router.shouldRecover(key1)); - BEAST_EXPECT(!router.shouldRecover(key1)); - // Expire, but since the next search will - // be for this entry, it will get refreshed - // instead. - ++stopwatch; - BEAST_EXPECT(router.shouldRecover(key1)); - // Expire, but since the next search will - // be for this entry, it will get refreshed - // instead. - ++stopwatch; - // Recover again. Recovery is independent of - // time as long as the entry doesn't expire. - BEAST_EXPECT(router.shouldRecover(key1)); - BEAST_EXPECT(router.shouldRecover(key1)); - BEAST_EXPECT(router.shouldRecover(key1)); - // Expire again - ++stopwatch; - BEAST_EXPECT(router.shouldRecover(key1)); - BEAST_EXPECT(!router.shouldRecover(key1)); - } - void testProcess() { using namespace std::chrono_literals; TestStopwatch stopwatch; - HashRouter router(stopwatch, 5s, 5); + HashRouter router(stopwatch, 5s); uint256 const key(1); HashRouter::PeerShortID peer = 1; int flags; @@ -286,7 +251,6 @@ public: testSuppression(); testSetFlags(); testRelay(); - testRecover(); testProcess(); } };